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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 30 August 2016 at 22:51, Simon Horman <horms@xxxxxxxxxxxx> wrote:
> On Mon, Aug 29, 2016 at 04:05:55PM +0200, Ulf Hansson wrote:
>> On 29 August 2016 at 14:05, Simon Horman <horms@xxxxxxxxxxxx> wrote:
>> > On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
>> >> On 25 August 2016 at 14:04, Simon Horman <horms@xxxxxxxxxxxx> 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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux