[v5,3/8] arm64: dts: qcom: sc7280: Add LPASS PIL node

Message ID 1675700201-12890-4-git-send-email-quic_srivasam@quicinc.com
State New
Headers
Series Add SC7280 audioreach device tree nodes |

Commit Message

Srinivasa Rao Mandadapu Feb. 6, 2023, 4:16 p.m. UTC
  Add LPASS PIL node for sc7280 based audioreach platforms.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 .../qcom/sc7280-herobrine-audioreach-wcd9385.dtsi  |  4 +
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 93 ++++++++++++++++++++++
 2 files changed, 97 insertions(+)
  

Comments

Stephen Boyd Feb. 9, 2023, 10:55 p.m. UTC | #1
Quoting Srinivasa Rao Mandadapu (2023-02-06 08:16:36)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
> index 1810a36..5e99f49 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
> @@ -107,3 +107,7 @@
>                 };
>         };
>  };
> +
> +&remoteproc_adsp {
> +       status = "okay";
> +};

Sort this file by phandle alphabetically?

> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 6908bca..27ab992 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3439,6 +3441,97 @@
>                         status = "disabled";
>                 };
>
> +               remoteproc_adsp: remoteproc@3000000 {

This should be sorted on physical address. I think the node above is
spi@88dc000 so this is in the wrong place.

> +                       compatible = "qcom,sc7280-adsp-pil";
> +                       reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>;
> +                       reg-names = "qdsp6ss_base", "lpass_efuse";
> +
> +                       interrupts-extended = <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
> +                                             <&adsp_smp2p_in 0 IRQ_TYPE_NONE>,

Can these have proper irq flags? Doubtful they're IRQ_TYPE_NONE.

> +                                             <&adsp_smp2p_in 1 IRQ_TYPE_NONE>,
> +                                             <&adsp_smp2p_in 2 IRQ_TYPE_NONE>,
> +                                             <&adsp_smp2p_in 3 IRQ_TYPE_NONE>,
> +                                             <&adsp_smp2p_in 7 IRQ_TYPE_NONE>;
> +
> +                       interrupt-names = "wdog", "fatal", "ready",
> +                                         "handover", "stop-ack",
> +                                         "shutdown-ack";
> +
> +                       qcom,qmp = <&aoss_qmp>;
> +
> +                       clocks = <&rpmhcc RPMH_CXO_CLK>,
> +                                <&gcc GCC_CFG_NOC_LPASS_CLK>;
> +

Drop newline so clocks properties are together please.

> +                       clock-names = "xo", "gcc_cfg_noc_lpass";
> +
> +                       iommus = <&apps_smmu 0x1800 0x0>;
> +
> +                       power-domains = <&rpmhpd SC7280_CX>;
> +                       power-domain-names = "cx";
> +
> +                       required-opps = <&rpmhpd_opp_nom>;
> +
> +                       resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
> +                                <&aoss_reset AOSS_CC_LPASS_RESTART>;
> +

Drop newline so reset properties are together please.

> +                       reset-names =  "pdc_sync", "cc_lpass";
> +                       qcom,halt-regs = <&tcsr_1 0x3000 0x5000 0x8000 0x13000>;
> +
> +                       memory-region = <&adsp_mem>;
> +
> +                       qcom,smem-states = <&adsp_smp2p_out 0>;
> +                       qcom,smem-state-names = "stop";
> +
> +                       status = "disabled";
> +
> +                       glink-edge {
> +                               interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
> +                                                      IPCC_MPROC_SIGNAL_GLINK_QMP
> +                                                      IRQ_TYPE_EDGE_RISING>;
> +
> +                               mboxes = <&ipcc IPCC_CLIENT_LPASS
> +                                        IPCC_MPROC_SIGNAL_GLINK_QMP>;
> +
> +                               label = "lpass";
> +                               qcom,remote-pid = <2>;
> +
> +                               gpr {

This node name should be apr per the qcom,glink-edge.yaml binding?

> +                                       compatible = "qcom,gpr";
> +                                       qcom,glink-channels = "adsp_apps";
> +                                       qcom,domain = <GPR_DOMAIN_ID_ADSP>;
> +                                       qcom,intents = <512 20>;
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       q6apm: service@1 {
> +                                               compatible = "qcom,q6apm";
> +                                               reg = <GPR_APM_MODULE_IID>;
> +                                               #sound-dai-cells = <0>;
> +
> +                                               q6apmdai: dais {
> +                                                       compatible = "qcom,q6apm-dais";
> +                                                       iommus = <&apps_smmu 0x1801 0x0>;
> +                                               };
> +
> +                                               q6apmbedai: bedais {
> +                                                       compatible = "qcom,q6apm-lpass-dais";
> +                                                       #sound-dai-cells = <1>;
> +                                               };
> +                                       };
> +
> +                                       q6prm: service@2 {
> +                                               compatible = "qcom,q6prm";
> +                                               reg = <GPR_PRM_MODULE_IID>;
> +
> +                                               q6prmcc: clock-controller {
> +                                                       compatible = "qcom,q6prm-lpass-clocks";

This is clk binding but not a clk driver? I'll look away now.

> +                                                       #clock-cells = <2>;
> +                                               };
> +                                       };
> +                               };
> +                       };
> +               };
> +
  
