Messages in this thread Patch in this message | | | Subject | Re: [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support | From | Florian Fainelli <> | Date | Thu, 23 Jan 2020 13:22:33 -0800 |
| |
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
| |