[v6,00/12] Unregister critical branch clocks + some RPM

Message ID	20230717-topic-branch_aon_cleanup-v6-0-46d136a4e8d0@linaro.org
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-25298-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; From: Konrad Dybcio <konrad.dybcio@linaro.org> Subject: [PATCH v6 00/12] Unregister critical branch clocks + some RPM Date: Sat, 13 Jan 2024 15:50:49 +0100 Message-Id: <20230717-topic-branch_aon_cleanup-v6-0-46d136a4e8d0@linaro.org> Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: Bjorn Andersson <andersson@kernel.org>, Andy Gross <agross@kernel.org>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org> Cc: Marijn Suijten <marijn.suijten@somainline.org>, linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Konrad Dybcio <konrad.dybcio@linaro.org>, Johan Hovold <johan+linaro@kernel.org>, Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Series	Unregister critical branch clocks + some RPM \| [v6,00/12] Unregister critical branch clocks + some RPM [v6,01/12] clk: qcom: branch: Add a helper for setting the enable bit [v6,02/12] clk: qcom: Use qcom_branch_set_clk_en() [v6,03/12] clk: qcom: gcc-sm6375: Unregister critical clocks [v6,04/12] clk: qcom: gpucc-sm6375: Unregister critical clocks [v6,05/12] clk: qcom: gpucc-sm6115: Unregister critical clocks [v6,06/12] clk: qcom: gpucc-sm6115: Add runtime PM [v6,07/12] clk: qcom: gcc-sm6115: Unregister critical clocks [v6,08/12] clk: qcom: gcc-qcm2290: Unregister critical clocks [v6,09/12] arm64: dts: qcom: sm6375: Add VDD_CX to GCC [v6,10/12] arm64: dts: qcom: qcm2290: Add VDD_CX to GCC [v6,11/12] arm64: dts: qcom: sm6115: Add VDD_CX to GCC [v6,12/12] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CC

Message ID

20230717-topic-branch_aon_cleanup-v6-0-46d136a4e8d0@linaro.org

Headers

Received-SPF: pass (google.com: domain of
 linux-kernel+bounces-25298-ouuuleilei=gmail.com@vger.kernel.org designates
 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Konrad Dybcio <konrad.dybcio@linaro.org>
Subject: [PATCH v6 00/12] Unregister critical branch clocks + some RPM
Date: Sat, 13 Jan 2024 15:50:49 +0100
Message-Id: <20230717-topic-branch_aon_cleanup-v6-0-46d136a4e8d0@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
To: Bjorn Andersson <andersson@kernel.org>, Andy Gross <agross@kernel.org>,
 Michael Turquette <mturquette@baylibre.com>,
 Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>
Cc: Marijn Suijten <marijn.suijten@somainline.org>,
 linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
 linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
 Konrad Dybcio <konrad.dybcio@linaro.org>,
 Johan Hovold <johan+linaro@kernel.org>,
 Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Series

Unregister critical branch clocks + some RPM |

Message

Konrad Dybcio Jan. 13, 2024, 2:50 p.m. UTC

  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,

Comments

Dmitry Baryshkov Jan. 14, 2024, 4:53 a.m. UTC | #1

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

Konrad Dybcio Jan. 15, 2024, 9:29 a.m. UTC | #2

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

Abel Vesa Jan. 24, 2024, 7:41 a.m. UTC | #3

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

Konrad Dybcio Jan. 24, 2024, 12:31 p.m. UTC | #4

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

Taniya Das Jan. 25, 2024, 10:16 a.m. UTC | #5

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,

Konrad Dybcio Jan. 25, 2024, 10:56 a.m. UTC | #6

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