[PATCH 1/5] dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC

[PATCH 1/5] dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC

[Konrad Dybcio]

MSM8996 A2NoC contains a UFS master, which means the UFS hardware is
accessed every time sync_state is called within the interconnect
framework. It's all good on devices where this clock is already enabled
(most likely from the bootloader), but devices with eMMC storage are
rather unlikely to have it like that. Add the missing 2 clocks to the
binding.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../bindings/interconnect/qcom,rpm.yaml       | 24 ++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpm.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpm.yaml
index 4b37aa88a375..5e6be4e79201 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,rpm.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,rpm.yaml
@@ -84,7 +84,6 @@ allOf:
               - qcom,msm8939-pcnoc
               - qcom,msm8939-snoc
               - qcom,msm8996-a1noc
-              - qcom,msm8996-a2noc
               - qcom,msm8996-bimc
               - qcom,msm8996-cnoc
               - qcom,msm8996-pnoc
@@ -186,6 +185,29 @@ allOf:
       required:
         - power-domains
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,msm8996-a2noc
+
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: bus
+            - const: bus_a
+            - const: aggre2_ufs_axi
+            - const: ufs_axi
+
+        clocks:
+          items:
+            - description: Bus Clock
+            - description: Bus A Clock
+            - description: Aggregate2 NoC UFS AXI Clock
+            - description: UFS AXI Clock
+
   - if:
       properties:
         compatible:
-- 
2.38.1

Re: [PATCH 1/5] dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC

[Krzysztof Kozlowski]

On 10/12/2022 21:03, Konrad Dybcio wrote:
> MSM8996 A2NoC contains a UFS master, which means the UFS hardware is
> accessed every time sync_state is called within the interconnect
> framework. It's all good on devices where this clock is already enabled
> (most likely from the bootloader), but devices with eMMC storage are
> rather unlikely to have it like that. Add the missing 2 clocks to the
> binding.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 2/5] interconnect: qcom: msm8996: Provide UFS clocks to A2NoC

[Konrad Dybcio]

On eMMC devices the bootloader has no business enabling UFS clocks.
That results in a platform hang and hard reboot when trying to vote
on paths including MASTER_UFS and since sync_state guarantees that
it's done at boot time, this effectively prevents such devices from
booting. Fix that.

Fixes: 7add937f5222 ("interconnect: qcom: Add MSM8996 interconnect provider driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/msm8996.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index c2903ae3b3bc..7ddb1f23fb2a 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -33,6 +33,13 @@ static const char * const bus_a0noc_clocks[] = {
 	"aggre0_noc_mpu_cfg"
 };
 
+static const char * const bus_a2noc_clocks[] = {
+	"bus",
+	"bus_a",
+	"aggre2_ufs_axi",
+	"ufs_axi"
+};
+
 static const u16 mas_a0noc_common_links[] = {
 	MSM8996_SLAVE_A0NOC_SNOC
 };
@@ -1859,6 +1866,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = a2noc_nodes,
 	.num_nodes = ARRAY_SIZE(a2noc_nodes),
+	.clocks = bus_a2noc_clocks,
+	.num_clocks = ARRAY_SIZE(bus_a2noc_clocks),
 	.regmap_cfg = &msm8996_a2noc_regmap_config
 };
 
-- 
2.38.1

Re: [PATCH 2/5] interconnect: qcom: msm8996: Provide UFS clocks to A2NoC

[Dmitry Baryshkov]

On Sat, 10 Dec 2022 at 23:04, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On eMMC devices the bootloader has no business enabling UFS clocks.
> That results in a platform hang and hard reboot when trying to vote
> on paths including MASTER_UFS and since sync_state guarantees that
> it's done at boot time, this effectively prevents such devices from
> booting. Fix that.

The vendor kernel separates bus clocks and QoS clocks, so there is
possibly room for improvement.

