| Message ID | 1469592803-13842-3-git-send-email-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +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; > + > + if (!host->init_tuning || !host->select_tuning) Check host->prepare_tuning, too? > + /* Tuning is not supported */ > + goto out; > + > + num = host->init_tuning(host); > + if (!num) > + /* Tuning is not supported */ > + goto out; > + > + tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL); > + if (!tap) { > + ret = -ENOMEM; > + goto out; > + } > + > + /* Issue CMD19 twice for each tap */ > + for (i = 0; i < 2 * num; i++) { > + if (host->prepare_tuning) > + host->prepare_tuning(host, i % num); > + > + ret = mmc_send_tuning(mmc, opcode, NULL); > + if (ret && ret != -EILSEQ) > + goto err_free; > + tap[i] = (ret != 0); > + > + mdelay(1); > + } > + > + ret = host->select_tuning(host, tap, num * 2); > + > +err_free: > + kfree(tap); > +out: > + if (ret < 0) { > + dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); > + tmio_mmc_hw_reset(mmc); > + } else { > + host->mmc->retune_period = 0; > + } > + > + return ret; > + Unnecessary blank line -- 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 27 July 2016 at 06:13, Simon Horman <horms+renesas@verge.net.au> wrote: > From: Ai Kyuse <ai.kyuse.uw@renesas.com> > > Add tuning support for use with SDR104 mode > > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > --- > v4 [Simon Horman] > As suggested by Wolfram Sang: > - Do not perform tuning if host->select_tuning is not set: > it seems to make little sense to do so and moreover there is currently > no such use-case > - Do not add mrc->sbc handling from tmio_mmc_request, > this is a hang-over from earlier versions of this patchset which > did not use core infrastructure for retuning > - Tidy up local variable usage > * Correct index passed to prepare_tuning(): this seems to have > been the last piece of resolving the timeouts during tuning puzzle > * Further cleanups to tmio_mmc_execute_tuning(): > - Ensure tap is sized proportionally to its members > - Remove stray '*' in comment > - Use mmc rather than host->mmc, these are equivalent but > the former seems tidier > - Correct inverted logic in setting tap values > * Re-introduce retuning support. This was removed in v3. > > v3 [Simon Horman] > * As suggested by Kuninori Morimoto: > - Do not add unused retuning callback to struct tmio_mmc_host > - Change return type of prepare_tuning callback to void > - Add tap_size parameter to select_tuning callback > > v2 [Simon Horman] > * As suggested by Kuninori Morimoto: > - Actually remove unnecessary TMIO_MMC_HAS_UHS_SCC define > * As suggested by Wolfram Sang: > - Rely on core to call tuning. This simplifies things somewhat. > - Use mmc_send_tuning() > - A side affect of this appears to be that we now see some recoverable > errors logged during tuning. These are typically corrected by > subsequent tuning. It is the logging that is the apparent side effect > of this change. > e.g. > sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD19) > sh_mobile_sdhi ee100000.sd: Tuning procedure failed > * Use bool rather than unsigned long to pass test status > to select_tuning() callback > * Do not retune if init_tuning callback is not present or > indicates that there are no taps present > * Retune on hardware reset > > v1 [Simon Horman] > * Omit start_signal_voltage_switch and tmio_mmc_card_busy changes which are > already present in mainline in a different form > * Return num from init_tuning rather than passing an extra parameter > to hold the return value > * Only call host->init_tuning if it is non-NULL > * Place tmio_mmc_execute_tuning() such that no new forward declarations are > required > * Remove unused TMIO_MMC_HAS_UHS_SCC define > > v0 [Ai Kyuse] > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > --- > drivers/mmc/host/tmio_mmc.h | 7 ++++ > drivers/mmc/host/tmio_mmc_pio.c | 77 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index 7f63ec05bdf4..316b0c3fe745 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -150,6 +150,7 @@ struct tmio_mmc_host { > struct mutex ios_lock; /* protect set_ios() context */ > bool native_hotplug; > bool sdio_irq_enabled; > + u32 scc_tappos; > > int (*write16_hook)(struct tmio_mmc_host *host, int addr); > int (*clk_enable)(struct tmio_mmc_host *host); > @@ -160,6 +161,12 @@ struct tmio_mmc_host { > unsigned int direction, int blk_size); > int (*start_signal_voltage_switch)(struct mmc_host *mmc, > struct mmc_ios *ios); > + unsigned int (*init_tuning)(struct tmio_mmc_host *host); > + void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); > + int (*select_tuning)(struct tmio_mmc_host *host, bool *tap, > + int tap_size); > + bool (*retuning)(struct tmio_mmc_host *host); > + void (*hw_reset)(struct tmio_mmc_host *host); Please add the HW reset support in separate patch. I guess you need it to go in before the tuning support. > }; > > 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 a9d07b5f3c63..83b5148a2684 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -36,6 +36,7 @@ > #include <linux/io.h> > #include <linux/irq.h> > #include <linux/mfd/tmio.h> > +#include <linux/mmc/card.h> > #include <linux/mmc/host.h> > #include <linux/mmc/mmc.h> > #include <linux/mmc/slot-gpio.h> > @@ -298,6 +299,15 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) > if (mrq->cmd->error || (mrq->data && mrq->data->error)) > tmio_mmc_abort_dma(host); > > + if (host->retuning) { > + int result = host->retuning(host); This looks like you need to re-tune between each an every request. :-) Although I guess what really are doing here is that you check if the auto-retuning has failed, correct? Perhaps one could update the naming of the new tmio callbacks for tuning as to make those more self-explained. > + > + if (result || (mrq->cmd->error == -EILSEQ)) { > + mmc_retune_timer_stop(host->mmc); > + mmc_retune_needed(host->mmc); The mmc core already deals with re-tuning when it get an -EILSEQ error from a request, so you shouldn't need to manage that here as well. > + } > + } > + > mmc_request_done(host->mmc, mrq); > } > > @@ -756,6 +766,68 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host, > return 0; > } > > +static void tmio_mmc_hw_reset(struct mmc_host *mmc) As stated earlier, please add the HW reset in a separate patch. > +{ > + struct tmio_mmc_host *host = mmc_priv(mmc); > + > + if (host->hw_reset) > + host->hw_reset(host); > + > + mmc_retune_timer_stop(host->mmc); > + mmc_retune_needed(host->mmc); This looks like it belongs in the mmc core when it invokes a HW reset sequence. Please try to move it into there (unless it already covers this). > +} > + > +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; > + > + if (!host->init_tuning || !host->select_tuning) When defining these callbacks, it would be nice to know which ones are optional and which ones are required. > + /* Tuning is not supported */ > + goto out; > + > + num = host->init_tuning(host); > + if (!num) > + /* Tuning is not supported */ > + goto out; > + > + tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL); > + if (!tap) { > + ret = -ENOMEM; > + goto out; > + } > + > + /* Issue CMD19 twice for each tap */ > + for (i = 0; i < 2 * num; i++) { > + if (host->prepare_tuning) > + host->prepare_tuning(host, i % num); > + > + ret = mmc_send_tuning(mmc, opcode, NULL); > + if (ret && ret != -EILSEQ) > + goto err_free; > + tap[i] = (ret != 0); > + > + mdelay(1); > + } > + > + ret = host->select_tuning(host, tap, num * 2); > + > +err_free: > + kfree(tap); > +out: > + if (ret < 0) { > + dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); > + tmio_mmc_hw_reset(mmc); > + } else { > + host->mmc->retune_period = 0; > + } > + > + return ret; > + > +} > + > /* Process requests from the MMC layer */ > static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > @@ -978,6 +1050,8 @@ static struct mmc_host_ops tmio_mmc_ops = { > .enable_sdio_irq = tmio_mmc_enable_sdio_irq, > .card_busy = tmio_mmc_card_busy, > .multi_io_quirk = tmio_multi_io_quirk, > + .execute_tuning = tmio_mmc_execute_tuning, > + .hw_reset = tmio_mmc_hw_reset, > }; > > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > struct mmc_host *mmc = dev_get_drvdata(dev); > struct tmio_mmc_host *host = mmc_priv(mmc); > > + mmc_retune_timer_stop(host->mmc); > + mmc_retune_needed(host->mmc); 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. > + > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > > if (host->clk_cache) > -- > 2.7.0.rc3.207.g0ac5344 > 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
Hi Ulf, thanks for your review. I have tried to address it as best I can below. On Mon, Aug 22, 2016 at 03:39:03PM +0200, Ulf Hansson wrote: > On 27 July 2016 at 06:13, Simon Horman <horms+renesas@verge.net.au> wrote: > > From: Ai Kyuse <ai.kyuse.uw@renesas.com> > > > > Add tuning support for use with SDR104 mode > > > > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > v4 [Simon Horman] > > As suggested by Wolfram Sang: > > - Do not perform tuning if host->select_tuning is not set: > > it seems to make little sense to do so and moreover there is currently > > no such use-case > > - Do not add mrc->sbc handling from tmio_mmc_request, > > this is a hang-over from earlier versions of this patchset which > > did not use core infrastructure for retuning > > - Tidy up local variable usage > > * Correct index passed to prepare_tuning(): this seems to have > > been the last piece of resolving the timeouts during tuning puzzle > > * Further cleanups to tmio_mmc_execute_tuning(): > > - Ensure tap is sized proportionally to its members > > - Remove stray '*' in comment > > - Use mmc rather than host->mmc, these are equivalent but > > the former seems tidier > > - Correct inverted logic in setting tap values > > * Re-introduce retuning support. This was removed in v3. > > > > v3 [Simon Horman] > > * As suggested by Kuninori Morimoto: > > - Do not add unused retuning callback to struct tmio_mmc_host > > - Change return type of prepare_tuning callback to void > > - Add tap_size parameter to select_tuning callback > > > > v2 [Simon Horman] > > * As suggested by Kuninori Morimoto: > > - Actually remove unnecessary TMIO_MMC_HAS_UHS_SCC define > > * As suggested by Wolfram Sang: > > - Rely on core to call tuning. This simplifies things somewhat. > > - Use mmc_send_tuning() > > - A side affect of this appears to be that we now see some recoverable > > errors logged during tuning. These are typically corrected by > > subsequent tuning. It is the logging that is the apparent side effect > > of this change. > > e.g. > > sh_mobile_sdhi ee100000.sd: timeout waiting for hardware interrupt (CMD19) > > sh_mobile_sdhi ee100000.sd: Tuning procedure failed > > * Use bool rather than unsigned long to pass test status > > to select_tuning() callback > > * Do not retune if init_tuning callback is not present or > > indicates that there are no taps present > > * Retune on hardware reset > > > > v1 [Simon Horman] > > * Omit start_signal_voltage_switch and tmio_mmc_card_busy changes which are > > already present in mainline in a different form > > * Return num from init_tuning rather than passing an extra parameter > > to hold the return value > > * Only call host->init_tuning if it is non-NULL > > * Place tmio_mmc_execute_tuning() such that no new forward declarations are > > required > > * Remove unused TMIO_MMC_HAS_UHS_SCC define > > > > v0 [Ai Kyuse] > > > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > drivers/mmc/host/tmio_mmc.h | 7 ++++ > > drivers/mmc/host/tmio_mmc_pio.c | 77 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 84 insertions(+) > > > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > > index 7f63ec05bdf4..316b0c3fe745 100644 > > --- a/drivers/mmc/host/tmio_mmc.h > > +++ b/drivers/mmc/host/tmio_mmc.h > > @@ -150,6 +150,7 @@ struct tmio_mmc_host { > > struct mutex ios_lock; /* protect set_ios() context */ > > bool native_hotplug; > > bool sdio_irq_enabled; > > + u32 scc_tappos; > > > > int (*write16_hook)(struct tmio_mmc_host *host, int addr); > > int (*clk_enable)(struct tmio_mmc_host *host); > > @@ -160,6 +161,12 @@ struct tmio_mmc_host { > > unsigned int direction, int blk_size); > > int (*start_signal_voltage_switch)(struct mmc_host *mmc, > > struct mmc_ios *ios); > > + unsigned int (*init_tuning)(struct tmio_mmc_host *host); > > + void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); > > + int (*select_tuning)(struct tmio_mmc_host *host, bool *tap, > > + int tap_size); > > + bool (*retuning)(struct tmio_mmc_host *host); > > + void (*hw_reset)(struct tmio_mmc_host *host); > > Please add the HW reset support in separate patch. I guess you need it > to go in before the tuning support. Sure, will do. And yes, I think it needs go go in before tuning support. > > }; > > > > 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 a9d07b5f3c63..83b5148a2684 100644 > > --- a/drivers/mmc/host/tmio_mmc_pio.c > > +++ b/drivers/mmc/host/tmio_mmc_pio.c > > @@ -36,6 +36,7 @@ > > #include <linux/io.h> > > #include <linux/irq.h> > > #include <linux/mfd/tmio.h> > > +#include <linux/mmc/card.h> > > #include <linux/mmc/host.h> > > #include <linux/mmc/mmc.h> > > #include <linux/mmc/slot-gpio.h> > > @@ -298,6 +299,15 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) > > if (mrq->cmd->error || (mrq->data && mrq->data->error)) > > tmio_mmc_abort_dma(host); > > > > + if (host->retuning) { > > + int result = host->retuning(host); > > This looks like you need to re-tune between each an every request. :-) > > Although I guess what really are doing here is that you check if the > auto-retuning has failed, correct? > > Perhaps one could update the naming of the new tmio callbacks for > tuning as to make those more self-explained. Perhaps calling it check_scc_error would be better, that is what the callback actually does > > > + > > + if (result || (mrq->cmd->error == -EILSEQ)) { > > + mmc_retune_timer_stop(host->mmc); > > + mmc_retune_needed(host->mmc); > > The mmc core already deals with re-tuning when it get an -EILSEQ error > from a request, so you shouldn't need to manage that here as well. Thanks, I'll clean that up. > > > + } > > + } > > + > > mmc_request_done(host->mmc, mrq); > > } > > > > @@ -756,6 +766,68 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host, > > return 0; > > } > > > > +static void tmio_mmc_hw_reset(struct mmc_host *mmc) > > As stated earlier, please add the HW reset in a separate patch. > > > +{ > > + struct tmio_mmc_host *host = mmc_priv(mmc); > > + > > + if (host->hw_reset) > > + host->hw_reset(host); > > + > > + mmc_retune_timer_stop(host->mmc); > > + mmc_retune_needed(host->mmc); > > This looks like it belongs in the mmc core when it invokes a HW reset > sequence. Please try to move it into there (unless it already covers > this). I think it is already handled by the core and I propose updating this patch as follows: @@ -772,9 +772,6 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc) if (host->hw_reset) host->hw_reset(host); - - mmc_retune_timer_stop(host->mmc); - mmc_retune_needed(host->mmc); } static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) @@ -819,7 +816,7 @@ err_free: out: if (ret < 0) { dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); - tmio_mmc_hw_reset(mmc); + mmc_hw_reset(mmc); } else { host->mmc->retune_period = 0; } > > +} > > + > > +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; > > + > > + if (!host->init_tuning || !host->select_tuning) > > When defining these callbacks, it would be nice to know which ones are > optional and which ones are required. Would some comments in struct tmio_mmc_host be an appropriate way to achieve that? > > + /* Tuning is not supported */ > > + goto out; > > + > > + num = host->init_tuning(host); > > + if (!num) > > + /* Tuning is not supported */ > > + goto out; > > + > > + tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL); > > + if (!tap) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + /* Issue CMD19 twice for each tap */ > > + for (i = 0; i < 2 * num; i++) { > > + if (host->prepare_tuning) > > + host->prepare_tuning(host, i % num); > > + > > + ret = mmc_send_tuning(mmc, opcode, NULL); > > + if (ret && ret != -EILSEQ) > > + goto err_free; > > + tap[i] = (ret != 0); > > + > > + mdelay(1); > > + } > > + > > + ret = host->select_tuning(host, tap, num * 2); > > + > > +err_free: > > + kfree(tap); > > +out: > > + if (ret < 0) { > > + dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); > > + tmio_mmc_hw_reset(mmc); > > + } else { > > + host->mmc->retune_period = 0; > > + } > > + > > + return ret; > > + > > +} > > + > > /* Process requests from the MMC layer */ > > static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > > { > > @@ -978,6 +1050,8 @@ static struct mmc_host_ops tmio_mmc_ops = { > > .enable_sdio_irq = tmio_mmc_enable_sdio_irq, > > .card_busy = tmio_mmc_card_busy, > > .multi_io_quirk = tmio_multi_io_quirk, > > + .execute_tuning = tmio_mmc_execute_tuning, > > + .hw_reset = tmio_mmc_hw_reset, > > }; > > > > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > > struct mmc_host *mmc = dev_get_drvdata(dev); > > struct tmio_mmc_host *host = mmc_priv(mmc); > > > > + mmc_retune_timer_stop(host->mmc); > > + mmc_retune_needed(host->mmc); > > 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. From a software POV that ought to be simple enough: a small bitmap ought to be sufficient to save the values for the hardware covered by this series. > > + > > tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); > > > > if (host->clk_cache) > > -- > > 2.7.0.rc3.207.g0ac5344 -- 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
[...] >> > +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; >> > + >> > + if (!host->init_tuning || !host->select_tuning) >> >> When defining these callbacks, it would be nice to know which ones are >> optional and which ones are required. > > Would some comments in struct tmio_mmc_host be an appropriate way > to achieve that? Yes, that would be nice! [...] >> > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) >> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) >> > struct mmc_host *mmc = dev_get_drvdata(dev); >> > struct tmio_mmc_host *host = mmc_priv(mmc); >> > >> > + mmc_retune_timer_stop(host->mmc); >> > + mmc_retune_needed(host->mmc); >> >> 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! > > From a software POV that ought to be simple enough: a small bitmap ought > to be sufficient to save the values for the hardware covered by this > series. Yes, indeed! 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 Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote: > [...] > > >> > +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; > >> > + > >> > + if (!host->init_tuning || !host->select_tuning) > >> > >> When defining these callbacks, it would be nice to know which ones are > >> optional and which ones are required. > > > > Would some comments in struct tmio_mmc_host be an appropriate way > > to achieve that? > > Yes, that would be nice! > > [...] > > >> > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > >> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > >> > struct mmc_host *mmc = dev_get_drvdata(dev); > >> > struct tmio_mmc_host *host = mmc_priv(mmc); > >> > > >> > + mmc_retune_timer_stop(host->mmc); > >> > + mmc_retune_needed(host->mmc); > >> > >> 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. * 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 Personally my feeling is that we should initiate a retune on resume for now as that seems to be the safest option. > > From a software POV that ought to be simple enough: a small bitmap ought > > to be sufficient to save the values for the hardware covered by this > > series. > > Yes, indeed! > > 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 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: >> [...] >> >> >> > +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; >> >> > + >> >> > + if (!host->init_tuning || !host->select_tuning) >> >> >> >> When defining these callbacks, it would be nice to know which ones are >> >> optional and which ones are required. >> > >> > Would some comments in struct tmio_mmc_host be an appropriate way >> > to achieve that? >> >> Yes, that would be nice! >> >> [...] >> >> >> > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) >> >> > @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) >> >> > struct mmc_host *mmc = dev_get_drvdata(dev); >> >> > struct tmio_mmc_host *host = mmc_priv(mmc); >> >> > >> >> > + mmc_retune_timer_stop(host->mmc); >> >> > + mmc_retune_needed(host->mmc); >> >> >> >> 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? [...] 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
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 7f63ec05bdf4..316b0c3fe745 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -150,6 +150,7 @@ struct tmio_mmc_host { struct mutex ios_lock; /* protect set_ios() context */ bool native_hotplug; bool sdio_irq_enabled; + u32 scc_tappos; int (*write16_hook)(struct tmio_mmc_host *host, int addr); int (*clk_enable)(struct tmio_mmc_host *host); @@ -160,6 +161,12 @@ struct tmio_mmc_host { unsigned int direction, int blk_size); int (*start_signal_voltage_switch)(struct mmc_host *mmc, struct mmc_ios *ios); + unsigned int (*init_tuning)(struct tmio_mmc_host *host); + void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); + int (*select_tuning)(struct tmio_mmc_host *host, bool *tap, + int tap_size); + bool (*retuning)(struct tmio_mmc_host *host); + void (*hw_reset)(struct tmio_mmc_host *host); }; 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 a9d07b5f3c63..83b5148a2684 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -36,6 +36,7 @@ #include <linux/io.h> #include <linux/irq.h> #include <linux/mfd/tmio.h> +#include <linux/mmc/card.h> #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> #include <linux/mmc/slot-gpio.h> @@ -298,6 +299,15 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) if (mrq->cmd->error || (mrq->data && mrq->data->error)) tmio_mmc_abort_dma(host); + if (host->retuning) { + int result = host->retuning(host); + + if (result || (mrq->cmd->error == -EILSEQ)) { + mmc_retune_timer_stop(host->mmc); + mmc_retune_needed(host->mmc); + } + } + mmc_request_done(host->mmc, mrq); } @@ -756,6 +766,68 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host, return 0; } +static void tmio_mmc_hw_reset(struct mmc_host *mmc) +{ + struct tmio_mmc_host *host = mmc_priv(mmc); + + if (host->hw_reset) + host->hw_reset(host); + + mmc_retune_timer_stop(host->mmc); + mmc_retune_needed(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; + + 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; + + tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL); + if (!tap) { + ret = -ENOMEM; + goto out; + } + + /* Issue CMD19 twice for each tap */ + for (i = 0; i < 2 * num; i++) { + if (host->prepare_tuning) + host->prepare_tuning(host, i % num); + + ret = mmc_send_tuning(mmc, opcode, NULL); + if (ret && ret != -EILSEQ) + goto err_free; + tap[i] = (ret != 0); + + mdelay(1); + } + + ret = host->select_tuning(host, tap, num * 2); + +err_free: + kfree(tap); +out: + if (ret < 0) { + dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); + tmio_mmc_hw_reset(mmc); + } else { + host->mmc->retune_period = 0; + } + + return ret; + +} + /* Process requests from the MMC layer */ static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) { @@ -978,6 +1050,8 @@ static struct mmc_host_ops tmio_mmc_ops = { .enable_sdio_irq = tmio_mmc_enable_sdio_irq, .card_busy = tmio_mmc_card_busy, .multi_io_quirk = tmio_multi_io_quirk, + .execute_tuning = tmio_mmc_execute_tuning, + .hw_reset = tmio_mmc_hw_reset, }; static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) @@ -1202,6 +1276,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); + mmc_retune_timer_stop(host->mmc); + mmc_retune_needed(host->mmc); + tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); if (host->clk_cache)