| Message ID | 20230717-topic-branch_aon_cleanup-v6-0-46d136a4e8d0@linaro.org |
|---|---|
| Headers | show |
| Series | Unregister critical branch clocks + some RPM | expand |
On Sat, 13 Jan 2024 at 16:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > We hardcode some clocks to be always-on, as they're essential to the > functioning of the SoC / some peripherals. Add a helper to do so > to make the writes less magic. > > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/clk/qcom/clk-branch.h | 7 +++++++ > 1 file changed, 7 insertions(+) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Sat, 13 Jan 2024 at 16:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > Instead of magically poking at the bit0 of branch clocks' CBCR, use > the newly introduced helper. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/clk/qcom/camcc-sc8280xp.c | 6 ++---- > drivers/clk/qcom/camcc-sm8550.c | 10 +++------- > drivers/clk/qcom/dispcc-qcm2290.c | 4 ++-- > drivers/clk/qcom/dispcc-sc7280.c | 7 ++----- > drivers/clk/qcom/dispcc-sc8280xp.c | 4 ++-- > drivers/clk/qcom/dispcc-sm6115.c | 4 ++-- > drivers/clk/qcom/dispcc-sm8250.c | 4 ++-- > drivers/clk/qcom/dispcc-sm8450.c | 7 ++----- > drivers/clk/qcom/dispcc-sm8550.c | 7 ++----- > drivers/clk/qcom/dispcc-sm8650.c | 4 ++-- > drivers/clk/qcom/gcc-sa8775p.c | 25 ++++++++++--------------- > drivers/clk/qcom/gcc-sc7180.c | 22 +++++++++------------- > drivers/clk/qcom/gcc-sc7280.c | 20 ++++++++------------ > drivers/clk/qcom/gcc-sc8180x.c | 28 +++++++++++----------------- > drivers/clk/qcom/gcc-sc8280xp.c | 25 ++++++++++--------------- > drivers/clk/qcom/gcc-sdx55.c | 12 ++++-------- > drivers/clk/qcom/gcc-sdx65.c | 13 +++++-------- > drivers/clk/qcom/gcc-sdx75.c | 10 +++------- > drivers/clk/qcom/gcc-sm4450.c | 28 +++++++++------------------- > drivers/clk/qcom/gcc-sm6375.c | 11 ++++------- > drivers/clk/qcom/gcc-sm7150.c | 23 +++++++++-------------- > drivers/clk/qcom/gcc-sm8250.c | 19 +++++++------------ > drivers/clk/qcom/gcc-sm8350.c | 20 ++++++++------------ > drivers/clk/qcom/gcc-sm8450.c | 21 ++++++++------------- > drivers/clk/qcom/gcc-sm8550.c | 21 ++++++++------------- > drivers/clk/qcom/gcc-sm8650.c | 16 ++++++++-------- > drivers/clk/qcom/gcc-x1e80100.c | 16 ++++++++-------- > drivers/clk/qcom/gpucc-sc7280.c | 9 +++------ > drivers/clk/qcom/gpucc-sc8280xp.c | 9 +++------ > drivers/clk/qcom/gpucc-sm8550.c | 10 +++------- > drivers/clk/qcom/lpasscorecc-sc7180.c | 7 ++----- > drivers/clk/qcom/videocc-sm8250.c | 6 +++--- > drivers/clk/qcom/videocc-sm8350.c | 10 +++------- > drivers/clk/qcom/videocc-sm8450.c | 13 ++++--------- > drivers/clk/qcom/videocc-sm8550.c | 13 ++++--------- > 35 files changed, 175 insertions(+), 289 deletions(-) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Sat, 13 Jan 2024 at 16:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On Qualcomm SoCs, certain branch clocks either need to be always-on, or > should be if you're interested in touching some part of the hardware. > > Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, > however that messes with the runtime pm handling - if a clock is > marked as such, the clock controller device will never enter the > "suspended" state, leaving the associated resources online, which in > turn breaks SoC-wide suspend. > > This series aims to solve that on a couple SoCs that I could test the > changes on and it sprinkles some runtime pm enablement atop these drivers. Probably it is out of scope for this I wonder if it makes sense to route (some) of the clocks properly. Should we use GCC_foo_SLEEEP_CLK as a sleep clock for the corresponding device? I'm not sure about the AHB and XO clocks. Another question is regarding the suspended state. Wouldn't leaving GCC_foo_XO clocks enabled keep the XO enabled as well? > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Changes in v6: > - Rebase (next-20240112) > - Reorder qcom_branch_set_clk_en calls by register in "*: Unregister > critical clocks" (Johan) > - Pick up tags > - Link to v5: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v5-0-99942e6bf1ba@linaro.org > > Changes in v5: > - Change the "Keep the critical clocks always-on" comment to "Keep > some clocks always-on" > - Add the same comment to commits unregistering clocks on 6115/6375/2290 > - Link to v4: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v4-0-32c293ded915@linaro.org > > Changes in v4: > - Add and unify the "/* Keep the critical clocks always-on */" comment > - Rebase (next-20231222), also include 8650, X1E and 8280camcc drivers > - Drop enabling runtime PM on GCC > - Improve the commit message of "clk: qcom: gpucc-sm6115: Add runtime PM" > - Link to v3: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v3-0-3e31bce9c626@linaro.org > > Changes in v3: > - Rebase (next-20231219) > - Fix up a copypaste mistake in "gcc-sm6375: Unregister critical clocks" (bod) > - Pick up tags > - Link to v2: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v2-0-2a583460ef26@linaro.org > > Changes in v2: > - Rebase > - Pick up tags > - Fix up missing pm_runtime_put in SM6375 GCC (Johan) > - Clarify the commit message of "Add runtime PM" commits (Johan) > - "GPU_CCC" -> "GPU_CC" (oops) > - Rebase atop next-20231129 > - Also fix up camcc-sm8550 & gcc-sm4450 > - Unify and clean up the comment style > - Fix missing comments in gcc-sc7180.. > - Drop Johan's ack from "clk: qcom: Use qcom_branch_set_clk_en()" > - Improve 6115 dt patch commit message (Bjorn) > - Link to v1: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org > > --- > Konrad Dybcio (12): > clk: qcom: branch: Add a helper for setting the enable bit > clk: qcom: Use qcom_branch_set_clk_en() > clk: qcom: gcc-sm6375: Unregister critical clocks > clk: qcom: gpucc-sm6375: Unregister critical clocks > clk: qcom: gpucc-sm6115: Unregister critical clocks > clk: qcom: gpucc-sm6115: Add runtime PM > clk: qcom: gcc-sm6115: Unregister critical clocks > clk: qcom: gcc-qcm2290: Unregister critical clocks > arm64: dts: qcom: sm6375: Add VDD_CX to GCC > arm64: dts: qcom: qcm2290: Add VDD_CX to GCC > arm64: dts: qcom: sm6115: Add VDD_CX to GCC > arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CC > > arch/arm64/boot/dts/qcom/qcm2290.dtsi | 1 + > arch/arm64/boot/dts/qcom/sm6115.dtsi | 3 + > arch/arm64/boot/dts/qcom/sm6375.dtsi | 1 + > drivers/clk/qcom/camcc-sc8280xp.c | 6 +- > drivers/clk/qcom/camcc-sm8550.c | 10 +-- > drivers/clk/qcom/clk-branch.h | 7 ++ > drivers/clk/qcom/dispcc-qcm2290.c | 4 +- > drivers/clk/qcom/dispcc-sc7280.c | 7 +- > drivers/clk/qcom/dispcc-sc8280xp.c | 4 +- > drivers/clk/qcom/dispcc-sm6115.c | 4 +- > drivers/clk/qcom/dispcc-sm8250.c | 4 +- > drivers/clk/qcom/dispcc-sm8450.c | 7 +- > drivers/clk/qcom/dispcc-sm8550.c | 7 +- > drivers/clk/qcom/dispcc-sm8650.c | 4 +- > drivers/clk/qcom/gcc-qcm2290.c | 106 +++-------------------------- > drivers/clk/qcom/gcc-sa8775p.c | 25 +++---- > drivers/clk/qcom/gcc-sc7180.c | 22 +++--- > drivers/clk/qcom/gcc-sc7280.c | 20 +++--- > drivers/clk/qcom/gcc-sc8180x.c | 28 +++----- > drivers/clk/qcom/gcc-sc8280xp.c | 25 +++---- > drivers/clk/qcom/gcc-sdx55.c | 12 ++-- > drivers/clk/qcom/gcc-sdx65.c | 13 ++-- > drivers/clk/qcom/gcc-sdx75.c | 10 +-- > drivers/clk/qcom/gcc-sm4450.c | 28 +++----- > drivers/clk/qcom/gcc-sm6115.c | 124 +++------------------------------- > drivers/clk/qcom/gcc-sm6375.c | 105 +++------------------------- > drivers/clk/qcom/gcc-sm7150.c | 23 +++---- > drivers/clk/qcom/gcc-sm8250.c | 19 ++---- > drivers/clk/qcom/gcc-sm8350.c | 20 +++--- > drivers/clk/qcom/gcc-sm8450.c | 21 +++--- > drivers/clk/qcom/gcc-sm8550.c | 21 +++--- > drivers/clk/qcom/gcc-sm8650.c | 16 ++--- > drivers/clk/qcom/gcc-x1e80100.c | 16 ++--- > drivers/clk/qcom/gpucc-sc7280.c | 9 +-- > drivers/clk/qcom/gpucc-sc8280xp.c | 9 +-- > drivers/clk/qcom/gpucc-sm6115.c | 53 ++++++--------- > drivers/clk/qcom/gpucc-sm6375.c | 34 ++-------- > drivers/clk/qcom/gpucc-sm8550.c | 10 +-- > drivers/clk/qcom/lpasscorecc-sc7180.c | 7 +- > drivers/clk/qcom/videocc-sm8250.c | 6 +- > drivers/clk/qcom/videocc-sm8350.c | 10 +-- > drivers/clk/qcom/videocc-sm8450.c | 13 ++-- > drivers/clk/qcom/videocc-sm8550.c | 13 ++-- > 43 files changed, 234 insertions(+), 653 deletions(-) > --- > base-commit: 8d04a7e2ee3fd6aabb8096b00c64db0d735bc874 > change-id: 20230717-topic-branch_aon_cleanup-6976c13fe71c > > Best regards, > -- > Konrad Dybcio <konrad.dybcio@linaro.org> > >
On 14.01.2024 05:53, Dmitry Baryshkov wrote: > On Sat, 13 Jan 2024 at 16:51, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On Qualcomm SoCs, certain branch clocks either need to be always-on, or >> should be if you're interested in touching some part of the hardware. >> >> Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, >> however that messes with the runtime pm handling - if a clock is >> marked as such, the clock controller device will never enter the >> "suspended" state, leaving the associated resources online, which in >> turn breaks SoC-wide suspend. >> >> This series aims to solve that on a couple SoCs that I could test the >> changes on and it sprinkles some runtime pm enablement atop these drivers. > > Probably it is out of scope for this > I wonder if it makes sense to route (some) of the clocks properly. > Should we use GCC_foo_SLEEEP_CLK as a sleep clock for the > corresponding device? > I'm not sure about the AHB and XO clocks. > > Another question is regarding the suspended state. Wouldn't leaving > GCC_foo_XO clocks enabled keep the XO enabled as well? Doesn't seem to be the case Konrad
On 24-01-13 15:50:49, Konrad Dybcio wrote: > On Qualcomm SoCs, certain branch clocks either need to be always-on, or > should be if you're interested in touching some part of the hardware. > > Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, > however that messes with the runtime pm handling - if a clock is > marked as such, the clock controller device will never enter the > "suspended" state, leaving the associated resources online, which in > turn breaks SoC-wide suspend. Generally speaking, HW-wise, if the power domain of a clock controller is being disabled, all clocks that it provides are being disabled. Are you saying that is not the case ? > > This series aims to solve that on a couple SoCs that I could test the > changes on and it sprinkles some runtime pm enablement atop these drivers. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Changes in v6: > - Rebase (next-20240112) > - Reorder qcom_branch_set_clk_en calls by register in "*: Unregister > critical clocks" (Johan) > - Pick up tags > - Link to v5: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v5-0-99942e6bf1ba@linaro.org > > Changes in v5: > - Change the "Keep the critical clocks always-on" comment to "Keep > some clocks always-on" > - Add the same comment to commits unregistering clocks on 6115/6375/2290 > - Link to v4: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v4-0-32c293ded915@linaro.org > > Changes in v4: > - Add and unify the "/* Keep the critical clocks always-on */" comment > - Rebase (next-20231222), also include 8650, X1E and 8280camcc drivers > - Drop enabling runtime PM on GCC > - Improve the commit message of "clk: qcom: gpucc-sm6115: Add runtime PM" > - Link to v3: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v3-0-3e31bce9c626@linaro.org > > Changes in v3: > - Rebase (next-20231219) > - Fix up a copypaste mistake in "gcc-sm6375: Unregister critical clocks" (bod) > - Pick up tags > - Link to v2: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v2-0-2a583460ef26@linaro.org > > Changes in v2: > - Rebase > - Pick up tags > - Fix up missing pm_runtime_put in SM6375 GCC (Johan) > - Clarify the commit message of "Add runtime PM" commits (Johan) > - "GPU_CCC" -> "GPU_CC" (oops) > - Rebase atop next-20231129 > - Also fix up camcc-sm8550 & gcc-sm4450 > - Unify and clean up the comment style > - Fix missing comments in gcc-sc7180.. > - Drop Johan's ack from "clk: qcom: Use qcom_branch_set_clk_en()" > - Improve 6115 dt patch commit message (Bjorn) > - Link to v1: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org > > --- > Konrad Dybcio (12): > clk: qcom: branch: Add a helper for setting the enable bit > clk: qcom: Use qcom_branch_set_clk_en() > clk: qcom: gcc-sm6375: Unregister critical clocks > clk: qcom: gpucc-sm6375: Unregister critical clocks > clk: qcom: gpucc-sm6115: Unregister critical clocks > clk: qcom: gpucc-sm6115: Add runtime PM > clk: qcom: gcc-sm6115: Unregister critical clocks > clk: qcom: gcc-qcm2290: Unregister critical clocks > arm64: dts: qcom: sm6375: Add VDD_CX to GCC > arm64: dts: qcom: qcm2290: Add VDD_CX to GCC > arm64: dts: qcom: sm6115: Add VDD_CX to GCC > arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CC > > arch/arm64/boot/dts/qcom/qcm2290.dtsi | 1 + > arch/arm64/boot/dts/qcom/sm6115.dtsi | 3 + > arch/arm64/boot/dts/qcom/sm6375.dtsi | 1 + > drivers/clk/qcom/camcc-sc8280xp.c | 6 +- > drivers/clk/qcom/camcc-sm8550.c | 10 +-- > drivers/clk/qcom/clk-branch.h | 7 ++ > drivers/clk/qcom/dispcc-qcm2290.c | 4 +- > drivers/clk/qcom/dispcc-sc7280.c | 7 +- > drivers/clk/qcom/dispcc-sc8280xp.c | 4 +- > drivers/clk/qcom/dispcc-sm6115.c | 4 +- > drivers/clk/qcom/dispcc-sm8250.c | 4 +- > drivers/clk/qcom/dispcc-sm8450.c | 7 +- > drivers/clk/qcom/dispcc-sm8550.c | 7 +- > drivers/clk/qcom/dispcc-sm8650.c | 4 +- > drivers/clk/qcom/gcc-qcm2290.c | 106 +++-------------------------- > drivers/clk/qcom/gcc-sa8775p.c | 25 +++---- > drivers/clk/qcom/gcc-sc7180.c | 22 +++--- > drivers/clk/qcom/gcc-sc7280.c | 20 +++--- > drivers/clk/qcom/gcc-sc8180x.c | 28 +++----- > drivers/clk/qcom/gcc-sc8280xp.c | 25 +++---- > drivers/clk/qcom/gcc-sdx55.c | 12 ++-- > drivers/clk/qcom/gcc-sdx65.c | 13 ++-- > drivers/clk/qcom/gcc-sdx75.c | 10 +-- > drivers/clk/qcom/gcc-sm4450.c | 28 +++----- > drivers/clk/qcom/gcc-sm6115.c | 124 +++------------------------------- > drivers/clk/qcom/gcc-sm6375.c | 105 +++------------------------- > drivers/clk/qcom/gcc-sm7150.c | 23 +++---- > drivers/clk/qcom/gcc-sm8250.c | 19 ++---- > drivers/clk/qcom/gcc-sm8350.c | 20 +++--- > drivers/clk/qcom/gcc-sm8450.c | 21 +++--- > drivers/clk/qcom/gcc-sm8550.c | 21 +++--- > drivers/clk/qcom/gcc-sm8650.c | 16 ++--- > drivers/clk/qcom/gcc-x1e80100.c | 16 ++--- > drivers/clk/qcom/gpucc-sc7280.c | 9 +-- > drivers/clk/qcom/gpucc-sc8280xp.c | 9 +-- > drivers/clk/qcom/gpucc-sm6115.c | 53 ++++++--------- > drivers/clk/qcom/gpucc-sm6375.c | 34 ++-------- > drivers/clk/qcom/gpucc-sm8550.c | 10 +-- > drivers/clk/qcom/lpasscorecc-sc7180.c | 7 +- > drivers/clk/qcom/videocc-sm8250.c | 6 +- > drivers/clk/qcom/videocc-sm8350.c | 10 +-- > drivers/clk/qcom/videocc-sm8450.c | 13 ++-- > drivers/clk/qcom/videocc-sm8550.c | 13 ++-- > 43 files changed, 234 insertions(+), 653 deletions(-) > --- > base-commit: 8d04a7e2ee3fd6aabb8096b00c64db0d735bc874 > change-id: 20230717-topic-branch_aon_cleanup-6976c13fe71c > > Best regards, > -- > Konrad Dybcio <konrad.dybcio@linaro.org> > >
On 1/24/24 08:41, Abel Vesa wrote: > On 24-01-13 15:50:49, Konrad Dybcio wrote: >> On Qualcomm SoCs, certain branch clocks either need to be always-on, or >> should be if you're interested in touching some part of the hardware. >> >> Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, >> however that messes with the runtime pm handling - if a clock is >> marked as such, the clock controller device will never enter the >> "suspended" state, leaving the associated resources online, which in >> turn breaks SoC-wide suspend. > > Generally speaking, HW-wise, if the power domain of a clock controller > is being disabled, all clocks that it provides are being disabled. Generally speaking, if that's the case, that's true. > > Are you saying that is not the case ? Dragons however, are peculiar creatures and it seems like the clock controllers are not *really* disabled when we think they are, e.g. due to RPM(h) owning a share of GCC clocks, or due to the MX rail being always-on. It would indeed be an issue with hibernation where the registers would need to be reprogrammed after battery power is removed. As we spoke off-list, I'll split this series into two: adding common helpers and then taking care of 2290/6375/6115. I'm not yet sure how far we can go with converting existing clock drivers to use pm_clk_add so that the _AHB, _XO, and _SLEEP clocks for a given subsystem are only enabled when necessary - if we require an entry in clock-names, backwards compatibility goes away, and if we don't - we potentially miss out on a devlink between X_CC and GCC, plus the name needs to be hardcoded for global parent lookup. For new drivers, we'll likely just require a clean solution (runtime PM enabled + subsys clocks gotten as pm_clk through a dt entry on the consumer). Konrad
Hi Konrad, Thanks for your patch. On 1/13/2024 8:20 PM, Konrad Dybcio wrote: > On Qualcomm SoCs, certain branch clocks either need to be always-on, or > should be if you're interested in touching some part of the hardware. > > Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, > however that messes with the runtime pm handling - if a clock is > marked as such, the clock controller device will never enter the > "suspended" state, leaving the associated resources online, which in > turn breaks SoC-wide suspend. I am really curious to know a little more about the SoC-Wide Suspend not happening on these targets. Could you add more details here ? The Resource Power Manager (RPM) is the main aggregator on these targets where the active & sleep votes on XO, shared rails (CX/MX) decide the SoC wide suspend. The High Level OS on our internal platforms never had any suspend issues due to clocks(GCC/GPUCC) or shared rails being kept enabled from the consumers. > > This series aims to solve that on a couple SoCs that I could test the > changes on and it sprinkles some runtime pm enablement atop these drivers. > As CX is a shared resource/rail on these specific targets we definitely do not achieve any power saving with the runtime pm attached to these clock controllers, but I see a little more SW overhead. Though you could please add your observations/comments. Removing the CLK_IS_CRITICAL is a good cleanup and moving them to probe is a good way to handle the always-on clocks. > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Changes in v6: > - Rebase (next-20240112) > - Reorder qcom_branch_set_clk_en calls by register in "*: Unregister > critical clocks" (Johan) > - Pick up tags > - Link to v5: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v5-0-99942e6bf1ba@linaro.org > > Changes in v5: > - Change the "Keep the critical clocks always-on" comment to "Keep > some clocks always-on" > - Add the same comment to commits unregistering clocks on 6115/6375/2290 > - Link to v4: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v4-0-32c293ded915@linaro.org > > Changes in v4: > - Add and unify the "/* Keep the critical clocks always-on */" comment > - Rebase (next-20231222), also include 8650, X1E and 8280camcc drivers > - Drop enabling runtime PM on GCC > - Improve the commit message of "clk: qcom: gpucc-sm6115: Add runtime PM" > - Link to v3: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v3-0-3e31bce9c626@linaro.org > > Changes in v3: > - Rebase (next-20231219) > - Fix up a copypaste mistake in "gcc-sm6375: Unregister critical clocks" (bod) > - Pick up tags > - Link to v2: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v2-0-2a583460ef26@linaro.org > > Changes in v2: > - Rebase > - Pick up tags > - Fix up missing pm_runtime_put in SM6375 GCC (Johan) > - Clarify the commit message of "Add runtime PM" commits (Johan) > - "GPU_CCC" -> "GPU_CC" (oops) > - Rebase atop next-20231129 > - Also fix up camcc-sm8550 & gcc-sm4450 > - Unify and clean up the comment style > - Fix missing comments in gcc-sc7180.. > - Drop Johan's ack from "clk: qcom: Use qcom_branch_set_clk_en()" > - Improve 6115 dt patch commit message (Bjorn) > - Link to v1: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org > > --- > Konrad Dybcio (12): > clk: qcom: branch: Add a helper for setting the enable bit > clk: qcom: Use qcom_branch_set_clk_en() > clk: qcom: gcc-sm6375: Unregister critical clocks > clk: qcom: gpucc-sm6375: Unregister critical clocks > clk: qcom: gpucc-sm6115: Unregister critical clocks > clk: qcom: gpucc-sm6115: Add runtime PM > clk: qcom: gcc-sm6115: Unregister critical clocks > clk: qcom: gcc-qcm2290: Unregister critical clocks > arm64: dts: qcom: sm6375: Add VDD_CX to GCC > arm64: dts: qcom: qcm2290: Add VDD_CX to GCC > arm64: dts: qcom: sm6115: Add VDD_CX to GCC > arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CC > > arch/arm64/boot/dts/qcom/qcm2290.dtsi | 1 + > arch/arm64/boot/dts/qcom/sm6115.dtsi | 3 + > arch/arm64/boot/dts/qcom/sm6375.dtsi | 1 + > drivers/clk/qcom/camcc-sc8280xp.c | 6 +- > drivers/clk/qcom/camcc-sm8550.c | 10 +-- > drivers/clk/qcom/clk-branch.h | 7 ++ > drivers/clk/qcom/dispcc-qcm2290.c | 4 +- > drivers/clk/qcom/dispcc-sc7280.c | 7 +- > drivers/clk/qcom/dispcc-sc8280xp.c | 4 +- > drivers/clk/qcom/dispcc-sm6115.c | 4 +- > drivers/clk/qcom/dispcc-sm8250.c | 4 +- > drivers/clk/qcom/dispcc-sm8450.c | 7 +- > drivers/clk/qcom/dispcc-sm8550.c | 7 +- > drivers/clk/qcom/dispcc-sm8650.c | 4 +- > drivers/clk/qcom/gcc-qcm2290.c | 106 +++-------------------------- > drivers/clk/qcom/gcc-sa8775p.c | 25 +++---- > drivers/clk/qcom/gcc-sc7180.c | 22 +++--- > drivers/clk/qcom/gcc-sc7280.c | 20 +++--- > drivers/clk/qcom/gcc-sc8180x.c | 28 +++----- > drivers/clk/qcom/gcc-sc8280xp.c | 25 +++---- > drivers/clk/qcom/gcc-sdx55.c | 12 ++-- > drivers/clk/qcom/gcc-sdx65.c | 13 ++-- > drivers/clk/qcom/gcc-sdx75.c | 10 +-- > drivers/clk/qcom/gcc-sm4450.c | 28 +++----- > drivers/clk/qcom/gcc-sm6115.c | 124 +++------------------------------- > drivers/clk/qcom/gcc-sm6375.c | 105 +++------------------------- > drivers/clk/qcom/gcc-sm7150.c | 23 +++---- > drivers/clk/qcom/gcc-sm8250.c | 19 ++---- > drivers/clk/qcom/gcc-sm8350.c | 20 +++--- > drivers/clk/qcom/gcc-sm8450.c | 21 +++--- > drivers/clk/qcom/gcc-sm8550.c | 21 +++--- > drivers/clk/qcom/gcc-sm8650.c | 16 ++--- > drivers/clk/qcom/gcc-x1e80100.c | 16 ++--- > drivers/clk/qcom/gpucc-sc7280.c | 9 +-- > drivers/clk/qcom/gpucc-sc8280xp.c | 9 +-- > drivers/clk/qcom/gpucc-sm6115.c | 53 ++++++--------- > drivers/clk/qcom/gpucc-sm6375.c | 34 ++-------- > drivers/clk/qcom/gpucc-sm8550.c | 10 +-- > drivers/clk/qcom/lpasscorecc-sc7180.c | 7 +- > drivers/clk/qcom/videocc-sm8250.c | 6 +- > drivers/clk/qcom/videocc-sm8350.c | 10 +-- > drivers/clk/qcom/videocc-sm8450.c | 13 ++-- > drivers/clk/qcom/videocc-sm8550.c | 13 ++-- > 43 files changed, 234 insertions(+), 653 deletions(-) > --- > base-commit: 8d04a7e2ee3fd6aabb8096b00c64db0d735bc874 > change-id: 20230717-topic-branch_aon_cleanup-6976c13fe71c > > Best regards,
On 1/25/24 11:16, Taniya Das wrote: > Hi Konrad, > > Thanks for your patch. > > On 1/13/2024 8:20 PM, Konrad Dybcio wrote: >> On Qualcomm SoCs, certain branch clocks either need to be always-on, or >> should be if you're interested in touching some part of the hardware. >> >> Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, >> however that messes with the runtime pm handling - if a clock is >> marked as such, the clock controller device will never enter the >> "suspended" state, leaving the associated resources online, which in >> turn breaks SoC-wide suspend. > > I am really curious to know a little more about the SoC-Wide Suspend not happening on these targets. Could you add more details here ? > > The Resource Power Manager (RPM) is the main aggregator on these targets where the active & sleep votes on XO, shared rails (CX/MX) decide the SoC wide suspend. The High Level OS on our internal platforms never had any suspend issues due to clocks(GCC/GPUCC) or shared rails being kept enabled from the consumers. With the common clock framework, CLK_IS_CRITICAL blocks pm operations, as clk_disable fails at some point. Since RPM(h)PDs are modeled as pmdomains, this in turn results in them never getting disabled, leading to outstanding votes. Then, RPM(h) sees these votes and (among other things which are not properly described on most SoCs leading to dangling votes) decides that CXPD/XOSD/AOSD can't be entered because there's a request on a resource. > >> >> This series aims to solve that on a couple SoCs that I could test the >> changes on and it sprinkles some runtime pm enablement atop these drivers. >> > > As CX is a shared resource/rail on these specific targets we definitely do not achieve any power saving with the runtime pm attached to these clock controllers, but I see a little more SW overhead. Though you could please add your observations/comments. Hm, simply adding a power-domains entry to denote the required performance state values when voting for downstream GDSCs would be enough and runtime PM only makes sense if there's an additional rail, say MMCX or GFX. But see the comment below. > > Removing the CLK_IS_CRITICAL is a good cleanup and moving them to probe is a good way to handle the always-on clocks. Unless it turns out to be really messy with keeping backwards DTS compatibilty, the goal is to use the pm_clk APIs to only keep the interface/xo/sleep clocks for subsystems active when that subsystem is active (i.e. when SUBSYS_CC is *runtime-active*). This will result in a treewide patchset. Konrad
On Qualcomm SoCs, certain branch clocks either need to be always-on, or should be if you're interested in touching some part of the hardware. Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, however that messes with the runtime pm handling - if a clock is marked as such, the clock controller device will never enter the "suspended" state, leaving the associated resources online, which in turn breaks SoC-wide suspend. This series aims to solve that on a couple SoCs that I could test the changes on and it sprinkles some runtime pm enablement atop these drivers. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Changes in v6: - Rebase (next-20240112) - Reorder qcom_branch_set_clk_en calls by register in "*: Unregister critical clocks" (Johan) - Pick up tags - Link to v5: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v5-0-99942e6bf1ba@linaro.org Changes in v5: - Change the "Keep the critical clocks always-on" comment to "Keep some clocks always-on" - Add the same comment to commits unregistering clocks on 6115/6375/2290 - Link to v4: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v4-0-32c293ded915@linaro.org Changes in v4: - Add and unify the "/* Keep the critical clocks always-on */" comment - Rebase (next-20231222), also include 8650, X1E and 8280camcc drivers - Drop enabling runtime PM on GCC - Improve the commit message of "clk: qcom: gpucc-sm6115: Add runtime PM" - Link to v3: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v3-0-3e31bce9c626@linaro.org Changes in v3: - Rebase (next-20231219) - Fix up a copypaste mistake in "gcc-sm6375: Unregister critical clocks" (bod) - Pick up tags - Link to v2: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v2-0-2a583460ef26@linaro.org Changes in v2: - Rebase - Pick up tags - Fix up missing pm_runtime_put in SM6375 GCC (Johan) - Clarify the commit message of "Add runtime PM" commits (Johan) - "GPU_CCC" -> "GPU_CC" (oops) - Rebase atop next-20231129 - Also fix up camcc-sm8550 & gcc-sm4450 - Unify and clean up the comment style - Fix missing comments in gcc-sc7180.. - Drop Johan's ack from "clk: qcom: Use qcom_branch_set_clk_en()" - Improve 6115 dt patch commit message (Bjorn) - Link to v1: https://lore.kernel.org/r/20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org --- Konrad Dybcio (12): clk: qcom: branch: Add a helper for setting the enable bit clk: qcom: Use qcom_branch_set_clk_en() clk: qcom: gcc-sm6375: Unregister critical clocks clk: qcom: gpucc-sm6375: Unregister critical clocks clk: qcom: gpucc-sm6115: Unregister critical clocks clk: qcom: gpucc-sm6115: Add runtime PM clk: qcom: gcc-sm6115: Unregister critical clocks clk: qcom: gcc-qcm2290: Unregister critical clocks arm64: dts: qcom: sm6375: Add VDD_CX to GCC arm64: dts: qcom: qcm2290: Add VDD_CX to GCC arm64: dts: qcom: sm6115: Add VDD_CX to GCC arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CC arch/arm64/boot/dts/qcom/qcm2290.dtsi | 1 + arch/arm64/boot/dts/qcom/sm6115.dtsi | 3 + arch/arm64/boot/dts/qcom/sm6375.dtsi | 1 + drivers/clk/qcom/camcc-sc8280xp.c | 6 +- drivers/clk/qcom/camcc-sm8550.c | 10 +-- drivers/clk/qcom/clk-branch.h | 7 ++ drivers/clk/qcom/dispcc-qcm2290.c | 4 +- drivers/clk/qcom/dispcc-sc7280.c | 7 +- drivers/clk/qcom/dispcc-sc8280xp.c | 4 +- drivers/clk/qcom/dispcc-sm6115.c | 4 +- drivers/clk/qcom/dispcc-sm8250.c | 4 +- drivers/clk/qcom/dispcc-sm8450.c | 7 +- drivers/clk/qcom/dispcc-sm8550.c | 7 +- drivers/clk/qcom/dispcc-sm8650.c | 4 +- drivers/clk/qcom/gcc-qcm2290.c | 106 +++-------------------------- drivers/clk/qcom/gcc-sa8775p.c | 25 +++---- drivers/clk/qcom/gcc-sc7180.c | 22 +++--- drivers/clk/qcom/gcc-sc7280.c | 20 +++--- drivers/clk/qcom/gcc-sc8180x.c | 28 +++----- drivers/clk/qcom/gcc-sc8280xp.c | 25 +++---- drivers/clk/qcom/gcc-sdx55.c | 12 ++-- drivers/clk/qcom/gcc-sdx65.c | 13 ++-- drivers/clk/qcom/gcc-sdx75.c | 10 +-- drivers/clk/qcom/gcc-sm4450.c | 28 +++----- drivers/clk/qcom/gcc-sm6115.c | 124 +++------------------------------- drivers/clk/qcom/gcc-sm6375.c | 105 +++------------------------- drivers/clk/qcom/gcc-sm7150.c | 23 +++---- drivers/clk/qcom/gcc-sm8250.c | 19 ++---- drivers/clk/qcom/gcc-sm8350.c | 20 +++--- drivers/clk/qcom/gcc-sm8450.c | 21 +++--- drivers/clk/qcom/gcc-sm8550.c | 21 +++--- drivers/clk/qcom/gcc-sm8650.c | 16 ++--- drivers/clk/qcom/gcc-x1e80100.c | 16 ++--- drivers/clk/qcom/gpucc-sc7280.c | 9 +-- drivers/clk/qcom/gpucc-sc8280xp.c | 9 +-- drivers/clk/qcom/gpucc-sm6115.c | 53 ++++++--------- drivers/clk/qcom/gpucc-sm6375.c | 34 ++-------- drivers/clk/qcom/gpucc-sm8550.c | 10 +-- drivers/clk/qcom/lpasscorecc-sc7180.c | 7 +- drivers/clk/qcom/videocc-sm8250.c | 6 +- drivers/clk/qcom/videocc-sm8350.c | 10 +-- drivers/clk/qcom/videocc-sm8450.c | 13 ++-- drivers/clk/qcom/videocc-sm8550.c | 13 ++-- 43 files changed, 234 insertions(+), 653 deletions(-) --- base-commit: 8d04a7e2ee3fd6aabb8096b00c64db0d735bc874 change-id: 20230717-topic-branch_aon_cleanup-6976c13fe71c Best regards,