[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Hannes Schmelzer]

This commit ports the existing (non-DM) function for writing the MAC-
address into the shadow ram (and flash) for DM.

Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
---

Changes in v2:
- fix build for non-DM board
- rebase on current master

 drivers/net/e1000.c | 89 +++++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index a34f697..3df9999 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -862,7 +862,6 @@ e1000_read_eeprom(struct e1000_hw *hw, uint16_t offset,
 	return E1000_SUCCESS;
 }
 
-#ifndef CONFIG_DM_ETH
 /******************************************************************************
  *  e1000_write_eeprom_srwr - Write to Shadow Ram using EEWR
  *  @hw: pointer to the HW structure
@@ -1028,7 +1027,6 @@ static int32_t e1000_update_eeprom_checksum_i210(struct e1000_hw *hw)
 out:
 	return ret_val;
 }
-#endif
 
 /******************************************************************************
  * Verifies that the EEPROM has a valid checksum
@@ -5488,6 +5486,53 @@ void e1000_get_bus_type(struct e1000_hw *hw)
 }
 
 #ifndef CONFIG_DM_ETH
+static int e1000_write_hwaddr(struct eth_device *dev)
+#else
+static int e1000_write_hwaddr(struct udevice *dev)
+#endif
+{
+#ifndef CONFIG_E1000_NO_NVM
+#ifndef CONFIG_DM_ETH
+	unsigned char *mac = dev->enetaddr;
+	struct e1000_hw *hw = dev->priv;
+#else
+	struct eth_pdata *plat = dev_get_platdata(dev);
+	unsigned char *mac = plat->enetaddr;
+	struct e1000_hw *hw = dev_get_priv(dev);
+#endif
+	unsigned char current_mac[6];
+	u16 data[3];
+	int ret_val, i;
+
+	DEBUGOUT("%s: mac=%pM\n", __func__, mac);
+
+	memset(current_mac, 0, 6);
+
+	/* Read from EEPROM, not from registers, to make sure
+	 * the address is persistently configured
+	 */
+	ret_val = e1000_read_mac_addr_from_eeprom(hw, current_mac);
+	DEBUGOUT("%s: current mac=%pM\n", __func__, current_mac);
+
+	/* Only write to EEPROM if the given address is different or
+	 * reading the current address failed
+	 */
+	if (!ret_val && memcmp(current_mac, mac, 6) == 0)
+		return 0;
+
+	for (i = 0; i < 3; ++i)
+		data[i] = mac[i * 2 + 1] << 8 | mac[i * 2];
+
+	ret_val = e1000_write_eeprom_srwr(hw, 0x0, 3, data);
+	if (!ret_val)
+		ret_val = e1000_update_eeprom_checksum_i210(hw);
+	return ret_val;
+#else	/* CONFIG_E1000_NO_NVM */
+	return 0;
+#endif
+}
+
+#ifndef CONFIG_DM_ETH
 /* A list of all registered e1000 devices */
 static LIST_HEAD(e1000_hw_list);
 #endif
@@ -5649,45 +5694,6 @@ e1000_poll(struct eth_device *nic)
 	return len ? 1 : 0;
 }
 
