[PATCH 0/4] MMCC MSM8998 fixes

[PATCH 0/4] MMCC MSM8998 fixes

[Konrad Dybcio]

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>
---
Konrad Dybcio (4):
      arm64: dts: qcom: msm8998: Properly describe 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

 arch/arm64/boot/dts/qcom/msm8998.dtsi | 8 +++++---
 drivers/clk/qcom/gcc-msm8998.c        | 6 +++---
 drivers/clk/qcom/mmcc-msm8998.c       | 7 ++++++-
 3 files changed, 14 insertions(+), 7 deletions(-)
---
base-commit: d4cee89031c80066ec461bb77b5e13a4f37d5fd2
change-id: 20230531-topic-8998_mmssclk-fd4b2fb7f8c1

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

[PATCH 1/4] arm64: dts: qcom: msm8998: Properly describe MMSS SMMU

[Konrad Dybcio]

The MMSS SMMU has been abusingly consuming the exposed RPM interconnect
clock and not describing the power domain it needs. Put an end to that.

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

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index f0e943ff0046..a4016085b750 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -2737,10 +2737,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 =
@@ -2764,6 +2764,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.40.1

Re: [PATCH 1/4] arm64: dts: qcom: msm8998: Properly describe MMSS SMMU

[Bryan O'Donoghue]

On Wed, May 31, 2023 at 11:01 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> The MMSS SMMU has been abusingly consuming the exposed RPM interconnect
> clock and not describing the power domain it needs. Put an end to that.
>
> Fixes: 05ce21b54423 ("arm64: dts: qcom: msm8998: Configure the multimedia subsystem iommu")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index f0e943ff0046..a4016085b750 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -2737,10 +2737,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 =
> @@ -2764,6 +2764,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.40.1
>

I think you should split this up.

This should be two patches. One to describe the power-domain and
another for the removal of interconnect clocks.

I guess none of the multi-media connected stuff on 8998 works ... no
venus, mdp, camera - which would surely fail without the multi-media
AXI clock being on..

Anyway please split this patch up !

[PATCH 2/4] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks

[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")
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 be024f8093c5..c343bdfc3cc1 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2061,7 +2061,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),
@@ -2074,7 +2074,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),
@@ -2100,7 +2100,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.40.1

Re: [PATCH 2/4] clk: qcom: gcc-msm8998: Don't check halt bit on some branch clks

[Jeffrey Hugo]

On 5/31/2023 3:01 AM, Konrad Dybcio wrote:
> 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")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

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

[PATCH 3/4] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks

[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")
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 4490594bde69..9b98e0fb8b91 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -2453,6 +2453,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),
@@ -2468,6 +2469,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 = {
@@ -2485,6 +2487,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.40.1

Re: [PATCH 3/4] clk: qcom: mmcc-msm8998: Don't check halt bit on some branch clks

[Jeffrey Hugo]

On 5/31/2023 3:01 AM, Konrad Dybcio wrote:
> 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")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

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

[PATCH 4/4] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC

[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 9b98e0fb8b91..7b1d105afbd8 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -2628,11 +2628,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.40.1

Re: [PATCH 4/4] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC

[Jeffrey Hugo]

On Wed, May 31, 2023 at 3:01 AM Konrad Dybcio <konrad.dybcio@linaro.org> 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.
>
> 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>

Was this tested on a system where the bootloader has enabled the
display and it is active during Linux boot?

I seem to recall that in that scenario, Linux would boot up, see that
the GDSC is on, not see any clients for it (still initializing), turn
it off, and kill the display which then results in either a mess of
errors or a bus lockup.

-Jeff

Re: [PATCH 4/4] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC

[Konrad Dybcio]

On 1.06.2023 16:14, Jeffrey Hugo wrote:
> On Wed, May 31, 2023 at 3:01 AM Konrad Dybcio <konrad.dybcio@linaro.org> 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.
>>
>> 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>
> 
> Was this tested on a system where the bootloader has enabled the
> display and it is active during Linux boot?
No, I only have a device whose bootloader doesn't initialize display.

> 
> I seem to recall that in that scenario, Linux would boot up, see that
> the GDSC is on, not see any clients for it (still initializing), turn
> it off, and kill the display which then results in either a mess of
> errors or a bus lockup.
I see 2 possible scenarios: either the display shuts off (because
somebody hasn't described the hardware properly and Linux isn't)
aware of all dependencies, or we get bus errors because we only
shut down / initialize some hardware partially, also possibly due
to bad dependency description.

Konrad
> 
> -Jeff