Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399

[Ziyuan Xu <xzy.xu@rock-chips.com>]

On 2016年09月02日 05:29, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>> Hi
>>
>>
>> On 2016年09月01日 12:20, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>> This is fine to pick up _only_ if you don't care about suspend/resume.
>>>>> If you care about suspend/resume then someone needs to first write a
>>>>> patch that will re-init all "corecfg" values after power is turned on.
>>>>
>>>> Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we
>>>> don't need to strore/re-init it after resume.
>>>> corecfg_clockmultiplier is only used to fetch host->clk_mul, and
>>>> host->clk_mul has been a fixed value at run-time, unless driver unbind.
>>>> The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to check
>>>> the xin_clk at probe time, we don't reference it at run-time.
>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>> works
>>>> fine.
>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>> actually do something useful and aren't used to just communicate back
>>> to software?
>>
>> Take corecfg_clockmultiplier as example.
>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
>> for further initialization.
>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>> frequency to play.
>>
>> I think we don't need to store it due to it's a fixed value at run-time,
>> even if it is reset after a power cycle, the above will not be changed via
>> software, except for dirver unbind .
>>
>>> I know that:
>>>
>>> 1. If I don't pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>> suspend / resume.
>>>
>>> 2. If I do pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>> after suspend/resume).
>>>
>>>
>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>> corecfg_baseclkfreq are wrong?
>>
>> Yup, corecfg_* stuff will be reset after a power cycle.
>> I mean that we need only to guarantee they're correct at probe time.
> So are you saying that the entire purpose of "corecfg_clockmultiplier"
> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
> register to get a certain value?
> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
> to get a certain value?
Yes, on rk3399:
corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock

If you re-write to either corecfg_* stuff,  the corresponding CAP 
register field will be changed too.
sdhci driver will fetch CAP register for initialization, we only need to 
guarantee they're correct at probe time.

Did that all make sense?
>
> That would have been nice to know before.  I had assumed that those
> "corecfg" settings did something else more useful.
>
> If it is indeed true that these corecfg values are as silly as it
> seems, then I guess it's not terribly important to re-set them after
> suspend/resume.  Eventually it would be nice/clean to actually do so
> (in case the SDHCI driver eventually changes), but I guess we wouldn't
> need to block. this patch from landing.
>
> Can you please confirm my understanding above?
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip