[v3,1/5] net: Introduce DSA class for Ethernet switches

From: Claudiu Manoil <claudiu.manoil@nxp.com>

DSA stands for Distributed Switch Architecture and it covers switches that
are connected to the CPU through an Ethernet link and generally use frame
tags to pass information about the source/destination ports to/from CPU.
Front panel ports are presented as regular ethernet devices in U-Boot and
they are expected to support the typical networking commands.
DSA switches may be cascaded, DSA class code does not currently support
this.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3:
- Removed all infrastructure associated with dsa_foreach_port, which
  is no longer needed.
- Only inherit the DSA master's MAC address if the environment does not
  already have a specific MAC address that should be used for the DSA
  port.
- Be compatible with the new "ethernet-ports" container name which has
  been introduced in the Linux kernel as commit 85e05d263ed2 ("net: dsa:
  of: Allow ethernet-ports as encapsulating node") in v5.9.

v2: none

 drivers/net/Kconfig    |  14 ++
 include/dm/uclass-id.h |   1 +
 include/net.h          |   6 +
 include/net/dsa.h      | 163 ++++++++++++++
 net/Makefile           |   1 +
 net/dsa-uclass.c       | 470 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 655 insertions(+)
 create mode 100644 include/net/dsa.h
 create mode 100644 net/dsa-uclass.c

Am 2021-01-22 20:16, schrieb Vladimir Oltean:
> From: Claudiu Manoil <claudiu.manoil@nxp.com>
> 
> DSA stands for Distributed Switch Architecture and it covers switches 
> that
> are connected to the CPU through an Ethernet link and generally use 
> frame
> tags to pass information about the source/destination ports to/from 
> CPU.
> Front panel ports are presented as regular ethernet devices in U-Boot 
> and
> they are expected to support the typical networking commands.
> DSA switches may be cascaded, DSA class code does not currently support
> this.
> 
> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v3:
> - Removed all infrastructure associated with dsa_foreach_port, which
>   is no longer needed.
> - Only inherit the DSA master's MAC address if the environment does not
>   already have a specific MAC address that should be used for the DSA
>   port.
> - Be compatible with the new "ethernet-ports" container name which has
>   been introduced in the Linux kernel as commit 85e05d263ed2 ("net: 
> dsa:
>   of: Allow ethernet-ports as encapsulating node") in v5.9.
> 
> v2: none
> 
>  drivers/net/Kconfig    |  14 ++
>  include/dm/uclass-id.h |   1 +
>  include/net.h          |   6 +
>  include/net/dsa.h      | 163 ++++++++++++++
>  net/Makefile           |   1 +
>  net/dsa-uclass.c       | 470 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 655 insertions(+)
>  create mode 100644 include/net/dsa.h
>  create mode 100644 net/dsa-uclass.c
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 3a5e03688059..382224d04c6e 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -37,6 +37,20 @@ config DM_MDIO_MUX
>  	  This is currently implemented in net/mdio-mux-uclass.c
>  	  Look in include/miiphy.h for details.
> 
> +config DM_DSA
> +	bool "Enable Driver Model for DSA switches"
> +	depends on DM_ETH && DM_MDIO
> +	help
> +	  Enable driver model for DSA switches
> +
> +	  Adds UCLASS_DSA class supporting switches that follow the 
> Distributed
> +	  Switch Architecture (DSA).  These switches rely on the presence of 
> a
> +	  management switch port connected to an Ethernet controller capable 
> of
> +	  receiving frames from the switch.  This host Ethernet controller is
> +	  called the "master" Ethernet interface in DSA terminology.
> +	  This is currently implemented in net/dsa-uclass.c, refer to
> +	  include/net/dsa.h for API details.
> +
>  config MDIO_SANDBOX
>  	depends on DM_MDIO && SANDBOX
>  	default y
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index ae4425d7a57a..d75de368c5a8 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -46,6 +46,7 @@ enum uclass_id {
>  	UCLASS_DISPLAY,		/* Display (e.g. DisplayPort, HDMI) */
>  	UCLASS_DSI_HOST,	/* Display Serial Interface host */
>  	UCLASS_DMA,		/* Direct Memory Access */
> +	UCLASS_DSA,		/* Distributed (Ethernet) Switch Architecture */
>  	UCLASS_EFI,		/* EFI managed devices */
>  	UCLASS_ETH,		/* Ethernet device */
>  	UCLASS_ETH_PHY,		/* Ethernet PHY device */
> diff --git a/include/net.h b/include/net.h
> index 13da69b7c145..b95d6a6f60eb 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -499,7 +499,13 @@ struct icmp_hdr {
>   * maximum packet size and multiple of 32 bytes =  1536
>   */
>  #define PKTSIZE			1522
> +#ifndef CONFIG_DM_DSA
>  #define PKTSIZE_ALIGN		1536
> +#else
> +/* Maximum DSA tagging overhead (headroom and/or tailroom) */
> +#define DSA_MAX_OVR		256
> +#define PKTSIZE_ALIGN		(1536 + DSA_MAX_OVR)
> +#endif
> 
>  /*
>   * Maximum receive ring size; that is, the number of packets
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> new file mode 100644
> index 000000000000..18bc7ffbe470
> --- /dev/null
> +++ b/include/net/dsa.h
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2019-2021 NXP Semiconductors
> + */
> +
> +#ifndef __DSA_H__
> +#define __DSA_H__
> +
> +#include <phy.h>
> +#include <net.h>
> +
> +/**
> + * DSA stands for Distributed Switch Architecture and it is 
> infrastructure
> + * intended to support drivers for Switches that rely on an 
> intermediary
> + * Ethernet device for I/O.  These switches may support cascading 
> allowing
> + * them to be arranged as a tree.
> + * DSA is documented in detail in the Linux kernel documentation under
> + * Documentation/networking/dsa/dsa.txt
> + * The network layout of such a switch is shown below:
> + *
> + *                      |------|
> + *                      | eth0 | <--- master eth device (regular eth 
> driver)
> + *                      |------|
> + *                        ^  |
> + * tag added by switch -->|  |
> + *                        |  |
> + *                        |  |<-- tag added by DSA driver
> + *                        |  v
> + *      |--------------------------------------|
> + *      |             | CPU port |             | <-- DSA (switch) 
> device
> + *      |             ------------             |     (DSA driver)
> + *      | _________  _________       _________ |
> + *      | | port0 |  | port1 |  ...  | portn | | <-- ports as eth 
> devices
> + *      |-+-------+--+-------+-------+-------+-|     ('dsa-port' eth 
> driver)
> + *
> + * In U-Boot the intent is to allow access to front panel ports (shown 
> at the
> + * bottom of the picture) through the master Ethernet dev (eth0 in
> the picture).
> + * Front panel ports are presented as regular Ethernet devices in 
> U-Boot and
> + * they are expected to support the typical networking commands.
> + * In general DSA switches require the use of tags, extra headers 
> added both by
> + * software on Tx and by the switch on Rx.  These tags carry at a 
> minimum port
> + * information and switch information for cascaded set-ups.
> + * In U-Boot these tags are inserted and parsed by the DSA switch 
> driver, the
> + * class code helps with headroom/tailroom for the extra headers.
> + *
> + * TODO:
> + * - handle switch cascading, for now U-Boot only supports
> stand-alone switches.
> + * - Add support to probe DSA switches connected to a MDIO bus, this 
> is needed
> + * to convert switch drivers that are now under drivers/net/phy.
> + */
> +
> +#define DSA_PORT_NAME_LENGTH	16
> +
> +/* Maximum number of ports each DSA device can have */
> +#define DSA_MAX_PORTS		12
> +
> +/**
> + * struct dsa_ops - DSA operations
> + *
> + * @port_enable:  Initialize a switch port for I/O.
> + * @port_disable: Disable I/O for a port.
> + * @xmit:         Insert the DSA tag for transmission.
> + *                DSA drivers receive a copy of the packet with 
> headroom and
> + *                tailroom reserved and set to 0. 'packet' points to 
> headroom
> + *                and 'length' is updated to include both head and 
> tailroom.
> + * @rcv:          Process the DSA tag on reception and return the port 
> index
> + *                from the h/w provided tag. Return the index via 
> 'portp'.
> + *                'packet' and 'length' describe the frame as received 
> from
> + *                master including any additional headers.
> + */
> +struct dsa_ops {
> +	int (*port_enable)(struct udevice *dev, int port,
> +			   struct phy_device *phy);
> +	void (*port_disable)(struct udevice *dev, int port,
> +			     struct phy_device *phy);
> +	int (*xmit)(struct udevice *dev, int port, void *packet, int length);
> +	int (*rcv)(struct udevice *dev, int *portp, void *packet, int 
> length);
> +};
> +
> +#define dsa_get_ops(dev) ((struct dsa_ops *)(dev)->driver->ops)
> +
> +/**
> + * struct dsa_port_pdata - DSA port platform data
> + *
> + * @phy:   PHY device associated with this port.
> + *         The uclass code attempts to set this field for all ports 
> except CPU
> + *         port, based on DT information.  It may be NULL.
> + * @index: Port index in the DSA switch, set by the uclass code.
> + * @name:  Name of the port Eth device.  If a label property is 
> present in the
> + *         port DT node, it is used as name.
> + */
> +struct dsa_port_pdata {
> +	struct phy_device *phy;
> +	u32 index;
> +	char name[DSA_PORT_NAME_LENGTH];
> +};
> +
> +/**
> + * struct dsa_pdata - Per-device platform data for DSA DM
> + *
> + * @num_ports:   Number of ports the device has, must be <= 
> DSA_MAX_PORTS.
> + *		 This number is extracted from the DT 'ports' node of this
> + *		 DSA device, and it counts the CPU port and all the other
> + *		 port subnodes including the disabled ones.
> + * @cpu_port:    Index of the switch port linked to the master 
> Ethernet.
> + *		 The uclass code sets this based on DT information.
> + * @master_node: DT node of the master Ethernet.
> + */
> +struct dsa_pdata {
> +	int num_ports;
> +	u32 cpu_port;
> +	ofnode master_node;
> +};
> +
> +/**
> + * dsa_set_tagging() - Configure the headroom and/or tailroom sizes
> + *
> + * The DSA class code allocates headroom and tailroom on Tx before
> + * calling the DSA driver's xmit function.
> + * All drivers must call this at probe time.
> + *
> + * @dev:	DSA device pointer
> + * @headroom:	Size, in bytes, of headroom needed for the DSA tag.
> + * @tailroom:	Size, in bytes, of tailroom needed for the DSA tag.
> + *		Total headroom and tailroom size should not exceed
> + *		DSA_MAX_OVR.
> + * @return 0 if OK, -ve on error
> + */
> +int dsa_set_tagging(struct udevice *dev, ushort headroom, ushort 
> tailroom);
> +
> +/* DSA helpers */
> +
> +/**
> + * dsa_get_master() - Return a reference to the master Ethernet device
> + *
> + * Can be called at driver probe time or later.
> + *
> + * @dev:	DSA device pointer
> + * @return Master Eth 'udevice' pointer if OK, NULL on error
> + */
> +struct udevice *dsa_get_master(struct udevice *dev);
> +
> +/**
> + * dsa_port_get_pdata() - Helper that returns the platdata of an 
> active
> + *			(non-CPU) DSA port device.
> + *
> + * Can be called at driver probe time or later.
> + *
> + * @pdev:	DSA port device pointer
> + * @return 'dsa_port_pdata' pointer if OK, NULL on error
> + */
> +static inline struct dsa_port_pdata *
> +	dsa_port_get_pdata(struct udevice *pdev)
> +{
> +	struct eth_pdata *eth = dev_get_plat(pdev);
> +
> +	if (!eth)
> +		return NULL;
> +
> +	return eth->priv_pdata;
> +}
> +
> +#endif /* __DSA_H__ */
> diff --git a/net/Makefile b/net/Makefile
> index 76527f704c47..fb3eba840fff 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_NET)      += arp.o
>  obj-$(CONFIG_CMD_BOOTP) += bootp.o
>  obj-$(CONFIG_CMD_CDP)  += cdp.o
>  obj-$(CONFIG_CMD_DNS)  += dns.o
> +obj-$(CONFIG_DM_DSA)   += dsa-uclass.o
>  ifdef CONFIG_DM_ETH
>  obj-$(CONFIG_NET)      += eth-uclass.o
>  else
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> new file mode 100644
> index 000000000000..f60e06b00f57
> --- /dev/null
> +++ b/net/dsa-uclass.c
> @@ -0,0 +1,470 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019-2021 NXP
> + */
> +
> +#include <net/dsa.h>
> +#include <dm/lists.h>
> +#include <dm/device_compat.h>
> +#include <dm/device-internal.h>
> +#include <dm/uclass-internal.h>
> +#include <linux/bitmap.h>
> +#include <miiphy.h>
> +
> +#define DSA_PORT_CHILD_DRV_NAME "dsa-port"
> +
> +/* per-device internal state structure */
> +struct dsa_priv {
> +	struct udevice *master_dev;
> +	int num_ports;
> +	u32 cpu_port;
> +	int headroom;
> +	int tailroom;
> +};
> +
> +/* external API */
> +int dsa_set_tagging(struct udevice *dev, ushort headroom, ushort 
> tailroom)
> +{
> +	struct dsa_priv *priv;
> +
> +	if (!dev || !dev_get_uclass_priv(dev))
> +		return -ENODEV;
> +
> +	if (headroom + tailroom > DSA_MAX_OVR)
> +		return -EINVAL;
> +
> +	priv = dev_get_uclass_priv(dev);
> +
> +	if (headroom > 0)
> +		priv->headroom = headroom;
> +	if (tailroom > 0)
> +		priv->tailroom = tailroom;
> +
> +	return 0;
> +}
> +
> +/* returns the DSA master Ethernet device */
> +struct udevice *dsa_get_master(struct udevice *dev)
> +{
> +	struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +
> +	if (!priv)
> +		return NULL;
> +
> +	return priv->master_dev;
> +}
> +
> +/*
> + * Start the desired port, the CPU port and the master Eth interface.
> + * TODO: if cascaded we may need to _start ports in other switches too
> + */
> +static int dsa_port_start(struct udevice *pdev)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +	struct udevice *master = dsa_get_master(dev);
> +	struct dsa_ops *ops = dsa_get_ops(dev);
> +	int err;
> +
> +	if (!priv)
> +		return -ENODEV;
> +
> +	if (!master) {
> +		dev_err(pdev, "DSA master Ethernet device not found!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ops->port_enable) {
> +		struct dsa_port_pdata *port_pdata;
> +
> +		port_pdata = dev_get_parent_plat(pdev);
> +		err = ops->port_enable(dev, port_pdata->index,
> +				       port_pdata->phy);
> +		if (err)
> +			return err;
> +
> +		err = ops->port_enable(dev, priv->cpu_port, NULL);

This will lead to a NULL pointer dereference in felix_port_enable()
when accessing the phy. Although somehow (due to caching?) u-boot
won't panic until reaching board_phy_config() (the weak one in
drivers/net/phy/phy.c).

-michael

> +		if (err)
> +			return err;
> +	}
> +
> +	return eth_get_ops(master)->start(master);
> +}
> +
> +/* Stop the desired port, the CPU port and the master Eth interface */
> +static void dsa_port_stop(struct udevice *pdev)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +	struct udevice *master = dsa_get_master(dev);
> +	struct dsa_ops *ops = dsa_get_ops(dev);
> +
> +	if (!priv)
> +		return;
> +
> +	if (ops->port_disable) {
> +		struct dsa_port_pdata *port_pdata;
> +
> +		port_pdata = dev_get_parent_plat(pdev);
> +		ops->port_disable(dev, port_pdata->index, port_pdata->phy);
> +		ops->port_disable(dev, priv->cpu_port, NULL);
> +	}
> +
> +	/*
> +	 * stop master only if it's active, don't probe it otherwise.
> +	 * Under normal usage it would be active because we're using it, but
> +	 * during tear-down it may have been removed ahead of us.
> +	 */
> +	if (master && device_active(master))
> +		eth_get_ops(master)->stop(master);
> +}
> +
> +/*
> + * Insert a DSA tag and call master Ethernet send on the resulting 
> packet
> + * We copy the frame to a stack buffer where we have reserved headroom 
> and
> + * tailroom space.  Headroom and tailroom are set to 0.
> + */
> +static int dsa_port_send(struct udevice *pdev, void *packet, int 
> length)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +	int head = priv->headroom, tail = priv->tailroom;
> +	struct udevice *master = dsa_get_master(dev);
> +	struct dsa_ops *ops = dsa_get_ops(dev);
> +	uchar dsa_packet_tmp[PKTSIZE_ALIGN];
> +	struct dsa_port_pdata *port_pdata;
> +	int err;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	if (length + head + tail > PKTSIZE_ALIGN)
> +		return -EINVAL;
> +
> +	memset(dsa_packet_tmp, 0, head);
> +	memset(dsa_packet_tmp + head + length, 0, tail);
> +	memcpy(dsa_packet_tmp + head, packet, length);
> +	length += head + tail;
> +	/* copy back to preserve original buffer alignment */
> +	memcpy(packet, dsa_packet_tmp, length);
> +
> +	port_pdata = dev_get_parent_plat(pdev);
> +	err = ops->xmit(dev, port_pdata->index, packet, length);
> +	if (err)
> +		return err;
> +
> +	return eth_get_ops(master)->send(master, packet, length);
> +}
> +
> +/* Receive a frame from master Ethernet, process it and pass it on */
> +static int dsa_port_recv(struct udevice *pdev, int flags, uchar 
> **packetp)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +	int head = priv->headroom, tail = priv->tailroom;
> +	struct udevice *master = dsa_get_master(dev);
> +	struct dsa_ops *ops = dsa_get_ops(dev);
> +	struct dsa_port_pdata *port_pdata;
> +	int length, port_index, err;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	length = eth_get_ops(master)->recv(master, flags, packetp);
> +	if (length <= 0)
> +		return length;
> +
> +	/*
> +	 * If we receive frames from a different port or frames that DSA 
> driver
> +	 * doesn't like we discard them here.
> +	 * In case of discard we return with no frame and expect to be called
> +	 * again instead of looping here, so upper layer can deal with 
> timeouts.
> +	 */
> +	port_pdata = dev_get_parent_plat(pdev);
> +	err = ops->rcv(dev, &port_index, *packetp, length);
> +	if (err || port_index != port_pdata->index || (length <= head + 
> tail)) {
> +		if (eth_get_ops(master)->free_pkt)
> +			eth_get_ops(master)->free_pkt(master, *packetp, length);
> +		return -EAGAIN;
> +	}
> +
> +	/*
> +	 * We move the pointer over headroom here to avoid a copy.  If 
> free_pkt
> +	 * gets called we move the pointer back before calling master 
> free_pkt.
> +	 */
> +	*packetp += head;
> +
> +	return length - head - tail;
> +}
> +
> +static int dsa_port_free_pkt(struct udevice *pdev, uchar *packet, int 
> length)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct udevice *master = dsa_get_master(dev);
> +	struct dsa_priv *priv;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	priv = dev_get_uclass_priv(dev);
> +	if (eth_get_ops(master)->free_pkt) {
> +		/* return the original pointer and length to master Eth */
> +		packet -= priv->headroom;
> +		length += priv->headroom - priv->tailroom;
> +
> +		return eth_get_ops(master)->free_pkt(master, packet, length);
> +	}
> +
> +	return 0;
> +}
> +
> +static int dsa_port_of_to_pdata(struct udevice *pdev)
> +{
> +	struct dsa_port_pdata *port_pdata;
> +	struct dsa_pdata *dsa_pdata;
> +	struct eth_pdata *eth_pdata;
> +	struct udevice *dev;
> +	const char *label;
> +	u32 index;
> +	int err;
> +
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	err = ofnode_read_u32(dev_ofnode(pdev), "reg", &index);
> +	if (err)
> +		return err;
> +
> +	dev = dev_get_parent(pdev);
> +	dsa_pdata = dev_get_uclass_plat(dev);
> +
> +	port_pdata = dev_get_parent_plat(pdev);
> +	port_pdata->index = index;
> +
> +	label = ofnode_read_string(dev_ofnode(pdev), "label");
> +	if (label)
> +		strncpy(port_pdata->name, label, DSA_PORT_NAME_LENGTH);
> +
> +	eth_pdata = dev_get_plat(pdev);
> +	eth_pdata->priv_pdata = port_pdata;
> +
> +	dev_dbg(pdev, "port %d node %s\n", port_pdata->index,
> +		ofnode_get_name(dev_ofnode(pdev)));
> +
> +	return 0;
> +}
> +
> +static const struct eth_ops dsa_port_ops = {
> +	.start		= dsa_port_start,
> +	.send		= dsa_port_send,
> +	.recv		= dsa_port_recv,
> +	.stop		= dsa_port_stop,
> +	.free_pkt	= dsa_port_free_pkt,
> +};
> +
> +static int dsa_port_probe(struct udevice *pdev)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct eth_pdata *eth_pdata, *master_pdata;
> +	unsigned char env_enetaddr[ARP_HLEN];
> +	struct dsa_port_pdata *port_pdata;
> +	struct dsa_priv *dsa_priv;
> +	struct udevice *master;
> +
> +	port_pdata = dev_get_parent_plat(pdev);
> +	dsa_priv = dev_get_uclass_priv(dev);
> +
> +	port_pdata->phy = dm_eth_phy_connect(pdev);
> +
> +	/*
> +	 * Inherit port's hwaddr from the DSA master, unless the port already
> +	 * has a unique MAC address specified in the environment.
> +	 */
> +	eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
> +	if (!is_zero_ethaddr(env_enetaddr))
> +		return 0;
> +
> +	master = dsa_get_master(dev);
> +	if (!master)
> +		return 0;
> +
> +	master_pdata = dev_get_plat(master);
> +	eth_pdata = dev_get_plat(pdev);
> +	memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
> +	eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
> +				      master_pdata->enetaddr);
> +
> +	return 0;
> +}
> +
> +static int dsa_port_remove(struct udevice *pdev)
> +{
> +	struct udevice *dev = dev_get_parent(pdev);
> +	struct dsa_port_pdata *port_pdata;
> +	struct dsa_priv *dsa_priv;
> +
> +	port_pdata = dev_get_parent_plat(pdev);
> +	dsa_priv = dev_get_uclass_priv(dev);
> +
> +	port_pdata->phy = NULL;
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(dsa_port) = {
> +	.name	= DSA_PORT_CHILD_DRV_NAME,
> +	.id	= UCLASS_ETH,
> +	.ops	= &dsa_port_ops,
> +	.probe	= dsa_port_probe,
> +	.remove	= dsa_port_remove,
> +	.of_to_plat = dsa_port_of_to_pdata,
> +	.plat_auto = sizeof(struct eth_pdata),
> +};
> +
> +/*
> + * This function mostly deals with pulling information out of the 
> device tree
> + * into the pdata structure.
> + * It goes through the list of switch ports, registers an eth device 
> for each
> + * front panel port and identifies the cpu port connected to master 
> eth device.
> + * TODO: support cascaded switches
> + */
> +static int dsa_post_bind(struct udevice *dev)
> +{
> +	struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
> +	ofnode node = dev_ofnode(dev), pnode;
> +	int i, err, first_err = 0;
> +
> +	if (!pdata || !ofnode_valid(node))
> +		return -ENODEV;
> +
> +	pdata->master_node = ofnode_null();
> +
> +	node = ofnode_find_subnode(node, "ports");
> +	if (!ofnode_valid(node))
> +		node = ofnode_find_subnode(node, "ethernet-ports");
> +	if (!ofnode_valid(node)) {
> +		dev_err(dev, "ports node is missing under DSA device!\n");
> +		return -EINVAL;
> +	}
> +
> +	pdata->num_ports = ofnode_get_child_count(node);
> +	if (pdata->num_ports <= 0 || pdata->num_ports > DSA_MAX_PORTS) {
> +		dev_err(dev, "invalid number of ports (%d)\n",
> +			pdata->num_ports);
> +		return -EINVAL;
> +	}
> +
> +	/* look for the CPU port */
> +	ofnode_for_each_subnode(pnode, node) {
> +		u32 ethernet;
> +
> +		if (ofnode_read_u32(pnode, "ethernet", &ethernet))
> +			continue;
> +
> +		pdata->master_node = ofnode_get_by_phandle(ethernet);
> +		break;
> +	}
> +
> +	if (!ofnode_valid(pdata->master_node)) {
> +		dev_err(dev, "master eth node missing!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ofnode_read_u32(pnode, "reg", &pdata->cpu_port)) {
> +		dev_err(dev, "CPU port node not valid!\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "master node %s on port %d\n",
> +		ofnode_get_name(pdata->master_node), pdata->cpu_port);
> +
> +	for (i = 0; i < pdata->num_ports; i++) {
> +		char name[DSA_PORT_NAME_LENGTH];
> +		struct udevice *pdev;
> +
> +		/*
> +		 * If this is the CPU port don't register it as an ETH device,
> +		 * we skip it on purpose since I/O to/from it from the CPU
> +		 * isn't useful
> +		 * TODO: cpu port may have a PHY and we don't handle that yet.
> +		 */
> +		if (i == pdata->cpu_port)
> +			continue;
> +
> +		/*
> +		 * Set up default port names.  If present, DT port labels
> +		 * will override the default port names.
> +		 */
> +		snprintf(name, DSA_PORT_NAME_LENGTH, "%s@%d", dev->name, i);
> +
> +		ofnode_for_each_subnode(pnode, node) {
> +			u32 reg;
> +
> +			if (ofnode_read_u32(pnode, "reg", &reg))
> +				continue;
> +
> +			if (reg == i)
> +				break;
> +		}
> +
> +		/*
> +		 * skip registration if port id not found or if the port
> +		 * is explicitly disabled in DT
> +		 */
> +		if (!ofnode_valid(pnode) || !ofnode_is_available(pnode))
> +			continue;
> +
> +		err = device_bind_driver_to_node(dev, DSA_PORT_CHILD_DRV_NAME,
> +						 name, pnode, &pdev);
> +		if (pdev) {
> +			struct dsa_port_pdata *port_pdata;
> +
> +			port_pdata = dev_get_parent_plat(pdev);
> +			strncpy(port_pdata->name, name, DSA_PORT_NAME_LENGTH);
> +			pdev->name = port_pdata->name;
> +		}
> +
> +		/* try to bind all ports but keep 1st error */
> +		if (err && !first_err)
> +			first_err = err;
> +	}
> +
> +	if (first_err)
> +		return first_err;
> +
> +	dev_dbg(dev, "DSA ports successfully bound\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * Initialize the uclass per device internal state structure (priv).
> + * TODO: pick up references to other switch devices here, if we're 
> cascaded.
> + */
> +static int dsa_pre_probe(struct udevice *dev)
> +{
> +	struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
> +	struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +
> +	if (!pdata || !priv)
> +		return -ENODEV;
> +
> +	priv->num_ports = pdata->num_ports;
> +	priv->cpu_port = pdata->cpu_port;
> +
> +	if (ofnode_valid(pdata->master_node))
> +		uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
> +					     &priv->master_dev);
> +	return 0;
> +}
> +
> +UCLASS_DRIVER(dsa) = {
> +	.id = UCLASS_DSA,
> +	.name = "dsa",
> +	.post_bind = dsa_post_bind,
> +	.pre_probe = dsa_pre_probe,
> +	.per_device_auto = sizeof(struct dsa_priv),
> +	.per_device_plat_auto = sizeof(struct dsa_pdata),
> +	.per_child_plat_auto = sizeof(struct dsa_port_pdata),
> +	.flags = DM_UC_FLAG_SEQ_ALIAS,
> +};

On Fri, Jan 22, 2021 at 10:30:48PM +0100, Michael Walle wrote:
> > +	if (ops->port_enable) {
> > +		struct dsa_port_pdata *port_pdata;
> > +
> > +		port_pdata = dev_get_parent_plat(pdev);
> > +		err = ops->port_enable(dev, port_pdata->index,
> > +				       port_pdata->phy);
> > +		if (err)
> > +			return err;
> > +
> > +		err = ops->port_enable(dev, priv->cpu_port, NULL);
>
> This will lead to a NULL pointer dereference in felix_port_enable()
> when accessing the phy. Although somehow (due to caching?) u-boot
> won't panic until reaching board_phy_config() (the weak one in
> drivers/net/phy/phy.c).

Weird that this never panicked for me. Also, it must be annoying to
debug a panic in board_phy_config() due to a bad dereference in
felix_start_pcs...

Also, I didn't think we'd reach such fundamental questions about DSA so
soon, but nonetheless, here we are. We don't have a struct phy_device
for the CPU port because we can't call phy_connect, because we don't
have an udevice for the CPU port, because we don't want to present it as
an UCLASS_ETH to anybody, because the user won't actually be able to use
it for traffic. That has been beaten to death already, the question is
what we can do here.

I would hate to inflict upon drivers the pain of checking for a NULL
pointer for the phy_device. It's also not only the checking that's
missing. It's actual information that's missing - we don't know if we
need to set up a 10G port, or 1G, or 100M port. So keeping the phy
argument as NULL is really off the table.

The DSA master should always have a fixed-link stanza at the very least,
no? And phy_connect_fixed fabricates an actual phy_device for that
fixed-link, that we could harvest for the speed, duplex and
phy_interface_t (well, _almost_ for the latter, we'd have to do some
direction reversal for PHY_INTERFACE_MODE_RGMII*, like TXID becomes
RXID, RGMII becomes RGMII_ID...). And this could only ever work for
the fixed-link species of a phy_device. There have been reports from the
people at Bootlin that they were working with some mv88e6xxx switch that
needed an actual struct phy_device in Linux, and phylink/phylib gained
support for working without an attached net device. But that's fairly
odd, so in practice I think it wouldn't be a huge loss if fixed-link is
all we had for the CPU port.

The crux of the problem is that phy_connect needs an udevice to parse
the device tree information from. So it can't just be any udevice, it
must be the udevice bound to the OF node holding the phy-handle and/or
fixed-link stanza. If it could have been any udevice I would have just
passed it the UCLASS_DSA udevice corresponding to the switch itself in a
heartbeat. So if we decided to treat the actual problem, we could:
- Register yet another uclass, something along the lines of
  UCLASS_DSA_HIDDEN_PORT, which will bind to the OF node of the CPU port
  so that we can pass it to dm_eth_phy_connect. I looked at the code
  path and it looks like nobody really cares if it's UCLASS_ETH or not.
  That would inflict some pain to the maintainers of the networking
  code, nonetheless.
- We could simply fabricate a struct phy_device by hand-parsing the
  fixed-link stanza of the CPU port, right where we're skipping over it
  in dsa_post_bind, where we left that damn TODO. We could then keep
  this struct phy_device cpu_phy in struct dsa_priv, not connected to
  any udevice, just fabricated, so that we could pass it to port_enable.
  We'd still be limiting ourselves to just fixed-links though.
- Bite the bullet and register a UCLASS_ETH for the CPU port. It won't
  do anything useful for the user, but it would keep the code simple...