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

[Sandeep Maheswaram <quic_c_sanm@quicinc.com>]

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