Re: [PATCH 1/2] dt-bindings: crypto: ice: Document sm8150 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: ice: sm8150 standalone ICE conflicts with qcom,ufs.yaml
- [Low] dt-bindings: crypto: ice: qcom,sm8150-inline-crypto-engine out of sort order
--

commit 8ecc5836840659392f33486ed20f5aa5057a1a4c
Author: Andrew Gigena <mail@andrewgigena.dev>

dt-bindings: crypto: ice: Document sm8150 inline crypto engine

This commit documents the qcom,sm8150-inline-crypto-engine compatible string
in Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml.

> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> index 23bf98c2b9fa0..1918f18fe8b10 100644
> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> @@ -23,6 +23,7 @@ properties:
>            - qcom,sa8775p-inline-crypto-engine
>            - qcom,sc7180-inline-crypto-engine
>            - qcom,sc7280-inline-crypto-engine
> +          - qcom,sm8150-inline-crypto-engine

[Severity: Medium]
Does adding a standalone ICE compatible for SM8150 conflict with the existing
qcom,sm8150-ufshc binding in Documentation/devicetree/bindings/ufs/qcom,ufs.yaml?

On SM8150, the inline crypto engine is associated with the UFS controller
(GCC only provides GCC_UFS_PHY_ICE_CORE_CLK, with no SDCC ICE clock in
include/dt-bindings/clock/qcom,gcc-sm8150.h).

In Documentation/devicetree/bindings/ufs/qcom,ufs.yaml, qcom,sm8150-ufshc is
defined as using the legacy embedded ICE representation:

    if:
      properties:
        compatible:
          contains:
            enum:
              - qcom,msm8996-ufshc
              - qcom,sdm845-ufshc
              - qcom,sm6350-ufshc
              - qcom,sm8150-ufshc
    then:
      properties:
        clocks:
          minItems: 9
          maxItems: 9
        clock-names:
          items:
            - const: core_clk
            ...
            - const: ice_core_clk
        reg:
          minItems: 2
        reg-names:
          items:
            - const: std
            - const: ice

In the same qcom,ufs.yaml schema, the conditional rule for nodes using the
standalone ICE binding via qcom,ice requires at most 1 reg entry and 8
clocks, which is mutually exclusive with the qcom,sm8150-ufshc constraints:

    if:
      required:
        - qcom,ice
    then:
      properties:
        clocks:
          maxItems: 8
        clock-names:
          maxItems: 8
        reg:
          maxItems: 1
        reg-names:
          maxItems: 1

When ufshc@1d84000 in arch/arm64/boot/dts/qcom/sm8150.dtsi is converted to a
standalone qcom,sm8150-inline-crypto-engine node referenced via qcom,ice and
the embedded ice register range and ice_core_clk are removed from the UFS
node, won't this fail dtbs_check against qcom,ufs.yaml and break Device Tree
ABI compatibility with existing kernels expecting the embedded ICE registers
and clock on qcom,sm8150-ufshc?

>            - qcom,shikra-inline-crypto-engine

[Severity: Low]
This isn't a bug, but the compatible enum in properties:compatible in
Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml is
sorted alphabetically. Should qcom,sm8150-inline-crypto-engine be placed
after qcom,shikra-inline-crypto-engine and before
qcom,sm8450-inline-crypto-engine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-sm8150-ice-rfc-v1-0-5737b6c2ecc0@andrewgigena.dev?part=1