Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support

[Rajendra Nayak <rnayak@codeaurora.org>]

On 7/2/2021 2:27 AM, Bjorn Andersson wrote:
> On Thu 01 Jul 15:12 CDT 2021, Dmitry Baryshkov wrote:
> 
>> On Thu, 1 Jul 2021 at 07:23, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>>
>>> On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:
>>>
>>>> On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>
>>>>> On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
>>>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>>>
>>>>>>> On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
>>>>>>>
>>>>>>>> On sm8250 dispcc and videocc registers are powered up by the MMCX power
>>>>>>>> domain. Currently we used a regulator to enable this domain on demand,
>>>>>>>> however this has some consequences, as genpd code is not reentrant.
>>>>>>>>
>>>>>>>> Teach Qualcomm clock controller code about setting up power domains and
>>>>>>>> using them for gdsc control.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>
>>>>>>> There's a proposal to add a generic binding for statically assigning a
>>>>>>> performance states here:
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
>>>>
>>>> I checked this thread. It looks like Rajendra will also switch to the
>>>> "required-opps" property. So if that series goes in first, we can drop
>>>> the call to set_performance_state. If this one goes in first, we can
>>>> drop the set_performance_state call after getting Rajendra's work in.
>>>>
>>>>>>>
>>>>>>>
>>>>>>> But that said, do you really need this?
>>>>>>>
>>>>>>> The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
>>>>>>> SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
>>>>>>> to 460MHz in &mdss.
>>>>>>>
>>>>>>> But then in &mdss_mdp you do the same using an opp-table based on the
>>>>>>> actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
>>>>>>
>>>>>> MDSS and DSI would bump up MMCX performance state requirements on
>>>>>> their own, depending on the frequency being selected.
>>>>>>
>>>>>
>>>>> Right, but as I copied things from the sm8250.dtsi to come up with
>>>>> sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
>>>>> in &mdss kicks in I need the performance state to be at NOM.
>>>>>
>>>>> So keeping the assigned-clockrate in &mdss means that MMCX will never go
>>>>> below NOM.
>>>>
>>>> No, because once MDP is fully running, it will lower the clock frequency:
>>>>
>>>> # grep mdp_clk /sys/kernel/debug/clk/clk_summary
>>>>            disp_cc_mdss_mdp_clk_src       1        1        0
>>>> 150000000          0     0  50000         ?
>>>>               disp_cc_mdss_mdp_clk       2        2        0
>>>> 150000000          0     0  50000         Y
>>>>
>>>
>>> But won't that just lower the performance state requested by the
>>> &mdss_mdp, while the &mdss still votes for NOM - with the outcome being
>>> that we maintain NOM even if the clock goes down?
>>
>> &mdss doesn't vote on performance state. At least it does not on
>> msm/msm-next which I have at hand right now.
>> &mdss toggles mdss_gdsc, but does not assign any performance state.
>>
> 
> Right, but per the upstream implementation, enabling MDSS_GDSC could in
> itself fail, because unless something else has driven up the performance
> state the enable that trickles up won't actually turn on the supply.
> 
>> On the other hand &mdss_mdp and &dsi0 clearly vote on mmcx's performance state.
>>
> 
> Right, but it does so as part of its clock scaling, so this makes
> perfect sense to me.
> 
>>>
>>>>>
>>>>>>> So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
>>>>>>> MMCX and then use opp-tables associated with the devices that scales the
>>>>>>> clock and thereby actually carries the "required-opps".
>>>>>>
>>>>>> Actually no. I set the performance state in the qcom_cc_map, so that
>>>>>> further register access is possible. Initially I was doing this in the
>>>>>> qcom_cc_really_probe() and it was already too late.
>>>>>> Just to remind: this patchset is not about MDSS_GDSC being parented by
>>>>>> MMCX, it is about dispcc/videocc registers being gated with MMCX.
>>>>>>
>>>>>
>>>>> So you're saying that just enabling MMCX isn't enough to touch the
>>>>> dispcc/videocc registers? If that's the case it seems like MMCX's
>>>>> definition of "on" needs to be adjusted - because just specifying MMCX
>>>>> as the power-domain for dispcc/videocc and enabling pm_runtime should
>>>>> ensure that MMCX is enabled when the clock registers are accessed (I
>>>>> don't see anything like that for the GDSC part though).
>>>>
>>>> No, it is not enough. If I comment out the set_performance_state call,
>>>> the board reboots.
>>>>
>>>> However I can set the opps as low as RET and register access will work.
>>>> I'll run more experiments and if everything works as expected, I can
>>>> use retention or min_svs level in the next iteration.
>>>> Just note that downstream specifies low_svs as minimum voltage level
>>>> for MMCX regulator.
>>>>
>>>
>>> It doesn't make sense to me that a lone power_on on the power-domain
>>> wouldn't give us enough juice to poke the registers.
>>>
>>> But digging into the rpmhpd implementation answers the question, simply
>>> invoking rpmhpd_power_on() is a nop, unless
>>> rpmhpd_set_performance_state() has previously been called, because
>>> pd->corner is 0. So this explains why enable isn't sufficient.
>>>
>>> Compare this with the rpmpd implementation that will send an
>>> enable request to the RPM in this case.

Right, in case of RPMh, there was no separate 'enable' request which
could be sent, there was just a 'corner' request.

I don't completely recall, but the reason to not send a 'default corner'
on enable was perhaps to keep the enable and set_performance orthogonal.

However, given we then decided to send the lowest possible corner
in disable, it perhaps makes sense to send a 'lowest non-zero corner' on enable
as well.

>>
>> Do you think that we should change that to:
>>
>> rpmhpd_aggregate_corner(pd, max(pd->corner, 1)) ?
>>
>> Or
>>
>> rpmhpd_aggregate_corner(pd, max(pd->corner, pd->levels[1])) ?
>>
> 
> In rpmhpd_power_on() and rpmhpd_set_performance_state() we pass the
> index of the entry in pd->levels[] that we want, but in
> rpmhpd_power_off() we pass the value of pd->levels[0].
> 
> So I would suggest dropping the if (pd->corner) and doing:
> 
>    rpmhpd_aggregate_corner(pd, max(pd->corner, 1));

So the index value represents the hlvl (0-15) that eventually gets sent to
rpmh, the pd->levels are the sparse vlvl values that come from the command
DB mappings.

What seems sane is to sent the lowest non-zero vlvl. That in most cases
would be at index 1, but for some which do not support complete off,
it could be at index 0.

> 
> And it seems both rb3 and rb5 still boots with this change (but I need
> to do some more testing to know for sure).
> 
>>>
>>>>> I thought our problem you had was that you need to set a
>>>>> performance_state in order to clock up some of the clocks - e.g.
>>>>> MDP_CLK.
>>>>
>>>> No, even register access needs proper perf state.
>>>>
>>>
>>> Per above finding you're right, enabling a rpmhpd power-domain doesn't
>>> do anything. And I don't find this intuitive or even in line with the
>>> expectations of the api...
>>>
>>>
>>>
>>> A quick test booting rb3 and rb5 seems to indicate that it's possible to
>>> initialize pd->corner to 1 (to ensure that enable at least gives us the
>>> lowest level).
>>>
>>> set_performance_state(0) will however then result in voting for "off",
>>> rather than the lowest enabled level.
>>
>> Well, set_performance_state(0) means that "the device wouldn't
>> participate anymore to find the target performance state of the
>> genpd".
> 
> I agree.
> 
>> Strictly speaking it does not specify whether it is ok to turn
>> it off or not. (like the regulator with the voltage set to 0V).
>> But I'd also like to hear a comment from Stephen here.
>>
> 
> Looking at other power-domains (e.g. gdsc and rpmpd) enabling the
> power-domain means it is no longer off and if you need some specific
> performance state you have to vote for that.
> 
> So I'm also interested in hearing if there's any reasoning behind how
> this was written.
> 
> Regards,
> Bjorn
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation