Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off

[Rajendra Nayak <rnayak@codeaurora.org>]

On 7/8/2021 10:05 AM, Bjorn Andersson wrote:
> On Wed 07 Jul 19:21 CDT 2021, Stephen Boyd wrote:
> 
>> Quoting Bjorn Andersson (2021-07-02 17:54:15)
>>> rpmhpd_aggregate_corner() takes a corner as parameter, but in
>>> rpmhpd_power_off() the code requests the level of the first corner
>>> instead.
>>>
>>> In all (known) current cases the first corner has level 0, so this
>>> change should be a nop, but in case that there's a power domain with a
>>> non-zero lowest level this makes sure that rpmhpd_power_off() actually
>>> requests the lowest level - which is the closest to "power off" we can
>>> get.
>>>
>>> While touching the code, also skip the unnecessary zero-initialization
>>> of "ret".
>>>
>>> Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver")
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>
>> I think this is why qcom folks talk about "virtual corner" and "physical
>> corner" because there's the one in command DB and the one in hardware.
> 
> I think the driver uses "level" and "corner" to denote the two different
> number spaces, so I think we're good...now that we after this patch
> don't pass a "level" as "corner" during power_off ;)
> 
>> Maybe we should change rpmhpd_aggregate_corner() to call the argument
>> 'vcorner'?
> 
> So "virtual corner" is "corner" and "physical corner" is level? I.e. 256
> is a "physical corner"?

I haven't heard of anything called a 'physical corner'. These were always
referred to as virtual corners, on older platforms it was just one contiguous
number space, on newer ones we added another higher level sparse number space
just for more fun :)
Command DB refers to these as hlvl and vlvl, I haven;t yet figured out what their
full forms are :/

> 
> Or did you get the suggestion backwards?
> 
>> Unfortunately we can't really build a type system here to
>> make this problem easy to catch with a mismatched type, unless there's
>> some sort of typedef trick we can play?
>>
> 
> s/i/corner/ in rpmhpd_set_performance_state() would further enforce the
> naming scheme used and reduce the risk for future confusion.
> 
> But we did just squash the final bug... ;)
> 
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> 
> Thanks,
> Bjorn
> 

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