[PATCH v2 0/4] Add resets for ADSP based audio clock controller driver.

[PATCH v2 0/4] Add resets for ADSP based audio clock controller driver.

[Srinivasa Rao Mandadapu]

Add resets and remove qdsp6ss clcok controller for audioreach based platforms. 

Chnges since v1:
    -- Update commit message.
    -- Remove qdsp6ss clock control.

Srinivasa Rao Mandadapu (4):
  dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss reg property
  dt-bindings: clock: qcom,sc7280-lpasscc: Add resets for audioreach
  clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock control
  clk: qcom: lpasscc-sc7280: Add resets for audioreach

 .../bindings/clock/qcom,sc7280-lpasscc.yaml        | 14 +++--
 drivers/clk/qcom/lpasscc-sc7280.c                  | 66 +++++-----------------
 2 files changed, 22 insertions(+), 58 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss reg property

[Srinivasa Rao Mandadapu]

The qdsp6ss memory region is being shared by ADSP remoteproc device and
lpasscc clock device, hence causing memory conflict.
As the qdsp6ss clocks are being enabled in remoteproc driver, remove
clock controlling in the clock driver.

Fixes: d15eb8012476 ("dt-bindings: clock: Add YAML schemas for LPASS clocks on SC7280")

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
index 6151fde..9c72b8e 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
@@ -33,12 +33,10 @@ properties:
 
   reg:
     items:
-      - description: LPASS qdsp6ss register
       - description: LPASS top-cc register
 
   reg-names:
     items:
-      - const: qdsp6ss
       - const: top_cc
 
 required:
@@ -54,10 +52,10 @@ examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-sc7280.h>
     #include <dt-bindings/clock/qcom,lpass-sc7280.h>
-    clock-controller@3000000 {
+    clock-controller@3c04000 {
       compatible = "qcom,sc7280-lpasscc";
-      reg = <0x03000000 0x40>, <0x03c04000 0x4>;
-      reg-names = "qdsp6ss", "top_cc";
+      reg = <0x03c04000 0x4>;
+      reg-names = "top_cc";
       clocks = <&gcc GCC_CFG_NOC_LPASS_CLK>;
       clock-names = "iface";
       #clock-cells = <1>;
-- 
2.7.4

Re: [PATCH v2 1/4] dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss reg property

[Krzysztof Kozlowski]

On 30/12/2022 14:43, Srinivasa Rao Mandadapu wrote:
> The qdsp6ss memory region is being shared by ADSP remoteproc device and
> lpasscc clock device, hence causing memory conflict.
> As the qdsp6ss clocks are being enabled in remoteproc driver, remove
> clock controlling in the clock driver.
> 

That's an ABI break change. You cannot just drop it because the driver
will fail now to probe with old DTS.

This ABI break (and Fixes tag below) requires extensive justification
where is the bug and how it affects users. Otherwise, I see no reason
for ABI changes.

> Fixes: d15eb8012476 ("dt-bindings: clock: Add YAML schemas for LPASS clocks on SC7280")
> 

Drop blank line.

> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
> index 6151fde..9c72b8e 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
> @@ -33,12 +33,10 @@ properties:
>  
>    reg:
>      items:
> -      - description: LPASS qdsp6ss register
>        - description: LPASS top-cc register
>  
Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss reg property

[Krzysztof Kozlowski]

On 03/01/2023 06:51, Srinivasa Rao Mandadapu wrote:
> 
> On 1/2/2023 9:33 PM, Krzysztof Kozlowski wrote:
> Thanks for Your time Krzyszto!!!
>> On 30/12/2022 14:43, Srinivasa Rao Mandadapu wrote:
>>> The qdsp6ss memory region is being shared by ADSP remoteproc device and
>>> lpasscc clock device, hence causing memory conflict.
>>> As the qdsp6ss clocks are being enabled in remoteproc driver, remove
>>> clock controlling in the clock driver.
>>>
>> That's an ABI break change. You cannot just drop it because the driver
>> will fail now to probe with old DTS.
>>
>> This ABI break (and Fixes tag below) requires extensive justification
>> where is the bug and how it affects users. Otherwise, I see no reason
>> for ABI changes.
> 
> As ADSP path is not used so far due to various vendor requirements, we 
> haven't seen this conflict till now.

The device is used in sc7280.dtsi (and it is not disabled), thus it is
available in every board.

> 
> Actually, in the below commit,  clock handling in remoteproc driver has 
> already up-streamed by Bjorn.
> 
> If we add PIL device node and use remoteproc driver, qcom_q6v5_adsp.c, 
> it's mandatory to remove it from clock
> 
> driver.
> 
> Also the memory region is used by clock driver is part of 
> LPASS_QDSP6V67SS register region.
> 
> Here is the commit 0c6de4c299cc8f3283c38f3778777e00ed7e4b69 
> ("remoteproc: qcom: qdsp6-adsp: Add support for QCS404 CDSP")

The commit introduces qcom,qcs404-cdsp-pil which is not used in upstream
DTS...

Anyway none of the reasons above explain to me why ABI has to be broken.
To remind - valid reasons are usually only: "it never worked" or "I was
introduced recently so there are no users".

> For ABI Break resolution shall I include, device tree changes also in 
> this series? Or Could you please suggest better approach?

Lack of DTS changes causes upstream to be broken. This is independent of
ABI break. ABI means supporting users of the binding, so firmware, other
OS, out-of-tree DTS users etc. All of them are broken.

You need to keep backwards compatibility.
Best regards,
Krzysztof

[PATCH v2 2/4] dt-bindings: clock: qcom,sc7280-lpasscc: Add resets for audioreach

[Srinivasa Rao Mandadapu]

Add support for LPASS audio clock gating for RX/TX/SWA core bus clocks
for audioreach based SC7280 platforms.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 .../devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml         | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
index 9c72b8e..40fc6ab 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
@@ -31,13 +31,18 @@ properties:
   '#clock-cells':
     const: 1
 
+  '#reset-cells':
+    const: 1
+
   reg:
     items:
       - description: LPASS top-cc register
+      - description: LPASS reset-cgcr register
 
   reg-names:
     items:
       - const: top_cc
+      - const: reset_cgcr
 
 required:
   - compatible
@@ -54,10 +59,11 @@ examples:
     #include <dt-bindings/clock/qcom,lpass-sc7280.h>
     clock-controller@3c04000 {
       compatible = "qcom,sc7280-lpasscc";
-      reg = <0x03c04000 0x4>;
-      reg-names = "top_cc";
+      reg = <0x03c04000 0x4>, <0x032a9000 0x1000>;
+      reg-names = "top_cc", "reset_cgcr";
       clocks = <&gcc GCC_CFG_NOC_LPASS_CLK>;
       clock-names = "iface";
       #clock-cells = <1>;
+      #reset-cells = <1>;
     };
 ...
-- 
2.7.4

Re: [PATCH v2 2/4] dt-bindings: clock: qcom,sc7280-lpasscc: Add resets for audioreach

[Krzysztof Kozlowski]

On 30/12/2022 14:43, Srinivasa Rao Mandadapu wrote:
> Add support for LPASS audio clock gating for RX/TX/SWA core bus clocks
> for audioreach based SC7280 platforms.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> ---
>  .../devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml         | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
> index 9c72b8e..40fc6ab 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml
> @@ -31,13 +31,18 @@ properties:
>    '#clock-cells':
>      const: 1
>  
> +  '#reset-cells':
> +    const: 1
> +
>    reg:
>      items:
>        - description: LPASS top-cc register
> +      - description: LPASS reset-cgcr register

It's now even a bigger mess. First ABI break, then bring it back to
previous stage - so two IO addresses - but with different values! There
is no way this works with other systems or DTS users.

>  
>    reg-names:
>      items:
>        - const: top_cc
> +      - const: reset_cgcr


The 'reg-names' is only a helper, order and contents of 'reg' is fixed.

Best regards,
Krzysztof

[PATCH v2 3/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock control

[Srinivasa Rao Mandadapu]

The qdsp6ss memory region is being shared by ADSP remoteproc device and
lpasscc clock device, hence causing memory conflict.
As the qdsp6ss clocks are being enabled in remoteproc driver,
remove clock controlling in the clock driver.

Fixes: 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280")

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 drivers/clk/qcom/lpasscc-sc7280.c | 63 +--------------------------------------
 1 file changed, 1 insertion(+), 62 deletions(-)

diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
index 5c1e17b..87e1c21 100644
--- a/drivers/clk/qcom/lpasscc-sc7280.c
+++ b/drivers/clk/qcom/lpasscc-sc7280.c
@@ -30,48 +30,6 @@ static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = {
 	},
 };
 
-static struct clk_branch lpass_qdsp6ss_core_clk = {
-	.halt_reg = 0x20,
-	/* CLK_OFF would not toggle until LPASS is out of reset */
-	.halt_check = BRANCH_HALT_SKIP,
-	.clkr = {
-		.enable_reg = 0x20,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "lpass_qdsp6ss_core_clk",
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
-static struct clk_branch lpass_qdsp6ss_xo_clk = {
-	.halt_reg = 0x38,
-	/* CLK_OFF would not toggle until LPASS is out of reset */
-	.halt_check = BRANCH_HALT_SKIP,
-	.clkr = {
-		.enable_reg = 0x38,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "lpass_qdsp6ss_xo_clk",
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
-static struct clk_branch lpass_qdsp6ss_sleep_clk = {
-	.halt_reg = 0x3c,
-	/* CLK_OFF would not toggle until LPASS is out of reset */
-	.halt_check = BRANCH_HALT_SKIP,
-	.clkr = {
-		.enable_reg = 0x3c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "lpass_qdsp6ss_sleep_clk",
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct regmap_config lpass_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
@@ -90,18 +48,6 @@ static const struct qcom_cc_desc lpass_cc_top_sc7280_desc = {
 	.num_clks = ARRAY_SIZE(lpass_cc_top_sc7280_clocks),
 };
 
-static struct clk_regmap *lpass_qdsp6ss_sc7280_clocks[] = {
-	[LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr,
-	[LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr,
-	[LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr,
-};
-
-static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
-	.config = &lpass_regmap_config,
-	.clks = lpass_qdsp6ss_sc7280_clocks,
-	.num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
-};
-
 static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
@@ -118,17 +64,10 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 		goto destroy_pm_clk;
 	}
 
-	lpass_regmap_config.name = "qdsp6ss";
-	desc = &lpass_qdsp6ss_sc7280_desc;
-
-	ret = qcom_cc_probe_by_index(pdev, 0, desc);
-	if (ret)
-		goto destroy_pm_clk;
-
 	lpass_regmap_config.name = "top_cc";
 	desc = &lpass_cc_top_sc7280_desc;
 
-	ret = qcom_cc_probe_by_index(pdev, 1, desc);
+	ret = qcom_cc_probe_by_index(pdev, 0, desc);
 	if (ret)
 		goto destroy_pm_clk;
 
-- 
2.7.4

Re: [PATCH v2 3/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock control

[Krzysztof Kozlowski]

On 30/12/2022 14:43, Srinivasa Rao Mandadapu wrote:
> The qdsp6ss memory region is being shared by ADSP remoteproc device and
> lpasscc clock device, hence causing memory conflict.
> As the qdsp6ss clocks are being enabled in remoteproc driver,
> remove clock controlling in the clock driver.
> 
> Fixes: 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280")
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> ---
>  drivers/clk/qcom/lpasscc-sc7280.c | 63 +--------------------------------------
>  1 file changed, 1 insertion(+), 62 deletions(-)
> 
> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
> index 5c1e17b..87e1c21 100644
> --- a/drivers/clk/qcom/lpasscc-sc7280.c
> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
> @@ -30,48 +30,6 @@ static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = {
>  	},
>  };
>  
> -static struct clk_branch lpass_qdsp6ss_core_clk = {
> -	.halt_reg = 0x20,
> -	/* CLK_OFF would not toggle until LPASS is out of reset */
> -	.halt_check = BRANCH_HALT_SKIP,
> -	.clkr = {
> -		.enable_reg = 0x20,
> -		.enable_mask = BIT(0),
> -		.hw.init = &(struct clk_init_data){
> -			.name = "lpass_qdsp6ss_core_clk",
> -			.ops = &clk_branch2_ops,
> -		},
> -	},
> -};
> -
> -static struct clk_branch lpass_qdsp6ss_xo_clk = {
> -	.halt_reg = 0x38,
> -	/* CLK_OFF would not toggle until LPASS is out of reset */
> -	.halt_check = BRANCH_HALT_SKIP,
> -	.clkr = {
> -		.enable_reg = 0x38,
> -		.enable_mask = BIT(0),
> -		.hw.init = &(struct clk_init_data){
> -			.name = "lpass_qdsp6ss_xo_clk",
> -			.ops = &clk_branch2_ops,
> -		},
> -	},
> -};
> -
> -static struct clk_branch lpass_qdsp6ss_sleep_clk = {
> -	.halt_reg = 0x3c,
> -	/* CLK_OFF would not toggle until LPASS is out of reset */
> -	.halt_check = BRANCH_HALT_SKIP,
> -	.clkr = {
> -		.enable_reg = 0x3c,
> -		.enable_mask = BIT(0),
> -		.hw.init = &(struct clk_init_data){
> -			.name = "lpass_qdsp6ss_sleep_clk",
> -			.ops = &clk_branch2_ops,
> -		},
> -	},
> -};
> -
>  static struct regmap_config lpass_regmap_config = {
>  	.reg_bits	= 32,
>  	.reg_stride	= 4,
> @@ -90,18 +48,6 @@ static const struct qcom_cc_desc lpass_cc_top_sc7280_desc = {
>  	.num_clks = ARRAY_SIZE(lpass_cc_top_sc7280_clocks),
>  };
>  
> -static struct clk_regmap *lpass_qdsp6ss_sc7280_clocks[] = {
> -	[LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr,
> -	[LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr,
> -	[LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr,
> -};
> -
> -static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
> -	.config = &lpass_regmap_config,
> -	.clks = lpass_qdsp6ss_sc7280_clocks,
> -	.num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
> -};
> -
>  static int lpass_cc_sc7280_probe(struct platform_device *pdev)
>  {
>  	const struct qcom_cc_desc *desc;
> @@ -118,17 +64,10 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
>  		goto destroy_pm_clk;
>  	}
>  
> -	lpass_regmap_config.name = "qdsp6ss";
> -	desc = &lpass_qdsp6ss_sc7280_desc;
> -
> -	ret = qcom_cc_probe_by_index(pdev, 0, desc);
> -	if (ret)
> -		goto destroy_pm_clk;
> -
>  	lpass_regmap_config.name = "top_cc";
>  	desc = &lpass_cc_top_sc7280_desc;
>  
> -	ret = qcom_cc_probe_by_index(pdev, 1, desc);
> +	ret = qcom_cc_probe_by_index(pdev, 0, desc);

ABI break but actually does not work with upstream either. DTS at this
point has here on index "0" different address and you probe now top_cc
on it.

Best regards,
Krzysztof

[PATCH v2 4/4] clk: qcom: lpasscc-sc7280: Add resets for audioreach

[Srinivasa Rao Mandadapu]

The clock gating control for TX/RX/WSA core bus clocks would be required
to be reset(moved from hardware control) from audio core driver. Thus
add the support for the reset clocks in audioreach based clock driver.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 drivers/clk/qcom/lpasscc-sc7280.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
index 87e1c21..a7f4ed4 100644
--- a/drivers/clk/qcom/lpasscc-sc7280.c
+++ b/drivers/clk/qcom/lpasscc-sc7280.c
@@ -12,10 +12,12 @@
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,lpass-sc7280.h>
+#include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
 
 #include "clk-regmap.h"
 #include "clk-branch.h"
 #include "common.h"
+#include "reset.h"
 
 static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = {
 	.halt_reg = 0x0,
@@ -48,6 +50,18 @@ static const struct qcom_cc_desc lpass_cc_top_sc7280_desc = {
 	.num_clks = ARRAY_SIZE(lpass_cc_top_sc7280_clocks),
 };
 
+static const struct qcom_reset_map lpass_cc_sc7280_resets[] = {
+	[LPASS_AUDIO_SWR_RX_CGCR] =  { 0xa0, 1 },
+	[LPASS_AUDIO_SWR_TX_CGCR] =  { 0xa8, 1 },
+	[LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },
+};
+
+static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = {
+	.config = &lpass_regmap_config,
+	.resets = lpass_cc_sc7280_resets,
+	.num_resets = ARRAY_SIZE(lpass_cc_sc7280_resets),
+};
+
 static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
@@ -71,6 +85,13 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 	if (ret)
 		goto destroy_pm_clk;
 
+	lpass_regmap_config.name = "reset_cgcr";
+	desc = &lpass_audio_cc_reset_sc7280_desc;
+
+	ret = qcom_cc_probe_by_index(pdev, 1, desc);
+	if (ret)
+		goto destroy_pm_clk;
+
 	return 0;
 
 destroy_pm_clk:
-- 
2.7.4