[PATCH v2 0/3] SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST removal

[PATCH v2 0/3] SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST removal

[Ludovic Desroches]

Hi,

I have recently observed that the quirk
SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST I have introduced doesn't fix all
the bugs relative to the internal clock disabling while configuring the SD
clock. This delay was introduced because of a re-synchronisation done when
disabling the internal clock.

Unfortunately, we can still have clock stabilization bug even if it occurs
rarely. Moreover, trying to use presets, disabling the internal clock causes an
unexpected switch to the base clock. It should be solved on next revision of
the chip.

For those reasons plus the non acceptance of new quirks, I have decided to
remove it and to implement my own set_clock() function. In ordrer to reduce
code duplication with the sdhci set_clock function, I moved some of the
code in a new sdhci_compute_clock_config() function.

Regards

Changes:
- v2:
  - sdhci_compute_clock_config uses a returned value instead of an
  out-parameter to provide the clock configuration.


Ludovic Desroches (3):
  mmc: sdhci: introduce sdhci_compute_clock_config
  mmc: sdhci-of-at91: implement specific set_clock function
  mmc: sdhci: removal of SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST

 drivers/mmc/host/sdhci-of-at91.c | 48 ++++++++++++++++++++++++++++++++++++++--
 drivers/mmc/host/sdhci.c         | 31 +++++++++++++++++---------
 drivers/mmc/host/sdhci.h         |  6 +----
 3 files changed, 67 insertions(+), 18 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Ludovic Desroches]

In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
reduce code duplication, put the code relative to the SD clock
configuration in a function which can be used by hosts for the
implementation of the set_clock() callback.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci.c | 33 ++++++++++++++++++++++-----------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6bd3d17..2c3dede 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1091,23 +1091,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
 	return preset;
 }
 
-void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock)
 {
 	int div = 0; /* Initialized for compiler warning */
 	int real_div = div, clk_mul = 1;
 	u16 clk = 0;
-	unsigned long timeout;
 	bool switch_base_clk = false;
 
-	host->mmc->actual_clock = 0;
-
-	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
-	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
-		mdelay(1);
-
-	if (clock == 0)
-		return;
-
 	if (host->version >= SDHCI_SPEC_300) {
 		if (host->preset_enabled) {
 			u16 pre_val;
@@ -1188,6 +1178,27 @@ clock_set:
 	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
 	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
 		<< SDHCI_DIVIDER_HI_SHIFT;
+
+	return clk;
+}
+EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
+
+void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	u16 clk;
+	unsigned long timeout;
+
+	host->mmc->actual_clock = 0;
+
+	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
+		mdelay(1);
+
+	if (clock == 0)
+		return;
+
+	clk = sdhci_compute_clock_config(host, clock);
+
 	clk |= SDHCI_CLOCK_INT_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0f39f4f..23ddd6e 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -661,6 +661,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
 	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
 }
 
+u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock);
 void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
 void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 		     unsigned short vdd);
-- 
2.5.0

[PATCH v2 2/3] mmc: sdhci-of-at91: implement specific set_clock function

[Ludovic Desroches]

Disabling the internal clock while configuring the SD card clock can
lead to internal clock stabilization issue and/or unexpected switch to
the base clock when using presets.
A quirk SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST was introduced to fix
these bugs. The cause was assumed to be a too long internal
re-synchronisation but it seems in some cases the delay (even if longer)
doesn't fix this bug. The safest workaround is to not disable/enable the
internal clock during the SD card clock configuration.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 48 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2703aa9..544850b 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/mmc/host.h>
@@ -37,8 +38,52 @@ struct sdhci_at91_priv {
 	struct clk *mainck;
 };
 
+static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	u16 clk;
+	unsigned long timeout;
+
+	host->mmc->actual_clock = 0;
+
+	/*
+	 * There is no requirement to disable the internal clock before
+	 * changing the SD clock configuration. Moreover, disabling the
+	 * internal clock, changing the configuration and re-enabling the
+	 * internal clock causes some bugs. It can prevent to get the internal
+	 * clock stable flag ready and an unexpected switch to the base clock
+	 * when using presets.
+	 */
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	clk &= SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	if (clock == 0)
+		return;
+
+	clk = sdhci_compute_clock_config(host, clock);
+
+	clk |= SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	/* Wait max 20 ms */
+	timeout = 20;
+	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
+		& SDHCI_CLOCK_INT_STABLE)) {
+		if (timeout == 0) {
+			pr_err("%s: Internal clock never stabilised.\n",
+			       mmc_hostname(host->mmc));
+			return;
+		}
+		timeout--;
+		mdelay(1);
+	}
+
+	clk |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+}
+
 static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
