| Message ID | 20200418105432.11233-2-calvin.johnson@oss.nxp.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | ACPI support for xgmac_mdio and dpaa2-mac drivers | expand |
On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote: > @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) > return value; > } > > +/* Extract the clause 22 phy ID from the compatible string of the form > + * ethernet-phy-idAAAA.BBBB This comment is incorrect. What about clause 45 PHYs? > + */ > +static int xgmac_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id) > +{ > + const char *cp; > + unsigned int upper, lower; > + int ret; > + > + ret = fwnode_property_read_string(fwnode, "compatible", &cp); > + if (!ret) { > + if (sscanf(cp, "ethernet-phy-id%4x.%4x", > + &upper, &lower) == 2) { > + *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF); > + return 0; > + } > + } > + return -EINVAL; > +} > + > +static int xgmac_mdiobus_register_phy(struct mii_bus *bus, > + struct fwnode_handle *child, u32 addr) > +{ > + struct phy_device *phy; > + bool is_c45 = false; > + int rc; > + const char *cp; > + u32 phy_id; > + > + fwnode_property_read_string(child, "compatible", &cp); > + if (!strcmp(cp, "ethernet-phy-ieee802.3-c45")) > + is_c45 = true; > + > + if (!is_c45 && !xgmac_get_phy_id(child, &phy_id)) > + phy = phy_device_create(bus, addr, phy_id, 0, NULL); > + else > + phy = get_phy_device(bus, addr, is_c45); > + if (IS_ERR(phy)) > + return PTR_ERR(phy); > + > + phy->irq = bus->irq[addr]; > + > + /* Associate the fwnode with the device structure so it > + * can be looked up later. > + */ > + phy->mdio.dev.fwnode = child; > + > + /* All data is now stored in the phy struct, so register it */ > + rc = phy_device_register(phy); > + if (rc) { > + phy_device_free(phy); > + fwnode_handle_put(child); > + return rc; > + } > + > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); > + > + return 0; You seem to be duplicating the OF implementation in a private driver, converting it to fwnode. This is not how we develop the Linux kernel. We fix subsystem problems by fixing the subsystems, not by throwing what should be subsystem code into private drivers.
On Sat, Apr 18, 2020 at 2:41 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote: > You seem to be duplicating the OF implementation in a private driver, > converting it to fwnode. This is not how we develop the Linux kernel. > We fix subsystem problems by fixing the subsystems, not by throwing > what should be subsystem code into private drivers. I didn't dive into the details, but I feel same way.
> > +static int xgmac_mdiobus_register_phy(struct mii_bus *bus, > > + struct fwnode_handle *child, u32 addr) > > +{ > > + struct phy_device *phy; > > + bool is_c45 = false; > > + int rc; > > + const char *cp; > > + u32 phy_id; > > + > > + fwnode_property_read_string(child, "compatible", &cp); > > + if (!strcmp(cp, "ethernet-phy-ieee802.3-c45")) > > + is_c45 = true; > > + > > + if (!is_c45 && !xgmac_get_phy_id(child, &phy_id)) > > + phy = phy_device_create(bus, addr, phy_id, 0, NULL); > > + else > > + phy = get_phy_device(bus, addr, is_c45); > > + if (IS_ERR(phy)) > > + return PTR_ERR(phy); > > + > > + phy->irq = bus->irq[addr]; > > + > > + /* Associate the fwnode with the device structure so it > > + * can be looked up later. > > + */ > > + phy->mdio.dev.fwnode = child; > > + > > + /* All data is now stored in the phy struct, so register it */ > > + rc = phy_device_register(phy); > > + if (rc) { > > + phy_device_free(phy); > > + fwnode_handle_put(child); > > + return rc; > > + } > > + > > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); > > + > > + return 0; > > You seem to be duplicating the OF implementation in a private driver, > converting it to fwnode. This is not how we develop the Linux kernel. > We fix subsystem problems by fixing the subsystems, not by throwing > what should be subsystem code into private drivers. And i think a similar comment was given for v1, but i could be remembering wrongly. Andrew
> - ret = of_mdiobus_register(bus, np);
So this is the interesting part. What you really want to be doing is
adding a device_mdiobus_register(bus, dev) to the core. And it needs
to share as much as possible with the of_mdiobus_register()
implementation.
Andrew
On Sat, Apr 18, 2020 at 12:41:16PM +0100, Russell King - ARM Linux admin wrote: > On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote: > > @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) > > return value; > > } > > > > +/* Extract the clause 22 phy ID from the compatible string of the form > > + * ethernet-phy-idAAAA.BBBB > > This comment is incorrect. What about clause 45 PHYs? Agree. Will correct it. May be we need a comment update for of_get_phy_id() also. https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/of/of_mdio.c#L28 <snip> > > + /* All data is now stored in the phy struct, so register it */ > > + rc = phy_device_register(phy); > > + if (rc) { > > + phy_device_free(phy); > > + fwnode_handle_put(child); > > + return rc; > > + } > > + > > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); > > + > > + return 0; > > You seem to be duplicating the OF implementation in a private driver, > converting it to fwnode. This is not how we develop the Linux kernel. > We fix subsystem problems by fixing the subsystems, not by throwing > what should be subsystem code into private drivers. I've used some part of the of_mdiobus_register_phy(). Looks like some other network drivers using acpi had also taken similar approach. Anyway, I'll try to make it generic and move out to subsystem. Thanks Calvin
On Sat, Apr 18, 2020 at 04:46:06PM +0200, Andrew Lunn wrote: > > - ret = of_mdiobus_register(bus, np); > > So this is the interesting part. What you really want to be doing is > adding a device_mdiobus_register(bus, dev) to the core. And it needs > to share as much as possible with the of_mdiobus_register() > implementation. Sure, will take this approach and submit v3. Thanks Calvin
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c index c82c85ef5fb3..d3480c0e0cf5 100644 --- a/drivers/net/ethernet/freescale/xgmac_mdio.c +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c @@ -2,6 +2,7 @@ * QorIQ 10G MDIO Controller * * Copyright 2012 Freescale Semiconductor, Inc. + * Copyright 2019 NXP * * Authors: Andy Fleming <afleming@freescale.com> * Timur Tabi <timur@freescale.com> @@ -11,6 +12,7 @@ * kind, whether express or implied. */ +#include <linux/property.h> #include <linux/kernel.h> #include <linux/slab.h> #include <linux/interrupt.h> @@ -20,6 +22,7 @@ #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/of_mdio.h> +#include <linux/acpi.h> /* Number of microseconds to wait for a register to respond */ #define TIMEOUT 1000 @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) return value; } +/* Extract the clause 22 phy ID from the compatible string of the form + * ethernet-phy-idAAAA.BBBB + */ +static int xgmac_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id) +{ + const char *cp; + unsigned int upper, lower; + int ret; + + ret = fwnode_property_read_string(fwnode, "compatible", &cp); + if (!ret) { + if (sscanf(cp, "ethernet-phy-id%4x.%4x", + &upper, &lower) == 2) { + *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF); + return 0; + } + } + return -EINVAL; +} + +static int xgmac_mdiobus_register_phy(struct mii_bus *bus, + struct fwnode_handle *child, u32 addr) +{ + struct phy_device *phy; + bool is_c45 = false; + int rc; + const char *cp; + u32 phy_id; + + fwnode_property_read_string(child, "compatible", &cp); + if (!strcmp(cp, "ethernet-phy-ieee802.3-c45")) + is_c45 = true; + + if (!is_c45 && !xgmac_get_phy_id(child, &phy_id)) + phy = phy_device_create(bus, addr, phy_id, 0, NULL); + else + phy = get_phy_device(bus, addr, is_c45); + if (IS_ERR(phy)) + return PTR_ERR(phy); + + phy->irq = bus->irq[addr]; + + /* Associate the fwnode with the device structure so it + * can be looked up later. + */ + phy->mdio.dev.fwnode = child; + + /* All data is now stored in the phy struct, so register it */ + rc = phy_device_register(phy); + if (rc) { + phy_device_free(phy); + fwnode_handle_put(child); + return rc; + } + + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); + + return 0; +} + static int xgmac_mdio_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; struct mii_bus *bus; - struct resource res; + struct resource *res; struct mdio_fsl_priv *priv; int ret; + struct fwnode_handle *fwnode; + struct fwnode_handle *child; + int addr, rc; - ret = of_address_to_resource(np, 0, &res); - if (ret) { + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { dev_err(&pdev->dev, "could not obtain address\n"); - return ret; + return -ENODEV; } bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv)); @@ -263,25 +329,55 @@ static int xgmac_mdio_probe(struct platform_device *pdev) bus->read = xgmac_mdio_read; bus->write = xgmac_mdio_write; bus->parent = &pdev->dev; - snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start); + snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start); /* Set the PHY base address */ priv = bus->priv; - priv->mdio_base = of_iomap(np, 0); - if (!priv->mdio_base) { + priv->mdio_base = ioremap(res->start, resource_size(res)); + if (IS_ERR_OR_NULL(priv->mdio_base)) { ret = -ENOMEM; goto err_ioremap; } - priv->is_little_endian = of_property_read_bool(pdev->dev.of_node, - "little-endian"); - - priv->has_a011043 = of_property_read_bool(pdev->dev.of_node, - "fsl,erratum-a011043"); - - ret = of_mdiobus_register(bus, np); - if (ret) { - dev_err(&pdev->dev, "cannot register MDIO bus\n"); + priv->is_little_endian = device_property_read_bool(&pdev->dev, + "little-endian"); + + priv->has_a011043 = device_property_read_bool(&pdev->dev, + "fsl,erratum-a011043"); + if (is_of_node(pdev->dev.fwnode)) { + ret = of_mdiobus_register(bus, np); + if (ret) { + dev_err(&pdev->dev, "cannot register MDIO bus\n"); + goto err_registration; + } + } else if (is_acpi_node(pdev->dev.fwnode)) { + /* Mask out all PHYs from auto probing. */ + bus->phy_mask = ~0; + ret = mdiobus_register(bus); + if (ret) { + dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret); + return ret; + } + + fwnode = pdev->dev.fwnode; + /* Loop over the child nodes and register a phy_device for each PHY */ + fwnode_for_each_child_node(fwnode, child) { + fwnode_property_read_u32(child, "phy-channel", &addr); + + if (addr < 0 || addr >= PHY_MAX_ADDR) { + dev_err(&bus->dev, "Invalid PHY address %i\n", addr); + continue; + } + + rc = xgmac_mdiobus_register_phy(bus, child, addr); + if (rc == -ENODEV) + dev_err(&bus->dev, + "MDIO device at address %d is missing.\n", + addr); + } + } else { + dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n"); + ret = -ENXIO; goto err_registration; } @@ -290,8 +386,6 @@ static int xgmac_mdio_probe(struct platform_device *pdev) return 0; err_registration: - iounmap(priv->mdio_base); - err_ioremap: mdiobus_free(bus); @@ -303,13 +397,12 @@ static int xgmac_mdio_remove(struct platform_device *pdev) struct mii_bus *bus = platform_get_drvdata(pdev); mdiobus_unregister(bus); - iounmap(bus->priv); mdiobus_free(bus); return 0; } -static const struct of_device_id xgmac_mdio_match[] = { +static const struct of_device_id xgmac_mdio_of_match[] = { { .compatible = "fsl,fman-xmdio", }, @@ -318,12 +411,18 @@ static const struct of_device_id xgmac_mdio_match[] = { }, {}, }; -MODULE_DEVICE_TABLE(of, xgmac_mdio_match); +MODULE_DEVICE_TABLE(of, xgmac_mdio_of_match); + +static const struct acpi_device_id xgmac_mdio_acpi_match[] = { + {"NXP0006", 0} +}; +MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match); static struct platform_driver xgmac_mdio_driver = { .driver = { .name = "fsl-fman_xmdio", - .of_match_table = xgmac_mdio_match, + .of_match_table = xgmac_mdio_of_match, + .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match), }, .probe = xgmac_mdio_probe, .remove = xgmac_mdio_remove,
Add ACPI support for MDIO bus registration while maintaining the existing DT support. Extract phy_id using xgmac_get_phy_id() from the compatible string passed through the ACPI table. Use ACPI property phy-channel to provide ID of the phy. Use xgmac_mdiobus_register_phy() to register PHY devices on the MDIO bus. Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v2: - Use IS_ERR_OR_NULL for priv->mdio_base instead of plain NULL check - Add missing terminator of struct acpi_device_id - Use device_property_read_bool and avoid redundancy - Add helper functions xgmac_get_phy_id() and xgmac_mdiobus_register_phy() drivers/net/ethernet/freescale/xgmac_mdio.c | 143 +++++++++++++++++--- 1 file changed, 121 insertions(+), 22 deletions(-)