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

[Thierry Reding <thierry.reding@gmail.com>]

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>