| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4yo7v7whxffebzhoxkbpm226vsj2twc56xuf7etwwcyfrf2lzh@x2udmlhvdiwu>
Date: Tue, 2 Sep 2025 09:43:57 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Prasad Kumpatla <quic_pkumpatl@...cinc.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Srinivas Kandagatla <srini@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, cros-qcom-dts-watchers@...omium.org,
linux-arm-msm@...r.kernel.org, linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org, kernel@....qualcomm.com,
Mohammad Rafi Shaik <mohammad.rafi.shaik@....qualcomm.com>, Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v8 1/9] arm64: dts: qcom: qcs6490-audioreach: Add gpr node
On Thu, Aug 21, 2025 at 10:19:06AM +0530, Prasad Kumpatla wrote:
> From: Mohammad Rafi Shaik <mohammad.rafi.shaik@....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@....qualcomm.com>
> Co-developed-by: Prasad Kumpatla <quic_pkumpatl@...cinc.com>
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@...cinc.com>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@....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@...0000 {
>
> 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
>
Powered by blists - more mailing lists