drivers: net: fsl_enetc: use write_hwaddr()

Intead of setting the MAC address in enetc_start() use the proper
write_hwaddr(). U-Boot takes care of the random MAC address, too. Also,
this will correctly handle ethNmacskip etc.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/fsl_enetc.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Hi Michael,

On 12/18/2019 5:47 PM, Michael Walle wrote:
> Intead of setting the MAC address in enetc_start() use the proper
> write_hwaddr(). U-Boot takes care of the random MAC address, too. Also,
> this will correctly handle ethNmacskip etc.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>   drivers/net/fsl_enetc.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
> index e86f3dddb5..1949530460 100644
> --- a/drivers/net/fsl_enetc.c
> +++ b/drivers/net/fsl_enetc.c
> @@ -272,14 +272,19 @@ static int enetc_remove(struct udevice *dev)
>   	return 0;
>   }
>   
> -/* ENETC Port MAC address registers, accepts big-endian format */
> -static void enetc_set_primary_mac_addr(struct enetc_priv *priv, const u8 *addr)
> +static int enetc_write_hwaddr(struct udevice *dev)
>   {
> +	struct eth_pdata *plat = dev_get_platdata(dev);
> +	struct enetc_priv *priv = dev_get_priv(dev);
> +	u8 *addr = plat->enetaddr;
> +
>   	u16 lower = *(const u16 *)(addr + 4);
>   	u32 upper = *(const u32 *)addr;
>   
>   	enetc_write_port(priv, ENETC_PSIPMAR0, upper);
>   	enetc_write_port(priv, ENETC_PSIPMAR1, lower);

The SI registers hold temporary values which are cleared on FLR.  These 
MAC addresses will be wiped out at the next _start.  The persistent 
values are the one stored in IERB.  Is this what you want?

> +
> +	return 0;
>   }
>   
>   /* Configure port parameters (# of rings, frame size, enable port) */
> @@ -410,7 +415,6 @@ static void enetc_setup_rx_bdr(struct udevice *dev)
>    */
>   static int enetc_start(struct udevice *dev)
>   {
> -	struct eth_pdata *plat = dev_get_platdata(dev);
>   	struct enetc_priv *priv = dev_get_priv(dev);
>   
>   	/* reset and enable the PCI device */
> @@ -418,12 +422,6 @@ static int enetc_start(struct udevice *dev)
>   	dm_pci_clrset_config16(dev, PCI_COMMAND, 0,
>   			       PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
>   
> -	if (!is_valid_ethaddr(plat->enetaddr)) {
> -		enetc_dbg(dev, "invalid MAC address, generate random ...\n");
> -		net_random_ethaddr(plat->enetaddr);
> -	}
> -	enetc_set_primary_mac_addr(priv, plat->enetaddr);
> -

This looks like the right thing to do, but with enetc_write_hwaddr 
writing to IERB.  Do you want to send a v2, or do you want me to pick it up?

Thanks!
Alex

>   	enetc_enable_si_port(priv);
>   
>   	/* setup Tx/Rx buffer descriptors */
> @@ -549,6 +547,7 @@ static const struct eth_ops enetc_ops = {
>   	.send	= enetc_send,
>   	.recv	= enetc_recv,
>   	.stop	= enetc_stop,
> +	.write_hwaddr = enetc_write_hwaddr,
>   };
>   
>   U_BOOT_DRIVER(eth_enetc) = {
>

Am 2019-12-18 19:44, schrieb Alexandru Marginean:
> Hi Michael,
> 
> On 12/18/2019 5:47 PM, Michael Walle wrote:
>> Intead of setting the MAC address in enetc_start() use the proper
>> write_hwaddr(). U-Boot takes care of the random MAC address, too. 
>> Also,
>> this will correctly handle ethNmacskip etc.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>   drivers/net/fsl_enetc.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
>> index e86f3dddb5..1949530460 100644
>> --- a/drivers/net/fsl_enetc.c
>> +++ b/drivers/net/fsl_enetc.c
>> @@ -272,14 +272,19 @@ static int enetc_remove(struct udevice *dev)
>>   	return 0;
>>   }
>>   -/* ENETC Port MAC address registers, accepts big-endian format */
>> -static void enetc_set_primary_mac_addr(struct enetc_priv *priv, const 
>> u8 *addr)
>> +static int enetc_write_hwaddr(struct udevice *dev)
>>   {
>> +	struct eth_pdata *plat = dev_get_platdata(dev);
>> +	struct enetc_priv *priv = dev_get_priv(dev);
>> +	u8 *addr = plat->enetaddr;
>> +
>>   	u16 lower = *(const u16 *)(addr + 4);
>>   	u32 upper = *(const u32 *)addr;
>>     	enetc_write_port(priv, ENETC_PSIPMAR0, upper);
>>   	enetc_write_port(priv, ENETC_PSIPMAR1, lower);
> 
> The SI registers hold temporary values which are cleared on FLR.
> These MAC addresses will be wiped out at the next _start.  The
> persistent values are the one stored in IERB.  Is this what you want?

Yes that was actually the intention. These register names are very
confusing ;)

> 
>> +
>> +	return 0;
>>   }
>>     /* Configure port parameters (# of rings, frame size, enable port) 
>> */
>> @@ -410,7 +415,6 @@ static void enetc_setup_rx_bdr(struct udevice 
>> *dev)
>>    */
>>   static int enetc_start(struct udevice *dev)
>>   {
>> -	struct eth_pdata *plat = dev_get_platdata(dev);
>>   	struct enetc_priv *priv = dev_get_priv(dev);
>>     	/* reset and enable the PCI device */
>> @@ -418,12 +422,6 @@ static int enetc_start(struct udevice *dev)
>>   	dm_pci_clrset_config16(dev, PCI_COMMAND, 0,
>>   			       PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
>>   -	if (!is_valid_ethaddr(plat->enetaddr)) {
>> -		enetc_dbg(dev, "invalid MAC address, generate random ...\n");
>> -		net_random_ethaddr(plat->enetaddr);
>> -	}
>> -	enetc_set_primary_mac_addr(priv, plat->enetaddr);
>> -
> 
> This looks like the right thing to do, but with enetc_write_hwaddr
> writing to IERB.  Do you want to send a v2, or do you want me to pick
> it up?

I'll send a v2 tomorrow.

-michael