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

[Vasilij Strassheim]

On Sun, 2026-09-27 at 12:28 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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.

I will remove this suffix. For a child of MFD "soce,swip-mdio" is
enough.

> 
> > +
> > +  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?
> 

Yes. I will address this in the MFD rework.

The MFD parent will register both child devices, with mdio-parent-bus
connecting the mux to the controller.

As already mentioned in other thread, this overlap is wrong.
The MDIO controller will use only the lower 16-bit half of the control
register through readw()/writew(). The sibling mdio-mux-mmioreg node
will use the upper half, so the resources will no longer overlap.

pw-bot: cr