Re: [PATCH 2/2] arm64: dts: qcom: sm8350: Add the inline crypto engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: crypto: qcom,inline-crypto-engine: inverted if-not condition for SM8350
- [Low] arm64: dts: qcom: sm8350: ICE reg size 0x11000 instead of 0x18000
--

commit da3fc875e05c7271bcc6b15fe532d10c5104a0b1
Author: Demeter Barna <me@fromdisposition.org>

arm64: dts: qcom: sm8350: Add the inline crypto engine

This commit adds the UFS inline crypto engine (ICE) device node at 0x1d88000
to the SM8350 device tree and links it to the UFS host controller via the
qcom,ice property.

> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> index 55b32050ce9c8..2b258c4b4fa71 100644
> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi

[ ... ]

> @@ -1807,6 +1809,17 @@ ufs_mem_phy: phy@1d87000 {
>  			status = "disabled";
>  		};
>  
> +		ice: crypto@1d88000 {
> +			compatible = "qcom,sm8350-inline-crypto-engine",
> +				     "qcom,inline-crypto-engine";

[Severity: Medium]
In the accompanying binding commit 4bdcd3be438d ("dt-bindings: crypto:
qcom,inline-crypto-engine: Add SM8350"), the commit message states:

  Document the compatible and require the power domain and clock names
  for it, as for SM8450.

However, in Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml,
qcom,sm8350-inline-crypto-engine was added to the negated compatible enum
under allOf:

  # Do not extend the list.
  # Legacy SoCs are allowed for single clock.
  # New SoCs must provide both clocks and power domains.
  allOf:
    - if:
        not:
          properties:
            compatible:
              contains:
                enum:
                  ...
                  - qcom,sc7280-inline-crypto-engine
                  - qcom,sm8350-inline-crypto-engine
                  - qcom,sm8450-inline-crypto-engine
                  ...
      then:
        required:
          - power-domains
          - clock-names
        properties:
          clocks:
            minItems: 2
          clock-names:
            minItems: 2

Because that condition uses if: not: (introduced in commit 3360fa760425 to
exempt legacy SoCs), doesn't adding qcom,sm8350-inline-crypto-engine to that
enum exempt SM8350 from requiring power-domains, clock-names, and two clocks
during dtbs_check validation instead of requiring them?

> +			reg = <0 0x01d88000 0 0x11000>;

[Severity: Low]
Should the reg size in ice: crypto@1d88000 be 0x18000 instead of 0x11000?

When qcom_ice_probe() in drivers/soc/qcom/ice.c maps this resource via
devm_platform_ioremap_resource(), 0x11000 covers the 0x8000 ICE region plus
only 0x9000 bytes of the HWKM region from the downstream vendor device tree,
leaving the upper 0x7000 bytes of the 0x10000 HWKM block unmapped.

Other upstream Qualcomm SoCs with ICE v3.2.0+ and HWKM at 0x01d88000, such as
SC7280 in arch/arm64/boot/dts/qcom/kodiak.dtsi and SM8450 in
arch/arm64/boot/dts/qcom/sm8450.dtsi (as well as SM8550, SM8650, SM8750,
SA8775P, and QCS8300), use:

  reg = <0 0x01d88000 0 0x18000>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-b4-sm8350-ice-v1-0-f55e6ea8e1ca@fromdisposition.org?part=2