linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/6] MMCC MSM8998 fixes
@ 2023-08-09 19:20 Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 1/6] arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU Konrad Dybcio
                   ` (8 more replies)
  0 siblings, 9 replies; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio, Krzysztof Kozlowski

8998 has a couple of issues related to its clock controllers. This series
attemps to fix some of them.

The DT patch should go in first for bisectability, otherwise
clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer
considered always-on.

This series results in less "clk stuck at 'on/off'" messages and should
marginally reduce power consumption.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v3:
- Make the commit message more meaningful in patch 6 (krzk)
- Pick up tags
- Link to v2: https://lore.kernel.org/r/20230531-topic-8998_mmssclk-v2-0-34273e275c51@linaro.org

Changes in v2:
- Update bindings
- Separate out the dt patch into two
- Pick up tags
- Link to v1: https://lore.kernel.org/r/20230531-topic-8998_mmssclk-v1-0-2b5a8fc90991@linaro.org

---
Konrad Dybcio (6):
      arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU
      arm64: dts: qcom: msm8998: Add missing power domain to MMSS SMMU
      clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks
      clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks
      clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
      dt-bindings: arm-smmu: Fix MSM8998 clocks description

 .../devicetree/bindings/iommu/arm,smmu.yaml        | 41 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/msm8998.dtsi              |  8 +++--
 drivers/clk/qcom/gcc-msm8998.c                     |  6 ++--
 drivers/clk/qcom/mmcc-msm8998.c                    |  7 +++-
 4 files changed, 55 insertions(+), 7 deletions(-)
---
base-commit: 21ef7b1e17d039053edaeaf41142423810572741
change-id: 20230531-topic-8998_mmssclk-fd4b2fb7f8c1

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/6] arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
@ 2023-08-09 19:20 ` Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 2/6] arm64: dts: qcom: msm8998: Add missing power domain to " Konrad Dybcio
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio

The MMSS SMMU has been abusingly consuming the exposed RPM interconnect
clock. Drop it.

Fixes: 05ce21b54423 ("arm64: dts: qcom: msm8998: Configure the multimedia subsystem iommu")
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index a41a34dbcc3c..e40f6461f4fd 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -2743,10 +2743,10 @@ mmss_smmu: iommu@cd00000 {
 
 			clocks = <&mmcc MNOC_AHB_CLK>,
 				 <&mmcc BIMC_SMMU_AHB_CLK>,
-				 <&rpmcc RPM_SMD_MMAXI_CLK>,
 				 <&mmcc BIMC_SMMU_AXI_CLK>;
-			clock-names = "iface-mm", "iface-smmu",
-				      "bus-mm", "bus-smmu";
+			clock-names = "iface-mm",
+				      "iface-smmu",
+				      "bus-smmu";
 
 			#global-interrupts = <0>;
 			interrupts =

-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/6] arm64: dts: qcom: msm8998: Add missing power domain to MMSS SMMU
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 1/6] arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU Konrad Dybcio
@ 2023-08-09 19:20 ` Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks Konrad Dybcio
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio

The MMSS SMMU has its own power domain. Attach it so that we can drop
the "keep it always-on" hack.

Fixes: 05ce21b54423 ("arm64: dts: qcom: msm8998: Configure the multimedia subsystem iommu")
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index e40f6461f4fd..39db67211fae 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -2770,6 +2770,8 @@ mmss_smmu: iommu@cd00000 {
 				<GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH>,
 				<GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH>,
 				<GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
+
+			power-domains = <&mmcc BIMC_SMMU_GDSC>;
 		};
 
 		remoteproc_adsp: remoteproc@17300000 {

-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 1/6] arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 2/6] arm64: dts: qcom: msm8998: Add missing power domain to " Konrad Dybcio
@ 2023-08-09 19:20 ` Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 4/6] clk: qcom: mmcc-msm8998: " Konrad Dybcio
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio

Some branch clocks are governed externally and we're only supposed to
send a request concerning their shutdown, not actually ensure it happens.

Use the BRANCH_HALT_SKIP define to skip checking the halt bit.

Fixes: db2c7c0a04b1 ("clk: qcom: Add missing msm8998 gcc_bimc_gfx_clk")
Fixes: b5f5f525c547 ("clk: qcom: Add MSM8998 Global Clock Control (GCC) driver")
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-msm8998.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index 1ba516a0c0f3..257b27d8eeb6 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2060,7 +2060,7 @@ static struct clk_branch gcc_gp3_clk = {
 
 static struct clk_branch gcc_bimc_gfx_clk = {
 	.halt_reg = 0x46040,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x46040,
 		.enable_mask = BIT(0),
@@ -2073,7 +2073,7 @@ static struct clk_branch gcc_bimc_gfx_clk = {
 
 static struct clk_branch gcc_gpu_bimc_gfx_clk = {
 	.halt_reg = 0x71010,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x71010,
 		.enable_mask = BIT(0),
@@ -2099,7 +2099,7 @@ static struct clk_branch gcc_gpu_bimc_gfx_src_clk = {
 
 static struct clk_branch gcc_gpu_cfg_ahb_clk = {
 	.halt_reg = 0x71004,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x71004,
 		.enable_mask = BIT(0),

-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 4/6] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
                   ` (2 preceding siblings ...)
  2023-08-09 19:20 ` [PATCH v3 3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks Konrad Dybcio
@ 2023-08-09 19:20 ` Konrad Dybcio
  2023-08-09 19:20 ` [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC Konrad Dybcio
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio

Some branch clocks are governed externally and we're only supposed to
send a request concerning their shutdown, not actually ensure it happens.

Use the BRANCH_HALT_SKIP define to skip checking the halt bit.

Fixes: d14b15b5931c ("clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver")
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/mmcc-msm8998.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
index c62aa270af1c..d0a5440e2291 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -2452,6 +2452,7 @@ static struct clk_branch fd_ahb_clk = {
 
 static struct clk_branch mnoc_ahb_clk = {
 	.halt_reg = 0x5024,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x5024,
 		.enable_mask = BIT(0),
@@ -2467,6 +2468,7 @@ static struct clk_branch mnoc_ahb_clk = {
 
 static struct clk_branch bimc_smmu_ahb_clk = {
 	.halt_reg = 0xe004,
+	.halt_check = BRANCH_HALT_SKIP,
 	.hwcg_reg = 0xe004,
 	.hwcg_bit = 1,
 	.clkr = {
@@ -2484,6 +2486,7 @@ static struct clk_branch bimc_smmu_ahb_clk = {
 
 static struct clk_branch bimc_smmu_axi_clk = {
 	.halt_reg = 0xe008,
+	.halt_check = BRANCH_HALT_SKIP,
 	.hwcg_reg = 0xe008,
 	.hwcg_bit = 1,
 	.clkr = {

-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
                   ` (3 preceding siblings ...)
  2023-08-09 19:20 ` [PATCH v3 4/6] clk: qcom: mmcc-msm8998: " Konrad Dybcio
@ 2023-08-09 19:20 ` Konrad Dybcio
  2023-08-10 18:20   ` Jeffrey Hugo
  2023-08-09 19:20 ` [PATCH v3 6/6] dt-bindings: arm-smmu: Fix MSM8998 clocks description Konrad Dybcio
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio

The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the
HW_CTRL flag (it's separate from hw_ctrl_addr).  In addition to that,
it should feature a cxc entry for bimc_smmu_axi_clk and be marked as
votable.

Fix all of these issues.

Fixes: d14b15b5931c ("clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/mmcc-msm8998.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
index d0a5440e2291..4fdc41e7d2a8 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -2627,11 +2627,13 @@ static struct gdsc camss_cpp_gdsc = {
 static struct gdsc bimc_smmu_gdsc = {
 	.gdscr = 0xe020,
 	.gds_hw_ctrl = 0xe024,
+	.cxcs = (unsigned int []){ 0xe008 },
+	.cxc_count = 1,
 	.pd = {
 		.name = "bimc_smmu",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL | ALWAYS_ON,
+	.flags = VOTABLE,
 };
 
 static struct clk_regmap *mmcc_msm8998_clocks[] = {

-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 6/6] dt-bindings: arm-smmu: Fix MSM8998 clocks description
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
                   ` (4 preceding siblings ...)
  2023-08-09 19:20 ` [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC Konrad Dybcio
@ 2023-08-09 19:20 ` Konrad Dybcio
  2023-08-11 11:44 ` [PATCH v3 0/6] MMCC MSM8998 fixes Will Deacon
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-09 19:20 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, AngeloGioacchino Del Regno, Michael Turquette,
	Stephen Boyd, Jeffrey Hugo, Imran Khan, Rajendra Nayak,
	Joonwoo Park, Jeffrey Hugo, Will Deacon, Robin Murphy,
	Joerg Roedel, Jeffrey Hugo
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Konrad Dybcio, Krzysztof Kozlowski

MSM8998 was abusingly referencing one of the internal bus clocks, that
were recently dropped from Linux (because the original implementation
did not make much sense), circumventing the interconnect framework.

Fix it by dropping the bus-mm clock (which requires separating 8998 from
similar entries) and keeping the rest as-is.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../devicetree/bindings/iommu/arm,smmu.yaml        | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
index 3a31a979709b..cf29ab10501c 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
@@ -270,6 +270,47 @@ allOf:
           contains:
             enum:
               - qcom,msm8998-smmu-v2
+    then:
+      anyOf:
+        - properties:
+            clock-names:
+              items:
+                - const: bus
+            clocks:
+              items:
+                - description: bus clock required for downstream bus access and for
+                    the smmu ptw
+        - properties:
+            clock-names:
+              items:
+                - const: iface
+                - const: mem
+                - const: mem_iface
+            clocks:
+              items:
+                - description: interface clock required to access smmu's registers
+                    through the TCU's programming interface.
+                - description: bus clock required for memory access
+                - description: bus clock required for GPU memory access
+        - properties:
+            clock-names:
+              items:
+                - const: iface-mm
+                - const: iface-smmu
+                - const: bus-smmu
+            clocks:
+              items:
+                - description: interface clock required to access mnoc's registers
+                    through the TCU's programming interface.
+                - description: interface clock required to access smmu's registers
+                    through the TCU's programming interface.
+                - description: bus clock required for the smmu ptw
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,sdm630-smmu-v2
               - qcom,sm6375-smmu-v2
     then:

-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
  2023-08-09 19:20 ` [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC Konrad Dybcio
@ 2023-08-10 18:20   ` Jeffrey Hugo
  2023-08-10 18:46     ` Konrad Dybcio
  0 siblings, 1 reply; 13+ messages in thread
From: Jeffrey Hugo @ 2023-08-10 18:20 UTC (permalink / raw)
  To: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, AngeloGioacchino Del Regno,
	Michael Turquette, Stephen Boyd, Jeffrey Hugo, Imran Khan,
	Rajendra Nayak, Joonwoo Park, Will Deacon, Robin Murphy,
	Joerg Roedel
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu

On 8/9/2023 1:20 PM, Konrad Dybcio wrote:
> The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the
> HW_CTRL flag (it's separate from hw_ctrl_addr).  In addition to that,
> it should feature a cxc entry for bimc_smmu_axi_clk and be marked as
> votable.

I appear to have confused HW_CTRL with hw_ctrl_addr.  Thanks for fixing 
that.

I recall I made it always-on for display handoff.  The bootloader on the 
laptops will enable the display, which means the MDP is active and using 
the SMMU.  The SMMU is powered by the GDSC as you know.  The MDP is 
going to be polling a framebuffer in DDR, which EFI services (efifb) is 
going to be updating.  All of this is active during linux boot, which is 
how the kernel bootlog gets printed on screen.

If I remember right, the GDSC will be registered.  When it is done 
probing, there will be no consumers.  So the Linux framework will step 
in and turn it off before the consumers come up.  This kills power to 
the SMMU.  If the SMMU doesn't come back on before the MDP polls DDR 
again, you get a bus hang and a crash.

I assumed that any msm8998 device would be using the MDP/GPU and thus 
the SMMU would pretty much always be powered on.

I expected this patch to break the laptop.  It does not in my testing. 
However, I see that I disabled the MMCC node in DT with a todo about the 
display.  So the GDSC is never registered, and then never gets turned 
off.  I believe that todo is pending some updates I need to make to the 
TI DSI/eDP bridge because the I2C port on the bridge is not wired up.  I 
should really dust that off and complete it.

Regardless, even with the todo addressed, I think removing always-on 
will still break the laptops unless the bootloader handoff of display 
was solved and I missed it.

I get that for your usecase, a phone where the bootloader does not init 
the display, always-on has the potential to burn extra power.  I'm not 
sure how to make both of us happy through.

Do you have any suggestions?

> 
> Fix all of these issues.
> 
> Fixes: d14b15b5931c ("clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/mmcc-msm8998.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
> index d0a5440e2291..4fdc41e7d2a8 100644
> --- a/drivers/clk/qcom/mmcc-msm8998.c
> +++ b/drivers/clk/qcom/mmcc-msm8998.c
> @@ -2627,11 +2627,13 @@ static struct gdsc camss_cpp_gdsc = {
>   static struct gdsc bimc_smmu_gdsc = {
>   	.gdscr = 0xe020,
>   	.gds_hw_ctrl = 0xe024,
> +	.cxcs = (unsigned int []){ 0xe008 },
> +	.cxc_count = 1,
>   	.pd = {
>   		.name = "bimc_smmu",
>   	},
>   	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = HW_CTRL | ALWAYS_ON,
> +	.flags = VOTABLE,
>   };
>   
>   static struct clk_regmap *mmcc_msm8998_clocks[] = {
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
  2023-08-10 18:20   ` Jeffrey Hugo
@ 2023-08-10 18:46     ` Konrad Dybcio
  2023-08-10 19:50       ` Jeffrey Hugo
  0 siblings, 1 reply; 13+ messages in thread
From: Konrad Dybcio @ 2023-08-10 18:46 UTC (permalink / raw)
  To: Jeffrey Hugo, Andy Gross, Bjorn Andersson, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, AngeloGioacchino Del Regno,
	Michael Turquette, Stephen Boyd, Jeffrey Hugo, Imran Khan,
	Rajendra Nayak, Joonwoo Park, Will Deacon, Robin Murphy,
	Joerg Roedel
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu

On 10.08.2023 20:20, Jeffrey Hugo wrote:
> On 8/9/2023 1:20 PM, Konrad Dybcio wrote:
>> The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the
>> HW_CTRL flag (it's separate from hw_ctrl_addr).  In addition to that,
>> it should feature a cxc entry for bimc_smmu_axi_clk and be marked as
>> votable.
> 
> I appear to have confused HW_CTRL with hw_ctrl_addr.  Thanks for fixing that.
> 
> I recall I made it always-on for display handoff.  The bootloader on the laptops will enable the display, which means the MDP is active and using the SMMU.  The SMMU is powered by the GDSC as you know.  The MDP is going to be polling a framebuffer in DDR, which EFI services (efifb) is going to be updating.  All of this is active during linux boot, which is how the kernel bootlog gets printed on screen.
This is essentially a missing / mis-configuration from the linux/dt POV and
I think the consensus for using display without describing it properly with
mdss has been to do one of:

- adding a simple-framebuffer node with all the necessary clocks/pds
- adding "clk_ignore_unused pd_ignore_unused" to your cmdline

> 
> If I remember right, the GDSC will be registered.  When it is done probing, there will be no consumers.  So the Linux framework will step in and turn it off before the consumers come up.  This kills power to the SMMU.  If the SMMU doesn't come back on before the MDP polls DDR again, you get a bus hang and a crash.
Yep

> I assumed that any msm8998 device would be using the MDP/GPU and thus the SMMU would pretty much always be powered on.
This flag however bans putting it to sleep when not in use.

> 
> I expected this patch to break the laptop.  It does not in my testing. However, I see that I disabled the MMCC node in DT with a todo about the display.  So the GDSC is never registered, and then never gets turned off.  I believe that todo is pending some updates I need to make to the TI DSI/eDP bridge because the I2C port on the bridge is not wired up.  I should really dust that off and complete it.
Right, so what you have now is a third, untold "solution" to the problem
described above.. not really a supported configuration as it's not "correct"

I'd happily see you wire up the bridge et al though!


> Regardless, even with the todo addressed, I think removing always-on will still break the laptops unless the bootloader handoff of display was solved and I missed it.
> 
> I get that for your usecase, a phone where the bootloader does not init the display, always-on has the potential to burn extra power.  I'm not sure how to make both of us happy through.
> 
> Do you have any suggestions?
Hope my replies above are enough.

Konrad

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
  2023-08-10 18:46     ` Konrad Dybcio
@ 2023-08-10 19:50       ` Jeffrey Hugo
  0 siblings, 0 replies; 13+ messages in thread
From: Jeffrey Hugo @ 2023-08-10 19:50 UTC (permalink / raw)
  To: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, AngeloGioacchino Del Regno,
	Michael Turquette, Stephen Boyd, Jeffrey Hugo, Imran Khan,
	Rajendra Nayak, Joonwoo Park, Will Deacon, Robin Murphy,
	Joerg Roedel
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu

On 8/10/2023 12:46 PM, Konrad Dybcio wrote:
> On 10.08.2023 20:20, Jeffrey Hugo wrote:
>> On 8/9/2023 1:20 PM, Konrad Dybcio wrote:
>>> The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the
>>> HW_CTRL flag (it's separate from hw_ctrl_addr).  In addition to that,
>>> it should feature a cxc entry for bimc_smmu_axi_clk and be marked as
>>> votable.
>>
>> I appear to have confused HW_CTRL with hw_ctrl_addr.  Thanks for fixing that.
>>
>> I recall I made it always-on for display handoff.  The bootloader on the laptops will enable the display, which means the MDP is active and using the SMMU.  The SMMU is powered by the GDSC as you know.  The MDP is going to be polling a framebuffer in DDR, which EFI services (efifb) is going to be updating.  All of this is active during linux boot, which is how the kernel bootlog gets printed on screen.
> This is essentially a missing / mis-configuration from the linux/dt POV and
> I think the consensus for using display without describing it properly with
> mdss has been to do one of:
> 
> - adding a simple-framebuffer node with all the necessary clocks/pds
> - adding "clk_ignore_unused pd_ignore_unused" to your cmdline
> 
>>
>> If I remember right, the GDSC will be registered.  When it is done probing, there will be no consumers.  So the Linux framework will step in and turn it off before the consumers come up.  This kills power to the SMMU.  If the SMMU doesn't come back on before the MDP polls DDR again, you get a bus hang and a crash.
> Yep
> 
>> I assumed that any msm8998 device would be using the MDP/GPU and thus the SMMU would pretty much always be powered on.
> This flag however bans putting it to sleep when not in use.
> 
>>
>> I expected this patch to break the laptop.  It does not in my testing. However, I see that I disabled the MMCC node in DT with a todo about the display.  So the GDSC is never registered, and then never gets turned off.  I believe that todo is pending some updates I need to make to the TI DSI/eDP bridge because the I2C port on the bridge is not wired up.  I should really dust that off and complete it.
> Right, so what you have now is a third, untold "solution" to the problem
> described above.. not really a supported configuration as it's not "correct"
> 
> I'd happily see you wire up the bridge et al though!
> 
> 
>> Regardless, even with the todo addressed, I think removing always-on will still break the laptops unless the bootloader handoff of display was solved and I missed it.
>>
>> I get that for your usecase, a phone where the bootloader does not init the display, always-on has the potential to burn extra power.  I'm not sure how to make both of us happy through.
>>
>> Do you have any suggestions?
> Hope my replies above are enough.

I still think there is an issue, but my setup is not as complete as your 
on mainline.  I'll clean things up and we'll solve the issues when we 
get to them.

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/6] MMCC MSM8998 fixes
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
                   ` (5 preceding siblings ...)
  2023-08-09 19:20 ` [PATCH v3 6/6] dt-bindings: arm-smmu: Fix MSM8998 clocks description Konrad Dybcio
@ 2023-08-11 11:44 ` Will Deacon
  2023-08-14  3:27 ` (subset) " Bjorn Andersson
  2023-09-20 17:14 ` Bjorn Andersson
  8 siblings, 0 replies; 13+ messages in thread
From: Will Deacon @ 2023-08-11 11:44 UTC (permalink / raw)
  To: Andy Gross, Joerg Roedel, Rajendra Nayak, Conor Dooley,
	Bjorn Andersson, Jeffrey Hugo, Konrad Dybcio, Rob Herring,
	Joonwoo Park, Michael Turquette, Stephen Boyd,
	Krzysztof Kozlowski, Imran Khan, Robin Murphy, Jeffrey Hugo,
	AngeloGioacchino Del Regno
  Cc: catalin.marinas, kernel-team, Will Deacon, Dmitry Baryshkov,
	linux-arm-msm, iommu, Jami Kettunen, Marijn Suijten,
	linux-arm-kernel, linux-clk, linux-kernel, devicetree,
	Krzysztof Kozlowski

On Wed, 09 Aug 2023 21:20:23 +0200, Konrad Dybcio wrote:
> 8998 has a couple of issues related to its clock controllers. This series
> attemps to fix some of them.
> 
> The DT patch should go in first for bisectability, otherwise
> clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer
> considered always-on.
> 
> [...]

Applied bindings patch to will (for-joerg/arm-smmu/bindings), thanks!

[6/6] dt-bindings: arm-smmu: Fix MSM8998 clocks description
      https://git.kernel.org/will/c/b606e2e8eded

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: (subset) [PATCH v3 0/6] MMCC MSM8998 fixes
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
                   ` (6 preceding siblings ...)
  2023-08-11 11:44 ` [PATCH v3 0/6] MMCC MSM8998 fixes Will Deacon
@ 2023-08-14  3:27 ` Bjorn Andersson
  2023-09-20 17:14 ` Bjorn Andersson
  8 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2023-08-14  3:27 UTC (permalink / raw)
  To: Andy Gross, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	AngeloGioacchino Del Regno, Michael Turquette, Stephen Boyd,
	Jeffrey Hugo, Imran Khan, Rajendra Nayak, Joonwoo Park,
	Jeffrey Hugo, Will Deacon, Robin Murphy, Joerg Roedel,
	Konrad Dybcio
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Krzysztof Kozlowski


On Wed, 09 Aug 2023 21:20:23 +0200, Konrad Dybcio wrote:
> 8998 has a couple of issues related to its clock controllers. This series
> attemps to fix some of them.
> 
> The DT patch should go in first for bisectability, otherwise
> clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer
> considered always-on.
> 
> [...]

Applied, thanks!

[3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks
      commit: a6f1e8623836bb6ce64c347d110ecb6259ae73c4
[4/6] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks
      (no commit info)

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: (subset) [PATCH v3 0/6] MMCC MSM8998 fixes
  2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
                   ` (7 preceding siblings ...)
  2023-08-14  3:27 ` (subset) " Bjorn Andersson
@ 2023-09-20 17:14 ` Bjorn Andersson
  8 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2023-09-20 17:14 UTC (permalink / raw)
  To: Andy Gross, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	AngeloGioacchino Del Regno, Michael Turquette, Stephen Boyd,
	Jeffrey Hugo, Imran Khan, Rajendra Nayak, Joonwoo Park,
	Jeffrey Hugo, Will Deacon, Robin Murphy, Joerg Roedel,
	Konrad Dybcio
  Cc: Marijn Suijten, Dmitry Baryshkov, Jami Kettunen, linux-arm-msm,
	devicetree, linux-kernel, linux-clk, linux-arm-kernel, iommu,
	Krzysztof Kozlowski


On Wed, 09 Aug 2023 21:20:23 +0200, Konrad Dybcio wrote:
> 8998 has a couple of issues related to its clock controllers. This series
> attemps to fix some of them.
> 
> The DT patch should go in first for bisectability, otherwise
> clk/pd_ignore_unused will need to be used, as the SMMU GDSC is no longer
> considered always-on.
> 
> [...]

Applied, thanks!

[3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks
      (no commit info)
[4/6] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks
      commit: 9906c4140897bbdbff7bb71c6ae67903cb9954ce
[5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
      commit: 1fc62c8347397faf4e18249e88ecd4470c0a5357

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-09-20 17:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-09 19:20 [PATCH v3 0/6] MMCC MSM8998 fixes Konrad Dybcio
2023-08-09 19:20 ` [PATCH v3 1/6] arm64: dts: qcom: msm8998: Drop bus clock reference from MMSS SMMU Konrad Dybcio
2023-08-09 19:20 ` [PATCH v3 2/6] arm64: dts: qcom: msm8998: Add missing power domain to " Konrad Dybcio
2023-08-09 19:20 ` [PATCH v3 3/6] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks Konrad Dybcio
2023-08-09 19:20 ` [PATCH v3 4/6] clk: qcom: mmcc-msm8998: " Konrad Dybcio
2023-08-09 19:20 ` [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC Konrad Dybcio
2023-08-10 18:20   ` Jeffrey Hugo
2023-08-10 18:46     ` Konrad Dybcio
2023-08-10 19:50       ` Jeffrey Hugo
2023-08-09 19:20 ` [PATCH v3 6/6] dt-bindings: arm-smmu: Fix MSM8998 clocks description Konrad Dybcio
2023-08-11 11:44 ` [PATCH v3 0/6] MMCC MSM8998 fixes Will Deacon
2023-08-14  3:27 ` (subset) " Bjorn Andersson
2023-09-20 17:14 ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).