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@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?
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@xxxxxxxxxxxxxx>
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
