Hi Joe, On 16 February 2016 at 09:06, Joe Hershberger <joe.hershber...@gmail.com> wrote: > > Hi Simon, > > On Tue, Feb 16, 2016 at 9:59 AM, Simon Glass <s...@chromium.org> wrote: > > Hi, > > > > On 15 February 2016 at 18:06, Bin Meng <bmeng...@gmail.com> wrote: > >> +Simon, > >> > >> Hi Joe, > >> > >> On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger > >> <joe.hershber...@gmail.com> wrote: > >>> Hi Bin, > >>> > >>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng...@gmail.com> wrote: > >>>> Hi Michal, > >>>> > >>>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <mon...@monstr.eu> wrote: > >>>>> Hi Bin, > >>>>> > >>>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng...@gmail.com>: > >>>>>> > >>>>>> Hi Michal, > >>>>>> > >>>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <mon...@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 <mon...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot