Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman <horms@verge.net.au>]

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.