Re: [PATCH] dt-bindings: net: snps,dwmac: allow stmmaceth-ocp reset name

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The SoCFPGA-only reset name "stmmaceth-ocp" is added
  unconditionally to the *generic* snps,dwmac.yaml second-reset slot…

Pre-existing issues:
- [Low] After this patch,
  Documentation/devicetree/bindings/net/altr,socfpga-stmmac.yaml…

--

dt-bindings: net: snps,dwmac: allow stmmaceth-ocp reset name

The patch relaxes the second entry of reset-names in the generic
snps,dwmac.yaml schema so that "stmmaceth-ocp" is accepted alongside
"ahb", and removes the Arria10 TODO note from the Altera SoCFPGA DWMAC
binding description.

> diff --git a/Documentation/devicetree/bindings/net/altr,socfpga-stmmac.yaml b/Documentation/devicetree/bindings/net/altr,socfpga-stmmac.yaml
> index db93436c96fa..d8f90d36d246 100644
> --- a/Documentation/devicetree/bindings/net/altr,socfpga-stmmac.yaml
> +++ b/Documentation/devicetree/bindings/net/altr,socfpga-stmmac.yaml
> @@ -13,8 +13,6 @@ description:
>    This binding describes the Altera SOCFPGA SoC implementation of the
>    Synopsys DWMAC for the Cyclone5, Arria5, Stratix10, Agilex5 and Agilex7
>    families of chips.
> -  # TODO: Determine how to handle the Arria10 reset-name, stmmaceth-ocp, that
> -  # does not validate against net/snps,dwmac.yaml.

[Severity: Low]
This isn't a bug introduced by this patch, but after the TODO is removed
this file no longer mentions Arria10 anywhere, while the family list in
the same description block still reads:

  Synopsys DWMAC for the Cyclone5, Arria5, Stratix10, Agilex5 and Agilex7
  families of chips.

The compatible documented in this same file, altr,socfpga-stmmac-a10-s10,
is the string used by the Arria10 gmac nodes in
arch/arm/boot/dts/intel/socfpga/socfpga_arria10.dtsi.  Could Arria10 be
added to that family list along with dropping the TODO?

> 
>  select:
>    properties:
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 2449311c6d28..c7f1a7c11c8d 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -174,7 +174,7 @@ properties:
>      minItems: 1
>      items:
>        - description: GMAC stmmaceth reset
> -      - description: AHB reset
> +      - description: AHB or OCP reset
>  
>    reset-names:
>      oneOf:
> @@ -182,7 +182,7 @@ properties:
>            - enum: [stmmaceth, ahb]
>        - items:
>            - const: stmmaceth
> -          - const: ahb
> +          - enum: [ahb, stmmaceth-ocp]

[Severity: Medium]
Should this new name be gated on the Altera compatible instead of being
allowed for every DWMAC binding?

"stmmaceth-ocp" only has one in-tree consumer,
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:socfpga_dwmac_probe():

	dwmac->stmmac_ocp_rst = devm_reset_control_get_optional(dev, "stmmaceth-ocp");

The generic parser, stmmac_probe_config_dt() in stmmac_platform.c, only
ever asks for the second reset by the name "ahb":

	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
							&pdev->dev, "ahb");

Since every other DWMAC binding (mediatek, rockchip, starfive, st, qcom,
...) $refs snps,dwmac.yaml, a device tree for one of those platforms can
now write reset-names = "stmmaceth", "stmmaceth-ocp" and still pass
dt_binding_check, while no driver on those platforms looks that name up,
so that reset is never deasserted and validation reports nothing.  Would
an if/then on the compatible be preferable?  snps,dwmac.yaml already
carries such blocks (for example the compatible-gated snps,tso: false
block), and altr,socfpga-stmmac.yaml already has an allOf with an
if: on altr,socfpga-stmmac-agilex5, which looks like the natural place to
constrain the Arria10 (altr,socfpga-stmmac-a10-s10) reset naming.

> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -174,7 +174,7 @@ properties:
>      minItems: 1
>      items:
>        - description: GMAC stmmaceth reset
> -      - description: AHB reset
> +      - description: AHB or OCP reset

[Severity: Medium]
Related to the above: this description now covers two different resets
with different acquisition semantics in one slot.  The AHB reset is taken
as a shared control via devm_reset_control_get_optional_shared() in
stmmac_platform.c, while the OCP reset is taken as an exclusive control
via devm_reset_control_get_optional() in dwmac-socfpga.c.  Would separate
per-compatible descriptions make the contract clearer?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909080559.2028-1-muhammad.nazim.amirul.nazle.asmade%40altera.com