Message ID | 1563894955-545-1-git-send-email-claudiu.manoil@nxp.com |
---|---|
Headers | show |
Series | enetc: Add mdio bus driver for the PCIe MDIO endpoint | expand |
On Tue, 2019-07-23 at 18:15 +0300, Claudiu Manoil wrote: > ENETC ports can manage the MDIO bus via local register > interface. However there's also a centralized way > to manage the MDIO bus, via the MDIO PCIe endpoint > device integrated by the same root complex that also > integrates the ENETC ports (eth controllers). > > Depending on board design and use case, centralized > access to MDIO may be better than using local ENETC > port registers. For instance, on the LS1028A QDS board > where MDIO muxing is requiered. Also, the LS1028A on-chip > switch doesn't have a local MDIO register interface. > > The current patch registers the above PCIe enpoint as a > separate MDIO bus and provides a driver for it by re-using > the code used for local MDIO access. It also allows the > ENETC port PHYs to be managed by this driver if the local > "mdio" node is missing from the ENETC port node. > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- > .../net/ethernet/freescale/enetc/enetc_mdio.c | 90 > +++++++++++++++++++ > .../net/ethernet/freescale/enetc/enetc_pf.c | 5 +- > 2 files changed, 94 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > index 77b9cd10ba2b..efa8a29f463d 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c > @@ -197,3 +197,93 @@ void enetc_mdio_remove(struct enetc_pf *pf) > mdiobus_free(pf->mdio); > } > } > + > +#define ENETC_MDIO_DEV_ID 0xee01 > +#define ENETC_MDIO_DEV_NAME "FSL PCIe IE Central MDIO" > +#define ENETC_MDIO_BUS_NAME ENETC_MDIO_DEV_NAME " Bus" > +#define ENETC_MDIO_DRV_NAME ENETC_MDIO_DEV_NAME " driver" > +#define ENETC_MDIO_DRV_ID "fsl_enetc_mdio" > + > +static int enetc_pci_mdio_probe(struct pci_dev *pdev, > + const struct pci_device_id *ent) > +{ > + struct device *dev = &pdev->dev; > + struct mii_bus *bus; > + int err; > + > + bus = mdiobus_alloc_size(sizeof(u32 *)); > + if (!bus) > + return -ENOMEM; > + > + bus->name = ENETC_MDIO_BUS_NAME; > + bus->read = enetc_mdio_read; > + bus->write = enetc_mdio_write; > + bus->parent = dev; > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); > + > + pcie_flr(pdev); > + err = pci_enable_device_mem(pdev); > + if (err) { > + dev_err(dev, "device enable failed\n"); mdiobus_free(bus) is missing here and in every error path. > + return err; > + } > + > + err = pci_request_mem_regions(pdev, ENETC_MDIO_DRV_ID); > + if (err) { > + dev_err(dev, "pci_request_regions failed\n"); > + goto err_pci_mem_reg; > + } > + > + bus->priv = pci_iomap_range(pdev, 0, ENETC_MDIO_REG_OFFSET, 0); > + if (!bus->priv) { > + err = -ENXIO; > + dev_err(dev, "ioremap failed\n"); > + goto err_ioremap; > + } > + > + err = of_mdiobus_register(bus, dev->of_node); > + if (err) > + goto err_mdiobus_reg; > + > + pci_set_drvdata(pdev, bus); > + > + return 0; > + > +err_mdiobus_reg: > + iounmap(bus->priv); > +err_ioremap: > + pci_release_mem_regions(pdev); > +err_pci_mem_reg: > + pci_disable_device(pdev); > + > + return err; > +} > + > +static void enetc_pci_mdio_remove(struct pci_dev *pdev) > +{ > + struct mii_bus *bus = pci_get_drvdata(pdev); > + > + mdiobus_unregister(bus); > + iounmap(bus->priv); > + mdiobus_free(bus); > + this should come last to be symmetrical with probe flow. > + pci_release_mem_regions(pdev); > + pci_disable_device(pdev); > +} > + > +static const struct pci_device_id enetc_pci_mdio_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, ENETC_MDIO_DEV_ID) }, > + { 0, } /* End of table. */ > +}; > +MODULE_DEVICE_TABLE(pci, enetc_mdio_id_table); > + > +static struct pci_driver enetc_pci_mdio_driver = { > + .name = ENETC_MDIO_DRV_ID, > + .id_table = enetc_pci_mdio_id_table, > + .probe = enetc_pci_mdio_probe, > + .remove = enetc_pci_mdio_remove, > +}; > +module_pci_driver(enetc_pci_mdio_driver); > + > +MODULE_DESCRIPTION(ENETC_MDIO_DRV_NAME); > +MODULE_LICENSE("Dual BSD/GPL"); > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c > b/drivers/net/ethernet/freescale/enetc/enetc_pf.c > index 258b3cb38a6f..7d6513ff8507 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c > @@ -750,6 +750,7 @@ static int enetc_of_get_phy(struct > enetc_ndev_priv *priv) > { > struct enetc_pf *pf = enetc_si_priv(priv->si); > struct device_node *np = priv->dev->of_node; > + struct device_node *mdio_np; > int err; > > if (!np) { > @@ -773,7 +774,9 @@ static int enetc_of_get_phy(struct > enetc_ndev_priv *priv) > priv->phy_node = of_node_get(np); > } > > - if (!of_phy_is_fixed_link(np)) { > + mdio_np = of_get_child_by_name(np, "mdio"); > + if (mdio_np) { > + of_node_put(mdio_np); > err = enetc_mdio_probe(pf); > if (err) { > of_node_put(priv->phy_node);
> + bus = mdiobus_alloc_size(sizeof(u32 *)); > + if (!bus) > + return -ENOMEM; > + > + bus->priv = pci_iomap_range(pdev, 0, ENETC_MDIO_REG_OFFSET, 0); This got me confused for a while. You allocate space for a u32 pointer. bus->priv will point to this space. However, you are not using this space, you {ab}use the pointer to directly hold the return from pci_iomap_range(). This works, but sparse is probably unhappy, and you are wasting the space the u32 pointer takes. Andrew
>-----Original Message----- >From: Andrew Lunn <andrew@lunn.ch> >Sent: Wednesday, July 24, 2019 1:25 AM >To: Claudiu Manoil <claudiu.manoil@nxp.com> >Cc: David S . Miller <davem@davemloft.net>; devicetree@vger.kernel.org; >netdev@vger.kernel.org; Alexandru Marginean ><alexandru.marginean@nxp.com>; linux-kernel@vger.kernel.org; Leo Li ><leoyang.li@nxp.com>; Rob Herring <robh+dt@kernel.org>; linux-arm- >kernel@lists.infradead.org >Subject: Re: [PATCH net-next 1/3] enetc: Add mdio bus driver for the PCIe MDIO >endpoint > >> + bus = mdiobus_alloc_size(sizeof(u32 *)); >> + if (!bus) >> + return -ENOMEM; >> + > >> + bus->priv = pci_iomap_range(pdev, 0, ENETC_MDIO_REG_OFFSET, 0); > >This got me confused for a while. You allocate space for a u32 >pointer. bus->priv will point to this space. However, you are not >using this space, you {ab}use the pointer to directly hold the return >from pci_iomap_range(). This works, but sparse is probably unhappy, >and you are wasting the space the u32 pointer takes. > Thanks Andrew, This is not what I wanted to do, don't ask me how I got to this, it's confusing indeed. What's needed here is mdiobus_alloc() or better, devm_mdiobus_alloc(). I've got to do some cleanup in the local mdio bus probing too. Will send v2. Thanks, Claudiu
>-----Original Message----- >From: Saeed Mahameed <saeedm@mellanox.com> [...] > >mdiobus_free(bus) is missing here and in every error path. > [...] > >this should come last to be symmetrical with probe flow. > Will clean these up too. Thanks.
>-----Original Message----- >From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On >Behalf Of Claudiu Manoil >Sent: Wednesday, July 24, 2019 12:53 PM >To: Andrew Lunn <andrew@lunn.ch> >Cc: David S . Miller <davem@davemloft.net>; devicetree@vger.kernel.org; >netdev@vger.kernel.org; Alexandru Marginean ><alexandru.marginean@nxp.com>; linux-kernel@vger.kernel.org; Leo Li ><leoyang.li@nxp.com>; Rob Herring <robh+dt@kernel.org>; linux-arm- >kernel@lists.infradead.org >Subject: RE: [PATCH net-next 1/3] enetc: Add mdio bus driver for the PCIe MDIO >endpoint > >>-----Original Message----- >>From: Andrew Lunn <andrew@lunn.ch> >>Sent: Wednesday, July 24, 2019 1:25 AM >>To: Claudiu Manoil <claudiu.manoil@nxp.com> >>Cc: David S . Miller <davem@davemloft.net>; devicetree@vger.kernel.org; >>netdev@vger.kernel.org; Alexandru Marginean >><alexandru.marginean@nxp.com>; linux-kernel@vger.kernel.org; Leo Li >><leoyang.li@nxp.com>; Rob Herring <robh+dt@kernel.org>; linux-arm- >>kernel@lists.infradead.org >>Subject: Re: [PATCH net-next 1/3] enetc: Add mdio bus driver for the >>PCIe MDIO endpoint >> >>> + bus = mdiobus_alloc_size(sizeof(u32 *)); >>> + if (!bus) >>> + return -ENOMEM; >>> + >> >>> + bus->priv = pci_iomap_range(pdev, 0, ENETC_MDIO_REG_OFFSET, 0); >> >>This got me confused for a while. You allocate space for a u32 pointer. >>bus->priv will point to this space. However, you are not using this >>space, you {ab}use the pointer to directly hold the return from >>pci_iomap_range(). This works, but sparse is probably unhappy, and you >>are wasting the space the u32 pointer takes. >> > >Thanks Andrew, >This is not what I wanted to do, don't ask me how I got to this, it's confusing >indeed. >What's needed here is mdiobus_alloc() or better, devm_mdiobus_alloc(). >I've got to do some cleanup in the local mdio bus probing too. >Will send v2. > This is tricky actually, mdiobus_alloc won't do it, I still need to allocate space for the pointer. So it's not ok to store the register space pointer directly into priv (even if iomap returns void *), it's unusual. Looks like I will have to use double pointers!