Messages in this thread | | | Subject | Re: [RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider to get PCS | From | Sean Anderson <> | Date | Tue, 19 Jul 2022 15:41:03 -0400 |
| |
On 7/19/22 1:26 PM, Vladimir Oltean wrote: > On Mon, Jul 11, 2022 at 12:05:15PM -0400, Sean Anderson wrote: >> There is a common flow in several drivers where a lynx PCS is created >> without a corresponding firmware node. Consolidate these into one helper >> function. Because we control when the mdiodev is registered, we can add >> a custom match function which will automatically bind our driver >> (instead of using device_driver_attach). >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> drivers/net/dsa/ocelot/felix_vsc9959.c | 25 ++++--------------- >> drivers/net/dsa/ocelot/seville_vsc9953.c | 25 ++++--------------- >> .../net/ethernet/freescale/enetc/enetc_pf.c | 21 +++------------- >> drivers/net/pcs/pcs-lynx.c | 24 ++++++++++++++++++ >> include/linux/pcs-lynx.h | 1 + >> 5 files changed, 39 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c >> index 57634e2296c0..0a756c25d5e8 100644 >> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c >> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c >> @@ -11,6 +11,7 @@ >> #include <net/tc_act/tc_gate.h> >> #include <soc/mscc/ocelot.h> >> #include <linux/dsa/ocelot.h> >> +#include <linux/pcs.h> >> #include <linux/pcs-lynx.h> >> #include <net/pkt_sched.h> >> #include <linux/iopoll.h> >> @@ -1089,16 +1090,9 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) >> if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL) >> continue; >> >> - mdio_device = mdio_device_create(felix->imdio, port); >> - if (IS_ERR(mdio_device)) >> + phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, port); >> + if (IS_ERR(phylink_pcs)) >> continue; >> - >> - phylink_pcs = lynx_pcs_create(mdio_device); >> - if (IS_ERR(phylink_pcs)) { >> - mdio_device_free(mdio_device); >> - continue; >> - } >> - >> felix->pcs[port] = phylink_pcs; >> >> dev_info(dev, "Found PCS at internal MDIO address %d\n", port); >> @@ -1112,17 +1106,8 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot) >> struct felix *felix = ocelot_to_felix(ocelot); >> int port; >> >> - for (port = 0; port < ocelot->num_phys_ports; port++) { >> - struct phylink_pcs *phylink_pcs = felix->pcs[port]; >> - struct mdio_device *mdio_device; >> - >> - if (!phylink_pcs) >> - continue; >> - >> - mdio_device = lynx_get_mdio_device(phylink_pcs); >> - mdio_device_free(mdio_device); >> - lynx_pcs_destroy(phylink_pcs); >> - } >> + for (port = 0; port < ocelot->num_phys_ports; port++) >> + pcs_put(felix->pcs[port]); >> mdiobus_unregister(felix->imdio); >> mdiobus_free(felix->imdio); >> } >> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c >> index 8c52de5d0b02..9006dec85ef0 100644 >> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c >> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c >> @@ -9,6 +9,7 @@ >> #include <linux/mdio/mdio-mscc-miim.h> >> #include <linux/of_mdio.h> >> #include <linux/of_platform.h> >> +#include <linux/pcs.h> >> #include <linux/pcs-lynx.h> >> #include <linux/dsa/ocelot.h> >> #include <linux/iopoll.h> >> @@ -1044,16 +1045,9 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot) >> if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL) >> continue; >> >> - mdio_device = mdio_device_create(felix->imdio, addr); >> - if (IS_ERR(mdio_device)) >> + phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, addr); >> + if (IS_ERR(phylink_pcs)) >> continue; >> - >> - phylink_pcs = lynx_pcs_create(mdio_device); >> - if (IS_ERR(phylink_pcs)) { >> - mdio_device_free(mdio_device); >> - continue; >> - } >> - >> felix->pcs[port] = phylink_pcs; >> >> dev_info(dev, "Found PCS at internal MDIO address %d\n", addr); >> @@ -1067,17 +1061,8 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot) >> struct felix *felix = ocelot_to_felix(ocelot); >> int port; >> >> - for (port = 0; port < ocelot->num_phys_ports; port++) { >> - struct phylink_pcs *phylink_pcs = felix->pcs[port]; >> - struct mdio_device *mdio_device; >> - >> - if (!phylink_pcs) >> - continue; >> - >> - mdio_device = lynx_get_mdio_device(phylink_pcs); >> - mdio_device_free(mdio_device); >> - lynx_pcs_destroy(phylink_pcs); >> - } >> + for (port = 0; port < ocelot->num_phys_ports; port++) >> + pcs_put(felix->pcs[port]); >> >> /* mdiobus_unregister and mdiobus_free handled by devres */ >> } >> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c >> index 8c923a93da88..8da7c8644e44 100644 >> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c >> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c >> @@ -8,6 +8,7 @@ >> #include <linux/of_platform.h> >> #include <linux/of_mdio.h> >> #include <linux/of_net.h> >> +#include <linux/pcs.h> >> #include <linux/pcs-lynx.h> >> #include "enetc_ierb.h" >> #include "enetc_pf.h" >> @@ -827,7 +828,6 @@ static int enetc_imdio_create(struct enetc_pf *pf) >> struct device *dev = &pf->si->pdev->dev; >> struct enetc_mdio_priv *mdio_priv; >> struct phylink_pcs *phylink_pcs; >> - struct mdio_device *mdio_device; >> struct mii_bus *bus; >> int err; >> >> @@ -851,16 +851,8 @@ static int enetc_imdio_create(struct enetc_pf *pf) >> goto free_mdio_bus; >> } >> >> - mdio_device = mdio_device_create(bus, 0); >> - if (IS_ERR(mdio_device)) { >> - err = PTR_ERR(mdio_device); >> - dev_err(dev, "cannot create mdio device (%d)\n", err); >> - goto unregister_mdiobus; >> - } >> - >> - phylink_pcs = lynx_pcs_create(mdio_device); >> + phylink_pcs = lynx_pcs_create_on_bus(bus, 0); >> if (IS_ERR(phylink_pcs)) { >> - mdio_device_free(mdio_device); >> err = PTR_ERR(phylink_pcs); >> dev_err(dev, "cannot create lynx pcs (%d)\n", err); >> goto unregister_mdiobus; >> @@ -880,13 +872,8 @@ static int enetc_imdio_create(struct enetc_pf *pf) >> >> static void enetc_imdio_remove(struct enetc_pf *pf) >> { >> - struct mdio_device *mdio_device; >> - >> - if (pf->pcs) { >> - mdio_device = lynx_get_mdio_device(pf->pcs); >> - mdio_device_free(mdio_device); >> - lynx_pcs_destroy(pf->pcs); >> - } >> + if (pf->pcs) >> + pcs_put(pf->pcs); >> if (pf->imdio) { >> mdiobus_unregister(pf->imdio); >> mdiobus_free(pf->imdio); >> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c >> index 8272072698e4..adb9fd5ce72e 100644 >> --- a/drivers/net/pcs/pcs-lynx.c >> +++ b/drivers/net/pcs/pcs-lynx.c >> @@ -403,6 +403,30 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) >> } >> EXPORT_SYMBOL(lynx_pcs_create); >> >> +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr) >> +{ >> + struct mdio_device *mdio; >> + struct phylink_pcs *pcs; >> + int err; >> + >> + mdio = mdio_device_create(bus, addr); >> + if (IS_ERR(mdio)) >> + return ERR_CAST(mdio); >> + >> + mdio->bus_match = mdio_device_bus_match; >> + strncpy(mdio->modalias, "lynx-pcs", sizeof(mdio->modalias)); >> + err = mdio_device_register(mdio); > > Yeah, so the reason why mdio_device_register() fails with -EBUSY for the > PCS devices created by felix_vsc9959.c is this: > > int mdiobus_register_device(struct mdio_device *mdiodev) > { > int err; > > if (mdiodev->bus->mdio_map[mdiodev->addr]) > return -EBUSY; > > In other words, we already have an existing mdiodev on the bus at > address mdiodev->addr. Funnily enough, that device is actually us. > It was created at MDIO bus creation time, a dummy phydev that no one > connects to, found by mdiobus_scan(). I knew this was taking place, > but forgot/didn't realize the connection with this patch set, and that > dummy phy_device was completely harmless until now. > > I can suppress its creation like this: > > From b1d1cd1625a27a62fd02598c7015b8ff0afdd28a Mon Sep 17 00:00:00 2001 > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Date: Tue, 19 Jul 2022 20:15:52 +0300 > Subject: [PATCH] net: dsa: ocelot: suppress PHY device scanning on the > internal MDIO bus > > This bus contains Lynx PCS devices, and if the lynx-pcs driver ever > decided to call mdio_device_register(), it would fail due to > mdiobus_scan() having created a dummy phydev for the same address > (the PCS responds to standard clause 22 PHY ID registers and can > therefore by autodetected by phylib which thinks it's a PHY). > > On the Seville driver, things are a bit more complicated, since bus > creation is handled by mscc_miim_setup() and that is shared with the > dedicated mscc-miim driver. Suppress PHY scanning only for the Seville > internal MDIO bus rather than for the whole mscc-miim driver, since we > know that on NXP T1040, this bus only contains Lynx PCS devices. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/ocelot/felix_vsc9959.c | 4 ++++ > drivers/net/dsa/ocelot/seville_vsc9953.c | 6 +++++- > 2 files changed, 9 insertions(+), 1 deletion(-)
Thanks! I'll pick this up for v2.
--Sean
| |