-static int e1000_write_hwaddr(struct eth_device *dev)
-{
-#ifndef CONFIG_E1000_NO_NVM
-	unsigned char *mac = dev->enetaddr;
-	unsigned char current_mac[6];
-	struct e1000_hw *hw = dev->priv;
-	uint16_t data[3];
-	int ret_val, i;
-
-	DEBUGOUT("%s: mac=%pM\n", __func__, mac);
-
-	memset(current_mac, 0, 6);
-
-	/* Read from EEPROM, not from registers, to make sure
-	 * the address is persistently configured
-	 */
-	ret_val = e1000_read_mac_addr_from_eeprom(hw, current_mac);
-	DEBUGOUT("%s: current mac=%pM\n", __func__, current_mac);
-
-	/* Only write to EEPROM if the given address is different or
-	 * reading the current address failed
-	 */
-	if (!ret_val && memcmp(current_mac, mac, 6) == 0)
-		return 0;
-
-	for (i = 0; i < 3; ++i)
-		data[i] = mac[i * 2 + 1] << 8 | mac[i * 2];
-
-	ret_val = e1000_write_eeprom_srwr(hw, 0x0, 3, data);
-
-	if (!ret_val)
-		ret_val = e1000_update_eeprom_checksum_i210(hw);
-
-	return ret_val;
-#else
-	return 0;
-#endif
-}
-
 /**************************************************************************
 PROBE - Look for an adapter, this routine's visible to the outside
 You should omit the last argument struct pci_device * for a non-PCI NIC
@@ -5914,6 +5920,7 @@ static const struct eth_ops e1000_eth_ops = {
 	.recv	= e1000_eth_recv,
 	.stop	= e1000_eth_stop,
 	.free_pkt = e1000_free_pkt,
+	.write_hwaddr = e1000_write_hwaddr,
 };
 
 static const struct udevice_id e1000_eth_ids[] = {
-- 
2.7.4

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Joe Hershberger]

On Thu, Oct 25, 2018 at 9:26 AM Hannes Schmelzer
<hannes.schmelzer@br-automation.com> wrote:
>
> This commit ports the existing (non-DM) function for writing the MAC-
> address into the shadow ram (and flash) for DM.
>
> Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
> ---
>
> Changes in v2:
> - fix build for non-DM board
> - rebase on current master
>
>  drivers/net/e1000.c | 89 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 48 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
> index a34f697..3df9999 100644
> --- a/drivers/net/e1000.c
> +++ b/drivers/net/e1000.c
> @@ -862,7 +862,6 @@ e1000_read_eeprom(struct e1000_hw *hw, uint16_t offset,
>         return E1000_SUCCESS;
>  }
>
> -#ifndef CONFIG_DM_ETH
>  /******************************************************************************
>   *  e1000_write_eeprom_srwr - Write to Shadow Ram using EEWR
>   *  @hw: pointer to the HW structure
> @@ -1028,7 +1027,6 @@ static int32_t e1000_update_eeprom_checksum_i210(struct e1000_hw *hw)
>  out:
>         return ret_val;
>  }
> -#endif
>
>  /******************************************************************************
>   * Verifies that the EEPROM has a valid checksum
> @@ -5488,6 +5486,53 @@ void e1000_get_bus_type(struct e1000_hw *hw)
>  }
>
>  #ifndef CONFIG_DM_ETH

Please don't use negative logic when an else is present. Switch the
cases and use #ifdef.

> +static int e1000_write_hwaddr(struct eth_device *dev)
> +#else
> +static int e1000_write_hwaddr(struct udevice *dev)
> +#endif
> +{
> +#ifndef CONFIG_E1000_NO_NVM

Same here

> +#ifndef CONFIG_DM_ETH

Same here

> +       unsigned char *mac = dev->enetaddr;
> +       struct e1000_hw *hw = dev->priv;
> +#else
> +       struct eth_pdata *plat = dev_get_platdata(dev);
> +       unsigned char *mac = plat->enetaddr;
> +       struct e1000_hw *hw = dev_get_priv(dev);
> +#endif
> +       unsigned char current_mac[6];
> +       u16 data[3];
> +       int ret_val, i;
> +
> +       DEBUGOUT("%s: mac=%pM\n", __func__, mac);
> +
> +       memset(current_mac, 0, 6);
> +
> +       /* Read from EEPROM, not from registers, to make sure

Please use a /* on a blank line for multi-line comments.

> +        * the address is persistently configured
> +        */
> +       ret_val = e1000_read_mac_addr_from_eeprom(hw, current_mac);
> +       DEBUGOUT("%s: current mac=%pM\n", __func__, current_mac);
> +
> +       /* Only write to EEPROM if the given address is different or

Please use a /* on a blank line for multi-line comments.

> +        * reading the current address failed
> +        */
> +       if (!ret_val && memcmp(current_mac, mac, 6) == 0)
> +               return 0;
> +
> +       for (i = 0; i < 3; ++i)
> +               data[i] = mac[i * 2 + 1] << 8 | mac[i * 2];
> +
> +       ret_val = e1000_write_eeprom_srwr(hw, 0x0, 3, data);

