Re: [PATCH net-next v3 2/8] dt-bindings: net: Add SoC-e SWIP MDIO controller

[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] In soce,swip-mdio.yaml the MDIO controller owns the whole
  32-bit 'control' register (<0x200 0x4>), which the binding calls the…
- [Low] The only compatible is 'soce,swip-mdio-23-02', and neither the
  binding nor the commit message explains the '23-02' suffix.

--

dt-bindings: net: Add SoC-e SWIP MDIO controller

This adds a devicetree binding for the MDIO controller built into SoC-e
SWIP Ethernet switch IP cores. The controller has a data register region
and a shared transaction control and bus selector register. It is the
parent bus for an MDIO mux that reaches the external MDIO interfaces.

> diff --git a/Documentation/devicetree/bindings/net/soce,swip-mdio.yaml b/Documentation/devicetree/bindings/net/soce,swip-mdio.yaml
> new file mode 100644
> index 0000000000000..3cf4bde0508ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/soce,swip-mdio.yaml
> @@ -0,0 +1,47 @@

[ ... ]

> +description:
> +  MDIO controller integrated into SoC-e Ethernet switch FPGA IP cores.
> +  The controller accesses multiple external MDIO buses through a mux.
> +
> +$ref: mdio.yaml#
> +
> +properties:
> +  compatible:
> +    const: soce,swip-mdio-23-02

[Severity: Low]
What does the 23-02 suffix mean? Neither the binding nor the commit
message explains it. It looks like a YY-MM IP release tag. The DSA
driver commit in this series mentions testing an MRS 25.01 core.

This MDIO block is part of the larger SWIP switch. It sits at
0x200-0x20f inside the switch register window.
Documentation/devicetree/bindings/writing-bindings.rst says:

  For sub-blocks/components of bigger device (e.g. SoC blocks) use rather
  device-based compatible (e.g. SoC-based compatible), instead of custom
  versioning of that component.

Could the version scheme be documented, or could a device-based
compatible be used instead? As it stands, it isn't clear whether each
future IP release would need a new compatible.

> +
> +  reg:
> +    items:
> +      - description: MDIO parameter, write data, and read data registers
> +      - description: Shared MDIO transaction control and bus selection register

[Severity: Medium]
Is this the right way to describe the bus selector? This node claims the
whole 32-bit control register at <0x200 0x4>, bus selector field
included. The binding gives the mux no way to reach that field. There is
no #mux-control-cells and no mux child node, even though the description
says the buses are reached through a mux.

Later in the series, the example in "dt-bindings: net: dsa: Add SoC-e
SWIP switch" covers this with a sibling mdio-mux-mmioreg node:

Documentation/devicetree/bindings/net/dsa/soce,swip.yaml:
    mdio-mux@202 {
        ...
        reg = <0x202 0x2>;
        ...

That range is the upper halfword of the register that mdio@204 already
claims as control. The driver commit "net: mdio: Add SoC-e SWIP MDIO
controller driver" says the same thing: the selector is bits 26:16, and
a generic MMIO MDIO mux manages it through the upper 16-bit halfword.

The result is two sibling nodes with overlapping reg ranges for one
hardware register. This only works because neither driver requests its
region exclusively. soce_mdio_iomap() in drivers/net/mdio/mdio-soce.c
does:

    return devm_ioremap(dev, res.start, resource_size(&res));

mdio_mux_mmioreg_switch_fn() also does a plain ioremap() on every
switch. If either driver moved to devm_platform_ioremap_resource(),
probe would fail with -EBUSY.

Nothing fails with the current series. There is no -EBUSY today. The
mux's 16-bit read-modify-write and the 32-bit read-modify-write in
soce_mdio_start() are serialized: mdio_mux_read() and mdio_mux_write()
hold the parent bus mdio_lock around both switch_fn() and the parent
access.

The concern is the DT ABI. Once this binding is merged, moving to a
description without overlapping ranges would break compatibility. Two
options would avoid the overlap:

  - make this controller a mux provider with #mux-control-cells
  - have the controller register its child buses itself

Would it be better to choose one of these before the binding is merged?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-devel-vstrassheim-soce-dsa-ml-v3-0-ddebafcb9ba7%40linutronix.de