[PATCH v3 0/3] SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST removal

[PATCH v3 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:
- v3:
  - s/sdhci_compute_clock_config/sdhci_calc_clk.
  - don't update actual_clock in sdhci_calc_clk but return its value as an
  output parameter.
- 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         | 34 ++++++++++++++++++----------
 drivers/mmc/host/sdhci.h         |  7 ++----
 3 files changed, 70 insertions(+), 19 deletions(-)

-- 
2.5.0

[PATCH v3 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 | 36 ++++++++++++++++++++++++------------
 drivers/mmc/host/sdhci.h |  2 ++
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6bd3d17..abd34d1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1091,23 +1091,14 @@ 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_calc_clk(struct sdhci_host *host, unsigned int clock,
+		   unsigned int *actual_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;
@@ -1184,10 +1175,31 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 
 clock_set:
 	if (real_div)
-		host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
+		*actual_clock = (host->max_clk * clk_mul) / real_div;
 	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_calc_clk);
+
+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_calc_clk(host, clock, &host->mmc->actual_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..9db5090 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
 	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
 }
 
+u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
+		   unsigned int *actual_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

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

[Adrian Hunter]

On 07/04/16 12:13, 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>

The subject needs changing to reflect the new function name i.e.
sdhci_compute_clock_config -> sdhci_calc_clk

Otherwise, for all 3 patches:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++++------------
>  drivers/mmc/host/sdhci.h |  2 ++
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6bd3d17..abd34d1 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1091,23 +1091,14 @@ 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_calc_clk(struct sdhci_host *host, unsigned int clock,
> +		   unsigned int *actual_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;
> @@ -1184,10 +1175,31 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  clock_set:
>  	if (real_div)
> -		host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
> +		*actual_clock = (host->max_clk * clk_mul) / real_div;
>  	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_calc_clk);
> +
> +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_calc_clk(host, clock, &host->mmc->actual_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..9db5090 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
>  	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
>  }
>  
> +u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> +		   unsigned int *actual_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 v3 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Ludovic Desroches]

On Tue, Apr 12, 2016 at 03:31:46PM +0300, Adrian Hunter wrote:
> On 07/04/16 12:13, 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>
> 
> The subject needs changing to reflect the new function name i.e.
> sdhci_compute_clock_config -> sdhci_calc_clk

Oh yes... sorry. Ulf, do you want me to resend it?

> 
> Otherwise, for all 3 patches:
> 
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> > ---
> >  drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++++------------
> >  drivers/mmc/host/sdhci.h |  2 ++
> >  2 files changed, 26 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 6bd3d17..abd34d1 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1091,23 +1091,14 @@ 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_calc_clk(struct sdhci_host *host, unsigned int clock,
> > +		   unsigned int *actual_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;
> > @@ -1184,10 +1175,31 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> >  
> >  clock_set:
> >  	if (real_div)
> > -		host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
> > +		*actual_clock = (host->max_clk * clk_mul) / real_div;
> >  	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_calc_clk);
> > +
> > +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_calc_clk(host, clock, &host->mmc->actual_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..9db5090 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> >  	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
> >  }
> >  
> > +u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> > +		   unsigned int *actual_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 v3 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Ulf Hansson]

On 12 April 2016 at 14:53, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> On Tue, Apr 12, 2016 at 03:31:46PM +0300, Adrian Hunter wrote:
>> On 07/04/16 12:13, 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>
>>
>> The subject needs changing to reflect the new function name i.e.
>> sdhci_compute_clock_config -> sdhci_calc_clk
>
> Oh yes... sorry. Ulf, do you want me to resend it?

No worries, I can amend the patch.

Kind regards
Uffe

>
>>
>> Otherwise, for all 3 patches:
>>
>> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>>
>> > ---
>> >  drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++++------------
>> >  drivers/mmc/host/sdhci.h |  2 ++
>> >  2 files changed, 26 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> > index 6bd3d17..abd34d1 100644
>> > --- a/drivers/mmc/host/sdhci.c
>> > +++ b/drivers/mmc/host/sdhci.c
>> > @@ -1091,23 +1091,14 @@ 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_calc_clk(struct sdhci_host *host, unsigned int clock,
>> > +              unsigned int *actual_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;
>> > @@ -1184,10 +1175,31 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>> >
>> >  clock_set:
>> >     if (real_div)
>> > -           host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
>> > +           *actual_clock = (host->max_clk * clk_mul) / real_div;
>> >     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_calc_clk);
>> > +
>> > +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_calc_clk(host, clock, &host->mmc->actual_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..9db5090 100644
>> > --- a/drivers/mmc/host/sdhci.h
>> > +++ b/drivers/mmc/host/sdhci.h
>> > @@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
>> >     return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
>> >  }
>> >
>> > +u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>> > +              unsigned int *actual_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 v3 1/3] mmc: sdhci: introduce sdhci_compute_clock_config

[Ludovic Desroches]

On Tue, Apr 12, 2016 at 02:59:15PM +0200, Ulf Hansson wrote:
> On 12 April 2016 at 14:53, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > On Tue, Apr 12, 2016 at 03:31:46PM +0300, Adrian Hunter wrote:
> >> On 07/04/16 12:13, 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>
> >>
> >> The subject needs changing to reflect the new function name i.e.
> >> sdhci_compute_clock_config -> sdhci_calc_clk
> >
> > Oh yes... sorry. Ulf, do you want me to resend it?
> 
> No worries, I can amend the patch.

Great, thx.

> 
> Kind regards
> Uffe
> 
> >
> >>
> >> Otherwise, for all 3 patches:
> >>
> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> >>
> >> > ---
> >> >  drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++++------------
> >> >  drivers/mmc/host/sdhci.h |  2 ++
> >> >  2 files changed, 26 insertions(+), 12 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> > index 6bd3d17..abd34d1 100644
> >> > --- a/drivers/mmc/host/sdhci.c
> >> > +++ b/drivers/mmc/host/sdhci.c
> >> > @@ -1091,23 +1091,14 @@ 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_calc_clk(struct sdhci_host *host, unsigned int clock,
> >> > +              unsigned int *actual_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;
> >> > @@ -1184,10 +1175,31 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> >> >
> >> >  clock_set:
> >> >     if (real_div)
> >> > -           host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
> >> > +           *actual_clock = (host->max_clk * clk_mul) / real_div;
> >> >     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_calc_clk);
> >> > +
> >> > +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_calc_clk(host, clock, &host->mmc->actual_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..9db5090 100644
> >> > --- a/drivers/mmc/host/sdhci.h
> >> > +++ b/drivers/mmc/host/sdhci.h
> >> > @@ -661,6 +661,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> >> >     return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
> >> >  }
> >> >
> >> > +u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> >> > +              unsigned int *actual_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);
> >> >
> >>

[PATCH v3 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..c1923c0 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_calc_clk(host, clock, &host->mmc->actual_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 v3 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 abd34d1..0ca16ee 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1192,8 +1192,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 9db5090..0decc85 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 v3 0/3] SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST removal

[Ulf Hansson]

On 7 April 2016 at 11:13, Ludovic Desroches <ludovic.desroches@atmel.com> wrote:
> 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:
> - v3:
>   - s/sdhci_compute_clock_config/sdhci_calc_clk.
>   - don't update actual_clock in sdhci_calc_clk but return its value as an
>   output parameter.
> - 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         | 34 ++++++++++++++++++----------
>  drivers/mmc/host/sdhci.h         |  7 ++----
>  3 files changed, 70 insertions(+), 19 deletions(-)
>
> --
> 2.5.0
>

Thanks, applied for next with some minor updates to change logs.

Kind regards
Uffe