| Message ID | 20230512150425.3171122-1-quic_bjorande@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ce7c014937c442be677963848c7db62eccd94eac |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: Flush RSC sleep & wake votes | expand |
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";
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";
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,
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 >
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";
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
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
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";
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 --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";
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(+)