Re: [PATCH v2 5/8] net: pxa168_eth: get and set the mac address on the Ethernet controller

[Jason Cooper <jason@lakedaemon.net>]

On Tue, Sep 09, 2014 at 04:44:05PM +0200, Antoine Tenart wrote:
> When changing the MAC address, in addition to updating the dev_addr in
> the net_device structure, this patch also update the MAC address
> registers (high and low) of the Ethernet controller with the new MAC.
> The address stored in these registers is used for IEEE 802.3x Ethernet
> flow control, which is already enabled.
> 
> This patch also tries reading the MAC address stored in these registers
> when probing the driver, to use the MAC address set by the bootloader
> and avoid using a random one.

Hmm, the wording here seems odd.  I think the preference should be:

 1) bootloader-supplied addr via DT
 2) addr read from device
 3) randomly generated one.

> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/net/ethernet/marvell/pxa168_eth.c | 36 +++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> index 10422f2df6cc..0ecbb3903b71 100644
> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> @@ -60,6 +60,8 @@
>  #define PORT_COMMAND		0x0410
>  #define PORT_STATUS		0x0418
>  #define HTPR			0x0428
> +#define MAC_ADDR_LOW		0x0430
> +#define MAC_ADDR_HIGH		0x0438
>  #define SDMA_CONFIG		0x0440
>  #define SDMA_CMD		0x0448
>  #define INT_CAUSE		0x0450
> @@ -604,16 +606,42 @@ static void pxa168_eth_set_rx_mode(struct net_device *dev)
>  		update_hash_table_mac_address(pep, NULL, ha->addr);
>  }
>  
> +static void pxa168_eth_get_mac_address(struct net_device *dev,
> +				       unsigned char *addr)
> +{
> +	struct pxa168_eth_private *pep = netdev_priv(dev);
> +	unsigned int mac_h = rdl(pep, MAC_ADDR_HIGH);
> +	unsigned int mac_l = rdl(pep, MAC_ADDR_LOW);
> +
> +	addr[0] = (mac_h >> 24) & 0xff;
> +	addr[1] = (mac_h >> 16) & 0xff;
> +	addr[2] = (mac_h >> 8) & 0xff;
> +	addr[3] = mac_h & 0xff;
> +	addr[4] = (mac_l >> 8) & 0xff;
> +	addr[5] = mac_l & 0xff;
> +}
> +
>  static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
>  {
>  	struct sockaddr *sa = addr;
>  	struct pxa168_eth_private *pep = netdev_priv(dev);
>  	unsigned char oldMac[ETH_ALEN];
> +	u32 mac_h, mac_l;
>  
>  	if (!is_valid_ether_addr(sa->sa_data))
>  		return -EADDRNOTAVAIL;
>  	memcpy(oldMac, dev->dev_addr, ETH_ALEN);
>  	memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
> +
> +	mac_h = sa->sa_data[0] << 24;
> +	mac_h |= sa->sa_data[1] << 16;
> +	mac_h |= sa->sa_data[2] << 8;
> +	mac_h |= sa->sa_data[3];
> +	mac_l = sa->sa_data[4] << 8;
> +	mac_l |= sa->sa_data[5];
> +	wrl(pep, MAC_ADDR_HIGH, mac_h);
> +	wrl(pep, MAC_ADDR_LOW, mac_l);
> +
>  	netif_addr_lock_bh(dev);
>  	update_hash_table_mac_address(pep, oldMac, dev->dev_addr);
>  	netif_addr_unlock_bh(dev);
> @@ -1494,8 +1522,12 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>  
>  	INIT_WORK(&pep->tx_timeout_task, pxa168_eth_tx_timeout_task);
>  
> -	dev_info(&pdev->dev, "Using random mac address\n");
> -	eth_hw_addr_random(dev);
> +	/* try reading the mac address, if set by the bootloader */
> +	pxa168_eth_get_mac_address(dev, dev->dev_addr);
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		dev_info(&pdev->dev, "Using random mac address\n");
> +		eth_hw_addr_random(dev);
> +	}

I would do the above iff there was no valid addr supplied by the DT.

thx,

Jason.
>  
>  	pep->rx_ring_size = NUM_RX_DESCS;
>  	pep->tx_ring_size = NUM_TX_DESCS;
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html