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

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

[Michal Simek]

Add support for reading MAC address from I2C EEPROM.

Signed-off-by: Michal Simek <monstr@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)))
+		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] [PATCH 2/6] ARM: zynq: zybo: Enabling reading MAC address from EEPROM

[Michal Simek]

Zybo has on board I2C EEPROM which contains preprogrammed MAC address.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---

 include/configs/zynq_zybo.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/configs/zynq_zybo.h b/include/configs/zynq_zybo.h
index c53ba79d48b0..644d462c01c9 100644
--- a/include/configs/zynq_zybo.h
+++ b/include/configs/zynq_zybo.h
@@ -18,6 +18,12 @@
 #define CONFIG_ZYNQ_USB
 #define CONFIG_ZYNQ_SDHCI0
 #define CONFIG_ZYNQ_BOOT_FREEBSD
+#define CONFIG_ZYNQ_I2C0
+#define CONFIG_ZYNQ_I2C1
+#define CONFIG_SYS_I2C_EEPROM_ADDR_LEN	1
+#define CONFIG_CMD_EEPROM
+#define CONFIG_ZYNQ_GEM_EEPROM_ADDR	0x50
+#define CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET	0xFA
 
 /* Define ZYBO PS Clock Frequency to 50MHz */
 #define CONFIG_ZYNQ_PS_CLK_FREQ	50000000UL
-- 
1.9.1

[U-Boot] [PATCH 3/6] net: phy: realtek: Use generic genphy_parse_link() for RTL8211E

[Michal Simek]

The problem with current implementation is that SPDDONE bit is 1
but link bit is zero. That's why phydev->link is setup to 0
which ending up in driver failure that link is not up.

Log:
Zynq> dhcp
ethernet at e000b000 Waiting for PHY auto negotiation to complete.......
done
ethernet at e000b000: No link.

There is at least 1ms delay between spddone bit and link up.

Use genphy_read_status() instead of realtek implemenation which is
working with page 11. Linux driver is also using generic implementation.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---

 drivers/net/phy/realtek.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index bba48da4099f..259a87fcc59e 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -203,6 +203,14 @@ static int rtl8211x_startup(struct phy_device *phydev)
 	return 0;
 }
 
