| Message ID | 20220818143250.2797111-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5fbb08eb7f945c7e8896ea39f03143ce66dfa4c7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode | expand |
On Thu, Aug 18, 2022 at 05:32:50PM +0300, Vladimir Oltean wrote: > DSA has multiple ways of specifying a MAC connection to an internal PHY. > One requires a DT description like this: > > port@0 { > reg = <0>; > phy-handle = <&internal_phy>; > phy-mode = "internal"; > }; > > (which is IMO the recommended approach, as it is the clearest > description) > > but it is also possible to leave the specification as just: > > port@0 { > reg = <0>; > } > > and if the driver implements ds->ops->phy_read and ds->ops->phy_write, > the DSA framework "knows" it should create a ds->slave_mii_bus, and it > should connect to a non-OF-based internal PHY on this MDIO bus, at an > MDIO address equal to the port address. > > There is also an intermediary way of describing things: > > port@0 { > reg = <0>; > phy-handle = <&internal_phy>; > }; > > In case 2, DSA calls phylink_connect_phy() and in case 3, it calls > phylink_of_phy_connect(). In both cases, phylink_create() has been > called with a phy_interface_t of PHY_INTERFACE_MODE_NA, and in both > cases, PHY_INTERFACE_MODE_NA is translated into phy->interface. > > It is important to note that phy_device_create() initializes > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the > PHY. > > All this means that in order to maintain compatibility with device tree > blobs where the phy-mode property is missing, we need to allow the > "gmii" phy-mode and treat it as "internal". > > Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320 > Reported-by: Craig McQueen <craig@mcqueen.id.au> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/microchip/ksz_common.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> Some quick grepping shows at least a few other drivers which do not set PHY_INTERFACE_MODE_GMII for their ports with internal PHY: bcm_sf2 ar9331 (*) lantiq_gswip Should these be "fixed" too? Or only if somebody reports a regression? (*) I note that ar9331 ought not to rely on DSA workarounds, per your other patchset, so I there is actually no need to "fix" that one, since the new validation you are introducing will require a phy-mode to be specified for those switches' ports anyway. > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index ed7d137cba99..7461272a6d41 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -803,9 +803,15 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port, > if (dev->info->supports_rgmii[port]) > phy_interface_set_rgmii(config->supported_interfaces); > > - if (dev->info->internal_phy[port]) > + if (dev->info->internal_phy[port]) { > __set_bit(PHY_INTERFACE_MODE_INTERNAL, > config->supported_interfaces); > + /* Compatibility for phylib's default interface type when the > + * phy-mode property is absent > + */ > + __set_bit(PHY_INTERFACE_MODE_GMII, > + config->supported_interfaces); > + } > > if (dev->dev_ops->get_caps) > dev->dev_ops->get_caps(dev, port, config); > -- > 2.34.1 >
> It is important to note that phy_device_create() initializes > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the > PHY. Is this actually a bug? With pure phylib, you should call one of the connect functions, which underneath calls phy_attach_direct() which has a phy_interface_t. So the default in practice does not matter. > All this means that in order to maintain compatibility with device tree > blobs where the phy-mode property is missing, we need to allow the > "gmii" phy-mode and treat it as "internal". of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is missing, which also suggests this is a bug. I wonder if we have any ports which actually rely on PHY_INTERFACE_MODE_GMII? Andrew
On Thu, Aug 18, 2022 at 03:06:44PM +0000, Alvin Šipraga wrote: > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> Thanks. > Some quick grepping shows at least a few other drivers which do not set > PHY_INTERFACE_MODE_GMII for their ports with internal PHY: > > bcm_sf2 > ar9331 (*) > lantiq_gswip > > Should these be "fixed" too? Or only if somebody reports a regression? > > (*) I note that ar9331 ought not to rely on DSA workarounds, per your > other patchset, so I there is actually no need to "fix" that one, since > the new validation you are introducing will require a phy-mode to be > specified for those switches' ports anyway. Note that my patch set which you are talking about https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/ only enforces phy-mode presence for CPU and DSA ports. Whereas internal PHYs are more typical on user ports. So the problem is equally applicable to ar9331, as long as it won't specify a phy-mode but rely on DSA's assumption that user ports with lacking DT descriptions want to connect to an internal PHY. IDK, if I had infinite time I'd go fix every driver. I don't know why some have GMII and some don't, and I'm not exactly keen to go study every driver's subtleties unless a problem is reported.
On Thu, Aug 18, 2022 at 05:13:51PM +0200, Andrew Lunn wrote: > > It is important to note that phy_device_create() initializes > > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use > > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we > > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the > > PHY. > > Is this actually a bug? > > With pure phylib, you should call one of the connect functions, which > underneath calls phy_attach_direct() which has a phy_interface_t. So > the default in practice does not matter. What do you consider "bug"? I think here is where phylink inherits what phy_device_create() set as default: int phylink_connect_phy(struct phylink *pl, struct phy_device *phy) { int ret; /* Use PHY device/driver interface */ if (pl->link_interface == PHY_INTERFACE_MODE_NA) { pl->link_interface = phy->interface; pl->link_config.interface = pl->link_interface; <- here } ret = phylink_attach_phy(pl, phy, pl->link_interface); <- here is the phy_attach_direct() if (ret < 0) return ret; ret = phylink_bringup_phy(pl, phy, pl->link_config.interface); if (ret) phy_detach(phy); return ret; } So yes, although you're right in that if you call phy_attach_direct() this doesn't happen, but if you go through phylink_connect_phy() I think it's expected that it will. > > All this means that in order to maintain compatibility with device tree > > blobs where the phy-mode property is missing, we need to allow the > > "gmii" phy-mode and treat it as "internal". > > of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is > missing, which also suggests this is a bug. > > I wonder if we have any ports which actually rely on > PHY_INTERFACE_MODE_GMII? What do you mean, you mean actual GMII wired to chip pinout?
On Thu, Aug 18, 2022 at 06:18:06PM +0300, Vladimir Oltean wrote: > On Thu, Aug 18, 2022 at 03:06:44PM +0000, Alvin Šipraga wrote: > > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > > Thanks. > > > Some quick grepping shows at least a few other drivers which do not set > > PHY_INTERFACE_MODE_GMII for their ports with internal PHY: > > > > bcm_sf2 > > ar9331 (*) > > lantiq_gswip > > > > Should these be "fixed" too? Or only if somebody reports a regression? > > > > (*) I note that ar9331 ought not to rely on DSA workarounds, per your > > other patchset, so I there is actually no need to "fix" that one, since > > the new validation you are introducing will require a phy-mode to be > > specified for those switches' ports anyway. > > Note that my patch set which you are talking about > https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/ > only enforces phy-mode presence for CPU and DSA ports. Whereas internal > PHYs are more typical on user ports. So the problem is equally > applicable to ar9331, as long as it won't specify a phy-mode but rely on > DSA's assumption that user ports with lacking DT descriptions want to > connect to an internal PHY. Thanks for the clarification. > > IDK, if I had infinite time I'd go fix every driver. I don't know why > some have GMII and some don't, and I'm not exactly keen to go study > every driver's subtleties unless a problem is reported. Yes, fair enough! Kind regards, Alvin
On Thu, Aug 18, 2022 at 05:13:51PM +0200, Andrew Lunn wrote: > > It is important to note that phy_device_create() initializes > > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use > > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we > > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the > > PHY. > > Is this actually a bug? > > With pure phylib, you should call one of the connect functions, which > underneath calls phy_attach_direct() which has a phy_interface_t. So > the default in practice does not matter. > > > All this means that in order to maintain compatibility with device tree > > blobs where the phy-mode property is missing, we need to allow the > > "gmii" phy-mode and treat it as "internal". > > of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is > missing, which also suggests this is a bug. > > I wonder if we have any ports which actually rely on > PHY_INTERFACE_MODE_GMII? Loads now. Florian contributed the code to phylink that detects when DSA initialises phylink with PHY_INTERFACE_MODE_NA, and then looks at phy->interface _before_ calling phy_attach_direct() - and this is how we end up with PHY_INTERFACE_MODE_GMII. See: commit 4904b6ea1f9dbf47107f50b1c502a22d0160712d Author: Florian Fainelli <f.fainelli@gmail.com> Date: Tue Dec 12 16:00:26 2017 -0800 net: phy: phylink: Use PHY device interface if N/A We may not always be able to resolve a correct phy_interface_t value before actually connecting to the PHY device, when that happens, just have phylink_connect_phy() utilize what the PHY device/driver provided. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> submitted for 4.16-rc1. DSA then used this in: commit aab9c4067d2389d0adfc9c53806437df7b0fe3d5 Author: Florian Fainelli <f.fainelli@gmail.com> Date: Thu May 10 13:17:36 2018 -0700 net: dsa: Plug in PHYLINK support ... for 4.18-rc1 to connect to its PHYs: static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr) { struct dsa_port *dp = dsa_slave_to_port(slave_dev); struct dsa_switch *ds = dp->ds; slave_dev->phydev = mdiobus_get_phy(ds->slave_mii_bus, addr); if (!slave_dev->phydev) { netdev_err(slave_dev, "no phy at %d\n", addr); return -ENODEV; } return phylink_connect_phy(dp->pl, slave_dev->phydev); } ... ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags); if (ret == -ENODEV) { /* We could not connect to a designated PHY or SFP, so use the * switch internal MDIO bus instead */ ret = dsa_slave_phy_connect(slave_dev, dp->index); if (ret) { netdev_err(slave_dev, "failed to connect to port %d: %d\n", dp->index, ret); phylink_destroy(dp->pl); return ret; } } which will be used when there is no phy-handle property. I extended the change in 4904b6ea1f9dbf47107f50b1c502a22d0160712d to also apply to fwnode setups in: commit a18e6521a7d95dae8c65b5b0ef6bbe624fbe808c Author: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Date: Fri Nov 19 16:28:06 2021 +0000 net: phylink: handle NA interface mode in phylink_fwnode_phy_connect() because we were ending up with DSA drivers using PHY_INTERFACE_MODE_NA inside phylink, and phylink has always special-cased that and drivers have taken it to mean "give me all interface modes that are supported" which is not what DSA should be using. It also gives a uniform and understandable behaviour from phylink for DSA, rather than having phylink behave one way (where no phy-mode and no phy-handle are specified) but in a completely different way when phy-handle without phy-mode is specified. It seemed to be the sensible thing to do, and Florian agreed at the time.
On 18/08/2022 16.32, Vladimir Oltean wrote: > DSA has multiple ways of specifying a MAC connection to an internal PHY. > One requires a DT description like this: > > port@0 { > reg = <0>; > phy-handle = <&internal_phy>; > phy-mode = "internal"; > }; > > (which is IMO the recommended approach, as it is the clearest > description) > > but it is also possible to leave the specification as just: > > port@0 { > reg = <0>; > } > > and if the driver implements ds->ops->phy_read and ds->ops->phy_write, > the DSA framework "knows" it should create a ds->slave_mii_bus, and it > should connect to a non-OF-based internal PHY on this MDIO bus, at an > MDIO address equal to the port address. > > There is also an intermediary way of describing things: > > port@0 { > reg = <0>; > phy-handle = <&internal_phy>; > }; Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts which sets the phy-mode but not the phy-handle: port@0 { reg = <0>; label = "lan1"; phy-mode = "internal"; }; And doing that in my case seems to fix things (I wouldn't know what phy-handle to point at anyway), so since we're still in development, I think I'll do that. But if I want to follow the new-world-order to the letter, should I also figure out a way to point at a phy-handle? > Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320 > Reported-by: Craig McQueen <craig@mcqueen.id.au> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> I've also tested this patch on top of v5.19 without altering my .dts, and that also seems to fix things, so I suppose you can add Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps") Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Thanks, Rasmus
On Fri, Aug 19, 2022 at 12:19:12PM +0200, Rasmus Villemoes wrote: > Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts > which sets the phy-mode but not the phy-handle: > > port@0 { > reg = <0>; > label = "lan1"; > phy-mode = "internal"; > }; Yeah, it looks like there's also that variation, curious. > And doing that in my case seems to fix things (I wouldn't know what > phy-handle to point at anyway), so since we're still in development, I > think I'll do that. But if I want to follow the new-world-order to the > letter, should I also figure out a way to point at a phy-handle? So if by "new world order" you mean https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/ then no, that patch set doesn't change the requirements for *user* ports (what you have here) but for CPU and DSA ports, where no phy-handle/ fixed-link/phy-mode means something entirely different than it means for user ports. To give you an idea of how things work for user ports. If a user port has a phy-handle, DSA will connect to that, irrespective of what OF-based MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is populated with a struct mii_bus by the driver. If it is, it connects in a non-OF based way to a PHY address equal to the port number. If ds->slave_mii_bus doesn't exist but the driver provides ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates ds->slave_mii_bus where its ops are the driver provided phy_read and phy_write, and it then does the same thing of connecting to the PHY in that non-OF based way. So to convert a driver that currently relies on DSA allocating the ds->slave_mii_bus, you need to allocate/register it yourself (using the of_mdiobus_* variants), and populate ds->slave_mii_bus with it. Look at lan937x_mdio_register() for instance. For existing device trees which connect in a non-OF based way, you still need to keep ds->ops->phy_read and ds->ops->phy_write, and let DSA create the ds->slave_mii_bus. The phy_read and phy_write can be the same between your MDIO bus ops and DSA's MDIO bus ops. > > Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support") > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320 > > Reported-by: Craig McQueen <craig@mcqueen.id.au> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > I've also tested this patch on top of v5.19 without altering my .dts, > and that also seems to fix things, so I suppose you can add > > Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps") > Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> So I don't intend to make you modify your device tree in this case, but there is something to be said about U-Boot compatibility. In U-Boot, with DM_DSA, I don't intend to support any unnecessary complexity and alternative ways of describing the same thing, so there, phy-mode and one of phy-handle or fixed-link are mandatory for all ports. And since U-Boot can pass its own device tree to Linux, it means Linux DSA drivers might need to gradually gain support for OF-based phy-handle on user ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c in the U-Boot source code, and try to work with his device tree format, as a starting point.
On 19/08/2022 12.57, Vladimir Oltean wrote: > On Fri, Aug 19, 2022 at 12:19:12PM +0200, Rasmus Villemoes wrote: >> Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts >> which sets the phy-mode but not the phy-handle: >> >> port@0 { >> reg = <0>; >> label = "lan1"; >> phy-mode = "internal"; >> }; > > Yeah, it looks like there's also that variation, curious. > >> And doing that in my case seems to fix things (I wouldn't know what >> phy-handle to point at anyway), so since we're still in development, I >> think I'll do that. But if I want to follow the new-world-order to the >> letter, should I also figure out a way to point at a phy-handle? > > So if by "new world order" you mean > https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/ > then no, that patch set doesn't change the requirements for *user* ports > (what you have here) but for CPU and DSA ports, where no phy-handle/ > fixed-link/phy-mode means something entirely different than it means for > user ports. No, I meant the new world order as of the things merged into v5.19. Which does seem to require an explicit phy-mode, and I'm happy to add that. > To give you an idea of how things work for user ports. If a user port > has a phy-handle, DSA will connect to that, irrespective of what OF-based > MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is > populated with a struct mii_bus by the driver. If it is, it connects in > a non-OF based way to a PHY address equal to the port number. If > ds->slave_mii_bus doesn't exist but the driver provides > ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates > ds->slave_mii_bus where its ops are the driver provided phy_read and > phy_write, and it then does the same thing of connecting to the PHY in > that non-OF based way. Thanks, that's quite useful. From quick grepping, it seems that ksz9567 currently falls into the latter category? > So to convert a driver that currently relies on DSA allocating the > ds->slave_mii_bus, you need to allocate/register it yourself (using the > of_mdiobus_* variants), and populate ds->slave_mii_bus with it. Look at > lan937x_mdio_register() for instance. > > For existing device trees which connect in a non-OF based way, you still > need to keep ds->ops->phy_read and ds->ops->phy_write, and let DSA > create the ds->slave_mii_bus. The phy_read and phy_write can be the same > between your MDIO bus ops and DSA's MDIO bus ops. > >>> Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support") >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320 >>> Reported-by: Craig McQueen <craig@mcqueen.id.au> >>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >> >> I've also tested this patch on top of v5.19 without altering my .dts, >> and that also seems to fix things, so I suppose you can add >> >> Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps") >> Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > > So I don't intend to make you modify your device tree in this case, No, but it's actually easier for me to just do that rather than carry an extra patch until the mainline fix hits 5.19.y. but > there is something to be said about U-Boot compatibility. In U-Boot, > with DM_DSA, I don't intend to support any unnecessary complexity and > alternative ways of describing the same thing, so there, phy-mode and > one of phy-handle or fixed-link are mandatory for all ports. OK. I suppose that means the linux driver for the ksz9477 family should learn to check if there's an "mdio" subnode and if so populate ds->slave_mii_bus, but unlike lan937x, absence of that node should not be fatal? And since > U-Boot can pass its own device tree to Linux, it means Linux DSA drivers > might need to gradually gain support for OF-based phy-handle on user > ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c > in the U-Boot source code, and try to work with his device tree format, > as a starting point. Hm. It does seem like that driver has the mdio bus optional (as in, probe doesn't fail just because the subnode isn't present). But I'm curious why ksz_probe_mdio() looks for a subnode called "mdios" rather than "mdio". Tim, just a typo? Thanks, Rasmus
On Fri, Aug 19, 2022 at 01:47:51PM +0200, Rasmus Villemoes wrote: > > To give you an idea of how things work for user ports. If a user port > > has a phy-handle, DSA will connect to that, irrespective of what OF-based > > MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is > > populated with a struct mii_bus by the driver. If it is, it connects in > > a non-OF based way to a PHY address equal to the port number. If > > ds->slave_mii_bus doesn't exist but the driver provides > > ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates > > ds->slave_mii_bus where its ops are the driver provided phy_read and > > phy_write, and it then does the same thing of connecting to the PHY in > > that non-OF based way. > > Thanks, that's quite useful. From quick grepping, it seems that ksz9567 > currently falls into the latter category? So it would appear. > No, but it's actually easier for me to just do that rather than carry an > extra patch until the mainline fix hits 5.19.y. Whatever suits you best. > > there is something to be said about U-Boot compatibility. In U-Boot, > > with DM_DSA, I don't intend to support any unnecessary complexity and > > alternative ways of describing the same thing, so there, phy-mode and > > one of phy-handle or fixed-link are mandatory for all ports. > > OK. I suppose that means the linux driver for the ksz9477 family should > learn to check if there's an "mdio" subnode and if so populate > ds->slave_mii_bus, but unlike lan937x, absence of that node should not > be fatal? Yes. > > U-Boot can pass its own device tree to Linux, it means Linux DSA drivers > > might need to gradually gain support for OF-based phy-handle on user > > ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c > > in the U-Boot source code, and try to work with his device tree format, > > as a starting point. > > Hm. It does seem like that driver has the mdio bus optional (as in, > probe doesn't fail just because the subnode isn't present). But I'm > curious why ksz_probe_mdio() looks for a subnode called "mdios" rather > than "mdio". Tim, just a typo? No, definitely not a typo. I have to explain this for what seems like the millionth time already, but the idea with an "mdios" container was copied from the sja1105 driver, where there are actually multiple MDIO buses. Documentation/devicetree/bindings/net/mdio.yaml wants the $nodename to have the "^mdio(@.*)?" pattern, and: - if you use "mdio" you can have a single node - if you don't put the MDIO nodes under a container with an #address-cells != 0, you can't use the "mdio@something" syntax https://lore.kernel.org/all/20210621234930.espjau5l2t5dr75y@skbuf/T/#me43cf6b1976a3c3aec8357f19ab967f98eea1f73 What I'm actually less clear about is whether ksz9477 actually needs this. Luckily U-Boot should have less compatibility issues to worry about, since the DT is appended to the bootloader image, so some corrections can be made if necessary.
On Thu, 18 Aug 2022 17:32:50 +0300 Vladimir Oltean wrote: > DSA has multiple ways of specifying a MAC connection to an internal PHY. > One requires a DT description like this: Too much side-conversations going on here for me to grasp, this is good to go in, correct?
On Fri, Aug 19, 2022 at 04:32:11PM -0700, Jakub Kicinski wrote: > On Thu, 18 Aug 2022 17:32:50 +0300 Vladimir Oltean wrote: > > DSA has multiple ways of specifying a MAC connection to an internal PHY. > > One requires a DT description like this: > > Too much side-conversations going on here for me to grasp, this is good > to go in, correct? Admittedly I'm not an impartial party, but yes please.
On Fri, Aug 19, 2022 at 9:42 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Fri, Aug 19, 2022 at 01:47:51PM +0200, Rasmus Villemoes wrote: > > > To give you an idea of how things work for user ports. If a user port > > > has a phy-handle, DSA will connect to that, irrespective of what OF-based > > > MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is > > > populated with a struct mii_bus by the driver. If it is, it connects in > > > a non-OF based way to a PHY address equal to the port number. If > > > ds->slave_mii_bus doesn't exist but the driver provides > > > ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates > > > ds->slave_mii_bus where its ops are the driver provided phy_read and > > > phy_write, and it then does the same thing of connecting to the PHY in > > > that non-OF based way. > > > > Thanks, that's quite useful. From quick grepping, it seems that ksz9567 > > currently falls into the latter category? > > So it would appear. > > > No, but it's actually easier for me to just do that rather than carry an > > extra patch until the mainline fix hits 5.19.y. > > Whatever suits you best. > > > > there is something to be said about U-Boot compatibility. In U-Boot, > > > with DM_DSA, I don't intend to support any unnecessary complexity and > > > alternative ways of describing the same thing, so there, phy-mode and > > > one of phy-handle or fixed-link are mandatory for all ports. > > > > OK. I suppose that means the linux driver for the ksz9477 family should > > learn to check if there's an "mdio" subnode and if so populate > > ds->slave_mii_bus, but unlike lan937x, absence of that node should not > > be fatal? > > Yes. > > > > U-Boot can pass its own device tree to Linux, it means Linux DSA drivers > > > might need to gradually gain support for OF-based phy-handle on user > > > ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c > > > in the U-Boot source code, and try to work with his device tree format, > > > as a starting point. > > > > Hm. It does seem like that driver has the mdio bus optional (as in, > > probe doesn't fail just because the subnode isn't present). But I'm > > curious why ksz_probe_mdio() looks for a subnode called "mdios" rather > > than "mdio". Tim, just a typo? > > No, definitely not a typo. I have to explain this for what seems like > the millionth time already, but the idea with an "mdios" container was > copied from the sja1105 driver, where there are actually multiple MDIO > buses. Documentation/devicetree/bindings/net/mdio.yaml wants the $nodename > to have the "^mdio(@.*)?" pattern, and: > - if you use "mdio" you can have a single node > - if you don't put the MDIO nodes under a container with an > #address-cells != 0, you can't use the "mdio@something" syntax > > https://lore.kernel.org/all/20210621234930.espjau5l2t5dr75y@skbuf/T/#me43cf6b1976a3c3aec8357f19ab967f98eea1f73 > > What I'm actually less clear about is whether ksz9477 actually needs this. > Luckily U-Boot should have less compatibility issues to worry about, > since the DT is appended to the bootloader image, so some corrections > can be made if necessary. Vladimir, The linux ksz9477 driver does not need the 'mdios' subnode or phy-handle's to function properly. I added these nodes to the U-Boot dts for imx8mm-venice-gw7901.dts and imx8mp-venice-gw74xx.dts in order for the U-Boot ksz9477 driver to work properly. So now I have out-of-sync dts files for those that will cause an issue the next time someone sync's dts from Linux back to U-Boot: diff --git a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts b/arch/arm64 /boot/dts/freescale/imx8mp-venice-gw74xx.dts index 8469d5ddf960..f0bd67f12bad 100644 --- a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts +++ b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts @@ -500,6 +500,7 @@ ports { lan1: port@0 { reg = <0>; label = "lan1"; + phy-handle = <&sw_phy0>; phy-mode = "internal"; local-mac-address = [00 00 00 00 00 00]; }; @@ -507,6 +508,7 @@ lan1: port@0 { lan2: port@1 { reg = <1>; label = "lan2"; + phy-handle = <&sw_phy1>; phy-mode = "internal"; local-mac-address = [00 00 00 00 00 00]; }; @@ -514,6 +516,7 @@ lan2: port@1 { lan3: port@2 { reg = <2>; label = "lan3"; + phy-handle = <&sw_phy2>; phy-mode = "internal"; local-mac-address = [00 00 00 00 00 00]; }; @@ -521,6 +524,7 @@ lan3: port@2 { lan4: port@3 { reg = <3>; label = "lan4"; + phy-handle = <&sw_phy3>; phy-mode = "internal"; local-mac-address = [00 00 00 00 00 00]; }; @@ -528,6 +532,7 @@ lan4: port@3 { lan5: port@4 { reg = <4>; label = "lan5"; + phy-handle = <&sw_phy4>; phy-mode = "internal"; local-mac-address = [00 00 00 00 00 00]; }; @@ -544,6 +549,38 @@ fixed-link { }; }; }; + + mdios { + #address-cells = <1>; + #size-cells = <0>; + + mdio@0 { + reg = <0>; + compatible = "microchip,ksz-mdio"; + #address-cells = <1>; + #size-cells = <0>; + + sw_phy0: ethernet-phy@0 { + reg = <0x0>; + }; + + sw_phy1: ethernet-phy@1 { + reg = <0x1>; + }; + + sw_phy2: ethernet-phy@2 { + reg = <0x2>; + }; + + sw_phy3: ethernet-phy@3 { + reg = <0x3>; + }; + + sw_phy4: ethernet-phy@4 { + reg = <0x4>; + }; + }; + }; }; }; I believe you are still of the opinion that support for parsing the mdios node above be added to the Linux DSA drivers and dt-bindings or were you suggesting just the bindings be added? I don't think bindings would get ack'd unless there was code actually using them for something and its not clear to me exactly what you were asking for. Best Regards, Tim
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Thu, 18 Aug 2022 17:32:50 +0300 you wrote: > DSA has multiple ways of specifying a MAC connection to an internal PHY. > One requires a DT description like this: > > port@0 { > reg = <0>; > phy-handle = <&internal_phy>; > phy-mode = "internal"; > }; > > [...] Here is the summary with links: - [net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode https://git.kernel.org/netdev/net/c/5fbb08eb7f94 You are awesome, thank you!
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index ed7d137cba99..7461272a6d41 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -803,9 +803,15 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port, if (dev->info->supports_rgmii[port]) phy_interface_set_rgmii(config->supported_interfaces); - if (dev->info->internal_phy[port]) + if (dev->info->internal_phy[port]) { __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces); + /* Compatibility for phylib's default interface type when the + * phy-mode property is absent + */ + __set_bit(PHY_INTERFACE_MODE_GMII, + config->supported_interfaces); + } if (dev->dev_ops->get_caps) dev->dev_ops->get_caps(dev, port, config);
DSA has multiple ways of specifying a MAC connection to an internal PHY. One requires a DT description like this: port@0 { reg = <0>; phy-handle = <&internal_phy>; phy-mode = "internal"; }; (which is IMO the recommended approach, as it is the clearest description) but it is also possible to leave the specification as just: port@0 { reg = <0>; } and if the driver implements ds->ops->phy_read and ds->ops->phy_write, the DSA framework "knows" it should create a ds->slave_mii_bus, and it should connect to a non-OF-based internal PHY on this MDIO bus, at an MDIO address equal to the port address. There is also an intermediary way of describing things: port@0 { reg = <0>; phy-handle = <&internal_phy>; }; In case 2, DSA calls phylink_connect_phy() and in case 3, it calls phylink_of_phy_connect(). In both cases, phylink_create() has been called with a phy_interface_t of PHY_INTERFACE_MODE_NA, and in both cases, PHY_INTERFACE_MODE_NA is translated into phy->interface. It is important to note that phy_device_create() initializes dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the PHY. All this means that in order to maintain compatibility with device tree blobs where the phy-mode property is missing, we need to allow the "gmii" phy-mode and treat it as "internal". Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support") Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320 Reported-by: Craig McQueen <craig@mcqueen.id.au> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/microchip/ksz_common.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)