Messages in this thread | | | Date | Mon, 31 Jan 2022 10:34:15 +0530 | Subject | Re: [PATCH v2 1/3] dt-bindings: usb: qcom,dwc3: Add multi-pd bindings for dwc3 qcom | From | Sandeep Maheswaram <> |
| |
On 1/19/2022 4:31 PM, Rajendra Nayak wrote: > > > On 1/17/2022 11:33 AM, Sandeep Maheswaram wrote: >> 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 . > > In the USB driver suspend when you check for this condition, in order > to keep the GDSC active, you would > perhaps have to drop the performance state vote and re-vote in resume. > I don;t think the genpd core can handle this in any way. > CX shutdown is not happening even after dropping the performance state in USB driver suspend.
Tried even without USB nodes in device tree cx shutdown is not happening
Adding CX as a power-domain for GCC along with below patch
https://lore.kernel.org/all/20210829154757.784699-6-dmitry.baryshkov@linaro.org/ preventing CX shutdown.
Regards
Sandeep
| |