+static int rtl8211e_startup(struct phy_device *phydev)
+{
+	genphy_update_link(phydev);
+	genphy_parse_link(phydev);
+
+	return 0;
+}
+
 static int rtl8211f_startup(struct phy_device *phydev)
 {
 	/* Read the Status (2x to make sure link is right) */
@@ -230,7 +238,7 @@ static struct phy_driver RTL8211E_driver = {
 	.mask = 0xffffff,
 	.features = PHY_GBIT_FEATURES,
 	.config = &rtl8211x_config,
-	.startup = &rtl8211x_startup,
+	.startup = &rtl8211e_startup,
 	.shutdown = &genphy_shutdown,
 };
 
-- 
1.9.1

[U-Boot] [PATCH 3/6] net: phy: realtek: Use generic genphy_parse_link() for RTL8211E

[Michal Simek]

On 13.2.2016 11:39, Michal Simek wrote:
> The problem with current implementation is that SPDDONE bit is 1
> but link bit is zero. That's why phydev->link is setup to 0
> which ending up in driver failure that link is not up.
> 
> Log:
> Zynq> dhcp
> ethernet at e000b000 Waiting for PHY auto negotiation to complete.......
> done
> ethernet at e000b000: No link.
> 
> There is at least 1ms delay between spddone bit and link up.
> 
> Use genphy_read_status() instead of realtek implemenation which is
> working with page 11. Linux driver is also using generic implementation.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
> 
>  drivers/net/phy/realtek.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index bba48da4099f..259a87fcc59e 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -203,6 +203,14 @@ static int rtl8211x_startup(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int rtl8211e_startup(struct phy_device *phydev)
> +{
> +	genphy_update_link(phydev);
> +	genphy_parse_link(phydev);
> +
> +	return 0;
> +}
> +
>  static int rtl8211f_startup(struct phy_device *phydev)
>  {
>  	/* Read the Status (2x to make sure link is right) */
> @@ -230,7 +238,7 @@ static struct phy_driver RTL8211E_driver = {
>  	.mask = 0xffffff,
>  	.features = PHY_GBIT_FEATURES,
>  	.config = &rtl8211x_config,
> -	.startup = &rtl8211x_startup,
> +	.startup = &rtl8211e_startup,
>  	.shutdown = &genphy_shutdown,
>  };
>  

Applied to zynq repo.

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160222/cef3a863/attachment.sig>

[U-Boot] [PATCH 4/6] ARM: zynq: Enable Realtek phys by default

[Michal Simek]

This phy is available at Zybo board.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---

 include/configs/zynq-common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/zynq-common.h b/include/configs/zynq-common.h
index e8c3ef0c3872..7b42a00b9e75 100644
--- a/include/configs/zynq-common.h
+++ b/include/configs/zynq-common.h
@@ -50,6 +50,7 @@
 # define CONFIG_MII
 # define CONFIG_SYS_FAULT_ECHO_LINK_DOWN
 # define CONFIG_PHY_MARVELL
+# define CONFIG_PHY_REALTEK
 # define CONFIG_BOOTP_SERVERIP
 # define CONFIG_BOOTP_BOOTPATH
 # define CONFIG_BOOTP_GATEWAY
-- 
1.9.1

[U-Boot] [PATCH 4/6] ARM: zynq: Enable Realtek phys by default

[Michal Simek]

On 13.2.2016 11:39, Michal Simek wrote:
> This phy is available at Zybo board.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
> 
>  include/configs/zynq-common.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/configs/zynq-common.h b/include/configs/zynq-common.h
> index e8c3ef0c3872..7b42a00b9e75 100644
> --- a/include/configs/zynq-common.h
> +++ b/include/configs/zynq-common.h
> @@ -50,6 +50,7 @@
>  # define CONFIG_MII
>  # define CONFIG_SYS_FAULT_ECHO_LINK_DOWN
>  # define CONFIG_PHY_MARVELL
> +# define CONFIG_PHY_REALTEK
>  # define CONFIG_BOOTP_SERVERIP
>  # define CONFIG_BOOTP_BOOTPATH
>  # define CONFIG_BOOTP_GATEWAY
> 

Applied to zynq repo.

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160222/171bf5ad/attachment.sig>

[U-Boot] [PATCH 5/6] ARM: zynq: Add reset-gpio property for USB on Zybo

[Michal Simek]

DTS syncup with Linux kernel.
Add missing reset-gpio property.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---

This patch is in queue to next kernel version

---
 arch/arm/dts/zynq-zybo.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/zynq-zybo.dts b/arch/arm/dts/zynq-zybo.dts
index fbbb8911910b..d04e9625d29e 100644
--- a/arch/arm/dts/zynq-zybo.dts
+++ b/arch/arm/dts/zynq-zybo.dts
@@ -31,8 +31,9 @@
 	};
 
 	usb_phy0: phy0 {
-		compatible = "usb-nop-xceiv";
 		#phy-cells = <0>;
+		compatible = "usb-nop-xceiv";
+		reset-gpios = <&gpio0 46 1>;
 	};
 };
 
-- 
1.9.1

[U-Boot] [PATCH 5/6] ARM: zynq: Add reset-gpio property for USB on Zybo

[Michal Simek]

On 13.2.2016 11:39, Michal Simek wrote:
> DTS syncup with Linux kernel.
> Add missing reset-gpio property.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
> 
> This patch is in queue to next kernel version
> 
> ---
>  arch/arm/dts/zynq-zybo.dts | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/dts/zynq-zybo.dts b/arch/arm/dts/zynq-zybo.dts
> index fbbb8911910b..d04e9625d29e 100644
> --- a/arch/arm/dts/zynq-zybo.dts
> +++ b/arch/arm/dts/zynq-zybo.dts
> @@ -31,8 +31,9 @@
>  	};
>  
>  	usb_phy0: phy0 {
> -		compatible = "usb-nop-xceiv";
>  		#phy-cells = <0>;
> +		compatible = "usb-nop-xceiv";
> +		reset-gpios = <&gpio0 46 1>;
>  	};
>  };
>  

