| Message ID | 20241029110732.1977064-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Side MDIO Support for LAN937x Switches | expand |
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@pengutronix.de> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > Reviewed-by: Andrew Lunn <andrew@lunn.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@abcd { ethernet-phy@2 { reg = <2>; }; }; child_bus: mdio@efgh { 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). 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. 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. 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.
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@pengutronix.de> > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > > Reviewed-by: Andrew Lunn <andrew@lunn.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@abcd { > ethernet-phy@2 { > reg = <2>; > }; > }; > > child_bus: mdio@efgh { > 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.
> > 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@abcd { > > ethernet-phy@2 { > > reg = <2>; > > }; > > }; > > > > child_bus: mdio@efgh { > > 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. This is not what i was expecting, especially from mv88e6xxx perspective. The older generation of devices had the PHYs available on the 'host bus', as well as the 'child bus', using a 1:1 address mapping. You could in theory even skip the 'child bus' and list the PHYs on the 'host bus' and phy-handle would make it work. However i see from a later comment that does not work here, you need some configuration done over SPI, which mv88e6xx does not need. > > > 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. You are talking about: ds->user_mii_bus->irq[phy] = irq; in ksz_irq_phy_setup. I naively expect 'phy' to be the 'reg' value in DT, and the 'dev' value which passed to mdiobus_read_nested(bus, dev, reg) ? Andrew
On Tue, Oct 29, 2024 at 09:35:48PM +0100, Andrew Lunn wrote: > > > 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@abcd { > > > ethernet-phy@2 { > > > reg = <2>; > > > }; > > > }; > > > > > > child_bus: mdio@efgh { > > > 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. > > This is not what i was expecting, especially from mv88e6xxx > perspective. The older generation of devices had the PHYs available on > the 'host bus', as well as the 'child bus', using a 1:1 address > mapping. You could in theory even skip the 'child bus' and list the > PHYs on the 'host bus' and phy-handle would make it work. However i > see from a later comment that does not work here, you need some > configuration done over SPI, which mv88e6xx does not need. > > > > > > 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. > > You are talking about: > > ds->user_mii_bus->irq[phy] = irq; > > in ksz_irq_phy_setup. > > I naively expect 'phy' to be the 'reg' value in DT, and the 'dev' > value which passed to mdiobus_read_nested(bus, dev, reg) ? Yes, this is correct. This can be implemented purely by parsing the devicetree. Based on previous experience, I expected you to suggest me to implement the validation so i jumped directly to a part of this step. Should I implement it based on the devicetree information and validate based on HW strapping?
> Yes, this is correct. This can be implemented purely by parsing the > devicetree. Based on previous experience, I expected you to suggest me > to implement the validation so i jumped directly to a part of this step. > > Should I implement it based on the devicetree information and validate > based on HW strapping? I assume you need to list the PHYs in DT because there is not a 1:1 mapping of port number to MDIO address? If there is a 1:1 mapping, port 1 has a PHY at MDIO address 1, the DSA core will connect the PHYs for you, there is no need to list them in DT. But if strapping can put them anywhere on the bus, this is not true, and then you need the phy-handle. I would then suggest DT described the hardware, the PHYs are listed on the correct MDIO address, and you validate the hardware matches DT just as a sanity check. Andrew
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