* [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency
@ 2018-07-05 11:39 Stefan Agner
2018-07-11 12:29 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2018-07-05 11:39 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: fabio.estevam, haibo.chen, aisheng.dong, linux-mmc, linux-kernel,
Stefan Agner
In the uSDHC case (e.g. i.MX 6) clocks only get disabled if frequency
is set to 0. However, it could be that the stack asks for a frequency
change while clocks are on. In that case the function clears the
divider registers (by clearing ESDHC_CLOCK_MASK) while the clock is
enabled! This causes a short period of time where the clock is
undivided (on a i.MX 6DL a clock of 196MHz has been measured).
For older IP variants the driver disables clock by clearing some bits
in ESDHC_SYSTEM_CONTROL.
Make sure to disable card clock before chainging frequency for uSDHC
IP variants too. Also fix indent to make disable/enable clock look
alike.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 85fd5a8b0b6d..aa48f4b2541a 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -708,14 +708,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
int div = 1;
u32 temp, val;
+ if (esdhc_is_usdhc(imx_data)) {
+ val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+ writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
+ host->ioaddr + ESDHC_VENDOR_SPEC);
+ }
+
if (clock == 0) {
host->mmc->actual_clock = 0;
-
- if (esdhc_is_usdhc(imx_data)) {
- val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
- writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
- host->ioaddr + ESDHC_VENDOR_SPEC);
- }
return;
}
@@ -761,7 +761,7 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
if (esdhc_is_usdhc(imx_data)) {
val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
- host->ioaddr + ESDHC_VENDOR_SPEC);
+ host->ioaddr + ESDHC_VENDOR_SPEC);
}
mdelay(1);
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency
2018-07-05 11:39 [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency Stefan Agner
@ 2018-07-11 12:29 ` Adrian Hunter
2018-07-11 13:04 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2018-07-11 12:29 UTC (permalink / raw)
To: Stefan Agner, ulf.hansson
Cc: fabio.estevam, haibo.chen, aisheng.dong, linux-mmc, linux-kernel
On 05/07/18 14:39, Stefan Agner wrote:
> In the uSDHC case (e.g. i.MX 6) clocks only get disabled if frequency
> is set to 0. However, it could be that the stack asks for a frequency
> change while clocks are on. In that case the function clears the
> divider registers (by clearing ESDHC_CLOCK_MASK) while the clock is
> enabled! This causes a short period of time where the clock is
> undivided (on a i.MX 6DL a clock of 196MHz has been measured).
>
> For older IP variants the driver disables clock by clearing some bits
> in ESDHC_SYSTEM_CONTROL.
>
> Make sure to disable card clock before chainging frequency for uSDHC
chainging -> changing
> IP variants too. Also fix indent to make disable/enable clock look
> alike.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 85fd5a8b0b6d..aa48f4b2541a 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -708,14 +708,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
> int div = 1;
> u32 temp, val;
>
> + if (esdhc_is_usdhc(imx_data)) {
> + val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> + writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> + host->ioaddr + ESDHC_VENDOR_SPEC);
> + }
> +
> if (clock == 0) {
> host->mmc->actual_clock = 0;
> -
> - if (esdhc_is_usdhc(imx_data)) {
> - val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> - writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> - host->ioaddr + ESDHC_VENDOR_SPEC);
> - }
> return;
> }
>
> @@ -761,7 +761,7 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
> if (esdhc_is_usdhc(imx_data)) {
> val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> - host->ioaddr + ESDHC_VENDOR_SPEC);
> + host->ioaddr + ESDHC_VENDOR_SPEC);
Ideally each patch does only 1 thing. Please make white-space fix-up a
separate patch.
> }
>
> mdelay(1);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency
2018-07-11 12:29 ` Adrian Hunter
@ 2018-07-11 13:04 ` Ulf Hansson
0 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2018-07-11 13:04 UTC (permalink / raw)
To: Adrian Hunter, Stefan Agner
Cc: Fabio Estevam, Haibo Chen, Aisheng Dong, linux-mmc,
Linux Kernel Mailing List
On 11 July 2018 at 14:29, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 05/07/18 14:39, Stefan Agner wrote:
>> In the uSDHC case (e.g. i.MX 6) clocks only get disabled if frequency
>> is set to 0. However, it could be that the stack asks for a frequency
>> change while clocks are on. In that case the function clears the
>> divider registers (by clearing ESDHC_CLOCK_MASK) while the clock is
>> enabled! This causes a short period of time where the clock is
>> undivided (on a i.MX 6DL a clock of 196MHz has been measured).
>>
>> For older IP variants the driver disables clock by clearing some bits
>> in ESDHC_SYSTEM_CONTROL.
>>
>> Make sure to disable card clock before chainging frequency for uSDHC
>
> chainging -> changing
>
>> IP variants too. Also fix indent to make disable/enable clock look
>> alike.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 85fd5a8b0b6d..aa48f4b2541a 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -708,14 +708,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
>> int div = 1;
>> u32 temp, val;
>>
>> + if (esdhc_is_usdhc(imx_data)) {
>> + val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> + writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
>> + host->ioaddr + ESDHC_VENDOR_SPEC);
>> + }
>> +
>> if (clock == 0) {
>> host->mmc->actual_clock = 0;
>> -
>> - if (esdhc_is_usdhc(imx_data)) {
>> - val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> - writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
>> - host->ioaddr + ESDHC_VENDOR_SPEC);
>> - }
>> return;
>> }
>>
>> @@ -761,7 +761,7 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
>> if (esdhc_is_usdhc(imx_data)) {
>> val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
>> - host->ioaddr + ESDHC_VENDOR_SPEC);
>> + host->ioaddr + ESDHC_VENDOR_SPEC);
>
> Ideally each patch does only 1 thing. Please make white-space fix-up a
> separate patch.
>
>> }
>>
>> mdelay(1);
>>
>
It turned out that I had already applied this one. I am going to
re-base my next branch next week, so dropping the patch by then.
Until then, the patch is reverted.
Stefan, please send a new version addressing Adrian's comments.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency
2018-07-05 13:09 ` Ulf Hansson
@ 2018-07-05 14:26 ` Stefan Agner
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Agner @ 2018-07-05 14:26 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Adrian Hunter, marcel, linux-mmc, Linux Kernel Mailing List
On 05.07.2018 15:09, Ulf Hansson wrote:
> On 5 July 2018 at 14:15, Stefan Agner <stefan@agner.ch> wrote:
>> In the uSDHC case (e.g. i.MX 6) clocks only get disabled if frequency
>> is set to 0. However, it could be that the stack asks for a frequency
>> change while clocks are on. In that case the function clears the
>> divider registers (by clearing ESDHC_CLOCK_MASK) while the clock is
>> enabled! This causes a short period of time where the clock is
>> undivided (on a i.MX 6DL a clock of 196MHz has been measured).
>>
>> For older IP variants the driver disables clock by clearing some bits
>> in ESDHC_SYSTEM_CONTROL.
>>
>> Make sure to disable card clock before chainging frequency for uSDHC
>> IP variants too. Also fix indent to make disable/enable clock look
>> alike.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>
> Thanks, applied for next!
>
> Please tell if you want this for fixes and if I should add a stable tag.
I haven't seen any direct negative impact by that, so I don't think its
worth back-porting at this point.
--
Stefan
>
> Kind regards
> Uffe
>
>> ---
>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 85fd5a8b0b6d..aa48f4b2541a 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -708,14 +708,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
>> int div = 1;
>> u32 temp, val;
>>
>> + if (esdhc_is_usdhc(imx_data)) {
>> + val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> + writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
>> + host->ioaddr + ESDHC_VENDOR_SPEC);
>> + }
>> +
>> if (clock == 0) {
>> host->mmc->actual_clock = 0;
>> -
>> - if (esdhc_is_usdhc(imx_data)) {
>> - val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> - writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
>> - host->ioaddr + ESDHC_VENDOR_SPEC);
>> - }
>> return;
>> }
>>
>> @@ -761,7 +761,7 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
>> if (esdhc_is_usdhc(imx_data)) {
>> val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
>> - host->ioaddr + ESDHC_VENDOR_SPEC);
>> + host->ioaddr + ESDHC_VENDOR_SPEC);
>> }
>>
>> mdelay(1);
>> --
>> 2.18.0
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency
2018-07-05 12:15 Stefan Agner
@ 2018-07-05 13:09 ` Ulf Hansson
2018-07-05 14:26 ` Stefan Agner
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2018-07-05 13:09 UTC (permalink / raw)
To: Stefan Agner; +Cc: Adrian Hunter, marcel, linux-mmc, Linux Kernel Mailing List
On 5 July 2018 at 14:15, Stefan Agner <stefan@agner.ch> wrote:
> In the uSDHC case (e.g. i.MX 6) clocks only get disabled if frequency
> is set to 0. However, it could be that the stack asks for a frequency
> change while clocks are on. In that case the function clears the
> divider registers (by clearing ESDHC_CLOCK_MASK) while the clock is
> enabled! This causes a short period of time where the clock is
> undivided (on a i.MX 6DL a clock of 196MHz has been measured).
>
> For older IP variants the driver disables clock by clearing some bits
> in ESDHC_SYSTEM_CONTROL.
>
> Make sure to disable card clock before chainging frequency for uSDHC
> IP variants too. Also fix indent to make disable/enable clock look
> alike.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Thanks, applied for next!
Please tell if you want this for fixes and if I should add a stable tag.
Kind regards
Uffe
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 85fd5a8b0b6d..aa48f4b2541a 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -708,14 +708,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
> int div = 1;
> u32 temp, val;
>
> + if (esdhc_is_usdhc(imx_data)) {
> + val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> + writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> + host->ioaddr + ESDHC_VENDOR_SPEC);
> + }
> +
> if (clock == 0) {
> host->mmc->actual_clock = 0;
> -
> - if (esdhc_is_usdhc(imx_data)) {
> - val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> - writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> - host->ioaddr + ESDHC_VENDOR_SPEC);
> - }
> return;
> }
>
> @@ -761,7 +761,7 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
> if (esdhc_is_usdhc(imx_data)) {
> val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> - host->ioaddr + ESDHC_VENDOR_SPEC);
> + host->ioaddr + ESDHC_VENDOR_SPEC);
> }
>
> mdelay(1);
> --
> 2.18.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency
@ 2018-07-05 12:15 Stefan Agner
2018-07-05 13:09 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2018-07-05 12:15 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson; +Cc: marcel, linux-mmc, linux-kernel, Stefan Agner
In the uSDHC case (e.g. i.MX 6) clocks only get disabled if frequency
is set to 0. However, it could be that the stack asks for a frequency
change while clocks are on. In that case the function clears the
divider registers (by clearing ESDHC_CLOCK_MASK) while the clock is
enabled! This causes a short period of time where the clock is
undivided (on a i.MX 6DL a clock of 196MHz has been measured).
For older IP variants the driver disables clock by clearing some bits
in ESDHC_SYSTEM_CONTROL.
Make sure to disable card clock before chainging frequency for uSDHC
IP variants too. Also fix indent to make disable/enable clock look
alike.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 85fd5a8b0b6d..aa48f4b2541a 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -708,14 +708,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
int div = 1;
u32 temp, val;
+ if (esdhc_is_usdhc(imx_data)) {
+ val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+ writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
+ host->ioaddr + ESDHC_VENDOR_SPEC);
+ }
+
if (clock == 0) {
host->mmc->actual_clock = 0;
-
- if (esdhc_is_usdhc(imx_data)) {
- val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
- writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
- host->ioaddr + ESDHC_VENDOR_SPEC);
- }
return;
}
@@ -761,7 +761,7 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
if (esdhc_is_usdhc(imx_data)) {
val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
- host->ioaddr + ESDHC_VENDOR_SPEC);
+ host->ioaddr + ESDHC_VENDOR_SPEC);
}
mdelay(1);
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-11 13:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-05 11:39 [PATCH] mmc: sdhci-esdhc-imx: disable clocks before changing frequency Stefan Agner
2018-07-11 12:29 ` Adrian Hunter
2018-07-11 13:04 ` Ulf Hansson
2018-07-05 12:15 Stefan Agner
2018-07-05 13:09 ` Ulf Hansson
2018-07-05 14:26 ` Stefan Agner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).