| Message ID | 20160829120533.GA8647@verge.net.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: > > ... > >> >> >> I am wondering whether it would it be possible to keep a cache of the >> >> >> currently used tuning values and then restore these values at >> >> >> runtime_resume? >> >> >> >> >> >> In that way you wouldn't need to force new re-tuning cycle here. >> >> > >> >> > I will check with the hardware people to see if it is practical from >> >> > that POV. >> >> >> >> Okay! >> > >> > The feedback I received is something like this: >> > >> > * The tap values calculated during retuning depend on the temperature of >> > the SoC and card. >> > * So after resume the values may be invalid. >> >> They values may also become invalid during long continues transfers, >> which prevents the ->runtime_suspend() callback to be invoked - >> because the temperature may increase. >> >> > * It may be possible to re-use the TAP values and re-tune if a transfer >> > fails but the people I spoke with were unsure of the merit of that >> > approach >> >> There's is a huge benefit! >> >> You will get a significant decreased latency at ->runtime_resume() as >> you don't need to run a complete re-tuning cycle. >> >> I can't really give you fresh numbers as it's a long time I tried this >> myself, although if you did some measurements on this it would be >> nice! :-) >> >> > >> > Personally my feeling is that we should initiate a retune on resume for >> > now as that seems to be the safest option. >> >> I don't agree. :-) I think it's better to postpone re-tune until it's >> actually needed. >> >> Re-tune happens in the request error handling path, but also if you >> configure a re-tuning period. That ought to be sufficient, don't you >> think? > > Hi Ulf, > > Is something like this close to what you have in mind? > > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c > index 94e79449c533..fc43cf5ce958 100644 > --- a/drivers/mmc/host/sh_mobile_sdhi.c > +++ b/drivers/mmc/host/sh_mobile_sdhi.c > @@ -357,11 +357,9 @@ static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host, > > #define SH_MOBILE_SDHI_MAX_TAP 3 > > -static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, > - bool *tap, int tap_size) > +static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host) > { > struct sh_mobile_sdhi *priv = host_to_priv(host); > - unsigned long tap_num; /* total number of taps */ > unsigned long tap_cnt; /* counter of tuning success */ > unsigned long tap_set; /* tap position */ > unsigned long tap_start;/* start position of tuning success */ > @@ -372,14 +370,6 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, > /* Clear SCC_RVSREQ */ > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0); > > - /* Select SCC */ > - tap_num = (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >> > - SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) & > - SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK; > - > - if (tap_num * 2 != tap_size) > - return -EINVAL; > - > /* > * Find the longest consecutive run of successful probes. If that > * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the > @@ -389,8 +379,8 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, > ntap = 0; > tap_start = 0; > tap_end = 0; > - for (i = 0; i < tap_num * 2; i++) { > - if (tap[i] == 0) > + for (i = 0; i < host->tap_num * 2; i++) { > + if (test_bit(i, host->taps)) > ntap++; > else { > if (ntap > tap_cnt) { > @@ -409,7 +399,7 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, > } > > if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP) > - tap_set = (tap_start + tap_end) / 2 % tap_num; > + tap_set = (tap_start + tap_end) / 2 % host->tap_num; > else > return -EIO; > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index c932fe876690..4186045cce0d 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -171,8 +171,12 @@ struct tmio_mmc_host { > /* Mandatory callback for tuning to occur which is > * optional for SDR50 and mandatory for SDR104 */ > unsigned int (*init_tuning)(struct tmio_mmc_host *host); > - int (*select_tuning)(struct tmio_mmc_host *host, bool *tap, > - int tap_size); > + int (*select_tuning)(struct tmio_mmc_host *host); > + > + /* Tuning values: 1 for success, 0 for failure */ > + DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); > + unsigned int tap_num; > + bool use_saved_taps; > }; > > struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev); > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index ded8fa53e0a0..2b5c8b090ed3 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -773,43 +773,50 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc) > static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > { > struct tmio_mmc_host *host = mmc_priv(mmc); > - unsigned int num; > - int i, ret = 0; > - bool *tap; > + int ret = 0; > > - if (!host->init_tuning || !host->select_tuning) > - /* Tuning is not supported */ > - goto out; > + if (!host->tap_num) { > + if (!host->init_tuning || !host->select_tuning) > + /* Tuning is not supported */ > + goto out; > > - num = host->init_tuning(host); > - if (!num) > - /* Tuning is not supported */ > - goto out; > + host->tap_num = host->init_tuning(host); > + if (!host->tap_num) > + /* Tuning is not supported */ > + goto out; > > - tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL); > - if (!tap) { > - ret = -ENOMEM; > - goto out; > + if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) > + dev_warn_once(&host->pdev->dev, > + "Too many taps, skipping tuning. Please consider " > + "updating size of taps field of tmio_mmc_host\n"); > + > + host->use_saved_taps = false; > } > > - /* Issue CMD19 twice for each tap */ > - for (i = 0; i < 2 * num; i++) { > - if (host->prepare_tuning) > - host->prepare_tuning(host, i % num); > + if (!host->use_saved_taps) { > + int i; > + > + bitmap_zero(host->taps, host->tap_num * 2); > > - ret = mmc_send_tuning(mmc, opcode, NULL); > - if (ret && ret != -EILSEQ) > - goto err_free; > - tap[i] = (ret != 0); > + /* Issue CMD19 twice for each tap */ > + for (i = 0; i < 2 * host->tap_num; i++) { > + if (host->prepare_tuning) > + host->prepare_tuning(host, i % host->tap_num); > > - mdelay(1); > + ret = mmc_send_tuning(mmc, opcode, NULL); > + if (ret && ret != -EILSEQ) > + goto out; > + if (ret == 0) > + set_bit(i, host->taps); > + > + mdelay(1); > + } > } > > - ret = host->select_tuning(host, tap, num * 2); > + ret = host->select_tuning(host); > > -err_free: > - kfree(tap); > out: > + host->use_saved_taps = false; > if (ret < 0) { > dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); > mmc_hw_reset(mmc); > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > > mmc_retune_timer_stop(host->mmc); > mmc_retune_needed(host->mmc); > + host->use_saved_taps = true; I don't think you should trigger a re-tune here at all. Moreover you don't need to keep track of whether you have valid tap values by using the ->use_saved_taps variable, the mmc core deals with this for you. Instead, you should restore the tap values by invoking host->select_tuning() from the ->runtime_resume() callback, although only when host->mmc->can_retune == 1. We should probably add a helper function in the mmc core for this check, instead of accessing "can_retune" directly. > > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > 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
On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote: > On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: > > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: > >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: > >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: > > > > ... ... > > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > > > > mmc_retune_timer_stop(host->mmc); > > mmc_retune_needed(host->mmc); > > + host->use_saved_taps = true; > > I don't think you should trigger a re-tune here at all. Moreover you > don't need to keep track of whether you have valid tap values by using > the ->use_saved_taps variable, the mmc core deals with this for you. > > Instead, you should restore the tap values by invoking > host->select_tuning() from the ->runtime_resume() callback, although > only when host->mmc->can_retune == 1. We should probably add a helper > function in the mmc core for this check, instead of accessing > "can_retune" directly. Thanks, I was wondering what the best way to handle this is. I tried your suggestion above but it seems that host->mmc->can_retune is zero when ->runtime_resume() is called because of the following call paths: a) _mmc_sd_suspend() -> mmc_power_off() -> mmc_set_initial_state() -> mmc_retune_disable and b) mmc_sd_runtime_resume() -> mmc_power_up -> mmc_set_initial_state() -> mmc_retune_disable > > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); I plan to repost this patchset without the tap restoration code. This is because I have a number of changes locally, in particular to address other parts of your review, and I would like them to see the light of day. I will mark the tap restoration as a TODO item which we can address before merging the code in question. -- 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
On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote: > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote: >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: >> > >> > ... > > ... > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) >> > >> > mmc_retune_timer_stop(host->mmc); >> > mmc_retune_needed(host->mmc); >> > + host->use_saved_taps = true; >> >> I don't think you should trigger a re-tune here at all. Moreover you >> don't need to keep track of whether you have valid tap values by using >> the ->use_saved_taps variable, the mmc core deals with this for you. >> >> Instead, you should restore the tap values by invoking >> host->select_tuning() from the ->runtime_resume() callback, although >> only when host->mmc->can_retune == 1. We should probably add a helper >> function in the mmc core for this check, instead of accessing >> "can_retune" directly. > > Thanks, I was wondering what the best way to handle this is. > > I tried your suggestion above but it seems that host->mmc->can_retune is > zero when ->runtime_resume() is called because of the following call paths: > > a) _mmc_sd_suspend() > -> mmc_power_off() > -> mmc_set_initial_state() > -> mmc_retune_disable > and > b) mmc_sd_runtime_resume() > -> mmc_power_up > -> mmc_set_initial_state() > -> mmc_retune_disable > >> > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); This is exactly what shall happen :-) If the mmc core suspends the card, it means it will also re-initialize the card when resuming it. In that way the regular tuning sequence will take place as a part of re-initialization of the card, so you definitely must not restore tap values in these case. That is why you should be using the can_retune to know when to restore the values. > > > I plan to repost this patchset without the tap restoration code. > This is because I have a number of changes locally, in particular > to address other parts of your review, and I would like > them to see the light of day. > > I will mark the tap restoration as a TODO item which we can address > before merging the code in question. It shouldn't be that hard to implement this, so I rather see that we get this right from the beginning. As matter of fact it probably requires less code than you had in your initial approach, especially since I don't think you need to deal with anything tuning related in the ->runtime_suspend() callback, but only in ->runtime_resume(). 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
On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote: > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote: > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote: > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: > >> > > >> > ... > > > > ... > > > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > >> > > >> > mmc_retune_timer_stop(host->mmc); > >> > mmc_retune_needed(host->mmc); > >> > + host->use_saved_taps = true; > >> > >> I don't think you should trigger a re-tune here at all. Moreover you > >> don't need to keep track of whether you have valid tap values by using > >> the ->use_saved_taps variable, the mmc core deals with this for you. > >> > >> Instead, you should restore the tap values by invoking > >> host->select_tuning() from the ->runtime_resume() callback, although > >> only when host->mmc->can_retune == 1. We should probably add a helper > >> function in the mmc core for this check, instead of accessing > >> "can_retune" directly. > > > > Thanks, I was wondering what the best way to handle this is. > > > > I tried your suggestion above but it seems that host->mmc->can_retune is > > zero when ->runtime_resume() is called because of the following call paths: > > > > a) _mmc_sd_suspend() > > -> mmc_power_off() > > -> mmc_set_initial_state() > > -> mmc_retune_disable > > and > > b) mmc_sd_runtime_resume() > > -> mmc_power_up > > -> mmc_set_initial_state() > > -> mmc_retune_disable > > > >> > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > > This is exactly what shall happen :-) > > If the mmc core suspends the card, it means it will also re-initialize > the card when resuming it. In that way the regular tuning sequence > will take place as a part of re-initialization of the card, so you > definitely must not restore tap values in these case. That is why you > should be using the can_retune to know when to restore the values. Understood. In that case things are working as expected. > > I plan to repost this patchset without the tap restoration code. > > This is because I have a number of changes locally, in particular > > to address other parts of your review, and I would like > > them to see the light of day. > > > > I will mark the tap restoration as a TODO item which we can address > > before merging the code in question. > > It shouldn't be that hard to implement this, so I rather see that we > get this right from the beginning. > > As matter of fact it probably requires less code than you had in your > initial approach, especially since I don't think you need to deal with > anything tuning related in the ->runtime_suspend() callback, but only > in ->runtime_resume(). Got it. -- 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
On Thu, Sep 01, 2016 at 08:46:44AM +0200, Simon Horman wrote: > On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote: > > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote: > > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote: > > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: > > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: > > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: > > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: > > >> > > > >> > ... > > > > > > ... > > > > > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > > >> > > > >> > mmc_retune_timer_stop(host->mmc); > > >> > mmc_retune_needed(host->mmc); > > >> > + host->use_saved_taps = true; > > >> > > >> I don't think you should trigger a re-tune here at all. Moreover you > > >> don't need to keep track of whether you have valid tap values by using > > >> the ->use_saved_taps variable, the mmc core deals with this for you. > > >> > > >> Instead, you should restore the tap values by invoking > > >> host->select_tuning() from the ->runtime_resume() callback, although > > >> only when host->mmc->can_retune == 1. We should probably add a helper > > >> function in the mmc core for this check, instead of accessing > > >> "can_retune" directly. > > > > > > Thanks, I was wondering what the best way to handle this is. > > > > > > I tried your suggestion above but it seems that host->mmc->can_retune is > > > zero when ->runtime_resume() is called because of the following call paths: > > > > > > a) _mmc_sd_suspend() > > > -> mmc_power_off() > > > -> mmc_set_initial_state() > > > -> mmc_retune_disable > > > and > > > b) mmc_sd_runtime_resume() > > > -> mmc_power_up > > > -> mmc_set_initial_state() > > > -> mmc_retune_disable > > > > > >> > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > > > > This is exactly what shall happen :-) > > > > If the mmc core suspends the card, it means it will also re-initialize > > the card when resuming it. In that way the regular tuning sequence > > will take place as a part of re-initialization of the card, so you > > definitely must not restore tap values in these case. That is why you > > should be using the can_retune to know when to restore the values. > > Understood. In that case things are working as expected. I do have one question. It seems to me that host->mmc->can_retune == 1 on resume. If so, when would the saved tap values be used? I feel that I am missing something here. > > > I plan to repost this patchset without the tap restoration code. > > > This is because I have a number of changes locally, in particular > > > to address other parts of your review, and I would like > > > them to see the light of day. > > > > > > I will mark the tap restoration as a TODO item which we can address > > > before merging the code in question. > > > > It shouldn't be that hard to implement this, so I rather see that we > > get this right from the beginning. > > > > As matter of fact it probably requires less code than you had in your > > initial approach, especially since I don't think you need to deal with > > anything tuning related in the ->runtime_suspend() callback, but only > > in ->runtime_resume(). > > Got it. -- 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
On 1 September 2016 at 10:37, Simon Horman <horms@verge.net.au> wrote: > On Thu, Sep 01, 2016 at 08:46:44AM +0200, Simon Horman wrote: >> On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote: >> > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote: >> > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote: >> > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: >> > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: >> > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: >> > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: >> > >> > >> > >> > ... >> > > >> > > ... >> > > >> > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) >> > >> > >> > >> > mmc_retune_timer_stop(host->mmc); >> > >> > mmc_retune_needed(host->mmc); >> > >> > + host->use_saved_taps = true; >> > >> >> > >> I don't think you should trigger a re-tune here at all. Moreover you >> > >> don't need to keep track of whether you have valid tap values by using >> > >> the ->use_saved_taps variable, the mmc core deals with this for you. >> > >> >> > >> Instead, you should restore the tap values by invoking >> > >> host->select_tuning() from the ->runtime_resume() callback, although >> > >> only when host->mmc->can_retune == 1. We should probably add a helper >> > >> function in the mmc core for this check, instead of accessing >> > >> "can_retune" directly. >> > > >> > > Thanks, I was wondering what the best way to handle this is. >> > > >> > > I tried your suggestion above but it seems that host->mmc->can_retune is >> > > zero when ->runtime_resume() is called because of the following call paths: >> > > >> > > a) _mmc_sd_suspend() >> > > -> mmc_power_off() >> > > -> mmc_set_initial_state() >> > > -> mmc_retune_disable >> > > and >> > > b) mmc_sd_runtime_resume() >> > > -> mmc_power_up >> > > -> mmc_set_initial_state() >> > > -> mmc_retune_disable >> > > >> > >> > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); >> > >> > This is exactly what shall happen :-) >> > >> > If the mmc core suspends the card, it means it will also re-initialize >> > the card when resuming it. In that way the regular tuning sequence >> > will take place as a part of re-initialization of the card, so you >> > definitely must not restore tap values in these case. That is why you >> > should be using the can_retune to know when to restore the values. >> >> Understood. In that case things are working as expected. > > I do have one question. It seems to me that host->mmc->can_retune == 1 on > resume. If so, when would the saved tap values be used? I feel that I am > missing something here. There are different "resumes" here, so this gets a bit tricky to discuss around. Let me give it another try. First, let's assume we have a card inserted, initialized and tuned. This also means we have a cache of tuning (tap) values in the host. When the *host* runtime PM suspends, which isn't related to when the card suspends, typically the card remains powered/initialized. Then, when the host runtime PM resumes the tuning values can be restored in the host controller, as those should still be working because the card has remained initialized. When the card system PM suspends, the card will be put to sleep/power off (can_retune == 0). Sooner or later the host also becomes runtime PM suspended in this sequence. When the card system PM resumes, the card will be woken up from sleep, power on and the re-initializations starts. Before the re-initializations starts, the host will be runtime PM resumed, but since can_retune == 0, no tuning values will be restored (correct). When the re-initialization sequence completes of the card, can_retune == 1 as a new tuning sequence has also been completed. This also means refreshed cached tunings values in the host, which it can use the next time it will be runtime PM resumed. Hope this made more sense now. [...] 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
On Thu, Sep 01, 2016 at 11:55:27AM +0200, Ulf Hansson wrote: > On 1 September 2016 at 10:37, Simon Horman <horms@verge.net.au> wrote: > > On Thu, Sep 01, 2016 at 08:46:44AM +0200, Simon Horman wrote: > >> On Wed, Aug 31, 2016 at 09:38:40AM +0200, Ulf Hansson wrote: > >> > On 30 August 2016 at 22:51, Simon Horman <horms@verge.net.au> wrote: > >> > > On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote: > >> > >> On 29 August 2016 at 14:05, Simon Horman <horms@verge.net.au> wrote: > >> > >> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote: > >> > >> >> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote: > >> > >> >> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: > >> > >> > > >> > >> > ... > >> > > > >> > > ... > >> > > > >> > >> > @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > >> > >> > > >> > >> > mmc_retune_timer_stop(host->mmc); > >> > >> > mmc_retune_needed(host->mmc); > >> > >> > + host->use_saved_taps = true; > >> > >> > >> > >> I don't think you should trigger a re-tune here at all. Moreover you > >> > >> don't need to keep track of whether you have valid tap values by using > >> > >> the ->use_saved_taps variable, the mmc core deals with this for you. > >> > >> > >> > >> Instead, you should restore the tap values by invoking > >> > >> host->select_tuning() from the ->runtime_resume() callback, although > >> > >> only when host->mmc->can_retune == 1. We should probably add a helper > >> > >> function in the mmc core for this check, instead of accessing > >> > >> "can_retune" directly. > >> > > > >> > > Thanks, I was wondering what the best way to handle this is. > >> > > > >> > > I tried your suggestion above but it seems that host->mmc->can_retune is > >> > > zero when ->runtime_resume() is called because of the following call paths: > >> > > > >> > > a) _mmc_sd_suspend() > >> > > -> mmc_power_off() > >> > > -> mmc_set_initial_state() > >> > > -> mmc_retune_disable > >> > > and > >> > > b) mmc_sd_runtime_resume() > >> > > -> mmc_power_up > >> > > -> mmc_set_initial_state() > >> > > -> mmc_retune_disable > >> > > > >> > >> > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > >> > > >> > This is exactly what shall happen :-) > >> > > >> > If the mmc core suspends the card, it means it will also re-initialize > >> > the card when resuming it. In that way the regular tuning sequence > >> > will take place as a part of re-initialization of the card, so you > >> > definitely must not restore tap values in these case. That is why you > >> > should be using the can_retune to know when to restore the values. > >> > >> Understood. In that case things are working as expected. > > > > I do have one question. It seems to me that host->mmc->can_retune == 1 on > > resume. If so, when would the saved tap values be used? I feel that I am > > missing something here. > > There are different "resumes" here, so this gets a bit tricky to > discuss around. Let me give it another try. > > First, let's assume we have a card inserted, initialized and tuned. > This also means we have a cache of tuning (tap) values in the host. > > When the *host* runtime PM suspends, which isn't related to when the > card suspends, typically the card remains powered/initialized. Then, > when the host runtime PM resumes the tuning values can be restored in > the host controller, as those should still be working because the card > has remained initialized. > > When the card system PM suspends, the card will be put to sleep/power > off (can_retune == 0). Sooner or later the host also becomes runtime > PM suspended in this sequence. > > When the card system PM resumes, the card will be woken up from sleep, > power on and the re-initializations starts. Before the > re-initializations starts, the host will be runtime PM resumed, but > since can_retune == 0, no tuning values will be restored (correct). > When the re-initialization sequence completes of the card, can_retune > == 1 as a new tuning sequence has also been completed. This also means > refreshed cached tunings values in the host, which it can use the next > time it will be runtime PM resumed. > > Hope this made more sense now. Thanks for your detailed explanation. I understand now. -- 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
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index 94e79449c533..fc43cf5ce958 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -357,11 +357,9 @@ static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host, #define SH_MOBILE_SDHI_MAX_TAP 3 -static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, - bool *tap, int tap_size) +static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host) { struct sh_mobile_sdhi *priv = host_to_priv(host); - unsigned long tap_num; /* total number of taps */ unsigned long tap_cnt; /* counter of tuning success */ unsigned long tap_set; /* tap position */ unsigned long tap_start;/* start position of tuning success */ @@ -372,14 +370,6 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, /* Clear SCC_RVSREQ */ sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0); - /* Select SCC */ - tap_num = (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >> - SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) & - SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK; - - if (tap_num * 2 != tap_size) - return -EINVAL; - /* * Find the longest consecutive run of successful probes. If that * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the @@ -389,8 +379,8 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, ntap = 0; tap_start = 0; tap_end = 0; - for (i = 0; i < tap_num * 2; i++) { - if (tap[i] == 0) + for (i = 0; i < host->tap_num * 2; i++) { + if (test_bit(i, host->taps)) ntap++; else { if (ntap > tap_cnt) { @@ -409,7 +399,7 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host, } if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP) - tap_set = (tap_start + tap_end) / 2 % tap_num; + tap_set = (tap_start + tap_end) / 2 % host->tap_num; else return -EIO; diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index c932fe876690..4186045cce0d 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -171,8 +171,12 @@ struct tmio_mmc_host { /* Mandatory callback for tuning to occur which is * optional for SDR50 and mandatory for SDR104 */ unsigned int (*init_tuning)(struct tmio_mmc_host *host); - int (*select_tuning)(struct tmio_mmc_host *host, bool *tap, - int tap_size); + int (*select_tuning)(struct tmio_mmc_host *host); + + /* Tuning values: 1 for success, 0 for failure */ + DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); + unsigned int tap_num; + bool use_saved_taps; }; struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev); diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index ded8fa53e0a0..2b5c8b090ed3 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -773,43 +773,50 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc) static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) { struct tmio_mmc_host *host = mmc_priv(mmc); - unsigned int num; - int i, ret = 0; - bool *tap; + int ret = 0; - if (!host->init_tuning || !host->select_tuning) - /* Tuning is not supported */ - goto out; + if (!host->tap_num) { + if (!host->init_tuning || !host->select_tuning) + /* Tuning is not supported */ + goto out; - num = host->init_tuning(host); - if (!num) - /* Tuning is not supported */ - goto out; + host->tap_num = host->init_tuning(host); + if (!host->tap_num) + /* Tuning is not supported */ + goto out; - tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL); - if (!tap) { - ret = -ENOMEM; - goto out; + if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) + dev_warn_once(&host->pdev->dev, + "Too many taps, skipping tuning. Please consider " + "updating size of taps field of tmio_mmc_host\n"); + + host->use_saved_taps = false; } - /* Issue CMD19 twice for each tap */ - for (i = 0; i < 2 * num; i++) { - if (host->prepare_tuning) - host->prepare_tuning(host, i % num); + if (!host->use_saved_taps) { + int i; + + bitmap_zero(host->taps, host->tap_num * 2); - ret = mmc_send_tuning(mmc, opcode, NULL); - if (ret && ret != -EILSEQ) - goto err_free; - tap[i] = (ret != 0); + /* Issue CMD19 twice for each tap */ + for (i = 0; i < 2 * host->tap_num; i++) { + if (host->prepare_tuning) + host->prepare_tuning(host, i % host->tap_num); - mdelay(1); + ret = mmc_send_tuning(mmc, opcode, NULL); + if (ret && ret != -EILSEQ) + goto out; + if (ret == 0) + set_bit(i, host->taps); + + mdelay(1); + } } - ret = host->select_tuning(host, tap, num * 2); + ret = host->select_tuning(host); -err_free: - kfree(tap); out: + host->use_saved_taps = false; if (ret < 0) { dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); mmc_hw_reset(mmc); @@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) mmc_retune_timer_stop(host->mmc); mmc_retune_needed(host->mmc); + host->use_saved_taps = true; tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);