Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Joe, On 16 February 2016 at 09:06, Joe Hershbergerwrote: > > Hi Simon, > > On Tue, Feb 16, 2016 at 9:59 AM, Simon Glass wrote: > > Hi, > > > > On 15 February 2016 at 18:06, Bin Meng wrote: > >> +Simon, > >> > >> Hi Joe, > >> > >> On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger > >> wrote: > >>> Hi Bin, > >>> > >>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: > Hi Michal, > > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: > > Hi Bin, > > > > 2016-02-14 3:25 GMT+01:00 Bin Meng : > >> > >> Hi Michal, > >> > >> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 > >> > --- > >> > > >> > 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
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Simon, On Tue, Feb 16, 2016 at 9:59 AM, Simon Glasswrote: > Hi, > > On 15 February 2016 at 18:06, Bin Meng wrote: >> +Simon, >> >> Hi Joe, >> >> On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger >> wrote: >>> Hi Bin, >>> >>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: Hi Michal, On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: > Hi Bin, > > 2016-02-14 3:25 GMT+01:00 Bin Meng : >> >> Hi Michal, >> >> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 >> > --- >> > >> > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi, On 15 February 2016 at 18:06, Bin Mengwrote: > +Simon, > > Hi Joe, > > On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger > wrote: >> Hi Bin, >> >> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: >>> Hi Michal, >>> >>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: Hi Bin, 2016-02-14 3:25 GMT+01:00 Bin Meng : > > Hi Michal, > > On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 > > --- > > > > 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? What will the other zynq boards do to read this information? How different will each implementation be? That said, since this is confined to zynq it's not a big concern. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
+Simon, Hi Joe, On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershbergerwrote: > Hi Bin, > > On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: >> Hi Michal, >> >> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: >>> Hi Bin, >>> >>> 2016-02-14 3:25 GMT+01:00 Bin Meng : Hi Michal, On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 > --- > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Michal, On Mon, Feb 15, 2016 at 12:51 PM, Michal Simekwrote: > Hi, > > 2016-02-15 18:53 GMT+01:00 Joe Hershberger : >> >> Hi Michal, >> >> On Mon, Feb 15, 2016 at 11:41 AM, Michal Simek wrote: >> > Hi Joe, >> > >> > 2016-02-15 17:01 GMT+01:00 Joe Hershberger : >> >> >> >> Hi Bin, >> >> >> >> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: >> >> > Hi Michal, >> >> > >> >> > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek >> >> > wrote: >> >> >> Hi Bin, >> >> >> >> >> >> 2016-02-14 3:25 GMT+01:00 Bin Meng : >> >> >>> >> >> >>> Hi Michal, >> >> >>> >> >> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek >> >> >>> 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 >> >> >>> > --- >> >> >>> > >> >> >>> > 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/ >> >> >> >> This is the concept I had in mind for handling this sort of thing. >> > >> > >> > This also works for me. I have no problem with both ways. It means this >> > one >> > or setting >> > up variable in board init file. >> > Also is there any hook that mac address will be updated in DTS file >> > (local-mac-address) >> > to be the same in u-boot and in Linux. >> >> I'm not aware of a common way. > > > Anyway you are network custodian and I believe you will tell me which way to > go to implement > this mac address reading from eeprom. :-) Unless someone can compel me otherwise, I think the reference that I sent should be the way for now. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi, 2016-02-15 18:53 GMT+01:00 Joe Hershberger: > Hi Michal, > > On Mon, Feb 15, 2016 at 11:41 AM, Michal Simek wrote: > > Hi Joe, > > > > 2016-02-15 17:01 GMT+01:00 Joe Hershberger : > >> > >> Hi Bin, > >> > >> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: > >> > Hi Michal, > >> > > >> > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek > wrote: > >> >> Hi Bin, > >> >> > >> >> 2016-02-14 3:25 GMT+01:00 Bin Meng : > >> >>> > >> >>> Hi Michal, > >> >>> > >> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek > >> >>> 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 > >> >>> > --- > >> >>> > > >> >>> > 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/ > >> > >> This is the concept I had in mind for handling this sort of thing. > > > > > > This also works for me. I have no problem with both ways. It means this > one > > or setting > > up variable in board init file. > > Also is there any hook that mac address will be updated in DTS file > > (local-mac-address) > > to be the same in u-boot and in Linux. > > I'm not aware of a common way. > Anyway you are network custodian and I believe you will tell me which way to go to implement this mac address reading from eeprom. :-) Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Joe, 2016-02-15 17:01 GMT+01:00 Joe Hershberger: > Hi Bin, > > On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng wrote: > > Hi Michal, > > > > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: > >> Hi Bin, > >> > >> 2016-02-14 3:25 GMT+01:00 Bin Meng : > >>> > >>> Hi Michal, > >>> > >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek > 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 > >>> > --- > >>> > > >>> > 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/ > > This is the concept I had in mind for handling this sort of thing. > This also works for me. I have no problem with both ways. It means this one or setting up variable in board init file. Also is there any hook that mac address will be updated in DTS file (local-mac-address) to be the same in u-boot and in Linux. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Bin, On Sun, Feb 14, 2016 at 6:00 AM, Bin Mengwrote: > Hi Michal, > > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: >> Hi Bin, >> >> 2016-02-14 3:25 GMT+01:00 Bin Meng : >>> >>> Hi Michal, >>> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 >>> > --- >>> > >>> > 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/ This is the concept I had in mind for handling this sort of thing. Thanks, -Joe ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
On 15.2.2016 12:10, Bin Meng wrote: > Hi Michal, > > On Mon, Feb 15, 2016 at 3:16 PM, Michal Simekwrote: >> Hi Bin, >> >> On 14.2.2016 13:00, Bin Meng wrote: >>> Hi Michal, >>> >>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: Hi Bin, 2016-02-14 3:25 GMT+01:00 Bin Meng : > > Hi Michal, > > On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 >> --- >> >> 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? Fair enough will fix it and send the patch. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Michal, On Mon, Feb 15, 2016 at 3:16 PM, Michal Simekwrote: > Hi Bin, > > On 14.2.2016 13:00, Bin Meng wrote: >> Hi Michal, >> >> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek wrote: >>> Hi Bin, >>> >>> 2016-02-14 3:25 GMT+01:00 Bin Meng : Hi Michal, On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 > --- > > 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
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Bin, On 14.2.2016 13:00, Bin Meng wrote: > Hi Michal, > > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simekwrote: >> Hi Bin, >> >> 2016-02-14 3:25 GMT+01:00 Bin Meng : >>> >>> Hi Michal, >>> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 --- 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 + switch ((zynq_slcr_get_boot_mode()) & ZYNQ_BM_MASK) { case ZYNQ_BM_NOR: setenv("modeboot", "norboot"); -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Michal, On Sun, Feb 14, 2016 at 6:03 PM, Michal Simekwrote: > Hi Bin, > > 2016-02-14 3:25 GMT+01:00 Bin Meng : >> >> Hi Michal, >> >> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 >> > --- >> > >> > 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)? Regards, Bin ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Bin, 2016-02-14 3:25 GMT+01:00 Bin Meng: > Hi Michal, > > On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek 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 > > --- > > > > 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? Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Add support for reading MAC address from I2C EEPROM. Signed-off-by: Michal Simek--- 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))) + printf("EEPROM MAC address read failed\n"); +#endif + return 0; +} + + static const struct eth_ops zynq_gem_ops = { .start = zynq_gem_init, .send = zynq_gem_send, @@ -634,6 +649,7 @@ static const struct eth_ops zynq_gem_ops = { .free_pkt = zynq_gem_free_pkt, .stop = zynq_gem_halt, .write_hwaddr = zynq_gem_setup_mac, + .read_rom_hwaddr= zynq_gem_read_rom_hwaddr, }; static int zynq_gem_ofdata_to_platdata(struct udevice *dev) -- 1.9.1 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM
Hi Michal, On Sat, Feb 13, 2016 at 6:39 PM, Michal Simekwrote: > Add support for reading MAC address from I2C EEPROM. > Is this a feature provided by the GEM MAC IP? > Signed-off-by: Michal Simek > --- > > 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? > + printf("EEPROM MAC address read failed\n"); > +#endif > + return 0; > +} > + > + > static const struct eth_ops zynq_gem_ops = { > .start = zynq_gem_init, > .send = zynq_gem_send, > @@ -634,6 +649,7 @@ static const struct eth_ops zynq_gem_ops = { > .free_pkt = zynq_gem_free_pkt, > .stop = zynq_gem_halt, > .write_hwaddr = zynq_gem_setup_mac, > + .read_rom_hwaddr= zynq_gem_read_rom_hwaddr, > }; > > static int zynq_gem_ofdata_to_platdata(struct udevice *dev) > -- Regards, Bin ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot