Re: [PATCH net-next 3/4] dt-bindings: net: qcom,ipq9574-ppe: Add IPQ5332 support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The IPQ5332 port limits (`"^ethernet-port@[3-6]$": false` and
  `reg: maximum: 2`) apply only under the `ethernet-ports` container…

--

dt-bindings: net: qcom,ipq9574-ppe: Add IPQ5332 support

This adds the qcom,ipq5332-ppe compatible to the IPQ9574 PPE binding. It
adds an optional GCC sleep clock and a second EDMA reset, and lowers the
minimum interconnect and EDMA interrupt counts. New per-SoC if/then blocks
constrain those counts, and limit IPQ5332 to ethernet-port@1 and
ethernet-port@2.

Link: https://lore.kernel.org/r/20240829082830.56959-7-quic_varada@xxxxxxxxxxx
Link: https://lore.kernel.org/r/20240110112059.2498-3-quic_luoj@xxxxxxxxxxx

> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml b/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
> index 1a78e2c82b90c..29dd34210ac89 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml

[ ... ]

> @@ -238,6 +255,65 @@ required:
>  
>  allOf:
>    - $ref: ethernet-switch.yaml
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,ipq5332-ppe
> +    then:

[ ... ]

> +        ethernet-ports:
> +          patternProperties:
> +            "^ethernet-port@[1-2]$":
> +              properties:
> +                reg:
> +                  maximum: 2
> +            "^ethernet-port@[3-6]$": false

[Severity: Low]
Is the IPQ5332 port limit enforced when the node uses "ports" as the
container name instead of "ethernet-ports"?

The new constraints only apply under ethernet-ports. The binding also
pulls in ethernet-switch.yaml through allOf, and that schema accepts
either container name:

Documentation/devicetree/bindings/net/ethernet-switch.yaml:
    patternProperties:
      '^(ethernet-)?ports$':
        ...
        patternProperties:
          '^(ethernet-)?port@[0-9a-f]+$':
            type: object
    ...
    oneOf:
      - required:
          - ports
      - required:
          - ethernet-ports

The referenced schema evaluates "ports", so the top-level
unevaluatedProperties: false in qcom,ipq9574-ppe.yaml does not reject it.
The PPE binding also does not list ethernet-ports in its required list.

So an IPQ5332 node like this would seem to pass dt_binding_check:

    ports {
        #address-cells = <1>;
        #size-cells = <0>;
        ethernet-port@3 {
            reg = <3>;
        };
    };

That would not match the commit message, which says IPQ5332 takes "only
ethernet-port@1 and ethernet-port@2".

The same gap already lets nodes skip the shared per-port clocks and resets
requirements for IPQ9574. The earlier patch in this series that tightened
the port node constraints also covered only ethernet-ports.

Would it make sense to forbid ports (for example ports: false), require
ethernet-ports, or apply the constraints to both spellings?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005152915.101780-1-mail%40tk154.de