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

[Bjorn Andersson]

On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@xxxxxxxxxxxxxx> wrote:
>
>
>
> On 7/3/2021 6:24 AM, Bjorn Andersson wrote:
> > 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@xxxxxxxxxx>
> > ---
> >   drivers/soc/qcom/rpmhpd.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> > index 2daa17ba54a3..fa209b479ab3 100644
> > --- a/drivers/soc/qcom/rpmhpd.c
> > +++ b/drivers/soc/qcom/rpmhpd.c
> > @@ -403,12 +403,11 @@ static int rpmhpd_power_on(struct generic_pm_domain *domain)
> >   static int rpmhpd_power_off(struct generic_pm_domain *domain)
> >   {
> >       struct rpmhpd *pd = domain_to_rpmhpd(domain);
> > -     int ret = 0;
> > +     int ret;
> >
> >       mutex_lock(&rpmhpd_lock);
> >
> > -     ret = rpmhpd_aggregate_corner(pd, pd->level[0]);
> > -
> > +     ret = rpmhpd_aggregate_corner(pd, 0);
>
> This won't work for cases where pd->level[0] != 0, rpmh would just ignore this and keep the
> resource at whatever corner it was previously at.
> (unless command DB tells you a 0 is 'valid' for a resource, sending a 0 is a nop)
> The right thing to do is to send in whatever command DB tells you is the lowest level that's valid,
> which is pd->level[0].
>

I'm afraid this doesn't make sense to me.

In rpmh_power_on() if cmd-db tells us that we have [0, 64, ...] and we
request 64 we rpmhpd_aggregate_corner(pd, 1); but in power off, if
cmd-db would provide [64, ...] we would end up sending
rpmhpd_aggregate_corner(pd, 64);
So in power_on we request the corner (i.e. index in the array provided
in cmd-db) and in power-off the same function takes the level?

Can you please help me understand what the actual number we're
supposed to send to the RPMh is? Is it numbers in the range [0-15] or
is it numbers such as {0, 64, 128, ...}?

Afaict it's the prior (i.e. [0-15]), as this is what we currently do
in both power_on and set_performance_state, and it happens to be what
we send in power_off as long as the first level from cmd-db is 0.

Regards,
Bjorn

>
> >       if (!ret)
> >               pd->enabled = false;
> >
> >
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation