Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
Hi Hannes, On Fri, Nov 23, 2018 at 3:34 PM Hannes Schmelzer 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 > >> 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 > >>> --- > >>> > >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
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 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 --- 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
On 10/30/2018 09:13 AM, Bin Meng wrote: On Thu, Oct 25, 2018 at 10:25 PM Hannes Schmelzer 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 --- 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
On Thu, Oct 25, 2018 at 10:25 PM Hannes Schmelzer 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 > --- > > 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
On Thu, Oct 25, 2018 at 2:34 PM Hannes Schmelzer wrote: > > > On 10/25/2018 08:41 PM, Joe Hershberger wrote: > > On Thu, Oct 25, 2018 at 9:26 AM Hannes Schmelzer > > 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@lists.denx.de > https://lists.denx.de/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
On 10/25/2018 08:41 PM, Joe Hershberger wrote: On Thu, Oct 25, 2018 at 9:26 AM Hannes Schmelzer 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] net: e1000: support 'write_hwaddr' in DM
On Thu, Oct 25, 2018 at 9:26 AM Hannes Schmelzer
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
> ---
>
> 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..3df 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
