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

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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.

2021-10-25 21:42:52

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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
>

2021-10-26 01:56:34

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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

2021-10-26 02:09:37

[permalink] [raw]

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

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 <[email protected]>
> > > > ---
> > > > 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.

2021-10-26 08:44:34

[permalink] [raw]

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

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 <[email protected]>
> > > > > ---
> > > > > 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

2021-10-27 13:55:05

[permalink] [raw]

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

+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?

2021-10-27 17:30:52

[permalink] [raw]

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

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

2021-10-27 21:27:28

by Ulf Hansson

[permalink] [raw]

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

On Wed, 27 Oct 2021 at 06:55, Bjorn Andersson
<[email protected]> 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

2021-10-27 21:28:50

[permalink] [raw]

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

On Wed 27 Oct 07:24 PDT 2021, Ulf Hansson wrote:

> On Wed, 27 Oct 2021 at 06:55, Bjorn Andersson
> <[email protected]> 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

2021-10-28 03:58:40

[permalink] [raw]

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

On 10/27/2021 7:54 PM, Ulf Hansson wrote:
> On Wed, 27 Oct 2021 at 06:55, Bjorn Andersson
> <[email protected]> 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()?

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

2021-10-28 10:34:59

by Ulf Hansson

[permalink] [raw]

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

On Wed, 27 Oct 2021 at 17:09, Bjorn Andersson
<[email protected]> wrote:
>
> On Wed 27 Oct 07:24 PDT 2021, Ulf Hansson wrote:
>
> > On Wed, 27 Oct 2021 at 06:55, Bjorn Andersson
> > <[email protected]> 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

2021-10-28 10:37:05

by Ulf Hansson

[permalink] [raw]

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

[...]

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

2021-10-28 10:48:08

[permalink] [raw]

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

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.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

2021-10-28 16:42:34

[permalink] [raw]

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

On Thu 28 Oct 03:31 PDT 2021, Ulf Hansson wrote:

> On Wed, 27 Oct 2021 at 17:09, Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Wed 27 Oct 07:24 PDT 2021, Ulf Hansson wrote:
> >
> > > On Wed, 27 Oct 2021 at 06:55, Bjorn Andersson
> > > <[email protected]> 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

2021-10-28 16:53:14

[permalink] [raw]

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

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

2021-10-28 20:05:44

[permalink] [raw]

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

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.

2021-10-29 00:21:08

[permalink] [raw]

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

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

2021-10-29 09:51:59

[permalink] [raw]

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

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)

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

2022-01-17 14:15:03

by Sandeep Maheswaram

[permalink] [raw]

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

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
>> <[email protected]> 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

2022-01-21 19:11:41

[permalink] [raw]

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

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
>>> <[email protected]> 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
>
>
>

2022-02-01 15:28:06

by Sandeep Maheswaram

[permalink] [raw]

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

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
>>>> <[email protected]> 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/[email protected]/
preventing CX shutdown.

Regards

Sandeep

2022-02-04 11:55:55