| Message ID | 20200621225451.12435-1-ioana.ciornei@nxp.com |
|---|---|
| Headers | show |
| Series | net: phy: add Lynx PCS MDIO module | expand |
On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote: > Add support for the Lynx PCS as a separate module in drivers/net/phy/. > The advantage of this structure is that multiple ethernet or switch > drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the > same implementation of PCS configuration and runtime management. > > The PCS is represented as an mdio_device and the callbacks exported are > highly tied with PHYLINK and can't be used without it. > > The first 3 patches add some missing pieces in PHYLINK and the locked > mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a > standalone module. The majority of the code is extracted from the Felix > DSA driver. The last patch makes the necessary changes in the Felix > driver in order to use the new common PCS implementation. > > At the moment, USXGMII (only with in-band AN and speeds up to 2500), > SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o > in-band AN) are supported by the Lynx PCS MDIO module since these were > also supported by Felix and no functional change is intended at this > time. Overall, I think we need to sort out the remaining changes in phylink before moving forward with this patch set - I've made some progress with Florian and the Broadcom DSA switches late last night. I'm now working on updating the felix DSA driver. There's another reason - having looked at the work I did with this same PHY, I think you are missing configuration of the link timer, which is different in SGMII and 1000BASE-X. Please can you look at the code I came up with? "dpaa2-mac: add 1000BASE-X/SGMII PCS support". Thanks.
On Mon, 22 Jun 2020 at 12:29, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote: > > Add support for the Lynx PCS as a separate module in drivers/net/phy/. > > The advantage of this structure is that multiple ethernet or switch > > drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the > > same implementation of PCS configuration and runtime management. > > > > The PCS is represented as an mdio_device and the callbacks exported are > > highly tied with PHYLINK and can't be used without it. > > > > The first 3 patches add some missing pieces in PHYLINK and the locked > > mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a > > standalone module. The majority of the code is extracted from the Felix > > DSA driver. The last patch makes the necessary changes in the Felix > > driver in order to use the new common PCS implementation. > > > > At the moment, USXGMII (only with in-band AN and speeds up to 2500), > > SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o > > in-band AN) are supported by the Lynx PCS MDIO module since these were > > also supported by Felix and no functional change is intended at this > > time. > > Overall, I think we need to sort out the remaining changes in phylink > before moving forward with this patch set - I've made some progress > with Florian and the Broadcom DSA switches late last night. I'm now > working on updating the felix DSA driver. > What needs to be done in the felix driver that is not part of this series? Maybe you could review this instead? > There's another reason - having looked at the work I did with this > same PHY, I think you are missing configuration of the link timer, > which is different in SGMII and 1000BASE-X. Please can you look at > the code I came up with? "dpaa2-mac: add 1000BASE-X/SGMII PCS support". > > Thanks. > felix does not have support code for 1000base-x, so I think it's natural to not clutter this series with things like that. Things like USXGMII up to 10G, 10GBase-R, are also missing, for much of the same reason - we wanted to make no functional change to the existing code, precisely because we wanted it to go in quickly. There are multiple things that are waiting for it: - Michael Walle's enetc patches are going to use pcs-lynx - The new Seville driver will use pcs-lynx > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! Thanks, -Vladimir
Hi all, Am 2020-06-22 11:34, schrieb Vladimir Oltean: > On Mon, 22 Jun 2020 at 12:29, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: >> >> On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote: >> > Add support for the Lynx PCS as a separate module in drivers/net/phy/. >> > The advantage of this structure is that multiple ethernet or switch >> > drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the >> > same implementation of PCS configuration and runtime management. >> > >> > The PCS is represented as an mdio_device and the callbacks exported are >> > highly tied with PHYLINK and can't be used without it. >> > >> > The first 3 patches add some missing pieces in PHYLINK and the locked >> > mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a >> > standalone module. The majority of the code is extracted from the Felix >> > DSA driver. The last patch makes the necessary changes in the Felix >> > driver in order to use the new common PCS implementation. >> > >> > At the moment, USXGMII (only with in-band AN and speeds up to 2500), >> > SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o >> > in-band AN) are supported by the Lynx PCS MDIO module since these were >> > also supported by Felix and no functional change is intended at this >> > time. >> >> Overall, I think we need to sort out the remaining changes in phylink >> before moving forward with this patch set - I've made some progress >> with Florian and the Broadcom DSA switches late last night. I'm now >> working on updating the felix DSA driver. >> > > What needs to be done in the felix driver that is not part of this > series? Maybe you could review this instead? > >> There's another reason - having looked at the work I did with this >> same PHY, I think you are missing configuration of the link timer, >> which is different in SGMII and 1000BASE-X. Please can you look at >> the code I came up with? "dpaa2-mac: add 1000BASE-X/SGMII PCS >> support". >> >> Thanks. >> > > felix does not have support code for 1000base-x, so I think it's > natural to not clutter this series with things like that. > Things like USXGMII up to 10G, 10GBase-R, are also missing, for much > of the same reason - we wanted to make no functional change to the > existing code, precisely because we wanted it to go in quickly. There > are multiple things that are waiting for it: > - Michael Walle's enetc patches are going to use pcs-lynx How likely is it that this will be sorted out before the 5.9 merge window will be closed? The thing is, we have boards out in the wild which have a non-working ethernet with their stock bootloader and which depend on the following patch series to get that fixed: https://lore.kernel.org/netdev/20200528063847.27704-1-michael@walle.cc/ Thus, if this is going to take longer, I'd do a respin of that series. We already missed the 5.8 release and I don't know if a "Fixes:" tag (or a CC stable) is appropriate here because it is kind of a new functionality. -michael
Hi Michael, On Tue, 30 Jun 2020 at 09:01, Michael Walle <michael@walle.cc> wrote: > > Hi all, > > Am 2020-06-22 11:34, schrieb Vladimir Oltean: > > On Mon, 22 Jun 2020 at 12:29, Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > >> > >> On Mon, Jun 22, 2020 at 01:54:42AM +0300, Ioana Ciornei wrote: > >> > Add support for the Lynx PCS as a separate module in drivers/net/phy/. > >> > The advantage of this structure is that multiple ethernet or switch > >> > drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the > >> > same implementation of PCS configuration and runtime management. > >> > > >> > The PCS is represented as an mdio_device and the callbacks exported are > >> > highly tied with PHYLINK and can't be used without it. > >> > > >> > The first 3 patches add some missing pieces in PHYLINK and the locked > >> > mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a > >> > standalone module. The majority of the code is extracted from the Felix > >> > DSA driver. The last patch makes the necessary changes in the Felix > >> > driver in order to use the new common PCS implementation. > >> > > >> > At the moment, USXGMII (only with in-band AN and speeds up to 2500), > >> > SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o > >> > in-band AN) are supported by the Lynx PCS MDIO module since these were > >> > also supported by Felix and no functional change is intended at this > >> > time. > >> > >> Overall, I think we need to sort out the remaining changes in phylink > >> before moving forward with this patch set - I've made some progress > >> with Florian and the Broadcom DSA switches late last night. I'm now > >> working on updating the felix DSA driver. > >> > > > > What needs to be done in the felix driver that is not part of this > > series? Maybe you could review this instead? > > > >> There's another reason - having looked at the work I did with this > >> same PHY, I think you are missing configuration of the link timer, > >> which is different in SGMII and 1000BASE-X. Please can you look at > >> the code I came up with? "dpaa2-mac: add 1000BASE-X/SGMII PCS > >> support". > >> > >> Thanks. > >> > > > > felix does not have support code for 1000base-x, so I think it's > > natural to not clutter this series with things like that. > > Things like USXGMII up to 10G, 10GBase-R, are also missing, for much > > of the same reason - we wanted to make no functional change to the > > existing code, precisely because we wanted it to go in quickly. There > > are multiple things that are waiting for it: > > - Michael Walle's enetc patches are going to use pcs-lynx > > How likely is it that this will be sorted out before the 5.9 merge > window will be closed? The thing is, we have boards out in the > wild which have a non-working ethernet with their stock bootloader > and which depend on the following patch series to get that fixed: > > https://lore.kernel.org/netdev/20200528063847.27704-1-michael@walle.cc/ > > Thus, if this is going to take longer, I'd do a respin of that > series. We already missed the 5.8 release and I don't know if > a "Fixes:" tag (or a CC stable) is appropriate here because it > is kind of a new functionality. > > -michael From my side I think it is reasonable that you resubmit your enetc patches using the existing framework. Looks like we're in for some pretty significant API changes for phylink, not sure if you need to depend on those if you just need the PCS to work. But, I don't know if marketing your patches as "fixes" is going to go that well. In fact, you are also moving some definitions around, from felix to enetc. I think if you want to break dependencies from felix here, you should leave the definitions where they are and just duplicate them. Thanks, -Vladimir
Hi Vladimir, Am 2020-07-01 15:37, schrieb Vladimir Oltean: [..] >> > felix does not have support code for 1000base-x, so I think it's >> > natural to not clutter this series with things like that. >> > Things like USXGMII up to 10G, 10GBase-R, are also missing, for much >> > of the same reason - we wanted to make no functional change to the >> > existing code, precisely because we wanted it to go in quickly. There >> > are multiple things that are waiting for it: >> > - Michael Walle's enetc patches are going to use pcs-lynx >> >> How likely is it that this will be sorted out before the 5.9 merge >> window will be closed? The thing is, we have boards out in the >> wild which have a non-working ethernet with their stock bootloader >> and which depend on the following patch series to get that fixed: >> >> https://lore.kernel.org/netdev/20200528063847.27704-1-michael@walle.cc/ >> >> Thus, if this is going to take longer, I'd do a respin of that >> series. We already missed the 5.8 release and I don't know if >> a "Fixes:" tag (or a CC stable) is appropriate here because it >> is kind of a new functionality. >> >> -michael > > From my side I think it is reasonable that you resubmit your enetc > patches using the existing framework. Looks like we're in for some > pretty significant API changes for phylink, not sure if you need to > depend on those if you just need the PCS to work. Ok, I'll resubmit them. > But, I don't know if marketing your patches as "fixes" is going to go > that well. In fact, you are also moving some definitions around, from > felix to enetc. I think if you want to break dependencies from felix > here, you should leave the definitions where they are and just > duplicate them. Nice idea, but I didn't intend to add the Fixes tag anyways. I'm fine with saying, please use the newest kernel. -michael
Add support for the Lynx PCS as a separate module in drivers/net/phy/. The advantage of this structure is that multiple ethernet or switch drivers used on NXP hardware (ENETC, Felix DSA switch etc) can share the same implementation of PCS configuration and runtime management. The PCS is represented as an mdio_device and the callbacks exported are highly tied with PHYLINK and can't be used without it. The first 3 patches add some missing pieces in PHYLINK and the locked mdiobus write accessor. Next, the Lynx PCS MDIO module is added as a standalone module. The majority of the code is extracted from the Felix DSA driver. The last patch makes the necessary changes in the Felix driver in order to use the new common PCS implementation. At the moment, USXGMII (only with in-band AN and speeds up to 2500), SGMII, QSGMII (with and without in-band AN) and 2500Base-X (only w/o in-band AN) are supported by the Lynx PCS MDIO module since these were also supported by Felix and no functional change is intended at this time. Changes in v2: * got rid of the mdio_lynx_pcs structure and directly exported the functions without the need of an indirection * made the necessary adjustments for this in the Felix DSA driver * solved the broken allmodconfig build test by making the module tristate instead of bool * fixed a memory leakage in the Felix driver (the pcs structure was allocated twice) Changes in v3: * added support for PHYLINK PCS ops in DSA (patch 5/9) * cleanup in Felix PHYLINK operations and migrate to phylink_mac_link_up() being the callback of choice for applying MAC configuration (patches 6-8) As per Russell's request, the DSA core now exports PHYLINK PCS ops. DSA adds a phylink_pcs_ops structure to PHYLINK only when a switch driver is implementing all of the 4 PCS operations (get_state, an_restart, config, link_up). Note that there is no longer anything to be done in phylink_mac_config() for Felix and, likely, for any other driver which uses the new PHYLINK format. However, PHYLINK does want the phylink_mac_config() pointer to be present. Currently, the DSA core supplied a stub and happily doesn't do anything because felix_phylink_mac_config() is missing. Felix's migration from mac_config() to mac_link_up() was added directly into this patch set since everything in this general area is intertwined and splitting would have been difficult. Ioana Ciornei (5): net: phylink: consider QSGMII interface mode in phylink_mii_c22_pcs_get_state net: mdiobus: add clause 45 mdiobus write accessor net: phy: add Lynx PCS module net: dsa: add support for phylink_pcs_ops net: dsa: felix: use the Lynx PCS helpers Russell King (1): net: phylink: add interface to configure clause 22 PCS PHY Vladimir Oltean (3): net: dsa: felix: unconditionally configure MAC speed to 1000Mbps net: dsa: felix: set proper pause frame timers based on link speed net: dsa: felix: use resolved link config in mac_link_up() MAINTAINERS | 7 + drivers/net/dsa/ocelot/Kconfig | 1 + drivers/net/dsa/ocelot/felix.c | 129 ++++++--- drivers/net/dsa/ocelot/felix.h | 16 +- drivers/net/dsa/ocelot/felix_vsc9959.c | 385 +++---------------------- drivers/net/phy/Kconfig | 6 + drivers/net/phy/Makefile | 1 + drivers/net/phy/pcs-lynx.c | 337 ++++++++++++++++++++++ drivers/net/phy/phylink.c | 38 +++ include/linux/fsl/enetc_mdio.h | 21 -- include/linux/mdio.h | 6 + include/linux/pcs-lynx.h | 25 ++ include/linux/phylink.h | 3 + include/net/dsa.h | 12 + net/dsa/dsa_priv.h | 1 + net/dsa/port.c | 46 +++ net/dsa/slave.c | 6 + 17 files changed, 617 insertions(+), 423 deletions(-) create mode 100644 drivers/net/phy/pcs-lynx.c create mode 100644 include/linux/pcs-lynx.h