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

[Jeremy Linton <jeremy.linton@arm.com>]

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!