From: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Add GPR(Generic Pack router) node along with
APM(Audio Process Manager) and PRM(Proxy resource
Manager) audio services.
A new qcs6490-audioreach.dtsi file has been added to
update AudioReach specific device tree configurations.
The existing audio nodes in sc7280.dtsi, which were designed
for the ADSP Bypass solution. The audio nodes now being updated
in qcs6490-audioreach.dtsi to support the ADSP-based AudioReach
architecture.
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Co-developed-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
.../boot/dts/qcom/qcs6490-audioreach.dtsi | 54 +++++++++++++++++++
arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
2 files changed, 55 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
diff --git a/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi b/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
new file mode 100644
index 000000000000..282938c042f7
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * qcs6490 device tree source for Audioreach Solution.
+ * This file will configure and manage nodes from sc7280.dtsi to
+ * support the AudioReach solution.
+ *
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <dt-bindings/clock/qcom,lpass-sc7280.h>
+#include <dt-bindings/soc/qcom,gpr.h>
+#include <dt-bindings/sound/qcom,q6afe.h>
+#include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
+
+&remoteproc_adsp_glink {
+ /delete-node/ apr;
+
+ 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>;
+ qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
+
+ 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>;
+ qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
+
+ q6prmcc: clock-controller {
+ compatible = "qcom,q6prm-lpass-clocks";
+ #clock-cells = <2>;
+ };
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 0dd6a5c91d10..18e959806a13 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3944,7 +3944,7 @@ remoteproc_adsp: remoteproc@3700000 {
status = "disabled";
- glink-edge {
+ remoteproc_adsp_glink: glink-edge {
interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
IPCC_MPROC_SIGNAL_GLINK_QMP
IRQ_TYPE_EDGE_RISING>;
--
2.34.1On Thu, Aug 21, 2025 at 10:19:06AM +0530, Prasad Kumpatla wrote:
> From: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Subject says "add gpr node", that sounds insignificant, but the patch
actually introduces the structure for how to model audioreach - and will
set a precedence that others will follow.
It must be clear from the commit message why this is a separate file, so
that others will understand, now and in the future.
>
> Add GPR(Generic Pack router) node along with
> APM(Audio Process Manager) and PRM(Proxy resource
> Manager) audio services.
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
says to start your commit message with a problem statement, that makes
the reviewer understand which problem you're trying to solve. "Adding
GPR node" is not the problem, that is part of the solution, it should
come last.
>
> A new qcs6490-audioreach.dtsi file has been added to
> update AudioReach specific device tree configurations.
"Has been added"? When?
> The existing audio nodes in sc7280.dtsi, which were designed
> for the ADSP Bypass solution.
Please complete this sentence.
> The audio nodes now being updated
> in qcs6490-audioreach.dtsi to support the ADSP-based AudioReach
> architecture.
No, you're not updating qcs6490-audioreach.dtsi, you're adding that
file.
Please start your commit message with a description of what exists
today and why that doesn't fit your need, explain why we need a separate
file to carry these things. Make it clear why the bypass solution should
be kept in sc7280.dtsi (isn't that design only used in
sc7280-herobrine?).
Also, is qcs6490 the only variant of this SoC that comes with
AudioReach, what about QCM6490 and SM7325 devices?
>
> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
> Co-developed-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> .../boot/dts/qcom/qcs6490-audioreach.dtsi | 54 +++++++++++++++++++
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> 2 files changed, 55 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi b/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
> new file mode 100644
> index 000000000000..282938c042f7
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * qcs6490 device tree source for Audioreach Solution.
That's pretty much what the file name says as well. It might make sense
to leave a comment here, but if so make it useful.
> + * This file will configure and manage nodes from sc7280.dtsi to
> + * support the AudioReach solution.
So far it's only adding things, not configuring and managing (which
isn't something DT does anyways).
Also "This file will" implies that in the future something will be added
here to deliver something. We don't communicate intent like this, and
once you add that thing you intend to add in the future this comment
won't be useful.
Something like this would be better:
"Common definitions for SC7280-based boards with AudioReach"
But I think that too can be derived from the file name. So, let's make
sure the commit message for the change that introduces the file has a
good explanation.
> + *
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
I think this would look better above the comment. But please use the
right copyright statement.
Regards,
Bjorn
> + */
> +
> +#include <dt-bindings/clock/qcom,lpass-sc7280.h>
> +#include <dt-bindings/soc/qcom,gpr.h>
> +#include <dt-bindings/sound/qcom,q6afe.h>
> +#include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
> +
> +&remoteproc_adsp_glink {
> + /delete-node/ apr;
> +
> + 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>;
> + qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
> +
> + 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>;
> + qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
> +
> + q6prmcc: clock-controller {
> + compatible = "qcom,q6prm-lpass-clocks";
> + #clock-cells = <2>;
> + };
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 0dd6a5c91d10..18e959806a13 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3944,7 +3944,7 @@ remoteproc_adsp: remoteproc@3700000 {
>
> status = "disabled";
>
> - glink-edge {
> + remoteproc_adsp_glink: glink-edge {
> interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
> IPCC_MPROC_SIGNAL_GLINK_QMP
> IRQ_TYPE_EDGE_RISING>;
> --
> 2.34.1
>
On 9/2/2025 8:13 PM, Bjorn Andersson wrote:
> On Thu, Aug 21, 2025 at 10:19:06AM +0530, Prasad Kumpatla wrote:
>> From: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
>
> Subject says "add gpr node", that sounds insignificant, but the patch
> actually introduces the structure for how to model audioreach - and will
> set a precedence that others will follow.
>
> It must be clear from the commit message why this is a separate file, so
> that others will understand, now and in the future.
>
>>
>> Add GPR(Generic Pack router) node along with
>> APM(Audio Process Manager) and PRM(Proxy resource
>> Manager) audio services.
>
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> says to start your commit message with a problem statement, that makes
> the reviewer understand which problem you're trying to solve. "Adding
> GPR node" is not the problem, that is part of the solution, it should
> come last.
>
>>
>> A new qcs6490-audioreach.dtsi file has been added to
>> update AudioReach specific device tree configurations.
>
> "Has been added"? When?
>
>> The existing audio nodes in sc7280.dtsi, which were designed
>> for the ADSP Bypass solution.
>
> Please complete this sentence.
>
>> The audio nodes now being updated
>> in qcs6490-audioreach.dtsi to support the ADSP-based AudioReach
>> architecture.
>
> No, you're not updating qcs6490-audioreach.dtsi, you're adding that
> file.
>
> Please start your commit message with a description of what exists
> today and why that doesn't fit your need, explain why we need a separate
> file to carry these things. Make it clear why the bypass solution should
> be kept in sc7280.dtsi (isn't that design only used in
> sc7280-herobrine?).
>
> Also, is qcs6490 the only variant of this SoC that comes with
> AudioReach, what about QCM6490 and SM7325 devices?
Sure, will update the commit text as per your recommendations.
Thanks,
Prasad
>
>>
>> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
>> Co-developed-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
>> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> ---
>> .../boot/dts/qcom/qcs6490-audioreach.dtsi | 54 +++++++++++++++++++
>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
>> 2 files changed, 55 insertions(+), 1 deletion(-)
>> create mode 100644 arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi b/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
>> new file mode 100644
>> index 000000000000..282938c042f7
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-audioreach.dtsi
>> @@ -0,0 +1,54 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * qcs6490 device tree source for Audioreach Solution.
>
> That's pretty much what the file name says as well. It might make sense
> to leave a comment here, but if so make it useful.
>
>> + * This file will configure and manage nodes from sc7280.dtsi to
>> + * support the AudioReach solution.
>
> So far it's only adding things, not configuring and managing (which
> isn't something DT does anyways).
>
> Also "This file will" implies that in the future something will be added
> here to deliver something. We don't communicate intent like this, and
> once you add that thing you intend to add in the future this comment
> won't be useful.
>
> Something like this would be better:
> "Common definitions for SC7280-based boards with AudioReach"
>
> But I think that too can be derived from the file name. So, let's make
> sure the commit message for the change that introduces the file has a
> good explanation.
>
>> + *
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>
> I think this would look better above the comment. But please use the
> right copyright statement.
>
> Regards,
> Bjorn
>
>> + */
>> +
>> +#include <dt-bindings/clock/qcom,lpass-sc7280.h>
>> +#include <dt-bindings/soc/qcom,gpr.h>
>> +#include <dt-bindings/sound/qcom,q6afe.h>
>> +#include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
>> +
>> +&remoteproc_adsp_glink {
>> + /delete-node/ apr;
>> +
>> + 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>;
>> + qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
>> +
>> + 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>;
>> + qcom,protection-domain = "avs/audio", "msm/adsp/audio_pd";
>> +
>> + q6prmcc: clock-controller {
>> + compatible = "qcom,q6prm-lpass-clocks";
>> + #clock-cells = <2>;
>> + };
>> + };
>> + };
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 0dd6a5c91d10..18e959806a13 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -3944,7 +3944,7 @@ remoteproc_adsp: remoteproc@3700000 {
>>
>> status = "disabled";
>>
>> - glink-edge {
>> + remoteproc_adsp_glink: glink-edge {
>> interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
>> IPCC_MPROC_SIGNAL_GLINK_QMP
>> IRQ_TYPE_EDGE_RISING>;
>> --
>> 2.34.1
>>
© 2016 - 2025 Red Hat, Inc.