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

Quoting Bjorn Andersson (2021-10-25 14:43:23)

quoted

On Mon 25 Oct 13:17 PDT 2021, Stephen Boyd wrote:

quoted

Quoting Bjorn Andersson (2021-10-25 12:10:35)

quoted

On Mon 25 Oct 02:07 PDT 2021, Sandeep Maheswaram wrote:

quoted

Add multi pd bindings to set performance state for cx domain
to maintain minimum corner voltage for USB clocks.

Signed-off-by: Sandeep Maheswaram <redacted>
---
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.

quoted

quoted

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.

quoted

quoted

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

quoted

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