Krzysztof Kozlowski Feb. 10, 2023, 11:12 a.m. UTC | #2
On 06/02/2023 17:16, Srinivasa Rao Mandadapu wrote:
> Add LPASS PIL node for sc7280 based audioreach platforms.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> ---
>  .../qcom/sc7280-herobrine-audioreach-wcd9385.dtsi  |  4 +
>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 93 ++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
> index 1810a36..5e99f49 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
> @@ -107,3 +107,7 @@
>  		};
>  	};
>  };
> +
> +&remoteproc_adsp {

Are you sure this is ordered by name?

> +	status = "okay";
> +};

There is lpasscc@3000000 so aren't you now enabling two nodes for the
same address?

> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 6908bca..27ab992 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -8,6 +8,7 @@
>  #include <dt-bindings/clock/qcom,dispcc-sc7280.h>
>  #include <dt-bindings/clock/qcom,gcc-sc7280.h>
>  #include <dt-bindings/clock/qcom,gpucc-sc7280.h>
> +#include <dt-bindings/clock/qcom,lpass-sc7280.h>
>  #include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
>  #include <dt-bindings/clock/qcom,lpasscorecc-sc7280.h>
>  #include <dt-bindings/clock/qcom,rpmh.h>
> @@ -21,6 +22,7 @@
>  #include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/reset/qcom,sdm845-aoss.h>
>  #include <dt-bindings/reset/qcom,sdm845-pdc.h>
> +#include <dt-bindings/soc/qcom,gpr.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>  #include <dt-bindings/sound/qcom,lpass.h>
>  #include <dt-bindings/thermal/thermal.h>
> @@ -3439,6 +3441,97 @@
>  			status = "disabled";
>  		};
>  
> +		remoteproc_adsp: remoteproc@3000000 {
> +			compatible = "qcom,sc7280-adsp-pil";
> +			reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>;
> +			reg-names = "qdsp6ss_base", "lpass_efuse";

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

You can test against specific schema with DT_SCHEMA_FILES

> +
> +			interrupts-extended = <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&adsp_smp2p_in 0 IRQ_TYPE_NONE>,
> +					      <&adsp_smp2p_in 1 IRQ_TYPE_NONE>,
> +					      <&adsp_smp2p_in 2 IRQ_TYPE_NONE>,
> +					      <&adsp_smp2p_in 3 IRQ_TYPE_NONE>,
> +					      <&adsp_smp2p_in 7 IRQ_TYPE_NONE>;
> +
> +			interrupt-names = "wdog", "fatal", "ready",
> +					  "handover", "stop-ack",
> +					  "shutdown-ack";
> +
> +			qcom,qmp = <&aoss_qmp>;
> +
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <&gcc GCC_CFG_NOC_LPASS_CLK>;
> +
> +			clock-names = "xo", "gcc_cfg_noc_lpass";
> +
> +			iommus = <&apps_smmu 0x1800 0x0>;
> +
> +			power-domains =	<&rpmhpd SC7280_CX>;
> +			power-domain-names = "cx";

Here as well. Binding says it is LCX domain.

> +
> +			required-opps = <&rpmhpd_opp_nom>;

This should also fail... please send code for review only if there are
no warnings.

> +
> +			resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
> +				 <&aoss_reset AOSS_CC_LPASS_RESTART>;
> +
> +			reset-names =  "pdc_sync", "cc_lpass";
> +			qcom,halt-regs = <&tcsr_1 0x3000 0x5000 0x8000 0x13000>;
> +
> +			memory-region = <&adsp_mem>;
> +
> +			qcom,smem-states = <&adsp_smp2p_out 0>;
> +			qcom,smem-state-names = "stop";
> +
> +			status = "disabled";
> +
> +			glink-edge {
> +				interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
> +						       IPCC_MPROC_SIGNAL_GLINK_QMP
> +						       IRQ_TYPE_EDGE_RISING>;
> +
> +				mboxes = <&ipcc IPCC_CLIENT_LPASS
> +					 IPCC_MPROC_SIGNAL_GLINK_QMP>;
> +
> +				label = "lpass";
> +				qcom,remote-pid = <2>;
> +
> +				gpr {
> +					compatible = "qcom,gpr";
> +					qcom,glink-channels = "adsp_apps";
> +					qcom,domain = <GPR_DOMAIN_ID_ADSP>;
> +					qcom,intents = <512 20>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					q6apm: service@1 {
> +						compatible = "qcom,q6apm";
> +						reg = <GPR_APM_MODULE_IID>;
> +						#sound-dai-cells = <0>;

That's also wrong and test would point the issue.


Best regards,
Krzysztof
  
Krzysztof Kozlowski Feb. 10, 2023, 11:26 a.m. UTC | #3
On 09/02/2023 23:55, Stephen Boyd wrote:
>> +
>> +                       glink-edge {
>> +                               interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
>> +                                                      IPCC_MPROC_SIGNAL_GLINK_QMP
>> +                                                      IRQ_TYPE_EDGE_RISING>;
>> +
>> +                               mboxes = <&ipcc IPCC_CLIENT_LPASS
>> +                                        IPCC_MPROC_SIGNAL_GLINK_QMP>;
>> +
>> +                               label = "lpass";
>> +                               qcom,remote-pid = <2>;
>> +
>> +                               gpr {
> 
> This node name should be apr per the qcom,glink-edge.yaml binding?

No, this is correct. I fixed the glink-edge binding last year.

> 
>> +                                       compatible = "qcom,gpr";
>> +                                       qcom,glink-channels = "adsp_apps";
>> +                                       qcom,domain = <GPR_DOMAIN_ID_ADSP>;
>> +                                       qcom,intents = <512 20>;
>> +                                       #address-cells = <1>;
>> +                                       #size-cells = <0>;
>> +
>> +                                       q6apm: service@1 {
>> +                                               compatible = "qcom,q6apm";
>> +                                               reg = <GPR_APM_MODULE_IID>;
>> +                                               #sound-dai-cells = <0>;
>> +
>> +                                               q6apmdai: dais {
>> +                                                       compatible = "qcom,q6apm-dais";
>> +                                                       iommus = <&apps_smmu 0x1801 0x0>;
>> +                                               };
>> +
>> +                                               q6apmbedai: bedais {
>> +                                                       compatible = "qcom,q6apm-lpass-dais";
>> +                                                       #sound-dai-cells = <1>;
>> +                                               };
>> +                                       };
>> +
>> +                                       q6prm: service@2 {
>> +                                               compatible = "qcom,q6prm";
>> +                                               reg = <GPR_PRM_MODULE_IID>;
>> +
>> +                                               q6prmcc: clock-controller {
>> +                                                       compatible = "qcom,q6prm-lpass-clocks";
> 
> This is clk binding but not a clk driver? I'll look away now.

It is a clock driver which was not put into clk. Maybe because it is
tightly tied to entire QDSP platform.

Best regards,
Krzysztof
  

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
index 1810a36..5e99f49 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
@@ -107,3 +107,7 @@ 
 		};
 	};
 };
+
+&remoteproc_adsp {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 6908bca..27ab992 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -8,6 +8,7 @@ 
 #include <dt-bindings/clock/qcom,dispcc-sc7280.h>
 #include <dt-bindings/clock/qcom,gcc-sc7280.h>
 #include <dt-bindings/clock/qcom,gpucc-sc7280.h>
+#include <dt-bindings/clock/qcom,lpass-sc7280.h>
 #include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
 #include <dt-bindings/clock/qcom,lpasscorecc-sc7280.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
@@ -21,6 +22,7 @@ 
 #include <dt-bindings/power/qcom-rpmpd.h>
 #include <dt-bindings/reset/qcom,sdm845-aoss.h>
 #include <dt-bindings/reset/qcom,sdm845-pdc.h>
+#include <dt-bindings/soc/qcom,gpr.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
 #include <dt-bindings/sound/qcom,lpass.h>
 #include <dt-bindings/thermal/thermal.h>
@@ -3439,6 +3441,97 @@ 
 			status = "disabled";
 		};
 
+		remoteproc_adsp: remoteproc@3000000 {
+			compatible = "qcom,sc7280-adsp-pil";
+			reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>;
+			reg-names = "qdsp6ss_base", "lpass_efuse";
+
+			interrupts-extended = <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
+					      <&adsp_smp2p_in 0 IRQ_TYPE_NONE>,
+					      <&adsp_smp2p_in 1 IRQ_TYPE_NONE>,
+					      <&adsp_smp2p_in 2 IRQ_TYPE_NONE>,
+					      <&adsp_smp2p_in 3 IRQ_TYPE_NONE>,
+					      <&adsp_smp2p_in 7 IRQ_TYPE_NONE>;
+
+			interrupt-names = "wdog", "fatal", "ready",
+					  "handover", "stop-ack",
+					  "shutdown-ack";
+
+			qcom,qmp = <&aoss_qmp>;
+
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_CFG_NOC_LPASS_CLK>;
+
+			clock-names = "xo", "gcc_cfg_noc_lpass";
+
+			iommus = <&apps_smmu 0x1800 0x0>;
+
+			power-domains =	<&rpmhpd SC7280_CX>;
+			power-domain-names = "cx";
+
+			required-opps = <&rpmhpd_opp_nom>;
+
+			resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
+				 <&aoss_reset AOSS_CC_LPASS_RESTART>;
+
+			reset-names =  "pdc_sync", "cc_lpass";
+			qcom,halt-regs = <&tcsr_1 0x3000 0x5000 0x8000 0x13000>;
+
+			memory-region = <&adsp_mem>;
+
+			qcom,smem-states = <&adsp_smp2p_out 0>;
+			qcom,smem-state-names = "stop";
+
+			status = "disabled";
+
+			glink-edge {
+				interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
+						       IPCC_MPROC_SIGNAL_GLINK_QMP
+						       IRQ_TYPE_EDGE_RISING>;
+
+				mboxes = <&ipcc IPCC_CLIENT_LPASS
+					 IPCC_MPROC_SIGNAL_GLINK_QMP>;
+
+				label = "lpass";
+				qcom,remote-pid = <2>;
+
+				gpr {
+					compatible = "qcom,gpr";
+					qcom,glink-channels = "adsp_apps";
+					qcom,domain = <GPR_DOMAIN_ID_ADSP>;
+					qcom,intents = <512 20>;
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					q6apm: service@1 {
+						compatible = "qcom,q6apm";
+						reg = <GPR_APM_MODULE_IID>;
+						#sound-dai-cells = <0>;
+
+						q6apmdai: dais {
+							compatible = "qcom,q6apm-dais";
+							iommus = <&apps_smmu 0x1801 0x0>;
+						};
+
+						q6apmbedai: bedais {
+							compatible = "qcom,q6apm-lpass-dais";
+							#sound-dai-cells = <1>;
+						};
+					};
+
+					q6prm: service@2 {
+						compatible = "qcom,q6prm";
+						reg = <GPR_PRM_MODULE_IID>;
+
+						q6prmcc: clock-controller {
+							compatible = "qcom,q6prm-lpass-clocks";
+							#clock-cells = <2>;
+						};
+					};
+				};
+			};
+		};
+
 		remoteproc_wpss: remoteproc@8a00000 {
 			compatible = "qcom,sc7280-wpss-pil";
 			reg = <0 0x08a00000 0 0x10000>;