| Message ID | 20200617171536.12014-3-calvin.johnson@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI support for xgmac_mdio drivers. | expand |
On Wed, Jun 17, 2020 at 8:16 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > From: Jeremy Linton <jeremy.linton@arm.com> > > Add ACPI support for xgmac MDIO bus registration while maintaining > the existing DT support. > > The function mdiobus_register() inside of_mdiobus_register(), brings > up all the PHYs on the mdio bus and attach them to the bus. ... > - 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); Since you are here, better to change the specifier to the dedicated one, i.e. "%pa". But I don't remember if it adds 0x to it... ... > +static const struct acpi_device_id xgmac_acpi_match[] = { > + { "NXP0006", (kernel_ulong_t)NULL }, { "NXP0006" }, is enough. And I suppose this is the official ID. > + { }, No need to have comma in terminator line. > +}; -- With Best Regards, Andy Shevchenko
On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > From: Jeremy Linton <jeremy.linton@arm.com> > +static const struct acpi_device_id xgmac_acpi_match[] = { > + { "NXP0006", (kernel_ulong_t)NULL }, Hi Jeremy What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP MDIO bus master? A cluster of Ethernet controllers? Is this documented somewhere? In the DT world we have a clear documentation for all the compatible strings. Is there anything similar in the ACPI world for these magic numbers? Thanks Andrew
On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > From: Jeremy Linton <jeremy.linton@arm.com> > > Add ACPI support for xgmac MDIO bus registration while maintaining > the existing DT support. > > The function mdiobus_register() inside of_mdiobus_register(), brings > up all the PHYs on the mdio bus and attach them to the bus. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++-------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c > index c82c85ef5fb3..fb7f8caff643 100644 > --- a/drivers/net/ethernet/freescale/xgmac_mdio.c > +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c > @@ -245,14 +245,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 -EINVAL; > } I think, as you're completely rewriting the resource handling, it would be a good idea to switch over to using devm_* stuff here. void __iomem *regs; regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(regs)) { dev_err(&pdev->dev, "could not map resource: %pe\n", regs); return PTR_ERR(regs); } This also gets rid of the error checking on priv->mdio_base below, and of course the unmap/resource handling for priv->mdio_base in the error paths and device removal paths. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jun 17, 2020 at 8:49 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: ... > > - 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 -EINVAL; > > } > > I think, as you're completely rewriting the resource handling, it would > be a good idea to switch over to using devm_* stuff here. > > void __iomem *regs; > > regs = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(regs)) { > dev_err(&pdev->dev, "could not map resource: %pe\n", > regs); And just in case, this message is dup. The API has few of them depending on the error conditions. > return PTR_ERR(regs); > } -- With Best Regards, Andy Shevchenko
On Wed, Jun 17, 2020 at 08:54:23PM +0300, Andy Shevchenko wrote: > On Wed, Jun 17, 2020 at 8:49 PM Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > > ... > > > > - 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 -EINVAL; > > > } > > > > I think, as you're completely rewriting the resource handling, it would > > be a good idea to switch over to using devm_* stuff here. > > > > void __iomem *regs; > > > > regs = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(regs)) > { > > dev_err(&pdev->dev, "could not map resource: %pe\n", > > regs); > > And just in case, this message is dup. The API has few of them > depending on the error conditions. I did try to check for that, but it seems it was rather buried. This seems to have been a common mistake, as I've seen patches removing such things from various drivers, but I'm never sure which require that treatment. Maybe adding such details to the kerneldoc for the functions (maybe actually _writing_ some kernel doc to describe what the functions are doing) would be a good start to prevent this kind of thing... There's a difference between the lazy kerneldoc style of: /** * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform * device * * @pdev: platform device to use both for memory resource lookup as well as * resource management * @index: resource index */ and the "lets try to be informative" style: /** * phy_lookup_setting - lookup a PHY setting * @speed: speed to match * @duplex: duplex to match * @mask: allowed link modes * @exact: an exact match is required * * Search the settings array for a setting that matches the speed and * duplex, and which is supported. * * If @exact is unset, either an exact match or %NULL for no match will * be returned. * * If @exact is set, an exact match, the fastest supported setting at * or below the specified speed, the slowest supported setting, or if * they all fail, %NULL will be returned. */ ;) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi, On 6/17/20 12:34 PM, Andrew Lunn wrote: > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: >> From: Jeremy Linton <jeremy.linton@arm.com> > >> +static const struct acpi_device_id xgmac_acpi_match[] = { >> + { "NXP0006", (kernel_ulong_t)NULL }, > > Hi Jeremy > > What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some > NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP > MDIO bus master? A cluster of Ethernet controllers? Strictly speaking its a NXP defined (they own the "NXP" prefix per https://uefi.org/pnp_id_list) id. So, they have tied it to a specific bit of hardware. In this case it appears to be a shared MDIO master which isn't directly contained in an Ethernet controller. Its somewhat similar to a "nxp,xxxxx" compatible id, depending on how they are using it to identify an ACPI device object (_HID()/_CID()). So AFAIK, this is all valid ACPI usage as long as the ID maps to a unique device/object. > > Is this documented somewhere? In the DT world we have a clear > documentation for all the compatible strings. Is there anything > similar in the ACPI world for these magic numbers? Sadly not fully. The mentioned PNP and ACPI (https://uefi.org/acpi_id_list) ids lists are requested and registered to a given organization. But, once the prefix is owned, it becomes the responsibility of that organization to assign & manage the ID's with their prefix. There are various individuals/etc which have collected lists, though like PCI ids, there aren't any formal publishing requirements.
On Thu, Jun 18, 2020 at 6:46 PM Jeremy Linton <jeremy.linton@arm.com> wrote: > On 6/17/20 12:34 PM, Andrew Lunn wrote: > > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > >> From: Jeremy Linton <jeremy.linton@arm.com> > > > >> +static const struct acpi_device_id xgmac_acpi_match[] = { > >> + { "NXP0006", (kernel_ulong_t)NULL }, > > > > Hi Jeremy > > > > What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some > > NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP > > MDIO bus master? A cluster of Ethernet controllers? > > Strictly speaking its a NXP defined (they own the "NXP" prefix per > https://uefi.org/pnp_id_list) id. So, they have tied it to a specific > bit of hardware. In this case it appears to be a shared MDIO master > which isn't directly contained in an Ethernet controller. Its somewhat > similar to a "nxp,xxxxx" compatible id, depending on how they are using > it to identify an ACPI device object (_HID()/_CID()). > > So AFAIK, this is all valid ACPI usage as long as the ID maps to a > unique device/object. > > > > > Is this documented somewhere? In the DT world we have a clear > > documentation for all the compatible strings. Is there anything > > similar in the ACPI world for these magic numbers? > > Sadly not fully. The mentioned PNP and ACPI > (https://uefi.org/acpi_id_list) ids lists are requested and registered > to a given organization. But, once the prefix is owned, it becomes the > responsibility of that organization to assign & manage the ID's with > their prefix. There are various individuals/etc which have collected > lists, though like PCI ids, there aren't any formal publishing requirements. And here is the question, do we have (in form of email or other means) an official response from NXP about above mentioned ID? -- With Best Regards, Andy Shevchenko
Hi, On Thu, Jun 18, 2020 at 07:00:20PM +0300, Andy Shevchenko wrote: > On Thu, Jun 18, 2020 at 6:46 PM Jeremy Linton <jeremy.linton@arm.com> wrote: > > On 6/17/20 12:34 PM, Andrew Lunn wrote: > > > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > > >> From: Jeremy Linton <jeremy.linton@arm.com> > > > > > >> +static const struct acpi_device_id xgmac_acpi_match[] = { > > >> + { "NXP0006", (kernel_ulong_t)NULL }, > > > > > > Hi Jeremy > > > > > > What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some > > > NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP > > > MDIO bus master? A cluster of Ethernet controllers? > > > > Strictly speaking its a NXP defined (they own the "NXP" prefix per > > https://uefi.org/pnp_id_list) id. So, they have tied it to a specific > > bit of hardware. In this case it appears to be a shared MDIO master > > which isn't directly contained in an Ethernet controller. Its somewhat > > similar to a "nxp,xxxxx" compatible id, depending on how they are using > > it to identify an ACPI device object (_HID()/_CID()). > > > > So AFAIK, this is all valid ACPI usage as long as the ID maps to a > > unique device/object. > > > > > > > > Is this documented somewhere? In the DT world we have a clear > > > documentation for all the compatible strings. Is there anything > > > similar in the ACPI world for these magic numbers? > > > > Sadly not fully. The mentioned PNP and ACPI > > (https://uefi.org/acpi_id_list) ids lists are requested and registered > > to a given organization. But, once the prefix is owned, it becomes the > > responsibility of that organization to assign & manage the ID's with > > their prefix. There are various individuals/etc which have collected > > lists, though like PCI ids, there aren't any formal publishing requirements. > > And here is the question, do we have (in form of email or other means) > an official response from NXP about above mentioned ID? At NXP, we've assgined NXP ids to various controllers and "NXP0006" is assigned to the xgmac_mdio controller. Regards Calvin
On Thu, Jun 18, 2020 at 8:43 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > On Thu, Jun 18, 2020 at 07:00:20PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 18, 2020 at 6:46 PM Jeremy Linton <jeremy.linton@arm.com> wrote: ... > > And here is the question, do we have (in form of email or other means) > > an official response from NXP about above mentioned ID? > > At NXP, we've assgined NXP ids to various controllers and "NXP0006" is > assigned to the xgmac_mdio controller. Thanks! -- With Best Regards, Andy Shevchenko
On Wed, Jun 17, 2020 at 06:49:31PM +0100, Russell King - ARM Linux admin wrote: > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > > From: Jeremy Linton <jeremy.linton@arm.com> > > > > Add ACPI support for xgmac MDIO bus registration while maintaining > > the existing DT support. > > > > The function mdiobus_register() inside of_mdiobus_register(), brings > > up all the PHYs on the mdio bus and attach them to the bus. > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > > > drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++-------- > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c > > index c82c85ef5fb3..fb7f8caff643 100644 > > --- a/drivers/net/ethernet/freescale/xgmac_mdio.c > > +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c > > @@ -245,14 +245,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 -EINVAL; > > } > > I think, as you're completely rewriting the resource handling, it would > be a good idea to switch over to using devm_* stuff here. > > void __iomem *regs; > > regs = devm_platform_ioremap_resource(pdev, 0); I had used devm_ API earlier in this place and ran into a regression. This mdio driver is used by both DPAA-1 and DPAA-2. In DPAA2 case, this works fine. But in DPAA-1 case, the existing device tree describes the memory map in a hierarchical manner. The FMan of DPAA-1 area include the MDIO, Port, MAC areas. Therefore, we may have to continue with existing method. Thanks Calvin
On Fri, Jun 19, 2020 at 08:29:27PM +0530, Calvin Johnson wrote: > On Wed, Jun 17, 2020 at 06:49:31PM +0100, Russell King - ARM Linux admin wrote: > > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote: > > > From: Jeremy Linton <jeremy.linton@arm.com> > > > > > > Add ACPI support for xgmac MDIO bus registration while maintaining > > > the existing DT support. > > > > > > The function mdiobus_register() inside of_mdiobus_register(), brings > > > up all the PHYs on the mdio bus and attach them to the bus. > > > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > --- > > > > > > drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++-------- > > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c > > > index c82c85ef5fb3..fb7f8caff643 100644 > > > --- a/drivers/net/ethernet/freescale/xgmac_mdio.c > > > +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c > > > @@ -245,14 +245,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 -EINVAL; > > > } > > > > I think, as you're completely rewriting the resource handling, it would > > be a good idea to switch over to using devm_* stuff here. > > > > void __iomem *regs; > > > > regs = devm_platform_ioremap_resource(pdev, 0); > > I had used devm_ API earlier in this place and ran into a regression. > This mdio driver is used by both DPAA-1 and DPAA-2. In DPAA2 case, this > works fine. > > But in DPAA-1 case, the existing device tree describes the memory map in a > hierarchical manner. The FMan of DPAA-1 area include the MDIO, Port, MAC areas. > Therefore, we may have to continue with existing method. And you need to document that in comments in the driver, otherwise you will have people come along and try to "clean up" the driver. You can still use devm_ioremap() though. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c index c82c85ef5fb3..fb7f8caff643 100644 --- a/drivers/net/ethernet/freescale/xgmac_mdio.c +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c @@ -245,14 +245,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 -EINVAL; } bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv)); @@ -263,21 +263,21 @@ 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 = ioremap(res->start, resource_size(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->is_little_endian = device_property_read_bool(&pdev->dev, + "little-endian"); - priv->has_a011043 = of_property_read_bool(pdev->dev.of_node, - "fsl,erratum-a011043"); + priv->has_a011043 = device_property_read_bool(&pdev->dev, + "fsl,erratum-a011043"); ret = of_mdiobus_register(bus, np); if (ret) { @@ -320,10 +320,17 @@ static const struct of_device_id xgmac_mdio_match[] = { }; MODULE_DEVICE_TABLE(of, xgmac_mdio_match); +static const struct acpi_device_id xgmac_acpi_match[] = { + { "NXP0006", (kernel_ulong_t)NULL }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, xgmac_acpi_match); + static struct platform_driver xgmac_mdio_driver = { .driver = { .name = "fsl-fman_xmdio", .of_match_table = xgmac_mdio_match, + .acpi_match_table = xgmac_acpi_match, }, .probe = xgmac_mdio_probe, .remove = xgmac_mdio_remove,