| Message ID | 20230303-topic-rpmcc_sleep-v2-1-ae80a325fe94@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SMD RPMCC sleep preparations | expand |
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: > Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all > (or at least most) of the oneline peripherals ask the interconnect > framework to keep their buses online and guarantee enough bandwidth, > we're relying on bootloader defaults to keep the said buses alive through > RPM requests and rate setting on RPM clocks. > > Without that in place, the RPM clocks are never enabled in the CCF, which > qualifies them to be cleaned up, since - as far as Linux is concerned - > nobody's using them and they're just wasting power. Doing so will end > tragically, as within miliseconds we'll get *some* access attempt on an > unlocked bus which will cause a platform crash. > > On the other hand, if we want to save power and put well-supported > platforms to sleep, we should be shutting off at least some of these > clocks (this time with a clear distinction of which ones are *actually* > not in use, coming from the interconnect driver). > > To differentiate between these two cases while not breaking older DTs, > introduce an opt-in property to correctly mark RPM clocks as enabled > after handoff (the initial max freq vote) and hence qualify them for the > common unused clock cleanup. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > index 2a95bf8664f9..386153f61971 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > @@ -58,6 +58,12 @@ properties: > minItems: 1 > maxItems: 2 > > + qcom,clk-disable-unused: > + type: boolean > + description: > + Indicates whether unused RPM clocks can be shut down with the common > + unused clock cleanup. Requires a functional interconnect driver. I don't think this should be QCom specific. Come up with something common (which will probably have some debate). Rob
On 16.03.2023 23:58, Rob Herring wrote: > On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: >> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all >> (or at least most) of the oneline peripherals ask the interconnect >> framework to keep their buses online and guarantee enough bandwidth, >> we're relying on bootloader defaults to keep the said buses alive through >> RPM requests and rate setting on RPM clocks. >> >> Without that in place, the RPM clocks are never enabled in the CCF, which >> qualifies them to be cleaned up, since - as far as Linux is concerned - >> nobody's using them and they're just wasting power. Doing so will end >> tragically, as within miliseconds we'll get *some* access attempt on an >> unlocked bus which will cause a platform crash. >> >> On the other hand, if we want to save power and put well-supported >> platforms to sleep, we should be shutting off at least some of these >> clocks (this time with a clear distinction of which ones are *actually* >> not in use, coming from the interconnect driver). >> >> To differentiate between these two cases while not breaking older DTs, >> introduce an opt-in property to correctly mark RPM clocks as enabled >> after handoff (the initial max freq vote) and hence qualify them for the >> common unused clock cleanup. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >> index 2a95bf8664f9..386153f61971 100644 >> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >> @@ -58,6 +58,12 @@ properties: >> minItems: 1 >> maxItems: 2 >> >> + qcom,clk-disable-unused: >> + type: boolean >> + description: >> + Indicates whether unused RPM clocks can be shut down with the common >> + unused clock cleanup. Requires a functional interconnect driver. > > I don't think this should be QCom specific. Come up with something > common (which will probably have some debate). Generally the opposite (ignoring unused clocks during the cleanup) is the thing you need to opt into. I can however see how (especially with the focus on not breaking things for older DTs) somebody else may also decide to only allow them to be cleaned up conditionally (by marking the clocks that were enabled earlier as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we do here. Stephen, Rob, would `clk-disable-unused` be a fitting generic property name for that? Should we also think about `clk-ignore-unused` as a clock-controller-specific alternative to the CCF-wide clk_ignore_unused cmdline? Konrad > > Rob
Quoting Konrad Dybcio (2023-03-16 17:31:34) > > On 16.03.2023 23:58, Rob Herring wrote: > > On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: > >> > >> + qcom,clk-disable-unused: > >> + type: boolean > >> + description: > >> + Indicates whether unused RPM clocks can be shut down with the common > >> + unused clock cleanup. Requires a functional interconnect driver. > > > > I don't think this should be QCom specific. Come up with something > > common (which will probably have some debate). > Generally the opposite (ignoring unused clocks during the cleanup) is > the thing you need to opt into. > > I can however see how (especially with the focus on not breaking things > for older DTs) somebody else may also decide to only allow them to be > cleaned up conditionally (by marking the clocks that were enabled earlier > as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we > do here. > > Stephen, Rob, would `clk-disable-unused` be a fitting generic property > name for that? Should we also think about `clk-ignore-unused` as a > clock-controller-specific alternative to the CCF-wide clk_ignore_unused > cmdline? > There are multiple threads on the list about disabling unused clks. Moving the decision to disable unused clks to a DT property is yet another approach. I'd rather not do that, because it really isn't describing the hardware configuration. If anything, I'd expect the property to be describing which clks are enabled by the firmware and then leave the decision to disable them because they're unused up to the software.
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: > Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all > (or at least most) of the oneline peripherals ask the interconnect > framework to keep their buses online and guarantee enough bandwidth, > we're relying on bootloader defaults to keep the said buses alive through > RPM requests and rate setting on RPM clocks. > > Without that in place, the RPM clocks are never enabled in the CCF, which > qualifies them to be cleaned up, since - as far as Linux is concerned - > nobody's using them and they're just wasting power. Doing so will end > tragically, as within miliseconds we'll get *some* access attempt on an > unlocked bus which will cause a platform crash. > > On the other hand, if we want to save power and put well-supported > platforms to sleep, we should be shutting off at least some of these > clocks (this time with a clear distinction of which ones are *actually* > not in use, coming from the interconnect driver). > > To differentiate between these two cases while not breaking older DTs, > introduce an opt-in property to correctly mark RPM clocks as enabled > after handoff (the initial max freq vote) and hence qualify them for the > common unused clock cleanup. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > index 2a95bf8664f9..386153f61971 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > @@ -58,6 +58,12 @@ properties: > minItems: 1 > maxItems: 2 > > + qcom,clk-disable-unused: This is a Linux implementation detail, not a description of the hardware. So it unfortunately doesn't belong here. > + type: boolean > + description: > + Indicates whether unused RPM clocks can be shut down with the common > + unused clock cleanup. Requires a functional interconnect driver. This is an interesting aspect though, there's a lot of things that will break if any one of these building blocks are missing, for any reason. Regards, Bjorn > + > required: > - compatible > - '#clock-cells' > > -- > 2.39.2 >
On 17.03.2023 19:20, Stephen Boyd wrote: > Quoting Konrad Dybcio (2023-03-16 17:31:34) >> >> On 16.03.2023 23:58, Rob Herring wrote: >>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: >>>> >>>> + qcom,clk-disable-unused: >>>> + type: boolean >>>> + description: >>>> + Indicates whether unused RPM clocks can be shut down with the common >>>> + unused clock cleanup. Requires a functional interconnect driver. >>> >>> I don't think this should be QCom specific. Come up with something >>> common (which will probably have some debate). >> Generally the opposite (ignoring unused clocks during the cleanup) is >> the thing you need to opt into. >> >> I can however see how (especially with the focus on not breaking things >> for older DTs) somebody else may also decide to only allow them to be >> cleaned up conditionally (by marking the clocks that were enabled earlier >> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we >> do here. >> >> Stephen, Rob, would `clk-disable-unused` be a fitting generic property >> name for that? Should we also think about `clk-ignore-unused` as a >> clock-controller-specific alternative to the CCF-wide clk_ignore_unused >> cmdline? >> > > There are multiple threads on the list about disabling unused clks. > Moving the decision to disable unused clks to a DT property is yet > another approach. I'd rather not do that, because it really isn't > describing the hardware configuration. If anything, I'd expect the > property to be describing which clks are enabled by the firmware and > then leave the decision to disable them because they're unused up to the > software. After some more thinking, I realized that this could be made opt-in simply with driver_data.. WDYT? Konrad
On 6.04.2023 16:44, Konrad Dybcio wrote: > > > On 17.03.2023 19:20, Stephen Boyd wrote: >> Quoting Konrad Dybcio (2023-03-16 17:31:34) >>> >>> On 16.03.2023 23:58, Rob Herring wrote: >>>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: >>>>> >>>>> + qcom,clk-disable-unused: >>>>> + type: boolean >>>>> + description: >>>>> + Indicates whether unused RPM clocks can be shut down with the common >>>>> + unused clock cleanup. Requires a functional interconnect driver. >>>> >>>> I don't think this should be QCom specific. Come up with something >>>> common (which will probably have some debate). >>> Generally the opposite (ignoring unused clocks during the cleanup) is >>> the thing you need to opt into. >>> >>> I can however see how (especially with the focus on not breaking things >>> for older DTs) somebody else may also decide to only allow them to be >>> cleaned up conditionally (by marking the clocks that were enabled earlier >>> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we >>> do here. >>> >>> Stephen, Rob, would `clk-disable-unused` be a fitting generic property >>> name for that? Should we also think about `clk-ignore-unused` as a >>> clock-controller-specific alternative to the CCF-wide clk_ignore_unused >>> cmdline? >>> >> >> There are multiple threads on the list about disabling unused clks. >> Moving the decision to disable unused clks to a DT property is yet >> another approach. I'd rather not do that, because it really isn't >> describing the hardware configuration. If anything, I'd expect the >> property to be describing which clks are enabled by the firmware and >> then leave the decision to disable them because they're unused up to the >> software. > After some more thinking, I realized that this could be made opt-in > simply with driver_data.. > > WDYT? ..on a re-evaluation, obviously not a great idea.. Old DTBs will not be happy about that. Konrad > > Konrad
On 7.04.2023 22:17, Konrad Dybcio wrote: > > > On 6.04.2023 16:44, Konrad Dybcio wrote: >> >> >> On 17.03.2023 19:20, Stephen Boyd wrote: >>> Quoting Konrad Dybcio (2023-03-16 17:31:34) >>>> >>>> On 16.03.2023 23:58, Rob Herring wrote: >>>>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: >>>>>> >>>>>> + qcom,clk-disable-unused: >>>>>> + type: boolean >>>>>> + description: >>>>>> + Indicates whether unused RPM clocks can be shut down with the common >>>>>> + unused clock cleanup. Requires a functional interconnect driver. >>>>> >>>>> I don't think this should be QCom specific. Come up with something >>>>> common (which will probably have some debate). >>>> Generally the opposite (ignoring unused clocks during the cleanup) is >>>> the thing you need to opt into. >>>> >>>> I can however see how (especially with the focus on not breaking things >>>> for older DTs) somebody else may also decide to only allow them to be >>>> cleaned up conditionally (by marking the clocks that were enabled earlier >>>> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we >>>> do here. >>>> >>>> Stephen, Rob, would `clk-disable-unused` be a fitting generic property >>>> name for that? Should we also think about `clk-ignore-unused` as a >>>> clock-controller-specific alternative to the CCF-wide clk_ignore_unused >>>> cmdline? >>>> >>> >>> There are multiple threads on the list about disabling unused clks. >>> Moving the decision to disable unused clks to a DT property is yet >>> another approach. I'd rather not do that, because it really isn't >>> describing the hardware configuration. If anything, I'd expect the >>> property to be describing which clks are enabled by the firmware and >>> then leave the decision to disable them because they're unused up to the >>> software. >> After some more thinking, I realized that this could be made opt-in >> simply with driver_data.. >> >> WDYT? > ..on a re-evaluation, obviously not a great idea.. Old DTBs will not > be happy about that. Another idea would be to yank out the not-very-useful "qcom,rpmcc" fallback compatible and present .is_enabled etc. when it's absent.. Directly checking for the interconnect handle to rpmcc is not possible, as interconnect requires rpmcc.. And then somebody's interconnect driver may not be "good enough" (like 8996 and pre-6.3 DTs). Konrad > > Konrad >> >> Konrad
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: > Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all > (or at least most) of the oneline peripherals ask the interconnect > framework to keep their buses online and guarantee enough bandwidth, > we're relying on bootloader defaults to keep the said buses alive through > RPM requests and rate setting on RPM clocks. > > Without that in place, the RPM clocks are never enabled in the CCF, which > qualifies them to be cleaned up, since - as far as Linux is concerned - > nobody's using them and they're just wasting power. Doing so will end > tragically, as within miliseconds we'll get *some* access attempt on an > unlocked bus which will cause a platform crash. > > On the other hand, if we want to save power and put well-supported > platforms to sleep, we should be shutting off at least some of these > clocks (this time with a clear distinction of which ones are *actually* > not in use, coming from the interconnect driver). > > To differentiate between these two cases while not breaking older DTs, > introduce an opt-in property to correctly mark RPM clocks as enabled > after handoff (the initial max freq vote) and hence qualify them for the > common unused clock cleanup. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > index 2a95bf8664f9..386153f61971 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > @@ -58,6 +58,12 @@ properties: > minItems: 1 > maxItems: 2 > > + qcom,clk-disable-unused: > + type: boolean > + description: > + Indicates whether unused RPM clocks can be shut down with the common > + unused clock cleanup. Requires a functional interconnect driver. > + I'm surprised that Stephen Boyd did not bring up his usual "rant" here of moving the interconnect clock voting out of rpmcc into the interconnect drivers (see [1], [2]). :-) I was a bit "cautious" about it back then but at this point I think it kind of makes sense. Make sure to read Stephen's detailed explanation in https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/ We keep looking for workarounds to prevent the CCF from "messing" with interconnect-related clocks. But the CCF cannot mess with "clocks" it does not manage. The RPM interconnect drivers already talk directly to the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there and just bypass the CCF entirely. For backwards compatibility (for platforms without interconnect drivers) one could either assume that the bootloader bandwidth votes will be sufficient and just leave those clocks completely alone. Or the "icc_smd_rpm" platform device could initially make max votes similar to the rpmcc device. By coincidence the "icc_smd_rpm" platform device is always created, no matter how the device tree looks or if the platform actually has an interconnect driver. Stephan [1]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/ [2]: https://lore.kernel.org/linux-arm-msm/20211209091005.D3344C004DD@smtp.kernel.org/
Quoting Stephan Gerhold (2023-04-17 12:05:06) > On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: > > Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all > > (or at least most) of the oneline peripherals ask the interconnect > > framework to keep their buses online and guarantee enough bandwidth, > > we're relying on bootloader defaults to keep the said buses alive through > > RPM requests and rate setting on RPM clocks. > > > > Without that in place, the RPM clocks are never enabled in the CCF, which > > qualifies them to be cleaned up, since - as far as Linux is concerned - > > nobody's using them and they're just wasting power. Doing so will end > > tragically, as within miliseconds we'll get *some* access attempt on an > > unlocked bus which will cause a platform crash. > > > > On the other hand, if we want to save power and put well-supported > > platforms to sleep, we should be shutting off at least some of these > > clocks (this time with a clear distinction of which ones are *actually* > > not in use, coming from the interconnect driver). > > > > To differentiate between these two cases while not breaking older DTs, > > introduce an opt-in property to correctly mark RPM clocks as enabled > > after handoff (the initial max freq vote) and hence qualify them for the > > common unused clock cleanup. > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > --- > > Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > > index 2a95bf8664f9..386153f61971 100644 > > --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > > +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > > @@ -58,6 +58,12 @@ properties: > > minItems: 1 > > maxItems: 2 > > > > + qcom,clk-disable-unused: > > + type: boolean > > + description: > > + Indicates whether unused RPM clocks can be shut down with the common > > + unused clock cleanup. Requires a functional interconnect driver. > > + > > I'm surprised that Stephen Boyd did not bring up his usual "rant" here > of moving the interconnect clock voting out of rpmcc into the > interconnect drivers (see [1], [2]). :-) :) I was hoping to get a fix for disabling unused clks during late init at the same time. Shucks! > > I was a bit "cautious" about it back then but at this point I think it > kind of makes sense. Make sure to read Stephen's detailed explanation in > https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/ > > We keep looking for workarounds to prevent the CCF from "messing" with > interconnect-related clocks. But the CCF cannot mess with "clocks" it > does not manage. The RPM interconnect drivers already talk directly to > the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be > quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there > and just bypass the CCF entirely. Please do it! > > For backwards compatibility (for platforms without interconnect drivers) > one could either assume that the bootloader bandwidth votes will be > sufficient and just leave those clocks completely alone. Or the > "icc_smd_rpm" platform device could initially make max votes similar to > the rpmcc device. By coincidence the "icc_smd_rpm" platform device is > always created, no matter how the device tree looks or if the platform > actually has an interconnect driver. > Yeah that's a good plan. Suspend will be broken or burn a lot of power, but presumably the new DTB will be used fairly quickly. Or you can implement something like clkdev for interconnects that lets you hack up an association between interconnects and consumers for existing DTs and then drop those lookups months later.
On 18.04.2023 02:19, Stephen Boyd wrote: > Quoting Stephan Gerhold (2023-04-17 12:05:06) >> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: >>> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all >>> (or at least most) of the oneline peripherals ask the interconnect >>> framework to keep their buses online and guarantee enough bandwidth, >>> we're relying on bootloader defaults to keep the said buses alive through >>> RPM requests and rate setting on RPM clocks. >>> >>> Without that in place, the RPM clocks are never enabled in the CCF, which >>> qualifies them to be cleaned up, since - as far as Linux is concerned - >>> nobody's using them and they're just wasting power. Doing so will end >>> tragically, as within miliseconds we'll get *some* access attempt on an >>> unlocked bus which will cause a platform crash. >>> >>> On the other hand, if we want to save power and put well-supported >>> platforms to sleep, we should be shutting off at least some of these >>> clocks (this time with a clear distinction of which ones are *actually* >>> not in use, coming from the interconnect driver). >>> >>> To differentiate between these two cases while not breaking older DTs, >>> introduce an opt-in property to correctly mark RPM clocks as enabled >>> after handoff (the initial max freq vote) and hence qualify them for the >>> common unused clock cleanup. >>> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>> --- >>> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >>> index 2a95bf8664f9..386153f61971 100644 >>> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >>> @@ -58,6 +58,12 @@ properties: >>> minItems: 1 >>> maxItems: 2 >>> >>> + qcom,clk-disable-unused: >>> + type: boolean >>> + description: >>> + Indicates whether unused RPM clocks can be shut down with the common >>> + unused clock cleanup. Requires a functional interconnect driver. >>> + >> >> I'm surprised that Stephen Boyd did not bring up his usual "rant" here >> of moving the interconnect clock voting out of rpmcc into the >> interconnect drivers (see [1], [2]). :-) > > :) I was hoping to get a fix for disabling unused clks during late init > at the same time. Shucks! > >> >> I was a bit "cautious" about it back then but at this point I think it >> kind of makes sense. Make sure to read Stephen's detailed explanation in >> https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/ >> >> We keep looking for workarounds to prevent the CCF from "messing" with >> interconnect-related clocks. But the CCF cannot mess with "clocks" it >> does not manage. The RPM interconnect drivers already talk directly to >> the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be >> quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there >> and just bypass the CCF entirely. > > Please do it! Okay, that's a plan.. > >> >> For backwards compatibility (for platforms without interconnect drivers) >> one could either assume that the bootloader bandwidth votes will be >> sufficient and just leave those clocks completely alone. Or the >> "icc_smd_rpm" platform device could initially make max votes similar to >> the rpmcc device. By coincidence the "icc_smd_rpm" platform device is >> always created, no matter how the device tree looks or if the platform >> actually has an interconnect driver. >> > > Yeah that's a good plan. Suspend will be broken or burn a lot of power, (that's what happens as of today, so sgtm!) > but presumably the new DTB will be used fairly quickly. Or you can > implement something like clkdev for interconnects that lets you hack up > an association between interconnects and consumers for existing DTs and > then drop those lookups months later. Uh.. let's not.. Let's just contain it in the interconnect driver. The buses will be at bearable frequencies coming from the bootloader (as RPM, storage etc. are enabled) and boosting them at icc_rpm_smd probe sounds sane. Konrad
On 18.04.2023 12:33, Konrad Dybcio wrote: > > > On 18.04.2023 02:19, Stephen Boyd wrote: >> Quoting Stephan Gerhold (2023-04-17 12:05:06) >>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: >>>> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all >>>> (or at least most) of the oneline peripherals ask the interconnect >>>> framework to keep their buses online and guarantee enough bandwidth, >>>> we're relying on bootloader defaults to keep the said buses alive through >>>> RPM requests and rate setting on RPM clocks. >>>> >>>> Without that in place, the RPM clocks are never enabled in the CCF, which >>>> qualifies them to be cleaned up, since - as far as Linux is concerned - >>>> nobody's using them and they're just wasting power. Doing so will end >>>> tragically, as within miliseconds we'll get *some* access attempt on an >>>> unlocked bus which will cause a platform crash. >>>> >>>> On the other hand, if we want to save power and put well-supported >>>> platforms to sleep, we should be shutting off at least some of these >>>> clocks (this time with a clear distinction of which ones are *actually* >>>> not in use, coming from the interconnect driver). >>>> >>>> To differentiate between these two cases while not breaking older DTs, >>>> introduce an opt-in property to correctly mark RPM clocks as enabled >>>> after handoff (the initial max freq vote) and hence qualify them for the >>>> common unused clock cleanup. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >>>> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >>>> index 2a95bf8664f9..386153f61971 100644 >>>> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >>>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml >>>> @@ -58,6 +58,12 @@ properties: >>>> minItems: 1 >>>> maxItems: 2 >>>> >>>> + qcom,clk-disable-unused: >>>> + type: boolean >>>> + description: >>>> + Indicates whether unused RPM clocks can be shut down with the common >>>> + unused clock cleanup. Requires a functional interconnect driver. >>>> + >>> >>> I'm surprised that Stephen Boyd did not bring up his usual "rant" here >>> of moving the interconnect clock voting out of rpmcc into the >>> interconnect drivers (see [1], [2]). :-) >> >> :) I was hoping to get a fix for disabling unused clks during late init >> at the same time. Shucks! >> >>> >>> I was a bit "cautious" about it back then but at this point I think it >>> kind of makes sense. Make sure to read Stephen's detailed explanation in >>> https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/ >>> >>> We keep looking for workarounds to prevent the CCF from "messing" with >>> interconnect-related clocks. But the CCF cannot mess with "clocks" it >>> does not manage. The RPM interconnect drivers already talk directly to >>> the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be >>> quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there >>> and just bypass the CCF entirely. >> >> Please do it! > Okay, that's a plan.. > >> >>> >>> For backwards compatibility (for platforms without interconnect drivers) >>> one could either assume that the bootloader bandwidth votes will be >>> sufficient and just leave those clocks completely alone. Or the >>> "icc_smd_rpm" platform device could initially make max votes similar to >>> the rpmcc device. By coincidence the "icc_smd_rpm" platform device is >>> always created, no matter how the device tree looks or if the platform >>> actually has an interconnect driver. >>> >> >> Yeah that's a good plan. Suspend will be broken or burn a lot of power, > (that's what happens as of today, so sgtm!) > >> but presumably the new DTB will be used fairly quickly. Or you can >> implement something like clkdev for interconnects that lets you hack up >> an association between interconnects and consumers for existing DTs and >> then drop those lookups months later. > Uh.. let's not.. Let's just contain it in the interconnect driver. > > The buses will be at bearable frequencies coming from the bootloader > (as RPM, storage etc. are enabled) and boosting them at icc_rpm_smd > probe sounds sane. What should we do about the non-bus RPM clocks though? I don't fancy IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN on msm8909 with these clocks shut down (albeit with a very basic dt setup)! Taking into account the old interconnect-enabled DTs, some of the clocks would need to be on so that the QoS writes can succeed (e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again.. I suppose something like this would work-ish: 0. remove clock handles as they're now contained within icc and use them as a "legacy marker" 1. add: if (qp->bus_clocks) // skip qos writes This will: - let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up - save massively on code complexity at the cost of retroactively removing features (QoS settings) for people with old DTs and new kernels (don't tell Torvalds!) This DTB ABI stuff really gets in the way sometimes :/ We're only now fixing up U-Boot to be able to use upstream Linux DTs and other than that I think only OpenBSD uses it with 8280.. Wish we could get rid of all old junk once and then establish immutability but oh well.. Konrad > > Konrad
On Wed, Apr 19, 2023 at 01:31:01PM +0200, Konrad Dybcio wrote: > What should we do about the non-bus RPM clocks though? I don't fancy > IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN > on msm8909 with these clocks shut down (albeit with a very basic dt setup)! > > Taking into account the old interconnect-enabled DTs, some of the > clocks would need to be on so that the QoS writes can succeed > (e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again.. > I guess MSM8996 is the only platform affected by this? sdm630.dtsi seems to list the clock already in the a2noc and all others don't seem to have an interconnect driver yet. This will be subjective and someone will surely disagree but... IMO forcing all RPM clocks on during boot and keeping them enabled is not part of the DT ABI. If you don't describe the hardware correctly and are missing necessary clocks in the description (like the IPA_CLK on the interconnect node) then your DT is wrong and should be fixed. I would see this a bit like typical optimizing C compilers nowadays. If you write correct code it can optimize, e.g. drop unnecessary function calls. But if you write incorrect code with undefined behavior it's not the fault of the compiler if you run into trouble. The code must be fixed. The DT bindings don't specify that unused resources (clocks, ...) stay "magically" active. They specify that that the resources you reference are available. As such, I would say the OS is free to optimize here and turn off unused resources. The more important point IMO is not breaking all platforms without interconnect drivers. This goes beyond just adding a missing clock to the DT, you need to write the driver first. But having the max vote in icc_smd_rpm (somehow) should hopefully take care of that. > I suppose something like this would work-ish: > > 0. remove clock handles as they're now contained within icc and > use them as a "legacy marker" > 1. add: > if (qp->bus_clocks) > // skip qos writes Maybe you can just check if all necessary clocks for QOS are there or not? I don't think it's a problem to skip it on broken DTs. I think it would be even fine to refuse loading the interconnect driver completely and just have the standard max vote (as long as that results in a booting system). > > This will: > - let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up > - save massively on code complexity > +1 > at the cost of retroactively removing features (QoS settings) for people > with old DTs and new kernels (don't tell Torvalds!) > I doubt anyone will notice :p > This DTB ABI stuff really gets in the way sometimes :/ We're only now > fixing up U-Boot to be able to use upstream Linux DTs and other than > that I think only OpenBSD uses it with 8280.. Wish we could get rid of > all old junk once and then establish immutability but oh well.. Nice, thanks a lot for working on addressing the Qualcomm DT mess in U-Boot. I've been meaning to work this myself for a long time but never found the time to start... :') Thanks, Stephan
On 19.04.2023 16:00, Stephan Gerhold wrote: > On Wed, Apr 19, 2023 at 01:31:01PM +0200, Konrad Dybcio wrote: >> What should we do about the non-bus RPM clocks though? I don't fancy >> IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN >> on msm8909 with these clocks shut down (albeit with a very basic dt setup)! >> >> Taking into account the old interconnect-enabled DTs, some of the >> clocks would need to be on so that the QoS writes can succeed >> (e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again.. >> > > I guess MSM8996 is the only platform affected by this? sdm630.dtsi seems > to list the clock already in the a2noc and all others don't seem to have > an interconnect driver yet. > > This will be subjective and someone will surely disagree but... > > IMO forcing all RPM clocks on during boot and keeping them enabled is > not part of the DT ABI. If you don't describe the hardware correctly and > are missing necessary clocks in the description (like the IPA_CLK on the > interconnect node) then your DT is wrong and should be fixed. > > I would see this a bit like typical optimizing C compilers nowadays. If > you write correct code it can optimize, e.g. drop unnecessary function > calls. But if you write incorrect code with undefined behavior it's not > the fault of the compiler if you run into trouble. The code must be > fixed. > > The DT bindings don't specify that unused resources (clocks, ...) stay > "magically" active. They specify that that the resources you reference > are available. As such, I would say the OS is free to optimize here and > turn off unused resources. > > The more important point IMO is not breaking all platforms without > interconnect drivers. This goes beyond just adding a missing clock to > the DT, you need to write the driver first. But having the max vote in > icc_smd_rpm (somehow) should hopefully take care of that. Hm, interesting argument. Krzysztof, Bjorn, what's your stance on this? We *need* to add unused cleanup to rpmcc for feature completion and there's no good way of discerning whether it's safe to do so.. Doing so will make clk_ignore_unused necessary to boot with legacy DTs. Stephan argues the DTs were incomplete from the start and the breakage is only a result of us previously abusing what's essentially undefined behavior.. I think I second this, but it is *a* breakage so I want to know your opinion. FWIW the same happens when we have simple-framebuffer enabled and then introduce dispcc on a given platform without adding the clocks under the simplefb node and we've not been frowning upon that too much, so I'd be willing to give it a pass if you're okay with it.. Not caring about this would make things far, far easier really.. Konrad > >> I suppose something like this would work-ish: >> >> 0. remove clock handles as they're now contained within icc and >> use them as a "legacy marker" >> 1. add: >> if (qp->bus_clocks) >> // skip qos writes > > Maybe you can just check if all necessary clocks for QOS are there or > not? I don't think it's a problem to skip it on broken DTs. I think it > would be even fine to refuse loading the interconnect driver completely > and just have the standard max vote (as long as that results in a > booting system). > >> >> This will: >> - let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up >> - save massively on code complexity >> > > +1 > >> at the cost of retroactively removing features (QoS settings) for people >> with old DTs and new kernels (don't tell Torvalds!) >> > > I doubt anyone will notice :p > >> This DTB ABI stuff really gets in the way sometimes :/ We're only now >> fixing up U-Boot to be able to use upstream Linux DTs and other than >> that I think only OpenBSD uses it with 8280.. Wish we could get rid of >> all old junk once and then establish immutability but oh well.. > > Nice, thanks a lot for working on addressing the Qualcomm DT mess in > U-Boot. I've been meaning to work this myself for a long time but never > found the time to start... :') > > Thanks, > Stephan
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote: > Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all > (or at least most) of the oneline peripherals ask the interconnect > framework to keep their buses online and guarantee enough bandwidth, > we're relying on bootloader defaults to keep the said buses alive through > RPM requests and rate setting on RPM clocks. > > Without that in place, the RPM clocks are never enabled in the CCF, which > qualifies them to be cleaned up, since - as far as Linux is concerned - > nobody's using them and they're just wasting power. Doing so will end > tragically, as within miliseconds we'll get *some* access attempt on an > unlocked bus which will cause a platform crash. > > On the other hand, if we want to save power and put well-supported > platforms to sleep, we should be shutting off at least some of these > clocks (this time with a clear distinction of which ones are *actually* > not in use, coming from the interconnect driver). > > To differentiate between these two cases while not breaking older DTs, > introduce an opt-in property to correctly mark RPM clocks as enabled > after handoff (the initial max freq vote) and hence qualify them for the > common unused clock cleanup. > My 2 cents here... First, this property doesn't belong in DT at all as it is OS specific handling. This leaves us with the option of using a cmdline or module params for rmpcc. But we already have one (clk_ignore_unused), so the platforms making use of old DTB's should use that instead. And that get's rid of the debate that when you start disabling rpmcc clocks, old platforms will break. I don't see a valid point to keep the old platforms alive since their DTB (firmware) is broken already. So either they have to fix the DTB or use a cmdline option. - Mani > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > index 2a95bf8664f9..386153f61971 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml > @@ -58,6 +58,12 @@ properties: > minItems: 1 > maxItems: 2 > > + qcom,clk-disable-unused: > + type: boolean > + description: > + Indicates whether unused RPM clocks can be shut down with the common > + unused clock cleanup. Requires a functional interconnect driver. > + > required: > - compatible > - '#clock-cells' > > -- > 2.39.2 >
diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml index 2a95bf8664f9..386153f61971 100644 --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml @@ -58,6 +58,12 @@ properties: minItems: 1 maxItems: 2 + qcom,clk-disable-unused: + type: boolean + description: + Indicates whether unused RPM clocks can be shut down with the common + unused clock cleanup. Requires a functional interconnect driver. + required: - compatible - '#clock-cells'
Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all (or at least most) of the oneline peripherals ask the interconnect framework to keep their buses online and guarantee enough bandwidth, we're relying on bootloader defaults to keep the said buses alive through RPM requests and rate setting on RPM clocks. Without that in place, the RPM clocks are never enabled in the CCF, which qualifies them to be cleaned up, since - as far as Linux is concerned - nobody's using them and they're just wasting power. Doing so will end tragically, as within miliseconds we'll get *some* access attempt on an unlocked bus which will cause a platform crash. On the other hand, if we want to save power and put well-supported platforms to sleep, we should be shutting off at least some of these clocks (this time with a clear distinction of which ones are *actually* not in use, coming from the interconnect driver). To differentiate between these two cases while not breaking older DTs, introduce an opt-in property to correctly mark RPM clocks as enabled after handoff (the initial max freq vote) and hence qualify them for the common unused clock cleanup. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++ 1 file changed, 6 insertions(+)