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?
Many thanks
Dirk
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
