[PATCH v1 0/4] Remove the qdsp6ss register from lpasscc

[PATCH v1 0/4] Remove the qdsp6ss register from lpasscc

[Mohammad Rafi Shaik]

This patch set is to remove the qdsp6ss register from lpasscc to
resolve memory conflict's between lpascc and ADSP remoteproc driver.

Mohammad Rafi Shaik (4):
  arm64: dts: qcom: sc7280: Modify lpasscc node name
  dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss register
    region
  arm64: dts: qcom: sc7280: Remove qdsp6ss regmap region
  clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

 .../bindings/clock/qcom,sc7280-lpasscc.yaml   |  8 +--
 arch/arm64/boot/dts/qcom/sc7280.dtsi          |  7 +--
 drivers/clk/qcom/lpasscc-sc7280.c             | 63 +------------------
 3 files changed, 7 insertions(+), 71 deletions(-)

-- 
2.25.1

[PATCH v1 1/4] arm64: dts: qcom: sc7280: Modify lpasscc node name

[Mohammad Rafi Shaik]

Modify lpasscc clock controller node name to generic name,
that is from lpasscc to clock-controller.

Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index bdcb74925313..3914f262aa12 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2230,7 +2230,7 @@ tcsr_2: syscon@1fc0000 {
 			reg = <0 0x01fc0000 0 0x30000>;
 		};
 
