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

[Bin Meng]

Hi Michal,

On Mon, Feb 15, 2016 at 3:16 PM, Michal Simek <monstr at 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 at monstr.eu> wrote:
>>> Hi Bin,
>>>
>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn at gmail.com>:
>>>>
>>>> Hi Michal,
>>>>
>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr at 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 at 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?

>         switch ((zynq_slcr_get_boot_mode()) & ZYNQ_BM_MASK) {
>         case ZYNQ_BM_NOR:
>                 setenv("modeboot", "norboot");
>
> --

Regards,
Bin