| Message ID | e65dc96eb24caf8baa5431a51fe694b969e2d51f.1627204633.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SDHCI clock handling fixes and cleanups | expand |
On 25/07/21 12:20 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. If the preset value doesn't make sense, why use it at all? > > Fixes: 52983382c74f ("mmc: sdhci: enhance preset value function") > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > --- > v4: no changes > v3: added a comment for this case > v2: no changes > --- > drivers/mmc/host/sdhci.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index c7438dd13e3e..3ab60e7f936b 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -1859,11 +1859,14 @@ 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; > + if (!clk_mul) { > + /* The clock frequency is unknown. Assume undivided base. */ > + clk_mul = 1; > + } > } else { > real_div = max_t(int, 1, div << 1); > } >
On Wed, Aug 04, 2021 at 01:52:21PM +0300, Adrian Hunter wrote: > On 25/07/21 12:20 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. > If the preset value doesn't make sense, why use it at all? If I understand the spec correctly, when the preset value is used the values in Clock Control register are ignored by the module and so the module can also actually use a different clock source than the ones available to the driver directly. So either way the driver can't be sure of the exact frequencu used. This is a cleanup to remove a case when the code ignores a bit's value based on other unspecified assumptions. [...] > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -1859,11 +1859,14 @@ 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; > > + if (!clk_mul) { > > + /* The clock frequency is unknown. Assume undivided base. */ > > + clk_mul = 1; > > + } > > } else { > > real_div = max_t(int, 1, div << 1); > > }
On 7/08/21 5:05 pm, Michał Mirosław wrote: > On Wed, Aug 04, 2021 at 01:52:21PM +0300, Adrian Hunter wrote: >> On 25/07/21 12:20 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. >> If the preset value doesn't make sense, why use it at all? > > If I understand the spec correctly, when the preset value is used the > values in Clock Control register are ignored by the module and so the > module can also actually use a different clock source than the ones > available to the driver directly. I don't remember, does it say that in the spec? > So either way the driver can't be > sure of the exact frequencu used. This is a cleanup to remove a case > when the code ignores a bit's value based on other unspecified assumptions. Is this fixing a real issue? It seems like switching from one undefined scenario to another. Are either of which known to have ever happened? Perhaps we should leave it as is. > > [...] >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -1859,11 +1859,14 @@ 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; >>> + if (!clk_mul) { >>> + /* The clock frequency is unknown. Assume undivided base. */ >>> + clk_mul = 1; >>> + } >>> } else { >>> real_div = max_t(int, 1, div << 1); >>> }
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index c7438dd13e3e..3ab60e7f936b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1859,11 +1859,14 @@ 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; + if (!clk_mul) { + /* The clock frequency is unknown. Assume undivided base. */ + clk_mul = 1; + } } else { real_div = max_t(int, 1, div << 1); }
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> --- v4: no changes v3: added a comment for this case v2: no changes --- drivers/mmc/host/sdhci.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)