[U-Boot] [PATCH v2 1/2] net: eth-uclass: Write MAC address to hardware after probe

[U-Boot] [PATCH v2 1/2] net: eth-uclass: Write MAC address to hardware after probe

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In order for the device to use the proper MAC address, which can have
been configured in the environment prior to the device being registered,
ensure that the MAC address is written after the device has been probed.
For devices that are registered before the network stack is initialized,
this is already done during eth_initialize(). If the Ethernet device is
on a bus that is not initialized on early boot, such as PCI, the device
is not available at the time eth_initialize() is called, so we need the
MAC address programming to also happen after probe.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 net/eth-uclass.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 2ef20df19203..4225aabf1fa1 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -524,6 +524,8 @@ static int eth_post_probe(struct udevice *dev)
 #endif
 	}
 
+	eth_write_hwaddr(dev);
+
 	return 0;
 }
 
-- 
2.21.0

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add the standard Ethernet device tree bindings (imported from v5.0 of
the Linux kernel) and implement support for reading the MAC address for
Ethernet devices in the Ethernet uclass. If the "mac-address" property
exists, the MAC address will be parsed from that. If that property does
not exist, the "local-mac-address" property will be tried as fallback.

MAC addresses from device tree take precedence over the ones stored in
a network interface card's ROM.

Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- use dev_read_u8_array_ptr()

 .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
 net/eth-uclass.c                              | 26 +++++++-
 2 files changed, 89 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
new file mode 100644
index 000000000000..cfc376bc977a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -0,0 +1,66 @@
+The following properties are common to the Ethernet controllers:
+
+NOTE: All 'phy*' properties documented below are Ethernet specific. For the
+generic PHY 'phys' property, see
+Documentation/devicetree/bindings/phy/phy-bindings.txt.
+
+- local-mac-address: array of 6 bytes, specifies the MAC address that was
+  assigned to the network device;
+- mac-address: array of 6 bytes, specifies the MAC address that was last used by
+  the boot program; should be used in cases where the MAC address assigned to
+  the device by the boot program is different from the "local-mac-address"
+  property;
+- nvmem-cells: phandle, reference to an nvmem node for the MAC address;
+- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used;
+- max-speed: number, specifies maximum speed in Mbit/s supported by the device;
+- max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
+  the maximum frame size (there's contradiction in the Devicetree
+  Specification).
+- phy-mode: string, operation mode of the PHY interface. This is now a de-facto
+  standard property; supported values are:
+  * "internal"
+  * "mii"
+  * "gmii"
+  * "sgmii"
+  * "qsgmii"
+  * "tbi"
+  * "rev-mii"
+  * "rmii"
+  * "rgmii" (RX and TX delays are added by the MAC when required)
+  * "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the
+     MAC should not add the RX or TX delays in this case)
+  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
+     should not add an RX delay in this case)
+  * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
+     should not add an TX delay in this case)
+  * "rtbi"
+  * "smii"
+  * "xgmii"
+  * "trgmii"
+  * "2000base-x",
+  * "2500base-x",
+  * "rxaui"
+  * "xaui"
+  * "10gbase-kr" (10GBASE-KR, XFI, SFI)
+- phy-connection-type: the same as "phy-mode" property but described in the
+  Devicetree Specification;
+- phy-handle: phandle, specifies a reference to a node representing a PHY
+  device; this property is described in the Devicetree Specification and so
+  preferred;
+- phy: the same as "phy-handle" property, not recommended for new bindings.
+- phy-device: the same as "phy-handle" property, not recommended for new
+  bindings.
+- rx-fifo-depth: the size of the controller's receive fifo in bytes. This
+  is used for components that can have configurable receive fifo sizes,
+  and is useful for determining certain configuration settings such as
+  flow control thresholds.
+- tx-fifo-depth: the size of the controller's transmit fifo in bytes. This
+  is used for components that can have configurable fifo sizes.
+- managed: string, specifies the PHY management type. Supported values are:
+  "auto", "in-band-status". "auto" is the default, it usess MDIO for
+  management if fixed-link is not specified.
+
+Child nodes of the Ethernet controller are typically the individual PHY devices
+connected via the MDIO bus (sometimes the MDIO bus controller is separate).
+They are described in the phy.txt file in this same directory.
+For non-MDIO PHY management see fixed-link.txt.
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 4225aabf1fa1..c6d5ec013bd8 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -455,6 +455,23 @@ static int eth_pre_unbind(struct udevice *dev)
 	return 0;
 }
 
+static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN])
+{
+	const uint8_t *p;
+
+	p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);
+	if (!p)
+		p = dev_read_u8_array_ptr(dev, "local-mac-address", ARP_HLEN);
+
+	if (!p) {
+		memset(mac, 0, ARP_HLEN);
+		return false;
+	}
+
+	memcpy(mac, p, ARP_HLEN);
+	return true;
+}
+
 static int eth_post_probe(struct udevice *dev)
 {
 	struct eth_device_priv *priv = dev->uclass_priv;
@@ -489,9 +506,12 @@ static int eth_post_probe(struct udevice *dev)
 
 	priv->state = ETH_STATE_INIT;
 
-	/* Check if the device has a MAC address in ROM */
-	if (eth_get_ops(dev)->read_rom_hwaddr)
-		eth_get_ops(dev)->read_rom_hwaddr(dev);
+	/* Check if the device has a MAC address in device tree */
+	if (!eth_dev_get_mac_address(dev, pdata->enetaddr)) {
+		/* Check if the device has a MAC address in ROM */
+		if (eth_get_ops(dev)->read_rom_hwaddr)
+			eth_get_ops(dev)->read_rom_hwaddr(dev);
+	}
 
 	eth_env_get_enetaddr_by_index("eth", dev->seq, env_enetaddr);
 	if (!is_zero_ethaddr(env_enetaddr)) {
-- 
2.21.0

[U-Boot] [PATCH v2 1/2] net: eth-uclass: Write MAC address to hardware after probe

[Joe Hershberger]

On Tue, Apr 16, 2019 at 11:24 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> In order for the device to use the proper MAC address, which can have
> been configured in the environment prior to the device being registered,
> ensure that the MAC address is written after the device has been probed.
> For devices that are registered before the network stack is initialized,
> this is already done during eth_initialize(). If the Ethernet device is
> on a bus that is not initialized on early boot, such as PCI, the device
> is not available at the time eth_initialize() is called, so we need the
> MAC address programming to also happen after probe.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Grygorii Strashko]

On 16.04.19 19:24, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Add the standard Ethernet device tree bindings (imported from v5.0 of
> the Linux kernel) and implement support for reading the MAC address for
> Ethernet devices in the Ethernet uclass. If the "mac-address" property
> exists, the MAC address will be parsed from that. If that property does
> not exist, the "local-mac-address" property will be tried as fallback.
> 
> MAC addresses from device tree take precedence over the ones stored in
> a network interface card's ROM.
> 
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - use dev_read_u8_array_ptr()
> 
>  .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
>  net/eth-uclass.c                              | 26 +++++++-
>  2 files changed, 89 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> new file mode 100644
> index 000000000000..cfc376bc977a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> @@ -0,0 +1,66 @@
> +The following properties are common to the Ethernet controllers:
> +
> +NOTE: All 'phy*' properties documented below are Ethernet specific. For the
> +generic PHY 'phys' property, see
> +Documentation/devicetree/bindings/phy/phy-bindings.txt.
> +
> +- local-mac-address: array of 6 bytes, specifies the MAC address that was
> +  assigned to the network device;
> +- mac-address: array of 6 bytes, specifies the MAC address that was last used by
> +  the boot program; should be used in cases where the MAC address assigned to
> +  the device by the boot program is different from the "local-mac-address"
> +  property;
> +- nvmem-cells: phandle, reference to an nvmem node for the MAC address;
> +- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used;
> +- max-speed: number, specifies maximum speed in Mbit/s supported by the device;
> +- max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
> +  the maximum frame size (there's contradiction in the Devicetree
> +  Specification).
> +- phy-mode: string, operation mode of the PHY interface. This is now a de-facto
> +  standard property; supported values are:
> +  * "internal"
> +  * "mii"
> +  * "gmii"
> +  * "sgmii"
> +  * "qsgmii"
> +  * "tbi"
> +  * "rev-mii"
> +  * "rmii"
> +  * "rgmii" (RX and TX delays are added by the MAC when required)
> +  * "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the
> +     MAC should not add the RX or TX delays in this case)
> +  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
> +     should not add an RX delay in this case)
> +  * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
> +     should not add an TX delay in this case)
> +  * "rtbi"
> +  * "smii"
> +  * "xgmii"
> +  * "trgmii"
> +  * "2000base-x",
> +  * "2500base-x",
> +  * "rxaui"
> +  * "xaui"
> +  * "10gbase-kr" (10GBASE-KR, XFI, SFI)
> +- phy-connection-type: the same as "phy-mode" property but described in the
> +  Devicetree Specification;
> +- phy-handle: phandle, specifies a reference to a node representing a PHY
> +  device; this property is described in the Devicetree Specification and so
> +  preferred;
> +- phy: the same as "phy-handle" property, not recommended for new bindings.
> +- phy-device: the same as "phy-handle" property, not recommended for new
> +  bindings.
> +- rx-fifo-depth: the size of the controller's receive fifo in bytes. This
> +  is used for components that can have configurable receive fifo sizes,
> +  and is useful for determining certain configuration settings such as
> +  flow control thresholds.
> +- tx-fifo-depth: the size of the controller's transmit fifo in bytes. This
> +  is used for components that can have configurable fifo sizes.
> +- managed: string, specifies the PHY management type. Supported values are:
> +  "auto", "in-band-status". "auto" is the default, it usess MDIO for
> +  management if fixed-link is not specified.
> +
> +Child nodes of the Ethernet controller are typically the individual PHY devices
> +connected via the MDIO bus (sometimes the MDIO bus controller is separate).
> +They are described in the phy.txt file in this same directory.
> +For non-MDIO PHY management see fixed-link.txt.
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 4225aabf1fa1..c6d5ec013bd8 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -455,6 +455,23 @@ static int eth_pre_unbind(struct udevice *dev)
>  	return 0;
>  }
>  
> +static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN])
> +{
> +	const uint8_t *p;
> +
> +	p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);
> +	if (!p)
> +		p = dev_read_u8_array_ptr(dev, "local-mac-address", ARP_HLEN);
> +
> +	if (!p) {
> +		memset(mac, 0, ARP_HLEN);
> +		return false;
> +	}
> +
> +	memcpy(mac, p, ARP_HLEN);
> +	return true;
> +}

