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

[Bin Meng]

+Simon,

Hi Joe,

On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger
<joe.hershberger at gmail.com> wrote:
> Hi Bin,
>
> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn at gmail.com> 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)?
>
> Did you see this: https://patchwork.ozlabs.org/patch/573373/
>

I haven't noticed this before.

> This is the concept I had in mind for handling this sort of thing.
>

Your patch looks good to me, but I added Simon, who is not a big fan
of using weak in driver model :-)

Regards,
Bin