This is a pretty uncommon implementation of this driver operation.
Does the i120 in this case read the MAC directly from the EEPROM?

Usually this function is used to write the hwaddr into the registers
of the MAC. The .read_rom_hwaddr is used to fetch the hwaddr from the
EEPROM and that address is used in the case that the U-Boot env has
not set one.

> +       if (!ret_val)
> +               ret_val = e1000_update_eeprom_checksum_i210(hw);
> +       return ret_val;
> +#else  /* CONFIG_E1000_NO_NVM */
> +       return 0;
> +#endif
> +}
> +
> +#ifndef CONFIG_DM_ETH
>  /* A list of all registered e1000 devices */
>  static LIST_HEAD(e1000_hw_list);
>  #endif
> @@ -5649,45 +5694,6 @@ e1000_poll(struct eth_device *nic)
>         return len ? 1 : 0;
>  }
>
> -static int e1000_write_hwaddr(struct eth_device *dev)
> -{
> -#ifndef CONFIG_E1000_NO_NVM
> -       unsigned char *mac = dev->enetaddr;
> -       unsigned char current_mac[6];
> -       struct e1000_hw *hw = dev->priv;
> -       uint16_t data[3];
> -       int ret_val, i;
> -
> -       DEBUGOUT("%s: mac=%pM\n", __func__, mac);
> -
> -       memset(current_mac, 0, 6);
> -
> -       /* Read from EEPROM, not from registers, to make sure
> -        * the address is persistently configured
> -        */
> -       ret_val = e1000_read_mac_addr_from_eeprom(hw, current_mac);
> -       DEBUGOUT("%s: current mac=%pM\n", __func__, current_mac);
> -
> -       /* Only write to EEPROM if the given address is different or
> -        * reading the current address failed
> -        */
> -       if (!ret_val && memcmp(current_mac, mac, 6) == 0)
> -               return 0;
> -
> -       for (i = 0; i < 3; ++i)
> -               data[i] = mac[i * 2 + 1] << 8 | mac[i * 2];
> -
> -       ret_val = e1000_write_eeprom_srwr(hw, 0x0, 3, data);
> -
> -       if (!ret_val)
> -               ret_val = e1000_update_eeprom_checksum_i210(hw);
> -
> -       return ret_val;
> -#else
> -       return 0;
> -#endif
> -}
> -
>  /**************************************************************************
>  PROBE - Look for an adapter, this routine's visible to the outside
>  You should omit the last argument struct pci_device * for a non-PCI NIC
> @@ -5914,6 +5920,7 @@ static const struct eth_ops e1000_eth_ops = {
>         .recv   = e1000_eth_recv,
>         .stop   = e1000_eth_stop,
>         .free_pkt = e1000_free_pkt,
> +       .write_hwaddr = e1000_write_hwaddr,
>  };
>
>  static const struct udevice_id e1000_eth_ids[] = {
> --
> 2.7.4
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Hannes Schmelzer]

On 10/25/2018 08:41 PM, Joe Hershberger wrote:
> On Thu, Oct 25, 2018 at 9:26 AM Hannes Schmelzer
> <hannes.schmelzer@br-automation.com> wrote:
Hi Joe,
>
>> +        * reading the current address failed
>> +        */
>> +       if (!ret_val && memcmp(current_mac, mac, 6) == 0)
>> +               return 0;
>> +
>> +       for (i = 0; i < 3; ++i)
>> +               data[i] = mac[i * 2 + 1] << 8 | mac[i * 2];
>> +
>> +       ret_val = e1000_write_eeprom_srwr(hw, 0x0, 3, data);
> This is a pretty uncommon implementation of this driver operation.
> Does the i120 in this case read the MAC directly from the EEPROM?
>
> Usually this function is used to write the hwaddr into the registers
> of the MAC. The .read_rom_hwaddr is used to fetch the hwaddr from the
> EEPROM and that address is used in the case that the U-Boot env has
> not set one.
This implementation is not new, just moved a bit upward in the file to 
avoid forward declaration for non-DM build.
I will double check it, but i guess there will be no change until we've 
no fail behavior of the existing code.

I didn't imagine that this little change would become so big ;-)
I will provide some v3 within next days with the cosmetic cleanup and 
other requested changes.

cheers,
Hannes

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Joe Hershberger]

On Thu, Oct 25, 2018 at 2:34 PM Hannes Schmelzer <hannes@schmelzer.or.at> wrote:
>
>
> On 10/25/2018 08:41 PM, Joe Hershberger wrote:
> > On Thu, Oct 25, 2018 at 9:26 AM Hannes Schmelzer
> > <hannes.schmelzer@br-automation.com> wrote:
> Hi Joe,
> >
> >> +        * reading the current address failed
> >> +        */
> >> +       if (!ret_val && memcmp(current_mac, mac, 6) == 0)
> >> +               return 0;
> >> +
> >> +       for (i = 0; i < 3; ++i)
> >> +               data[i] = mac[i * 2 + 1] << 8 | mac[i * 2];
> >> +
> >> +       ret_val = e1000_write_eeprom_srwr(hw, 0x0, 3, data);
> > This is a pretty uncommon implementation of this driver operation.
> > Does the i120 in this case read the MAC directly from the EEPROM?
> >
> > Usually this function is used to write the hwaddr into the registers
> > of the MAC. The .read_rom_hwaddr is used to fetch the hwaddr from the
> > EEPROM and that address is used in the case that the U-Boot env has
> > not set one.
> This implementation is not new, just moved a bit upward in the file to
> avoid forward declaration for non-DM build.

Yes, I understand... I just brought it up since I was looking at it
and maybe you had some insight.

> I will double check it, but i guess there will be no change until we've
> no fail behavior of the existing code.

What do you mean here?

>
> I didn't imagine that this little change would become so big ;-)

:) Sorry, wasn't meaning for it to be big... my comment about the
current implementation is not meant to block this change.

> I will provide some v3 within next days with the cosmetic cleanup and
> other requested changes.

Thanks!

>
> cheers,
> Hannes
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Bin Meng]

On Thu, Oct 25, 2018 at 10:25 PM Hannes Schmelzer
<hannes.schmelzer@br-automation.com> wrote:
>
> This commit ports the existing (non-DM) function for writing the MAC-
> address into the shadow ram (and flash) for DM.
>
> Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
> ---
>
> Changes in v2:
> - fix build for non-DM board
> - rebase on current master
>
>  drivers/net/e1000.c | 89 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 48 insertions(+), 41 deletions(-)
>

FYI

I was trying to port the most recent Linux kernel e1000e driver to
U-Boot. Current e1000 driver in U-Boot is way out of sync to Linux
kernel and it has many many ad-hoc hacks here and there to support
newer Intel NICs.

Regards,
Bin

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Hannes Schmelzer]

On 10/30/2018 09:13 AM, Bin Meng wrote:
> On Thu, Oct 25, 2018 at 10:25 PM Hannes Schmelzer
> <hannes.schmelzer@br-automation.com> wrote:
>> This commit ports the existing (non-DM) function for writing the MAC-
>> address into the shadow ram (and flash) for DM.
>>
>> Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
>> ---
>>
>> Changes in v2:
>> - fix build for non-DM board
>> - rebase on current master
>>
>>   drivers/net/e1000.c | 89 +++++++++++++++++++++++++++++------------------------
>>   1 file changed, 48 insertions(+), 41 deletions(-)
>>
> FYI
>
> I was trying to port the most recent Linux kernel e1000e driver to
> U-Boot. Current e1000 driver in U-Boot is way out of sync to Linux
> kernel and it has many many ad-hoc hacks here and there to support
> newer Intel NICs.
>
> Regards,
> Bin
good to know. is this work still in progress ? maybe i should wait with 
my changes.
pls let me know if there will be some progress next time, otherwise i 
will move on to bring in my changes as planned.

cheers,
Hannes

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Hannes Schmelzer]

On 30.10.2018 13:06, Hannes Schmelzer wrote:
>
> On 10/30/2018 09:13 AM, Bin Meng wrote:
>> On Thu, Oct 25, 2018 at 10:25 PM Hannes Schmelzer
>> <hannes.schmelzer@br-automation.com> wrote:
>>> This commit ports the existing (non-DM) function for writing the MAC-
>>> address into the shadow ram (and flash) for DM.
>>>
>>> Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
>>> ---
>>>
>>> Changes in v2:
>>> - fix build for non-DM board
>>> - rebase on current master
>>>
>>>   drivers/net/e1000.c | 89 
>>> +++++++++++++++++++++++++++++------------------------
>>>   1 file changed, 48 insertions(+), 41 deletions(-)
>>>
>> FYI
>>
>> I was trying to port the most recent Linux kernel e1000e driver to
>> U-Boot. Current e1000 driver in U-Boot is way out of sync to Linux
>> kernel and it has many many ad-hoc hacks here and there to support
>> newer Intel NICs.
>>
>> Regards,
>> Bin
> good to know. is this work still in progress ? maybe i should wait 
> with my changes.
> pls let me know if there will be some progress next time, otherwise i 
> will move on to bring in my changes as planned.
>
> cheers,
> Hannes
Bin,

do you plan to have some progress next time or should i move on with my 
changes?
cheers,
Hannes

[U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM

[Bin Meng]

Hi Hannes,

On Fri, Nov 23, 2018 at 3:34 PM Hannes Schmelzer <hannes@schmelzer.or.at> wrote:
>
>
>
> On 30.10.2018 13:06, Hannes Schmelzer wrote:
> >
> > On 10/30/2018 09:13 AM, Bin Meng wrote:
> >> On Thu, Oct 25, 2018 at 10:25 PM Hannes Schmelzer
> >> <hannes.schmelzer@br-automation.com> wrote:
> >>> This commit ports the existing (non-DM) function for writing the MAC-
> >>> address into the shadow ram (and flash) for DM.
> >>>
> >>> Signed-off-by: Hannes Schmelzer <hannes.schmelzer@br-automation.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - fix build for non-DM board
> >>> - rebase on current master
> >>>
> >>>   drivers/net/e1000.c | 89
> >>> +++++++++++++++++++++++++++++------------------------
> >>>   1 file changed, 48 insertions(+), 41 deletions(-)
> >>>
> >> FYI
> >>
> >> I was trying to port the most recent Linux kernel e1000e driver to
> >> U-Boot. Current e1000 driver in U-Boot is way out of sync to Linux
> >> kernel and it has many many ad-hoc hacks here and there to support
> >> newer Intel NICs.
> >>
> >> Regards,
> >> Bin
> > good to know. is this work still in progress ? maybe i should wait
> > with my changes.
> > pls let me know if there will be some progress next time, otherwise i
> > will move on to bring in my changes as planned.
> >
> > cheers,
> > Hannes
> Bin,
>
> do you plan to have some progress next time or should i move on with my
> changes?

Please move on with your changes. I did not have enough time to
continue the e1000e driver port.

Regards,
Bin