| Message ID | 20200131153440.20870-4-calvin.johnson@nxp.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | ACPI support for xgmac_mdio and dpaa2-mac drivers. | expand |
On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@nxp.com> wrote: > > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > Add ACPI support for MDIO bus registration while maintaining > the existing DT support. ... > - 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; > } ... > - 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); Why this has been touched? ... > - priv->mdio_base = of_iomap(np, 0); > + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res); > if (!priv->mdio_base) { Are you sure the check is correct now? > 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"); > + if (is_of_node(pdev->dev.fwnode)) { > + } else if (is_acpi_node(pdev->dev.fwnode)) { Oh, no, this is wrong. Pure approach AFAICS is to use fwnode API or device property API. And actually what you need to include is rather <linux/property.h>, and not acpi.h. > + } else { > + dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n"); > + ret = -ENXIO; > goto err_registration; > } > +static const struct acpi_device_id xgmac_mdio_acpi_match[] = { > + {"NXP0006", 0} How did you test this on platforms with the same IP and without device of this ACPI ID present? (Hint: missed terminator) > +}; > +MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match); > + .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match), ACPI_PTR is not needed otherwise you will get a compiler warning.
On 1/31/2020 7:34 AM, Calvin Johnson wrote: > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > Add ACPI support for MDIO bus registration while maintaining > the existing DT support. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- [snip] > bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv)); > @@ -263,25 +265,41 @@ 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); You could omit this clean up change. > > /* Set the PHY base address */ > priv = bus->priv; > - priv->mdio_base = of_iomap(np, 0); > + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res); > if (!priv->mdio_base) { This probably needs to become IS_ERR() instead of a plain NULL check > 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"); > + if (is_of_node(pdev->dev.fwnode)) { > + 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"); > + goto err_registration; > + } > + } else if (is_acpi_node(pdev->dev.fwnode)) { > + priv->is_little_endian = > + fwnode_property_read_bool(pdev->dev.fwnode, > + "little-endian"); > + ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode); > + if (ret) { > + dev_err(&pdev->dev, "cannot register MDIO bus\n"); > + goto err_registration; > + } The little-endian property read can be moved out of the DT/ACPI paths and you can just use device_property_read_bool() for that purpose. Having both fwnode_mdiobus_register() and of_mdiobus_register() looks fairly redundant, you could quite easily introduce a wrapper: device_mdiobus_register() which internally takes the appropriate DT/ACPI paths as needed.
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus > > On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@nxp.com> > wrote: > > > > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > Add ACPI support for MDIO bus registration while maintaining the > > existing DT support. > > ... > > > - 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; > > } > > ... > > > - 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); > > Why this has been touched? Without this change, I get: --------------------------------------------------------- drivers/net/ethernet/freescale/xgmac_mdio.c: In function 'xgmac_mdio_probe': drivers/net/ethernet/freescale/xgmac_mdio.c:269:27: error: request for member 'start' in something not a structure or union (unsigned long long)res.start); ^ scripts/Makefile.build:265: recipe for target 'drivers/net/ethernet/freescale/xgmac_mdio.o' failed make[4]: *** [drivers/net/ethernet/freescale/xgmac_mdio.o] Error 1 --------------------------------------------------------- On checking other files that calls platform_get_resource, I can see that this is the way they refer 'start'. > > ... > > > - priv->mdio_base = of_iomap(np, 0); > > + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res); > > if (!priv->mdio_base) { > > Are you sure the check is correct now? devm_ioremap_resource returns non-NULL error values. So, this doesn't look right. I'll work on it for v2. > > 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"); > > > + if (is_of_node(pdev->dev.fwnode)) { > > > + } else if (is_acpi_node(pdev->dev.fwnode)) { > > Oh, no, this is wrong. Pure approach AFAICS is to use fwnode API or device > property API. > > And actually what you need to include is rather <linux/property.h>, and not > acpi.h. Understood. I had got some issues while using fwnode API to handle DT case due to which DT/ACPI checks were done and both are handled separately. Let me see if I can root cause it. > > > + } else { > > + dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n"); > > + ret = -ENXIO; > > goto err_registration; > > } > > > +static const struct acpi_device_id xgmac_mdio_acpi_match[] = { > > + {"NXP0006", 0} > > How did you test this on platforms with the same IP and without device of > this ACPI ID present? I didn't test it on any other platforms other than LX2160ARDB. AFAIU, without device of this ACPI ID present, the driver won't get probed. > (Hint: missed terminator) static const struct acpi_device_id xgmac_mdio_acpi_match[] = { { "NXP0006", 0 }, { } }; Is this what you meant? > > > +}; > > +MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match); > > > + .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match), > > ACPI_PTR is not needed otherwise you will get a compiler warning. No compiler warning was observed in both cases. I can see other drivers using this macro. drivers/net/ethernet/apm/xgene-v2/main.c:734: .acpi_match_table = ACPI_PTR(xge_acpi_match), drivers/net/ethernet/apm/xgene/xgene_enet_main.c:2172: .acpi_match_table = ACPI_PTR(xgene_enet_acpi_match), drivers/net/ethernet/hisilicon/hns/hns_enet.c:2445: .acpi_match_table = ACPI_PTR(hns_enet_acpi_match), drivers/net/ethernet/hisilicon/hns_mdio.c:566: .acpi_match_table = ACPI_PTR(hns_mdio_acpi_match), drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5997: .acpi_match_table = ACPI_PTR(mvpp2_acpi_match), drivers/net/ethernet/qualcomm/emac/emac.c:766: .acpi_match_table = ACPI_PTR(emac_acpi_match), drivers/net/ethernet/smsc/smsc911x.c:2667: .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), drivers/net/ethernet/socionext/netsec.c:2187: .acpi_match_table = ACPI_PTR(netsec_acpi_ids), drivers/net/phy/mdio-xgene.c:456: .acpi_match_table = ACPI_PTR(xgene_mdio_acpi_match), Thanks Calvin
On Tue, Feb 4, 2020 at 9:18 AM Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com> wrote: > > -----Original Message----- > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus > > On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@nxp.com> > > wrote: ... > > > - 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); > > > > Why this has been touched? > > Without this change, I get: > --------------------------------------------------------- > drivers/net/ethernet/freescale/xgmac_mdio.c: In function 'xgmac_mdio_probe': > drivers/net/ethernet/freescale/xgmac_mdio.c:269:27: error: request for member 'start' in something not a structure or union > (unsigned long long)res.start); > ^ > scripts/Makefile.build:265: recipe for target 'drivers/net/ethernet/freescale/xgmac_mdio.o' failed > make[4]: *** [drivers/net/ethernet/freescale/xgmac_mdio.o] Error 1 > --------------------------------------------------------- I see. Thanks. Can you leave it one line as it was before? ... > > (Hint: missed terminator) > static const struct acpi_device_id xgmac_mdio_acpi_match[] = { > { "NXP0006", 0 }, > { } > }; > Is this what you meant? Yes! ... > > > + .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match), > > > > ACPI_PTR is not needed otherwise you will get a compiler warning. > > No compiler warning was observed in both cases. You mean you tried CONFIG_ACPI=n and didn't get a warning about unused static variable? Perhaps you may run `make W=1 ...` > I can see other drivers using this macro. They might have hidden same issue.
On Sun, Feb 02, 2020 at 07:44:40PM -0800, Florian Fainelli wrote: > > > On 1/31/2020 7:34 AM, Calvin Johnson wrote: > > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > Add ACPI support for MDIO bus registration while maintaining > > the existing DT support. > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > [snip] > > > bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv)); > > @@ -263,25 +265,41 @@ 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); > > You could omit this clean up change. Sure, will avoid split to newline. > > > > /* Set the PHY base address */ > > priv = bus->priv; > > - priv->mdio_base = of_iomap(np, 0); > > + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res); > > if (!priv->mdio_base) { > > This probably needs to become IS_ERR() instead of a plain NULL check Ok. Will take care in v2. > > > 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"); > > + if (is_of_node(pdev->dev.fwnode)) { > > + 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"); > > + goto err_registration; > > + } > > + } else if (is_acpi_node(pdev->dev.fwnode)) { > > + priv->is_little_endian = > > + fwnode_property_read_bool(pdev->dev.fwnode, > > + "little-endian"); > > + ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode); > > + if (ret) { > > + dev_err(&pdev->dev, "cannot register MDIO bus\n"); > > + goto err_registration; > > + } > > The little-endian property read can be moved out of the DT/ACPI paths > and you can just use device_property_read_bool() for that purpose. > Having both fwnode_mdiobus_register() and of_mdiobus_register() looks > fairly redundant, you could quite easily introduce a wrapper: > device_mdiobus_register() which internally takes the appropriate DT/ACPI > paths as needed. Had some difficulty with DT while using fwnode APIs. Will resolve them and provide better integrated code. Thanks Calvin
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c index c82c85ef5fb3..51db7482b3de 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/acpi.h> #include <linux/kernel.h> #include <linux/slab.h> #include <linux/interrupt.h> @@ -245,14 +247,14 @@ 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; - 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 +265,41 @@ 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); + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res); if (!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"); + if (is_of_node(pdev->dev.fwnode)) { + 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"); + goto err_registration; + } + } else if (is_acpi_node(pdev->dev.fwnode)) { + priv->is_little_endian = + fwnode_property_read_bool(pdev->dev.fwnode, + "little-endian"); + ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode); + if (ret) { + dev_err(&pdev->dev, "cannot register MDIO bus\n"); + goto err_registration; + } + } else { + dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n"); + ret = -ENXIO; goto err_registration; } @@ -290,8 +308,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 +319,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 +333,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,