Date:   Thu, 18 Jul 2019 16:08:48 -0700
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>,
        Peter De Schrijver <pdeschrijver@...dia.com>
CC:     <sboyd@...nel.org>, Michael Turquette <mturquette@...libre.com>,
        Joseph Lo <josephl@...dia.com>, <thierry.reding@...il.com>,
        <jonathanh@...dia.com>, <tglx@...utronix.de>,
        <jason@...edaemon.net>, <marc.zyngier@....com>,
        <linus.walleij@...aro.org>, <stefan@...er.ch>,
        <mark.rutland@....com>, <pgaikwad@...dia.com>,
        <linux-clk@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <jckuo@...dia.com>, <talho@...dia.com>,
        <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <mperttunen@...dia.com>, <spatra@...dia.com>, <robh+dt@...nel.org>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks


On 7/18/19 1:36 PM, Sowjanya Komatineni wrote:
>
> On 7/18/19 1:26 PM, Dmitry Osipenko wrote:
>> 18.07.2019 22:42, Peter De Schrijver пишет:
>>> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
>>>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS 
>>>>> peripheral
>>>>> clocks which need to be restored prior to DFLL reinit.
>>>> Okay, but that shouldn't be a problem if clock dependencies are set up
>>>> properly.
>>>>
>>>>>>> reverse list order during restore might not work as all other 
>>>>>>> clocks are
>>>>>>> in proper order no with any ref clocks for plls getting restored 
>>>>>>> prior
>>>>>>> to their clients
>>>>>> Why? The ref clocks should be registered first and be the roots 
>>>>>> for PLLs
>>>>>> and the rest. If it's not currently the case, then this need to be
>>>>>> fixed. You need to ensure that each clock is modeled properly. If 
>>>>>> some
>>>>>> child clock really depends on multiple parents, then the parents 
>>>>>> need to
>>>>>> in the correct order or CCF need to be taught about such
>>>>>> multi-dependencies.
>>>>>>
>>>>>> If some required feature is missed, then you have to implement it
>>>>>> properly and for all, that's how things are done in upstream. 
>>>>>> Sometimes
>>>>>> it's quite a lot of extra work that everyone are benefiting from in
>>>>>> the end.
>>>>>>
>>>>>> [snip]
>>>>> Yes, we should register ref/parents before their clients.
>>>>>
>>>>> cclk_g clk is registered last after all pll and peripheral clocks are
>>>>> registers during clock init.
>>>>>
>>>>> dfllCPU_out clk is registered later during dfll-fcpu driver probe and
>>>>> gets added to the clock list.
>>>>>
>>>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
>>>>> dependencies for dfllCPU_out thru clock list.
>>>>>
>>>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk 
>>>>> reference
>>>>> thru DT.
>>> The dfll does not have any parents. It has some clocks which are needed
>>> for the logic part of the dfll to function, but there's no parent clock
>>> as such unlike for peripheral clocks or PLLs where the parent is at
>>> least used as a reference. The I2C controller of the DFLL shares the
>>> lines with a normal I2C controller using some arbitration logic. That
>>> logic only works if the clock for the normal I2C controller is enabled.
>>> So you need probably 3 clocks enabled to initialize the dfll in that
>>> case. I don't think it makes sense to add complicated logic to the 
>>> clock
>>> core to deal with this rather strange case. To me it makes more 
>>> sense to
>>> use pmops and open code the sequence there.
>> It looks to me that dfllCPU is a PLL and dfll_ref is its reference
>> parent, while dfll_soc clocks the logic that dynamically reconfigures
>> dfllCPU in background. I see that PLLP is defined as a parent for
>> dfll_ref and dfll_soc in the code. Hence seems dfll_ref should be set as
>> a parent for dfllCPU, no?
>
> dfll_soc will not be restored by the time dfllCPU resume happens after 
> dfll_ref.
>
> without dfll_soc, dfllCPU cannot be resumed either. So if we decide to 
> use parent we should use dfll_soc.
>
>> Either way is good to me, given that DFLL will be disabled during
>> suspend. Resetting DFLL on DFLL's driver resume using PM ops should be
>> good. And then it also will be better to error out if DFLL is active
>> during suspend on the DFLL's driver suspend.
>
> Doing in dfll-fcpu pm_ops is much better as it happens right after all 
> clocks are restored and unlike other clock enables, dfll need dfll 
> controller programming as well and is actually registered in dfll-fcpu 
> driver.
>
> With this, below is the sequence:
>
> CPUFreq suspend switches CPU to PLLP and disables dfll
>
> Will add dfll_suspend/resume in dfll-fcpu driver and in dfll suspend 
> will check for dfll active and will error out suspend.
>
> dfll resume does dfll reinit.
>
> CPUFreq resume enables dfll and switches CPU to dfll.
>
>
> Will go with doing in dfll-fcpu pm_ops rather than parenting 
> dfllCPU_OUT...
>
Does is make sense to return error EBUSY if dfll is not disabled by the 
time dfll-fcpu suspend happens?

Or should I use ETIMEOUT?

Powered by blists - more mailing lists