Applied to zynq repo.

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160222/be8bcbbd/attachment.sig>

[U-Boot] [PATCH 6/6] ARM: zynq: Enable EDID for zybo

[Michal Simek]

Zybo contains on board HDMI that's why enable EDID.
Doing it via config because zynq i2c driver hasn't been moved to DM yet
and enabling via Kconfig requires DM_I2C.
This will be moved that driver is moved to DM.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---

Currently the biggest problem is EEPROM support with DM together.

Log for zybo:
Zynq> i2c dev 1
Setting bus to 1
Zynq> i2c probe
Valid chip addresses: 50
Zynq> i2c edid 50
EDID version: 1.3
Product ID code: 05cd
Manufacturer: SAM
Serial number: 30303033
Manufactured in week: 42 year: 2010
Video input definition: digital signal, voltage level 0
Monitor is non-RGB
Maximum visible display size: 16 cm x 9 cm
Power management features: active off, no suspend, no standby
Estabilished timings:
	640x480		60 Hz (VGA)
	800x600		56 Hz (VESA)
	800x600		60 Hz (VESA)
	1024x768	60 Hz (VESA)
Standard timings:
	1600x1200 	60 Hz
	1280x1024 	60 Hz
	1280x960 	60 Hz
	1280x800 	60 Hz
	1440x900 	60 Hz
	1680x1050 	60 Hz
	1920x1080 	60 Hz (detailed)
	1280x720 	60 Hz (detailed)
Monitor range limits, horizontal sync: 30-81 kHz, vertical refresh:
50-60 Hz, max pixel clock: 170 MHz
Monitor name: SyncMaster

---
 include/configs/zynq_zybo.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/configs/zynq_zybo.h b/include/configs/zynq_zybo.h
index 644d462c01c9..3db3c13e8a3e 100644
--- a/include/configs/zynq_zybo.h
+++ b/include/configs/zynq_zybo.h
@@ -24,6 +24,8 @@
 #define CONFIG_CMD_EEPROM
 #define CONFIG_ZYNQ_GEM_EEPROM_ADDR	0x50
 #define CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET	0xFA
+#define CONFIG_DISPLAY
+#define CONFIG_I2C_EDID
 
 /* Define ZYBO PS Clock Frequency to 50MHz */
 #define CONFIG_ZYNQ_PS_CLK_FREQ	50000000UL
-- 
1.9.1

[U-Boot] [PATCH 6/6] ARM: zynq: Enable EDID for zybo

[Michal Simek]

On 13.2.2016 11:39, Michal Simek wrote:
> Zybo contains on board HDMI that's why enable EDID.
> Doing it via config because zynq i2c driver hasn't been moved to DM yet
> and enabling via Kconfig requires DM_I2C.
> This will be moved that driver is moved to DM.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
> 
> Currently the biggest problem is EEPROM support with DM together.
> 
> Log for zybo:
> Zynq> i2c dev 1
> Setting bus to 1
> Zynq> i2c probe
> Valid chip addresses: 50
> Zynq> i2c edid 50
> EDID version: 1.3
> Product ID code: 05cd
> Manufacturer: SAM
> Serial number: 30303033
> Manufactured in week: 42 year: 2010
> Video input definition: digital signal, voltage level 0
> Monitor is non-RGB
> Maximum visible display size: 16 cm x 9 cm
> Power management features: active off, no suspend, no standby
> Estabilished timings:
> 	640x480		60 Hz (VGA)
> 	800x600		56 Hz (VESA)
> 	800x600		60 Hz (VESA)
> 	1024x768	60 Hz (VESA)
> Standard timings:
> 	1600x1200 	60 Hz
> 	1280x1024 	60 Hz
> 	1280x960 	60 Hz
> 	1280x800 	60 Hz
> 	1440x900 	60 Hz
> 	1680x1050 	60 Hz
> 	1920x1080 	60 Hz (detailed)
> 	1280x720 	60 Hz (detailed)
> Monitor range limits, horizontal sync: 30-81 kHz, vertical refresh:
> 50-60 Hz, max pixel clock: 170 MHz
> Monitor name: SyncMaster
> 
> ---
>  include/configs/zynq_zybo.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/configs/zynq_zybo.h b/include/configs/zynq_zybo.h
> index 644d462c01c9..3db3c13e8a3e 100644
> --- a/include/configs/zynq_zybo.h
> +++ b/include/configs/zynq_zybo.h
> @@ -24,6 +24,8 @@
>  #define CONFIG_CMD_EEPROM
>  #define CONFIG_ZYNQ_GEM_EEPROM_ADDR	0x50
>  #define CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET	0xFA
> +#define CONFIG_DISPLAY
> +#define CONFIG_I2C_EDID
>  
>  /* Define ZYBO PS Clock Frequency to 50MHz */
>  #define CONFIG_ZYNQ_PS_CLK_FREQ	50000000UL
> 

Applied to zynq repo.

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160222/909633eb/attachment.sig>

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

[Bin Meng]

Hi Michal,

On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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?

> +               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] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Michal Simek]

Hi Bin,

2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:

> Hi Michal,
>
> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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?

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] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Bin Meng]

Hi Michal,

On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
> Hi Bin,
>
> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>
>> Hi Michal,
>>
>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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)?

Regards,
Bin

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

[Michal Simek]

Hi Bin,

On 14.2.2016 13:00, Bin Meng wrote:
> Hi Michal,
> 
> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Bin,
>>
>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>
>>> Hi Michal,
>>>
>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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
+
        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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160215/e1c54ec3/attachment.sig>

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

[Bin Meng]

Hi Michal,

On Mon, Feb 15, 2016 at 3:16 PM, Michal Simek <monstr@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 <monstr@monstr.eu> wrote:
>>> Hi Bin,
>>>
>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>>
>>>> Hi Michal,
>>>>
>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Michal Simek]

On 15.2.2016 12:10, Bin Meng wrote:
> Hi Michal,
> 
> On Mon, Feb 15, 2016 at 3:16 PM, Michal Simek <monstr@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 <monstr@monstr.eu> wrote:
>>>> Hi Bin,
>>>>
>>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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?

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160215/205ea824/attachment.sig>

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

[Joe Hershberger]

Hi Bin,

On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Michal,
>
> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Bin,
>>
>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>
>>> Hi Michal,
>>>
>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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/

This is the concept I had in mind for handling this sort of thing.

Thanks,
-Joe

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

[Michal Simek]

Hi Joe,

2016-02-15 17:01 GMT+01:00 Joe Hershberger <joe.hershberger@gmail.com>:

> Hi Bin,
>
> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> > Hi Michal,
> >
> > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
> >> Hi Bin,
> >>
> >> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
> >>>
> >>> Hi Michal,
> >>>
> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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/
>
> 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] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Joe Hershberger]

Hi Michal,

On Mon, Feb 15, 2016 at 11:41 AM, Michal Simek <monstr@monstr.eu> wrote:
> Hi Joe,
>
> 2016-02-15 17:01 GMT+01:00 Joe Hershberger <joe.hershberger@gmail.com>:
>>
>> Hi Bin,
>>
>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> > Hi Michal,
>> >
>> > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>> >> Hi Bin,
>> >>
>> >> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>> >>>
>> >>> Hi Michal,
>> >>>
>> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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/
>>
>> 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.

