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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 &&
>
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
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?
Acked-by: Al Cooper <[email protected]>On Thu, Apr 16, 2020 at 4:27
AM Adrian Hunter <[email protected]> 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 <[email protected]>
On 17/04/20 4:55 pm, Alan Cooper wrote:
> Acked-by: Al Cooper <[email protected]>On Thu, Apr 16, 2020 at 4:27
> AM Adrian Hunter <[email protected]> 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 <[email protected]>
>
That is good to know. The commit message needs to be updated with that
information, otherwise:
Acked-by: Adrian Hunter <[email protected]>