diff mbox series

arm64: dts: qcom: sc8280xp: Flush RSC sleep & wake votes

Message ID 20230512150425.3171122-1-quic_bjorande@quicinc.com
State New
Headers
Series arm64: dts: qcom: sc8280xp: Flush RSC sleep & wake votes |

Commit Message

Bjorn Andersson May 12, 2023, 3:04 p.m. UTC 
  The rpmh driver will cache sleep and wake votes until the cluster
power-domain is about to enter idle, to avoid unnecessary writes. So
associate the apps_rsc with the cluster pd, so that it can be notified
about this event.

Without this, only AMC votes are being commited.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
 1 file changed, 1 insertion(+)

Comments

Konrad Dybcio May 13, 2023, 9:09 a.m. UTC | #1 
On 12.05.2023 17:04, Bjorn Andersson wrote:
> The rpmh driver will cache sleep and wake votes until the cluster
> power-domain is about to enter idle, to avoid unnecessary writes. So
> associate the apps_rsc with the cluster pd, so that it can be notified
> about this event.
> 
> Without this, only AMC votes are being commited.
Ouch.

Should we make this required: in bindings and add it to all
platforms?

> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")

Konrad
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 8fa9fbfe5d00..5c68f2182c2f 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3982,6 +3982,7 @@ apps_rsc: rsc@18200000 {
>  			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
>  					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
>  			label = "apps_rsc";
> +			power-domains = <&CLUSTER_PD>;
>  
>  			apps_bcm_voter: bcm-voter {
>  				compatible = "qcom,bcm-voter";
Bjorn Andersson May 15, 2023, 2:38 a.m. UTC | #2 
On Sat, May 13, 2023 at 11:09:07AM +0200, Konrad Dybcio wrote:
> 
> 
> On 12.05.2023 17:04, Bjorn Andersson wrote:
> > The rpmh driver will cache sleep and wake votes until the cluster
> > power-domain is about to enter idle, to avoid unnecessary writes. So
> > associate the apps_rsc with the cluster pd, so that it can be notified
> > about this event.
> > 
> > Without this, only AMC votes are being commited.
> Ouch.
> 
> Should we make this required: in bindings and add it to all
> platforms?
> 

I though this was an optimization and in the absence of this callback
the driver would just write out wake and sleep sets as well. But per the
current implementation (and perhaps some underlying cause?) it is indeed
required, if you care about power consumption.

> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> 

The Fixes sounds reasonable.

Thanks,
Bjorn

> Konrad
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index 8fa9fbfe5d00..5c68f2182c2f 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -3982,6 +3982,7 @@ apps_rsc: rsc@18200000 {
> >  			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
> >  					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
> >  			label = "apps_rsc";
> > +			power-domains = <&CLUSTER_PD>;
> >  
> >  			apps_bcm_voter: bcm-voter {
> >  				compatible = "qcom,bcm-voter";
Bjorn Andersson May 15, 2023, 3:33 a.m. UTC | #3 
On Fri, 12 May 2023 08:04:25 -0700, Bjorn Andersson wrote:
> The rpmh driver will cache sleep and wake votes until the cluster
> power-domain is about to enter idle, to avoid unnecessary writes. So
> associate the apps_rsc with the cluster pd, so that it can be notified
> about this event.
> 
> Without this, only AMC votes are being commited.
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: qcom: sc8280xp: Flush RSC sleep & wake votes
      commit: ce7c014937c442be677963848c7db62eccd94eac

Best regards,
Manivannan Sadhasivam May 15, 2023, 4:36 a.m. UTC | #4 
On Fri, May 12, 2023 at 08:04:25AM -0700, Bjorn Andersson wrote:
> The rpmh driver will cache sleep and wake votes until the cluster
> power-domain is about to enter idle, to avoid unnecessary writes. So
> associate the apps_rsc with the cluster pd, so that it can be notified
> about this event.
> 
> Without this, only AMC votes are being commited.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

It still didn't fix the BCM vote issue I'm seeing on the CRD but that might be
due to RPMh resources not entering sleep mode to apply the sleep vote flushed
with the help of this patch.

Acked-by: Manivannan Sadhasivam <mani@kernel.org>

- Mani

> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 8fa9fbfe5d00..5c68f2182c2f 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3982,6 +3982,7 @@ apps_rsc: rsc@18200000 {
>  			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
>  					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
>  			label = "apps_rsc";
> +			power-domains = <&CLUSTER_PD>;
>  
>  			apps_bcm_voter: bcm-voter {
>  				compatible = "qcom,bcm-voter";
> -- 
> 2.25.1
>
Konrad Dybcio May 15, 2023, 9:34 a.m. UTC | #5 
On 15.05.2023 04:38, Bjorn Andersson wrote:
> On Sat, May 13, 2023 at 11:09:07AM +0200, Konrad Dybcio wrote:
>>
>>
>> On 12.05.2023 17:04, Bjorn Andersson wrote:
>>> The rpmh driver will cache sleep and wake votes until the cluster
>>> power-domain is about to enter idle, to avoid unnecessary writes. So
>>> associate the apps_rsc with the cluster pd, so that it can be notified
>>> about this event.
>>>
>>> Without this, only AMC votes are being commited.
>> Ouch.
>>
>> Should we make this required: in bindings and add it to all
>> platforms?
>>
> 
> I though this was an optimization and in the absence of this callback
> the driver would just write out wake and sleep sets as well. But per the
> current implementation (and perhaps some underlying cause?) it is indeed
> required, if you care about power consumption.
Hm.. since it's not strictly required for operation, would something
like this be fitting?:

oneOf:
  - required:
      [...]
      - power-domains

  - required:
      [...]
    deprecated: true

(if it even works this way)

Konrad
> 
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
>>
> 
> The Fixes sounds reasonable.
> 
> Thanks,
> Bjorn
> 
>> Konrad
>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> index 8fa9fbfe5d00..5c68f2182c2f 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> @@ -3982,6 +3982,7 @@ apps_rsc: rsc@18200000 {
>>>  			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
>>>  					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
>>>  			label = "apps_rsc";
>>> +			power-domains = <&CLUSTER_PD>;
>>>  
>>>  			apps_bcm_voter: bcm-voter {
>>>  				compatible = "qcom,bcm-voter";
Johan Hovold May 15, 2023, 11:52 a.m. UTC | #6 
On Fri, May 12, 2023 at 08:04:25AM -0700, Bjorn Andersson wrote:
> The rpmh driver will cache sleep and wake votes until the cluster
> power-domain is about to enter idle, to avoid unnecessary writes. So
> associate the apps_rsc with the cluster pd, so that it can be notified
> about this event.
> 
> Without this, only AMC votes are being commited.

I'm sure I'm missing some details here, but from reading (and tracing)
the code it looks like the cached votes are still being flushed albeit
via rpmh_rsc_cpu_pm_callback() as the individual CPUs go down.

There wasn't really any obvious motivation in the series adding support
for the cluster domain as to why the new mechanism is preferred (e.g. if
this is an actual fix or just an optimisation done to align with
downstream):

	https://lore.kernel.org/all/20221018152837.619426-1-ulf.hansson@linaro.org/

Either way, this appears to work as intended even if it does not in
itself have any significant effect on the X13s power consumption (in
idle or suspend).

> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan
Johan Hovold May 15, 2023, 11:54 a.m. UTC | #7 
On Sun, May 14, 2023 at 07:38:28PM -0700, Bjorn Andersson wrote:
> On Sat, May 13, 2023 at 11:09:07AM +0200, Konrad Dybcio wrote:
> > 
> > 
> > On 12.05.2023 17:04, Bjorn Andersson wrote:
> > > The rpmh driver will cache sleep and wake votes until the cluster
> > > power-domain is about to enter idle, to avoid unnecessary writes. So
> > > associate the apps_rsc with the cluster pd, so that it can be notified
> > > about this event.
> > > 
> > > Without this, only AMC votes are being commited.
> > Ouch.
> > 
> > Should we make this required: in bindings and add it to all
> > platforms?
> > 
> 
> I though this was an optimization and in the absence of this callback
> the driver would just write out wake and sleep sets as well. But per the
> current implementation (and perhaps some underlying cause?) it is indeed
> required, if you care about power consumption.
> 
> > > 
> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > ---
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > 
> 
> The Fixes sounds reasonable.

Note that support for the cluster domain was not added until 6.2 and
commit 25092e6100ac ("soc: qcom: rpmh-rsc: Attach RSC to cluster PM
domain") while support for sc8280xp went into 5.19 IIRC.

Johan
Bjorn Andersson May 16, 2023, 9:01 p.m. UTC | #8 
On Mon, May 15, 2023 at 11:34:45AM +0200, Konrad Dybcio wrote:
> 
> 
> On 15.05.2023 04:38, Bjorn Andersson wrote:
> > On Sat, May 13, 2023 at 11:09:07AM +0200, Konrad Dybcio wrote:
> >>
> >>
> >> On 12.05.2023 17:04, Bjorn Andersson wrote:
> >>> The rpmh driver will cache sleep and wake votes until the cluster
> >>> power-domain is about to enter idle, to avoid unnecessary writes. So
> >>> associate the apps_rsc with the cluster pd, so that it can be notified
> >>> about this event.
> >>>
> >>> Without this, only AMC votes are being commited.
> >> Ouch.
> >>
> >> Should we make this required: in bindings and add it to all
> >> platforms?
> >>
> > 
> > I though this was an optimization and in the absence of this callback
> > the driver would just write out wake and sleep sets as well. But per the
> > current implementation (and perhaps some underlying cause?) it is indeed
> > required, if you care about power consumption.
> Hm.. since it's not strictly required for operation, would something
> like this be fitting?:
> 

I don't think it's required for operation, but the current
implementation does require it.

So I think we should either require it in the binding to mimic the
implementation, or the implementation should handle either case (only
with a performance impact)

> oneOf:
>   - required:
>       [...]
>       - power-domains
> 
>   - required:
>       [...]
>     deprecated: true
> 
> (if it even works this way)

I don't think it's worth supporting the combinations.

Regards,
Bjorn

> 
> Konrad
> > 
> >>>
> >>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> >>> ---
> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> >>
> > 
> > The Fixes sounds reasonable.
> > 
> > Thanks,
> > Bjorn
> > 
> >> Konrad
> >>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >>> index 8fa9fbfe5d00..5c68f2182c2f 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >>> @@ -3982,6 +3982,7 @@ apps_rsc: rsc@18200000 {
> >>>  			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
> >>>  					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
> >>>  			label = "apps_rsc";
> >>> +			power-domains = <&CLUSTER_PD>;
> >>>  
> >>>  			apps_bcm_voter: bcm-voter {
> >>>  				compatible = "qcom,bcm-voter";
Konrad Dybcio May 17, 2023, 9:23 p.m. UTC | #9 
On 16.05.2023 23:01, Bjorn Andersson wrote:
> On Mon, May 15, 2023 at 11:34:45AM +0200, Konrad Dybcio wrote:
>>
>>
>> On 15.05.2023 04:38, Bjorn Andersson wrote:
>>> On Sat, May 13, 2023 at 11:09:07AM +0200, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 12.05.2023 17:04, Bjorn Andersson wrote:
>>>>> The rpmh driver will cache sleep and wake votes until the cluster
>>>>> power-domain is about to enter idle, to avoid unnecessary writes. So
>>>>> associate the apps_rsc with the cluster pd, so that it can be notified
>>>>> about this event.
>>>>>
>>>>> Without this, only AMC votes are being commited.
>>>> Ouch.
>>>>
>>>> Should we make this required: in bindings and add it to all
>>>> platforms?
>>>>
>>>
>>> I though this was an optimization and in the absence of this callback
>>> the driver would just write out wake and sleep sets as well. But per the
>>> current implementation (and perhaps some underlying cause?) it is indeed
>>> required, if you care about power consumption.
>> Hm.. since it's not strictly required for operation, would something
>> like this be fitting?:
>>
> 
> I don't think it's required for operation, but the current
> implementation does require it.
> 
> So I think we should either require it in the binding to mimic the
> implementation, or the implementation should handle either case (only
> with a performance impact)
Let's just require it then.

Konrad
> 
>> oneOf:
>>   - required:
>>       [...]
>>       - power-domains
>>
>>   - required:
>>       [...]
>>     deprecated: true
>>
>> (if it even works this way)
> 
> I don't think it's worth supporting the combinations.
> 
> Regards,
> Bjorn
> 
>>
>> Konrad
>>>
>>>>>
>>>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>>>> ---
>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
>>>>
>>>
>>> The Fixes sounds reasonable.
>>>
>>> Thanks,
>>> Bjorn
>>>
>>>> Konrad
>>>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> index 8fa9fbfe5d00..5c68f2182c2f 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> @@ -3982,6 +3982,7 @@ apps_rsc: rsc@18200000 {
>>>>>  			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
>>>>>  					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
>>>>>  			label = "apps_rsc";
>>>>> +			power-domains = <&CLUSTER_PD>;
>>>>>  
>>>>>  			apps_bcm_voter: bcm-voter {
>>>>>  				compatible = "qcom,bcm-voter";
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 8fa9fbfe5d00..5c68f2182c2f 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3982,6 +3982,7 @@  apps_rsc: rsc@18200000 {
 			qcom,tcs-config = <ACTIVE_TCS  2>, <SLEEP_TCS   3>,
 					  <WAKE_TCS    3>, <CONTROL_TCS 1>;
 			label = "apps_rsc";
+			power-domains = <&CLUSTER_PD>;
 
 			apps_bcm_voter: bcm-voter {
 				compatible = "qcom,bcm-voter";