[U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Michal Simek <monstr@monstr.eu>]

On 15.2.2016 12:10, Bin Meng wrote:
> Hi Michal,
> 
> On Mon, Feb 15, 2016 at 3:16 PM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Bin,
>>
>> On 14.2.2016 13:00, Bin Meng wrote:
>>> Hi Michal,
>>>
>>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>>>> Hi Bin,
>>>>
>>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@monstr.eu> wrote:
>>>>>> Add support for reading MAC address from I2C EEPROM.
>>>>>>
>>>>>
>>>>> Is this a feature provided by the GEM MAC IP?
>>>>>
>>>>>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>>>>>> ---
>>>>>>
>>>>>>  drivers/net/zynq_gem.c | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
>>>>>> index b3821c31a91d..ace60c901cb5 100644
>>>>>> --- a/drivers/net/zynq_gem.c
>>>>>> +++ b/drivers/net/zynq_gem.c
>>>>>> @@ -627,6 +627,21 @@ static int zynq_gem_remove(struct udevice *dev)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int zynq_gem_read_rom_hwaddr(struct udevice *dev)
>>>>>> +{
>>>>>> +#if defined(CONFIG_ZYNQ_GEM_EEPROM_ADDR) && \
>>>>>> +    defined(CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET)
>>>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>> +
>>>>>> +       if (eeprom_read(CONFIG_ZYNQ_GEM_EEPROM_ADDR,
>>>>>> +                       CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET,
>>>>>> +                       pdata->enetaddr, ARRAY_SIZE(pdata->enetaddr)))
>>>>>
>>>>> This call to eeprom_read() looks to me a board-specific feature, that
>>>>> an on-board eeprom is used to store the MAC address for the GEM?
>>>>>
>>>>
>>>> Right. it is board specific feature I can
>>>> The question is if this should really go to board specific file
>>>> or to be the part of DT binding. I didn't look at the kernel if someone has
>>>> some sort of eeprom binding for this case
>>>> but I expect local mac addresses via DT are used. Or passing via command
>>>> line does it.
>>>>
>>>> Anyway there is mac_read_from_eeprom() but it is ancient one.
>>>> Do you any preference for the name of function?
>>>
>>> I think the intention of the read_rom_hwaddr op is to read the MAC
>>> address via the ethernet controller defined mechanism (eg: e1000 can
>>> read its MAC address from either an EEPROM or an SPI flash), or via
>>> SoC defined mechanism (eg: the ethernet IP is only seen on a specific
>>> SoC, and reading the MAC address only makes sense on that specific
>>> SoC). If this is a board-specific mechanism, I don't think we should
>>> put the codes into driver's read_rom_hwaddr(). Again, if it is
>>> board-specific, we may not come up with a standard DT binding for such
>>> stuff, unless we can enumerate all possible ways for storing MAC
>>> address on a board (EEPROM, SPI flash, eMMC, SD)?
>>
>> It is interesting that there is no standard binding for it. Maybe in
>> future. Anyway is this better way (look below)?
>> (I will send it as regular patch)
>>
>> Thanks,
>> Michal
>>
>> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
>> index 01bae5d67e3f..ae115245b7ea 100644
>> --- a/board/xilinx/zynq/board.c
>> +++ b/board/xilinx/zynq/board.c
>> @@ -72,6 +72,18 @@ int board_init(void)
>>
>>  int board_late_init(void)
>>  {
>> +#if defined(CONFIG_ZYNQ_GEM_EEPROM_ADDR) && \
>> +    defined(CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET)
>> +       unsigned char enetaddr[6];
>> +
>> +       if (eeprom_read(CONFIG_ZYNQ_GEM_EEPROM_ADDR,
>> +                       CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET,
>> +                       enetaddr, ARRAY_SIZE(enetaddr)))
>> +               printf("I2C EEPROM MAC address read failed\n");
>> +       else
>> +               eth_setenv_enetaddr("ethaddr", enetaddr);
>> +#endif
>> +
> 
> Looks good, but one comment regarding to the naming:
> CONFIG_ZYNQ_GEM_EEPROM_ADDR & CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET.
> 
> Should we include _ZYNQ_GEM_ in the macro name since it may mislead it
> has something to do with the GEM controller? Is the on-board EEPROM
> only used to store the MAC address? If not, maybe think about some
> another generic naming?

Fair enough will fix it and send the patch.

Thanks,
Michal


-- 
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: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160215/205ea824/attachment.sig>