Re: [PATCH v2 1/3] dt-bindings: usb: qcom,dwc3: Add multi-pd bindings for dwc3 qcom

[Rajendra Nayak <rnayak@codeaurora.org>]

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