Messages in this thread | | | Subject | Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c | From | Rajendra Nayak <> | Date | Tue, 27 Jul 2021 13:05:52 +0530 |
| |
On 7/25/2021 10:31 PM, Bjorn Andersson wrote: > On Mon 19 Jul 23:29 CDT 2021, Rajendra Nayak wrote: > >> >> >> On 7/20/2021 12:49 AM, Bjorn Andersson wrote: >>> On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: >>> >>>> >>>> >>>> On 7/17/2021 3:29 AM, Bjorn Andersson wrote: >>>>> On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: >>>>> >>>>>> Quoting Bjorn Andersson (2021-07-16 13:52:12) >>>>>>> On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: >>>>>>> >>>>>>>> Quoting Bjorn Andersson (2021-07-16 13:18:56) >>>>>>>>> On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: >>>>>>>>> >>>>>>>>>> qup-i2c devices on sc7180 are clocked with a fixed clock (19.2 MHz) >>>>>>>>>> Though qup-i2c does not support DVFS, it still needs to vote for a >>>>>>>>>> performance state on 'CX' to satisfy the 19.2 Mhz clock frequency >>>>>>>>>> requirement. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Sounds good, but... >>>>>>>>> >>>>>>>>>> Use 'required-opps' to pass this information from >>>>>>>>>> device tree, and also add the power-domains property to specify >>>>>>>>>> the CX power-domain. >>>>>>>>>> >>>>>>>>> >>>>>>>>> ..is the required-opps really needed with my rpmhpd patch in place? >>>>>>>>> >>>>>>>> >>>>>>>> Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for >>>>>>>> CX. >>>>>>> >>>>>>> On e.g. sm8250 the first available non-zero corner presented in cmd-db >>>>>>> is low_svs. >>>> >>>> what rail is this? the mmcx? Perhaps it does not support RET. >>>> cx usually supports both collapse state and RET. >>>> >>> >>> That was the one I was specifically looking at for the MDSS_GDSC->MMCX >>> issue, so it's likely I didn't look elsewhere. >>> >>>>>> >>>>>> Indeed. On sc7180 it's not the first non-zero corner. I suppose >>>>>> retention for CX isn't actually used when the SoC is awake so your >>>>>> rpmhpd patch is putting in a vote for something that doesn't do anything >>>>>> at runtime for CX? I imagine that rpmh only sets the aggregate corner to >>>>>> retention when the whole SoC is suspended/sleeping, otherwise things >>>>>> wouldn't go very well. Similarly, min_svs may be VDD minimization? If >>>>>> so, those first two states are basically states that shouldn't be used >>>>>> at runtime, almost like sleep states. >>>>>> >>>>> >>>>> But if that's the case, I don't think it's appropriate for the "enabled >>>>> state" of the domain to use any of those corners. >>>> >>>> I rechecked the downstream kernels where all this voting happens from within >>>> the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is >>>> right that RET is not something that's voted on while in active state. >>>> >>>> But always going with something just above the ret level while active will also >>>> not work for all devices, for instance for i2c on 7180, it needs a cx vote of >>>> low svs while the rail (cx) does support something lower than that which is min svs. >>>> (why can't it just work with min svs?, I don't know, these values and recommendations >>>> come in from the voltage plans published by HW teams for every SoC and we just end up >>>> using them in SW, perhaps something to dig further and understand which I will try and >>>> do but these are the values in voltage plans and downstream kernels which work for now) >>>> >>> >>> So to some degree this invalidates my argumentation about the >>> enabled_corner in rpmhpd, given that "enabled" means a different corner >>> for each rail - not just the one with lowest non-zero value. >> >> Right, it might work in some cases but might not work for all. >> > > Which makes it way less desirable. > > The enable state for rpmhpd power domains doesn't meet my expectations > for how a power domain should behave,
Right and that's perhaps because these are not the usual power-domains, which have one "on/active" state and one or more "off/inactive" states (off/ret/clock-stop) Rpmhpd has multiple "on/active" states, and whats "on/active" for one consumer might not be "on/active" for another, so this information is hard to be managed at a generic level and these requests in some way or the other need to come in explicitly from the resp. consumers.
> but we should at least be > consistent across all consumers of it then... > > > But the original issue remains, that when a device is powered by > MDSS_GDSC, which is a subdomain of MMCX we still need to ensure that > "on" for MMCX is actually "on" - which just happens to be the first > non-0 corner. > > But I presume we will end up having to do the same with &gcc's GDSCs, > which are subdomains of CX and MX where this isn't true. > >>> >>> So perhaps instead of introducing the enabled_corner we need to >>> introduce your patch and slap a WARN_ON(corner == 0) in >>> rpmhpd_power_on() - to ensure that all clients that uses a rpmhpd domain >>> actually do vote for a high enough corner? >> >> So this would mean the expectation is that the clients set the perf state/corner >> before they call power_on? I don;t think that's the case today with most clients, >> infact its the opposite, we power on first and then make a call to set the perf >> state of the domain. >> > > You're right, it's pretty much always the opposite, given that genpd > will always enable the domain during attach. > > Regards, > Bjorn > >>> >>> Regards, >>> Bjorn >>> >>>>> >>>>> As this means that anyone who needs any of the rpmhpd domains active >>>>> also needs to specify required-opps, which wouldn't be needed for any >>>>> other power domain provider. >>>>> >>>>> And more importantly it means that a device sitting in a GDSC, which >>>>> would be parented by a rpmhpd domain has no way to specify the GDSC and >>>>> trickle the minimum-vote up to the rpmhpd domain. (And I know that we >>>>> don't describe the parentship of the GDSCs today, but this patch >>>>> tells me that it's around the corner - for more than MMCX) >>>>> >>>>> Regards, >>>>> Bjorn >>>>> >>>>>>> >>>>>>> And if this (which?) clock requires a higher corner than the lowest >>>>>>> possible in order to tick at this "lowest" frequency, I'm certainly >>>>>>> interested in some more details. >>>>>>> >>>> >>>> -- >>>> 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
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |