[U-Boot] Setting up MAC address for eth drivers

[U-Boot] Setting up MAC address for eth drivers

[Michal Simek]

Hi,

I have a question regarding setting mac address for drivers.
Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
which is called from common/board_r.c.

But then there are some drivers(macb, designware, altera_tse) which also calls
mac setup from dev->init which has side effect for example when you setup CONFIG_ENV_OVERWRITE
and change mac address you can directly use it.

It also means if there is intention to call hwaddr from dev->init
that for the first packet mac address is setup twice - in eth core init
and then before every driver use.

I am asking this question because I would like to know the right flow
for eth mac setup.
If it is
set ethaddr xx....
saveenv
reset
eth with new mac

or if
set ethaddr
eth with new mac
should work

The second approach looks reasonable when ethaddr is not set at all
but then at least our driver needs to be fixed to support this feature.

Thanks,
Michal

[U-Boot] Setting up MAC address for eth drivers

[Michal Simek]

Hi,

any update on this one?

Thanks,
Michal

On 03/13/2015 01:25 PM, Michal Simek wrote:
> Hi,
> 
> I have a question regarding setting mac address for drivers.
> Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
> which is called from common/board_r.c.
> 
> But then there are some drivers(macb, designware, altera_tse) which also calls
> mac setup from dev->init which has side effect for example when you setup CONFIG_ENV_OVERWRITE
> and change mac address you can directly use it.
> 
> It also means if there is intention to call hwaddr from dev->init
> that for the first packet mac address is setup twice - in eth core init
> and then before every driver use.
> 
> I am asking this question because I would like to know the right flow
> for eth mac setup.
> If it is
> set ethaddr xx....
> saveenv
> reset
> eth with new mac
> 
> or if
> set ethaddr
> eth with new mac
> should work
> 
> The second approach looks reasonable when ethaddr is not set at all
> but then at least our driver needs to be fixed to support this feature.
> 
> Thanks,
> Michal
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150317/68647c18/attachment.sig>

[U-Boot] Setting up MAC address for eth drivers

[Joe Hershberger]

Hi Michal,

On Fri, Mar 13, 2015 at 7:25 AM, Michal Simek <michal.simek@xilinx.com>
wrote:
>
> Hi,
>
> I have a question regarding setting mac address for drivers.
> Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
> which is called from common/board_r.c.

This is because on at least ARM kernels, the MAC is passed via the MAC's
filter registers. Because of this, we need to write hwaddr even before the
MAC is started.

> But then there are some drivers(macb, designware, altera_tse) which also
calls
> mac setup from dev->init which has side effect for example when you setup
CONFIG_ENV_OVERWRITE
> and change mac address you can directly use it.

This is probably a work-around for a shortcoming of the net/eth.c not
calling write_hwaddr() when the env MAC changes. It already updates the
registers used by the network stack, so presumably if those MACs are
filtering incoming traffic based on the register as expected, changing the
MAC after boot without rewriting that register would cause all traffic to
not be returned.

> It also means if there is intention to call hwaddr from dev->init
> that for the first packet mac address is setup twice - in eth core init
> and then before every driver use.

The design of the network stack assumes the MAC is started each time a
network operation is requested and stopped when done.

As for the setting of the hwaddr, we should check if it changed.

> I am asking this question because I would like to know the right flow
> for eth mac setup.
> If it is
> set ethaddr xx....
> saveenv
> reset
> eth with new mac
>
> or if
> set ethaddr
> eth with new mac
> should work
>
> The second approach looks reasonable when ethaddr is not set at all
> but then at least our driver needs to be fixed to support this feature.

I think the second should work ideally, but we need to add a call to
eth_init() if the "ethaddr" in the env changes and set it again if so. We
should also remove the calls from the drivers you identified since the
framework will now do it.

Cheers,
-Joe

[U-Boot] Setting up MAC address for eth drivers

[Michal Simek]

Hi Joe,

On 03/17/2015 04:56 PM, Joe Hershberger wrote:
> Hi Michal,
> 
> On Fri, Mar 13, 2015 at 7:25 AM, Michal Simek <michal.simek@xilinx.com>
> wrote:
>>
>> Hi,
>>
>> I have a question regarding setting mac address for drivers.
>> Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
>> which is called from common/board_r.c.
> 
> This is because on at least ARM kernels, the MAC is passed via the MAC's
> filter registers. Because of this, we need to write hwaddr even before the
> MAC is started.
> 
>> But then there are some drivers(macb, designware, altera_tse) which also
> calls
>> mac setup from dev->init which has side effect for example when you setup
> CONFIG_ENV_OVERWRITE
>> and change mac address you can directly use it.
> 
> This is probably a work-around for a shortcoming of the net/eth.c not
> calling write_hwaddr() when the env MAC changes. It already updates the
> registers used by the network stack, so presumably if those MACs are
> filtering incoming traffic based on the register as expected, changing the
> MAC after boot without rewriting that register would cause all traffic to
> not be returned.
> 
>> It also means if there is intention to call hwaddr from dev->init
>> that for the first packet mac address is setup twice - in eth core init
>> and then before every driver use.
> 
> The design of the network stack assumes the MAC is started each time a
> network operation is requested and stopped when done.
> 
> As for the setting of the hwaddr, we should check if it changed.
> 
>> I am asking this question because I would like to know the right flow
>> for eth mac setup.
>> If it is
>> set ethaddr xx....
>> saveenv
>> reset
>> eth with new mac
>>
>> or if
>> set ethaddr
>> eth with new mac
>> should work
>>
>> The second approach looks reasonable when ethaddr is not set at all
>> but then at least our driver needs to be fixed to support this feature.
> 
> I think the second should work ideally, but we need to add a call to
> eth_init() if the "ethaddr" in the env changes and set it again if so. We
> should also remove the calls from the drivers you identified since the
> framework will now do it.

Does it mean that you are going to write that code for it?

Thanks,
Michal

[U-Boot] Setting up MAC address for eth drivers

[Joe Hershberger]

Hi Michal,

On Tue, Mar 17, 2015 at 11:57 AM, Michal Simek <michal.simek@xilinx.com>
wrote:
>
> Hi Joe,
>
> On 03/17/2015 04:56 PM, Joe Hershberger wrote:
> > Hi Michal,
> >
> > On Fri, Mar 13, 2015 at 7:25 AM, Michal Simek <michal.simek@xilinx.com>
> > wrote:
> >>
> >> Hi,
> >>
> >> I have a question regarding setting mac address for drivers.
> >> Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
> >> which is called from common/board_r.c.
> >
> > This is because on at least ARM kernels, the MAC is passed via the MAC's
> > filter registers. Because of this, we need to write hwaddr even before
the
> > MAC is started.
> >
> >> But then there are some drivers(macb, designware, altera_tse) which
also
> > calls
> >> mac setup from dev->init which has side effect for example when you
setup
> > CONFIG_ENV_OVERWRITE
> >> and change mac address you can directly use it.
> >
> > This is probably a work-around for a shortcoming of the net/eth.c not
> > calling write_hwaddr() when the env MAC changes. It already updates the
> > registers used by the network stack, so presumably if those MACs are
> > filtering incoming traffic based on the register as expected, changing
the
> > MAC after boot without rewriting that register would cause all traffic
to
> > not be returned.
> >
> >> It also means if there is intention to call hwaddr from dev->init
> >> that for the first packet mac address is setup twice - in eth core init
> >> and then before every driver use.
> >
> > The design of the network stack assumes the MAC is started each time a
> > network operation is requested and stopped when done.
> >
> > As for the setting of the hwaddr, we should check if it changed.
> >
> >> I am asking this question because I would like to know the right flow
> >> for eth mac setup.
> >> If it is
> >> set ethaddr xx....
> >> saveenv
> >> reset
> >> eth with new mac
> >>
> >> or if
> >> set ethaddr
> >> eth with new mac
> >> should work
> >>
> >> The second approach looks reasonable when ethaddr is not set at all
> >> but then at least our driver needs to be fixed to support this feature.
> >
> > I think the second should work ideally, but we need to add a call to
> > eth_init() if the "ethaddr" in the env changes and set it again if so.
We
> > should also remove the calls from the drivers you identified since the
> > framework will now do it.
>
> Does it mean that you are going to write that code for it?

Yes... I'll write it, but it needs to be on top of the DM_ETH patches, so
I'll probably wait until that's pulled.

-Joe

[U-Boot] Setting up MAC address for eth drivers

[Michal Simek]

Hi Joe,

On 03/17/2015 06:21 PM, Joe Hershberger wrote:
> Hi Michal,
> 
> On Tue, Mar 17, 2015 at 11:57 AM, Michal Simek <michal.simek@xilinx.com>
> wrote:
>>
>> Hi Joe,
>>
>> On 03/17/2015 04:56 PM, Joe Hershberger wrote:
>>> Hi Michal,
>>>
>>> On Fri, Mar 13, 2015 at 7:25 AM, Michal Simek <michal.simek@xilinx.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I have a question regarding setting mac address for drivers.
>>>> Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
>>>> which is called from common/board_r.c.
>>>
>>> This is because on at least ARM kernels, the MAC is passed via the MAC's
>>> filter registers. Because of this, we need to write hwaddr even before
> the
>>> MAC is started.
>>>
>>>> But then there are some drivers(macb, designware, altera_tse) which
> also
>>> calls
>>>> mac setup from dev->init which has side effect for example when you
> setup
>>> CONFIG_ENV_OVERWRITE
>>>> and change mac address you can directly use it.
>>>
>>> This is probably a work-around for a shortcoming of the net/eth.c not
>>> calling write_hwaddr() when the env MAC changes. It already updates the
>>> registers used by the network stack, so presumably if those MACs are
>>> filtering incoming traffic based on the register as expected, changing
> the
>>> MAC after boot without rewriting that register would cause all traffic
> to
>>> not be returned.
>>>
>>>> It also means if there is intention to call hwaddr from dev->init
>>>> that for the first packet mac address is setup twice - in eth core init
>>>> and then before every driver use.
>>>
>>> The design of the network stack assumes the MAC is started each time a
>>> network operation is requested and stopped when done.
>>>
>>> As for the setting of the hwaddr, we should check if it changed.
>>>
>>>> I am asking this question because I would like to know the right flow
>>>> for eth mac setup.
>>>> If it is
>>>> set ethaddr xx....
>>>> saveenv
>>>> reset
>>>> eth with new mac
>>>>
>>>> or if
>>>> set ethaddr
>>>> eth with new mac
>>>> should work
>>>>
>>>> The second approach looks reasonable when ethaddr is not set at all
>>>> but then at least our driver needs to be fixed to support this feature.
>>>
>>> I think the second should work ideally, but we need to add a call to
>>> eth_init() if the "ethaddr" in the env changes and set it again if so.
> We
>>> should also remove the calls from the drivers you identified since the
>>> framework will now do it.
>>
>> Does it mean that you are going to write that code for it?
> 
> Yes... I'll write it, but it needs to be on top of the DM_ETH patches, so
> I'll probably wait until that's pulled.

That's fine.

Thanks,
Michal

[U-Boot] Setting up MAC address for eth drivers

[Simon Glass]

Hi,

On 17 March 2015 at 11:47, Michal Simek <michal.simek@xilinx.com> wrote:
> Hi Joe,
>
> On 03/17/2015 06:21 PM, Joe Hershberger wrote:
>> Hi Michal,
>>
>> On Tue, Mar 17, 2015 at 11:57 AM, Michal Simek <michal.simek@xilinx.com>
>> wrote:
>>>
>>> Hi Joe,
>>>
>>> On 03/17/2015 04:56 PM, Joe Hershberger wrote:
>>>> Hi Michal,
>>>>
>>>> On Fri, Mar 13, 2015 at 7:25 AM, Michal Simek <michal.simek@xilinx.com>
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I have a question regarding setting mac address for drivers.
>>>>> Drivers setting up write_hwaddr via eth_write_hwaddr via eth_initialize
>>>>> which is called from common/board_r.c.
>>>>
>>>> This is because on at least ARM kernels, the MAC is passed via the MAC's
>>>> filter registers. Because of this, we need to write hwaddr even before
>> the
>>>> MAC is started.
>>>>
>>>>> But then there are some drivers(macb, designware, altera_tse) which
>> also
>>>> calls
>>>>> mac setup from dev->init which has side effect for example when you
>> setup
>>>> CONFIG_ENV_OVERWRITE
>>>>> and change mac address you can directly use it.
>>>>
>>>> This is probably a work-around for a shortcoming of the net/eth.c not
>>>> calling write_hwaddr() when the env MAC changes. It already updates the
>>>> registers used by the network stack, so presumably if those MACs are
>>>> filtering incoming traffic based on the register as expected, changing
>> the
>>>> MAC after boot without rewriting that register would cause all traffic
>> to
>>>> not be returned.
>>>>
>>>>> It also means if there is intention to call hwaddr from dev->init
>>>>> that for the first packet mac address is setup twice - in eth core init
>>>>> and then before every driver use.
>>>>
>>>> The design of the network stack assumes the MAC is started each time a
>>>> network operation is requested and stopped when done.
>>>>
>>>> As for the setting of the hwaddr, we should check if it changed.
>>>>
>>>>> I am asking this question because I would like to know the right flow
>>>>> for eth mac setup.
>>>>> If it is
>>>>> set ethaddr xx....
>>>>> saveenv
>>>>> reset
>>>>> eth with new mac
>>>>>
>>>>> or if
>>>>> set ethaddr
>>>>> eth with new mac
>>>>> should work
>>>>>
>>>>> The second approach looks reasonable when ethaddr is not set at all
>>>>> but then at least our driver needs to be fixed to support this feature.
>>>>
>>>> I think the second should work ideally, but we need to add a call to
>>>> eth_init() if the "ethaddr" in the env changes and set it again if so.
>> We
>>>> should also remove the calls from the drivers you identified since the
>>>> framework will now do it.
>>>
>>> Does it mean that you are going to write that code for it?
>>
>> Yes... I'll write it, but it needs to be on top of the DM_ETH patches, so
>> I'll probably wait until that's pulled.
>
> That's fine.

This is in u-boot-dm/next now.

Regards,
Simon

[U-Boot] [PATCH v1] net: Update hardware MAC address if it changes in env

[Joe Hershberger]

When the ethaddr changes in the env, the hardware should also be updated
so that MAC filtering will work properly without resetting U-Boot.

Also remove the manual calls to set the hwaddr that was included in a
few drivers as a result of the framework not doing it.

Reported-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

---

 drivers/net/bcm-sf2-eth.c |  6 ------
 drivers/net/designware.c  |  4 ----
 drivers/net/macb.c        |  9 ---------
 net/eth.c                 | 15 +++++++++++++--
 4 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/net/bcm-sf2-eth.c b/drivers/net/bcm-sf2-eth.c
index 5252d49..9ae88d3 100644
--- a/drivers/net/bcm-sf2-eth.c
+++ b/drivers/net/bcm-sf2-eth.c
@@ -154,12 +154,6 @@ static int bcm_sf2_eth_open(struct eth_device *dev, bd_t *bt)
 
 	debug("Enabling BCM SF2 Ethernet.\n");
 
-	/* Set MAC address from env */
-	if (bcm_sf2_eth_write_hwaddr(dev) != 0) {
-		error("%s: MAC set error when opening !\n", __func__);
-		return -1;
-	}
-
 	eth->enable_mac();
 
 	/* enable tx and rx DMA */
diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index cc01604..d35236b 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -244,10 +244,6 @@ static int dw_eth_init(struct eth_device *dev, bd_t *bis)
 		mdelay(100);
 	};
 
-	/* Soft reset above clears HW address registers.
-	 * So we have to set it here once again */
-	dw_write_hwaddr(dev);
-
 	rx_descs_init(dev);
 	tx_descs_init(dev);
 
diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 170ff06..7d5b36b 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -525,7 +525,6 @@ static int macb_phy_init(struct macb_device *macb)
 	return 1;
 }
 
-static int macb_write_hwaddr(struct eth_device *dev);
 static int macb_init(struct eth_device *netdev, bd_t *bd)
 {
 	struct macb_device *macb = to_macb(netdev);
@@ -594,14 +593,6 @@ static int macb_init(struct eth_device *netdev, bd_t *bd)
 #endif /* CONFIG_RMII */
 	}
 
-	/* update the ethaddr */
-	if (is_valid_ether_addr(netdev->enetaddr)) {
-		macb_write_hwaddr(netdev);
-	} else {
-		printf("%s: mac address is not valid\n", netdev->name);
-		return -1;
-	}
-
 	if (!macb_phy_init(macb))
 		return -1;
 
diff --git a/net/eth.c b/net/eth.c
index 13b7723..776fd2f 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -258,13 +258,24 @@ int eth_init(void)
 		if (device_active(current)) {
 			uchar env_enetaddr[6];
 			struct eth_pdata *pdata = current->platdata;
+			int enetaddr_changed = 0;
 
 			/* Sync environment with network device */
 			if (eth_getenv_enetaddr_by_index("eth", current->seq,
-							 env_enetaddr))
+							 env_enetaddr)) {
+				enetaddr_changed = memcmp(pdata->enetaddr,
+					env_enetaddr, 6);
 				memcpy(pdata->enetaddr, env_enetaddr, 6);
-			else
+			} else {
+				memset(env_enetaddr, 0, 6);
+				enetaddr_changed = memcmp(pdata->enetaddr,
+					env_enetaddr, 6);
 				memset(pdata->enetaddr, 0, 6);
+			}
+			if (enetaddr_changed &&
+			    eth_get_ops(current)->write_hwaddr) {
+				eth_get_ops(current)->write_hwaddr(current);
+			}
 
 			ret = eth_get_ops(current)->start(current);
 			if (ret >= 0) {
-- 
1.7.11.5

[U-Boot] [PATCH v1] net: Update hardware MAC address if it changes in env

[Joe Hershberger]

On Tue, Mar 24, 2015 at 1:49 AM, Joe Hershberger <joe.hershberger@ni.com>
wrote:
>
> When the ethaddr changes in the env, the hardware should also be updated
> so that MAC filtering will work properly without resetting U-Boot.
>
> Also remove the manual calls to set the hwaddr that was included in a
> few drivers as a result of the framework not doing it.
>
> Reported-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> ---
>
>  drivers/net/bcm-sf2-eth.c |  6 ------
>  drivers/net/designware.c  |  4 ----
>  drivers/net/macb.c        |  9 ---------
>  net/eth.c                 | 15 +++++++++++++--
>  4 files changed, 13 insertions(+), 21 deletions(-)

This requires dm/next

[U-Boot] [PATCH v2] net: Update hardware MAC address if it changes in env

[Joe Hershberger]

When the ethaddr changes in the env, the hardware should also be updated
so that MAC filtering will work properly without resetting U-Boot.

Also remove the manual calls to set the hwaddr that was included in a
few drivers as a result of the framework not doing it.

Reported-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

---

Changes in v2:
-Implement the check in non-DM eth_initialize also
-Use static helper function in DM case

 drivers/net/bcm-sf2-eth.c |  6 ----
 drivers/net/designware.c  |  4 ---
 drivers/net/macb.c        |  9 ------
 net/eth.c                 | 76 ++++++++++++++++++++++++++++++-----------------
 4 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/drivers/net/bcm-sf2-eth.c b/drivers/net/bcm-sf2-eth.c
index 5252d49..9ae88d3 100644
--- a/drivers/net/bcm-sf2-eth.c
+++ b/drivers/net/bcm-sf2-eth.c
@@ -154,12 +154,6 @@ static int bcm_sf2_eth_open(struct eth_device *dev, bd_t *bt)
 
 	debug("Enabling BCM SF2 Ethernet.\n");
 
-	/* Set MAC address from env */
-	if (bcm_sf2_eth_write_hwaddr(dev) != 0) {
-		error("%s: MAC set error when opening !\n", __func__);
-		return -1;
-	}
-
 	eth->enable_mac();
 
 	/* enable tx and rx DMA */
diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index cc01604..d35236b 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -244,10 +244,6 @@ static int dw_eth_init(struct eth_device *dev, bd_t *bis)
 		mdelay(100);
 	};
 
-	/* Soft reset above clears HW address registers.
-	 * So we have to set it here once again */
-	dw_write_hwaddr(dev);
-
 	rx_descs_init(dev);
 	tx_descs_init(dev);
 
diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 170ff06..7d5b36b 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -525,7 +525,6 @@ static int macb_phy_init(struct macb_device *macb)
 	return 1;
 }
 
-static int macb_write_hwaddr(struct eth_device *dev);
 static int macb_init(struct eth_device *netdev, bd_t *bd)
 {
 	struct macb_device *macb = to_macb(netdev);
@@ -594,14 +593,6 @@ static int macb_init(struct eth_device *netdev, bd_t *bd)
 #endif /* CONFIG_RMII */
 	}
 
-	/* update the ethaddr */
-	if (is_valid_ether_addr(netdev->enetaddr)) {
-		macb_write_hwaddr(netdev);
-	} else {
-		printf("%s: mac address is not valid\n", netdev->name);
-		return -1;
-	}
-
 	if (!macb_phy_init(macb))
 		return -1;
 
diff --git a/net/eth.c b/net/eth.c
index 13b7723..ed2d52a 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -239,6 +239,31 @@ int eth_get_dev_index(void)
 	return -1;
 }
 
+static int eth_write_hwaddr(struct udevice *dev)
+{
+	struct eth_pdata *pdata = dev->platdata;
+	int ret = 0;
+
+	if (!dev || !device_active(dev))
+		return -EINVAL;
+
+	/* seq is valid since the device is active */
+	if (eth_get_ops(dev)->write_hwaddr && !eth_mac_skip(dev->seq)) {
+		if (!is_valid_ether_addr(pdata->enetaddr)) {
+			printf("\nError: %s address %pM illegal value\n",
+			       dev->name, pdata->enetaddr);
+			return -EINVAL;
+		}
+
+		ret = eth_get_ops(dev)->write_hwaddr(dev);
+		if (ret)
+			printf("\nWarning: %s failed to set MAC address\n",
+			       dev->name);
+	}
+
+	return ret;
+}
+
 int eth_init(void)
 {
 	struct udevice *current;
@@ -258,13 +283,22 @@ int eth_init(void)
 		if (device_active(current)) {
 			uchar env_enetaddr[6];
 			struct eth_pdata *pdata = current->platdata;
+			int enetaddr_changed = 0;
 
 			/* Sync environment with network device */
 			if (eth_getenv_enetaddr_by_index("eth", current->seq,
-							 env_enetaddr))
+							 env_enetaddr)) {
+				enetaddr_changed = memcmp(pdata->enetaddr,
+					env_enetaddr, 6);
 				memcpy(pdata->enetaddr, env_enetaddr, 6);
-			else
+			} else {
+				memset(env_enetaddr, 0, 6);
+				enetaddr_changed = memcmp(pdata->enetaddr,
+					env_enetaddr, 6);
 				memset(pdata->enetaddr, 0, 6);
+			}
+			if (enetaddr_changed)
+				eth_write_hwaddr(current);
 
 			ret = eth_get_ops(current)->start(current);
 			if (ret >= 0) {
@@ -356,31 +390,6 @@ int eth_rx(void)
 	return ret;
 }
 
-static int eth_write_hwaddr(struct udevice *dev)
-{
-	struct eth_pdata *pdata = dev->platdata;
-	int ret = 0;
-
-	if (!dev || !device_active(dev))
-		return -EINVAL;
-
-	/* seq is valid since the device is active */
-	if (eth_get_ops(dev)->write_hwaddr && !eth_mac_skip(dev->seq)) {
-		if (!is_valid_ether_addr(pdata->enetaddr)) {
-			printf("\nError: %s address %pM illegal value\n",
-			       dev->name, pdata->enetaddr);
-			return -EINVAL;
-		}
-
-		ret = eth_get_ops(dev)->write_hwaddr(dev);
-		if (ret)
-			printf("\nWarning: %s failed to set MAC address\n",
-			       dev->name);
-	}
-
-	return ret;
-}
-
 int eth_initialize(void)
 {
 	int num_devices = 0;
@@ -820,10 +829,21 @@ int eth_init(void)
 	dev = eth_devices;
 	do {
 		uchar env_enetaddr[6];
+		int enetaddr_changed = 0;
 
 		if (eth_getenv_enetaddr_by_index("eth", dev->index,
-						 env_enetaddr))
+						 env_enetaddr)) {
+			enetaddr_changed = memcmp(dev->enetaddr,
+				env_enetaddr, 6);
 			memcpy(dev->enetaddr, env_enetaddr, 6);
+		} else {
+			memset(env_enetaddr, 0, 6);
+			enetaddr_changed = memcmp(dev->enetaddr,
+				env_enetaddr, 6);
+			memset(dev->enetaddr, 0, 6);
+		}
+		if (enetaddr_changed)
+			eth_write_hwaddr(dev, "eth", dev->index);
 
 		dev = dev->next;
 	} while (dev != eth_devices);
-- 
1.7.11.5

[U-Boot] [PATCH v2] net: Update hardware MAC address if it changes in env

[Joe Hershberger]

Hi Michal,

On Tue, Mar 24, 2015 at 2:41 AM, Joe Hershberger <joe.hershberger@ni.com>
wrote:
>
> When the ethaddr changes in the env, the hardware should also be updated
> so that MAC filtering will work properly without resetting U-Boot.
>
> Also remove the manual calls to set the hwaddr that was included in a
> few drivers as a result of the framework not doing it.
>
> Reported-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> ---
>
> Changes in v2:
> -Implement the check in non-DM eth_initialize also
> -Use static helper function in DM case
>
>  drivers/net/bcm-sf2-eth.c |  6 ----
>  drivers/net/designware.c  |  4 ---
>  drivers/net/macb.c        |  9 ------
>  net/eth.c                 | 76
++++++++++++++++++++++++++++++-----------------
>  4 files changed, 48 insertions(+), 47 deletions(-)

Does this sufficiently resolve the issue you have?

Thanks,
-Joe

[U-Boot] [PATCH v2] net: Update hardware MAC address if it changes in env

[Michal Simek]

On 03/30/2015 08:08 PM, Joe Hershberger wrote:
> Hi Michal,
> 
> On Tue, Mar 24, 2015 at 2:41 AM, Joe Hershberger <joe.hershberger@ni.com>
> wrote:
>>
>> When the ethaddr changes in the env, the hardware should also be updated
>> so that MAC filtering will work properly without resetting U-Boot.
>>
>> Also remove the manual calls to set the hwaddr that was included in a
>> few drivers as a result of the framework not doing it.
>>
>> Reported-by: Michal Simek <michal.simek@xilinx.com>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>
>> ---
>>
>> Changes in v2:
>> -Implement the check in non-DM eth_initialize also
>> -Use static helper function in DM case
>>
>>  drivers/net/bcm-sf2-eth.c |  6 ----
>>  drivers/net/designware.c  |  4 ---
>>  drivers/net/macb.c        |  9 ------
>>  net/eth.c                 | 76
> ++++++++++++++++++++++++++++++-----------------
>>  4 files changed, 48 insertions(+), 47 deletions(-)
> 
> Does this sufficiently resolve the issue you have?

I wouldn't say it was my issue. It was more about asking what's the
right thing to do.
But anyway I have tested it with our ethernet driver and it is working fine.

I have applied it on the top of u-boot-dm/next.

Tested-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

[U-Boot] [PATCH v2] net: Update hardware MAC address if it changes in env

[Joe Hershberger]

On Tue, Mar 31, 2015 at 6:41 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 03/30/2015 08:08 PM, Joe Hershberger wrote:
>> Hi Michal,
>>
>> On Tue, Mar 24, 2015 at 2:41 AM, Joe Hershberger <joe.hershberger@ni.com>
>> wrote:
>>>
>>> When the ethaddr changes in the env, the hardware should also be updated
>>> so that MAC filtering will work properly without resetting U-Boot.
>>>
>>> Also remove the manual calls to set the hwaddr that was included in a
>>> few drivers as a result of the framework not doing it.
>>>
>>> Reported-by: Michal Simek <michal.simek@xilinx.com>
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> -Implement the check in non-DM eth_initialize also
>>> -Use static helper function in DM case
>>>
>>>  drivers/net/bcm-sf2-eth.c |  6 ----
>>>  drivers/net/designware.c  |  4 ---
>>>  drivers/net/macb.c        |  9 ------
>>>  net/eth.c                 | 76
>> ++++++++++++++++++++++++++++++-----------------
>>>  4 files changed, 48 insertions(+), 47 deletions(-)
>>
>> Does this sufficiently resolve the issue you have?
>
> I wouldn't say it was my issue. It was more about asking what's the
> right thing to do.
> But anyway I have tested it with our ethernet driver and it is working fine.
>
> I have applied it on the top of u-boot-dm/next.
>
> Tested-by: Michal Simek <michal.simek@xilinx.com>

Applied to u-boot-net/next!
-Joe

[U-Boot] [PATCH v1] net: Update hardware MAC address if it changes in env

[Bin Meng]

Hi Joe,

On Tue, Mar 24, 2015 at 2:49 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
> When the ethaddr changes in the env, the hardware should also be updated
> so that MAC filtering will work properly without resetting U-Boot.
>
> Also remove the manual calls to set the hwaddr that was included in a
> few drivers as a result of the framework not doing it.
>
> Reported-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> ---
>
>  drivers/net/bcm-sf2-eth.c |  6 ------
>  drivers/net/designware.c  |  4 ----
>  drivers/net/macb.c        |  9 ---------
>  net/eth.c                 | 15 +++++++++++++--
>  4 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/bcm-sf2-eth.c b/drivers/net/bcm-sf2-eth.c
> index 5252d49..9ae88d3 100644
> --- a/drivers/net/bcm-sf2-eth.c
> +++ b/drivers/net/bcm-sf2-eth.c
> @@ -154,12 +154,6 @@ static int bcm_sf2_eth_open(struct eth_device *dev, bd_t *bt)
>
>         debug("Enabling BCM SF2 Ethernet.\n");
>
> -       /* Set MAC address from env */
> -       if (bcm_sf2_eth_write_hwaddr(dev) != 0) {
> -               error("%s: MAC set error when opening !\n", __func__);
> -               return -1;
> -       }
> -
>         eth->enable_mac();
>
>         /* enable tx and rx DMA */
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index cc01604..d35236b 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -244,10 +244,6 @@ static int dw_eth_init(struct eth_device *dev, bd_t *bis)
>                 mdelay(100);
>         };
>
> -       /* Soft reset above clears HW address registers.
> -        * So we have to set it here once again */
> -       dw_write_hwaddr(dev);
> -
>         rx_descs_init(dev);
>         tx_descs_init(dev);
>
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 170ff06..7d5b36b 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -525,7 +525,6 @@ static int macb_phy_init(struct macb_device *macb)
>         return 1;
>  }
>
> -static int macb_write_hwaddr(struct eth_device *dev);
>  static int macb_init(struct eth_device *netdev, bd_t *bd)
>  {
>         struct macb_device *macb = to_macb(netdev);
> @@ -594,14 +593,6 @@ static int macb_init(struct eth_device *netdev, bd_t *bd)
>  #endif /* CONFIG_RMII */
>         }
>
> -       /* update the ethaddr */
> -       if (is_valid_ether_addr(netdev->enetaddr)) {
> -               macb_write_hwaddr(netdev);
> -       } else {
> -               printf("%s: mac address is not valid\n", netdev->name);
> -               return -1;
> -       }
> -
>         if (!macb_phy_init(macb))
>                 return -1;
>
> diff --git a/net/eth.c b/net/eth.c
> index 13b7723..776fd2f 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -258,13 +258,24 @@ int eth_init(void)
>                 if (device_active(current)) {
>                         uchar env_enetaddr[6];
>                         struct eth_pdata *pdata = current->platdata;
> +                       int enetaddr_changed = 0;
>
>                         /* Sync environment with network device */
>                         if (eth_getenv_enetaddr_by_index("eth", current->seq,
> -                                                        env_enetaddr))
> +                                                        env_enetaddr)) {
> +                               enetaddr_changed = memcmp(pdata->enetaddr,
> +                                       env_enetaddr, 6);
>                                 memcpy(pdata->enetaddr, env_enetaddr, 6);
> -                       else
> +                       } else {
> +                               memset(env_enetaddr, 0, 6);
> +                               enetaddr_changed = memcmp(pdata->enetaddr,
> +                                       env_enetaddr, 6);
>                                 memset(pdata->enetaddr, 0, 6);
> +                       }
> +                       if (enetaddr_changed &&
> +                           eth_get_ops(current)->write_hwaddr) {
> +                               eth_get_ops(current)->write_hwaddr(current);
> +                       }
>
>                         ret = eth_get_ops(current)->start(current);
>                         if (ret >= 0) {
> --

I've created a u-boot.rom for Intel Galileo board and found the
designware ethernet port does not work any more. Git bisect shows that
this commit broke the things. Do you have any idea on what might be
the issue?

Regards,
Bin

[U-Boot] [PATCH v1] net: Update hardware MAC address if it changes in env

[Bin Meng]

On Mon, Jun 15, 2015 at 5:07 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Tue, Mar 24, 2015 at 2:49 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> When the ethaddr changes in the env, the hardware should also be updated
>> so that MAC filtering will work properly without resetting U-Boot.
>>
>> Also remove the manual calls to set the hwaddr that was included in a
>> few drivers as a result of the framework not doing it.
>>
>> Reported-by: Michal Simek <michal.simek@xilinx.com>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>
>> ---
>>
>>  drivers/net/bcm-sf2-eth.c |  6 ------
>>  drivers/net/designware.c  |  4 ----
>>  drivers/net/macb.c        |  9 ---------
>>  net/eth.c                 | 15 +++++++++++++--
>>  4 files changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/bcm-sf2-eth.c b/drivers/net/bcm-sf2-eth.c
>> index 5252d49..9ae88d3 100644
>> --- a/drivers/net/bcm-sf2-eth.c
>> +++ b/drivers/net/bcm-sf2-eth.c
>> @@ -154,12 +154,6 @@ static int bcm_sf2_eth_open(struct eth_device *dev, bd_t *bt)
>>
>>         debug("Enabling BCM SF2 Ethernet.\n");
>>
>> -       /* Set MAC address from env */
>> -       if (bcm_sf2_eth_write_hwaddr(dev) != 0) {
>> -               error("%s: MAC set error when opening !\n", __func__);
>> -               return -1;
>> -       }
>> -
>>         eth->enable_mac();
>>
>>         /* enable tx and rx DMA */
>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
>> index cc01604..d35236b 100644
>> --- a/drivers/net/designware.c
>> +++ b/drivers/net/designware.c
>> @@ -244,10 +244,6 @@ static int dw_eth_init(struct eth_device *dev, bd_t *bis)
>>                 mdelay(100);
>>         };
>>
>> -       /* Soft reset above clears HW address registers.
>> -        * So we have to set it here once again */
>> -       dw_write_hwaddr(dev);
>> -
>>         rx_descs_init(dev);
>>         tx_descs_init(dev);
>>
>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>> index 170ff06..7d5b36b 100644
>> --- a/drivers/net/macb.c
>> +++ b/drivers/net/macb.c
>> @@ -525,7 +525,6 @@ static int macb_phy_init(struct macb_device *macb)
>>         return 1;
>>  }
>>
>> -static int macb_write_hwaddr(struct eth_device *dev);
>>  static int macb_init(struct eth_device *netdev, bd_t *bd)
>>  {
>>         struct macb_device *macb = to_macb(netdev);
>> @@ -594,14 +593,6 @@ static int macb_init(struct eth_device *netdev, bd_t *bd)
>>  #endif /* CONFIG_RMII */
>>         }
>>
>> -       /* update the ethaddr */
>> -       if (is_valid_ether_addr(netdev->enetaddr)) {
>> -               macb_write_hwaddr(netdev);
>> -       } else {
>> -               printf("%s: mac address is not valid\n", netdev->name);
>> -               return -1;
>> -       }
>> -
>>         if (!macb_phy_init(macb))
>>                 return -1;
>>
>> diff --git a/net/eth.c b/net/eth.c
>> index 13b7723..776fd2f 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -258,13 +258,24 @@ int eth_init(void)
>>                 if (device_active(current)) {
>>                         uchar env_enetaddr[6];
>>                         struct eth_pdata *pdata = current->platdata;
>> +                       int enetaddr_changed = 0;
>>
>>                         /* Sync environment with network device */
>>                         if (eth_getenv_enetaddr_by_index("eth", current->seq,
>> -                                                        env_enetaddr))
>> +                                                        env_enetaddr)) {
>> +                               enetaddr_changed = memcmp(pdata->enetaddr,
>> +                                       env_enetaddr, 6);
>>                                 memcpy(pdata->enetaddr, env_enetaddr, 6);
>> -                       else
>> +                       } else {
>> +                               memset(env_enetaddr, 0, 6);
>> +                               enetaddr_changed = memcmp(pdata->enetaddr,
>> +                                       env_enetaddr, 6);
>>                                 memset(pdata->enetaddr, 0, 6);
>> +                       }
>> +                       if (enetaddr_changed &&
>> +                           eth_get_ops(current)->write_hwaddr) {
>> +                               eth_get_ops(current)->write_hwaddr(current);
>> +                       }
>>
>>                         ret = eth_get_ops(current)->start(current);
>>                         if (ret >= 0) {
>> --
>
> I've created a u-boot.rom for Intel Galileo board and found the
> designware ethernet port does not work any more. Git bisect shows that
> this commit broke the things. Do you have any idea on what might be
> the issue?
>

Further investigation shows that we should not remove writing
designware MAC address after soft reset. The patch [1] is sent out.

[1] http://patchwork.ozlabs.org/patch/484214/

Regards,
Bin