mmc: sdhci: Disable re-tuning for HS400
Adrian Hunter
maintainercover mmc: sdhci: Disable re-tuning for HS400
Hi
As described in patch 3, re-tuning for HS400 has to be done in HS200 mode, but there is no support for that, so re-tuning needs to be disabled until support is added. I have some patches for that which I will send in the next day or two.
Adrian Hunter (3):
mmc: sdhci: Tuning should not change max_blk_count
mmc: sdhci: Add out_unlock to sdhci_execute_tuning
mmc: sdhci: Disable re-tuning for HS400
Regards
Adrian
Adrian Hunter
maintainerPATCH 1/3 mmc: sdhci: Tuning should not change max_blk_count
Re-tuning requires that the maximum data length is limited to 4MiB. The code currently changes max_blk_count in an attempt to achieve that. This is wrong because max_blk_count is a different limit, but it is also un-necessary because max_req_size is 512KiB anyway. Consequently, the changes to max_blk_count are removed and the comment for max_req_size adjusted accordingly. The comment is also tweaked to show that the 512KiB limit is a SDMA limit not an ADMA limit.
--- drivers/mmc/host/sdhci.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/mmc/host/sdhci.c +3 -6
@@ -259,8 +259,6 @@ static void sdhci_reinit(struct sdhci_host *host)del_timer_sync (&host->tuning_timer); host->flags &= ~SDHCI_NEEDS_RETUNING ; - host->mmc->max_blk_count = - (host->quirks &SDHCI_QUIRK_NO_MULTIBLOCK ) ?1 :65535 ; }sdhci_enable_card_detection (host); }
@@ -2048,8 +2046,6 @@ out: host->flags |=SDHCI_USING_RETUNING_TIMER ;mod_timer (&host->tuning_timer, jiffies + host->tuning_count *HZ ); -/* Tuning mode 1 limits the maximum data length to 4MB */ - mmc->max_blk_count = (4 *1024 *1024 ) / mmc->max_blk_size; }else if (host->flags &SDHCI_USING_RETUNING_TIMER ) { host->flags &= ~SDHCI_NEEDS_RETUNING ;/* Reload the new initial value for timer */
@@ -3263,8 +3259,9 @@ int sdhci_add_host(struct sdhci_host *host) mmc->max_segs =SDHCI_MAX_SEGS ;/* -* Maximum number of sectors in one transfer. Limited by DMA boundary -* size (512KiB). +* Maximum number of sectors in one transfer. Limited by SDMA boundary +* size (512KiB). Note some tuning modes impose a 4MiB limit, but this +* is less anyway. */ mmc->max_req_size =524288 ;
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 2/3 mmc: sdhci: Add out_unlock to sdhci_execute_tuning
In reply to Adrian HunterA 'goto' can be used to save duplicating unlocking and returning.
--- drivers/mmc/host/sdhci.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/mmc/host/sdhci.c +2 -3
@@ -1909,9 +1909,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)/* FALLTHROUGH */ default : -spin_unlock_irqrestore (&host->lock, flags); -sdhci_runtime_pm_put (host); -return 0 ; + goto out_unlock; }if (host->ops->platform_execute_tuning) {
@@ -2066,6 +2064,7 @@ out:sdhci_writel (host, host->ier,SDHCI_INT_ENABLE );sdhci_writel (host, host->ier,SDHCI_SIGNAL_ENABLE ); +out_unlock:spin_unlock_irqrestore (&host->lock, flags);sdhci_runtime_pm_put (host);
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 3/3 mmc: sdhci: Disable re-tuning for HS400
In reply to Adrian HunterRe-tuning for HS400 mode must be done in HS200 mode. Currently there is no support for that. That needs to be reflected in the code. Specifically, if tuning is executed in HS400 mode then return an error, and if the re-tuning timer is running when switching to HS400 mode, then disable the timer.
Note that periodic re-tuning is not expected to be needed for HS400 but re-tuning is still needed after the host controller has lost power. In the case of suspend/resume that is not necessary because the card is fully re-initialised. That just leaves runtime suspend/resume with no support for HS400 re-tuning.
--- drivers/mmc/host/sdhci.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci.c +15 -1
@@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)else if ((timing ==MMC_TIMING_UHS_DDR50 ) || (timing ==MMC_TIMING_MMC_DDR52 )) ctrl_2 |=SDHCI_CTRL_UHS_DDR50 ; -else if (timing ==MMC_TIMING_MMC_HS400 ) +else if (timing ==MMC_TIMING_MMC_HS400 ) { ctrl_2 |=SDHCI_CTRL_HS400 ;/* Non-standard */ +/* +* Periodic re-tuning for HS400 is not expected to be needed, so +* disable it here. +*/ +if (host->flags &SDHCI_USING_RETUNING_TIMER ) { + host->flags &= ~SDHCI_USING_RETUNING_TIMER ; +del_timer_sync (&host->tuning_timer); + host->flags &= ~SDHCI_NEEDS_RETUNING ; + } + }sdhci_writew (host, ctrl_2,SDHCI_HOST_CONTROL2 ); }EXPORT_SYMBOL_GPL (sdhci_set_uhs_signaling);
@@ -1897,7 +1907,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)* tuning function has to be executed. */ switch (host->timing) { +/* HS400 tuning is done in HS200 mode */ case MMC_TIMING_MMC_HS400 : + err = -EINVAL ; + goto out_unlock; +case MMC_TIMING_MMC_HS200 :case MMC_TIMING_UHS_SDR104 :break ;
Signature · 1 line
1.9.1
Quoted 11 lines · Adrian Hunter
Re-tuning for HS400 mode must be done in HS200 mode. Currently there is no support for that. That needs to be reflected in the code. Specifically, if tuning is executed in HS400 mode then return an error, and if the re-tuning timer is running when switching to HS400 mode, then disable the timer.
Note that periodic re-tuning is not expected to be needed for HS400 but re-tuning is still needed after the host controller has lost power.
Why can't the old values be restored instead of trigger a re-tuning?
Quoted 15 lines
In the case of suspend/resume that is not necessary because the card is fully re-initialised. That just leaves runtime suspend/resume with no support for HS400 re-tuning.
Signed-off-by Adrian Hunter <[email protected]>--- drivers/mmc/host/sdhci.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)drivers/mmc/host/sdhci.c +0
-0
drivers/mmc/host/sdhci.c void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing) · line 1483
Quoted 9 lines
drivers/mmc/host/sdhci.c +4
-1@@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)else if ((timing ==MMC_TIMING_UHS_DDR50 ) || (timing ==MMC_TIMING_MMC_DDR52 )) ctrl_2 |=SDHCI_CTRL_UHS_DDR50 ; -else if (timing ==MMC_TIMING_MMC_HS400 ) +else if (timing ==MMC_TIMING_MMC_HS400 ) { ctrl_2 |=SDHCI_CTRL_HS400 ;/* Non-standard */ +/* +* Periodic re-tuning for HS400 is not expected to be needed, so +* disable it here.
Urgh, I don't like that the periodic tuning is handled by the host. We should never had merged that.
How about trying to move the periodic tuning to be handled by the mmc core instead?
Quoted 23 lines
drivers/mmc/host/sdhci.c +11
-1+*/ +if (host->flags &SDHCI_USING_RETUNING_TIMER ) { + host->flags &= ~SDHCI_USING_RETUNING_TIMER ; +del_timer_sync (&host->tuning_timer); + host->flags &= ~SDHCI_NEEDS_RETUNING ; + } + }sdhci_writew (host, ctrl_2,SDHCI_HOST_CONTROL2 ); }EXPORT_SYMBOL_GPL (sdhci_set_uhs_signaling);@@ -1897,7 +1907,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)* tuning function has to be executed. */ switch (host->timing) { +/* HS400 tuning is done in HS200 mode */ case MMC_TIMING_MMC_HS400 : + err = -EINVAL ; + goto out_unlock; +case MMC_TIMING_MMC_HS200 :case MMC_TIMING_UHS_SDR104 :break ; --
Quoted 2 lines
1.9.1
Kind regards
Uffe
Adrian Hunter
maintainerQuoted 9 lines · Ulf Hansson
Quoted 8 lines · Adrian Hunter
Re-tuning for HS400 mode must be done in HS200 mode. Currently there is no support for that. That needs to be reflected in the code. Specifically, if tuning is executed in HS400 mode then return an error, and if the re-tuning timer is running when switching to HS400 mode, then disable the timer.
Quoted 5 lines
Quoted 3 lines
Note that periodic re-tuning is not expected to be needed for HS400 but re-tuning is still needed after the host controller has lost power.
Why can't the old values be restored instead of trigger a re-tuning?
The "values" (not sure what you mean by that) are not available to the driver. Even if they were the operating conditions may have changed, (i.e. temperature change) so the old "values" could still be wrong.
Jedec spec. says:
It is recommended to perform tuning procedure while Device wakes up, after sleep.
SDHCI spec. says:
If the Host System goes into power down mode, the Host Driver should stop the re-tuning timer and set the expiration flag to 1 when the Host System resumes from power down mode.
Quoted 29 lines
Quoted 16 lines
In the case of suspend/resume that is not necessary because the card is fully re-initialised. That just leaves runtime suspend/resume with no support for HS400 re-tuning.
Signed-off-by Adrian Hunter <[email protected]>--- drivers/mmc/host/sdhci.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)drivers/mmc/host/sdhci.c +0
-0drivers/mmc/host/sdhci.c void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing) · line 1483
Quoted 9 lines
drivers/mmc/host/sdhci.c +4
-1@@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)else if ((timing ==MMC_TIMING_UHS_DDR50 ) || (timing ==MMC_TIMING_MMC_DDR52 )) ctrl_2 |=SDHCI_CTRL_UHS_DDR50 ; -else if (timing ==MMC_TIMING_MMC_HS400 ) +else if (timing ==MMC_TIMING_MMC_HS400 ) { ctrl_2 |=SDHCI_CTRL_HS400 ;/* Non-standard */ +/* +* Periodic re-tuning for HS400 is not expected to be needed, so +* disable it here. Urgh, I don't like that the periodic tuning is handled by the host. We should never had merged that.
Quoted 2 lines
How about trying to move the periodic tuning to be handled by the mmc core instead?
I have patches for that. I hope to send them today.
Quoted 31 lines
Quoted 24 lines
drivers/mmc/host/sdhci.c +11
-1+*/ +if (host->flags &SDHCI_USING_RETUNING_TIMER ) { + host->flags &= ~SDHCI_USING_RETUNING_TIMER ; +del_timer_sync (&host->tuning_timer); + host->flags &= ~SDHCI_NEEDS_RETUNING ; + } + }sdhci_writew (host, ctrl_2,SDHCI_HOST_CONTROL2 ); }EXPORT_SYMBOL_GPL (sdhci_set_uhs_signaling);@@ -1897,7 +1907,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)* tuning function has to be executed. */ switch (host->timing) { +/* HS400 tuning is done in HS200 mode */ case MMC_TIMING_MMC_HS400 : + err = -EINVAL ; + goto out_unlock; +case MMC_TIMING_MMC_HS200 :case MMC_TIMING_UHS_SDR104 :break ; --Quoted 2 lines
1.9.1
Kind regards
Uffe
Quoted 14 lines · Adrian Hunter
Quoted 13 lines · Ulf Hansson
Quoted 11 lines · Adrian Hunter
Re-tuning for HS400 mode must be done in HS200 mode. Currently there is no support for that. That needs to be reflected in the code. Specifically, if tuning is executed in HS400 mode then return an error, and if the re-tuning timer is running when switching to HS400 mode, then disable the timer.
Note that periodic re-tuning is not expected to be needed for HS400 but re-tuning is still needed after the host controller has lost power.
Quoted 5 lines
Quoted 1 line
Why can't the old values be restored instead of trigger a re-tuning?
The "values" (not sure what you mean by that) are not available to the driver. Even if they were the operating conditions may have changed, (i.e. temperature change) so the old "values" could still be wrong.
The "values" I refer to is those which we "calculated" during the tuning process.
What I had in mind, was that we should save these values at runtime PM suspend. And restore them at runtime PM resume. For some mmc controllers the "values" are typically just a some bits in a controller register, but that might not be true for all cases.
Regarding the temperature change, etc. I think that is what the periodic retuning should be taken care off.
Could you elaborate on why the "values" is not available to the driver?
Quoted 11 lines
Jedec spec. says:
It is recommended to perform tuning procedure while Device wakes up, after sleep.
SDHCI spec. says:
If the Host System goes into power down mode, the Host Driver should stop the re-tuning timer and set the expiration flag to 1 when the Host System resumes from power down mode.
I am not sure how to interpret this. Is the context about system PM or runtime PM?
Quoted 30 lines
Quoted 30 lines
Quoted 17 lines
In the case of suspend/resume that is not necessary because the card is fully re-initialised. That just leaves runtime suspend/resume with no support for HS400 re-tuning.
Signed-off-by Adrian Hunter <[email protected]>--- drivers/mmc/host/sdhci.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)drivers/mmc/host/sdhci.c +0
-0drivers/mmc/host/sdhci.c void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing) · line 1483
Quoted 9 lines
drivers/mmc/host/sdhci.c +4
-1@@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)else if ((timing ==MMC_TIMING_UHS_DDR50 ) || (timing ==MMC_TIMING_MMC_DDR52 )) ctrl_2 |=SDHCI_CTRL_UHS_DDR50 ; -else if (timing ==MMC_TIMING_MMC_HS400 ) +else if (timing ==MMC_TIMING_MMC_HS400 ) { ctrl_2 |=SDHCI_CTRL_HS400 ;/* Non-standard */ +/* +* Periodic re-tuning for HS400 is not expected to be needed, so +* disable it here. Urgh, I don't like that the periodic tuning is handled by the host. We should never had merged that.
Quoted 4 lines
Quoted 2 lines
How about trying to move the periodic tuning to be handled by the mmc core instead?
I have patches for that. I hope to send them today.
Great! Looking forward to review them!
Kind regards
Uffe
Adrian Hunter
maintainerQuoted 29 lines · Ulf Hansson
Quoted 14 lines · Adrian Hunter
Quoted 13 lines · Ulf Hansson
Quoted 11 lines · Adrian Hunter
Re-tuning for HS400 mode must be done in HS200 mode. Currently there is no support for that. That needs to be reflected in the code. Specifically, if tuning is executed in HS400 mode then return an error, and if the re-tuning timer is running when switching to HS400 mode, then disable the timer.
Note that periodic re-tuning is not expected to be needed for HS400 but re-tuning is still needed after the host controller has lost power.
Quoted 5 lines
Quoted 1 line
Why can't the old values be restored instead of trigger a re-tuning?
The "values" (not sure what you mean by that) are not available to the driver. Even if they were the operating conditions may have changed, (i.e. temperature change) so the old "values" could still be wrong.
The "values" I refer to is those which we "calculated" during the tuning process.
What I had in mind, was that we should save these values at runtime PM suspend. And restore them at runtime PM resume. For some mmc controllers the "values" are typically just a some bits in a controller register, but that might not be true for all cases.
Quoted 4 lines
Regarding the temperature change, etc. I think that is what the periodic retuning should be taken care off.
Could you elaborate on why the "values" is not available to the driver?
The "optimal sampling point" has no corresponding register or value in SDHCI.
Quoted 7 lines
Quoted 7 lines
Jedec spec. says:
It is recommended to perform tuning procedure while Device wakes up, after sleep.
Quoted 8 lines
Quoted 5 lines
SDHCI spec. says:
If the Host System goes into power down mode, the Host Driver should stop the re-tuning timer and set the expiration flag to 1 when the Host System resumes from power down mode.
I am not sure how to interpret this. Is the context about system PM or runtime PM?
I interpret it to mean any situation where the host controller loses power and therefore loses its tuning.
Quoted 42 lines
Quoted 31 lines
Quoted 31 lines
Quoted 18 lines
In the case of suspend/resume that is not necessary because the card is fully re-initialised. That just leaves runtime suspend/resume with no support for HS400 re-tuning.
Signed-off-by Adrian Hunter <[email protected]>--- drivers/mmc/host/sdhci.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)drivers/mmc/host/sdhci.c +0
-0drivers/mmc/host/sdhci.c void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing) · line 1483
Quoted 9 lines
drivers/mmc/host/sdhci.c +4
-1@@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)else if ((timing ==MMC_TIMING_UHS_DDR50 ) || (timing ==MMC_TIMING_MMC_DDR52 )) ctrl_2 |=SDHCI_CTRL_UHS_DDR50 ; -else if (timing ==MMC_TIMING_MMC_HS400 ) +else if (timing ==MMC_TIMING_MMC_HS400 ) { ctrl_2 |=SDHCI_CTRL_HS400 ;/* Non-standard */ +/* +* Periodic re-tuning for HS400 is not expected to be needed, so +* disable it here. Urgh, I don't like that the periodic tuning is handled by the host. We should never had merged that.
Quoted 4 lines
Quoted 2 lines
How about trying to move the periodic tuning to be handled by the mmc core instead?
I have patches for that. I hope to send them today.
Great! Looking forward to review them!
Kind regards
Uffe
Quoted 30 lines · Adrian Hunter
Quoted 29 lines · Ulf Hansson
Quoted 14 lines · Adrian Hunter
Quoted 13 lines · Ulf Hansson
Quoted 11 lines · Adrian Hunter
Re-tuning for HS400 mode must be done in HS200 mode. Currently there is no support for that. That needs to be reflected in the code. Specifically, if tuning is executed in HS400 mode then return an error, and if the re-tuning timer is running when switching to HS400 mode, then disable the timer.
Note that periodic re-tuning is not expected to be needed for HS400 but re-tuning is still needed after the host controller has lost power.
Quoted 5 lines
Quoted 1 line
Why can't the old values be restored instead of trigger a re-tuning?
The "values" (not sure what you mean by that) are not available to the driver. Even if they were the operating conditions may have changed, (i.e. temperature change) so the old "values" could still be wrong.
The "values" I refer to is those which we "calculated" during the tuning process.
What I had in mind, was that we should save these values at runtime PM suspend. And restore them at runtime PM resume. For some mmc controllers the "values" are typically just a some bits in a controller register, but that might not be true for all cases.
Quoted 7 lines
Quoted 4 lines
Regarding the temperature change, etc. I think that is what the periodic retuning should be taken care off.
Could you elaborate on why the "values" is not available to the driver?
The "optimal sampling point" has no corresponding register or value in SDHCI.
Is that really the case for all sdhci variants? For sure I am not an sdhci expert, but I just find it to be a very poor HW design.
Especially if the sdhci controller may lose power frequently, it will add a significant latency for each runtime PM resume cycle, right!?
Anyway, thanks for sharing the information.
Kind regards
Uffe