Messages in this thread Patch in this message | | | Date | Tue, 19 Jul 2022 20:26:28 +0300 | From | Vladimir Oltean <> | Subject | Re: [RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider to get PCS |
| |
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(-)
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 927225e51f05..1ff71f1df316 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -1062,6 +1062,10 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) bus->read = enetc_mdio_read; bus->write = enetc_mdio_write; bus->parent = dev; + /* Suppress PHY device creation in mdiobus_scan(), + * we have Lynx PCSs + */ + bus->phy_mask = ~0; mdio_priv = bus->priv; mdio_priv->hw = hw; /* This gets added to imdio_regs, which already maps addresses diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index 9006dec85ef0..9f400867fce3 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -1018,12 +1018,16 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot) rc = mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus", ocelot->targets[GCB], ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK]); - if (rc) { dev_err(dev, "failed to setup MDIO bus\n"); return rc; } + /* Suppress PHY device creation in mdiobus_scan(), + * we have Lynx PCSs + */ + bus->phy_mask = ~0; + /* Needed in order to initialize the bus mutex lock */ rc = devm_of_mdiobus_register(dev, bus, NULL); if (rc < 0) { -- 2.34.1 and then things start working (including traffic).
> + if (err) { > + mdio_device_free(mdio); > + return ERR_PTR(err); > + } > + > + pcs = pcs_get_by_provider(&mdio->dev); > + mdio_device_free(mdio); > + return pcs; > +} > +EXPORT_SYMBOL(lynx_pcs_create_on_bus); > + > void lynx_pcs_destroy(struct phylink_pcs *pcs) > { > pcs_put(pcs); > diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h > index 5712cc2ce775..1c14342bb8c4 100644 > --- a/include/linux/pcs-lynx.h > +++ b/include/linux/pcs-lynx.h > @@ -12,6 +12,7 @@ > struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs); > > struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio); > +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr); > > void lynx_pcs_destroy(struct phylink_pcs *pcs); > > -- > 2.35.1.1320.gc452695387.dirty >
| |