* [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling @ 2021-07-03 0:54 Bjorn Andersson 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Bjorn Andersson @ 2021-07-03 0:54 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ulf Hansson, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov Cc: linux-arm-msm, linux-kernel During the discussion and investigation of [1] it became apparent that enabling a rpmhpd, without requesting a performance state is a nop. This results in a situation where drivers that normally would just describe their dependency on the power-domain and have the core implicitly enable that power domain also needs to make an explicit vote for a performance state - e.g. by a lone required-opp. [1] https://lore.kernel.org/linux-arm-msm/20210630133149.3204290-4-dmitry.baryshkov@linaro.org/ Bjorn Andersson (2): soc: qcom: rpmhpd: Use corner in power_off soc: qcom: rpmhpd: Make power_on actually enable the domain drivers/soc/qcom/rpmhpd.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-03 0:54 [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Bjorn Andersson @ 2021-07-03 0:54 ` Bjorn Andersson 2021-07-05 4:26 ` Rajendra Nayak ` (2 more replies) 2021-07-03 0:54 ` [PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain Bjorn Andersson 2021-07-05 12:55 ` [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Ulf Hansson 2 siblings, 3 replies; 24+ messages in thread From: Bjorn Andersson @ 2021-07-03 0:54 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ulf Hansson, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov Cc: linux-arm-msm, linux-kernel 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> --- 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); if (!ret) pd->enabled = false; -- 2.29.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson @ 2021-07-05 4:26 ` Rajendra Nayak 2021-07-05 5:06 ` Bjorn Andersson 2021-07-08 0:21 ` Stephen Boyd 2021-07-15 10:40 ` Sibi Sankar 2 siblings, 1 reply; 24+ messages in thread From: Rajendra Nayak @ 2021-07-05 4:26 UTC (permalink / raw) To: Bjorn Andersson, Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov Cc: linux-arm-msm, linux-kernel 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@linaro.org> > --- > 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]. > 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-05 4:26 ` Rajendra Nayak @ 2021-07-05 5:06 ` Bjorn Andersson 2021-07-05 5:40 ` Rajendra Nayak 0 siblings, 1 reply; 24+ messages in thread From: Bjorn Andersson @ 2021-07-05 5:06 UTC (permalink / raw) To: Rajendra Nayak Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, linux-kernel On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@codeaurora.org> 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@linaro.org> > > --- > > 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-05 5:06 ` Bjorn Andersson @ 2021-07-05 5:40 ` Rajendra Nayak 2021-07-07 4:49 ` Bjorn Andersson 0 siblings, 1 reply; 24+ messages in thread From: Rajendra Nayak @ 2021-07-05 5:40 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, linux-kernel On 7/5/2021 10:36 AM, Bjorn Andersson wrote: > On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@codeaurora.org> 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@linaro.org> >>> --- >>> 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? ah that's right, I did not read the commit log properly and got confused. Looks like this bug existed from the day this driver for merged :/, thanks for catching it. Does it make sense to also mark this fix for stable? > > 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 -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-05 5:40 ` Rajendra Nayak @ 2021-07-07 4:49 ` Bjorn Andersson 2021-07-07 6:31 ` Rajendra Nayak 0 siblings, 1 reply; 24+ messages in thread From: Bjorn Andersson @ 2021-07-07 4:49 UTC (permalink / raw) To: Rajendra Nayak Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, linux-kernel On Mon 05 Jul 00:40 CDT 2021, Rajendra Nayak wrote: > On 7/5/2021 10:36 AM, Bjorn Andersson wrote: > > On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@codeaurora.org> 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@linaro.org> > > > > --- > > > > 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? > > ah that's right, I did not read the commit log properly and got confused. Thanks for confirming my understanding. > Looks like this bug existed from the day this driver for merged :/, thanks > for catching it. > Does it make sense to also mark this fix for stable? > I can certainly add a Cc: stable@ as I'm applying this. May I have your R-b? PS. Do you have any input on patch 2/2? That actually solves a practical problem we're seeing. Would it perhaps aid in your need for the new "assigned-opp-level" property? Regards, Bjorn ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-07 4:49 ` Bjorn Andersson @ 2021-07-07 6:31 ` Rajendra Nayak 2021-07-07 15:48 ` Bjorn Andersson 0 siblings, 1 reply; 24+ messages in thread From: Rajendra Nayak @ 2021-07-07 6:31 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, linux-kernel On 7/7/2021 10:19 AM, Bjorn Andersson wrote: > On Mon 05 Jul 00:40 CDT 2021, Rajendra Nayak wrote: >> On 7/5/2021 10:36 AM, Bjorn Andersson wrote: >>> On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@codeaurora.org> 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@linaro.org> >>>>> --- >>>>> 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? >> >> ah that's right, I did not read the commit log properly and got confused. > > Thanks for confirming my understanding. > >> Looks like this bug existed from the day this driver for merged :/, thanks >> for catching it. >> Does it make sense to also mark this fix for stable? >> > > I can certainly add a Cc: stable@ as I'm applying this. sure, sounds good > May I have your R-b? Reviewed-by: Rajendra Nayak <rnayak@codeaurora.org> > > PS. Do you have any input on patch 2/2? That actually solves a practical > problem we're seeing. Would it perhaps aid in your need for the new > "assigned-opp-level" property? We would perhaps still need the 'assigned-opp-level' or equivalent since the default requirement of devices is not always the least level supported, in some cases it might be slightly higher corner which would then need to be set explicitly. I was hoping on getting some more testing done with that patch especially for any regression on the sc7180 and sc7280 devices, which I haven't got to yet. Are you getting these patches ready for merge for the -rc cycle or for the next merge window? -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-07 6:31 ` Rajendra Nayak @ 2021-07-07 15:48 ` Bjorn Andersson 2021-07-07 16:58 ` Dmitry Baryshkov 0 siblings, 1 reply; 24+ messages in thread From: Bjorn Andersson @ 2021-07-07 15:48 UTC (permalink / raw) To: Rajendra Nayak Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, linux-kernel On Wed 07 Jul 01:31 CDT 2021, Rajendra Nayak wrote: > > > On 7/7/2021 10:19 AM, Bjorn Andersson wrote: > > On Mon 05 Jul 00:40 CDT 2021, Rajendra Nayak wrote: > > > On 7/5/2021 10:36 AM, Bjorn Andersson wrote: > > > > On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@codeaurora.org> 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@linaro.org> > > > > > > --- > > > > > > 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? > > > > > > ah that's right, I did not read the commit log properly and got confused. > > > > Thanks for confirming my understanding. > > > > > Looks like this bug existed from the day this driver for merged :/, thanks > > > for catching it. > > > Does it make sense to also mark this fix for stable? > > > > > > > I can certainly add a Cc: stable@ as I'm applying this. > > sure, sounds good > > May I have your R-b? > > Reviewed-by: Rajendra Nayak <rnayak@codeaurora.org> > Thank you. > > > > PS. Do you have any input on patch 2/2? That actually solves a practical > > problem we're seeing. Would it perhaps aid in your need for the new > > "assigned-opp-level" property? > > We would perhaps still need the 'assigned-opp-level' or equivalent since > the default requirement of devices is not always the least level supported, > in some cases it might be slightly higher corner which would then need to > be set explicitly. > Right, for situations where we use assign-clock-rates to drive up the clock rate this mechanism might be needed in order to keep things stable. But I presume as soon as you have some sort of dynamic nature to that you'll be back to an opp-table and the means we already have. > I was hoping on getting some more testing done with that patch especially for > any regression on the sc7180 and sc7280 devices, which I haven't got to yet. > Are you getting these patches ready for merge for the -rc cycle or for the > next merge window? > That would be much appreciated, I've not done extensive testing myself, mostly just booted a few different boards. But I would like to see us correct the MDSS_GDSC->MMCX setup in time for v5.15, in particular since we have a few new users of the mmcx power-domain-regulator arriving in this cycle. Regards, Bjorn ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-07 15:48 ` Bjorn Andersson @ 2021-07-07 16:58 ` Dmitry Baryshkov 0 siblings, 0 replies; 24+ messages in thread From: Dmitry Baryshkov @ 2021-07-07 16:58 UTC (permalink / raw) To: Bjorn Andersson, Rajendra Nayak Cc: Andy Gross, Ulf Hansson, Stephen Boyd, linux-arm-msm, linux-kernel On 07/07/2021 18:48, Bjorn Andersson wrote: > On Wed 07 Jul 01:31 CDT 2021, Rajendra Nayak wrote: > >> >> >> On 7/7/2021 10:19 AM, Bjorn Andersson wrote: >>> On Mon 05 Jul 00:40 CDT 2021, Rajendra Nayak wrote: >>>> On 7/5/2021 10:36 AM, Bjorn Andersson wrote: >>>>> On Sun, Jul 4, 2021 at 11:27 PM Rajendra Nayak <rnayak@codeaurora.org> 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@linaro.org> >>>>>>> --- >>>>>>> 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? >>>> >>>> ah that's right, I did not read the commit log properly and got confused. >>> >>> Thanks for confirming my understanding. >>> >>>> Looks like this bug existed from the day this driver for merged :/, thanks >>>> for catching it. >>>> Does it make sense to also mark this fix for stable? >>>> >>> >>> I can certainly add a Cc: stable@ as I'm applying this. >> >> sure, sounds good >>> May I have your R-b? >> >> Reviewed-by: Rajendra Nayak <rnayak@codeaurora.org> >> > > Thank you. > >>> >>> PS. Do you have any input on patch 2/2? That actually solves a practical >>> problem we're seeing. Would it perhaps aid in your need for the new >>> "assigned-opp-level" property? >> >> We would perhaps still need the 'assigned-opp-level' or equivalent since >> the default requirement of devices is not always the least level supported, >> in some cases it might be slightly higher corner which would then need to >> be set explicitly. >> > > Right, for situations where we use assign-clock-rates to drive up the > clock rate this mechanism might be needed in order to keep things > stable. > > But I presume as soon as you have some sort of dynamic nature to that > you'll be back to an opp-table and the means we already have. > >> I was hoping on getting some more testing done with that patch especially for >> any regression on the sc7180 and sc7280 devices, which I haven't got to yet. >> Are you getting these patches ready for merge for the -rc cycle or for the >> next merge window? >> > > That would be much appreciated, I've not done extensive testing myself, > mostly just booted a few different boards. > > But I would like to see us correct the MDSS_GDSC->MMCX setup in time for > v5.15, in particular since we have a few new users of the mmcx > power-domain-regulator arriving in this cycle. I will rebase my patches on top of this patch series and submit soon. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson 2021-07-05 4:26 ` Rajendra Nayak @ 2021-07-08 0:21 ` Stephen Boyd 2021-07-08 4:35 ` Bjorn Andersson 2021-07-15 10:40 ` Sibi Sankar 2 siblings, 1 reply; 24+ messages in thread From: Stephen Boyd @ 2021-07-08 0:21 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Rajendra Nayak, Ulf Hansson Cc: linux-arm-msm, linux-kernel 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. Maybe we should change rpmhpd_aggregate_corner() to call the argument 'vcorner'? 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? Reviewed-by: Stephen Boyd <swboyd@chromium.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-08 0:21 ` Stephen Boyd @ 2021-07-08 4:35 ` Bjorn Andersson 2021-07-08 5:03 ` Rajendra Nayak 0 siblings, 1 reply; 24+ messages in thread From: Bjorn Andersson @ 2021-07-08 4:35 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Dmitry Baryshkov, Rajendra Nayak, Ulf Hansson, linux-arm-msm, linux-kernel 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"? 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-08 4:35 ` Bjorn Andersson @ 2021-07-08 5:03 ` Rajendra Nayak 2021-07-08 6:51 ` Stephen Boyd 0 siblings, 1 reply; 24+ messages in thread From: Rajendra Nayak @ 2021-07-08 5:03 UTC (permalink / raw) To: Bjorn Andersson, Stephen Boyd Cc: Andy Gross, Dmitry Baryshkov, Ulf Hansson, linux-arm-msm, linux-kernel 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-08 5:03 ` Rajendra Nayak @ 2021-07-08 6:51 ` Stephen Boyd 0 siblings, 0 replies; 24+ messages in thread From: Stephen Boyd @ 2021-07-08 6:51 UTC (permalink / raw) To: Bjorn Andersson, Rajendra Nayak Cc: Andy Gross, Dmitry Baryshkov, Ulf Hansson, linux-arm-msm, linux-kernel Quoting Rajendra Nayak (2021-07-07 22:03:53) > > > 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 ;) Alright then nothing to do. Yay? > > > >> 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 :/ Ah maybe I'm mixing up CPR terms with this stuff. I suspect hlvl is "hardware level" and vlvl is "virtual level", but probably should have been "software level". As far as I remember, the command DB layer was stacked on top so that they could insert more levels in between two levels in the hardware number space and not have to change all the rpmh clients out there (of which there could be many considering all the independent operating systems running on the SoC). For example, [0, 128, 256] maps to [0, 1, 2] and then they realize they need to jam another level between 1 and 2 so they remap 256 to 3 so everyone keeps considering 256 as the previous vlvl to clear the way for 2 to be reused as 198 or something like that. I don't think this ever really changes after the device ships, but it lets them decouple rpmh firmware updates from the rest of the system. As long as they're kept as far apart in vlvl space as there are numbers in hlvl space they can easily do this remap trick and hardware can treat it as levels that bounce around physical voltages that are monotonically increasing. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson 2021-07-05 4:26 ` Rajendra Nayak 2021-07-08 0:21 ` Stephen Boyd @ 2021-07-15 10:40 ` Sibi Sankar 2 siblings, 0 replies; 24+ messages in thread From: Sibi Sankar @ 2021-07-15 10:40 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov, linux-arm-msm, linux-kernel On 2021-07-03 06:24, 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@linaro.org> Reviewed-by: Sibi Sankar <sibis@codeaurora.org> Tested-by: Sibi Sankar <sibis@codeaurora.org> > --- > 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); > if (!ret) > pd->enabled = false; -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 0:54 [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Bjorn Andersson 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson @ 2021-07-03 0:54 ` Bjorn Andersson 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson 2021-07-05 12:55 ` [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Ulf Hansson 2 siblings, 1 reply; 24+ messages in thread From: Bjorn Andersson @ 2021-07-03 0:54 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ulf Hansson, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov Cc: linux-arm-msm, linux-kernel The general expectation is that powering on a power-domain should make the power domain deliver some power, and if a specific performace state is needed further requests has to be made. But in contrast with other power-domain implementations (e.g. rpmpd) the RPMh does not have an interface to enable the power, so the driver has to vote for a particular corner (performance level) in rpmh_power_on(). But the corner is never initialized, so a typical request to simply enable the power domain would not actually turn on the hardware. Further more, when no more clients vote for a performance state (i.e. the aggregated vote is 0) the power domain would be turn off. Fix both of these issues by always voting for a corner with non-zero value, when the power domain is enabled. The tracking of the lowest non-zero corner is performed to handle the corner case if there's ever a domain with a non-zero lowest corner, in which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() would be allowed to use this lowest corner. Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/soc/qcom/rpmhpd.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c index fa209b479ab3..29a339942301 100644 --- a/drivers/soc/qcom/rpmhpd.c +++ b/drivers/soc/qcom/rpmhpd.c @@ -30,6 +30,7 @@ * @active_only: True if it represents an Active only peer * @corner: current corner * @active_corner: current active corner + * @enable_corner: lowest non-zero corner * @level: An array of level (vlvl) to corner (hlvl) mappings * derived from cmd-db * @level_count: Number of levels supported by the power domain. max @@ -47,6 +48,7 @@ struct rpmhpd { const bool active_only; unsigned int corner; unsigned int active_corner; + unsigned int enable_corner; u32 level[RPMH_ARC_MAX_LEVELS]; size_t level_count; bool enabled; @@ -385,13 +387,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner) static int rpmhpd_power_on(struct generic_pm_domain *domain) { struct rpmhpd *pd = domain_to_rpmhpd(domain); - int ret = 0; + unsigned int corner; + int ret; mutex_lock(&rpmhpd_lock); - if (pd->corner) - ret = rpmhpd_aggregate_corner(pd, pd->corner); - + corner = max(pd->corner, pd->enable_corner); + ret = rpmhpd_aggregate_corner(pd, corner); if (!ret) pd->enabled = true; @@ -472,6 +474,10 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd) for (i = 0; i < rpmhpd->level_count; i++) { rpmhpd->level[i] = buf[i]; + /* Remember the first non-zero corner */ + if (!rpmhpd->enable_corner) + rpmhpd->enable_corner = i; + /* * The AUX data may be zero padded. These 0 valued entries at * the end of the map must be ignored. -- 2.29.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 0:54 ` [PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain Bjorn Andersson @ 2021-07-03 2:54 ` Bjorn Andersson 2021-07-08 0:23 ` Stephen Boyd ` (4 more replies) 0 siblings, 5 replies; 24+ messages in thread From: Bjorn Andersson @ 2021-07-03 2:54 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ulf Hansson, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov Cc: linux-arm-msm, linux-kernel The general expectation is that powering on a power-domain should make the power domain deliver some power, and if a specific performace state is needed further requests has to be made. But in contrast with other power-domain implementations (e.g. rpmpd) the RPMh does not have an interface to enable the power, so the driver has to vote for a particular corner (performance level) in rpmh_power_on(). But the corner is never initialized, so a typical request to simply enable the power domain would not actually turn on the hardware. Further more, when no more clients vote for a performance state (i.e. the aggregated vote is 0) the power domain would be turn off. Fix both of these issues by always voting for a corner with non-zero value, when the power domain is enabled. The tracking of the lowest non-zero corner is performed to handle the corner case if there's ever a domain with a non-zero lowest corner, in which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() would be allowed to use this lowest corner. Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Resending because the hunk in rpmhpd_update_level_mapping() was left in the index. drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c index fa209b479ab3..76ea6b053ef0 100644 --- a/drivers/soc/qcom/rpmhpd.c +++ b/drivers/soc/qcom/rpmhpd.c @@ -30,6 +30,7 @@ * @active_only: True if it represents an Active only peer * @corner: current corner * @active_corner: current active corner + * @enable_corner: lowest non-zero corner * @level: An array of level (vlvl) to corner (hlvl) mappings * derived from cmd-db * @level_count: Number of levels supported by the power domain. max @@ -47,6 +48,7 @@ struct rpmhpd { const bool active_only; unsigned int corner; unsigned int active_corner; + unsigned int enable_corner; u32 level[RPMH_ARC_MAX_LEVELS]; size_t level_count; bool enabled; @@ -385,13 +387,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner) static int rpmhpd_power_on(struct generic_pm_domain *domain) { struct rpmhpd *pd = domain_to_rpmhpd(domain); - int ret = 0; + unsigned int corner; + int ret; mutex_lock(&rpmhpd_lock); - if (pd->corner) - ret = rpmhpd_aggregate_corner(pd, pd->corner); - + corner = max(pd->corner, pd->enable_corner); + ret = rpmhpd_aggregate_corner(pd, corner); if (!ret) pd->enabled = true; @@ -436,6 +438,10 @@ static int rpmhpd_set_performance_state(struct generic_pm_domain *domain, i--; if (pd->enabled) { + /* Ensure that the domain isn't turn off */ + if (i < pd->enable_corner) + i = pd->enable_corner; + ret = rpmhpd_aggregate_corner(pd, i); if (ret) goto out; @@ -472,6 +478,10 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd) for (i = 0; i < rpmhpd->level_count; i++) { rpmhpd->level[i] = buf[i]; + /* Remember the first non-zero corner */ + if (!rpmhpd->enable_corner) + rpmhpd->enable_corner = i; + /* * The AUX data may be zero padded. These 0 valued entries at * the end of the map must be ignored. -- 2.29.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson @ 2021-07-08 0:23 ` Stephen Boyd 2021-07-08 0:25 ` Stephen Boyd ` (3 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: Stephen Boyd @ 2021-07-08 0:23 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Rajendra Nayak, Ulf Hansson Cc: linux-arm-msm, linux-kernel Quoting Bjorn Andersson (2021-07-02 19:54:49) > The general expectation is that powering on a power-domain should make > the power domain deliver some power, and if a specific performace state > is needed further requests has to be made. > > But in contrast with other power-domain implementations (e.g. rpmpd) the > RPMh does not have an interface to enable the power, so the driver has > to vote for a particular corner (performance level) in rpmh_power_on(). > > But the corner is never initialized, so a typical request to simply > enable the power domain would not actually turn on the hardware. Further > more, when no more clients vote for a performance state (i.e. the > aggregated vote is 0) the power domain would be turn off. s/turn/turned/ > > Fix both of these issues by always voting for a corner with non-zero > value, when the power domain is enabled. > > The tracking of the lowest non-zero corner is performed to handle the > corner case if there's ever a domain with a non-zero lowest corner, in > which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() > would be allowed to use this lowest corner. > > Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson 2021-07-08 0:23 ` Stephen Boyd @ 2021-07-08 0:25 ` Stephen Boyd 2021-07-14 9:22 ` Rajendra Nayak ` (2 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: Stephen Boyd @ 2021-07-08 0:25 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Rajendra Nayak, Ulf Hansson Cc: linux-arm-msm, linux-kernel Quoting Bjorn Andersson (2021-07-02 19:54:49) > @@ -472,6 +478,10 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd) > for (i = 0; i < rpmhpd->level_count; i++) { > rpmhpd->level[i] = buf[i]; BTW, this looks like it should actually be an __le16 pointer and then we should do le16_to_cpup() here. Hooray for void pointers. > > + /* Remember the first non-zero corner */ > + if (!rpmhpd->enable_corner) > + rpmhpd->enable_corner = i; > + ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson 2021-07-08 0:23 ` Stephen Boyd 2021-07-08 0:25 ` Stephen Boyd @ 2021-07-14 9:22 ` Rajendra Nayak 2021-07-15 12:16 ` Sibi Sankar 2021-08-12 13:21 ` Dmitry Baryshkov 4 siblings, 0 replies; 24+ messages in thread From: Rajendra Nayak @ 2021-07-14 9:22 UTC (permalink / raw) To: Bjorn Andersson, Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov Cc: linux-arm-msm, linux-kernel On 7/3/2021 8:24 AM, Bjorn Andersson wrote: > The general expectation is that powering on a power-domain should make > the power domain deliver some power, and if a specific performace state > is needed further requests has to be made. > > But in contrast with other power-domain implementations (e.g. rpmpd) the > RPMh does not have an interface to enable the power, so the driver has > to vote for a particular corner (performance level) in rpmh_power_on(). > > But the corner is never initialized, so a typical request to simply > enable the power domain would not actually turn on the hardware. Further > more, when no more clients vote for a performance state (i.e. the > aggregated vote is 0) the power domain would be turn off. > > Fix both of these issues by always voting for a corner with non-zero > value, when the power domain is enabled. > > The tracking of the lowest non-zero corner is performed to handle the > corner case if there's ever a domain with a non-zero lowest corner, in > which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() > would be allowed to use this lowest corner. > > Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Did some testing on sc7180 and sc7280 devices, no regressions seen, Reviewed-by: Rajendra Nayak <rnayak@codeaurora.org> Tested-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > > Resending because the hunk in rpmhpd_update_level_mapping() was left in the > index. > > drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > index fa209b479ab3..76ea6b053ef0 100644 > --- a/drivers/soc/qcom/rpmhpd.c > +++ b/drivers/soc/qcom/rpmhpd.c > @@ -30,6 +30,7 @@ > * @active_only: True if it represents an Active only peer > * @corner: current corner > * @active_corner: current active corner > + * @enable_corner: lowest non-zero corner > * @level: An array of level (vlvl) to corner (hlvl) mappings > * derived from cmd-db > * @level_count: Number of levels supported by the power domain. max > @@ -47,6 +48,7 @@ struct rpmhpd { > const bool active_only; > unsigned int corner; > unsigned int active_corner; > + unsigned int enable_corner; > u32 level[RPMH_ARC_MAX_LEVELS]; > size_t level_count; > bool enabled; > @@ -385,13 +387,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner) > static int rpmhpd_power_on(struct generic_pm_domain *domain) > { > struct rpmhpd *pd = domain_to_rpmhpd(domain); > - int ret = 0; > + unsigned int corner; > + int ret; > > mutex_lock(&rpmhpd_lock); > > - if (pd->corner) > - ret = rpmhpd_aggregate_corner(pd, pd->corner); > - > + corner = max(pd->corner, pd->enable_corner); > + ret = rpmhpd_aggregate_corner(pd, corner); > if (!ret) > pd->enabled = true; > > @@ -436,6 +438,10 @@ static int rpmhpd_set_performance_state(struct generic_pm_domain *domain, > i--; > > if (pd->enabled) { > + /* Ensure that the domain isn't turn off */ > + if (i < pd->enable_corner) > + i = pd->enable_corner; > + > ret = rpmhpd_aggregate_corner(pd, i); > if (ret) > goto out; > @@ -472,6 +478,10 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd) > for (i = 0; i < rpmhpd->level_count; i++) { > rpmhpd->level[i] = buf[i]; > > + /* Remember the first non-zero corner */ > + if (!rpmhpd->enable_corner) > + rpmhpd->enable_corner = i; > + > /* > * The AUX data may be zero padded. These 0 valued entries at > * the end of the map must be ignored. > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson ` (2 preceding siblings ...) 2021-07-14 9:22 ` Rajendra Nayak @ 2021-07-15 12:16 ` Sibi Sankar 2021-07-15 12:24 ` Rajendra Nayak 2021-08-12 13:21 ` Dmitry Baryshkov 4 siblings, 1 reply; 24+ messages in thread From: Sibi Sankar @ 2021-07-15 12:16 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov, linux-arm-msm, linux-kernel Hey Bjorn, Thanks for the patch. On 2021-07-03 08:24, Bjorn Andersson wrote: > The general expectation is that powering on a power-domain should make > the power domain deliver some power, and if a specific performace state s/performace/performance/ > is needed further requests has to be made. > > But in contrast with other power-domain implementations (e.g. rpmpd) > the > RPMh does not have an interface to enable the power, so the driver has > to vote for a particular corner (performance level) in rpmh_power_on(). > > But the corner is never initialized, so a typical request to simply > enable the power domain would not actually turn on the hardware. > Further > more, when no more clients vote for a performance state (i.e. the > aggregated vote is 0) the power domain would be turn off. > > Fix both of these issues by always voting for a corner with non-zero > value, when the power domain is enabled. > > The tracking of the lowest non-zero corner is performed to handle the > corner case if there's ever a domain with a non-zero lowest corner, in > which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() > would be allowed to use this lowest corner. > > Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Resending because the hunk in rpmhpd_update_level_mapping() was left in > the > index. > > drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > index fa209b479ab3..76ea6b053ef0 100644 > --- a/drivers/soc/qcom/rpmhpd.c > +++ b/drivers/soc/qcom/rpmhpd.c > @@ -30,6 +30,7 @@ > * @active_only: True if it represents an Active only peer > * @corner: current corner > * @active_corner: current active corner > + * @enable_corner: lowest non-zero corner > * @level: An array of level (vlvl) to corner (hlvl) mappings > * derived from cmd-db > * @level_count: Number of levels supported by the power domain. max > @@ -47,6 +48,7 @@ struct rpmhpd { > const bool active_only; > unsigned int corner; > unsigned int active_corner; > + unsigned int enable_corner; > u32 level[RPMH_ARC_MAX_LEVELS]; > size_t level_count; > bool enabled; > @@ -385,13 +387,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd > *pd, unsigned int corner) > static int rpmhpd_power_on(struct generic_pm_domain *domain) > { > struct rpmhpd *pd = domain_to_rpmhpd(domain); > - int ret = 0; > + unsigned int corner; > + int ret; > > mutex_lock(&rpmhpd_lock); > > - if (pd->corner) > - ret = rpmhpd_aggregate_corner(pd, pd->corner); > - > + corner = max(pd->corner, pd->enable_corner); > + ret = rpmhpd_aggregate_corner(pd, corner); > if (!ret) > pd->enabled = true; > > @@ -436,6 +438,10 @@ static int rpmhpd_set_performance_state(struct > generic_pm_domain *domain, > i--; > > if (pd->enabled) { > + /* Ensure that the domain isn't turn off */ > + if (i < pd->enable_corner) > + i = pd->enable_corner; > + > ret = rpmhpd_aggregate_corner(pd, i); > if (ret) > goto out; > @@ -472,6 +478,10 @@ static int rpmhpd_update_level_mapping(struct > rpmhpd *rpmhpd) > for (i = 0; i < rpmhpd->level_count; i++) { > rpmhpd->level[i] = buf[i]; > > + /* Remember the first non-zero corner */ Shouldn't we be tracking the corner that corresponds to the first non-zero level instead? > + if (!rpmhpd->enable_corner) > + rpmhpd->enable_corner = i; > + > /* > * The AUX data may be zero padded. These 0 valued entries at > * the end of the map must be ignored. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-15 12:16 ` Sibi Sankar @ 2021-07-15 12:24 ` Rajendra Nayak 0 siblings, 0 replies; 24+ messages in thread From: Rajendra Nayak @ 2021-07-15 12:24 UTC (permalink / raw) To: Sibi Sankar, Bjorn Andersson Cc: Andy Gross, Ulf Hansson, Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, linux-kernel On 7/15/2021 5:46 PM, Sibi Sankar wrote: > Hey Bjorn, > > Thanks for the patch. > > On 2021-07-03 08:24, Bjorn Andersson wrote: >> The general expectation is that powering on a power-domain should make >> the power domain deliver some power, and if a specific performace state > > s/performace/performance/ > >> is needed further requests has to be made. >> >> But in contrast with other power-domain implementations (e.g. rpmpd) the >> RPMh does not have an interface to enable the power, so the driver has >> to vote for a particular corner (performance level) in rpmh_power_on(). >> >> But the corner is never initialized, so a typical request to simply >> enable the power domain would not actually turn on the hardware. Further >> more, when no more clients vote for a performance state (i.e. the >> aggregated vote is 0) the power domain would be turn off. >> >> Fix both of these issues by always voting for a corner with non-zero >> value, when the power domain is enabled. >> >> The tracking of the lowest non-zero corner is performed to handle the >> corner case if there's ever a domain with a non-zero lowest corner, in >> which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() >> would be allowed to use this lowest corner. >> >> Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") >> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> --- >> >> Resending because the hunk in rpmhpd_update_level_mapping() was left in the >> index. >> >> drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c >> index fa209b479ab3..76ea6b053ef0 100644 >> --- a/drivers/soc/qcom/rpmhpd.c >> +++ b/drivers/soc/qcom/rpmhpd.c >> @@ -30,6 +30,7 @@ >> * @active_only: True if it represents an Active only peer >> * @corner: current corner >> * @active_corner: current active corner >> + * @enable_corner: lowest non-zero corner >> * @level: An array of level (vlvl) to corner (hlvl) mappings >> * derived from cmd-db >> * @level_count: Number of levels supported by the power domain. max >> @@ -47,6 +48,7 @@ struct rpmhpd { >> const bool active_only; >> unsigned int corner; >> unsigned int active_corner; >> + unsigned int enable_corner; >> u32 level[RPMH_ARC_MAX_LEVELS]; >> size_t level_count; >> bool enabled; >> @@ -385,13 +387,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd >> *pd, unsigned int corner) >> static int rpmhpd_power_on(struct generic_pm_domain *domain) >> { >> struct rpmhpd *pd = domain_to_rpmhpd(domain); >> - int ret = 0; >> + unsigned int corner; >> + int ret; >> >> mutex_lock(&rpmhpd_lock); >> >> - if (pd->corner) >> - ret = rpmhpd_aggregate_corner(pd, pd->corner); >> - >> + corner = max(pd->corner, pd->enable_corner); >> + ret = rpmhpd_aggregate_corner(pd, corner); >> if (!ret) >> pd->enabled = true; >> >> @@ -436,6 +438,10 @@ static int rpmhpd_set_performance_state(struct >> generic_pm_domain *domain, >> i--; >> >> if (pd->enabled) { >> + /* Ensure that the domain isn't turn off */ >> + if (i < pd->enable_corner) >> + i = pd->enable_corner; >> + >> ret = rpmhpd_aggregate_corner(pd, i); >> if (ret) >> goto out; >> @@ -472,6 +478,10 @@ static int rpmhpd_update_level_mapping(struct >> rpmhpd *rpmhpd) >> for (i = 0; i < rpmhpd->level_count; i++) { >> rpmhpd->level[i] = buf[i]; >> >> + /* Remember the first non-zero corner */ > > Shouldn't we be tracking the corner that > corresponds to the first non-zero level > instead? Thats correct, thanks for spotting this, the first non-zero corner is always 1 :) > >> + if (!rpmhpd->enable_corner) >> + rpmhpd->enable_corner = i; >> + >> /* >> * The AUX data may be zero padded. These 0 valued entries at >> * the end of the map must be ignored. > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson ` (3 preceding siblings ...) 2021-07-15 12:16 ` Sibi Sankar @ 2021-08-12 13:21 ` Dmitry Baryshkov 2021-08-13 9:45 ` Ulf Hansson 4 siblings, 1 reply; 24+ messages in thread From: Dmitry Baryshkov @ 2021-08-12 13:21 UTC (permalink / raw) To: Bjorn Andersson, Andy Gross, Ulf Hansson, Stephen Boyd, Rajendra Nayak Cc: linux-arm-msm, linux-kernel On 03/07/2021 05:54, Bjorn Andersson wrote: > The general expectation is that powering on a power-domain should make > the power domain deliver some power, and if a specific performace state > is needed further requests has to be made. > > But in contrast with other power-domain implementations (e.g. rpmpd) the > RPMh does not have an interface to enable the power, so the driver has > to vote for a particular corner (performance level) in rpmh_power_on(). > > But the corner is never initialized, so a typical request to simply > enable the power domain would not actually turn on the hardware. Further > more, when no more clients vote for a performance state (i.e. the > aggregated vote is 0) the power domain would be turn off. > > Fix both of these issues by always voting for a corner with non-zero > value, when the power domain is enabled. > > The tracking of the lowest non-zero corner is performed to handle the > corner case if there's ever a domain with a non-zero lowest corner, in > which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() > would be allowed to use this lowest corner. > > Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Resending because the hunk in rpmhpd_update_level_mapping() was left in the > index. So, colleagues, what is the fate of this patch? Is it going to be applied? Or we agree that current approach (power_on + set_performance_state) is the expected behaviour? My patches on gdsc rework depend on this patch, but I can rework in them in favour of required-opp approach. > > drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > index fa209b479ab3..76ea6b053ef0 100644 > --- a/drivers/soc/qcom/rpmhpd.c > +++ b/drivers/soc/qcom/rpmhpd.c > @@ -30,6 +30,7 @@ > * @active_only: True if it represents an Active only peer > * @corner: current corner > * @active_corner: current active corner > + * @enable_corner: lowest non-zero corner > * @level: An array of level (vlvl) to corner (hlvl) mappings > * derived from cmd-db > * @level_count: Number of levels supported by the power domain. max > @@ -47,6 +48,7 @@ struct rpmhpd { > const bool active_only; > unsigned int corner; > unsigned int active_corner; > + unsigned int enable_corner; > u32 level[RPMH_ARC_MAX_LEVELS]; > size_t level_count; > bool enabled; > @@ -385,13 +387,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner) > static int rpmhpd_power_on(struct generic_pm_domain *domain) > { > struct rpmhpd *pd = domain_to_rpmhpd(domain); > - int ret = 0; > + unsigned int corner; > + int ret; > > mutex_lock(&rpmhpd_lock); > > - if (pd->corner) > - ret = rpmhpd_aggregate_corner(pd, pd->corner); > - > + corner = max(pd->corner, pd->enable_corner); > + ret = rpmhpd_aggregate_corner(pd, corner); > if (!ret) > pd->enabled = true; > > @@ -436,6 +438,10 @@ static int rpmhpd_set_performance_state(struct generic_pm_domain *domain, > i--; > > if (pd->enabled) { > + /* Ensure that the domain isn't turn off */ > + if (i < pd->enable_corner) > + i = pd->enable_corner; > + > ret = rpmhpd_aggregate_corner(pd, i); > if (ret) > goto out; > @@ -472,6 +478,10 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd) > for (i = 0; i < rpmhpd->level_count; i++) { > rpmhpd->level[i] = buf[i]; > > + /* Remember the first non-zero corner */ > + if (!rpmhpd->enable_corner) > + rpmhpd->enable_corner = i; > + > /* > * The AUX data may be zero padded. These 0 valued entries at > * the end of the map must be ignored. > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain 2021-08-12 13:21 ` Dmitry Baryshkov @ 2021-08-13 9:45 ` Ulf Hansson 0 siblings, 0 replies; 24+ messages in thread From: Ulf Hansson @ 2021-08-13 9:45 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Bjorn Andersson, Andy Gross, Stephen Boyd, Rajendra Nayak, linux-arm-msm, Linux Kernel Mailing List On Thu, 12 Aug 2021 at 15:21, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 03/07/2021 05:54, Bjorn Andersson wrote: > > The general expectation is that powering on a power-domain should make > > the power domain deliver some power, and if a specific performace state > > is needed further requests has to be made. > > > > But in contrast with other power-domain implementations (e.g. rpmpd) the > > RPMh does not have an interface to enable the power, so the driver has > > to vote for a particular corner (performance level) in rpmh_power_on(). > > > > But the corner is never initialized, so a typical request to simply > > enable the power domain would not actually turn on the hardware. Further > > more, when no more clients vote for a performance state (i.e. the > > aggregated vote is 0) the power domain would be turn off. > > > > Fix both of these issues by always voting for a corner with non-zero > > value, when the power domain is enabled. > > > > The tracking of the lowest non-zero corner is performed to handle the > > corner case if there's ever a domain with a non-zero lowest corner, in > > which case both rpmh_power_on() and rpmh_rpmhpd_set_performance_state() > > would be allowed to use this lowest corner. > > > > Fixes: 279b7e8a62cc ("soc: qcom: rpmhpd: Add RPMh power domain driver") > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Resending because the hunk in rpmhpd_update_level_mapping() was left in the > > index. > > So, colleagues, what is the fate of this patch? Is it going to be > applied? Or we agree that current approach (power_on + > set_performance_state) is the expected behaviour? My patches on gdsc > rework depend on this patch, but I can rework in them in favour of > required-opp approach. Today, genpd treats performance states and power on/off states as orthogonal. You know this already, ofcourse. Although, to clarify, this means that the genpd provider has to deal with the scenario when its ->set_performance_state() callback may be invoked, while the PM domain is turned off, for example. Similarly, genpd may power on the PM domain by invoking the ->power_on() callback, before the ->set_performance_state() has been invoked. And finally, the power domain may be turned off even if there are some active votes for a performance state. So for now, the genpd provider needs to deal with these cases. Yes, we have discussed changing the behaviour in genpd around this and I think there have been some good reasons for it, but we are not there, at least yet. [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling 2021-07-03 0:54 [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Bjorn Andersson 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson 2021-07-03 0:54 ` [PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain Bjorn Andersson @ 2021-07-05 12:55 ` Ulf Hansson 2 siblings, 0 replies; 24+ messages in thread From: Ulf Hansson @ 2021-07-05 12:55 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Stephen Boyd, Rajendra Nayak, Dmitry Baryshkov, linux-arm-msm, Linux Kernel Mailing List On Sat, 3 Jul 2021 at 02:55, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > During the discussion and investigation of [1] it became apparent that > enabling a rpmhpd, without requesting a performance state is a nop. This > results in a situation where drivers that normally would just describe > their dependency on the power-domain and have the core implicitly enable > that power domain also needs to make an explicit vote for a performance > state - e.g. by a lone required-opp. > > [1] https://lore.kernel.org/linux-arm-msm/20210630133149.3204290-4-dmitry.baryshkov@linaro.org/ > > Bjorn Andersson (2): > soc: qcom: rpmhpd: Use corner in power_off > soc: qcom: rpmhpd: Make power_on actually enable the domain > > drivers/soc/qcom/rpmhpd.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > FWIW: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-08-13 9:46 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-03 0:54 [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Bjorn Andersson 2021-07-03 0:54 ` [PATCH 1/2] soc: qcom: rpmhpd: Use corner in power_off Bjorn Andersson 2021-07-05 4:26 ` Rajendra Nayak 2021-07-05 5:06 ` Bjorn Andersson 2021-07-05 5:40 ` Rajendra Nayak 2021-07-07 4:49 ` Bjorn Andersson 2021-07-07 6:31 ` Rajendra Nayak 2021-07-07 15:48 ` Bjorn Andersson 2021-07-07 16:58 ` Dmitry Baryshkov 2021-07-08 0:21 ` Stephen Boyd 2021-07-08 4:35 ` Bjorn Andersson 2021-07-08 5:03 ` Rajendra Nayak 2021-07-08 6:51 ` Stephen Boyd 2021-07-15 10:40 ` Sibi Sankar 2021-07-03 0:54 ` [PATCH 2/2] soc: qcom: rpmhpd: Make power_on actually enable the domain Bjorn Andersson 2021-07-03 2:54 ` [RESEND PATCH " Bjorn Andersson 2021-07-08 0:23 ` Stephen Boyd 2021-07-08 0:25 ` Stephen Boyd 2021-07-14 9:22 ` Rajendra Nayak 2021-07-15 12:16 ` Sibi Sankar 2021-07-15 12:24 ` Rajendra Nayak 2021-08-12 13:21 ` Dmitry Baryshkov 2021-08-13 9:45 ` Ulf Hansson 2021-07-05 12:55 ` [PATCH 0/2] soc: qcom: rpmhpd: Improve rpmhpd enable handling Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.