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@xxxxxxxxxxx> > > > > > --- > > > > > 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
