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

[Simon Glass]

Hi Joe,

On 16 February 2016 at 09:06, Joe Hershberger <joe.hershberger at gmail.com> wrote:
>
> 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.

Sounds reasonable to me.

Regards,
Simon