There are set of DT files in u-boot which have
mac-address = [ 00 00 00 00 00 00 ];

How will it work for them? 
Wouldn't cause unexpected skipping eth_get_ops(dev)->read_rom_hwaddr(dev) call?

> +
>  static int eth_post_probe(struct udevice *dev)
>  {
>  	struct eth_device_priv *priv = dev->uclass_priv;
> @@ -489,9 +506,12 @@ static int eth_post_probe(struct udevice *dev)
>  
>  	priv->state = ETH_STATE_INIT;
>  
> -	/* Check if the device has a MAC address in ROM */
> -	if (eth_get_ops(dev)->read_rom_hwaddr)
> -		eth_get_ops(dev)->read_rom_hwaddr(dev);
> +	/* Check if the device has a MAC address in device tree */
> +	if (!eth_dev_get_mac_address(dev, pdata->enetaddr)) {
> +		/* Check if the device has a MAC address in ROM */
> +		if (eth_get_ops(dev)->read_rom_hwaddr)
> +			eth_get_ops(dev)->read_rom_hwaddr(dev);
> +	}
>  
>  	eth_env_get_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>  	if (!is_zero_ethaddr(env_enetaddr)) {
> 

-- 
Best regards,
grygorii

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Thierry Reding]

On Wed, Apr 17, 2019 at 02:49:22PM +0300, Grygorii Strashko wrote:
> 
> 
> On 16.04.19 19:24, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Add the standard Ethernet device tree bindings (imported from v5.0 of
> > the Linux kernel) and implement support for reading the MAC address for
> > Ethernet devices in the Ethernet uclass. If the "mac-address" property
> > exists, the MAC address will be parsed from that. If that property does
> > not exist, the "local-mac-address" property will be tried as fallback.
> > 
> > MAC addresses from device tree take precedence over the ones stored in
> > a network interface card's ROM.
> > 
> > Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - use dev_read_u8_array_ptr()
> > 
> >  .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
> >  net/eth-uclass.c                              | 26 +++++++-
> >  2 files changed, 89 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> > new file mode 100644
> > index 000000000000..cfc376bc977a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> > @@ -0,0 +1,66 @@
> > +The following properties are common to the Ethernet controllers:
> > +
> > +NOTE: All 'phy*' properties documented below are Ethernet specific. For the
> > +generic PHY 'phys' property, see
> > +Documentation/devicetree/bindings/phy/phy-bindings.txt.
> > +
> > +- local-mac-address: array of 6 bytes, specifies the MAC address that was
> > +  assigned to the network device;
> > +- mac-address: array of 6 bytes, specifies the MAC address that was last used by
> > +  the boot program; should be used in cases where the MAC address assigned to
> > +  the device by the boot program is different from the "local-mac-address"
> > +  property;
> > +- nvmem-cells: phandle, reference to an nvmem node for the MAC address;
> > +- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used;
> > +- max-speed: number, specifies maximum speed in Mbit/s supported by the device;
> > +- max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
> > +  the maximum frame size (there's contradiction in the Devicetree
> > +  Specification).
> > +- phy-mode: string, operation mode of the PHY interface. This is now a de-facto
> > +  standard property; supported values are:
> > +  * "internal"
> > +  * "mii"
> > +  * "gmii"
> > +  * "sgmii"
> > +  * "qsgmii"
> > +  * "tbi"
> > +  * "rev-mii"
> > +  * "rmii"
> > +  * "rgmii" (RX and TX delays are added by the MAC when required)
> > +  * "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the
> > +     MAC should not add the RX or TX delays in this case)
> > +  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
> > +     should not add an RX delay in this case)
> > +  * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
> > +     should not add an TX delay in this case)
> > +  * "rtbi"
> > +  * "smii"
> > +  * "xgmii"
> > +  * "trgmii"
> > +  * "2000base-x",
> > +  * "2500base-x",
> > +  * "rxaui"
> > +  * "xaui"
> > +  * "10gbase-kr" (10GBASE-KR, XFI, SFI)
> > +- phy-connection-type: the same as "phy-mode" property but described in the
> > +  Devicetree Specification;
> > +- phy-handle: phandle, specifies a reference to a node representing a PHY
> > +  device; this property is described in the Devicetree Specification and so
> > +  preferred;
> > +- phy: the same as "phy-handle" property, not recommended for new bindings.
> > +- phy-device: the same as "phy-handle" property, not recommended for new
> > +  bindings.
> > +- rx-fifo-depth: the size of the controller's receive fifo in bytes. This
> > +  is used for components that can have configurable receive fifo sizes,
> > +  and is useful for determining certain configuration settings such as
> > +  flow control thresholds.
> > +- tx-fifo-depth: the size of the controller's transmit fifo in bytes. This
> > +  is used for components that can have configurable fifo sizes.
> > +- managed: string, specifies the PHY management type. Supported values are:
> > +  "auto", "in-band-status". "auto" is the default, it usess MDIO for
> > +  management if fixed-link is not specified.
> > +
> > +Child nodes of the Ethernet controller are typically the individual PHY devices
> > +connected via the MDIO bus (sometimes the MDIO bus controller is separate).
> > +They are described in the phy.txt file in this same directory.
> > +For non-MDIO PHY management see fixed-link.txt.
> > diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> > index 4225aabf1fa1..c6d5ec013bd8 100644
> > --- a/net/eth-uclass.c
> > +++ b/net/eth-uclass.c
> > @@ -455,6 +455,23 @@ static int eth_pre_unbind(struct udevice *dev)
> >  	return 0;
> >  }
> >  
> > +static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN])
> > +{
> > +	const uint8_t *p;
> > +
> > +	p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);
> > +	if (!p)
> > +		p = dev_read_u8_array_ptr(dev, "local-mac-address", ARP_HLEN);
> > +
> > +	if (!p) {
> > +		memset(mac, 0, ARP_HLEN);
> > +		return false;
> > +	}
> > +
> > +	memcpy(mac, p, ARP_HLEN);
> > +	return true;
> > +}
> 
> There are set of DT files in u-boot which have
> mac-address = [ 00 00 00 00 00 00 ];
> 
> How will it work for them? 
> Wouldn't cause unexpected skipping eth_get_ops(dev)->read_rom_hwaddr(dev) call?

Not sure. Are devices with mac-address set to all zeros in the DT
expected to have a ROM from which the address could be read? Seems like
if the MAC address can be read from ROM there shouldn't be a need to
have an extra mac-address property in device tree.

That said, if this really is a problem, I suppose we could add an
additional check to see if the MAC address read from DT is all zeros,
and if so attempt to read the ROM anyway.

Thierry

> 
> > +
> >  static int eth_post_probe(struct udevice *dev)
> >  {
> >  	struct eth_device_priv *priv = dev->uclass_priv;
> > @@ -489,9 +506,12 @@ static int eth_post_probe(struct udevice *dev)
> >  
> >  	priv->state = ETH_STATE_INIT;
> >  
> > -	/* Check if the device has a MAC address in ROM */
> > -	if (eth_get_ops(dev)->read_rom_hwaddr)
> > -		eth_get_ops(dev)->read_rom_hwaddr(dev);
> > +	/* Check if the device has a MAC address in device tree */
> > +	if (!eth_dev_get_mac_address(dev, pdata->enetaddr)) {
> > +		/* Check if the device has a MAC address in ROM */
> > +		if (eth_get_ops(dev)->read_rom_hwaddr)
> > +			eth_get_ops(dev)->read_rom_hwaddr(dev);
> > +	}
> >  
> >  	eth_env_get_enetaddr_by_index("eth", dev->seq, env_enetaddr);
> >  	if (!is_zero_ethaddr(env_enetaddr)) {
> > 
> 
> -- 
> Best regards,
> grygorii
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190417/1e0c091c/attachment.sig>

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Simon Glass]

Hi Thierry,

On Wed, 17 Apr 2019 at 08:03, Thierry Reding <thierry.reding@gmail.com>
wrote:

> On Wed, Apr 17, 2019 at 02:49:22PM +0300, Grygorii Strashko wrote:
> >
> >
> > On 16.04.19 19:24, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Add the standard Ethernet device tree bindings (imported from v5.0 of
> > > the Linux kernel) and implement support for reading the MAC address for
> > > Ethernet devices in the Ethernet uclass. If the "mac-address" property
> > > exists, the MAC address will be parsed from that. If that property does
> > > not exist, the "local-mac-address" property will be tried as fallback.
> > >
> > > MAC addresses from device tree take precedence over the ones stored in
> > > a network interface card's ROM.
> > >
> > > Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v2:
> > > - use dev_read_u8_array_ptr()
>

It would be good to have test cases for these.


> > >
> > >  .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
> > >  net/eth-uclass.c                              | 26 +++++++-
> > >  2 files changed, 89 insertions(+), 3 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt
>
> Regards,
Simon

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Grygorii Strashko]

On 17.04.19 18:03, Thierry Reding wrote:
> On Wed, Apr 17, 2019 at 02:49:22PM +0300, Grygorii Strashko wrote:
>>
>>
>> On 16.04.19 19:24, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Add the standard Ethernet device tree bindings (imported from v5.0 of
>>> the Linux kernel) and implement support for reading the MAC address for
>>> Ethernet devices in the Ethernet uclass. If the "mac-address" property
>>> exists, the MAC address will be parsed from that. If that property does
>>> not exist, the "local-mac-address" property will be tried as fallback.
>>>
>>> MAC addresses from device tree take precedence over the ones stored in
>>> a network interface card's ROM.
>>>
>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> Changes in v2:
>>> - use dev_read_u8_array_ptr()
>>>
>>>  .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
>>>  net/eth-uclass.c                              | 26 +++++++-
>>>  2 files changed, 89 insertions(+), 3 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
>>> new file mode 100644
>>> index 000000000000..cfc376bc977a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
>>> @@ -0,0 +1,66 @@
>>> +The following properties are common to the Ethernet controllers:
>>> +
>>> +NOTE: All 'phy*' properties documented below are Ethernet specific. For the
>>> +generic PHY 'phys' property, see
>>> +Documentation/devicetree/bindings/phy/phy-bindings.txt.
>>> +
>>> +- local-mac-address: array of 6 bytes, specifies the MAC address that was
>>> +  assigned to the network device;
>>> +- mac-address: array of 6 bytes, specifies the MAC address that was last used by
>>> +  the boot program; should be used in cases where the MAC address assigned to
>>> +  the device by the boot program is different from the "local-mac-address"
>>> +  property;
>>> +- nvmem-cells: phandle, reference to an nvmem node for the MAC address;
>>> +- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used;
>>> +- max-speed: number, specifies maximum speed in Mbit/s supported by the device;
>>> +- max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
>>> +  the maximum frame size (there's contradiction in the Devicetree
>>> +  Specification).
>>> +- phy-mode: string, operation mode of the PHY interface. This is now a de-facto
>>> +  standard property; supported values are:
>>> +  * "internal"
>>> +  * "mii"
>>> +  * "gmii"
>>> +  * "sgmii"
>>> +  * "qsgmii"
>>> +  * "tbi"
>>> +  * "rev-mii"
>>> +  * "rmii"
>>> +  * "rgmii" (RX and TX delays are added by the MAC when required)
>>> +  * "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the
>>> +     MAC should not add the RX or TX delays in this case)
>>> +  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
>>> +     should not add an RX delay in this case)
>>> +  * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
>>> +     should not add an TX delay in this case)
>>> +  * "rtbi"
>>> +  * "smii"
>>> +  * "xgmii"
>>> +  * "trgmii"
>>> +  * "2000base-x",
>>> +  * "2500base-x",
>>> +  * "rxaui"
>>> +  * "xaui"
>>> +  * "10gbase-kr" (10GBASE-KR, XFI, SFI)
>>> +- phy-connection-type: the same as "phy-mode" property but described in the
>>> +  Devicetree Specification;
>>> +- phy-handle: phandle, specifies a reference to a node representing a PHY
>>> +  device; this property is described in the Devicetree Specification and so
>>> +  preferred;
>>> +- phy: the same as "phy-handle" property, not recommended for new bindings.
>>> +- phy-device: the same as "phy-handle" property, not recommended for new
>>> +  bindings.
>>> +- rx-fifo-depth: the size of the controller's receive fifo in bytes. This
>>> +  is used for components that can have configurable receive fifo sizes,
>>> +  and is useful for determining certain configuration settings such as
>>> +  flow control thresholds.
>>> +- tx-fifo-depth: the size of the controller's transmit fifo in bytes. This
>>> +  is used for components that can have configurable fifo sizes.
>>> +- managed: string, specifies the PHY management type. Supported values are:
>>> +  "auto", "in-band-status". "auto" is the default, it usess MDIO for
>>> +  management if fixed-link is not specified.
>>> +
>>> +Child nodes of the Ethernet controller are typically the individual PHY devices
>>> +connected via the MDIO bus (sometimes the MDIO bus controller is separate).
>>> +They are described in the phy.txt file in this same directory.
>>> +For non-MDIO PHY management see fixed-link.txt.
>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>> index 4225aabf1fa1..c6d5ec013bd8 100644
>>> --- a/net/eth-uclass.c
>>> +++ b/net/eth-uclass.c
>>> @@ -455,6 +455,23 @@ static int eth_pre_unbind(struct udevice *dev)
>>>  	return 0;
>>>  }
>>>  
>>> +static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN])
>>> +{
>>> +	const uint8_t *p;
>>> +
>>> +	p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);
>>> +	if (!p)
>>> +		p = dev_read_u8_array_ptr(dev, "local-mac-address", ARP_HLEN);
>>> +
>>> +	if (!p) {
>>> +		memset(mac, 0, ARP_HLEN);
>>> +		return false;
>>> +	}
>>> +
>>> +	memcpy(mac, p, ARP_HLEN);
>>> +	return true;
>>> +}
>>
>> There are set of DT files in u-boot which have
>> mac-address = [ 00 00 00 00 00 00 ];
>>
>> How will it work for them? 
>> Wouldn't cause unexpected skipping eth_get_ops(dev)->read_rom_hwaddr(dev) call?
> 
> Not sure. Are devices with mac-address set to all zeros in the DT
> expected to have a ROM from which the address could be read? Seems like
> if the MAC address can be read from ROM there shouldn't be a need to
> have an extra mac-address property in device tree.


correct, for TI boards it's expected to populate mac address in DT by u-boot,
so linux can use it.

Not sure why there are zero "mac-address" property in DT - probably due to
some historical reasons.


> 
> That said, if this really is a problem, I suppose we could add an
> additional check to see if the MAC address read from DT is all zeros,
> and if so attempt to read the ROM anyway.

I think, the linux code can be used as reference here:

static const void *of_get_mac_addr(struct device_node *np, const char *name)
{
	struct property *pp = of_find_property(np, name, NULL);

	if (pp && pp->length == ETH_ALEN && is_valid_ether_addr(pp->value))

^^^^ there is additional check is_valid_ether_addr()

		return pp->value;
	return NULL;
}



>>
>>> +
>>>  static int eth_post_probe(struct udevice *dev)
>>>  {
>>>  	struct eth_device_priv *priv = dev->uclass_priv;
>>> @@ -489,9 +506,12 @@ static int eth_post_probe(struct udevice *dev)
>>>  
>>>  	priv->state = ETH_STATE_INIT;
>>>  
>>> -	/* Check if the device has a MAC address in ROM */
>>> -	if (eth_get_ops(dev)->read_rom_hwaddr)
>>> -		eth_get_ops(dev)->read_rom_hwaddr(dev);
>>> +	/* Check if the device has a MAC address in device tree */
>>> +	if (!eth_dev_get_mac_address(dev, pdata->enetaddr)) {
>>> +		/* Check if the device has a MAC address in ROM */
>>> +		if (eth_get_ops(dev)->read_rom_hwaddr)
>>> +			eth_get_ops(dev)->read_rom_hwaddr(dev);
>>> +	}
>>>  
>>>  	eth_env_get_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>>>  	if (!is_zero_ethaddr(env_enetaddr)) {
>>>
>>
>> -- 
>> Best regards,
>> grygorii

-- 
Best regards,
grygorii

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Thierry Reding]

On Wed, Apr 17, 2019 at 09:32:26PM -0700, Simon Glass wrote:
> Hi Thierry,
> 
> On Wed, 17 Apr 2019 at 08:03, Thierry Reding <thierry.reding@gmail.com>
> wrote:
> 
> > On Wed, Apr 17, 2019 at 02:49:22PM +0300, Grygorii Strashko wrote:
> > >
> > >
> > > On 16.04.19 19:24, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Add the standard Ethernet device tree bindings (imported from v5.0 of
> > > > the Linux kernel) and implement support for reading the MAC address for
> > > > Ethernet devices in the Ethernet uclass. If the "mac-address" property
> > > > exists, the MAC address will be parsed from that. If that property does
> > > > not exist, the "local-mac-address" property will be tried as fallback.
> > > >
> > > > MAC addresses from device tree take precedence over the ones stored in
> > > > a network interface card's ROM.
> > > >
> > > > Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > > Changes in v2:
> > > > - use dev_read_u8_array_ptr()
> >
> 
> It would be good to have test cases for these.

By "these", do you mean the eth_dev_get_mac_address() function that this
patche introduces, or the dev_read_u8_array_ptr() function?

For the former, that'd be a little difficult because it is not a public
API, it's only called from the eth-uclass.c code.

Thierry

> > > >
> > > >  .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
> > > >  net/eth-uclass.c                              | 26 +++++++-
> > > >  2 files changed, 89 insertions(+), 3 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt
> >
> > Regards,
> Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190425/ee3ab4d0/attachment.sig>

[U-Boot] [PATCH v2 2/2] net: eth-uclass: Support device tree MAC addresses

[Thierry Reding]

On Thu, Apr 18, 2019 at 07:30:05PM +0300, Grygorii Strashko wrote:
> 
> 
> On 17.04.19 18:03, Thierry Reding wrote:
> > On Wed, Apr 17, 2019 at 02:49:22PM +0300, Grygorii Strashko wrote:
> >>
> >>
> >> On 16.04.19 19:24, Thierry Reding wrote:
> >>> From: Thierry Reding <treding@nvidia.com>
> >>>
> >>> Add the standard Ethernet device tree bindings (imported from v5.0 of
> >>> the Linux kernel) and implement support for reading the MAC address for
> >>> Ethernet devices in the Ethernet uclass. If the "mac-address" property
> >>> exists, the MAC address will be parsed from that. If that property does
> >>> not exist, the "local-mac-address" property will be tried as fallback.
> >>>
> >>> MAC addresses from device tree take precedence over the ones stored in
> >>> a network interface card's ROM.
> >>>
> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> >>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>> ---
> >>> Changes in v2:
> >>> - use dev_read_u8_array_ptr()
> >>>
> >>>  .../devicetree/bindings/net/ethernet.txt      | 66 +++++++++++++++++++
> >>>  net/eth-uclass.c                              | 26 +++++++-
> >>>  2 files changed, 89 insertions(+), 3 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/net/ethernet.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> >>> new file mode 100644
> >>> index 000000000000..cfc376bc977a
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> >>> @@ -0,0 +1,66 @@
> >>> +The following properties are common to the Ethernet controllers:
> >>> +
> >>> +NOTE: All 'phy*' properties documented below are Ethernet specific. For the
> >>> +generic PHY 'phys' property, see
> >>> +Documentation/devicetree/bindings/phy/phy-bindings.txt.
> >>> +
> >>> +- local-mac-address: array of 6 bytes, specifies the MAC address that was
> >>> +  assigned to the network device;
> >>> +- mac-address: array of 6 bytes, specifies the MAC address that was last used by
> >>> +  the boot program; should be used in cases where the MAC address assigned to
> >>> +  the device by the boot program is different from the "local-mac-address"
> >>> +  property;
> >>> +- nvmem-cells: phandle, reference to an nvmem node for the MAC address;
> >>> +- nvmem-cell-names: string, should be "mac-address" if nvmem is to be used;
> >>> +- max-speed: number, specifies maximum speed in Mbit/s supported by the device;
> >>> +- max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
> >>> +  the maximum frame size (there's contradiction in the Devicetree
> >>> +  Specification).
> >>> +- phy-mode: string, operation mode of the PHY interface. This is now a de-facto
> >>> +  standard property; supported values are:
> >>> +  * "internal"
> >>> +  * "mii"
> >>> +  * "gmii"
> >>> +  * "sgmii"
> >>> +  * "qsgmii"
> >>> +  * "tbi"
> >>> +  * "rev-mii"
> >>> +  * "rmii"
> >>> +  * "rgmii" (RX and TX delays are added by the MAC when required)
> >>> +  * "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the
> >>> +     MAC should not add the RX or TX delays in this case)
> >>> +  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
> >>> +     should not add an RX delay in this case)
> >>> +  * "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
> >>> +     should not add an TX delay in this case)
> >>> +  * "rtbi"
> >>> +  * "smii"
> >>> +  * "xgmii"
> >>> +  * "trgmii"
> >>> +  * "2000base-x",
> >>> +  * "2500base-x",
> >>> +  * "rxaui"
> >>> +  * "xaui"
> >>> +  * "10gbase-kr" (10GBASE-KR, XFI, SFI)
> >>> +- phy-connection-type: the same as "phy-mode" property but described in the
> >>> +  Devicetree Specification;
> >>> +- phy-handle: phandle, specifies a reference to a node representing a PHY
> >>> +  device; this property is described in the Devicetree Specification and so
> >>> +  preferred;
> >>> +- phy: the same as "phy-handle" property, not recommended for new bindings.
> >>> +- phy-device: the same as "phy-handle" property, not recommended for new
> >>> +  bindings.
> >>> +- rx-fifo-depth: the size of the controller's receive fifo in bytes. This
> >>> +  is used for components that can have configurable receive fifo sizes,
> >>> +  and is useful for determining certain configuration settings such as
> >>> +  flow control thresholds.
> >>> +- tx-fifo-depth: the size of the controller's transmit fifo in bytes. This
> >>> +  is used for components that can have configurable fifo sizes.
> >>> +- managed: string, specifies the PHY management type. Supported values are:
> >>> +  "auto", "in-band-status". "auto" is the default, it usess MDIO for
> >>> +  management if fixed-link is not specified.
> >>> +
> >>> +Child nodes of the Ethernet controller are typically the individual PHY devices
> >>> +connected via the MDIO bus (sometimes the MDIO bus controller is separate).
> >>> +They are described in the phy.txt file in this same directory.
> >>> +For non-MDIO PHY management see fixed-link.txt.
> >>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> >>> index 4225aabf1fa1..c6d5ec013bd8 100644
> >>> --- a/net/eth-uclass.c
> >>> +++ b/net/eth-uclass.c
> >>> @@ -455,6 +455,23 @@ static int eth_pre_unbind(struct udevice *dev)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN])
> >>> +{
> >>> +	const uint8_t *p;
> >>> +
> >>> +	p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);
> >>> +	if (!p)
> >>> +		p = dev_read_u8_array_ptr(dev, "local-mac-address", ARP_HLEN);
> >>> +
> >>> +	if (!p) {
> >>> +		memset(mac, 0, ARP_HLEN);
> >>> +		return false;
> >>> +	}
> >>> +
> >>> +	memcpy(mac, p, ARP_HLEN);
> >>> +	return true;
> >>> +}
> >>
> >> There are set of DT files in u-boot which have
> >> mac-address = [ 00 00 00 00 00 00 ];
> >>
> >> How will it work for them? 
> >> Wouldn't cause unexpected skipping eth_get_ops(dev)->read_rom_hwaddr(dev) call?
> > 
> > Not sure. Are devices with mac-address set to all zeros in the DT
> > expected to have a ROM from which the address could be read? Seems like
> > if the MAC address can be read from ROM there shouldn't be a need to
> > have an extra mac-address property in device tree.
> 
> 
> correct, for TI boards it's expected to populate mac address in DT by u-boot,
> so linux can use it.

Are you passing on the DT from U-Boot to the kernel for these devices?
This patch operates on U-Boot's built-in DTB (i.e. the control DTB), so
it should have no effect on what's passed to Linux.

> Not sure why there are zero "mac-address" property in DT - probably due to
> some historical reasons.

> 
> 
> > 
> > That said, if this really is a problem, I suppose we could add an
> > additional check to see if the MAC address read from DT is all zeros,
> > and if so attempt to read the ROM anyway.
> 
> I think, the linux code can be used as reference here:
> 
> static const void *of_get_mac_addr(struct device_node *np, const char *name)
> {
> 	struct property *pp = of_find_property(np, name, NULL);
> 
> 	if (pp && pp->length == ETH_ALEN && is_valid_ether_addr(pp->value))
> 
> ^^^^ there is additional check is_valid_ether_addr()
> 
> 		return pp->value;
> 	return NULL;
> }

I've added that check now and will resend the patch.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190425/dc11455d/attachment.sig>