| Message ID | 20200123060823.1902366-2-jeremy.linton@arm.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add ACPI bindings to the genet | expand |
On 1/22/20 10:08 PM, Jeremy Linton wrote: > The rpi4 is capable of booting in ACPI mode with the latest > edk2-platform commits. As such, it would be helpful if the genet > platform device were usable. > > To achive this we convert some of the of_ calls to device_ and > add the ACPI id module table, and tweak the phy connection code > to use phy_connect() in the ACPI path. This seems reasonable to me at first glance, although I would be splitting the bcmgenet.c changes from the bcmmii.c for clarity. There are some more specific comments below. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > .../net/ethernet/broadcom/genet/bcmgenet.c | 19 +++-- > drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++------- > 2 files changed, 63 insertions(+), 32 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index 120fa05a39ff..c736700f829e 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -7,6 +7,7 @@ > > #define pr_fmt(fmt) "bcmgenet: " fmt > > +#include <linux/acpi.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/sched.h> > @@ -3476,7 +3477,7 @@ static int bcmgenet_probe(struct platform_device *pdev) > const struct bcmgenet_plat_data *pdata; > struct bcmgenet_priv *priv; > struct net_device *dev; > - const void *macaddr; > + const void *macaddr = NULL; > unsigned int i; > int err = -EIO; > const char *phy_mode_str; > @@ -3510,7 +3511,7 @@ static int bcmgenet_probe(struct platform_device *pdev) > > if (dn) > macaddr = of_get_mac_address(dn); > - else > + else if (pd) > macaddr = pd->mac_address; > > priv->base = devm_platform_ioremap_resource(pdev, 0); > @@ -3555,8 +3556,9 @@ static int bcmgenet_probe(struct platform_device *pdev) > > priv->dev = dev; > priv->pdev = pdev; > - if (of_id) { > - pdata = of_id->data; > + > + pdata = device_get_match_data(&pdev->dev); > + if (pdata) { > priv->version = pdata->version; > priv->dma_max_burst_length = pdata->dma_max_burst_length; > } else { > @@ -3595,7 +3597,7 @@ static int bcmgenet_probe(struct platform_device *pdev) > /* If this is an internal GPHY, power it on now, before UniMAC is > * brought out of reset as absolutely no UniMAC activity is allowed > */ > - if (dn && !of_property_read_string(dn, "phy-mode", &phy_mode_str) && > + if (!device_property_read_string(&pdev->dev, "phy-mode", &phy_mode_str) && > !strcasecmp(phy_mode_str, "internal")) > bcmgenet_power_up(priv, GENET_POWER_PASSIVE); > > @@ -3768,6 +3770,12 @@ static int bcmgenet_suspend(struct device *d) > > static SIMPLE_DEV_PM_OPS(bcmgenet_pm_ops, bcmgenet_suspend, bcmgenet_resume); > > +static const struct acpi_device_id genet_acpi_match[] = { > + { "BCM6E4E", (kernel_ulong_t)&bcm2711_plat_data }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, genet_acpi_match); > + > static struct platform_driver bcmgenet_driver = { > .probe = bcmgenet_probe, > .remove = bcmgenet_remove, > @@ -3776,6 +3784,7 @@ static struct platform_driver bcmgenet_driver = { > .name = "bcmgenet", > .of_match_table = bcmgenet_match, > .pm = &bcmgenet_pm_ops, > + .acpi_match_table = ACPI_PTR(genet_acpi_match), > }, > }; > module_platform_driver(bcmgenet_driver); > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c > index 6392a2530183..054be1eaa1ae 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c > @@ -5,7 +5,7 @@ > * Copyright (c) 2014-2017 Broadcom > */ > > - > +#include <linux/acpi.h> > #include <linux/types.h> > #include <linux/delay.h> > #include <linux/wait.h> > @@ -308,10 +308,21 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) > return 0; > } > > +static void bcmgenet_phy_name(char *phy_name, int mdid, int phid) > +{ > + char mdio_bus_id[MII_BUS_ID_SIZE]; > + > + snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d", > + UNIMAC_MDIO_DRV_NAME, mdid); > + snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phid); > +} > + > int bcmgenet_mii_probe(struct net_device *dev) > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - struct device_node *dn = priv->pdev->dev.of_node; > + struct device *kdev = &priv->pdev->dev; > + struct device_node *dn = kdev->of_node; > + > struct phy_device *phydev; > u32 phy_flags = 0; > int ret; > @@ -333,6 +344,16 @@ int bcmgenet_mii_probe(struct net_device *dev) > pr_err("could not attach to PHY\n"); > return -ENODEV; > } > + } else if (has_acpi_companion(kdev)) { > + char phy_name[MII_BUS_ID_SIZE + 3]; > + > + bcmgenet_phy_name(phy_name, priv->pdev->id, 1); There is no guarantee that 1 is valid other than for the current Raspberry Pi 4 design that we have in the wild, would ACPI be used in the future with other designs, this would likely be different. Can you find a way to communicate that address via appropriate firmware properties? > + phydev = phy_connect(dev, phy_name, bcmgenet_mii_setup, > + priv->phy_interface); > + if (!IS_ERR(phydev)) > + phydev->dev_flags = phy_flags; > + else > + return -ENODEV; > } else { > phydev = dev->phydev; > phydev->dev_flags = phy_flags; > @@ -435,6 +456,7 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) > ppd.wait_func = bcmgenet_mii_wait; > ppd.wait_func_data = priv; > ppd.bus_name = "bcmgenet MII bus"; > + ppd.phy_mask = ~0; This is going to be breaking PHY scanning for the platform_data case, so maybe something like: if (acpi_has_companion()) ppd.phy_mask = ~BIT(acpi_phy_id); or something like that? > > /* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD > * and is 2 * 32-bits word long, 8 bytes total. > @@ -477,12 +499,28 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) > return ret; > } > > +static int bcmgenet_mii_phy_init(struct bcmgenet_priv *priv) > +{ Maybe name that phy_interface_init(), there is not strictly much PHY initialization going on here, just property fetching and internal book keeping.
Hi, First, thanks for taking a look at this. On 1/23/20 3:22 PM, Florian Fainelli wrote: > On 1/22/20 10:08 PM, Jeremy Linton wrote: >> The rpi4 is capable of booting in ACPI mode with the latest >> edk2-platform commits. As such, it would be helpful if the genet >> platform device were usable. >> >> To achive this we convert some of the of_ calls to device_ and >> add the ACPI id module table, and tweak the phy connection code >> to use phy_connect() in the ACPI path. > > This seems reasonable to me at first glance, although I would be > splitting the bcmgenet.c changes from the bcmmii.c for clarity. Sure. > > There are some more specific comments below. > >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> .../net/ethernet/broadcom/genet/bcmgenet.c | 19 +++-- >> drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++------- >> 2 files changed, 63 insertions(+), 32 deletions(-) >> (trimming some) >> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c >> index 6392a2530183..054be1eaa1ae 100644 >> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c >> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c >> @@ -5,7 +5,7 @@ >> * Copyright (c) 2014-2017 Broadcom >> */ >> >> - >> +#include <linux/acpi.h> >> #include <linux/types.h> >> #include <linux/delay.h> >> #include <linux/wait.h> >> @@ -308,10 +308,21 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) >> return 0; >> } >> >> +static void bcmgenet_phy_name(char *phy_name, int mdid, int phid) >> +{ >> + char mdio_bus_id[MII_BUS_ID_SIZE]; >> + >> + snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d", >> + UNIMAC_MDIO_DRV_NAME, mdid); >> + snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phid); >> +} >> + >> int bcmgenet_mii_probe(struct net_device *dev) >> { >> struct bcmgenet_priv *priv = netdev_priv(dev); >> - struct device_node *dn = priv->pdev->dev.of_node; >> + struct device *kdev = &priv->pdev->dev; >> + struct device_node *dn = kdev->of_node; >> + >> struct phy_device *phydev; >> u32 phy_flags = 0; >> int ret; >> @@ -333,6 +344,16 @@ int bcmgenet_mii_probe(struct net_device *dev) >> pr_err("could not attach to PHY\n"); >> return -ENODEV; >> } >> + } else if (has_acpi_companion(kdev)) { >> + char phy_name[MII_BUS_ID_SIZE + 3]; >> + >> + bcmgenet_phy_name(phy_name, priv->pdev->id, 1); > > There is no guarantee that 1 is valid other than for the current > Raspberry Pi 4 design that we have in the wild, would ACPI be used in > the future with other designs, this would likely be different. Can you > find a way to communicate that address via appropriate firmware properties? Your right, that "1" seems like it should be dynamic despite a large number of these nic drivers hardcoding the phy address. Another _DSD property could be added, but that likely just moves the hardcoding from one place to another. Particularly, given that the mdiobus_register() phy scanning is working correctly and the machine knows what the phy address is. AFAIK, The correct choice is something like phy_find_first(), but the mii_bus structure is layered down in the unimac module's private data, which we could retrieve, but that would be really ugly. There is of_mdio_find_bus(), but that doesn't apply either. A generic mdio_find_bus() that takes the bus name string, or maybe the parent device and does the bus_find_device itself would be helpful. Suggestions? > >> + phydev = phy_connect(dev, phy_name, bcmgenet_mii_setup, >> + priv->phy_interface); >> + if (!IS_ERR(phydev)) >> + phydev->dev_flags = phy_flags; >> + else >> + return -ENODEV; >> } else { >> phydev = dev->phydev; >> phydev->dev_flags = phy_flags; >> @@ -435,6 +456,7 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) >> ppd.wait_func = bcmgenet_mii_wait; >> ppd.wait_func_data = priv; >> ppd.bus_name = "bcmgenet MII bus"; >> + ppd.phy_mask = ~0; > > This is going to be breaking PHY scanning for the platform_data case, so > maybe something like: Does it? I thought it was being reset in bcmgenet_mii_pdata_init(). I guess I don't fully understand the what happens in PHY_INTERFACE_MODE_MOCA. > > if (acpi_has_companion()) > ppd.phy_mask = ~BIT(acpi_phy_id); > > or something like that? Sure, if nothing else I can wrap the mask in if (acpi) though, as I mentioned above I've got some reservations about picking up the phy id from the firmware unless its absolutely required. > >> >> /* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD >> * and is 2 * 32-bits word long, 8 bytes total. >> @@ -477,12 +499,28 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) >> return ret; >> } >> >> +static int bcmgenet_mii_phy_init(struct bcmgenet_priv *priv) >> +{ > > Maybe name that phy_interface_init(), there is not strictly much PHY > initialization going on here, just property fetching and internal book > keeping. > Yup, sounds good. Thanks, again!
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 120fa05a39ff..c736700f829e 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -7,6 +7,7 @@ #define pr_fmt(fmt) "bcmgenet: " fmt +#include <linux/acpi.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/sched.h> @@ -3476,7 +3477,7 @@ static int bcmgenet_probe(struct platform_device *pdev) const struct bcmgenet_plat_data *pdata; struct bcmgenet_priv *priv; struct net_device *dev; - const void *macaddr; + const void *macaddr = NULL; unsigned int i; int err = -EIO; const char *phy_mode_str; @@ -3510,7 +3511,7 @@ static int bcmgenet_probe(struct platform_device *pdev) if (dn) macaddr = of_get_mac_address(dn); - else + else if (pd) macaddr = pd->mac_address; priv->base = devm_platform_ioremap_resource(pdev, 0); @@ -3555,8 +3556,9 @@ static int bcmgenet_probe(struct platform_device *pdev) priv->dev = dev; priv->pdev = pdev; - if (of_id) { - pdata = of_id->data; + + pdata = device_get_match_data(&pdev->dev); + if (pdata) { priv->version = pdata->version; priv->dma_max_burst_length = pdata->dma_max_burst_length; } else { @@ -3595,7 +3597,7 @@ static int bcmgenet_probe(struct platform_device *pdev) /* If this is an internal GPHY, power it on now, before UniMAC is * brought out of reset as absolutely no UniMAC activity is allowed */ - if (dn && !of_property_read_string(dn, "phy-mode", &phy_mode_str) && + if (!device_property_read_string(&pdev->dev, "phy-mode", &phy_mode_str) && !strcasecmp(phy_mode_str, "internal")) bcmgenet_power_up(priv, GENET_POWER_PASSIVE); @@ -3768,6 +3770,12 @@ static int bcmgenet_suspend(struct device *d) static SIMPLE_DEV_PM_OPS(bcmgenet_pm_ops, bcmgenet_suspend, bcmgenet_resume); +static const struct acpi_device_id genet_acpi_match[] = { + { "BCM6E4E", (kernel_ulong_t)&bcm2711_plat_data }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, genet_acpi_match); + static struct platform_driver bcmgenet_driver = { .probe = bcmgenet_probe, .remove = bcmgenet_remove, @@ -3776,6 +3784,7 @@ static struct platform_driver bcmgenet_driver = { .name = "bcmgenet", .of_match_table = bcmgenet_match, .pm = &bcmgenet_pm_ops, + .acpi_match_table = ACPI_PTR(genet_acpi_match), }, }; module_platform_driver(bcmgenet_driver); diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 6392a2530183..054be1eaa1ae 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -5,7 +5,7 @@ * Copyright (c) 2014-2017 Broadcom */ - +#include <linux/acpi.h> #include <linux/types.h> #include <linux/delay.h> #include <linux/wait.h> @@ -308,10 +308,21 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) return 0; } +static void bcmgenet_phy_name(char *phy_name, int mdid, int phid) +{ + char mdio_bus_id[MII_BUS_ID_SIZE]; + + snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d", + UNIMAC_MDIO_DRV_NAME, mdid); + snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phid); +} + int bcmgenet_mii_probe(struct net_device *dev) { struct bcmgenet_priv *priv = netdev_priv(dev); - struct device_node *dn = priv->pdev->dev.of_node; + struct device *kdev = &priv->pdev->dev; + struct device_node *dn = kdev->of_node; + struct phy_device *phydev; u32 phy_flags = 0; int ret; @@ -333,6 +344,16 @@ int bcmgenet_mii_probe(struct net_device *dev) pr_err("could not attach to PHY\n"); return -ENODEV; } + } else if (has_acpi_companion(kdev)) { + char phy_name[MII_BUS_ID_SIZE + 3]; + + bcmgenet_phy_name(phy_name, priv->pdev->id, 1); + phydev = phy_connect(dev, phy_name, bcmgenet_mii_setup, + priv->phy_interface); + if (!IS_ERR(phydev)) + phydev->dev_flags = phy_flags; + else + return -ENODEV; } else { phydev = dev->phydev; phydev->dev_flags = phy_flags; @@ -435,6 +456,7 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) ppd.wait_func = bcmgenet_mii_wait; ppd.wait_func_data = priv; ppd.bus_name = "bcmgenet MII bus"; + ppd.phy_mask = ~0; /* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD * and is 2 * 32-bits word long, 8 bytes total. @@ -477,12 +499,28 @@ static int bcmgenet_mii_register(struct bcmgenet_priv *priv) return ret; } +static int bcmgenet_mii_phy_init(struct bcmgenet_priv *priv) +{ + struct device *kdev = &priv->pdev->dev; + + priv->phy_interface = device_get_phy_mode(kdev); + if (priv->phy_interface < 0) + priv->phy_interface = PHY_INTERFACE_MODE_RGMII; + + /* We need to specifically look up whether this PHY interface is internal + * or not *before* we even try to probe the PHY driver over MDIO as we + * may have shut down the internal PHY for power saving purposes. + */ + if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL) + priv->internal_phy = true; + + return 0; +} + static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv) { struct device_node *dn = priv->pdev->dev.of_node; - struct device *kdev = &priv->pdev->dev; struct phy_device *phydev; - phy_interface_t phy_mode; int ret; /* Fetch the PHY phandle */ @@ -500,23 +538,10 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv) } /* Get the link mode */ - ret = of_get_phy_mode(dn, &phy_mode); - if (ret) { - dev_err(kdev, "invalid PHY mode property\n"); - return ret; - } - - priv->phy_interface = phy_mode; - - /* We need to specifically look up whether this PHY interface is internal - * or not *before* we even try to probe the PHY driver over MDIO as we - * may have shut down the internal PHY for power saving purposes. - */ - if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL) - priv->internal_phy = true; + bcmgenet_mii_phy_init(priv); /* Make sure we initialize MoCA PHYs with a link down */ - if (phy_mode == PHY_INTERFACE_MODE_MOCA) { + if (priv->phy_interface == PHY_INTERFACE_MODE_MOCA) { phydev = of_phy_find_device(dn); if (phydev) { phydev->link = 0; @@ -532,16 +557,10 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) struct device *kdev = &priv->pdev->dev; struct bcmgenet_platform_data *pd = kdev->platform_data; char phy_name[MII_BUS_ID_SIZE + 3]; - char mdio_bus_id[MII_BUS_ID_SIZE]; struct phy_device *phydev; - snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d", - UNIMAC_MDIO_DRV_NAME, priv->pdev->id); - if (pd->phy_interface != PHY_INTERFACE_MODE_MOCA && pd->mdio_enabled) { - snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, - mdio_bus_id, pd->phy_address); - + bcmgenet_phy_name(phy_name, priv->pdev->id, pd->phy_address); /* * Internal or external PHY with MDIO access */ @@ -581,10 +600,13 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv) static int bcmgenet_mii_bus_init(struct bcmgenet_priv *priv) { - struct device_node *dn = priv->pdev->dev.of_node; + struct device *kdev = &priv->pdev->dev; + struct device_node *dn = kdev->of_node; if (dn) return bcmgenet_mii_of_init(priv); + else if (has_acpi_companion(kdev)) + return bcmgenet_mii_phy_init(priv); else return bcmgenet_mii_pd_init(priv); }
The rpi4 is capable of booting in ACPI mode with the latest edk2-platform commits. As such, it would be helpful if the genet platform device were usable. To achive this we convert some of the of_ calls to device_ and add the ACPI id module table, and tweak the phy connection code to use phy_connect() in the ACPI path. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- .../net/ethernet/broadcom/genet/bcmgenet.c | 19 +++-- drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++------- 2 files changed, 63 insertions(+), 32 deletions(-)