Hi Dirk, On Tue, Feb 7, 2012 at 11:13 PM, Dirk Behme <dirk.be...@de.bosch.com> wrote: > On 23.01.2012 17:17, Simon Glass wrote: >> >> Hi Dirk, >> >> >> On Jan 23, 2012 12:30 AM, "Dirk Behme" <dirk.be...@de.bosch.com >> <mailto:dirk.be...@de.bosch.com>> wrote: >> > >> > On 23.01.2012 08:31, Simon Glass wrote: >> >> >> >> Hi, >> >> >> >> On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme <dirk.be...@de.bosch.com >> <mailto:dirk.be...@de.bosch.com>> wrote: >> >>> >> >>> From: Eric Miao <eric.m...@linaro.org <mailto:eric.m...@linaro.org>> >> >> >>> >> >>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it >> >>> fails, fall back to use dev->enetaddr, which could be filled up by >> >>> the ethernet device driver: >> >>> >> >>> With the current code, introduced with below commit, >> eth_write_hwaddr() >> >>> will fail immediately if there is no eth<n>addr in the environment >> variables. >> >>> >> >>> However, e.g. for an overo based product that uses the SMSC911x >> ethernet >> >>> chip (with the MAC address set via EEPROM connected to the SMSC911x >> chip), >> >>> the MAC address is still OK. >> >>> >> >>> On mx28 boards that are depending on the OCOTP bits to set the MAC >> address >> >>> (like the Denx m28 board), the OCOTP bits should be used instead of >> >>> failing on the environment variables. >> >>> >> >>> Actually, this was the original behavior, and was later changed by >> >>> commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587. >> >>> >> >>> Signed-off-by: Eric Miao <eric.m...@linaro.org >> <mailto:eric.m...@linaro.org>> >> >>> Acked-by: Simon Glass <s...@chromium.org <mailto:s...@chromium.org>> >> >>> Acked-by: Dirk Behme <dirk.be...@de.bosch.com >> <mailto:dirk.be...@de.bosch.com>> >> >>> CC: Stefan Roese <s...@denx.de <mailto:s...@denx.de>> >> >>> CC: Eric Miao <eric.m...@linaro.org <mailto:eric.m...@linaro.org>> >> >>> CC: Wolfgang Denk <w...@denx.de <mailto:w...@denx.de>> >> >>> CC: Philip Balister <phi...@balister.org >> <mailto:phi...@balister.org>> >> >>> CC: Zach Sadecki <z...@itwatchdogs.com <mailto:z...@itwatchdogs.com>> >> >> >>> --- >> >>> v2: Correct the referenced commit ID and update the commit message. >> >>> No functional change at the code itself. >> >>> >> >>> Note: This resend is based on my understanding from >> >>> >> >>> http://lists.denx.de/pipermail/u-boot/2012-January/116118.html >> >>> >> >>> Please let Eric and me know if I missed anything there. >> >> >> >> >> >> I don't think you have missed anything and I have already acked this. >> >> But I want to start a related discussion. >> >> >> >> The code structure does bug me a bit - I think it is too confusing. >> >> eth_getenv_enetaddr() returns an error if there is no environment >> >> variable set or if the address it gets from the environment variable >> >> is invalid. We should probably not conflate those two. The first is ok >> >> here, but the second isn't, I think. >> >> >> >> What if the driver has no write_hwaddr method? Do we silently ignore >> >> the environment variable value? >> >> >> >> Why use memcmp() against env_enetaddr when the function we just called >> >> returns an error that tells us whether it is supposed to be valid (the >> >> error return your patch squashes)? >> >> >> >> We set the hwaddr by writing directly into the dev->enet_addr field >> >> and then calling write_hwaddr() if it exists. Maybe that is ok - is >> >> the lack of write_hwaddr() an indication that the driver does MAC >> >> address handling on the fly, or just that it can't set the MAC address >> >> at all? >> >> >> >> Overall I feel that eth_write_hwaddr() should return success or >> >> failure, confident in its determination that there is either a valid >> >> MAC address or there is not. The message you are seeing is I suppose >> >> an indication that it thinks there is a problem, when in fact none >> >> exists in this case. At the moment it feels fragile. >> >> >> >> I wonder whether a little refactor here would be best? >> >> >> >> That said, your patch restores the original behaviour, hiding the >> >> problem which isn't actually a problem in this case, and which we >> >> don't want to report. So it is better than the status quo. >> > >> > >> > Ok, thanks. >> > >> > I'm not an expert for this code, nor is the patch from me. It's from >> Eric ;) I just try to help to mainline all the stuff we have collected for >> i.MX6. >> > >> > Therefore I wonder if it would be possible to split this into two >> steps: >> > >> > a) Improve the status quo by applying this patch >> > b) In parallel discuss how to refactor and improve this code as you >> describe above >> > >> > It's my feeling that with (a) we still have a chance to improve >> v2012.03. But I doubt that (b) would make it into v2012.03. >> >> Yes agreed, it is a separate discussion. I added Wolfgang on cc to see >> what he thinks. > > > Any news to this?
Already acked by me. Are you going to start a separate discussion on how to clean up this code? If so please cc me. Regards, Simon > > Many thanks > > Dirk > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
