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 <[email protected]>
---
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
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 <[email protected]>
---
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
On 10 November 2015 at 10:43, Haibo Chen <[email protected]> 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 <[email protected]>
> ---
> 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
On 10 November 2015 at 10:43, Haibo Chen <[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Nov 19, 2015 at 7:17 PM, Ulf Hansson <[email protected]> wrote:
> On 10 November 2015 at 10:43, Haibo Chen <[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf,
On Thu, Nov 19, 2015 at 7:20 PM, Ulf Hansson <[email protected]> wrote:
> On 10 November 2015 at 10:43, Haibo Chen <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Regards
Dong Aisheng
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 November 2015 at 17:34, Dong Aisheng <[email protected]> wrote:
> Hi Ulf,
>
> On Thu, Nov 19, 2015 at 7:20 PM, Ulf Hansson <[email protected]> wrote:
>> On 10 November 2015 at 10:43, Haibo Chen <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
Okay. I have applied it for next, thanks!
Kind regards
Uffe