Messages in this thread | | | Subject | Re: [PATCH v2 1/3] dt-bindings: usb: qcom,dwc3: Add multi-pd bindings for dwc3 qcom | From | Rajendra Nayak <> | Date | Thu, 28 Oct 2021 09:26:58 +0530 |
| |
On 10/27/2021 7:54 PM, Ulf Hansson wrote: > On Wed, 27 Oct 2021 at 06:55, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: >> >> On Tue 26 Oct 19:48 CDT 2021, Stephen Boyd wrote: >> >>> +Rajendra >>> >>> Quoting Bjorn Andersson (2021-10-25 19:48:02) >>>> On Mon 25 Oct 15:41 PDT 2021, Stephen Boyd wrote: >>>> >>>>> >>>>> When the binding was introduced I recall we punted on the parent child >>>>> conversion stuff. One problem at a time. There's also the possibility >>>>> for a power domain to be parented by multiple power domains so >>>>> translation tables need to account for that. >>>>> >>>> >>>> But for this case - and below display case - the subdomain (the device's >>>> power-domain) is just a dumb gate. So there is no translation, the given >>>> performance_state applies to the parent. Or perhaps such implicitness >>>> will come back and bite us? >>> >>> In the gate case I don't see how the implicitness will ever be a >>> problem. >>> >>>> >>>> I don't think we allow a power-domain to be a subdomain of two >>>> power-domains - and again it's not applicable to USB or display afaict. >>> >>> Ah maybe. I always confuse power domains and genpd. >>> >>>> >>>>>> >>>>>>> Or we may need to make another part of the OPP binding to indicate the >>>>>>> relationship between the power domain and the OPP and the parent of >>>>>>> the power domain. >>>>>> >>>>>> I suspect this would be useful if a power-domain provider needs to >>>>>> translate a performance_state into a different supply-performance_state. >>>>>> Not sure if we have such case currently; these examples are all an >>>>>> adjustable power-domain with "gating" subdomains. >>>>> >>>>> Even for this case, we should be able to have the GDSC map the on state >>>>> to some performance state in the parent domain. Maybe we need to add >>>>> some code to the gdsc.c file to set a performance state on the parent >>>>> domain when it is turned on. I'm not sure where the value for that perf >>>>> state comes from. I guess we can hardcode it in the driver for now and >>>>> if it needs to be multiple values based on the clk frequency we can push >>>>> it out to an OPP table or something like that. >>>>> >>>> >>>> For the GDSC I believe we only have 1:1 mapping, so implementing >>>> set_performance_state to just pass that on to the parent might do the >>>> trick (although I haven't thought this through). >>>> >>>> Conceptually I guess this would be like calling clk_set_rate() on a >>>> clock gate, relying on it being propagated upwards. The problem here is >>>> that the performance_state is just a "random" integer without a well >>>> defined unit. >>>> >>> >>> Right. Ideally it would be in the core code somehow so that if there >>> isn't a set_performance_state function we go to the parent or some >>> special return value from the function says "call it on my parent". The >>> translation scheme could come later so we can translate the "random" >>> integer between parent-child domains. >> >> As a proof of concept it should be sufficient to just add an >> implementation of sc->pd.set_performance_state in gdsc.c. But I agree >> that it would be nice to push this into some framework code, perhaps >> made opt-in by some GENPD_FLAG_xyz. >> >>> At the end of the day the device >>> driver wants to set a frequency or runtime pm get the device and let the >>> OPP table or power domain code figure out what the level is supposed to >>> be. >>> >> >> Yes and this is already working for the non-nested case - where the >> single power-domain jumps between performance states as the opp code >> switches from one opp to another. >> >> So if we can list only the child power-domain (i.e. the GDSC) and have >> the performance_stat requests propagate up to the parent rpmhpd resource >> I think we're good. >> >> >> Let's give this a spin and confirm that this is the case... >> >>>> >>>> >>>> The one case where I believe we talked about having different mapping >>>> between the performance_state levels was in the relationship between CX >>>> and MX. But I don't think we ever did anything about that... >>> >>> Hmm alright. I think there's a constraint but otherwise nobody really >>> wants to change both at the same time. >>> >>>>> >>>>> Yes, a GDSC is really a gate on a parent power domain like CX or MMCX, >>>>> etc. Is the display subsystem an example of different clk frequencies >>>>> wanting to change the perf state of CX? If so it's a good place to work >>>>> out the translation scheme for devices that aren't listing the CX power >>>>> domain in DT. >>>> >>>> Yes, the various display components sits in MDSS_GDSC but the opp-tables >>>> needs to change the performance_state of MDSS_GDSC->parent (i.e. CX or >>>> MMCX, depending on platform). >>>> >>>> As I said, today we hack this by trusting that the base drm/msm driver >>>> will keep MDSS_GDSC on and listing MMCX (or CX) as power-domain for each >>>> of these components. >>>> >>>> >>>> So if we solve this, then that seems to directly map to the static case >>>> for USB as well. >>>> >>> >>> Got it. So in this case we could have the various display components >>> that are in the mdss gdsc domain set their frequency via OPP and then >>> have that translate to a level in CX or MMCX. How do we parent the power >>> domains outside of DT? I'm thinking that we'll need to do that if MMCX >>> is parented by CX or something like that and the drivers for those two >>> power domains are different. Is it basic string matching? >> >> In one way or another we need to invoke pm_genpd_add_subdomain() to link >> the two power-domains (actually genpds) together, like what was done in >> 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support"). >> >> In the case of MMCX and CX, my impression of the documentation is that >> they are independent - but if we need to express that CX is parent of >> MMCX, they are both provided by rpmhpd which already supports this by >> just specifying .parent on mmcx to point to cx. > > I was trying to follow the discussion, but it turned out to be a bit > complicated to catch up and answer all things. In any case, let me > just add a few overall comments, perhaps that can help to move things > forward. > > First, one domain can have two parent domains. Both from DT and from > genpd point of view, just to make this clear. > > Although, it certainly looks questionable to me, to hook up the USB > device to two separate power domains, one to control power and one to > control performance. Especially, if it's really the same piece of HW > that is managing both things. [].. > Additionally, if it's correct to model > the USB GDSC power domain as a child to the CX power domain from HW > point of view, we should likely do that.
I think this would still require a few things in genpd, since CX and USB GDSC are power domains from different providers. Perhaps a pm_genpd_add_subdomain_by_name()?
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |