Re: [PATCH net-next v3 2/9] dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says Shikra's EMAC needs an OPP table whose
  required-opps vote VDD_CX to SVS while the NOC clocks are enabled.
- [Low] The commit message says the new if/else block constrains Shikra
  "while leaving existing compatibles unchanged".

--

dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

This commit adds the qcom,shikra-ethqos compatible to the qcom,ethqos and
snps,dwmac bindings. It allows up to six clocks by adding axi-noc and
pcie-tile-axi-noc, and adds the operating-points-v2 and opp-table
properties. A new if/else block requires Shikra to have exactly six clocks
and an OPP table.

> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> index 423959cb928d9..1167f11adbf30 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml

[ ... ]

> @@ -81,6 +83,11 @@ properties:
>        - const: cpu-mac
>        - const: mac-mem
>  
> +  operating-points-v2: true
> +
> +  opp-table:
> +    type: object
> +
>    phys: true
>  
>    phy-names:
> @@ -92,6 +99,46 @@ required:
>    - clock-names
>    - reg-names
>  
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,shikra-ethqos
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 6
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: pclk
> +            - const: ptp_ref
> +            - const: rgmii
> +            - const: axi-noc
> +            - const: pcie-tile-axi-noc
> +        operating-points-v2:
> +          description: |
> +            The OPP table specifies the rates for the NOC access
> +            clocks in "stmmaceth", "axi-noc", "pcie-tile-axi-noc"
> +            order.
> +      required:
> +        - operating-points-v2

[Severity: Medium]
Should power-domains also be required for Shikra here?

The commit message says the OPP table carries required-opps "to vote
VDD_CX to SVS when the NOC clocks are enabled". That vote only happens if
the device has a power domain attached. Here, power-domains is optional and
only comes in through the snps,dwmac.yaml reference (maxItems: 1).

If power-domains is missing, genpd never attaches the device, so
required_devs[0] stays NULL. _set_required_opps() then skips the entry and
does not return an error:

drivers/opp/core.c:_set_required_opps() {
    ...
		if (devs[index]) {
			required_opp = opp ? opp->required_opps[index] : NULL;

			ret = _set_opp_level(devs[index], required_opp);
    ...
}

As a result, a Shikra node with six clocks and an OPP table but no
power-domains passes dt_binding_check. The driver's ethqos_clks_config()
path through qcom_ethqos_set_opp() still succeeds, but CX is never raised
before the NOC clocks run at their OPP rates.

Other Shikra devices that vote CX through required-opps pair the OPP table
with the power domain. For example, sdhc_1 in
arch/arm64/boot/dts/qcom/shikra.dtsi has:

			power-domains = <&rpmpd RPMPD_VDDCX>;
			operating-points-v2 = <&sdhc1_opp_table>;

Could power-domains be added to the required list for Shikra? Could the
operating-points-v2 description also state that the OPP entries need
required-opps pointing at the CX domain?

> +    else:
> +      properties:
> +        clocks:
> +          maxItems: 4
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: pclk
> +            - const: ptp_ref
> +            - enum:
> +                - rgmii
> +                - phyaux
> +

[Severity: Low]
The commit message says the if/else block leaves "existing compatibles
unchanged". Is that accurate?

operating-points-v2 and opp-table are added to the top-level properties, so
they apply to every compatible. This else branch only re-limits clocks and
clock-names.

Before this patch, neither qcom,ethqos.yaml nor snps,dwmac.yaml defined
these properties, so unevaluatedProperties: false rejected them. Now nodes
for qcs404, qcs615, sa8775p, qcs8300, sc8280xp and sm8150 pass validation
with an OPP table, even though those platforms have no NOC clocks.

Should the else branch also include something like this?

        operating-points-v2: false
        opp-table: false

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com