Messages in this thread | | | Subject | Re: [PATCH v2 1/3] dt-bindings: usb: qcom,dwc3: Add multi-pd bindings for dwc3 qcom | From | Sandeep Maheswaram <> | Date | Mon, 17 Jan 2022 11:33:04 +0530 |
| |
Hi Rajendra,
On 10/28/2021 9:26 AM, Rajendra Nayak wrote: > > > 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()? > Tried with the changes provided by you where USB GDSC power domains added as a child to the CX power domain
But cx shutdown is not happening during sytem suspend as we need to keep USB GDSC active in host mode .
Regards
Sandeep
| |