-	.set_clock		= sdhci_set_clock,
+	.set_clock		= sdhci_at91_set_clock,
 	.set_bus_width		= sdhci_set_bus_width,
 	.reset			= sdhci_reset,
 	.set_uhs_signaling	= sdhci_set_uhs_signaling,
@@ -46,7 +91,6 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
 
 static const struct sdhci_pltfm_data soc_data_sama5d2 = {
 	.ops = &sdhci_at91_sama5d2_ops,
-	.quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST,
 };
 
 static const struct of_device_id sdhci_at91_dt_match[] = {
-- 
2.5.0

[PATCH v2 3/3] mmc: sdhci: removal of SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST

[Ludovic Desroches]

SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST quirk is not used anymore so
remove it.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/mmc/host/sdhci.c | 2 --
 drivers/mmc/host/sdhci.h | 5 -----
 2 files changed, 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2c3dede..e2cfe6c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1191,8 +1191,6 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	host->mmc->actual_clock = 0;
 
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
-	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
-		mdelay(1);
 
 	if (clock == 0)
 		return;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 23ddd6e..4f60105 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -417,11 +417,6 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
 /* Broken Clock divider zero in controller */
 #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
-/*
- * When internal clock is disabled, a delay is needed before modifying the
- * SD clock frequency or enabling back the internal clock.
- */
-#define SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST	(1<<16)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.5.0

Re: [PATCH v2 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Adrian Hunter]

On 04/04/16 18:27, Ludovic Desroches wrote:
> In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
> reduce code duplication, put the code relative to the SD clock
> configuration in a function which can be used by hosts for the
> implementation of the set_clock() callback.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>  drivers/mmc/host/sdhci.c | 33 ++++++++++++++++++++++-----------
>  drivers/mmc/host/sdhci.h |  1 +
>  2 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6bd3d17..2c3dede 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1091,23 +1091,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
>  	return preset;
>  }
>  
> -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock)

The function name 'sdhci_compute_clock_config' seems a bit long.  How about
just sdhci_calc_clk.

Also it needs to calculate 'actual_clock' too, so it needs to do that and
return the value.

>  {
>  	int div = 0; /* Initialized for compiler warning */
>  	int real_div = div, clk_mul = 1;
>  	u16 clk = 0;
> -	unsigned long timeout;
>  	bool switch_base_clk = false;
>  
> -	host->mmc->actual_clock = 0;
> -
> -	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> -	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> -		mdelay(1);
> -
> -	if (clock == 0)
> -		return;
> -
>  	if (host->version >= SDHCI_SPEC_300) {
>  		if (host->preset_enabled) {
>  			u16 pre_val;
> @@ -1188,6 +1178,27 @@ clock_set:
>  	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>  	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>  		<< SDHCI_DIVIDER_HI_SHIFT;
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
> +
> +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	u16 clk;
> +	unsigned long timeout;
> +
> +	host->mmc->actual_clock = 0;
> +
> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> +		mdelay(1);
> +
> +	if (clock == 0)
> +		return;
> +
> +	clk = sdhci_compute_clock_config(host, clock);
> +
>  	clk |= SDHCI_CLOCK_INT_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>  
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0f39f4f..23ddd6e 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -661,6 +661,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
>  	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
>  }
>  
> +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock);
>  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
>  void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>  		     unsigned short vdd);
> 

Re: [PATCH v2 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Ludovic Desroches]

On Wed, Apr 06, 2016 at 03:37:28PM +0300, Adrian Hunter wrote:
> On 04/04/16 18:27, Ludovic Desroches wrote:
> > In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
> > reduce code duplication, put the code relative to the SD clock
> > configuration in a function which can be used by hosts for the
> > implementation of the set_clock() callback.
> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> >  drivers/mmc/host/sdhci.c | 33 ++++++++++++++++++++++-----------
> >  drivers/mmc/host/sdhci.h |  1 +
> >  2 files changed, 23 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 6bd3d17..2c3dede 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1091,23 +1091,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
> >  	return preset;
> >  }
> >  
> > -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock)
> 
> The function name 'sdhci_compute_clock_config' seems a bit long.  How about
> just sdhci_calc_clk.

Ok.

> 
> Also it needs to calculate 'actual_clock' too, so it needs to do that and
> return the value.
> 

actual_clock is updated at the end of the function.

Which value has to be returned? actual_clock or clock configuration?

Regards

Ludovic

> >  {
> >  	int div = 0; /* Initialized for compiler warning */
> >  	int real_div = div, clk_mul = 1;
> >  	u16 clk = 0;
> > -	unsigned long timeout;
> >  	bool switch_base_clk = false;
> >  
> > -	host->mmc->actual_clock = 0;
> > -
> > -	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> > -	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> > -		mdelay(1);
> > -
> > -	if (clock == 0)
> > -		return;
> > -
> >  	if (host->version >= SDHCI_SPEC_300) {
> >  		if (host->preset_enabled) {
> >  			u16 pre_val;
> > @@ -1188,6 +1178,27 @@ clock_set:
> >  	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
> >  	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
> >  		<< SDHCI_DIVIDER_HI_SHIFT;
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
> > +
> > +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > +{
> > +	u16 clk;
> > +	unsigned long timeout;
> > +
> > +	host->mmc->actual_clock = 0;
> > +
> > +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> > +	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> > +		mdelay(1);
> > +
> > +	if (clock == 0)
> > +		return;
> > +
> > +	clk = sdhci_compute_clock_config(host, clock);
> > +
> >  	clk |= SDHCI_CLOCK_INT_EN;
> >  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> >  
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index 0f39f4f..23ddd6e 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -661,6 +661,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> >  	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
> >  }
> >  
> > +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock);
> >  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
> >  void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >  		     unsigned short vdd);
> > 
> 

Re: [PATCH v2 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Ludovic Desroches]

On Wed, Apr 06, 2016 at 05:04:45PM +0200, Ludovic Desroches wrote:
> On Wed, Apr 06, 2016 at 03:37:28PM +0300, Adrian Hunter wrote:
> > On 04/04/16 18:27, Ludovic Desroches wrote:
> > > In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
> > > reduce code duplication, put the code relative to the SD clock
> > > configuration in a function which can be used by hosts for the
> > > implementation of the set_clock() callback.
> > > 
> > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > > ---
> > >  drivers/mmc/host/sdhci.c | 33 ++++++++++++++++++++++-----------
> > >  drivers/mmc/host/sdhci.h |  1 +
> > >  2 files changed, 23 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index 6bd3d17..2c3dede 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1091,23 +1091,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
> > >  	return preset;
> > >  }
> > >  
> > > -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > > +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock)
> > 
> > The function name 'sdhci_compute_clock_config' seems a bit long.  How about
> > just sdhci_calc_clk.
> 
> Ok.
> 
> > 
> > Also it needs to calculate 'actual_clock' too, so it needs to do that and
> > return the value.
> > 
> 
> actual_clock is updated at the end of the function.

Maybe it makes more sens to keep it in sdhci_set_clock().

> 
> Which value has to be returned? actual_clock or clock configuration?
> 
> Regards
> 
> Ludovic
> 
> > >  {
> > >  	int div = 0; /* Initialized for compiler warning */
> > >  	int real_div = div, clk_mul = 1;
> > >  	u16 clk = 0;
> > > -	unsigned long timeout;
> > >  	bool switch_base_clk = false;
> > >  
> > > -	host->mmc->actual_clock = 0;
> > > -
> > > -	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> > > -	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> > > -		mdelay(1);
> > > -
> > > -	if (clock == 0)
> > > -		return;
> > > -
> > >  	if (host->version >= SDHCI_SPEC_300) {
> > >  		if (host->preset_enabled) {
> > >  			u16 pre_val;
> > > @@ -1188,6 +1178,27 @@ clock_set:
> > >  	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
> > >  	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
> > >  		<< SDHCI_DIVIDER_HI_SHIFT;
> > > +
> > > +	return clk;
> > > +}
> > > +EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
> > > +
> > > +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > > +{
> > > +	u16 clk;
> > > +	unsigned long timeout;
> > > +
> > > +	host->mmc->actual_clock = 0;
> > > +
> > > +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> > > +	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
> > > +		mdelay(1);
> > > +
> > > +	if (clock == 0)
> > > +		return;
> > > +
> > > +	clk = sdhci_compute_clock_config(host, clock);
> > > +
> > >  	clk |= SDHCI_CLOCK_INT_EN;
> > >  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> > >  
> > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > > index 0f39f4f..23ddd6e 100644
> > > --- a/drivers/mmc/host/sdhci.h
> > > +++ b/drivers/mmc/host/sdhci.h
> > > @@ -661,6 +661,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> > >  	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
> > >  }
> > >  
> > > +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock);
> > >  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
> > >  void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > >  		     unsigned short vdd);
> > > 
> > 

Re: [PATCH v2 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Adrian Hunter]

On 06/04/16 18:04, Ludovic Desroches wrote:
> On Wed, Apr 06, 2016 at 03:37:28PM +0300, Adrian Hunter wrote:
>> On 04/04/16 18:27, Ludovic Desroches wrote:
>>> In order to remove the SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST and to
>>> reduce code duplication, put the code relative to the SD clock
>>> configuration in a function which can be used by hosts for the
>>> implementation of the set_clock() callback.
>>>
>>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>>> ---
>>>  drivers/mmc/host/sdhci.c | 33 ++++++++++++++++++++++-----------
>>>  drivers/mmc/host/sdhci.h |  1 +
>>>  2 files changed, 23 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 6bd3d17..2c3dede 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1091,23 +1091,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
>>>  	return preset;
>>>  }
>>>  
>>> -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>> +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock)
>>
>> The function name 'sdhci_compute_clock_config' seems a bit long.  How about
>> just sdhci_calc_clk.
> 
> Ok.
> 
>>
>> Also it needs to calculate 'actual_clock' too, so it needs to do that and
>> return the value.
>>
> 
> actual_clock is updated at the end of the function.
> 
> Which value has to be returned? actual_clock or clock configuration?

The function doesn't update the clock, so I don't think it should update
actual_clock either, just return it. i.e.

u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, unsigned int *actual_clock)
{
	...
	if (real_div)
		*actual_clock = (host->max_clk * clk_mul) / real_div;
	...
	return clk;
}

then

	clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);



> 
> Regards
> 
> Ludovic
> 
>>>  {
>>>  	int div = 0; /* Initialized for compiler warning */
>>>  	int real_div = div, clk_mul = 1;
>>>  	u16 clk = 0;
>>> -	unsigned long timeout;
>>>  	bool switch_base_clk = false;
>>>  
>>> -	host->mmc->actual_clock = 0;
>>> -
>>> -	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>> -	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
>>> -		mdelay(1);
>>> -
>>> -	if (clock == 0)
>>> -		return;
>>> -
>>>  	if (host->version >= SDHCI_SPEC_300) {
>>>  		if (host->preset_enabled) {
>>>  			u16 pre_val;
>>> @@ -1188,6 +1178,27 @@ clock_set:
>>>  	clk |= (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
>>>  	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN)
>>>  		<< SDHCI_DIVIDER_HI_SHIFT;
>>> +
>>> +	return clk;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sdhci_compute_clock_config);
>>> +
>>> +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>> +{
>>> +	u16 clk;
>>> +	unsigned long timeout;
>>> +
>>> +	host->mmc->actual_clock = 0;
>>> +
>>> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>> +	if (host->quirks2 & SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST)
>>> +		mdelay(1);
>>> +
>>> +	if (clock == 0)
>>> +		return;
>>> +
>>> +	clk = sdhci_compute_clock_config(host, clock);
>>> +
>>>  	clk |= SDHCI_CLOCK_INT_EN;
>>>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>  
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 0f39f4f..23ddd6e 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -661,6 +661,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
>>>  	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
>>>  }
>>>  
>>> +u16 sdhci_compute_clock_config(struct sdhci_host *host, unsigned int clock);
>>>  void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
>>>  void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>>>  		     unsigned short vdd);
>>>
>>
>