Re: [PATCH v1 06/10] mci: atmel-sdhci: add Microchip LAN9691 / LAN969X SDHCI support

On 26-06-12, Oleksij Rempel wrote:
> Match the "microchip,lan9691-sdhci" compatible and allow building on
> ARCH_MICROCHIP (and COMPILE_TEST). LAN969X uses a different GCK rate
> (100 MHz) than the AT91 SAMA5D2 (240 MHz), so plumb the per-compatible
> rate through device_get_match_data() and fall back to the SAMA5D2 rate
> when no match data is supplied.
> 
> The LAN969X SDHCI binding doesn't expose the PMC base clock that
> sama5d2/sam9x60 carry as "baseclk" (the GCK driver handles the upstream
> source internally), so make that lookup optional.
> 
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
>  drivers/mci/Kconfig       |  6 +++---
>  drivers/mci/atmel-sdhci.c | 22 ++++++++++++++++++----
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
> index b38f7a3bdf8b..8108c7b14848 100644
> --- a/drivers/mci/Kconfig
> +++ b/drivers/mci/Kconfig
> @@ -197,12 +197,12 @@ config MCI_ATMEL
>  	  Atmel AT91.
>  
>  config MCI_ATMEL_SDHCI
> -	bool "ATMEL SDHCI (sama5d2)"
> +	bool "ATMEL SDHCI (sama5d2, lan9691)"
>  	select MCI_SDHCI
> -	depends on ARCH_AT91
> +	depends on ARCH_AT91 || ARCH_MICROCHIP || COMPILE_TEST
>  	help
>  	  Enable this entry to add support to read and write SD cards on an
> -	  Atmel sama5d2
> +	  Atmel sama5d2 or Microchip LAN969X.
>  
>  config MCI_MMCI
>  	bool "ARM PL180 MMCI"
> diff --git a/drivers/mci/atmel-sdhci.c b/drivers/mci/atmel-sdhci.c
> index 462cf21bc25f..458d022fa3fa 100644
> --- a/drivers/mci/atmel-sdhci.c
> +++ b/drivers/mci/atmel-sdhci.c
> @@ -18,6 +18,7 @@
>  
>  #define ATMEL_SDHC_MIN_FREQ	400000
>  #define ATMEL_SDHC_GCK_RATE	240000000
> +#define LAN969X_GCK_RATE	100000000
>  
>  struct at91_sdhci_priv {
>  	struct at91_sdhci host;
> @@ -55,7 +56,8 @@ static int at91_sdhci_mci_init(struct mci_host *mci, struct device *dev)
>  			       priv->mci.non_removable, priv->cal_always_on);
>  }
>  
> -static int at91_sdhci_conf_clks(struct at91_sdhci_priv *priv)
> +static int at91_sdhci_conf_clks(struct at91_sdhci_priv *priv,
> +				unsigned long gck_rate)
>  {
>  	unsigned long real_gck_rate;
>  	int ret;
> @@ -66,7 +68,7 @@ static int at91_sdhci_conf_clks(struct at91_sdhci_priv *priv)
>  	 * base clock rate and the clock mult from capabilities.
>  	 */
>  	clk_enable(priv->hclock);
> -	ret = clk_set_rate(priv->gck, ATMEL_SDHC_GCK_RATE);
> +	ret = clk_set_rate(priv->gck, gck_rate);
>  	if (ret < 0) {
>  		clk_disable(priv->hclock);
>  		return ret;
> @@ -109,6 +111,11 @@ static int at91_sdhci_probe(struct device *dev)
>  {
>  	struct at91_sdhci_priv *priv;
>  	struct resource *iores;
> +	unsigned long gck_rate;
> +
> +	gck_rate = (uintptr_t)device_get_match_data(dev);
> +	if (!gck_rate)
> +		gck_rate = ATMEL_SDHC_GCK_RATE;

Could be set as of_device_data as well to avoid this default assingment.

>  	priv = xzalloc(sizeof(*priv));
>  	dev->priv = priv;
> @@ -119,7 +126,12 @@ static int at91_sdhci_probe(struct device *dev)
>  		return PTR_ERR(iores);
>  	}
>  
> -	priv->mainck = clk_get(dev, "baseclk");
> +	/*
> +	 * "baseclk" is the PMC base clock on sama5d2/sam9x60. The LAN969X
> +	 * SDHCI binding doesn't expose it (the GCK driver handles the upstream
> +	 * source internally), so treat it as optional.
> +	 */
> +	priv->mainck = clk_get_optional(dev, "baseclk");

This commit may introduce bugs in case the clock is not found and you're
running on a SoC which requires it. This is no longer covered.

Please see Linux commit 3976656d67c1 ("mmc: sdhci-of-at91: rework clocks
management to support SAM9x60 device"). This commits introduces a proper
driver struct and the optional internal clock handling.

Regards,
  Marco

>  	if (IS_ERR(priv->mainck)) {
>  		dev_err(dev, "failed to get baseclk\n");
>  		return PTR_ERR(priv->mainck);
> @@ -147,7 +159,7 @@ static int at91_sdhci_probe(struct device *dev)
>  	at91_sdhci_mmio_init(&priv->host, IOMEM(iores->start));
>  	priv->host.sdhci.mci = &priv->mci;
>  
> -	priv->gck_rate = at91_sdhci_conf_clks(priv);
> +	priv->gck_rate = at91_sdhci_conf_clks(priv, gck_rate);
>  	if (priv->gck_rate < 0)
>  		return priv->gck_rate;
>  
> @@ -164,6 +176,8 @@ static int at91_sdhci_probe(struct device *dev)
>  static const struct of_device_id at91_sdhci_dt_match[] = {
>  	{ .compatible = "atmel,sama5d2-sdhci"  },
>  	{ .compatible = "microchip,sam9x60-sdhci" },
> +	{ .compatible = "microchip,lan9691-sdhci",
> +	  .data = (void *)LAN969X_GCK_RATE },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, at91_sdhci_dt_match);
> -- 
> 2.47.3
> 
> 
> 

-- 
#gernperDu 
#CallMeByMyFirstName

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |