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

[Joe Hershberger]

Hi Simon,

On Tue, Feb 16, 2016 at 9:59 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 15 February 2016 at 18:06, Bin Meng <bmeng.cn at gmail.com> wrote:
>> +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 :-)
>
> My main concern with 'weak' is using it in place of what I see as a
> proper API. The drivers should really stand alone and not call into
> board code, and vice versa. If the driver needs some settings, then
> perhaps we can provide it via device tree / platform data?

The problem with using the device tree is we want to share with Linux.
Linux DTs will not describe this sort of thing. Platform data,
maybe... but it is not something that is defined. That means it will
be very difficult to make standard.

On a side note, the same thing troubles me wrt MDIO and phy
descriptions in DTs. It seems each MAC device tree has a different way
to describe the relationships.

> What will the other zynq boards do to read this information? How
> different will each implementation be?

That's part of the problem. It's hard to make a good API when it's
only the first or second board that wants to do this. The next one
will have a different requirement.

> That said, since this is confined to zynq it's not a big concern.

It seems like it's worth cleaning up after there is a clear need to
have an API. Until there are a number of boards that need something
like this, I think an informal API (like this weak stuff) is simpler.

-Joe