* [RFC 0/2] Add ACPI bindings to the genet
@ 2020-01-23 6:08 Jeremy Linton
2020-01-23 6:08 ` [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support Jeremy Linton
2020-01-23 6:08 ` [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter Jeremy Linton
0 siblings, 2 replies; 6+ messages in thread
From: Jeremy Linton @ 2020-01-23 6:08 UTC (permalink / raw)
To: netdev
Cc: opendmb, f.fainelli, davem, bcm-kernel-feedback-list,
linux-kernel, wahrenst, Jeremy Linton
This patch series allows the BCM GENET, as used on the RPi4,
to attach when booted in an ACPI enviroment. The DSDT entry to trigger
this is seen below. The second patch in the set retrieves the MAC
address from the umac registers rather than carrying it directly in
the DSDT. This of course requires the firmware to pre-program it, so
we continue to fall back to a random one if it appears to be garbage.
+ Device (ETH0)
+ {
+ Name (_HID, "BCM6E4E")
+ Name (_CID, "BCM6E4E")
+ Name (_UID, 0)
+ Name (_CCA, 0x0)
+ Method (_STA)
+ {
+ Return (0xf)
+ }
+ Method (_CRS, 0x0, Serialized)
+ {
+ Name (RBUF, ResourceTemplate ()
+ {
+ Memory32Fixed (ReadWrite, 0xFd580000, 0x10000, )
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBD }
+ Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBE }
+ })
+ Return (RBUF)
+ }
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () { "phy-mode", "rgmii" },
+ }
+ })
+ }
+
Jeremy Linton (2):
net: bcmgenet: Initial bcmgenet ACPI support
net: bcmgenet: Fetch MAC address from the adapter
.../net/ethernet/broadcom/genet/bcmgenet.c | 64 ++++++++++++----
drivers/net/ethernet/broadcom/genet/bcmmii.c | 76 ++++++++++++-------
2 files changed, 98 insertions(+), 42 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support 2020-01-23 6:08 [RFC 0/2] Add ACPI bindings to the genet Jeremy Linton @ 2020-01-23 6:08 ` Jeremy Linton 2020-01-23 21:22 ` Florian Fainelli 2020-01-23 6:08 ` [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter Jeremy Linton 1 sibling, 1 reply; 6+ messages in thread From: Jeremy Linton @ 2020-01-23 6:08 UTC (permalink / raw) To: netdev Cc: opendmb, f.fainelli, davem, bcm-kernel-feedback-list, linux-kernel, wahrenst, Jeremy Linton 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(-) 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); } -- 2.24.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support 2020-01-23 6:08 ` [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support Jeremy Linton @ 2020-01-23 21:22 ` Florian Fainelli 2020-01-23 22:32 ` Jeremy Linton 0 siblings, 1 reply; 6+ messages in thread From: Florian Fainelli @ 2020-01-23 21:22 UTC (permalink / raw) To: Jeremy Linton, netdev Cc: opendmb, f.fainelli, davem, bcm-kernel-feedback-list, linux-kernel, wahrenst 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. -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support 2020-01-23 21:22 ` Florian Fainelli @ 2020-01-23 22:32 ` Jeremy Linton 0 siblings, 0 replies; 6+ messages in thread From: Jeremy Linton @ 2020-01-23 22:32 UTC (permalink / raw) To: Florian Fainelli, netdev Cc: opendmb, davem, bcm-kernel-feedback-list, linux-kernel, wahrenst, Andrew Lunn 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! ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter 2020-01-23 6:08 [RFC 0/2] Add ACPI bindings to the genet Jeremy Linton 2020-01-23 6:08 ` [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support Jeremy Linton @ 2020-01-23 6:08 ` Jeremy Linton 2020-01-23 21:13 ` Florian Fainelli 1 sibling, 1 reply; 6+ messages in thread From: Jeremy Linton @ 2020-01-23 6:08 UTC (permalink / raw) To: netdev Cc: opendmb, f.fainelli, davem, bcm-kernel-feedback-list, linux-kernel, wahrenst, Jeremy Linton ARM/ACPI machines should utilize self describing hardware when possible. The MAC address on the BCM GENET can be read from the adapter if a full featured firmware has already programmed it. Lets try using the address already programmed, if it appears to be valid. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- .../net/ethernet/broadcom/genet/bcmgenet.c | 47 ++++++++++++++----- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index c736700f829e..6782bb0a24bd 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -2779,6 +2779,27 @@ static void bcmgenet_set_hw_addr(struct bcmgenet_priv *priv, bcmgenet_umac_writel(priv, (addr[4] << 8) | addr[5], UMAC_MAC1); } +static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv, + unsigned char *addr) +{ + u32 addr_tmp; + bool acpi_mode = has_acpi_companion(&priv->pdev->dev); + + /* UEFI/ACPI machines and possibly others will preprogram the MAC */ + if (acpi_mode) { + addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC0); + addr[0] = addr_tmp >> 24; + addr[1] = (addr_tmp >> 16) & 0xff; + addr[2] = (addr_tmp >> 8) & 0xff; + addr[3] = addr_tmp & 0xff; + addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC1); + addr[4] = (addr_tmp >> 8) & 0xff; + addr[5] = addr_tmp & 0xff; + } else { + memset(addr, 0, ETH_ALEN); + } +} + /* Returns a reusable dma control register value */ static u32 bcmgenet_dma_disable(struct bcmgenet_priv *priv) { @@ -3509,11 +3530,6 @@ static int bcmgenet_probe(struct platform_device *pdev) } priv->wol_irq = platform_get_irq_optional(pdev, 2); - if (dn) - macaddr = of_get_mac_address(dn); - else if (pd) - macaddr = pd->mac_address; - priv->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(priv->base)) { err = PTR_ERR(priv->base); @@ -3524,12 +3540,6 @@ static int bcmgenet_probe(struct platform_device *pdev) SET_NETDEV_DEV(dev, &pdev->dev); dev_set_drvdata(&pdev->dev, dev); - if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) { - dev_warn(&pdev->dev, "using random Ethernet MAC\n"); - eth_hw_addr_random(dev); - } else { - ether_addr_copy(dev->dev_addr, macaddr); - } dev->watchdog_timeo = 2 * HZ; dev->ethtool_ops = &bcmgenet_ethtool_ops; dev->netdev_ops = &bcmgenet_netdev_ops; @@ -3601,6 +3611,21 @@ static int bcmgenet_probe(struct platform_device *pdev) !strcasecmp(phy_mode_str, "internal")) bcmgenet_power_up(priv, GENET_POWER_PASSIVE); + if (dn) + macaddr = of_get_mac_address(dn); + else if (pd) + macaddr = pd->mac_address; + + if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) { + bcmgenet_get_hw_addr(priv, dev->dev_addr); + if (!is_valid_ether_addr(dev->dev_addr)) { + dev_warn(&pdev->dev, "using random Ethernet MAC\n"); + eth_hw_addr_random(dev); + } + } else { + ether_addr_copy(dev->dev_addr, macaddr); + } + reset_umac(priv); err = bcmgenet_mii_init(dev); -- 2.24.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter 2020-01-23 6:08 ` [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter Jeremy Linton @ 2020-01-23 21:13 ` Florian Fainelli 0 siblings, 0 replies; 6+ messages in thread From: Florian Fainelli @ 2020-01-23 21:13 UTC (permalink / raw) To: Jeremy Linton, netdev Cc: opendmb, davem, bcm-kernel-feedback-list, linux-kernel, wahrenst On 1/22/20 10:08 PM, Jeremy Linton wrote: > ARM/ACPI machines should utilize self describing hardware > when possible. The MAC address on the BCM GENET can be > read from the adapter if a full featured firmware has already > programmed it. Lets try using the address already programmed, > if it appears to be valid. s/BCM GENET/BCMGENET/ or just GENET. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > .../net/ethernet/broadcom/genet/bcmgenet.c | 47 ++++++++++++++----- > 1 file changed, 36 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index c736700f829e..6782bb0a24bd 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -2779,6 +2779,27 @@ static void bcmgenet_set_hw_addr(struct bcmgenet_priv *priv, > bcmgenet_umac_writel(priv, (addr[4] << 8) | addr[5], UMAC_MAC1); > } > > +static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv, > + unsigned char *addr) > +{ > + u32 addr_tmp; > + bool acpi_mode = has_acpi_companion(&priv->pdev->dev); > + > + /* UEFI/ACPI machines and possibly others will preprogram the MAC */ > + if (acpi_mode) { > + addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC0); > + addr[0] = addr_tmp >> 24; > + addr[1] = (addr_tmp >> 16) & 0xff; > + addr[2] = (addr_tmp >> 8) & 0xff; > + addr[3] = addr_tmp & 0xff; > + addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC1); > + addr[4] = (addr_tmp >> 8) & 0xff; > + addr[5] = addr_tmp & 0xff; > + } else { > + memset(addr, 0, ETH_ALEN); > + } > +} > + > /* Returns a reusable dma control register value */ > static u32 bcmgenet_dma_disable(struct bcmgenet_priv *priv) > { > @@ -3509,11 +3530,6 @@ static int bcmgenet_probe(struct platform_device *pdev) > } > priv->wol_irq = platform_get_irq_optional(pdev, 2); > > - if (dn) > - macaddr = of_get_mac_address(dn); > - else if (pd) > - macaddr = pd->mac_address; > - > priv->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(priv->base)) { > err = PTR_ERR(priv->base); > @@ -3524,12 +3540,6 @@ static int bcmgenet_probe(struct platform_device *pdev) > > SET_NETDEV_DEV(dev, &pdev->dev); > dev_set_drvdata(&pdev->dev, dev); > - if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) { > - dev_warn(&pdev->dev, "using random Ethernet MAC\n"); > - eth_hw_addr_random(dev); > - } else { > - ether_addr_copy(dev->dev_addr, macaddr); > - } > dev->watchdog_timeo = 2 * HZ; > dev->ethtool_ops = &bcmgenet_ethtool_ops; > dev->netdev_ops = &bcmgenet_netdev_ops; > @@ -3601,6 +3611,21 @@ static int bcmgenet_probe(struct platform_device *pdev) > !strcasecmp(phy_mode_str, "internal")) > bcmgenet_power_up(priv, GENET_POWER_PASSIVE); > > + if (dn) > + macaddr = of_get_mac_address(dn); > + else if (pd) > + macaddr = pd->mac_address; I would be adding an: else if (has_acpi_companion()) bcmgenet_get_hw_addr(priv, macaddr); such that bcmgenet_get_hw_addr() does not have much ACPI specific knowledge and get be used outside, with the caller knowing whether it is appropriate. You should also indicate in your commit message that you are moving the fetching of the MAC address at a later point where the clocks are turned on, to guarantee that the registers can be read. With that fixed: Acked-by: Florian Fainelli <f.fainelli@gmail.com> > + > + if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) { > + bcmgenet_get_hw_addr(priv, dev->dev_addr); > + if (!is_valid_ether_addr(dev->dev_addr)) { > + dev_warn(&pdev->dev, "using random Ethernet MAC\n"); > + eth_hw_addr_random(dev); > + } > + } else { > + ether_addr_copy(dev->dev_addr, macaddr); > + } > + > reset_umac(priv); > > err = bcmgenet_mii_init(dev); > -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-23 22:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-23 6:08 [RFC 0/2] Add ACPI bindings to the genet Jeremy Linton 2020-01-23 6:08 ` [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support Jeremy Linton 2020-01-23 21:22 ` Florian Fainelli 2020-01-23 22:32 ` Jeremy Linton 2020-01-23 6:08 ` [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter Jeremy Linton 2020-01-23 21:13 ` Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).