-		lpasscc: lpasscc@3000000 {
+		lpasscc: clock-controller@3000000 {
 			compatible = "qcom,sc7280-lpasscc";
 			reg = <0 0x03000000 0 0x40>,
 			      <0 0x03c04000 0 0x4>;
-- 
2.25.1

[PATCH v1 2/4] dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss register region

[Mohammad Rafi Shaik]

The qdsp6ss memory region 0x3000000 is being shared by ADSP remoteproc
device and lpasscc clock device, hence causing memory conflict and 
as the qdsp6ss clocks are being enabled in remoteproc driver,
remove the qdsp6ss register from lpasscc.

Changing the base address of lpasscc based on the first register memory.

Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 .../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 6151fdebbff8..9c72b8eca450 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.25.1

Re: [PATCH v1 2/4] dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss register region

[Krzysztof Kozlowski]

On 27/03/2023 18:32, Mohammad Rafi Shaik wrote:
> The qdsp6ss memory region 0x3000000 is being shared by ADSP remoteproc
> device and lpasscc clock device, hence causing memory conflict and 
> as the qdsp6ss clocks are being enabled in remoteproc driver,
> remove the qdsp6ss register from lpasscc.
> 
> Changing the base address of lpasscc based on the first register memory.
> 
> Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>

What this patch is not saying is that several clocks are going to
disappear as well. It clearly should cause some impact on users of the
binding and driver however commit msg lacks explanation here.

Best regards,
Krzysztof

[PATCH v1 3/4] arm64: dts: qcom: sc7280: Remove qdsp6ss regmap region

[Mohammad Rafi Shaik]

The qdsp6ss memory region is being shared by ADSP remoteproc device and
lpasscc clock device, hence causing memory conflict and as the qdsp6ss
clocks are being enabled in remoteproc driver, remove the register memory
region from lpasscc device tree node.

Change the base address of lpasscc with top_cc register address.

Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 3914f262aa12..625ac36790a0 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2230,11 +2230,10 @@ tcsr_2: syscon@1fc0000 {
 			reg = <0 0x01fc0000 0 0x30000>;
 		};
 
-		lpasscc: clock-controller@3000000 {
+		lpasscc: clock-controller@3c04000 {
 			compatible = "qcom,sc7280-lpasscc";
-			reg = <0 0x03000000 0 0x40>,
-			      <0 0x03c04000 0 0x4>;
-			reg-names = "qdsp6ss", "top_cc";
+			reg = <0 0x03c04000 0 0x4>;
+			reg-names = "top_cc";
 			clocks = <&gcc GCC_CFG_NOC_LPASS_CLK>;
 			clock-names = "iface";
 			#clock-cells = <1>;
-- 
2.25.1

[PATCH v1 4/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

[Mohammad Rafi Shaik]

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 the
qdsp6ss clock registration.

Fixes: 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280")
Signed-off-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 48432010ce24..4719e3fa8b05 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;
@@ -121,17 +67,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.25.1

Re: [PATCH v1 4/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

[Stephen Boyd]

Quoting Mohammad Rafi Shaik (2023-03-27 09:32:49)
> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
> index 48432010ce24..4719e3fa8b05 100644
> --- a/drivers/clk/qcom/lpasscc-sc7280.c
> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
> @@ -121,17 +67,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);

Instead of changing the binding, it may be better to leave it as is and
ignore the first reg property in the driver. Then you don't need any DTS
patch or binding patch. You can just have this one patch. After that you
can introduce a new compatible string for the proper design and make it
have only a single reg property and deprecate the old binding. The
driver can then pick index 0 if the new compatible is present.

Re: [PATCH v1 4/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

[Mohammad Rafi Shaik]

On 3/29/2023 8:41 AM, Stephen Boyd wrote:
> Quoting Mohammad Rafi Shaik (2023-03-27 09:32:49)
>> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
>> index 48432010ce24..4719e3fa8b05 100644
>> --- a/drivers/clk/qcom/lpasscc-sc7280.c
>> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
>> @@ -121,17 +67,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);
> Instead of changing the binding, it may be better to leave it as is and
> ignore the first reg property in the driver. Then you don't need any DTS
> patch or binding patch. You can just have this one patch. After that you
> can introduce a new compatible string for the proper design and make it
> have only a single reg property and deprecate the old binding. The
> driver can then pick index 0 if the new compatible is present.

Thanks for comment,

The main issue with sc7280.dtsi file.

Required to upstream remoteproc_adsp node for audioreach adsp based 
solution.
The base address for remoteproc_adsp dts node is 0x3000000.

Please refer below link audioreach dts patch:
https://patchwork.kernel.org/project/linux-arm-msm/patch/1675700201-12890-4-git-send-email-quic_srivasam@quicinc.com/

remoteproc_adsp: remoteproc@3000000 {
             compatible = "qcom,sc7280-adsp-pil";
             reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>;
             reg-names = "qdsp6ss_base", "lpass_efuse";

and in sc7280.dtsi lpasscc node base address also same.

lpasscc: lpasscc@3000000 {
             compatible = "qcom,sc7280-lpasscc";
             reg = <0 0x03000000 0 0x40>,
                       <0 0x03c04000 0 0x4>,

In single dtsi file should not have same physical address node.
Required to sort the nodes based on physical address.

As the qdsp6ss clocks are being enabled in remoteproc driver,
removing the qdsp6ss reg region from lpasscc in sc7280.dtsi.

Re: [PATCH v1 4/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

[Stephen Boyd]

Quoting Mohammad Rafi Shaik (2023-03-29 02:24:43)
> 
> The main issue with sc7280.dtsi file.
> 
> Required to upstream remoteproc_adsp node for audioreach adsp based 
> solution.
> The base address for remoteproc_adsp dts node is 0x3000000.
> 
> Please refer below link audioreach dts patch:
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1675700201-12890-4-git-send-email-quic_srivasam@quicinc.com/
> 
> remoteproc_adsp: remoteproc@3000000 {
>              compatible = "qcom,sc7280-adsp-pil";
>              reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>;
>              reg-names = "qdsp6ss_base", "lpass_efuse";
> 
> and in sc7280.dtsi lpasscc node base address also same.
> 
> lpasscc: lpasscc@3000000 {
>              compatible = "qcom,sc7280-lpasscc";
>              reg = <0 0x03000000 0 0x40>,
>                        <0 0x03c04000 0 0x4>,
> 
> In single dtsi file should not have same physical address node.
> Required to sort the nodes based on physical address.

Yes the same address shouldn't be used twice, but it still compiles,
right? The node name is different, remoteproc vs. clock-controller, so
it should work for the interim while the qcom,sc7280-lpasscc-2 binding
is written that only has one reg property.

I'm suggesting you don't change the existing binding. Instead, deprecate
the compatible and add a new compatible for the binding that omits the
second reg property. Then the driver patch can work with old and new dts
files.

Re: [PATCH v1 4/4] clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

[Krzysztof Kozlowski]

On 27/03/2023 18:32, Mohammad Rafi Shaik 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 the
> qdsp6ss clock registration.
> 
> Fixes: 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280")

I don't understand why this is a fix. The clocks were working before,
right? So removing them is both ABI break and not a fix.

More over, this *cannot* be backported because it will break users, thus
Fixes tag is here misleading stable folks.

Best regards,
Krzysztof

Re: [PATCH v1 0/4] Remove the qdsp6ss register from lpasscc

[Stephen Boyd]

Quoting Mohammad Rafi Shaik (2023-03-27 09:32:45)
> This patch set is to remove the qdsp6ss register from lpasscc to
> resolve memory conflict's between lpascc and ADSP remoteproc driver.

Is this related to the other patch series[1] ("[PATCH v9 0/4] Add resets
for ADSP based audio clock controller driver")? Does it supersede those?

> 
> Mohammad Rafi Shaik (4):
>   arm64: dts: qcom: sc7280: Modify lpasscc node name
>   dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss register
>     region
>   arm64: dts: qcom: sc7280: Remove qdsp6ss regmap region
>   clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration

[1] https://lore.kernel.org/all/20230317141622.1926573-1-quic_mohs@quicinc.com/

Re: [PATCH v1 0/4] Remove the qdsp6ss register from lpasscc

[Mohammad Rafi Shaik]

On 3/27/2023 11:11 PM, Stephen Boyd wrote:
> Quoting Mohammad Rafi Shaik (2023-03-27 09:32:45)
>> This patch set is to remove the qdsp6ss register from lpasscc to
>> resolve memory conflict's between lpascc and ADSP remoteproc driver.
> Is this related to the other patch series[1] ("[PATCH v9 0/4] Add resets
> for ADSP based audio clock controller driver")? Does it supersede those?
Thanks for comment,

yes, its superseded form patch series[1] ("[PATCH v9 0/4] Add resets
for ADSP based audio clock controller driver") which is required many
changes.

As the qdsp6ss clocks are being enabled in remoteproc driver,
the qdsp6ss not required in lpasscc node.

For audioreach solution required to create the remoteproc_adsp
device tree node with base address 0x3000000 for remoteproc driver,
as already this address being used in lpasscc node it's causing memory
conflict.
>> Mohammad Rafi Shaik (4):
>>    arm64: dts: qcom: sc7280: Modify lpasscc node name
>>    dt-bindings: clock: qcom,sc7280-lpasscc: Remove qdsp6ss register
>>      region
>>    arm64: dts: qcom: sc7280: Remove qdsp6ss regmap region
>>    clk: qcom: lpasscc-sc7280: Remove qdsp6ss clock registration
> [1] https://lore.kernel.org/all/20230317141622.1926573-1-quic_mohs@quicinc.com/

Re: [PATCH v1 0/4] Remove the qdsp6ss register from lpasscc

[Stephen Boyd]

Quoting Mohammad Rafi Shaik (2023-03-27 23:02:38)
> 
> On 3/27/2023 11:11 PM, Stephen Boyd wrote:
> > Quoting Mohammad Rafi Shaik (2023-03-27 09:32:45)
> >> This patch set is to remove the qdsp6ss register from lpasscc to
> >> resolve memory conflict's between lpascc and ADSP remoteproc driver.
> > Is this related to the other patch series[1] ("[PATCH v9 0/4] Add resets
> > for ADSP based audio clock controller driver")? Does it supersede those?
> Thanks for comment,
> 
> yes, its superseded form patch series[1] ("[PATCH v9 0/4] Add resets
> for ADSP based audio clock controller driver") which is required many
> changes.
> 
> As the qdsp6ss clocks are being enabled in remoteproc driver,
> the qdsp6ss not required in lpasscc node.
> 
> For audioreach solution required to create the remoteproc_adsp
> device tree node with base address 0x3000000 for remoteproc driver,
> as already this address being used in lpasscc node it's causing memory
> conflict.

Ok. Please add the details of superseded patch series to the cover
letter. It helps us understand what to do with the other patches on the
list.