Hi Michal, On Mon, Feb 15, 2016 at 3:16 PM, Michal Simek <mon...@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 <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)? > > 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot