* [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe
@ 2015-11-10 9:43 Haibo Chen
2015-11-10 9:43 ` [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting Haibo Chen
2015-11-19 11:17 ` [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Ulf Hansson
0 siblings, 2 replies; 7+ messages in thread
From: Haibo Chen @ 2015-11-10 9:43 UTC (permalink / raw)
To: ulf.hansson, aisheng.dong; +Cc: haibo.chen, linux-mmc, linux-kernel
Currently, we config the watermark_level register only in probe.
This will cause the mmc write operation timeout issue after system
resume back in LPSR mode. Because in LPSR mode, after system resume
back, the watermark_level register(0x44) changes to 0x08000880, which
set the write watermark level as 0, and set the read watermark level
as 128. This value is incorrect.
This patch move the setting of watermark level register out of probe,
so after system resume back, mmc driver can set this watermark level
register back to 0x10401040.
Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 1f1582f..1508949 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -584,6 +584,14 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
mask = 0xffff & ~(ESDHC_CTRL_BUSWIDTH_MASK | ESDHC_CTRL_D3CD);
esdhc_clrset_le(host, mask, new_val, reg);
+
+ /*
+ * The imx6q/imx7d ROM code will change the default watermark
+ * level setting to something insane. Change it back here.
+ */
+ if (esdhc_is_usdhc(imx_data))
+ writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
+
return;
}
esdhc_clrset_le(host, 0xff, val, reg);
@@ -1155,13 +1163,7 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
| SDHCI_QUIRK_BROKEN_ADMA;
- /*
- * The imx6q ROM code will change the default watermark level setting
- * to something insane. Change it back here.
- */
if (esdhc_is_usdhc(imx_data)) {
- writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
-
host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
host->mmc->caps |= MMC_CAP_1_8V_DDR;
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting
2015-11-10 9:43 [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Haibo Chen
@ 2015-11-10 9:43 ` Haibo Chen
2015-11-19 11:20 ` Ulf Hansson
2015-11-19 11:17 ` [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Ulf Hansson
1 sibling, 1 reply; 7+ messages in thread
From: Haibo Chen @ 2015-11-10 9:43 UTC (permalink / raw)
To: ulf.hansson, aisheng.dong; +Cc: haibo.chen, linux-mmc, linux-kernel
Here we use '|=' to set the tuning-step, but before that, we should
clear the tuning-step, otherwise we could got the wrong setting.
Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 1508949..64275c7 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -76,6 +76,7 @@
#define ESDHC_STD_TUNING_EN (1 << 24)
/* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
#define ESDHC_TUNING_START_TAP 0x1
+#define ESDHC_TUNING_STEP_MASK 0x00070000
#define ESDHC_TUNING_STEP_SHIFT 16
/* pinctrl state */
@@ -489,9 +490,11 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
m |= ESDHC_MIX_CTRL_FBCLK_SEL;
tuning_ctrl = readl(host->ioaddr + ESDHC_TUNING_CTRL);
tuning_ctrl |= ESDHC_STD_TUNING_EN | ESDHC_TUNING_START_TAP;
- if (imx_data->boarddata.tuning_step)
+ if (imx_data->boarddata.tuning_step) {
+ tuning_ctrl &= ~ESDHC_TUNING_STEP_MASK;
tuning_ctrl |= imx_data->boarddata.tuning_step << ESDHC_TUNING_STEP_SHIFT;
- writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
+ }
+ writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
} else {
v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting
2015-11-10 9:43 ` [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting Haibo Chen
@ 2015-11-19 11:20 ` Ulf Hansson
2015-11-19 16:34 ` Dong Aisheng
0 siblings, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2015-11-19 11:20 UTC (permalink / raw)
To: Haibo Chen; +Cc: Dong Aisheng, linux-mmc, linux-kernel@vger.kernel.org
On 10 November 2015 at 10:43, Haibo Chen <haibo.chen@freescale.com> wrote:
> Here we use '|=' to set the tuning-step, but before that, we should
> clear the tuning-step, otherwise we could got the wrong setting.
>
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 1508949..64275c7 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -76,6 +76,7 @@
> #define ESDHC_STD_TUNING_EN (1 << 24)
> /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
> #define ESDHC_TUNING_START_TAP 0x1
> +#define ESDHC_TUNING_STEP_MASK 0x00070000
> #define ESDHC_TUNING_STEP_SHIFT 16
>
> /* pinctrl state */
> @@ -489,9 +490,11 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
> m |= ESDHC_MIX_CTRL_FBCLK_SEL;
> tuning_ctrl = readl(host->ioaddr + ESDHC_TUNING_CTRL);
> tuning_ctrl |= ESDHC_STD_TUNING_EN | ESDHC_TUNING_START_TAP;
> - if (imx_data->boarddata.tuning_step)
> + if (imx_data->boarddata.tuning_step) {
> + tuning_ctrl &= ~ESDHC_TUNING_STEP_MASK;
> tuning_ctrl |= imx_data->boarddata.tuning_step << ESDHC_TUNING_STEP_SHIFT;
> - writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
> + }
> + writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
> } else {
> v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
> }
> --
> 1.9.1
>
Looks good to me, but is there a dependency to patch 1/2 that should
prevent me from applying this one?
Kind regards
Uffe
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting
2015-11-19 11:20 ` Ulf Hansson
@ 2015-11-19 16:34 ` Dong Aisheng
2015-11-20 15:23 ` Ulf Hansson
0 siblings, 1 reply; 7+ messages in thread
From: Dong Aisheng @ 2015-11-19 16:34 UTC (permalink / raw)
To: Ulf Hansson
Cc: Haibo Chen, Dong Aisheng, linux-mmc, linux-kernel@vger.kernel.org
Hi Ulf,
On Thu, Nov 19, 2015 at 7:20 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 10 November 2015 at 10:43, Haibo Chen <haibo.chen@freescale.com> wrote:
>> Here we use '|=' to set the tuning-step, but before that, we should
>> clear the tuning-step, otherwise we could got the wrong setting.
>>
>> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
>> ---
>> drivers/mmc/host/sdhci-esdhc-imx.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 1508949..64275c7 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -76,6 +76,7 @@
>> #define ESDHC_STD_TUNING_EN (1 << 24)
>> /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
>> #define ESDHC_TUNING_START_TAP 0x1
>> +#define ESDHC_TUNING_STEP_MASK 0x00070000
>> #define ESDHC_TUNING_STEP_SHIFT 16
>>
>> /* pinctrl state */
>> @@ -489,9 +490,11 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>> m |= ESDHC_MIX_CTRL_FBCLK_SEL;
>> tuning_ctrl = readl(host->ioaddr + ESDHC_TUNING_CTRL);
>> tuning_ctrl |= ESDHC_STD_TUNING_EN | ESDHC_TUNING_START_TAP;
>> - if (imx_data->boarddata.tuning_step)
>> + if (imx_data->boarddata.tuning_step) {
>> + tuning_ctrl &= ~ESDHC_TUNING_STEP_MASK;
>> tuning_ctrl |= imx_data->boarddata.tuning_step << ESDHC_TUNING_STEP_SHIFT;
>> - writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
>> + }
>> + writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
>> } else {
>> v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
>> }
>> --
>> 1.9.1
>>
>
> Looks good to me, but is there a dependency to patch 1/2 that should
> prevent me from applying this one?
>
No dependency.
Acked-by: Dong Aisheng <aisheng.dong@freescale.com>
Regards
Dong Aisheng
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting
2015-11-19 16:34 ` Dong Aisheng
@ 2015-11-20 15:23 ` Ulf Hansson
0 siblings, 0 replies; 7+ messages in thread
From: Ulf Hansson @ 2015-11-20 15:23 UTC (permalink / raw)
To: Dong Aisheng, Haibo Chen
Cc: Dong Aisheng, linux-mmc, linux-kernel@vger.kernel.org
On 19 November 2015 at 17:34, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Ulf,
>
> On Thu, Nov 19, 2015 at 7:20 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 10 November 2015 at 10:43, Haibo Chen <haibo.chen@freescale.com> wrote:
>>> Here we use '|=' to set the tuning-step, but before that, we should
>>> clear the tuning-step, otherwise we could got the wrong setting.
>>>
>>> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
>>> ---
>>> drivers/mmc/host/sdhci-esdhc-imx.c | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> index 1508949..64275c7 100644
>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> @@ -76,6 +76,7 @@
>>> #define ESDHC_STD_TUNING_EN (1 << 24)
>>> /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
>>> #define ESDHC_TUNING_START_TAP 0x1
>>> +#define ESDHC_TUNING_STEP_MASK 0x00070000
>>> #define ESDHC_TUNING_STEP_SHIFT 16
>>>
>>> /* pinctrl state */
>>> @@ -489,9 +490,11 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>>> m |= ESDHC_MIX_CTRL_FBCLK_SEL;
>>> tuning_ctrl = readl(host->ioaddr + ESDHC_TUNING_CTRL);
>>> tuning_ctrl |= ESDHC_STD_TUNING_EN | ESDHC_TUNING_START_TAP;
>>> - if (imx_data->boarddata.tuning_step)
>>> + if (imx_data->boarddata.tuning_step) {
>>> + tuning_ctrl &= ~ESDHC_TUNING_STEP_MASK;
>>> tuning_ctrl |= imx_data->boarddata.tuning_step << ESDHC_TUNING_STEP_SHIFT;
>>> - writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
>>> + }
>>> + writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
>>> } else {
>>> v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
>>> }
>>> --
>>> 1.9.1
>>>
>>
>> Looks good to me, but is there a dependency to patch 1/2 that should
>> prevent me from applying this one?
>>
>
> No dependency.
> Acked-by: Dong Aisheng <aisheng.dong@freescale.com>
>
Okay. I have applied it for next, thanks!
Kind regards
Uffe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe
2015-11-10 9:43 [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Haibo Chen
2015-11-10 9:43 ` [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting Haibo Chen
@ 2015-11-19 11:17 ` Ulf Hansson
2015-11-19 16:29 ` Dong Aisheng
1 sibling, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2015-11-19 11:17 UTC (permalink / raw)
To: Haibo Chen; +Cc: Dong Aisheng, linux-mmc, linux-kernel@vger.kernel.org
On 10 November 2015 at 10:43, Haibo Chen <haibo.chen@freescale.com> wrote:
> Currently, we config the watermark_level register only in probe.
> This will cause the mmc write operation timeout issue after system
> resume back in LPSR mode. Because in LPSR mode, after system resume
> back, the watermark_level register(0x44) changes to 0x08000880, which
> set the write watermark level as 0, and set the read watermark level
> as 128. This value is incorrect.
>
> This patch move the setting of watermark level register out of probe,
> so after system resume back, mmc driver can set this watermark level
> register back to 0x10401040.
This seems all reasonable! But...
>
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 1f1582f..1508949 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -584,6 +584,14 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
> mask = 0xffff & ~(ESDHC_CTRL_BUSWIDTH_MASK | ESDHC_CTRL_D3CD);
>
> esdhc_clrset_le(host, mask, new_val, reg);
> +
> + /*
> + * The imx6q/imx7d ROM code will change the default watermark
> + * level setting to something insane. Change it back here.
> + */
> + if (esdhc_is_usdhc(imx_data))
> + writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
> +
... why don't keep setting the watermark in ->probe() and instead just
add it at as additional task to do at system PM resume?
> return;
> }
> esdhc_clrset_le(host, 0xff, val, reg);
> @@ -1155,13 +1163,7 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
> host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
> | SDHCI_QUIRK_BROKEN_ADMA;
>
> - /*
> - * The imx6q ROM code will change the default watermark level setting
> - * to something insane. Change it back here.
> - */
> if (esdhc_is_usdhc(imx_data)) {
> - writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
> -
> host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> host->mmc->caps |= MMC_CAP_1_8V_DDR;
>
> --
> 1.9.1
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe
2015-11-19 11:17 ` [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Ulf Hansson
@ 2015-11-19 16:29 ` Dong Aisheng
0 siblings, 0 replies; 7+ messages in thread
From: Dong Aisheng @ 2015-11-19 16:29 UTC (permalink / raw)
To: Ulf Hansson
Cc: Haibo Chen, Dong Aisheng, linux-mmc, linux-kernel@vger.kernel.org
On Thu, Nov 19, 2015 at 7:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 10 November 2015 at 10:43, Haibo Chen <haibo.chen@freescale.com> wrote:
>> Currently, we config the watermark_level register only in probe.
>> This will cause the mmc write operation timeout issue after system
>> resume back in LPSR mode. Because in LPSR mode, after system resume
>> back, the watermark_level register(0x44) changes to 0x08000880, which
>> set the write watermark level as 0, and set the read watermark level
>> as 128. This value is incorrect.
>>
>> This patch move the setting of watermark level register out of probe,
>> so after system resume back, mmc driver can set this watermark level
>> register back to 0x10401040.
>
> This seems all reasonable! But...
>
>>
>> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
>> ---
>> drivers/mmc/host/sdhci-esdhc-imx.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 1f1582f..1508949 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -584,6 +584,14 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
>> mask = 0xffff & ~(ESDHC_CTRL_BUSWIDTH_MASK | ESDHC_CTRL_D3CD);
>>
>> esdhc_clrset_le(host, mask, new_val, reg);
>> +
>> + /*
>> + * The imx6q/imx7d ROM code will change the default watermark
>> + * level setting to something insane. Change it back here.
>> + */
>> + if (esdhc_is_usdhc(imx_data))
>> + writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
>> +
>
> ... why don't keep setting the watermark in ->probe() and instead just
> add it at as additional task to do at system PM resume?
>
I guess the reason may be keep using common sdhci_pltfrm_suspend() function
instead of creating new one.
But i think it's a good suggestion that we can create a platform
specific suspend/resume
function and put pm stuff into there for better code organization.
Haibo, you may try Ulf's suggestion.
Regards
Dong Aisheng
>> return;
>> }
>> esdhc_clrset_le(host, 0xff, val, reg);
>> @@ -1155,13 +1163,7 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>> host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK
>> | SDHCI_QUIRK_BROKEN_ADMA;
>>
>> - /*
>> - * The imx6q ROM code will change the default watermark level setting
>> - * to something insane. Change it back here.
>> - */
>> if (esdhc_is_usdhc(imx_data)) {
>> - writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
>> -
>> host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>> host->mmc->caps |= MMC_CAP_1_8V_DDR;
>>
>> --
>> 1.9.1
>>
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-20 15:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-10 9:43 [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Haibo Chen
2015-11-10 9:43 ` [PATCH 2/2] mmc: sdhci-esdhc-imx: correct the tuning-step setting Haibo Chen
2015-11-19 11:20 ` Ulf Hansson
2015-11-19 16:34 ` Dong Aisheng
2015-11-20 15:23 ` Ulf Hansson
2015-11-19 11:17 ` [PATCH 1/2] mmc: sdhci-esdhc-imx: move the setting of watermark level out of probe Ulf Hansson
2015-11-19 16:29 ` Dong Aisheng
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).