Message-ID: <ZyDe_ObZ-laVk8c2@pengutronix.de>
Date: Tue, 29 Oct 2024 14:11:24 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Vladimir Oltean <olteanv@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>,
	Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Rob Herring <robh+dt@...nel.org>, Rob Herring <robh@...nel.org>,
	kernel@...gutronix.de, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
	"Russell King (Oracle)" <linux@...linux.org.uk>,
	devicetree@...r.kernel.org, Marek Vasut <marex@...x.de>
Subject: Re: [PATCH net-next v2 2/5] dt-bindings: net: dsa: ksz: add
 mdio-parent-bus property for internal MDIO

On Tue, Oct 29, 2024 at 02:31:07PM +0200, Vladimir Oltean wrote:
> On Tue, Oct 29, 2024 at 12:07:29PM +0100, Oleksij Rempel wrote:
> > Introduce `mdio-parent-bus` property in the ksz DSA bindings to
> > reference the parent MDIO bus when the internal MDIO bus is attached to
> > it, bypassing the main management interface.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> > Reviewed-by: Rob Herring (Arm) <robh@...nel.org>
> > Reviewed-by: Andrew Lunn <andrew@...n.ch>
> > ---
> >  .../devicetree/bindings/net/dsa/microchip,ksz.yaml       | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > index a4e463819d4d7..121a4bbd147be 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > @@ -84,6 +84,15 @@ properties:
> >    mdio:
> >      $ref: /schemas/net/mdio.yaml#
> >      unevaluatedProperties: false
> > +    properties:
> > +      mdio-parent-bus:
> > +        $ref: /schemas/types.yaml#/definitions/phandle
> > +        description:
> > +          Phandle pointing to the MDIO bus controller connected to the
> > +          secondary MDIO interface. This property should be used when
> > +          the internal MDIO bus is accessed via a secondary MDIO
> > +          interface rather than the primary management interface.
> > +
> >      patternProperties:
> >        "^ethernet-phy@[0-9a-f]$":
> >          type: object
> > -- 
> > 2.39.5
> > 
> 
> I'm not saying whether this is good or bad, I'm just worried about
> mixing quantities having different measurement units into the same
> address space.
> 
> Just like in the case of an mdio-mux, there is no address space isolation
> between the parent bus and the child bus. AKA you can't have this,
> because there would be clashes:
> 
> 	host_bus: mdio@...d {
> 		ethernet-phy@2 {
> 			reg = <2>;
> 		};
> 	};
> 
> 	child_bus: mdio@...h {
> 		mdio-parent-bus = <&host_bus>;
> 
> 		ethernet-phy@2 {
> 			reg = <2>;
> 		};
> 	};
> 
> But there is a big difference. With an mdio-mux, you could statically
> detect address space clashes by inspecting the PHY addresses on the 2
> buses. But with the lan937x child MDIO bus, in this design, you can't,
> because the "reg" values don't represent MDIO addresses, but switch port
> numbers (this is kind of important, but I don't see it mentioned in the
> dt-binding).

In current state, the driver still require properly configured addresses
in the devicetree. So, it will be visible in the DT.

> These are translated by lan937x_create_phy_addr_map() using
> the CASCADE_ID/VPHY_ADD pin strapping information read over SPI.
> I.e. with the same device tree, you may or may not have address space
> clashes depending on pin strapping. No way to tell.

The PHY address to port mapping in the driver is needed to make the
internal switch interrupt controller assign interrupts to proper PHYs.

It would be possible to assign interrupts per devicetree, but the driver
was previously implemented to not need it, so i decided to follow this
design pattern.

The driver can be extended to validate DT addresses, but I was not sure
that my current decisions are the way to go.

> Have you considered putting the switch's internal PHYs directly under
> the host MDIO bus node, with their 'real' MDIO bus computed statically
> by the DT writer based on the pin straps? Yes, I'm aware that this means
> different pin straps mean different device trees.

Yes, i tried this. In this case, the host MDIO registration procedure
will fail to find the PHYs, because they are not accessible before
switch driver initialization. I had in mind some different variants to
handle it, like:
- use compatible property in the PHY nodes in the host MDIO node.
- trigger MDIO re-scan from the switch
- mimic the MDIO mux. 

The last variant seems to be more or less better fit for this use case.

> Under certain circumstances I could understand this dt-binding design
> with an mdio-parent-bus, like for example if the MDIO addresses at which
> the internal PHYs respond would be configurable over SPI, from the switch
> registers. But I'm not led to believe that here, this is the case.

ack.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists