[PATCH 0/7] SDHCI clock handling fixes and cleanups

[PATCH 0/7] SDHCI clock handling fixes and cleanups

[Michał Mirosław]

This patch set combines a few of code improvements for SDHCI clock handling.
Besides small fixes, most value comes from simplifying the code, so it's
easier to understand.

Michał Mirosław (7):
  mmc: sdhci: fix base clock usage in preset value
  mmc: sdhci: fix programmable clock config from preset value
  mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN
  mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit
  mmc: sdhci: simplify clock frequency calculation
  mmc: sdhci: squash v2/v3+ clock calculation differences
  mmc: sdhci: respect non-zero div quirk in programmable clock mode

 drivers/mmc/host/sdhci-of-arasan.c |   7 +-
 drivers/mmc/host/sdhci.c           | 126 +++++++++++++----------------
 drivers/mmc/host/sdhci.h           |   4 +-
 3 files changed, 64 insertions(+), 73 deletions(-)

-- 
2.20.1

[PATCH 3/7] mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN

[Michał Mirosław]

Fix returned clock rate for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN case.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Cc: stable@kernel.vger.org
Fixes: d1955c3a9a1d ("mmc: sdhci: add quirk SDHCI_QUIRK_CLOCK_DIV_ZERO_BROKEN")
---
 drivers/mmc/host/sdhci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b2dc4f1cfa5c..a043bf5e3565 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1807,9 +1807,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 
 		if (!host->clk_mul || switch_base_clk) {
 			/* Version 3.00 divisors must be a multiple of 2. */
-			if (host->max_clk <= clock)
+			if (host->max_clk <= clock) {
 				div = 1;
-			else {
+				if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
+					&& host->max_clk <= 25000000)
+					div = 2;
+			} else {
 				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
 				     div += 2) {
 					if ((host->max_clk / div) <= clock)
@@ -1818,9 +1821,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 			}
 			real_div = div;
 			div >>= 1;
-			if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
-				&& !div && host->max_clk <= 25000000)
-				div = 1;
 		}
 	} else {
 		/* Version 2.00 divisors must be a power of 2. */
-- 
2.20.1

Re: [PATCH 3/7] mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN

[Adrian Hunter]

On 2/04/20 2:54 pm, Michał Mirosław wrote:
> Fix returned clock rate for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN case.

Does this change anything, because it looks the same to me?

> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Cc: stable@kernel.vger.org
> Fixes: d1955c3a9a1d ("mmc: sdhci: add quirk SDHCI_QUIRK_CLOCK_DIV_ZERO_BROKEN")
> ---
>  drivers/mmc/host/sdhci.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index b2dc4f1cfa5c..a043bf5e3565 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1807,9 +1807,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  
>  		if (!host->clk_mul || switch_base_clk) {
>  			/* Version 3.00 divisors must be a multiple of 2. */
> -			if (host->max_clk <= clock)
> +			if (host->max_clk <= clock) {
>  				div = 1;
> -			else {
> +				if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> +					&& host->max_clk <= 25000000)
> +					div = 2;
> +			} else {
>  				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
>  				     div += 2) {
>  					if ((host->max_clk / div) <= clock)
> @@ -1818,9 +1821,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  			}
>  			real_div = div;
>  			div >>= 1;
> -			if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> -				&& !div && host->max_clk <= 25000000)
> -				div = 1;
>  		}
>  	} else {
>  		/* Version 2.00 divisors must be a power of 2. */
> 

Re: [PATCH 3/7] mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN

[Michał Mirosław]

On Wed, Apr 15, 2020 at 04:06:02PM +0300, Adrian Hunter wrote:
> On 2/04/20 2:54 pm, Michał Mirosław wrote:
> > Fix returned clock rate for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN case.
> 
> Does this change anything, because it looks the same to me?

The value of real_div is fixed this way. With previous code after
applying the quirk you would have real_div = 1 instead of real_div = 2.

Best Regards,
Michał Mirosław

> 
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Cc: stable@kernel.vger.org
> > Fixes: d1955c3a9a1d ("mmc: sdhci: add quirk SDHCI_QUIRK_CLOCK_DIV_ZERO_BROKEN")
> > ---
> >  drivers/mmc/host/sdhci.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index b2dc4f1cfa5c..a043bf5e3565 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1807,9 +1807,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> >  
> >  		if (!host->clk_mul || switch_base_clk) {
> >  			/* Version 3.00 divisors must be a multiple of 2. */
> > -			if (host->max_clk <= clock)
> > +			if (host->max_clk <= clock) {
> >  				div = 1;
> > -			else {
> > +				if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> > +					&& host->max_clk <= 25000000)
> > +					div = 2;
> > +			} else {
> >  				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
> >  				     div += 2) {
> >  					if ((host->max_clk / div) <= clock)
> > @@ -1818,9 +1821,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> >  			}
> >  			real_div = div;
> >  			div >>= 1;
> > -			if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> > -				&& !div && host->max_clk <= 25000000)
> > -				div = 1;
> >  		}
> >  	} else {
> >  		/* Version 2.00 divisors must be a power of 2. */
> > 
> 

Re: [PATCH 3/7] mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN

[Adrian Hunter]

On 15/04/20 7:03 pm, Michał Mirosław wrote:
> On Wed, Apr 15, 2020 at 04:06:02PM +0300, Adrian Hunter wrote:
>> On 2/04/20 2:54 pm, Michał Mirosław wrote:
>>> Fix returned clock rate for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN case.
>>
>> Does this change anything, because it looks the same to me?
> 
> The value of real_div is fixed this way. With previous code after
> applying the quirk you would have real_div = 1 instead of real_div = 2.

That kind of thing should be in the commit message.  Please also explain
what effect this has (the actual clock value will be too high, but also what
problems does that manifest) and what hardware is affected.

> 
> Best Regards,
> Michał Mirosław
> 
>>
>>>
>>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>>> Cc: stable@kernel.vger.org
>>> Fixes: d1955c3a9a1d ("mmc: sdhci: add quirk SDHCI_QUIRK_CLOCK_DIV_ZERO_BROKEN")
>>> ---
>>>  drivers/mmc/host/sdhci.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index b2dc4f1cfa5c..a043bf5e3565 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1807,9 +1807,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>>>  
>>>  		if (!host->clk_mul || switch_base_clk) {
>>>  			/* Version 3.00 divisors must be a multiple of 2. */
>>> -			if (host->max_clk <= clock)
>>> +			if (host->max_clk <= clock) {
>>>  				div = 1;
>>> -			else {
>>> +				if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
>>> +					&& host->max_clk <= 25000000)
>>> +					div = 2;
>>> +			} else {
>>>  				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
>>>  				     div += 2) {
>>>  					if ((host->max_clk / div) <= clock)
>>> @@ -1818,9 +1821,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>>>  			}
>>>  			real_div = div;
>>>  			div >>= 1;
>>> -			if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
>>> -				&& !div && host->max_clk <= 25000000)
>>> -				div = 1;
>>>  		}
>>>  	} else {
>>>  		/* Version 2.00 divisors must be a power of 2. */
>>>
>>

[PATCH 2/7] mmc: sdhci: fix programmable clock config from preset value

[Michał Mirosław]

When host controller uses programmable clock presets but doesn't
advertise programmable clock support, we can only guess what frequency
it generates. Let's at least return correct SDHCI_PROG_CLOCK_MODE bit
value in this case.

Fixes: 52983382c74f ("mmc: sdhci: enhance preset value function")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/mmc/host/sdhci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9aa3af5826df..b2dc4f1cfa5c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1767,11 +1767,10 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 
 			pre_val = sdhci_get_preset_value(host);
 			div = FIELD_GET(SDHCI_PRESET_SDCLK_FREQ_MASK, pre_val);
-			if (host->clk_mul &&
-				(pre_val & SDHCI_PRESET_CLKGEN_SEL)) {
+			if (pre_val & SDHCI_PRESET_CLKGEN_SEL) {
 				clk = SDHCI_PROG_CLOCK_MODE;
 				real_div = div + 1;
-				clk_mul = host->clk_mul;
+				clk_mul = host->clk_mul ?: 1;
 			} else {
 				real_div = max_t(int, 1, div << 1);
 			}
-- 
2.20.1

Re: [PATCH 2/7] mmc: sdhci: fix programmable clock config from preset value

[Adrian Hunter]

On 2/04/20 2:54 pm, Michał Mirosław wrote:
> When host controller uses programmable clock presets but doesn't
> advertise programmable clock support, we can only guess what frequency
> it generates. Let's at least return correct SDHCI_PROG_CLOCK_MODE bit
> value in this case.

It sounds like the hardware is broken, so I tend to think a workaround
should be in the relevant vendor driver, because that is where
device-specific information really belongs.

> 
> Fixes: 52983382c74f ("mmc: sdhci: enhance preset value function")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/mmc/host/sdhci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9aa3af5826df..b2dc4f1cfa5c 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1767,11 +1767,10 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  
>  			pre_val = sdhci_get_preset_value(host);
>  			div = FIELD_GET(SDHCI_PRESET_SDCLK_FREQ_MASK, pre_val);
> -			if (host->clk_mul &&
> -				(pre_val & SDHCI_PRESET_CLKGEN_SEL)) {
> +			if (pre_val & SDHCI_PRESET_CLKGEN_SEL) {
>  				clk = SDHCI_PROG_CLOCK_MODE;
>  				real_div = div + 1;
> -				clk_mul = host->clk_mul;
> +				clk_mul = host->clk_mul ?: 1;
>  			} else {
>  				real_div = max_t(int, 1, div << 1);
>  			}
> 

[PATCH 1/7] mmc: sdhci: fix base clock usage in preset value

[Michał Mirosław]

Fixed commit added an unnecessary read of CLOCK_CONTROL. The value read
is overwritten for programmable clock preset, but is carried over for
divided clock preset. This can confuse sdhci_enable_clk() if the register
has enable bits set for some reason at time time of clock calculation.
value to be ORed with enable flags. Remove the read.

Fixes: 52983382c74f ("mmc: sdhci: enhance preset value function")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/mmc/host/sdhci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3f716466fcfd..9aa3af5826df 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1765,7 +1765,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 		if (host->preset_enabled) {
 			u16 pre_val;
 
-			clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 			pre_val = sdhci_get_preset_value(host);
 			div = FIELD_GET(SDHCI_PRESET_SDCLK_FREQ_MASK, pre_val);
 			if (host->clk_mul &&
-- 
2.20.1

Re: [PATCH 1/7] mmc: sdhci: fix base clock usage in preset value

[Adrian Hunter]

On 2/04/20 2:54 pm, Michał Mirosław wrote:
> Fixed commit added an unnecessary read of CLOCK_CONTROL. The value read
> is overwritten for programmable clock preset, but is carried over for
> divided clock preset. This can confuse sdhci_enable_clk() if the register
> has enable bits set for some reason at time time of clock calculation.
> value to be ORed with enable flags. Remove the read.

The read is not needed, but drivers usually manage the enable bits,
especially disabling the clock before changing the frequency.  What driver
is it?

> 
> Fixes: 52983382c74f ("mmc: sdhci: enhance preset value function")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/mmc/host/sdhci.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 3f716466fcfd..9aa3af5826df 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1765,7 +1765,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  		if (host->preset_enabled) {
>  			u16 pre_val;
>  
> -			clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>  			pre_val = sdhci_get_preset_value(host);
>  			div = FIELD_GET(SDHCI_PRESET_SDCLK_FREQ_MASK, pre_val);
>  			if (host->clk_mul &&
> 

Re: [PATCH 1/7] mmc: sdhci: fix base clock usage in preset value

[Michał Mirosław]

On Wed, Apr 15, 2020 at 03:25:52PM +0300, Adrian Hunter wrote:
> On 2/04/20 2:54 pm, Michał Mirosław wrote:
> > Fixed commit added an unnecessary read of CLOCK_CONTROL. The value read
> > is overwritten for programmable clock preset, but is carried over for
> > divided clock preset. This can confuse sdhci_enable_clk() if the register
> > has enable bits set for some reason at time time of clock calculation.
> > value to be ORed with enable flags. Remove the read.
> 
> The read is not needed, but drivers usually manage the enable bits,
> especially disabling the clock before changing the frequency.  What driver
> is it?

Hopefully no driver requires this. It's just removing a trap.

Best Regards,
Michał Mirosław

Re: [PATCH 1/7] mmc: sdhci: fix base clock usage in preset value

[Adrian Hunter]

On 15/04/20 7:28 pm, Michał Mirosław wrote:
> On Wed, Apr 15, 2020 at 03:25:52PM +0300, Adrian Hunter wrote:
>> On 2/04/20 2:54 pm, Michał Mirosław wrote:
>>> Fixed commit added an unnecessary read of CLOCK_CONTROL. The value read
>>> is overwritten for programmable clock preset, but is carried over for
>>> divided clock preset. This can confuse sdhci_enable_clk() if the register
>>> has enable bits set for some reason at time time of clock calculation.
>>> value to be ORed with enable flags. Remove the read.
>>
>> The read is not needed, but drivers usually manage the enable bits,
>> especially disabling the clock before changing the frequency.  What driver
>> is it?
> 
> Hopefully no driver requires this. It's just removing a trap.

The only driver that looks like it would benefit is sdhci-brcmstb because it
does not clear enable bits in sdhci_brcmstb_set_clock().  Adding Al Cooper.
Al, can you ack this?

Re: [PATCH 1/7] mmc: sdhci: fix base clock usage in preset value

[Alan Cooper]

Acked-by: Al Cooper <alcooperx@gmail.com>On Thu, Apr 16, 2020 at 4:27
AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 15/04/20 7:28 pm, Michał Mirosław wrote:
> > On Wed, Apr 15, 2020 at 03:25:52PM +0300, Adrian Hunter wrote:
> >> On 2/04/20 2:54 pm, Michał Mirosław wrote:
> >>> Fixed commit added an unnecessary read of CLOCK_CONTROL. The value read
> >>> is overwritten for programmable clock preset, but is carried over for
> >>> divided clock preset. This can confuse sdhci_enable_clk() if the register
> >>> has enable bits set for some reason at time time of clock calculation.
> >>> value to be ORed with enable flags. Remove the read.
> >>
> >> The read is not needed, but drivers usually manage the enable bits,
> >> especially disabling the clock before changing the frequency.  What driver
> >> is it?
> >
> > Hopefully no driver requires this. It's just removing a trap.
>
> The only driver that looks like it would benefit is sdhci-brcmstb because it
> does not clear enable bits in sdhci_brcmstb_set_clock().  Adding Al Cooper.
> Al, can you ack this?

sdhci_brcmstb_set_clock() assumed that sdhci_calc_clk() would always
return the divider value without the enable set, so this fixes a case
for DDR52 where the enable was not being cleared when the divider
value was changed.
Acked-by: Al Cooper <alcooperx@gmail.com>

Re: [PATCH 1/7] mmc: sdhci: fix base clock usage in preset value

[Adrian Hunter]

On 17/04/20 4:55 pm, Alan Cooper wrote:
> Acked-by: Al Cooper <alcooperx@gmail.com>On Thu, Apr 16, 2020 at 4:27
> AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 15/04/20 7:28 pm, Michał Mirosław wrote:
>>> On Wed, Apr 15, 2020 at 03:25:52PM +0300, Adrian Hunter wrote:
>>>> On 2/04/20 2:54 pm, Michał Mirosław wrote:
>>>>> Fixed commit added an unnecessary read of CLOCK_CONTROL. The value read
>>>>> is overwritten for programmable clock preset, but is carried over for
>>>>> divided clock preset. This can confuse sdhci_enable_clk() if the register
>>>>> has enable bits set for some reason at time time of clock calculation.
>>>>> value to be ORed with enable flags. Remove the read.
>>>>
>>>> The read is not needed, but drivers usually manage the enable bits,
>>>> especially disabling the clock before changing the frequency.  What driver
>>>> is it?
>>>
>>> Hopefully no driver requires this. It's just removing a trap.
>>
>> The only driver that looks like it would benefit is sdhci-brcmstb because it
>> does not clear enable bits in sdhci_brcmstb_set_clock().  Adding Al Cooper.
>> Al, can you ack this?
> 
> sdhci_brcmstb_set_clock() assumed that sdhci_calc_clk() would always
> return the divider value without the enable set, so this fixes a case
> for DDR52 where the enable was not being cleared when the divider
> value was changed.
> Acked-by: Al Cooper <alcooperx@gmail.com>
> 

That is good to know.  The commit message needs to be updated with that
information, otherwise:

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

[PATCH 4/7] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit

[Michał Mirosław]

Move clock frequency limit for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN where
it belongs.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/mmc/host/sdhci-of-arasan.c | 7 ++++---
 drivers/mmc/host/sdhci.c           | 3 +--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index d4905c106c06..5e3b9131a631 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -339,7 +339,6 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
 	.ops = &sdhci_arasan_ops,
 	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
 	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-			SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
 			SDHCI_QUIRK2_STOP_WITH_TC,
 };
 
@@ -410,8 +409,7 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops = {
 static const struct sdhci_pltfm_data sdhci_arasan_cqe_pdata = {
 	.ops = &sdhci_arasan_cqe_ops,
 	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-			SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 };
 
 static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
@@ -1155,6 +1153,9 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
 	bool dma64;
 	int ret;
 
+	if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
+		host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
+
 	if (!sdhci_arasan->has_cqe)
 		return sdhci_add_host(host);
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a043bf5e3565..ed88ac4e4cf3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1809,8 +1809,7 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 			/* Version 3.00 divisors must be a multiple of 2. */
 			if (host->max_clk <= clock) {
 				div = 1;
-				if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
-					&& host->max_clk <= 25000000)
+				if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
 					div = 2;
 			} else {
 				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
-- 
2.20.1

Re: [PATCH 4/7] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit

[Adrian Hunter]

On 2/04/20 2:54 pm, Michał Mirosław wrote:
> Move clock frequency limit for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN where
> it belongs.

Did you consider getting rid of SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN and
handling it in sdhci-of-arasan instead?

> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 7 ++++---
>  drivers/mmc/host/sdhci.c           | 3 +--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index d4905c106c06..5e3b9131a631 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -339,7 +339,6 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
>  	.ops = &sdhci_arasan_ops,
>  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>  	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> -			SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
>  			SDHCI_QUIRK2_STOP_WITH_TC,
>  };
>  
> @@ -410,8 +409,7 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops = {
>  static const struct sdhci_pltfm_data sdhci_arasan_cqe_pdata = {
>  	.ops = &sdhci_arasan_cqe_ops,
>  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> -	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> -			SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>  };
>  
>  static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
> @@ -1155,6 +1153,9 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
>  	bool dma64;
>  	int ret;
>  
> +	if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
> +		host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
> +
>  	if (!sdhci_arasan->has_cqe)
>  		return sdhci_add_host(host);
>  
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a043bf5e3565..ed88ac4e4cf3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1809,8 +1809,7 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  			/* Version 3.00 divisors must be a multiple of 2. */
>  			if (host->max_clk <= clock) {
>  				div = 1;
> -				if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> -					&& host->max_clk <= 25000000)
> +				if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
>  					div = 2;
>  			} else {
>  				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
> 

Re: [PATCH 4/7] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit

[Michał Mirosław]

On Wed, Apr 15, 2020 at 04:16:04PM +0300, Adrian Hunter wrote:
> On 2/04/20 2:54 pm, Michał Mirosław wrote:
> > Move clock frequency limit for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN where
> > it belongs.
> 
> Did you consider getting rid of SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN and
> handling it in sdhci-of-arasan instead?

I'm expecting to use this quirk for DDR mode support in other host drivers,
but I can't test this, yet.

Best Regards,
Michał Mirosław

Re: [PATCH 4/7] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit

[Adrian Hunter]

On 15/04/20 7:16 pm, Michał Mirosław wrote:
> On Wed, Apr 15, 2020 at 04:16:04PM +0300, Adrian Hunter wrote:
>> On 2/04/20 2:54 pm, Michał Mirosław wrote:
>>> Move clock frequency limit for SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN where
>>> it belongs.
>>
>> Did you consider getting rid of SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN and
>> handling it in sdhci-of-arasan instead?
> 
> I'm expecting to use this quirk for DDR mode support in other host drivers,
> but I can't test this, yet.

I didn't mean get rid of the functionality.  I meant implement it in
sdhci-of-arasan.  In general we want to reduce the quirks in sdhci.c and
implement them instead in vendor drivers.

[PATCH 6/7] mmc: sdhci: squash v2/v3+ clock calculation differences

[Michał Mirosław]

SDHCI V2 differs from base-clock-V3+ only in allowed divisor values.
Remove the duplicate version of code. We can see now, that 'real_div'
can't be zero.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/mmc/host/sdhci.c | 106 ++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 57 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d750c0997c3f..01fd897f8f3c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1756,82 +1756,74 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
 u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 		   unsigned int *actual_clock)
 {
-	unsigned int div = 0; /* Initialized for compiler warning */
-	int real_div = div, clk_mul = 1;
+	unsigned int div, real_div, clk_mul = 1;
 	u16 clk = 0;
-	bool use_base_clk;
 
 	if (clock == 0)
 		unreachable();
 
-	if (host->version >= SDHCI_SPEC_300) {
-		if (host->preset_enabled) {
-			u16 pre_val;
-
-			pre_val = sdhci_get_preset_value(host);
-			div = FIELD_GET(SDHCI_PRESET_SDCLK_FREQ_MASK, pre_val);
-			if (pre_val & SDHCI_PRESET_CLKGEN_SEL) {
-				clk = SDHCI_PROG_CLOCK_MODE;
-				real_div = div + 1;
-				clk_mul = host->clk_mul ?: 1;
-			} else {
-				real_div = max_t(int, 1, div << 1);
-			}
+	if (host->preset_enabled) {
+		/* Only version 3.00+ can have preset_enabled */
+		u16 pre_val;
+
+		pre_val = sdhci_get_preset_value(host);
+		div = FIELD_GET(SDHCI_PRESET_SDCLK_FREQ_MASK, pre_val);
+		if (pre_val & SDHCI_PRESET_CLKGEN_SEL) {
+			clk = SDHCI_PROG_CLOCK_MODE;
+			real_div = div + 1;
+			clk_mul = host->clk_mul ?: 1;
+		} else {
+			real_div = max_t(int, 1, div << 1);
+		}
+
+		goto clock_set;
+	}
+
+	/*
+	 * Check if the Host Controller supports Programmable Clock
+	 * Mode.
+	 */
+	if (host->version >= SDHCI_SPEC_300 && host->clk_mul) {
+		div = DIV_ROUND_UP(host->max_clk * host->clk_mul, clock);
+
+		if (div <= SDHCI_MAX_DIV_SPEC_300 / 2 + 1) {
+			/*
+			 * Set Programmable Clock Mode in the Clock
+			 * Control register.
+			 */
+			clk = SDHCI_PROG_CLOCK_MODE;
+			real_div = div;
+			clk_mul = host->clk_mul;
+			div--;
+
 			goto clock_set;
 		}
 
 		/*
-		 * Check if the Host Controller supports Programmable Clock
-		 * Mode.
+		 * Divisor is too big for requested clock rate.
+		 * Fall back to the base clock, then.
 		 */
-		use_base_clk = !host->clk_mul;
+	}
 
-		if (!use_base_clk) {
-			div = DIV_ROUND_UP(host->max_clk * host->clk_mul, clock);
+	div = DIV_ROUND_UP(host->max_clk, clock);
 
-			if (div <= SDHCI_MAX_DIV_SPEC_300 / 2 + 1) {
-				/*
-				 * Set Programmable Clock Mode in the Clock
-				 * Control register.
-				 */
-				clk = SDHCI_PROG_CLOCK_MODE;
-				real_div = div;
-				clk_mul = host->clk_mul;
-				div--;
-			} else {
-				/*
-				 * Divisor is too big for requested clock rate.
-				 * Use the base clock, then.
-				 */
-				use_base_clk = true;
-			}
-		}
+	if (div == 1 && (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN))
+		div = 2;
 
-		if (use_base_clk) {
-			/* Version 3.00 divisors must be 1 or a multiple of 2. */
-			div = DIV_ROUND_UP(host->max_clk, clock);
-			if (div > 1) {
-				div = min(div, SDHCI_MAX_DIV_SPEC_300);
-				div = round_up(div, 2);
-			}
-			div >>= 1;
-			if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
-				div += !div;
-
-			real_div = div * 2 + !div;
-		}
+	if (host->version >= SDHCI_SPEC_300) {
+		/* Version 3.00 divisors must be a multiple of 2. */
+		div = min(div, SDHCI_MAX_DIV_SPEC_300);
+		div = DIV_ROUND_UP(div, 2);
 	} else {
 		/* Version 2.00 divisors must be a power of 2. */
-		div = DIV_ROUND_UP(host->max_clk, clock);
 		div = min(div, SDHCI_MAX_DIV_SPEC_200);
-		div = roundup_pow_of_two(div);
-		real_div = div;
-		div >>= 1;
+		div = roundup_pow_of_two(div) / 2;
 	}
 
+	real_div = div * 2 + !div;
+
 clock_set:
-	if (real_div)
-		*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;
-- 
2.20.1

[PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

[Michał Mirosław]

Make clock frequency calculations simpler by replacing loops
with divide-and-clamp.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/mmc/host/sdhci.c | 56 +++++++++++++++++++---------------------
 drivers/mmc/host/sdhci.h |  4 +--
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ed88ac4e4cf3..d750c0997c3f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1756,10 +1756,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
 u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 		   unsigned int *actual_clock)
 {
-	int div = 0; /* Initialized for compiler warning */
+	unsigned int div = 0; /* Initialized for compiler warning */
 	int real_div = div, clk_mul = 1;
 	u16 clk = 0;
-	bool switch_base_clk = false;
+	bool use_base_clk;
+
+	if (clock == 0)
+		unreachable();
 
 	if (host->version >= SDHCI_SPEC_300) {
 		if (host->preset_enabled) {
@@ -1781,13 +1784,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 		 * Check if the Host Controller supports Programmable Clock
 		 * Mode.
 		 */
-		if (host->clk_mul) {
-			for (div = 1; div <= 1024; div++) {
-				if ((host->max_clk * host->clk_mul / div)
-					<= clock)
-					break;
-			}
-			if ((host->max_clk * host->clk_mul / div) <= clock) {
+		use_base_clk = !host->clk_mul;
+
+		if (!use_base_clk) {
+			div = DIV_ROUND_UP(host->max_clk * host->clk_mul, clock);
+
+			if (div <= SDHCI_MAX_DIV_SPEC_300 / 2 + 1) {
 				/*
 				 * Set Programmable Clock Mode in the Clock
 				 * Control register.
@@ -1798,35 +1800,31 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 				div--;
 			} else {
 				/*
-				 * Divisor can be too small to reach clock
-				 * speed requirement. Then use the base clock.
+				 * Divisor is too big for requested clock rate.
+				 * Use the base clock, then.
 				 */
-				switch_base_clk = true;
+				use_base_clk = true;
 			}
 		}
 
-		if (!host->clk_mul || switch_base_clk) {
-			/* Version 3.00 divisors must be a multiple of 2. */
-			if (host->max_clk <= clock) {
-				div = 1;
-				if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
-					div = 2;
-			} else {
-				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
-				     div += 2) {
-					if ((host->max_clk / div) <= clock)
-						break;
-				}
+		if (use_base_clk) {
+			/* Version 3.00 divisors must be 1 or a multiple of 2. */
+			div = DIV_ROUND_UP(host->max_clk, clock);
+			if (div > 1) {
+				div = min(div, SDHCI_MAX_DIV_SPEC_300);
+				div = round_up(div, 2);
 			}
-			real_div = div;
 			div >>= 1;
+			if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
+				div += !div;
+
+			real_div = div * 2 + !div;
 		}
 	} else {
 		/* Version 2.00 divisors must be a power of 2. */
-		for (div = 1; div < SDHCI_MAX_DIV_SPEC_200; div *= 2) {
-			if ((host->max_clk / div) <= clock)
-				break;
-		}
+		div = DIV_ROUND_UP(host->max_clk, clock);
+		div = min(div, SDHCI_MAX_DIV_SPEC_200);
+		div = roundup_pow_of_two(div);
 		real_div = div;
 		div >>= 1;
 	}
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 79dffbb731d3..ea8aabb3bf16 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -290,8 +290,8 @@
  * End of controller registers.
  */
 
-#define SDHCI_MAX_DIV_SPEC_200	256
-#define SDHCI_MAX_DIV_SPEC_300	2046
+#define SDHCI_MAX_DIV_SPEC_200	256u
+#define SDHCI_MAX_DIV_SPEC_300	2046u
 
 /*
  * Host SDMA buffer boundary. Valid values from 4K to 512K in powers of 2.
-- 
2.20.1

Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

Hi "Michał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on next-20200403]
[cannot apply to xlnx/master v5.6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Micha-Miros-aw/SDHCI-clock-handling-fixes-and-cleanups/20200403-132632
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bef7b2a7be28638770972ab2709adf11d601c11a
config: x86_64-randconfig-s2-20200403 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci.o: warning: objtool: sdhci_calc_clk() falls through to next function sdhci_set_clock()

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32642 bytes --]

Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

[Michał Mirosław]

On Fri, Apr 03, 2020 at 09:02:35PM +0800, kbuild test robot wrote:
[...]
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/mmc/host/sdhci.o: warning: objtool: sdhci_calc_clk() falls through to next function sdhci_set_clock()

This looks like false positive, as there is 'return clk;' at the
function's end untouched by the patches.

Best Regards,
Michał Mirosław

Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

[Adrian Hunter]

On 2/04/20 2:54 pm, Michał Mirosław wrote:
> Make clock frequency calculations simpler by replacing loops
> with divide-and-clamp.

I am sorry, but I am not really sure the simplification is worth the code
churn, risk of introducing new bugs, or validation effort.

IMO, the loops, while perhaps inefficient, are not hard to understand.

> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/mmc/host/sdhci.c | 56 +++++++++++++++++++---------------------
>  drivers/mmc/host/sdhci.h |  4 +--
>  2 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ed88ac4e4cf3..d750c0997c3f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1756,10 +1756,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
>  u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  		   unsigned int *actual_clock)
>  {
> -	int div = 0; /* Initialized for compiler warning */
> +	unsigned int div = 0; /* Initialized for compiler warning */
>  	int real_div = div, clk_mul = 1;
>  	u16 clk = 0;
> -	bool switch_base_clk = false;
> +	bool use_base_clk;
> +
> +	if (clock == 0)
> +		unreachable();
>  
>  	if (host->version >= SDHCI_SPEC_300) {
>  		if (host->preset_enabled) {
> @@ -1781,13 +1784,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  		 * Check if the Host Controller supports Programmable Clock
>  		 * Mode.
>  		 */
> -		if (host->clk_mul) {
> -			for (div = 1; div <= 1024; div++) {
> -				if ((host->max_clk * host->clk_mul / div)
> -					<= clock)
> -					break;
> -			}
> -			if ((host->max_clk * host->clk_mul / div) <= clock) {
> +		use_base_clk = !host->clk_mul;
> +
> +		if (!use_base_clk) {
> +			div = DIV_ROUND_UP(host->max_clk * host->clk_mul, clock);
> +
> +			if (div <= SDHCI_MAX_DIV_SPEC_300 / 2 + 1) {
>  				/*
>  				 * Set Programmable Clock Mode in the Clock
>  				 * Control register.
> @@ -1798,35 +1800,31 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>  				div--;
>  			} else {
>  				/*
> -				 * Divisor can be too small to reach clock
> -				 * speed requirement. Then use the base clock.
> +				 * Divisor is too big for requested clock rate.
> +				 * Use the base clock, then.
>  				 */
> -				switch_base_clk = true;
> +				use_base_clk = true;
>  			}
>  		}
>  
> -		if (!host->clk_mul || switch_base_clk) {
> -			/* Version 3.00 divisors must be a multiple of 2. */
> -			if (host->max_clk <= clock) {
> -				div = 1;
> -				if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> -					div = 2;
> -			} else {
> -				for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
> -				     div += 2) {
> -					if ((host->max_clk / div) <= clock)
> -						break;
> -				}
> +		if (use_base_clk) {
> +			/* Version 3.00 divisors must be 1 or a multiple of 2. */
> +			div = DIV_ROUND_UP(host->max_clk, clock);
> +			if (div > 1) {
> +				div = min(div, SDHCI_MAX_DIV_SPEC_300);
> +				div = round_up(div, 2);
>  			}
> -			real_div = div;
>  			div >>= 1;
> +			if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> +				div += !div;
> +
> +			real_div = div * 2 + !div;
>  		}
>  	} else {
>  		/* Version 2.00 divisors must be a power of 2. */
> -		for (div = 1; div < SDHCI_MAX_DIV_SPEC_200; div *= 2) {
> -			if ((host->max_clk / div) <= clock)
> -				break;
> -		}
> +		div = DIV_ROUND_UP(host->max_clk, clock);
> +		div = min(div, SDHCI_MAX_DIV_SPEC_200);
> +		div = roundup_pow_of_two(div);
>  		real_div = div;
>  		div >>= 1;
>  	}
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 79dffbb731d3..ea8aabb3bf16 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -290,8 +290,8 @@
>   * End of controller registers.
>   */
>  
> -#define SDHCI_MAX_DIV_SPEC_200	256
> -#define SDHCI_MAX_DIV_SPEC_300	2046
> +#define SDHCI_MAX_DIV_SPEC_200	256u
> +#define SDHCI_MAX_DIV_SPEC_300	2046u
>  
>  /*
>   * Host SDMA buffer boundary. Valid values from 4K to 512K in powers of 2.
> 

Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

[Michał Mirosław]

On Wed, Apr 15, 2020 at 04:54:33PM +0300, Adrian Hunter wrote:
> On 2/04/20 2:54 pm, Michał Mirosław wrote:
> > Make clock frequency calculations simpler by replacing loops
> > with divide-and-clamp.
> 
> I am sorry, but I am not really sure the simplification is worth the code
> churn, risk of introducing new bugs, or validation effort.
> 
> IMO, the loops, while perhaps inefficient, are not hard to understand.

I guess this is a kind of religious subject. ;-)
I tend to prefer shorter and less-branchy code.

Best Regards,
Michał Mirosław

[PATCH 7/7] mmc: sdhci: respect non-zero div quirk in programmable clock mode

[Michał Mirosław]

Make SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN respected also in programmable
clock mode.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/mmc/host/sdhci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 01fd897f8f3c..df80f39c570b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1807,9 +1807,6 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 
 	div = DIV_ROUND_UP(host->max_clk, clock);
 
-	if (div == 1 && (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN))
-		div = 2;
-
 	if (host->version >= SDHCI_SPEC_300) {
 		/* Version 3.00 divisors must be a multiple of 2. */
 		div = min(div, SDHCI_MAX_DIV_SPEC_300);
@@ -1823,6 +1820,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 	real_div = div * 2 + !div;
 
 clock_set:
+	if (!div && (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)) {
+		/* for div == 1, clock rate is divided by 2 in both modes */
+		div = 1;
+		real_div = 2;
+	}
+
 	*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)
-- 
2.20.1

Re: [PATCH 7/7] mmc: sdhci: respect non-zero div quirk in programmable clock mode

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Hi "Michał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on next-20200403]
[cannot apply to xlnx/master ulf.hansson-mmc/next v5.6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Micha-Miros-aw/SDHCI-clock-handling-fixes-and-cleanups/20200403-132632
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bef7b2a7be28638770972ab2709adf11d601c11a
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci.o: warning: objtool: sdhci_calc_clk() falls through to next function sdhci_start_signal_voltage_switch()

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44397 bytes --]

Re: [PATCH 0/7] SDHCI clock handling fixes and cleanups

[Ulf Hansson]

On Thu, 2 Apr 2020 at 13:54, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> This patch set combines a few of code improvements for SDHCI clock handling.
> Besides small fixes, most value comes from simplifying the code, so it's
> easier to understand.
>
> Michał Mirosław (7):
>   mmc: sdhci: fix base clock usage in preset value
>   mmc: sdhci: fix programmable clock config from preset value
>   mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN
>   mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit
>   mmc: sdhci: simplify clock frequency calculation
>   mmc: sdhci: squash v2/v3+ clock calculation differences
>   mmc: sdhci: respect non-zero div quirk in programmable clock mode
>
>  drivers/mmc/host/sdhci-of-arasan.c |   7 +-
>  drivers/mmc/host/sdhci.c           | 126 +++++++++++++----------------
>  drivers/mmc/host/sdhci.h           |   4 +-
>  3 files changed, 64 insertions(+), 73 deletions(-)
>
> --
> 2.20.1
>

Adrian, whenever you get the time, I would like to get your feedback
on these, especially on patch1->patch3 as those may be targeted for
fixes.

Kind regards
Uffe