Anyway:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>
> Fixes: 7add937f5222 ("interconnect: qcom: Add MSM8996 interconnect provider driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/msm8996.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index c2903ae3b3bc..7ddb1f23fb2a 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -33,6 +33,13 @@ static const char * const bus_a0noc_clocks[] = {
>         "aggre0_noc_mpu_cfg"
>  };
>
> +static const char * const bus_a2noc_clocks[] = {
> +       "bus",
> +       "bus_a",
> +       "aggre2_ufs_axi",
> +       "ufs_axi"
> +};
> +
>  static const u16 mas_a0noc_common_links[] = {
>         MSM8996_SLAVE_A0NOC_SNOC
>  };
> @@ -1859,6 +1866,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
>         .type = QCOM_ICC_NOC,
>         .nodes = a2noc_nodes,
>         .num_nodes = ARRAY_SIZE(a2noc_nodes),
> +       .clocks = bus_a2noc_clocks,
> +       .num_clocks = ARRAY_SIZE(bus_a2noc_clocks),
>         .regmap_cfg = &msm8996_a2noc_regmap_config
>  };
>
> --
> 2.38.1
>


-- 
With best wishes
Dmitry

[PATCH 3/5] interconnect: qcom: msm8996: Fix regmap max_register values

[Konrad Dybcio]

The device tree reg starts at BUS_BASE + QoS_OFFSET, but the regmap
configs in the ICC driver had values suggesting the reg started at
BUS_BASE. Shrink them down (where they haven't been already, so for
providers where QoS_OFFSET = 0) to make sure they stay within their
window.

Fixes: 7add937f5222 ("interconnect: qcom: Add MSM8996 interconnect provider driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/msm8996.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 7ddb1f23fb2a..25a1a32bc611 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -1813,7 +1813,7 @@ static const struct regmap_config msm8996_a0noc_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
 	.val_bits	= 32,
-	.max_register	= 0x9000,
+	.max_register	= 0x6000,
 	.fast_io	= true
 };
 
@@ -1837,7 +1837,7 @@ static const struct regmap_config msm8996_a1noc_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
 	.val_bits	= 32,
-	.max_register	= 0x7000,
+	.max_register	= 0x5000,
 	.fast_io	= true
 };
 
@@ -1858,7 +1858,7 @@ static const struct regmap_config msm8996_a2noc_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
 	.val_bits	= 32,
-	.max_register	= 0xa000,
+	.max_register	= 0x7000,
 	.fast_io	= true
 };
 
@@ -1886,7 +1886,7 @@ static const struct regmap_config msm8996_bimc_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
 	.val_bits	= 32,
-	.max_register	= 0x62000,
+	.max_register	= 0x5a000,
 	.fast_io	= true
 };
 
@@ -1997,7 +1997,7 @@ static const struct regmap_config msm8996_mnoc_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
 	.val_bits	= 32,
-	.max_register	= 0x20000,
+	.max_register	= 0x1c000,
 	.fast_io	= true
 };
 
-- 
2.38.1

Re: [PATCH 3/5] interconnect: qcom: msm8996: Fix regmap max_register values

[Dmitry Baryshkov]

On Sat, 10 Dec 2022 at 23:04, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> The device tree reg starts at BUS_BASE + QoS_OFFSET, but the regmap
> configs in the ICC driver had values suggesting the reg started at
> BUS_BASE. Shrink them down (where they haven't been already, so for
> providers where QoS_OFFSET = 0) to make sure they stay within their
> window.
>
> Fixes: 7add937f5222 ("interconnect: qcom: Add MSM8996 interconnect provider driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/msm8996.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 7ddb1f23fb2a..25a1a32bc611 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -1813,7 +1813,7 @@ static const struct regmap_config msm8996_a0noc_regmap_config = {
>         .reg_bits       = 32,
>         .reg_stride     = 4,
>         .val_bits       = 32,
> -       .max_register   = 0x9000,
> +       .max_register   = 0x6000,
>         .fast_io        = true
>  };
>
> @@ -1837,7 +1837,7 @@ static const struct regmap_config msm8996_a1noc_regmap_config = {
>         .reg_bits       = 32,
>         .reg_stride     = 4,
>         .val_bits       = 32,
> -       .max_register   = 0x7000,
> +       .max_register   = 0x5000,
>         .fast_io        = true
>  };
>
> @@ -1858,7 +1858,7 @@ static const struct regmap_config msm8996_a2noc_regmap_config = {
>         .reg_bits       = 32,
>         .reg_stride     = 4,
>         .val_bits       = 32,
> -       .max_register   = 0xa000,
> +       .max_register   = 0x7000,
>         .fast_io        = true
>  };
>
> @@ -1886,7 +1886,7 @@ static const struct regmap_config msm8996_bimc_regmap_config = {
>         .reg_bits       = 32,
>         .reg_stride     = 4,
>         .val_bits       = 32,
> -       .max_register   = 0x62000,
> +       .max_register   = 0x5a000,
>         .fast_io        = true
>  };
>
> @@ -1997,7 +1997,7 @@ static const struct regmap_config msm8996_mnoc_regmap_config = {
>         .reg_bits       = 32,
>         .reg_stride     = 4,
>         .val_bits       = 32,
> -       .max_register   = 0x20000,
> +       .max_register   = 0x1c000,
>         .fast_io        = true
>  };
>
> --
> 2.38.1
>


-- 
With best wishes
Dmitry

[PATCH 4/5] interconnect: qcom: rpm: Use _optional func for provider clocks

[Konrad Dybcio]

It turned out that - very unfortunately - msm8996 needs a binding
update, adding 2 more clocks to the A2NoC node. Use the _optional
variant of devm_clk_get to make sure old DTs will still probe with
newer versions of the driver.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index ba6781f54ab7..df3196f72536 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -488,7 +488,7 @@ int qnoc_probe(struct platform_device *pdev)
 	}
 
 regmap_done:
-	ret = devm_clk_bulk_get(dev, qp->num_clks, qp->bus_clks);
+	ret = devm_clk_bulk_get_optional(dev, qp->num_clks, qp->bus_clks);
 	if (ret)
 		return ret;
 
-- 
2.38.1

Re: [PATCH 4/5] interconnect: qcom: rpm: Use _optional func for provider clocks

[Dmitry Baryshkov]

On Sat, 10 Dec 2022 at 23:04, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> It turned out that - very unfortunately - msm8996 needs a binding
> update, adding 2 more clocks to the A2NoC node. Use the _optional
> variant of devm_clk_get to make sure old DTs will still probe with
> newer versions of the driver.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 5/5] arm64: dts: qcom: msm8996: Add additional A2NoC clocks

[Konrad Dybcio]

On eMMC devices, the UFS clocks aren't started in the bootloader (or well,
at least it should not be, as that would just leak power..), which results
in platform reboots when trying to access the unclocked UFS hardware,
which unfortunately happens on each and every boot, as interconnect calls
sync_state and goes over each and every path.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 95dc10a1deb2..616b5b1b1fb0 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -826,9 +826,11 @@ a2noc: interconnect@583000 {
 			compatible = "qcom,msm8996-a2noc";
 			reg = <0x00583000 0x7000>;
 			#interconnect-cells = <1>;
-			clock-names = "bus", "bus_a";
+			clock-names = "bus", "bus_a", "aggre2_ufs_axi", "ufs_axi";
 			clocks = <&rpmcc RPM_SMD_AGGR2_NOC_CLK>,
-				 <&rpmcc RPM_SMD_AGGR2_NOC_A_CLK>;
+				 <&rpmcc RPM_SMD_AGGR2_NOC_A_CLK>,
+				 <&gcc GCC_AGGRE2_UFS_AXI_CLK>,
+				 <&gcc GCC_UFS_AXI_CLK>;
 		};
 
 		mnoc: interconnect@5a4000 {
-- 
2.38.1

Re: [PATCH 5/5] arm64: dts: qcom: msm8996: Add additional A2NoC clocks

[Dmitry Baryshkov]

On Sat, 10 Dec 2022 at 23:04, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On eMMC devices, the UFS clocks aren't started in the bootloader (or well,
> at least it should not be, as that would just leak power..), which results
> in platform reboots when trying to access the unclocked UFS hardware,
> which unfortunately happens on each and every boot, as interconnect calls
> sync_state and goes over each and every path.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry