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

Sandeep Maheswaram
2022-01-31T05:04:32Z

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