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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 25 Oct 15:41 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-25 14:43:23)
> > On Mon 25 Oct 13:17 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-25 12:10:35)
> > > > On Mon 25 Oct 02:07 PDT 2021, Sandeep Maheswaram wrote:
> > > >
> > > > > Add multi pd bindings to set performance state for cx domain
> > > > > to maintain minimum corner voltage for USB clocks.
> > > > >
> > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> > > > > ---
> > > > > v2:
> > > > > Make cx domain mandatory.
> > > > >
> > > > >  Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > > > > index 2bdaba0..fd595a8 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > > > > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > > > > @@ -42,7 +42,13 @@ properties:
> > > > >
> > > > >    power-domains:
> > > > >      description: specifies a phandle to PM domain provider node
> > > > > -    maxItems: 1
> > > > > +    minItems: 2
> > > > > +    items:
> > > > > +      - description: cx power domain
> > > > > +      - description: USB gdsc power domain
> > > > > +
> > > > > +  required-opps:
> > > > > +    description: specifies the performance state to power domain
> > > >
> > > > I'm still worried about the fact that we can't just rely on the USB GDSC
> > > > being a subdomin of CX in order to just "turn on" CX.
> > > >
> > > > Afaict accepting this path forward means that for any device that sits
> > > > in a GDSC power domain we will have to replicate this series for the
> > > > related driver.
> > > >
> > >
> > > I suspect the problem is that it's not just "turn on" but wanting to
> > > turn it on and then set the performance state to some value based on the
> > > clk frequency.
> >
> > I don't see an opp-table involved, just the required-opps for the
> > purpose of turning CX on a little bit more. Perhaps I'm missing
> > something here though.
> 
> Indeed. There's only one clk frequency for USB so only one performance
> state/required-opps is used. In general that isn't the case and so we'll
> eventually need to map some GDSC on/off state to the clk frequency of
> whatever clk domain is associated with CX for a device.
> 

Makes sense, just because we don't use opp-tables to scale the frequency
and performance_state, the issue remains the same.

> >
> > > Maybe the simplest version of that could be supported
> > > somehow by having dev_pm_opp_set_rate() figure out that the 'level'
> > > applies to the parent power domain instead of the child one?
> >
> > Having the performance_state request cascade up through the GDSC sounds
> > like a nice solution; I've not looked at the code to see if this is
> > feasible though.
> 
> 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?

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.

> >
> > > 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.



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...

> >
> >
> > PS. I think we have the same problem in the display subsystem, the
> > sub-blocks are powered by MDSS_GDSC, which is a subdomain of MMCX. We
> > trust the parent mdss node to keep the GDSC powered and specify MMCX as
> > the power-domain for the children, so that we can affect their levels by
> > respective opp-table.
> >
> 
> 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.

Regards,
Bjorn