Re: [RFC 1/2] net: bcmgenet: Initial bcmgenet ACPI support

[Jeremy Linton]

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@xxxxxxx>
> > ---
> >   .../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!