-Joe

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

[Michal Simek]

Hi,

2016-02-15 18:53 GMT+01:00 Joe Hershberger <joe.hershberger@gmail.com>:

> Hi Michal,
>
> On Mon, Feb 15, 2016 at 11:41 AM, Michal Simek <monstr@monstr.eu> wrote:
> > Hi Joe,
> >
> > 2016-02-15 17:01 GMT+01:00 Joe Hershberger <joe.hershberger@gmail.com>:
> >>
> >> Hi Bin,
> >>
> >> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> >> > Hi Michal,
> >> >
> >> > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu>
> wrote:
> >> >> Hi Bin,
> >> >>
> >> >> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
> >> >>>
> >> >>> Hi Michal,
> >> >>>
> >> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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/
> >>
> >> 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] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Joe Hershberger]

Hi Michal,

On Mon, Feb 15, 2016 at 12:51 PM, Michal Simek <monstr@monstr.eu> wrote:
> Hi,
>
> 2016-02-15 18:53 GMT+01:00 Joe Hershberger <joe.hershberger@gmail.com>:
>>
>> Hi Michal,
>>
>> On Mon, Feb 15, 2016 at 11:41 AM, Michal Simek <monstr@monstr.eu> wrote:
>> > Hi Joe,
>> >
>> > 2016-02-15 17:01 GMT+01:00 Joe Hershberger <joe.hershberger@gmail.com>:
>> >>
>> >> Hi Bin,
>> >>
>> >> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> >> > Hi Michal,
>> >> >
>> >> > On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu>
>> >> > wrote:
>> >> >> Hi Bin,
>> >> >>
>> >> >> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>> >> >>>
>> >> >>> Hi Michal,
>> >> >>>
>> >> >>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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/
>> >>
>> >> 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] [PATCH 1/6] net: gem: Add support for reading MAC from I2C EEPROM

[Bin Meng]

+Simon,

Hi Joe,

On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Michal,
>>
>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>>> Hi Bin,
>>>
>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>>
>>>> Hi Michal,
>>>>
>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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 :-)

Regards,
Bin

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

[Simon Glass]

Hi,

On 15 February 2016 at 18:06, Bin Meng <bmeng.cn@gmail.com> wrote:
> +Simon,
>
> Hi Joe,
>
> On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Michal,
>>>
>>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>>>> Hi Bin,
>>>>
>>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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?

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] [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@chromium.org> wrote:
> Hi,
>
> On 15 February 2016 at 18:06, Bin Meng <bmeng.cn@gmail.com> wrote:
>> +Simon,
>>
>> Hi Joe,
>>
>> On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger
>> <joe.hershberger@gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Michal,
>>>>
>>>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
>>>>>>
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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

[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@gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Feb 16, 2016 at 9:59 AM, Simon Glass <sjg@chromium.org> wrote:
> > Hi,
> >
> > On 15 February 2016 at 18:06, Bin Meng <bmeng.cn@gmail.com> wrote:
> >> +Simon,
> >>
> >> Hi Joe,
> >>
> >> On Tue, Feb 16, 2016 at 12:01 AM, Joe Hershberger
> >> <joe.hershberger@gmail.com> wrote:
> >>> Hi Bin,
> >>>
> >>> On Sun, Feb 14, 2016 at 6:00 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>> Hi Michal,
> >>>>
> >>>> On Sun, Feb 14, 2016 at 6:03 PM, Michal Simek <monstr@monstr.eu> wrote:
> >>>>> Hi Bin,
> >>>>>
> >>>>> 2016-02-14 3:25 GMT+01:00 Bin Meng <bmeng.cn@gmail.com>:
> >>>>>>
> >>>>>> Hi Michal,
> >>>>>>
> >>>>>> On Sat, Feb 13, 2016 at 6:39 PM, Michal Simek <monstr@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@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