[PATCH 0/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[PATCH 0/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[Stephan Gerhold]

num-channels and qcom,num-ees are required for BAM nodes without clock,
because the driver cannot ensure the hardware is powered on when trying to
obtain the information from the hardware registers. Specifying the node
without these properties is unsafe and has caused early boot crashes for
other SoCs before [1, 2].

The bam_dma driver has always printed an error to the kernel log in these
situations, but that was not enough to prevent people from upstreaming
patches without the required properties.

To prevent these situations in the future, enforce the presence of the
properties in both driver and schema and add the missing properties to the
affected platforms.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
Luca Weiss (1):
      arm64: dts: qcom: sm8350: Reenable crypto & cryptobam

Stephan Gerhold (7):
      arm64: dts: qcom: sm8450: Add missing properties for cryptobam
      arm64: dts: qcom: sm8550: Add missing properties for cryptobam
      arm64: dts: qcom: sm8650: Add missing properties for cryptobam
      arm64: dts: qcom: sa8775p: Add missing properties for cryptobam
      arm64: dts: qcom: ipq9574: Add missing properties for cryptobam
      dt-bindings: dma: qcom: bam-dma: Add missing required properties
      dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

 Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi                   | 2 ++
 arch/arm64/boot/dts/qcom/sa8775p.dtsi                   | 2 ++
 arch/arm64/boot/dts/qcom/sm8350.dtsi                    | 6 ++----
 arch/arm64/boot/dts/qcom/sm8450.dtsi                    | 2 ++
 arch/arm64/boot/dts/qcom/sm8550.dtsi                    | 2 ++
 arch/arm64/boot/dts/qcom/sm8650.dtsi                    | 2 ++
 drivers/dma/qcom/bam_dma.c                              | 8 ++++++--
 8 files changed, 22 insertions(+), 6 deletions(-)
---
base-commit: c674aa7c289e51659e40dda0f954886ef7f80042
change-id: 20250204-bam-dma-fixes-1e06f2a6c418

Best regards,
-- 
Stephan Gerhold <stephan.gerhold@linaro.org>

[PATCH 1/8] arm64: dts: qcom: sm8350: Reenable crypto & cryptobam

[Stephan Gerhold]

From: Luca Weiss <luca.weiss@fairphone.com>

When num-channels and qcom,num-ees is not provided in devicetree, the
driver will try to read these values from the registers during probe but
this fails if the interconnect is not on and then crashes the system.

So we can provide these properties in devicetree (queried after patching
BAM driver to enable the necessary interconnect) so we can probe
cryptobam without reading registers and then also use the QCE as
expected.

Fixes: 4d29db204361 ("arm64: dts: qcom: sm8350: fix BAM DMA crash and reboot")
Fixes: f1040a7fe8f0 ("arm64: dts: qcom: sm8350: Add Crypto Engine support")
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
Luca sent this patch before, but it was never applied:
https://lore.kernel.org/linux-arm-msm/20240108-sm8350-qce-v1-1-b7d586ff38af@fairphone.com/
---
 arch/arm64/boot/dts/qcom/sm8350.dtsi | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
index 69da30f35baaab67e51afcbca3486fb89c14f281..5f93cae01b060cf99c0de56f603e860aeebc5f67 100644
--- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
@@ -1806,11 +1806,11 @@ cryptobam: dma-controller@1dc4000 {
 			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
 			qcom,ee = <0>;
+			qcom,num-ees = <4>;
+			num-channels = <16>;
 			qcom,controlled-remotely;
 			iommus = <&apps_smmu 0x594 0x0011>,
 				 <&apps_smmu 0x596 0x0011>;
-			/* FIXME: Probing BAM DMA causes some abort and system hang */
-			status = "fail";
 		};
 
 		crypto: crypto@1dfa000 {
@@ -1822,8 +1822,6 @@ crypto: crypto@1dfa000 {
 				 <&apps_smmu 0x596 0x0011>;
 			interconnects = <&aggre2_noc MASTER_CRYPTO 0 &mc_virt SLAVE_EBI1 0>;
 			interconnect-names = "memory";
-			/* FIXME: dependency BAM DMA is disabled */
-			status = "disabled";
 		};
 
 		ipa: ipa@1e40000 {

-- 
2.47.2

[PATCH 2/8] arm64: dts: qcom: sm8450: Add missing properties for cryptobam

[Stephan Gerhold]

num-channels and qcom,num-ees are required for BAM nodes without clock,
because the driver cannot ensure the hardware is powered on when trying to
obtain the information from the hardware registers. Specifying the node
without these properties is unsafe and has caused early boot crashes for
other SoCs before [1, 2].

Add the missing information from the hardware registers to ensure the
driver can probe successfully without causing crashes.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Cc: stable@vger.kernel.org
Fixes: b92b0d2f7582 ("arm64: dts: qcom: sm8450: add crypto nodes")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 9c809fc5fa45a98ff5441a0b6809931588897243..419df72cd04b0c328756fdc484f4e46b6c325412 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -5283,6 +5283,8 @@ cryptobam: dma-controller@1dc4000 {
 			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
 			qcom,ee = <0>;
+			qcom,num-ees = <4>;
+			num-channels = <16>;
 			qcom,controlled-remotely;
 			iommus = <&apps_smmu 0x584 0x11>,
 				 <&apps_smmu 0x588 0x0>,

-- 
2.47.2

[PATCH 3/8] arm64: dts: qcom: sm8550: Add missing properties for cryptobam

[Stephan Gerhold]

num-channels and qcom,num-ees are required for BAM nodes without clock,
because the driver cannot ensure the hardware is powered on when trying to
obtain the information from the hardware registers. Specifying the node
without these properties is unsafe and has caused early boot crashes for
other SoCs before [1, 2].

Add the missing information from the hardware registers to ensure the
driver can probe successfully without causing crashes.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Cc: stable@vger.kernel.org
Fixes: 433477c3bf0b ("arm64: dts: qcom: sm8550: add QCrypto nodes")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index eac8de4005d82f246bc50f64f09515631d895c99..ac3e00ad417719be2885d76d3197f96137848337 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -1957,6 +1957,8 @@ cryptobam: dma-controller@1dc4000 {
 			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
 			qcom,ee = <0>;
+			qcom,num-ees = <4>;
+			num-channels = <20>;
 			qcom,controlled-remotely;
 			iommus = <&apps_smmu 0x480 0x0>,
 				 <&apps_smmu 0x481 0x0>;

-- 
2.47.2

[PATCH 4/8] arm64: dts: qcom: sm8650: Add missing properties for cryptobam

[Stephan Gerhold]

num-channels and qcom,num-ees are required for BAM nodes without clock,
because the driver cannot ensure the hardware is powered on when trying to
obtain the information from the hardware registers. Specifying the node
without these properties is unsafe and has caused early boot crashes for
other SoCs before [1, 2].

Add the missing information from the hardware registers to ensure the
driver can probe successfully without causing crashes.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Cc: stable@vger.kernel.org
Fixes: 10e024671295 ("arm64: dts: qcom: sm8650: add interconnect dependent device nodes")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..c8a2a76a98f000610f33cd1ada82eebd6ae95343 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -2533,6 +2533,8 @@ cryptobam: dma-controller@1dc4000 {
 				 <&apps_smmu 0x481 0>;
 
 			qcom,ee = <0>;
+			qcom,num-ees = <4>;
+			num-channels = <20>;
 			qcom,controlled-remotely;
 		};
 

-- 
2.47.2

[PATCH 5/8] arm64: dts: qcom: sa8775p: Add missing properties for cryptobam

[Stephan Gerhold]

num-channels and qcom,num-ees are required for BAM nodes without clock,
because the driver cannot ensure the hardware is powered on when trying to
obtain the information from the hardware registers. Specifying the node
without these properties is unsafe and has caused early boot crashes for
other SoCs before [1, 2].

Add the missing information from the hardware registers to ensure the
driver can probe successfully without causing crashes.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Cc: stable@vger.kernel.org
Fixes: 7ff3da43ef44 ("arm64: dts: qcom: sa8775p: add QCrypto nodes")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 3394ae2d13003417a15e64c9e47833725ec779e6..8d7d157ff2f56a31a97121c882e3e0336356eda0 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -2413,6 +2413,8 @@ cryptobam: dma-controller@1dc4000 {
 			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
 			qcom,ee = <0>;
+			qcom,num-ees = <4>;
+			num-channels = <20>;
 			qcom,controlled-remotely;
 			iommus = <&apps_smmu 0x480 0x00>,
 				 <&apps_smmu 0x481 0x00>;

-- 
2.47.2

[PATCH 6/8] arm64: dts: qcom: ipq9574: Add missing properties for cryptobam

[Stephan Gerhold]

num-channels and qcom,num-ees are required for BAM nodes without clock,
because the driver cannot ensure the hardware is powered on when trying to
obtain the information from the hardware registers. Specifying the node
without these properties is unsafe and has caused early boot crashes for
other SoCs before [1, 2].

Add the missing information from the hardware registers to ensure the
driver can probe successfully without causing crashes.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Cc: stable@vger.kernel.org
Tested-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Fixes: ffadc79ed99f ("arm64: dts: qcom: ipq9574: Enable crypto nodes")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 9422900289725774da8cfea9848529891038e57a..3c02351fbb156a314b7911def3caeff0c14b92e4 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -378,6 +378,8 @@ cryptobam: dma-controller@704000 {
 			interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
 			qcom,ee = <1>;
+			qcom,num-ees = <4>;
+			num-channels = <16>;
 			qcom,controlled-remotely;
 		};
 

-- 
2.47.2

[PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Stephan Gerhold]

num-channels and qcom,num-ees are required when there are no clocks
specified in the device tree, because we have no reliable way to read them
from the hardware registers if we cannot ensure the BAM hardware is up when
the device is being probed.

This has often been forgotten when adding new SoC device trees, so make
this clear by describing this requirement in the schema.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
--- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
+++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
@@ -90,8 +90,12 @@ required:
 anyOf:
   - required:
       - qcom,powered-remotely
+      - num-channels
+      - qcom,num-ees
   - required:
       - qcom,controlled-remotely
+      - num-channels
+      - qcom,num-ees
   - required:
       - clocks
       - clock-names

-- 
2.47.2

[PATCH 8/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[Stephan Gerhold]

When we don't have a clock specified in the device tree, we have no way to
ensure the BAM is on. This is often the case for remotely-controlled or
remotely-powered BAM instances. In this case, we need to read num-channels
from the DT to have all the necessary information to complete probing.

However, at the moment invalid device trees without clock and without
num-channels still continue probing, because the error handling is missing
return statements. The driver will then later try to read the number of
channels from the registers. This is unsafe, because it relies on boot
firmware and lucky timing to succeed. Unfortunately, the lack of proper
error handling here has been abused for several Qualcomm SoCs upstream,
causing early boot crashes in several situations [1, 2].

Avoid these early crashes by erroring out when any of the required DT
properties are missing. Note that this will break some of the existing DTs
upstream (mainly BAM instances related to the crypto engine). However,
clearly these DTs have never been tested properly, since the error in the
kernel log was just ignored. It's safer to disable the crypto engine for
these broken DTBs.

[1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
[2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/

Cc: stable@vger.kernel.org
Fixes: 48d163b1aa6e ("dmaengine: qcom: bam_dma: get num-channels and num-ees from dt")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index c14557efd577046adc74fa83fd45eb239977b5fa..a2f1f8902c7f88398a5412e8673e24b3c10bb86f 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1291,13 +1291,17 @@ static int bam_dma_probe(struct platform_device *pdev)
 	if (!bdev->bamclk) {
 		ret = of_property_read_u32(pdev->dev.of_node, "num-channels",
 					   &bdev->num_channels);
-		if (ret)
+		if (ret) {
 			dev_err(bdev->dev, "num-channels unspecified in dt\n");
+			return ret;
+		}
 
 		ret = of_property_read_u32(pdev->dev.of_node, "qcom,num-ees",
 					   &bdev->num_ees);
-		if (ret)
+		if (ret) {
 			dev_err(bdev->dev, "num-ees unspecified in dt\n");
+			return ret;
+		}
 	}
 
 	ret = clk_prepare_enable(bdev->bamclk);

-- 
2.47.2

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Konrad Dybcio]

On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
> num-channels and qcom,num-ees are required when there are no clocks
> specified in the device tree, because we have no reliable way to read them
> from the hardware registers if we cannot ensure the BAM hardware is up when
> the device is being probed.
> 
> This has often been forgotten when adding new SoC device trees, so make
> this clear by describing this requirement in the schema.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> @@ -90,8 +90,12 @@ required:
>  anyOf:
>    - required:
>        - qcom,powered-remotely
> +      - num-channels
> +      - qcom,num-ees
>    - required:
>        - qcom,controlled-remotely
> +      - num-channels
> +      - qcom,num-ees

I think I'd rather see these deprecated and add the clock everywhere..
Do we know which one we need to add on newer platforms? Or maybe it's
been transformed into an icc path?

Reading back things from this piece of HW only to add it to DT to avoid
reading it later is a really messy solution.

Konrad

>    - required:
>        - clocks
>        - clock-names
> 

Re: [PATCH 8/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[Konrad Dybcio]

On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
> When we don't have a clock specified in the device tree, we have no way to
> ensure the BAM is on. This is often the case for remotely-controlled or
> remotely-powered BAM instances. In this case, we need to read num-channels
> from the DT to have all the necessary information to complete probing.
> 
> However, at the moment invalid device trees without clock and without
> num-channels still continue probing, because the error handling is missing
> return statements. The driver will then later try to read the number of
> channels from the registers. This is unsafe, because it relies on boot
> firmware and lucky timing to succeed. Unfortunately, the lack of proper
> error handling here has been abused for several Qualcomm SoCs upstream,
> causing early boot crashes in several situations [1, 2].
> 
> Avoid these early crashes by erroring out when any of the required DT
> properties are missing. Note that this will break some of the existing DTs
> upstream (mainly BAM instances related to the crypto engine). However,
> clearly these DTs have never been tested properly, since the error in the
> kernel log was just ignored. It's safer to disable the crypto engine for
> these broken DTBs.
> 
> [1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
> [2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/
> 
> Cc: stable@vger.kernel.org
> Fixes: 48d163b1aa6e ("dmaengine: qcom: bam_dma: get num-channels and num-ees from dt")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577046adc74fa83fd45eb239977b5fa..a2f1f8902c7f88398a5412e8673e24b3c10bb86f 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1291,13 +1291,17 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	if (!bdev->bamclk) {
>  		ret = of_property_read_u32(pdev->dev.of_node, "num-channels",
>  					   &bdev->num_channels);
> -		if (ret)
> +		if (ret) {
>  			dev_err(bdev->dev, "num-channels unspecified in dt\n");
> +			return ret;
> +		}
>  
>  		ret = of_property_read_u32(pdev->dev.of_node, "qcom,num-ees",
>  					   &bdev->num_ees);
> -		if (ret)
> +		if (ret) {
>  			dev_err(bdev->dev, "num-ees unspecified in dt\n");
> +			return ret;
> +		}

I like dev_err_probe, but this works too

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Stephan Gerhold]

On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote:
> On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
> > num-channels and qcom,num-ees are required when there are no clocks
> > specified in the device tree, because we have no reliable way to read them
> > from the hardware registers if we cannot ensure the BAM hardware is up when
> > the device is being probed.
> > 
> > This has often been forgotten when adding new SoC device trees, so make
> > this clear by describing this requirement in the schema.
> > 
> > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
> > --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > @@ -90,8 +90,12 @@ required:
> >  anyOf:
> >    - required:
> >        - qcom,powered-remotely
> > +      - num-channels
> > +      - qcom,num-ees
> >    - required:
> >        - qcom,controlled-remotely
> > +      - num-channels
> > +      - qcom,num-ees
> 
> I think I'd rather see these deprecated and add the clock everywhere..
> Do we know which one we need to add on newer platforms? Or maybe it's
> been transformed into an icc path?

This isn't feasible, there are too many different setups. Also often the
BAM power management is tightly integrated into the consumer interface.
To give a short excerpt (I'm sure there are even more obscure uses):

 - BLSP BAM (UART, I2C, SPI on older SoCs):
    1. Enable GCC_BLSP_AHB_CLK
    -> This is what the bam_dma driver supports currently.

 - Crypto BAM: Either
    OR 1. Vote for single RPM clock
    OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI)
    OR 1. Vote dummy bandwidth for interconnect

 - BAM DMUX (WWAN on older SoCs):
    1. Start modem firmware
    2. Wait for BAM DMUX service to be up
    3. Vote for power up via the BAM-DMUX-specific SMEM state
    4. Hope the firmware agrees and brings up the BAM

 - SLIMbus BAM (audio on some SoCs):
    1. Start ADSP firmware
    2. Wait for QMI SLIMBUS service to be up via QRTR
    3. Vote for power up via SLIMbus-specific QMI messages
    4. Hope the firmware agrees and brings up the BAM

Especially for the last two, we can't implement support for those
consumer-specific interfaces in the BAM driver. Implementing support for
the 3 variants of the Crypto BAM would be possible, but it's honestly
the least interesting use case of all these. It's not really clear why
we are bothing with the crypto engine on newer SoCs at all, see e.g. [1].

[1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/

> Reading back things from this piece of HW only to add it to DT to avoid
> reading it later is a really messy solution.

In retrospect, it could have been cleaner to avoid describing the BAM as
device node independent of the consumer. We wouldn't have this problem
if the BAM driver would only probe when the consumer is already ready.

But I think specifying num-channels in the device tree is the cleanest
way out of this mess. I have a second patch series ready that drops
qcom,num-ees and validates the num-channels once it's safe reading from
the BAM registers. That way, you just need one boot test to ensure the
device tree description is really correct.

Thanks,
Stephan

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Konrad Dybcio]

On 13.02.2025 10:13 AM, Stephan Gerhold wrote:
> On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote:
>> On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
>>> num-channels and qcom,num-ees are required when there are no clocks
>>> specified in the device tree, because we have no reliable way to read them
>>> from the hardware registers if we cannot ensure the BAM hardware is up when
>>> the device is being probed.
>>>
>>> This has often been forgotten when adding new SoC device trees, so make
>>> this clear by describing this requirement in the schema.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>> @@ -90,8 +90,12 @@ required:
>>>  anyOf:
>>>    - required:
>>>        - qcom,powered-remotely
>>> +      - num-channels
>>> +      - qcom,num-ees
>>>    - required:
>>>        - qcom,controlled-remotely
>>> +      - num-channels
>>> +      - qcom,num-ees
>>
>> I think I'd rather see these deprecated and add the clock everywhere..
>> Do we know which one we need to add on newer platforms? Or maybe it's
>> been transformed into an icc path?
> 
> This isn't feasible, there are too many different setups. Also often the
> BAM power management is tightly integrated into the consumer interface.
> To give a short excerpt (I'm sure there are even more obscure uses):
> 
>  - BLSP BAM (UART, I2C, SPI on older SoCs):
>     1. Enable GCC_BLSP_AHB_CLK
>     -> This is what the bam_dma driver supports currently.
> 
>  - Crypto BAM: Either
>     OR 1. Vote for single RPM clock
>     OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI)
>     OR 1. Vote dummy bandwidth for interconnect
> 
>  - BAM DMUX (WWAN on older SoCs):
>     1. Start modem firmware
>     2. Wait for BAM DMUX service to be up
>     3. Vote for power up via the BAM-DMUX-specific SMEM state
>     4. Hope the firmware agrees and brings up the BAM
> 
>  - SLIMbus BAM (audio on some SoCs):
>     1. Start ADSP firmware
>     2. Wait for QMI SLIMBUS service to be up via QRTR
>     3. Vote for power up via SLIMbus-specific QMI messages
>     4. Hope the firmware agrees and brings up the BAM
> 
> Especially for the last two, we can't implement support for those
> consumer-specific interfaces in the BAM driver. Implementing support for
> the 3 variants of the Crypto BAM would be possible, but it's honestly
> the least interesting use case of all these. It's not really clear why
> we are bothing with the crypto engine on newer SoCs at all, see e.g. [1].
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/
> 
>> Reading back things from this piece of HW only to add it to DT to avoid
>> reading it later is a really messy solution.
> 
> In retrospect, it could have been cleaner to avoid describing the BAM as
> device node independent of the consumer. We wouldn't have this problem
> if the BAM driver would only probe when the consumer is already ready.
> 
> But I think specifying num-channels in the device tree is the cleanest
> way out of this mess. I have a second patch series ready that drops
> qcom,num-ees and validates the num-channels once it's safe reading from
> the BAM registers. That way, you just need one boot test to ensure the
> device tree description is really correct.

Thanks for the detailed explanation!

Do you think it could maybe make sense to expose a clock/power-domain
from the modem/adsp rproc and feed it to the DMUX / SLIM instances when
an appropriate ping arrives? This way we'd also defer probing the drivers
until the device is actually accessible.

Konrad

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Stephan Gerhold]

On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote:
> On 13.02.2025 10:13 AM, Stephan Gerhold wrote:
> > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote:
> >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
> >>> num-channels and qcom,num-ees are required when there are no clocks
> >>> specified in the device tree, because we have no reliable way to read them
> >>> from the hardware registers if we cannot ensure the BAM hardware is up when
> >>> the device is being probed.
> >>>
> >>> This has often been forgotten when adding new SoC device trees, so make
> >>> this clear by describing this requirement in the schema.
> >>>
> >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
> >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> >>> @@ -90,8 +90,12 @@ required:
> >>>  anyOf:
> >>>    - required:
> >>>        - qcom,powered-remotely
> >>> +      - num-channels
> >>> +      - qcom,num-ees
> >>>    - required:
> >>>        - qcom,controlled-remotely
> >>> +      - num-channels
> >>> +      - qcom,num-ees
> >>
> >> I think I'd rather see these deprecated and add the clock everywhere..
> >> Do we know which one we need to add on newer platforms? Or maybe it's
> >> been transformed into an icc path?
> > 
> > This isn't feasible, there are too many different setups. Also often the
> > BAM power management is tightly integrated into the consumer interface.
> > To give a short excerpt (I'm sure there are even more obscure uses):
> > 
> >  - BLSP BAM (UART, I2C, SPI on older SoCs):
> >     1. Enable GCC_BLSP_AHB_CLK
> >     -> This is what the bam_dma driver supports currently.
> > 
> >  - Crypto BAM: Either
> >     OR 1. Vote for single RPM clock
> >     OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI)
> >     OR 1. Vote dummy bandwidth for interconnect
> > 
> >  - BAM DMUX (WWAN on older SoCs):
> >     1. Start modem firmware
> >     2. Wait for BAM DMUX service to be up
> >     3. Vote for power up via the BAM-DMUX-specific SMEM state
> >     4. Hope the firmware agrees and brings up the BAM
> > 
> >  - SLIMbus BAM (audio on some SoCs):
> >     1. Start ADSP firmware
> >     2. Wait for QMI SLIMBUS service to be up via QRTR
> >     3. Vote for power up via SLIMbus-specific QMI messages
> >     4. Hope the firmware agrees and brings up the BAM
> > 
> > Especially for the last two, we can't implement support for those
> > consumer-specific interfaces in the BAM driver. Implementing support for
> > the 3 variants of the Crypto BAM would be possible, but it's honestly
> > the least interesting use case of all these. It's not really clear why
> > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1].
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/
> > 
> >> Reading back things from this piece of HW only to add it to DT to avoid
> >> reading it later is a really messy solution.
> > 
> > In retrospect, it could have been cleaner to avoid describing the BAM as
> > device node independent of the consumer. We wouldn't have this problem
> > if the BAM driver would only probe when the consumer is already ready.
> > 
> > But I think specifying num-channels in the device tree is the cleanest
> > way out of this mess. I have a second patch series ready that drops
> > qcom,num-ees and validates the num-channels once it's safe reading from
> > the BAM registers. That way, you just need one boot test to ensure the
> > device tree description is really correct.
> 
> Thanks for the detailed explanation!
> 
> Do you think it could maybe make sense to expose a clock/power-domain
> from the modem/adsp rproc and feed it to the DMUX / SLIM instances when
> an appropriate ping arrives? This way we'd also defer probing the drivers
> until the device is actually accessible.
> 

Maybe, but that would result in a cyclic dependency between the DMA
provider and consumer. E.g.

	bam_dmux_dma: dma-controller@ {
		#dma-cells = <1>;
		power-domains = <&bam_dmux>;
	};

	remoteproc@ {
		/* ... */

		bam_dmux: bam-dmux {
			dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>;
			dma-names = "tx", "rx";
		};
	};

fw_devlink will likely get confused by that.

At the end my thought process here is the following:

 1. BAM-DMA is a legacy block at this point, it doesn't look like there
    are any new use cases being added on new SoCs
 2. We need to preserve compatibility with the old bindings anyway
 3. I trimmed it down to having to specify just "num-channels"
 4. Everything else is read from the hardware registers, and
    num-channels gets validated when the first DMA channel is requested

I think it's the best we can do here at this point.

Thanks,
Stephan

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Konrad Dybcio]

On 13.02.2025 4:22 PM, Stephan Gerhold wrote:
> On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote:
>> On 13.02.2025 10:13 AM, Stephan Gerhold wrote:
>>> On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote:
>>>> On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
>>>>> num-channels and qcom,num-ees are required when there are no clocks
>>>>> specified in the device tree, because we have no reliable way to read them
>>>>> from the hardware registers if we cannot ensure the BAM hardware is up when
>>>>> the device is being probed.
>>>>>
>>>>> This has often been forgotten when adding new SoC device trees, so make
>>>>> this clear by describing this requirement in the schema.
>>>>>
>>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
>>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>> @@ -90,8 +90,12 @@ required:
>>>>>  anyOf:
>>>>>    - required:
>>>>>        - qcom,powered-remotely
>>>>> +      - num-channels
>>>>> +      - qcom,num-ees
>>>>>    - required:
>>>>>        - qcom,controlled-remotely
>>>>> +      - num-channels
>>>>> +      - qcom,num-ees
>>>>
>>>> I think I'd rather see these deprecated and add the clock everywhere..
>>>> Do we know which one we need to add on newer platforms? Or maybe it's
>>>> been transformed into an icc path?
>>>
>>> This isn't feasible, there are too many different setups. Also often the
>>> BAM power management is tightly integrated into the consumer interface.
>>> To give a short excerpt (I'm sure there are even more obscure uses):
>>>
>>>  - BLSP BAM (UART, I2C, SPI on older SoCs):
>>>     1. Enable GCC_BLSP_AHB_CLK
>>>     -> This is what the bam_dma driver supports currently.
>>>
>>>  - Crypto BAM: Either
>>>     OR 1. Vote for single RPM clock
>>>     OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI)
>>>     OR 1. Vote dummy bandwidth for interconnect
>>>
>>>  - BAM DMUX (WWAN on older SoCs):
>>>     1. Start modem firmware
>>>     2. Wait for BAM DMUX service to be up
>>>     3. Vote for power up via the BAM-DMUX-specific SMEM state
>>>     4. Hope the firmware agrees and brings up the BAM
>>>
>>>  - SLIMbus BAM (audio on some SoCs):
>>>     1. Start ADSP firmware
>>>     2. Wait for QMI SLIMBUS service to be up via QRTR
>>>     3. Vote for power up via SLIMbus-specific QMI messages
>>>     4. Hope the firmware agrees and brings up the BAM
>>>
>>> Especially for the last two, we can't implement support for those
>>> consumer-specific interfaces in the BAM driver. Implementing support for
>>> the 3 variants of the Crypto BAM would be possible, but it's honestly
>>> the least interesting use case of all these. It's not really clear why
>>> we are bothing with the crypto engine on newer SoCs at all, see e.g. [1].
>>>
>>> [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/
>>>
>>>> Reading back things from this piece of HW only to add it to DT to avoid
>>>> reading it later is a really messy solution.
>>>
>>> In retrospect, it could have been cleaner to avoid describing the BAM as
>>> device node independent of the consumer. We wouldn't have this problem
>>> if the BAM driver would only probe when the consumer is already ready.
>>>
>>> But I think specifying num-channels in the device tree is the cleanest
>>> way out of this mess. I have a second patch series ready that drops
>>> qcom,num-ees and validates the num-channels once it's safe reading from
>>> the BAM registers. That way, you just need one boot test to ensure the
>>> device tree description is really correct.
>>
>> Thanks for the detailed explanation!
>>
>> Do you think it could maybe make sense to expose a clock/power-domain
>> from the modem/adsp rproc and feed it to the DMUX / SLIM instances when
>> an appropriate ping arrives? This way we'd also defer probing the drivers
>> until the device is actually accessible.
>>
> 
> Maybe, but that would result in a cyclic dependency between the DMA
> provider and consumer. E.g.
> 
> 	bam_dmux_dma: dma-controller@ {
> 		#dma-cells = <1>;
> 		power-domains = <&bam_dmux>;
> 	};
> 
> 	remoteproc@ {
> 		/* ... */
> 
> 		bam_dmux: bam-dmux {
> 			dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>;
> 			dma-names = "tx", "rx";
> 		};
> 	};
> 
> fw_devlink will likely get confused by that.
> 
> At the end my thought process here is the following:
> 
>  1. BAM-DMA is a legacy block at this point, it doesn't look like there
>     are any new use cases being added on new SoCs
>  2. We need to preserve compatibility with the old bindings anyway
>  3. I trimmed it down to having to specify just "num-channels"
>  4. Everything else is read from the hardware registers, and
>     num-channels gets validated when the first DMA channel is requested
> 
> I think it's the best we can do here at this point.

Alright, let's go this route then.

Konrad

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Rob Herring]

On Thu, Feb 13, 2025 at 04:22:17PM +0100, Stephan Gerhold wrote:
> On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote:
> > On 13.02.2025 10:13 AM, Stephan Gerhold wrote:
> > > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote:
> > >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
> > >>> num-channels and qcom,num-ees are required when there are no clocks
> > >>> specified in the device tree, because we have no reliable way to read them
> > >>> from the hardware registers if we cannot ensure the BAM hardware is up when
> > >>> the device is being probed.
> > >>>
> > >>> This has often been forgotten when adding new SoC device trees, so make
> > >>> this clear by describing this requirement in the schema.
> > >>>
> > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
> > >>>  1 file changed, 4 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
> > >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > >>> @@ -90,8 +90,12 @@ required:
> > >>>  anyOf:
> > >>>    - required:
> > >>>        - qcom,powered-remotely
> > >>> +      - num-channels
> > >>> +      - qcom,num-ees
> > >>>    - required:
> > >>>        - qcom,controlled-remotely
> > >>> +      - num-channels
> > >>> +      - qcom,num-ees
> > >>
> > >> I think I'd rather see these deprecated and add the clock everywhere..
> > >> Do we know which one we need to add on newer platforms? Or maybe it's
> > >> been transformed into an icc path?
> > > 
> > > This isn't feasible, there are too many different setups. Also often the
> > > BAM power management is tightly integrated into the consumer interface.
> > > To give a short excerpt (I'm sure there are even more obscure uses):
> > > 
> > >  - BLSP BAM (UART, I2C, SPI on older SoCs):
> > >     1. Enable GCC_BLSP_AHB_CLK
> > >     -> This is what the bam_dma driver supports currently.
> > > 
> > >  - Crypto BAM: Either
> > >     OR 1. Vote for single RPM clock
> > >     OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI)
> > >     OR 1. Vote dummy bandwidth for interconnect
> > > 
> > >  - BAM DMUX (WWAN on older SoCs):
> > >     1. Start modem firmware
> > >     2. Wait for BAM DMUX service to be up
> > >     3. Vote for power up via the BAM-DMUX-specific SMEM state
> > >     4. Hope the firmware agrees and brings up the BAM
> > > 
> > >  - SLIMbus BAM (audio on some SoCs):
> > >     1. Start ADSP firmware
> > >     2. Wait for QMI SLIMBUS service to be up via QRTR
> > >     3. Vote for power up via SLIMbus-specific QMI messages
> > >     4. Hope the firmware agrees and brings up the BAM
> > > 
> > > Especially for the last two, we can't implement support for those
> > > consumer-specific interfaces in the BAM driver. Implementing support for
> > > the 3 variants of the Crypto BAM would be possible, but it's honestly
> > > the least interesting use case of all these. It's not really clear why
> > > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1].
> > > 
> > > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/
> > > 
> > >> Reading back things from this piece of HW only to add it to DT to avoid
> > >> reading it later is a really messy solution.
> > > 
> > > In retrospect, it could have been cleaner to avoid describing the BAM as
> > > device node independent of the consumer. We wouldn't have this problem
> > > if the BAM driver would only probe when the consumer is already ready.
> > > 
> > > But I think specifying num-channels in the device tree is the cleanest
> > > way out of this mess. I have a second patch series ready that drops
> > > qcom,num-ees and validates the num-channels once it's safe reading from
> > > the BAM registers. That way, you just need one boot test to ensure the
> > > device tree description is really correct.
> > 
> > Thanks for the detailed explanation!
> > 
> > Do you think it could maybe make sense to expose a clock/power-domain
> > from the modem/adsp rproc and feed it to the DMUX / SLIM instances when
> > an appropriate ping arrives? This way we'd also defer probing the drivers
> > until the device is actually accessible.
> > 
> 
> Maybe, but that would result in a cyclic dependency between the DMA
> provider and consumer. E.g.
> 
> 	bam_dmux_dma: dma-controller@ {
> 		#dma-cells = <1>;
> 		power-domains = <&bam_dmux>;
> 	};
> 
> 	remoteproc@ {
> 		/* ... */
> 
> 		bam_dmux: bam-dmux {
> 			dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>;
> 			dma-names = "tx", "rx";
> 		};
> 	};
> 
> fw_devlink will likely get confused by that.

Why? We have a property to break cycles: post-init-providers

That doesn't work here?

Rob

Re: [PATCH 7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties

[Stephan Gerhold]

On Wed, Feb 19, 2025 at 04:27:39PM -0600, Rob Herring wrote:
> On Thu, Feb 13, 2025 at 04:22:17PM +0100, Stephan Gerhold wrote:
> > On Thu, Feb 13, 2025 at 03:00:00PM +0100, Konrad Dybcio wrote:
> > > On 13.02.2025 10:13 AM, Stephan Gerhold wrote:
> > > > On Wed, Feb 12, 2025 at 10:01:59PM +0100, Konrad Dybcio wrote:
> > > >> On 12.02.2025 6:03 PM, Stephan Gerhold wrote:
> > > >>> num-channels and qcom,num-ees are required when there are no clocks
> > > >>> specified in the device tree, because we have no reliable way to read them
> > > >>> from the hardware registers if we cannot ensure the BAM hardware is up when
> > > >>> the device is being probed.
> > > >>>
> > > >>> This has often been forgotten when adding new SoC device trees, so make
> > > >>> this clear by describing this requirement in the schema.
> > > >>>
> > > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> > > >>> ---
> > > >>>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 4 ++++
> > > >>>  1 file changed, 4 insertions(+)
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > > >>> index 3ad0d9b1fbc5e4f83dd316d1ad79773c288748ba..5f7e7763615578717651014cfd52745ea2132115 100644
> > > >>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > > >>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> > > >>> @@ -90,8 +90,12 @@ required:
> > > >>>  anyOf:
> > > >>>    - required:
> > > >>>        - qcom,powered-remotely
> > > >>> +      - num-channels
> > > >>> +      - qcom,num-ees
> > > >>>    - required:
> > > >>>        - qcom,controlled-remotely
> > > >>> +      - num-channels
> > > >>> +      - qcom,num-ees
> > > >>
> > > >> I think I'd rather see these deprecated and add the clock everywhere..
> > > >> Do we know which one we need to add on newer platforms? Or maybe it's
> > > >> been transformed into an icc path?
> > > > 
> > > > This isn't feasible, there are too many different setups. Also often the
> > > > BAM power management is tightly integrated into the consumer interface.
> > > > To give a short excerpt (I'm sure there are even more obscure uses):
> > > > 
> > > >  - BLSP BAM (UART, I2C, SPI on older SoCs):
> > > >     1. Enable GCC_BLSP_AHB_CLK
> > > >     -> This is what the bam_dma driver supports currently.
> > > > 
> > > >  - Crypto BAM: Either
> > > >     OR 1. Vote for single RPM clock
> > > >     OR 1. Enable 3 separate clocks (CE, CE_AHB, CE_AXI)
> > > >     OR 1. Vote dummy bandwidth for interconnect
> > > > 
> > > >  - BAM DMUX (WWAN on older SoCs):
> > > >     1. Start modem firmware
> > > >     2. Wait for BAM DMUX service to be up
> > > >     3. Vote for power up via the BAM-DMUX-specific SMEM state
> > > >     4. Hope the firmware agrees and brings up the BAM
> > > > 
> > > >  - SLIMbus BAM (audio on some SoCs):
> > > >     1. Start ADSP firmware
> > > >     2. Wait for QMI SLIMBUS service to be up via QRTR
> > > >     3. Vote for power up via SLIMbus-specific QMI messages
> > > >     4. Hope the firmware agrees and brings up the BAM
> > > > 
> > > > Especially for the last two, we can't implement support for those
> > > > consumer-specific interfaces in the BAM driver. Implementing support for
> > > > the 3 variants of the Crypto BAM would be possible, but it's honestly
> > > > the least interesting use case of all these. It's not really clear why
> > > > we are bothing with the crypto engine on newer SoCs at all, see e.g. [1].
> > > > 
> > > > [1]: https://lore.kernel.org/linux-arm-msm/20250118080604.GA721573@sol.localdomain/
> > > > 
> > > >> Reading back things from this piece of HW only to add it to DT to avoid
> > > >> reading it later is a really messy solution.
> > > > 
> > > > In retrospect, it could have been cleaner to avoid describing the BAM as
> > > > device node independent of the consumer. We wouldn't have this problem
> > > > if the BAM driver would only probe when the consumer is already ready.
> > > > 
> > > > But I think specifying num-channels in the device tree is the cleanest
> > > > way out of this mess. I have a second patch series ready that drops
> > > > qcom,num-ees and validates the num-channels once it's safe reading from
> > > > the BAM registers. That way, you just need one boot test to ensure the
> > > > device tree description is really correct.
> > > 
> > > Thanks for the detailed explanation!
> > > 
> > > Do you think it could maybe make sense to expose a clock/power-domain
> > > from the modem/adsp rproc and feed it to the DMUX / SLIM instances when
> > > an appropriate ping arrives? This way we'd also defer probing the drivers
> > > until the device is actually accessible.
> > > 
> > 
> > Maybe, but that would result in a cyclic dependency between the DMA
> > provider and consumer. E.g.
> > 
> > 	bam_dmux_dma: dma-controller@ {
> > 		#dma-cells = <1>;
> > 		power-domains = <&bam_dmux>;
> > 	};
> > 
> > 	remoteproc@ {
> > 		/* ... */
> > 
> > 		bam_dmux: bam-dmux {
> > 			dmas = <&bam_dmux_dma 4>, <&bam_dmux_dma 5>;
> > 			dma-names = "tx", "rx";
> > 		};
> > 	};
> > 
> > fw_devlink will likely get confused by that.
> 
> Why? We have a property to break cycles: post-init-providers
> 
> That doesn't work here?
> 

Thanks, I was not aware of that property. This looks quite useful for
fixing up some of the other cyclic dependencies we have!

Nevertheless, for this specific case, I still think we should not make
such large breaking changes at this point. As I pointed out further
below in my quoted email, this is a legacy hardware block that will
likely not get any major new users in the future. We're essentially
discussing to rework several bindings and drivers just to drop a single
straightforward "num-channels = <N>" property. A property that we will
need to keep support for anyway, to support users with older DTBs. This
effort (and risk) is really better spent elsewhere.

Thanks,
Stephan

Re: (subset) [PATCH 0/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[Bjorn Andersson]

On Wed, 12 Feb 2025 18:03:46 +0100, Stephan Gerhold wrote:
> num-channels and qcom,num-ees are required for BAM nodes without clock,
> because the driver cannot ensure the hardware is powered on when trying to
> obtain the information from the hardware registers. Specifying the node
> without these properties is unsafe and has caused early boot crashes for
> other SoCs before [1, 2].
> 
> The bam_dma driver has always printed an error to the kernel log in these
> situations, but that was not enough to prevent people from upstreaming
> patches without the required properties.
> 
> [...]

Applied, thanks!

[1/8] arm64: dts: qcom: sm8350: Reenable crypto & cryptobam
      commit: 75eefd474469abf95aa9ef6da8161d69f86b98b4
[2/8] arm64: dts: qcom: sm8450: Add missing properties for cryptobam
      commit: 0fe6357229cb15a64b6413c62f1c3d4de68ce55f
[3/8] arm64: dts: qcom: sm8550: Add missing properties for cryptobam
      commit: 663cd2cad36da23cf1a3db7868fce9f1a19b2d61
[4/8] arm64: dts: qcom: sm8650: Add missing properties for cryptobam
      commit: 38b88722bce07b6a5927f45fbf7a9a85e834572c
[5/8] arm64: dts: qcom: sa8775p: Add missing properties for cryptobam
      commit: a2517331f11bd22cded60e791a8818cec3e7597a
[6/8] arm64: dts: qcom: ipq9574: Add missing properties for cryptobam
      commit: b4cd966edb2deb5c75fe356191422e127445b830

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: (subset) [PATCH 0/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[Vinod Koul]

On Wed, 12 Feb 2025 18:03:46 +0100, Stephan Gerhold wrote:
> num-channels and qcom,num-ees are required for BAM nodes without clock,
> because the driver cannot ensure the hardware is powered on when trying to
> obtain the information from the hardware registers. Specifying the node
> without these properties is unsafe and has caused early boot crashes for
> other SoCs before [1, 2].
> 
> The bam_dma driver has always printed an error to the kernel log in these
> situations, but that was not enough to prevent people from upstreaming
> patches without the required properties.
> 
> [...]

Applied, thanks!

[7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties
      commit: b04b950fddfcf2f7b0e6f789499c256d15b70720

Best regards,
-- 
~Vinod

Re: (subset) [PATCH 0/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees

[Vinod Koul]

On Wed, 12 Feb 2025 18:03:46 +0100, Stephan Gerhold wrote:
> num-channels and qcom,num-ees are required for BAM nodes without clock,
> because the driver cannot ensure the hardware is powered on when trying to
> obtain the information from the hardware registers. Specifying the node
> without these properties is unsafe and has caused early boot crashes for
> other SoCs before [1, 2].
> 
> The bam_dma driver has always printed an error to the kernel log in these
> situations, but that was not enough to prevent people from upstreaming
> patches without the required properties.
> 
> [...]

Applied, thanks!

[7/8] dt-bindings: dma: qcom: bam-dma: Add missing required properties
      commit: e0e2cea86f75c8255b7da13ec92a34bb35a9c4fb
[8/8] dmaengine: qcom: bam_dma: Fix DT error handling for num-channels/ees
      commit: 2f8a2cfd0994adf5f71737bb0946a76800479220

Best regards,
-- 
~Vinod

Re: [PATCH 3/8] arm64: dts: qcom: sm8550: Add missing properties for cryptobam

[Neil Armstrong]

On 12/02/2025 18:03, Stephan Gerhold wrote:
> num-channels and qcom,num-ees are required for BAM nodes without clock,
> because the driver cannot ensure the hardware is powered on when trying to
> obtain the information from the hardware registers. Specifying the node
> without these properties is unsafe and has caused early boot crashes for
> other SoCs before [1, 2].
> 
> Add the missing information from the hardware registers to ensure the
> driver can probe successfully without causing crashes.
> 
> [1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
> [2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/
> 
> Cc: stable@vger.kernel.org
> Fixes: 433477c3bf0b ("arm64: dts: qcom: sm8550: add QCrypto nodes")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index eac8de4005d82f246bc50f64f09515631d895c99..ac3e00ad417719be2885d76d3197f96137848337 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -1957,6 +1957,8 @@ cryptobam: dma-controller@1dc4000 {
>   			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
>   			#dma-cells = <1>;
>   			qcom,ee = <0>;
> +			qcom,num-ees = <4>;
> +			num-channels = <20>;
>   			qcom,controlled-remotely;
>   			iommus = <&apps_smmu 0x480 0x0>,
>   				 <&apps_smmu 0x481 0x0>;
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 4/8] arm64: dts: qcom: sm8650: Add missing properties for cryptobam

[Neil Armstrong]

On 12/02/2025 18:03, Stephan Gerhold wrote:
> num-channels and qcom,num-ees are required for BAM nodes without clock,
> because the driver cannot ensure the hardware is powered on when trying to
> obtain the information from the hardware registers. Specifying the node
> without these properties is unsafe and has caused early boot crashes for
> other SoCs before [1, 2].
> 
> Add the missing information from the hardware registers to ensure the
> driver can probe successfully without causing crashes.
> 
> [1]: https://lore.kernel.org/r/CY01EKQVWE36.B9X5TDXAREPF@fairphone.com/
> [2]: https://lore.kernel.org/r/20230626145959.646747-1-krzysztof.kozlowski@linaro.org/
> 
> Cc: stable@vger.kernel.org
> Fixes: 10e024671295 ("arm64: dts: qcom: sm8650: add interconnect dependent device nodes")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..c8a2a76a98f000610f33cd1ada82eebd6ae95343 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -2533,6 +2533,8 @@ cryptobam: dma-controller@1dc4000 {
>   				 <&apps_smmu 0x481 0>;
>   
>   			qcom,ee = <0>;
> +			qcom,num-ees = <4>;
> +			num-channels = <20>;
>   			qcom,controlled-remotely;
>   		};
>   
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>