Re: [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter

[Florian Fainelli <f.fainelli@gmail.com>]

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