| Message ID | 1635152851-23660-2-git-send-email-quic_c_sanm@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | USB DWC3 QCOM Multi power domain support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Mon, 25 Oct 2021 14:37:29 +0530, 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(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml: properties:power-domains: 'oneOf' conditional failed, one must be fixed: [{'description': 'cx power domain'}, {'description': 'USB gdsc power domain'}] is too long [{'description': 'cx power domain'}, {'description': 'USB gdsc power domain'}] is too short False schema does not allow 2 1 was expected hint: "minItems" is only needed if less than the "items" list length from schema $id: http://devicetree.org/meta-schemas/items.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml: ignoring, error in schema: properties: power-domains warning: no schema found in file: ./Documentation/devicetree/bindings/usb/qcom,dwc3.yaml Documentation/devicetree/bindings/usb/qcom,dwc3.example.dt.yaml:0:0: /example-0/soc/usb@a6f8800: failed to match any schema with compatible: ['qcom,sdm845-dwc3', 'qcom,dwc3'] Documentation/devicetree/bindings/usb/qcom,dwc3.example.dt.yaml:0:0: /example-0/soc/usb@a6f8800: failed to match any schema with compatible: ['qcom,sdm845-dwc3', 'qcom,dwc3'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1545621 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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 mentioned this in v1, but I don't think we reached a conclusion. Regards, Bjorn > > clocks: > description: > -- > 2.7.4 >
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. 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? 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.
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. > 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. > 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. 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. Regards, Bjorn
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. > > > 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. > > > 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. > > > 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.
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
+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. 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. > > > 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?
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. Regards, Bjorn
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. From the performance states point of view, genpd supports propagating performance states to parent domains, via a 1:1 mapping of the performance state. Note that, we have also quite recently made genpd's ->set_performance_state() callback to be optional. A vote for a performance state will be propagated to the parent domain, even if the child domain would lack the ->set_performance_state() callback. This should be useful, where a child domain relies on its parent domain for performance state management, which seems to be the case for the USB GDSC/CX power domains, right? In regards to the parsing of the "required-opps" DT binding for a device node, I think that should work for cases like these, too. Or is there something missing around this? Kind regards Uffe
On Wed 27 Oct 07:24 PDT 2021, 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. > Thanks for jumping in Ulf. > First, one domain can have two parent domains. Both from DT and from > genpd point of view, just to make this clear. > I was under the impression that the only such configuration we supported was that we can explicitly attach and control multiple PDs from a driver. I didn't think we could say that a given genpd is a subdomain of multiple other genpds... That said, it's better if we can ignore this, as it doesn't apply to our problem at hand. > 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. > So to clarify, we have the following situation: +---------------+ | CX | | +-----------+ | | | USB_GDSC | | | | +-------+ | | | | | dwc3 | | | | | +-------+ | | | +-----------+ | +---------------+ CX can operate at different performance_states, USB_GDSC can be toggled on/off and hence dwc3 needs CX to operate at a performance_state meeting its needs. The proposed patch is to list both CX and USB_GDSC as power-domains for dwc3, in order for the required-opp in the dwc3 to affect CX. > From the performance states point of view, genpd supports propagating > performance states to parent domains, via a 1:1 mapping of the > performance state. Note that, we have also quite recently made genpd's > ->set_performance_state() callback to be optional. A vote for a > performance state will be propagated to the parent domain, even if the > child domain would lack the ->set_performance_state() callback. This > should be useful, where a child domain relies on its parent domain for > performance state management, which seems to be the case for the USB > GDSC/CX power domains, right? > I presume you're referring to the first half of _genpd_set_performance_state(). This looks to be exactly what Stephen and I discussed implementing. I had a rather messy tree when I looked at this last time, presumably missing something else to hide this propagation. For the USB_GDSC we today don't describe that as a subdomain of CX, but per your guidance and the recently introduced 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support") we should be fairly close to the solution. The one "problem" I can see is that I believe that some of the GDSCs in GCC should be subdomains of MX, so the above referenced patch would then need to be extended to allow specifying which of the multiple power-domains each GDSC should be a subdomain of - something Dmitry and I did discuss, but wasn't needed for the display GDSC. Perhaps I'm just misinformed regarding this need though. > In regards to the parsing of the "required-opps" DT binding for a > device node, I think that should work for cases like these, too. Or is > there something missing around this? > Given that Sandeep's proposed patch solves his problem without touching the framework those patches (required-opps) must already have been picked up. Regards, Bjorn
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()?
On Wed, 27 Oct 2021 at 17:09, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Wed 27 Oct 07:24 PDT 2021, 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. > > > > Thanks for jumping in Ulf. > > > First, one domain can have two parent domains. Both from DT and from > > genpd point of view, just to make this clear. > > > > I was under the impression that the only such configuration we supported > was that we can explicitly attach and control multiple PDs from a > driver. I didn't think we could say that a given genpd is a subdomain of > multiple other genpds... > > That said, it's better if we can ignore this, as it doesn't apply to our > problem at hand. > > > 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. > > > > So to clarify, we have the following situation: > > +---------------+ > | CX | > | +-----------+ | > | | USB_GDSC | | > | | +-------+ | | > | | | dwc3 | | | > | | +-------+ | | > | +-----------+ | > +---------------+ > > CX can operate at different performance_states, USB_GDSC can be toggled > on/off and hence dwc3 needs CX to operate at a performance_state meeting > its needs. > > The proposed patch is to list both CX and USB_GDSC as power-domains for > dwc3, in order for the required-opp in the dwc3 to affect CX. Okay. Then I need to point out that this looks wrong to me. We should be able to support the needs for dwc3, by letting CX to become the parent domain for USB_GDSC. If there is something missing from the genpd point of view, for example, let's fix that! > > > From the performance states point of view, genpd supports propagating > > performance states to parent domains, via a 1:1 mapping of the > > performance state. Note that, we have also quite recently made genpd's > > ->set_performance_state() callback to be optional. A vote for a > > performance state will be propagated to the parent domain, even if the > > child domain would lack the ->set_performance_state() callback. This > > should be useful, where a child domain relies on its parent domain for > > performance state management, which seems to be the case for the USB > > GDSC/CX power domains, right? > > > > I presume you're referring to the first half of > _genpd_set_performance_state(). This looks to be exactly what Stephen > and I discussed implementing. Yes. > > I had a rather messy tree when I looked at this last time, presumably > missing something else to hide this propagation. > > > For the USB_GDSC we today don't describe that as a subdomain of CX, but > per your guidance and the recently introduced 3652265514f5 ("clk: qcom: > gdsc: enable optional power domain support") we should be fairly close > to the solution. Great! > > > The one "problem" I can see is that I believe that some of the GDSCs in > GCC should be subdomains of MX, so the above referenced patch would then > need to be extended to allow specifying which of the multiple > power-domains each GDSC should be a subdomain of - something Dmitry and > I did discuss, but wasn't needed for the display GDSC. > Perhaps I'm just misinformed regarding this need though. I didn't quite follow all of this. But, perhaps using "#power-domain-cells = <2>" for the power-domain provider can help to specify this for the consumer/child-domain? > > > In regards to the parsing of the "required-opps" DT binding for a > > device node, I think that should work for cases like these, too. Or is > > there something missing around this? > > > > Given that Sandeep's proposed patch solves his problem without touching > the framework those patches (required-opps) must already have been > picked up. Right! Kind regards Uffe
[...] > >>> 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()? > I think of_genpd_add_subdomain() should help to address this. No? Kind regards Uffe
On 10/28/2021 4:05 PM, Ulf Hansson wrote: > [...] > >>>>> 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()? >> > > I think of_genpd_add_subdomain() should help to address this. No? We only describe the provider nodes in DT and not the individual power domains. For instance GCC is the power domain provider which is in DT, and USB GDSC is one of the many power domains it supports, similarly RPMHPD is the provider node in DT and CX is one of the many power domains it supports. So we would need some non-DT way of hooking up power domains from two different providers as parent/child.
On Thu 28 Oct 03:31 PDT 2021, Ulf Hansson wrote: > On Wed, 27 Oct 2021 at 17:09, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Wed 27 Oct 07:24 PDT 2021, 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. > > > > > > > Thanks for jumping in Ulf. > > > > > First, one domain can have two parent domains. Both from DT and from > > > genpd point of view, just to make this clear. > > > > > > > I was under the impression that the only such configuration we supported > > was that we can explicitly attach and control multiple PDs from a > > driver. I didn't think we could say that a given genpd is a subdomain of > > multiple other genpds... > > > > That said, it's better if we can ignore this, as it doesn't apply to our > > problem at hand. > > > > > 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. > > > > > > > So to clarify, we have the following situation: > > > > +---------------+ > > | CX | > > | +-----------+ | > > | | USB_GDSC | | > > | | +-------+ | | > > | | | dwc3 | | | > > | | +-------+ | | > > | +-----------+ | > > +---------------+ > > > > CX can operate at different performance_states, USB_GDSC can be toggled > > on/off and hence dwc3 needs CX to operate at a performance_state meeting > > its needs. > > > > The proposed patch is to list both CX and USB_GDSC as power-domains for > > dwc3, in order for the required-opp in the dwc3 to affect CX. > > Okay. Then I need to point out that this looks wrong to me. > Thank you :) > We should be able to support the needs for dwc3, by letting CX to > become the parent domain for USB_GDSC. > > If there is something missing from the genpd point of view, for > example, let's fix that! > > > > > > From the performance states point of view, genpd supports propagating > > > performance states to parent domains, via a 1:1 mapping of the > > > performance state. Note that, we have also quite recently made genpd's > > > ->set_performance_state() callback to be optional. A vote for a > > > performance state will be propagated to the parent domain, even if the > > > child domain would lack the ->set_performance_state() callback. This > > > should be useful, where a child domain relies on its parent domain for > > > performance state management, which seems to be the case for the USB > > > GDSC/CX power domains, right? > > > > > > > I presume you're referring to the first half of > > _genpd_set_performance_state(). This looks to be exactly what Stephen > > and I discussed implementing. > > Yes. > > > > > I had a rather messy tree when I looked at this last time, presumably > > missing something else to hide this propagation. > > > > > > For the USB_GDSC we today don't describe that as a subdomain of CX, but > > per your guidance and the recently introduced 3652265514f5 ("clk: qcom: > > gdsc: enable optional power domain support") we should be fairly close > > to the solution. > > Great! > > > > > > > The one "problem" I can see is that I believe that some of the GDSCs in > > GCC should be subdomains of MX, so the above referenced patch would then > > need to be extended to allow specifying which of the multiple > > power-domains each GDSC should be a subdomain of - something Dmitry and > > I did discuss, but wasn't needed for the display GDSC. > > Perhaps I'm just misinformed regarding this need though. > > I didn't quite follow all of this. > > But, perhaps using "#power-domain-cells = <2>" for the power-domain > provider can help to specify this for the consumer/child-domain? > I believe the genpd controller sits in CX, but among the domains registered by the driver we find some that are subdomains of CX and some that are subdomains of MX. In the case Dmitry previously implemented, for the display, we have the display domain controller sitting in MMCX and it exposes a set of domains all being subdomains of MMCX - so 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support") simply adds all the domains as subdomain of the controllers domain. Here we would need to specify both power-domains and then make sure for each registered domain that it is added as subdomain of the right one. Hence the quotation marks around "problem", I don't see that we have a problem, but it's something lacking in the current implementation. Regards, Bjorn > > > > > In regards to the parsing of the "required-opps" DT binding for a > > > device node, I think that should work for cases like these, too. Or is > > > there something missing around this? > > > > > > > Given that Sandeep's proposed patch solves his problem without touching > > the framework those patches (required-opps) must already have been > > picked up. > > Right! > > Kind regards > Uffe
On Thu 28 Oct 03:46 PDT 2021, Rajendra Nayak wrote: > > On 10/28/2021 4:05 PM, Ulf Hansson wrote: > > [...] > > > > > > > > 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()? > > > > > > > I think of_genpd_add_subdomain() should help to address this. No? > > We only describe the provider nodes in DT and not the individual power domains. > For instance GCC is the power domain provider which is in DT, and USB GDSC is one > of the many power domains it supports, similarly RPMHPD is the provider node in > DT and CX is one of the many power domains it supports. > So we would need some non-DT way of hooking up power domains from two different > providers as parent/child. > See 266e5cf39a0f ("arm64: dts: qcom: sm8250: remove mmcx regulator") and 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support") MMCX is declared as power-domain for the dispcc (which is correct in itself) and the gdsc code will register GDSCs as subdomains of the same power-domain. To ensure this code path is invoked the clock driver itself needed this 6158b94ec807 ("clk: qcom: dispcc-sm8250: use runtime PM for the clock controller") So at least in theory, considering only USB the minimum would be to pm_runtime_enable() gcc-7280 and add power-domains = <CX> to the gcc node. The "problem" I described would be if there are GDSCs that are subdomains of MX - which I've seen hinted in some documentation. If so we should to specify both CX and MX as power-domains for &gcc and the gdsc implementation needs to be extended to allow us to select between the two. For this I believe a combination of genpd_dev_pm_attach_by_name() and of_genpd_add_subdomain() would do the trick. That is, if there actually are GDSCs exposed by gcc that are not subdomains of CX - otherwise none of this is needed. Regards, Bjorn
Quoting Bjorn Andersson (2021-10-28 09:53:14) > On Thu 28 Oct 03:46 PDT 2021, Rajendra Nayak wrote: > > > > > On 10/28/2021 4:05 PM, Ulf Hansson wrote: > > > [...] > > > > > > > > > > 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()? > > > > > > > > > > I think of_genpd_add_subdomain() should help to address this. No? > > > > We only describe the provider nodes in DT and not the individual power domains. > > For instance GCC is the power domain provider which is in DT, and USB GDSC is one > > of the many power domains it supports, similarly RPMHPD is the provider node in > > DT and CX is one of the many power domains it supports. > > So we would need some non-DT way of hooking up power domains from two different > > providers as parent/child. > > > > See 266e5cf39a0f ("arm64: dts: qcom: sm8250: remove mmcx regulator") and > 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support") > > MMCX is declared as power-domain for the dispcc (which is correct > in itself) and the gdsc code will register GDSCs as subdomains of > the same power-domain. > > > To ensure this code path is invoked the clock driver itself needed this > 6158b94ec807 ("clk: qcom: dispcc-sm8250: use runtime PM for the clock > controller") > > So at least in theory, considering only USB the minimum would be to > pm_runtime_enable() gcc-7280 and add power-domains = <CX> to the gcc > node. I'm wary of runtime PM enabling the main clock controller. Does it work? I can understand that we need to get the CX power domain pointer into the gdsc code somehow, and thus setting the power domain to CX in DT is a way to do that. Why do we need to runtime pm enable the clk controller though? Just to make genpd_dev_pm_attach_by_name() hand us a genpd? I see in commit 3652265514f5 that we also use it to have gdsc_enable() enable the parent domain by using runtime PM to get the clk controller and enable the parent domain. That is convoluted. I'd prefer if we could list that the parent domain is in the registering device's power-domain index number, ala clk_parent_data, so that we don't have to make the power domain provider into a consumer itself. This would clean up the gdsc code so that it doesn't have to go from the provider's genpd enable through the provider device to the parent power domain enable. Obviously it works but it's hard to follow. > > > The "problem" I described would be if there are GDSCs that are > subdomains of MX - which I've seen hinted in some documentation. If so > we should to specify both CX and MX as power-domains for &gcc and the > gdsc implementation needs to be extended to allow us to select between > the two. > > For this I believe a combination of genpd_dev_pm_attach_by_name() and > of_genpd_add_subdomain() would do the trick. > > That is, if there actually are GDSCs exposed by gcc that are not > subdomains of CX - otherwise none of this is needed. > Rajendra can correct me, but I believe every device that has a GDSC gating power to it is actually inside CX and MX. The CX is for digital logic (think registers and interrupts, fetching data from the bus, etc.) and the MX is for memories (think flops that retain state of registers and internal state of the device). In more modern SoCs they've split multimedia (MMCX) and GPU (gpu_gx) out of CX and supply it with a different voltage supply and pin on the SoC package. Historically, MX voltage has been maintained by the power manager, RPM or RPMh, so that when CX is changed, MX >= CX constraints are maintained. I think that also changed over time though and MX had to be controlled in addition to CX on some firmwares. I recall we had some constraint code that bumped up MX whenever CX got higher than it. Having to control both led to more round trip time when changing clk rates though so it got combined on the backend so that only one message had to be sent to the RPM. We probably ought to list both CX and MX as power-domains on the clk nodes that provide GDSCs to match the hardware. Then we need to know which power domain each GDSC wants to set a minimum level on. That would be the most correct way to do it.
On Thu 28 Oct 13:04 PDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-10-28 09:53:14) > > On Thu 28 Oct 03:46 PDT 2021, Rajendra Nayak wrote: > > > > > > > > On 10/28/2021 4:05 PM, Ulf Hansson wrote: > > > > [...] > > > > > > > > > > > > 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()? > > > > > > > > > > > > > I think of_genpd_add_subdomain() should help to address this. No? > > > > > > We only describe the provider nodes in DT and not the individual power domains. > > > For instance GCC is the power domain provider which is in DT, and USB GDSC is one > > > of the many power domains it supports, similarly RPMHPD is the provider node in > > > DT and CX is one of the many power domains it supports. > > > So we would need some non-DT way of hooking up power domains from two different > > > providers as parent/child. > > > > > > > See 266e5cf39a0f ("arm64: dts: qcom: sm8250: remove mmcx regulator") and > > 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support") > > > > MMCX is declared as power-domain for the dispcc (which is correct > > in itself) and the gdsc code will register GDSCs as subdomains of > > the same power-domain. > > > > > > To ensure this code path is invoked the clock driver itself needed this > > 6158b94ec807 ("clk: qcom: dispcc-sm8250: use runtime PM for the clock > > controller") > > > > So at least in theory, considering only USB the minimum would be to > > pm_runtime_enable() gcc-7280 and add power-domains = <CX> to the gcc > > node. > > I'm wary of runtime PM enabling the main clock controller. I thought of that as well after sending my last reply. Seems like a good idea if we can avoid it. > Does it work? > Had to test it, but specifying power-domain = <CX> in &gcc and adding the required-opp to the usb node causes CX to enter the given performance_state. So it boots just fine and I didn't pm_runtime_enable() gcc, so gdsc_pm_runtime_get() will just return. That said, I don't grasp all the details happening under the hood, so I might be missing some details that will bite us when it comes to suspend or power collapse? > I can understand that we need to get the CX power domain pointer into > the gdsc code somehow, and thus setting the power domain to CX in DT is > a way to do that. Why do we need to runtime pm enable the clk controller > though? In the case of dispcc we need it because accessing the clock registers without it crashes the board. > Just to make genpd_dev_pm_attach_by_name() hand us a genpd? dispcc has a single power-domain so ((struct device*)dispcc)->pm_domain will automatically be filled out. > I > see in commit 3652265514f5 that we also use it to have gdsc_enable() > enable the parent domain by using runtime PM to get the clk controller > and enable the parent domain. That is convoluted. > The purpose of the gdsc_pm_runtime_get() call from gdsc_enable() is to ensure that the clock controller is powered up, so that we can access the registers - just as you do in clk_pm_runtime_get() So regardless of us changing how the subdomains are setup I think this part should stay, to work for the clock controllers that need to ensure their registers are accessible. > I'd prefer if we could list that the parent domain is in the registering > device's power-domain index number, ala clk_parent_data, so that we > don't have to make the power domain provider into a consumer itself. > This would clean up the gdsc code so that it doesn't have to go from the > provider's genpd enable through the provider device to the parent power > domain enable. Obviously it works but it's hard to follow. > Giving it another look I think the current implementation in gdsc.c will enable the parent power-domain twice; once in the core because of the dependency from the subdomain and one by the device itself. That said, I do find this technically correct for the dispcc case - MDSS_GDSC has a vote and dispcc has another vote. I don't have any objections to replacing the current pd_to_genpd(dev->pm_domain) in gdsc's call to pm_genpd_add_subdomain() with something carrying information from the clock driver indicating which of the multiple power domains the gdsc should be parented by. > > > > > > The "problem" I described would be if there are GDSCs that are > > subdomains of MX - which I've seen hinted in some documentation. If so > > we should to specify both CX and MX as power-domains for &gcc and the > > gdsc implementation needs to be extended to allow us to select between > > the two. > > > > For this I believe a combination of genpd_dev_pm_attach_by_name() and > > of_genpd_add_subdomain() would do the trick. > > > > That is, if there actually are GDSCs exposed by gcc that are not > > subdomains of CX - otherwise none of this is needed. > > > > Rajendra can correct me, but I believe every device that has a GDSC > gating power to it is actually inside CX and MX. The CX is for digital > logic (think registers and interrupts, fetching data from the bus, etc.) > and the MX is for memories (think flops that retain state of registers > and internal state of the device). In more modern SoCs they've split > multimedia (MMCX) and GPU (gpu_gx) out of CX and supply it with a > different voltage supply and pin on the SoC package. Historically, MX > voltage has been maintained by the power manager, RPM or RPMh, so that > when CX is changed, MX >= CX constraints are maintained. I think that > also changed over time though and MX had to be controlled in addition to > CX on some firmwares. I recall we had some constraint code that bumped > up MX whenever CX got higher than it. Having to control both led to more > round trip time when changing clk rates though so it got combined on the > backend so that only one message had to be sent to the RPM. > That would explain the current hack(?) in rpmhpd.c which states that mx is the parent of cx - which I presume is there to say that "mx needs to be on when cx is on". As a side effect of all this magic though this means that my vote from usb is actually applied to MX as well (as would it in Sandeep's original proposal)... > We probably ought to list both CX and MX as power-domains on the clk > nodes that provide GDSCs to match the hardware. Then we need to know > which power domain each GDSC wants to set a minimum level on. That would > be the most correct way to do it. As we describe multiple power-domains we won't get dev->pm_domain filled out for us, so we need to genpd_dev_pm_attach_by_{id,name}() to get the struct devices for each power-domain and then use those when setting up the subdomain for each gdsc. But per the current implementation (with the CX votes trickling down to MX as well) we should be able to just grab genpd_dev_pm_attach_by_id(0) in gdsc_register() and use that if dev->pm_domain isn't set - at least until we explicitly need to vote for MX only... Let's see what Rajendra says about the requirements here. Regards, Bjorn
On 10/29/2021 5:51 AM, Bjorn Andersson wrote: [].. >>> See 266e5cf39a0f ("arm64: dts: qcom: sm8250: remove mmcx regulator") and >>> 3652265514f5 ("clk: qcom: gdsc: enable optional power domain support") >>> >>> MMCX is declared as power-domain for the dispcc (which is correct >>> in itself) and the gdsc code will register GDSCs as subdomains of >>> the same power-domain. >>> >>> >>> To ensure this code path is invoked the clock driver itself needed this >>> 6158b94ec807 ("clk: qcom: dispcc-sm8250: use runtime PM for the clock >>> controller") >>> >>> So at least in theory, considering only USB the minimum would be to >>> pm_runtime_enable() gcc-7280 and add power-domains = <CX> to the gcc >>> node. >> >> I'm wary of runtime PM enabling the main clock controller. > > I thought of that as well after sending my last reply. Seems like a good > idea if we can avoid it. > >> Does it work? >> > > Had to test it, but specifying power-domain = <CX> in &gcc and adding > the required-opp to the usb node causes CX to enter the given > performance_state. > > So it boots just fine and I didn't pm_runtime_enable() gcc, so > gdsc_pm_runtime_get() will just return. > > That said, I don't grasp all the details happening under the hood, so I > might be missing some details that will bite us when it comes to suspend > or power collapse? > >> I can understand that we need to get the CX power domain pointer into >> the gdsc code somehow, and thus setting the power domain to CX in DT is >> a way to do that. Why do we need to runtime pm enable the clk controller >> though? > > In the case of dispcc we need it because accessing the clock registers > without it crashes the board. > >> Just to make genpd_dev_pm_attach_by_name() hand us a genpd? > > dispcc has a single power-domain so ((struct device*)dispcc)->pm_domain > will automatically be filled out. > >> I >> see in commit 3652265514f5 that we also use it to have gdsc_enable() >> enable the parent domain by using runtime PM to get the clk controller >> and enable the parent domain. That is convoluted. >> > > The purpose of the gdsc_pm_runtime_get() call from gdsc_enable() is to > ensure that the clock controller is powered up, so that we can access > the registers - just as you do in clk_pm_runtime_get() > > So regardless of us changing how the subdomains are setup I think this > part should stay, to work for the clock controllers that need to ensure > their registers are accessible. > >> I'd prefer if we could list that the parent domain is in the registering >> device's power-domain index number, ala clk_parent_data, so that we >> don't have to make the power domain provider into a consumer itself. >> This would clean up the gdsc code so that it doesn't have to go from the >> provider's genpd enable through the provider device to the parent power >> domain enable. Obviously it works but it's hard to follow. >> > > Giving it another look I think the current implementation in gdsc.c will > enable the parent power-domain twice; once in the core because of the > dependency from the subdomain and one by the device itself. > > That said, I do find this technically correct for the dispcc case - > MDSS_GDSC has a vote and dispcc has another vote. > > > I don't have any objections to replacing the current > pd_to_genpd(dev->pm_domain) in gdsc's call to pm_genpd_add_subdomain() > with something carrying information from the clock driver indicating > which of the multiple power domains the gdsc should be parented by. > >>> >>> >>> The "problem" I described would be if there are GDSCs that are >>> subdomains of MX - which I've seen hinted in some documentation. If so >>> we should to specify both CX and MX as power-domains for &gcc and the >>> gdsc implementation needs to be extended to allow us to select between >>> the two. >>> >>> For this I believe a combination of genpd_dev_pm_attach_by_name() and >>> of_genpd_add_subdomain() would do the trick. >>> >>> That is, if there actually are GDSCs exposed by gcc that are not >>> subdomains of CX - otherwise none of this is needed. >>> >> >> Rajendra can correct me, but I believe every device that has a GDSC >> gating power to it is actually inside CX and MX. The CX is for digital >> logic (think registers and interrupts, fetching data from the bus, etc.) >> and the MX is for memories (think flops that retain state of registers Thats true in general but for devices specifically in GCC I doubt anything is part of MX. For the Multimedia ones, yes there is a CX/MX split. >> and internal state of the device). In more modern SoCs they've split >> multimedia (MMCX) and GPU (gpu_gx) out of CX and supply it with a >> different voltage supply and pin on the SoC package. Historically, MX >> voltage has been maintained by the power manager, RPM or RPMh, so that >> when CX is changed, MX >= CX constraints are maintained. I think that >> also changed over time though and MX had to be controlled in addition to >> CX on some firmwares. I recall we had some constraint code that bumped >> up MX whenever CX got higher than it. Having to control both led to more >> round trip time when changing clk rates though so it got combined on the >> backend so that only one message had to be sent to the RPM. >> > > That would explain the current hack(?) in rpmhpd.c which states that mx > is the parent of cx - which I presume is there to say that "mx needs to > be on when cx is on". I always thought we did this only on sdm845, but now that I looked at the code again, I realized we ended up reusing those struct definitions and now that applies to all supported SoCs :/ I will double check but FWIK it should be needed only on sdm845, I will send in a patch to fix that once I get that confirmed. That said the constraints of MX >= CX exist on all SoCs but like Stephen mentioned its expected to be taken care of by RPMh, on sdm845 though for whatever reason that got pushed onto the clients. > As a side effect of all this magic though this means that my vote from > usb is actually applied to MX as well (as would it in Sandeep's original > proposal)... right, that should not be needed from clients, RPMh should be able to handle that internally >> We probably ought to list both CX and MX as power-domains on the clk >> nodes that provide GDSCs to match the hardware. Then we need to know >> which power domain each GDSC wants to set a minimum level on. That would >> be the most correct way to do it. > > As we describe multiple power-domains we won't get dev->pm_domain filled > out for us, so we need to genpd_dev_pm_attach_by_{id,name}() to get the > struct devices for each power-domain and then use those when setting up > the subdomain for each gdsc. > > But per the current implementation (with the CX votes trickling down to > MX as well) we should be able to just grab genpd_dev_pm_attach_by_id(0) > in gdsc_register() and use that if dev->pm_domain isn't set - at least > until we explicitly need to vote for MX only... Atleast for GCC I think it makes sense to have only CX (since nothing should be powered by MX), for some of the other MM controllers if it makes sense we could perhaps look at having both, again in case we really need to explicitly vote on MX, which I haven't seen even in downstream code (barring the sdm845 hack)
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
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. > > Regards > > Sandeep > > >
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
On 1/31/2022 10:34 AM, Sandeep Maheswaram wrote: > > 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. Thats perhaps because you leave the gdsc enabled, which then means that GCC will not drop the CX vote to RPMh > Tried even without USB nodes in device tree cx shutdown is not happening Perhaps some other gdsc is left enabled too other than usb? > 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. Dmitry/Bjorn, can you confirm this is by design that if a gdsc is left enabled the CX vote would not be dropped? What this means is that in usecases where usb leaves its gdsc enabled in system suspend (to make sure its possible for it to wakeup the system), we would be burning a lot more power.
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 clocks: description:
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(-)