[PATCH 0/6] Add MV88E61xx DSA driver and use on gwventana

[PATCH 0/6] Add MV88E61xx DSA driver and use on gwventana

[Tim Harvey]

This series adds a DSA driver for the MV88E61xx based on
drivers/net/phy/mv88e61xx and uses in on the gwventana_gw5904_defconfig.

The hope is that the other three boards that use the MV88E61xx driver
can move to this as well eventually so that we can remove the non-dm
driver and the 4 Kconfig options it requires.

The MV88E61xx has an MDIO interface this DM_MDIO must be used so support
for a UCLASS_MDIO driver is added to the fec_mxc ethernet driver in a
way that allows a fallback to the previous non DM_MDIO case as there are
many boards out there using this driver that define DM_MDIO but do not
have the required dt props for a DM_MDIO driver which would cause a
regression.

Additionally a patch to ensure MDIO children are scanned on post-bind is
needed.

I'm making use of the dm_mdio_read/dm_mdio_write wrapper from Merak that
is pending so it is in this series as well.

Best Regards,

Tim

Tim Harvey (6):
  net: mdio-uclass: scan for dm mdio children on post-bind
  net: dsa: move cpu port probe to dsa_post_probe
  net: mdio-uclass: add wrappers for read/write/reset operations
  net: fec: add support for DM_MDIO
  net: add MV88E61xx DSA driver
  board: gw_ventana: enable MV88E61XX DSA support

 arch/arm/dts/imx6qdl-gw5904.dtsi        |  35 +
 board/gateworks/gw_ventana/gw_ventana.c |  50 +-
 configs/gwventana_gw5904_defconfig      |   7 +-
 drivers/net/Kconfig                     |   7 +
 drivers/net/Makefile                    |   1 +
 drivers/net/fec_mxc.c                   | 113 ++-
 drivers/net/fec_mxc.h                   |   1 +
 drivers/net/mv88e61xx.c                 | 982 ++++++++++++++++++++++++
 include/miiphy.h                        |  31 +
 net/dsa-uclass.c                        |  14 +-
 net/mdio-uclass.c                       |  35 +
 11 files changed, 1227 insertions(+), 49 deletions(-)
 create mode 100644 drivers/net/mv88e61xx.c

-- 
2.17.1

[PATCH 1/6] net: mdio-uclass: scan for dm mdio children on post-bind

[Tim Harvey]

If a DM_MDIO driver is used we need to scan the subnodes as well.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/mdio-uclass.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index e74e34f78f9c..190cb08b31d8 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -59,7 +59,11 @@ static int dm_mdio_post_bind(struct udevice *dev)
 		return -EINVAL;
 	}
 
+#if CONFIG_IS_ENABLED(OF_REAL)
+	return dm_scan_fdt_dev(dev);
+#else
 	return 0;
+#endif
 }
 
 /*
-- 
2.17.1

[PATCH 2/6] net: dsa: move cpu port probe to dsa_post_probe

[Tim Harvey]

In order to ensure that a DSA driver probe gets called before
dsa_ops->port_probe move the port_probe of the cpu_port to
a post-probe function.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 net/dsa-uclass.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index 9ff55a02fb23..07edc584daf3 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -466,8 +466,6 @@ 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);
-	struct dsa_ops *ops = dsa_get_ops(dev);
-	int err;
 
 	priv->num_ports = pdata->num_ports;
 	priv->cpu_port = pdata->cpu_port;
@@ -480,6 +478,15 @@ static int dsa_pre_probe(struct udevice *dev)
 	uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
 				     &priv->master_dev);
 
+	return 0;
+}
+
+static int dsa_post_probe(struct udevice *dev)
+{
+	struct dsa_priv *priv = dev_get_uclass_priv(dev);
+	struct dsa_ops *ops = dsa_get_ops(dev);
+	int err;
+
 	/* Simulate a probing event for the CPU port */
 	if (ops->port_probe) {
 		err = ops->port_probe(dev, priv->cpu_port,
@@ -489,13 +496,14 @@ static int dsa_pre_probe(struct udevice *dev)
 	}
 
 	return 0;
-}
+};
 
 UCLASS_DRIVER(dsa) = {
 	.id = UCLASS_DSA,
 	.name = "dsa",
 	.post_bind = dsa_post_bind,
 	.pre_probe = dsa_pre_probe,
+	.post_probe = dsa_post_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),
-- 
2.17.1

[PATCH 3/6] net: mdio-uclass: add wrappers for read/write/reset operations

[Tim Harvey]

Add wrappers dm_mdio_read(), dm_mdio_write() and dm_mdio_reset() for
DM MDIO's .read(), .write() and .reset() operations.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---
 include/miiphy.h  | 31 +++++++++++++++++++++++++++++++
 net/mdio-uclass.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/miiphy.h b/include/miiphy.h
index 235ae066dddd..110921f20d2a 100644
--- a/include/miiphy.h
+++ b/include/miiphy.h
@@ -157,6 +157,37 @@ struct mdio_ops {
  */
 void dm_mdio_probe_devices(void);
 
+/**
+ * dm_mdio_read - Wrapper over .read() operation for DM MDIO
+ *
+ * @mdiodev: mdio device
+ * @addr: PHY address on MDIO bus
+ * @devad: device address on PHY if C45; should be MDIO_DEVAD_NONE if C22
+ * @reg: register address
+ * Return: register value if non-negative, -error code otherwise
+ */
+int dm_mdio_read(struct udevice *mdio_dev, int addr, int devad, int reg);
+
+/**
+ * dm_mdio_write - Wrapper over .write() operation for DM MDIO
+ *
+ * @mdiodev: mdio device
+ * @addr: PHY address on MDIO bus
+ * @devad: device address on PHY if C45; should be MDIO_DEVAD_NONE if C22
+ * @reg: register address
+ * @val: value to write
+ * Return: 0 on success, -error code otherwise
+ */
+int dm_mdio_write(struct udevice *mdio_dev, int addr, int devad, int reg, u16 val);
+
+/**
+ * dm_mdio_reset - Wrapper over .reset() operation for DM MDIO
+ *
+ * @mdiodev: mdio device
+ * Return: 0 on success, -error code otherwise
+ */
+int dm_mdio_reset(struct udevice *mdio_dev);
+
 /**
  * dm_mdio_phy_connect - Wrapper over phy_connect for DM MDIO
  *
diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index 190cb08b31d8..523c0b12c7d2 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -66,6 +66,37 @@ static int dm_mdio_post_bind(struct udevice *dev)
 #endif
 }
 
+int dm_mdio_read(struct udevice *mdio_dev, int addr, int devad, int reg)
+{
+	struct mdio_ops *ops = mdio_get_ops(mdio_dev);
+
+	if (!ops->read)
+		return -ENOSYS;
+
+	return ops->read(mdio_dev, addr, devad, reg);
+}
+
+int dm_mdio_write(struct udevice *mdio_dev, int addr, int devad, int reg,
+		  u16 val)
+{
+	struct mdio_ops *ops = mdio_get_ops(mdio_dev);
+
+	if (!ops->write)
+		return -ENOSYS;
+
+	return ops->write(mdio_dev, addr, devad, reg, val);
+}
+
+int dm_mdio_reset(struct udevice *mdio_dev)
+{
+	struct mdio_ops *ops = mdio_get_ops(mdio_dev);
+
+	if (!ops->reset)
+		return 0;
+
+	return ops->reset(mdio_dev);
+}
+
 /*
  * Following read/write/reset functions are registered with legacy MII code.
  * These are called for PHY operations by upper layers and we further call the
-- 
2.17.1

[PATCH 4/6] net: fec: add support for DM_MDIO

[Tim Harvey]

Add support for DM_MDIO by registering a UCLASS_MDIO driver and
attempting to use it. This is necessary if wanting to use a DSA
driver for example hanging off of the FEC MAC.

Care is taken to fallback to non DM_MDIO as several boards define
DM_MDIO without having the proper device-tree configuration necessary
such as an mdio subnode, a phy-mode prop, and either a valid phy-handle
prop or fixed-phy subnode.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/net/fec_mxc.c | 113 ++++++++++++++++++++++++++++++++++++++----
 drivers/net/fec_mxc.h |   1 +
 2 files changed, 104 insertions(+), 10 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index e8ebef09032a..374ef9d67d5e 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -30,6 +30,8 @@
 #include <asm/arch/imx-regs.h>
 #include <asm/mach-imx/sys_proto.h>
 #include <asm-generic/gpio.h>
+#include <dm/device_compat.h>
+#include <dm/lists.h>
 
 #include "fec_mxc.h"
 #include <eth_phy.h>
@@ -1025,6 +1027,80 @@ struct mii_dev *fec_get_miibus(ulong base_addr, int dev_id)
 	return bus;
 }
 
+#ifdef CONFIG_DM_MDIO
+struct dm_fec_mdio_priv {
+	struct ethernet_regs *regs;
+};
+
+static int dm_fec_mdio_read(struct udevice *dev, int addr, int devad, int reg)
+{
+	struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
+
+	return fec_mdio_read(priv->regs, addr, reg);
+}
+
+static int dm_fec_mdio_write(struct udevice *dev, int addr, int devad, int reg, u16 data)
+{
+	struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
+
+	return fec_mdio_write(priv->regs, addr, reg, data);
+}
+
+static const struct mdio_ops dm_fec_mdio_ops = {
+	.read = dm_fec_mdio_read,
+	.write = dm_fec_mdio_write,
+};
+
+static int dm_fec_mdio_probe(struct udevice *dev)
+{
+	struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
+
+	priv->regs = (struct ethernet_regs *)ofnode_get_addr(dev_ofnode(dev->parent));
+
+	return 0;
+}
+
+U_BOOT_DRIVER(fec_mdio) = {
+	.name		= "fec_mdio",
+	.id		= UCLASS_MDIO,
+	.probe		= dm_fec_mdio_probe,
+	.ops		= &dm_fec_mdio_ops,
+	.priv_auto	= sizeof(struct dm_fec_mdio_priv),
+};
+
+static int dm_fec_bind_mdio(struct udevice *dev)
+{
+	struct udevice *mdiodev;
+	const char *name;
+	ofnode mdio;
+	int ret;
+
+	/* for a UCLASS_MDIO driver we need to bind and probe manually
+	 * for an internal MDIO bus that has no dt compatible of its own
+	 */
+	ofnode_for_each_subnode(mdio, dev_ofnode(dev)) {
+		name = ofnode_get_name(mdio);
+
+		if (strcmp(name, "mdio"))
+			continue;
+
+		ret = device_bind_driver_to_node(dev, "fec_mdio",
+						 name, mdio, &mdiodev);
+		if (ret)
+			return ret;
+
+		/* need to probe it as there is no compatible to do so */
+		ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdiodev);
+		if (!ret)
+			return 0;
+
+		break;
+	}
+
+	return ret;
+}
+#endif
+
 static int fecmxc_read_rom_hwaddr(struct udevice *dev)
 {
 	struct fec_priv *priv = dev_get_priv(dev);
@@ -1088,7 +1164,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev)
 
 static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
 {
-	struct phy_device *phydev;
+	struct phy_device *phydev = NULL;
 	int addr;
 
 	addr = device_get_phy_addr(priv, dev);
@@ -1096,7 +1172,12 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
 	addr = CONFIG_FEC_MXC_PHYADDR;
 #endif
 
-	phydev = phy_connect(priv->bus, addr, dev, priv->interface);
+#ifdef CONFIG_DM_MDIO
+	if (priv->dm_mdio)
+		phydev = dm_eth_phy_connect(dev);
+#endif
+	if (!phydev)
+		phydev = phy_connect(priv->bus, addr, dev, priv->interface);
 	if (!phydev)
 		return -ENODEV;
 
@@ -1227,11 +1308,19 @@ static int fecmxc_probe(struct udevice *dev)
 
 	priv->dev_id = dev_seq(dev);
 
+#ifdef CONFIG_DM_MDIO
+	ret = dm_fec_bind_mdio(dev);
+	if (!ret) {
+		ret = fec_phy_init(priv, dev);
+		if (!ret)
+			priv->dm_mdio = true;
+	}
+#endif
 #ifdef CONFIG_DM_ETH_PHY
 	bus = eth_phy_get_mdio_bus(dev);
 #endif
 
-	if (!bus) {
+	if (!bus && !priv->dm_mdio) {
 		dm_mii_bus = false;
 #ifdef CONFIG_FEC_MXC_MDIO_BASE
 		bus = fec_get_miibus((ulong)CONFIG_FEC_MXC_MDIO_BASE,
@@ -1240,7 +1329,7 @@ static int fecmxc_probe(struct udevice *dev)
 		bus = fec_get_miibus((ulong)priv->eth, dev_seq(dev));
 #endif
 	}
-	if (!bus) {
+	if (!bus && !priv->dm_mdio) {
 		ret = -ENOMEM;
 		goto err_mii;
 	}
@@ -1271,14 +1360,16 @@ static int fecmxc_probe(struct udevice *dev)
 		break;
 	}
 
-	ret = fec_phy_init(priv, dev);
-	if (ret)
-		goto err_phy;
+	if (!priv->dm_mdio) {
+		ret = fec_phy_init(priv, dev);
+		if (ret)
+			goto err_phy;
+	}
 
 	return 0;
 
 err_phy:
-	if (!dm_mii_bus) {
+	if (!dm_mii_bus && !priv->dm_mdio) {
 		mdio_unregister(bus);
 		free(bus);
 	}
@@ -1294,8 +1385,10 @@ static int fecmxc_remove(struct udevice *dev)
 
 	free(priv->phydev);
 	fec_free_descs(priv);
-	mdio_unregister(priv->bus);
-	mdio_free(priv->bus);
+	if (priv->bus) {
+		mdio_unregister(priv->bus);
+		mdio_free(priv->bus);
+	}
 
 #ifdef CONFIG_DM_REGULATOR
 	if (priv->phy_supply)
diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
index 48faa33d66ec..c297880ccc54 100644
--- a/drivers/net/fec_mxc.h
+++ b/drivers/net/fec_mxc.h
@@ -248,6 +248,7 @@ struct fec_priv {
 	uint8_t *tdb_ptr;
 	int dev_id;
 	struct mii_dev *bus;
+	bool dm_mdio;
 #ifdef CONFIG_PHYLIB
 	struct phy_device *phydev;
 	ofnode phy_of_node;
-- 
2.17.1

[PATCH 5/6] net: add MV88E61xx DSA driver

[Tim Harvey]

Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/net/Kconfig     |   7 +
 drivers/net/Makefile    |   1 +
 drivers/net/mv88e61xx.c | 982 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 990 insertions(+)
 create mode 100644 drivers/net/mv88e61xx.c

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index a6171a7c7ffd..fc018f5ba47f 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -428,6 +428,13 @@ config LPC32XX_ETH
 	depends on ARCH_LPC32XX
 	default y
 
+config MV88E61XX
+	bool "Marvell MV88E61xx GbE switch DSA driver"
+	depends on DM_DSA && DM_MDIO
+	help
+	  This driver implements a DSA switch driver for the MV88E61xx family
+	  of GbE switches using the MDIO interface
+
 config MVGBE
 	bool "Marvell Orion5x/Kirkwood network interface support"
 	depends on ARCH_KIRKWOOD || ARCH_ORION5X
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index a6d0c23f02d3..11ada73658e9 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_MEDIATEK_ETH) += mtk_eth.o
 obj-$(CONFIG_MPC8XX_FEC) += mpc8xx_fec.o
 obj-$(CONFIG_MT7620_ETH) += mt7620-eth.o
 obj-$(CONFIG_MT7628_ETH) += mt7628-eth.o
+obj-$(CONFIG_MV88E61XX) += mv88e61xx.o
 obj-$(CONFIG_MVGBE) += mvgbe.o
 obj-$(CONFIG_MVMDIO) += mvmdio.o
 obj-$(CONFIG_MVNETA) += mvneta.o
diff --git a/drivers/net/mv88e61xx.c b/drivers/net/mv88e61xx.c
new file mode 100644
index 000000000000..9dd7a0c7f42e
--- /dev/null
+++ b/drivers/net/mv88e61xx.c
@@ -0,0 +1,982 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2022
+ * Gateworks Corporation <www.gateworks.com>
+ * Tim Harvey <tharvey@gateworks.com>
+ *
+ * (C) Copyright 2015
+ * Elecsys Corporation <www.elecsyscorp.com>
+ * Kevin Smith <kevin.smith@elecsyscorp.com>
+ *
+ * Original driver:
+ * (C) Copyright 2009
+ * Marvell Semiconductor <www.marvell.com>
+ * Prafulla Wadaskar <prafulla@marvell.com>
+ */
+
+/*
+ * DSA driver for mv88e61xx ethernet switches.
+ *
+ * This driver configures the mv88e61xx for basic use as a DSA switch.
+ *
+ * This driver was adapted from drivers/net/phy/mv88e61xx and tested
+ * on the mv88e6176 via an SGMII interface.
+ */
+
+#include <bitfield.h>
+#include <common.h>
+#include <miiphy.h>
+#include <dm/device.h>
+#include <dm/device_compat.h>
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/of_extra.h>
+#include <linux/delay.h>
+#include <net/dsa.h>
+
+#define PORT_MASK(port_count)		((1 << (port_count)) - 1)
+
+/* Device addresses */
+#define DEVADDR_PHY(p)			(p)
+#define DEVADDR_SERDES			0x0F
+
+/* SMI indirection registers for multichip addressing mode */
+#define SMI_CMD_REG			0x00
+#define SMI_DATA_REG			0x01
+
+/* Global registers */
+#define GLOBAL1_STATUS			0x00
+#define GLOBAL1_CTRL			0x04
+#define GLOBAL1_MON_CTRL		0x1A
+
+/* Global 2 registers */
+#define GLOBAL2_REG_PHY_CMD		0x18
+#define GLOBAL2_REG_PHY_DATA		0x19
+#define GLOBAL2_REG_SCRATCH		0x1A
+
+/* Port registers */
+#define PORT_REG_STATUS			0x00
+#define PORT_REG_PHYS_CTRL		0x01
+#define PORT_REG_SWITCH_ID		0x03
+#define PORT_REG_CTRL			0x04
+#define PORT_REG_VLAN_MAP		0x06
+#define PORT_REG_VLAN_ID		0x07
+#define PORT_REG_LED_CTRL		0x16
+
+/* Phy registers */
+#define PHY_REG_CTRL1			0x10
+#define PHY_REG_STATUS1			0x11
+#define PHY_REG_PAGE			0x16
+
+/* Serdes registers */
+#define SERDES_REG_CTRL_1		0x10
+
+/* Phy page numbers */
+#define PHY_PAGE_COPPER			0
+#define PHY_PAGE_SERDES			1
+
+/* Register fields */
+#define GLOBAL1_CTRL_SWRESET		BIT(15)
+
+#define GLOBAL1_MON_CTRL_CPUDEST_SHIFT	4
+#define GLOBAL1_MON_CTRL_CPUDEST_WIDTH	4
+
+#define PORT_REG_STATUS_SPEED_SHIFT	8
+#define PORT_REG_STATUS_SPEED_10	0
+#define PORT_REG_STATUS_SPEED_100	1
+#define PORT_REG_STATUS_SPEED_1000	2
+
+#define PORT_REG_STATUS_CMODE_MASK		0xF
+#define PORT_REG_STATUS_CMODE_100BASE_X		0x8
+#define PORT_REG_STATUS_CMODE_1000BASE_X	0x9
+#define PORT_REG_STATUS_CMODE_SGMII		0xa
+
+#define PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK	BIT(15)
+#define PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK	BIT(14)
+#define PORT_REG_PHYS_CTRL_PCS_AN_EN	BIT(10)
+#define PORT_REG_PHYS_CTRL_PCS_AN_RST	BIT(9)
+#define PORT_REG_PHYS_CTRL_FC_VALUE	BIT(7)
+#define PORT_REG_PHYS_CTRL_FC_FORCE	BIT(6)
+#define PORT_REG_PHYS_CTRL_LINK_VALUE	BIT(5)
+#define PORT_REG_PHYS_CTRL_LINK_FORCE	BIT(4)
+#define PORT_REG_PHYS_CTRL_DUPLEX_VALUE	BIT(3)
+#define PORT_REG_PHYS_CTRL_DUPLEX_FORCE	BIT(2)
+#define PORT_REG_PHYS_CTRL_SPD1000	BIT(1)
+#define PORT_REG_PHYS_CTRL_SPD100	BIT(0)
+#define PORT_REG_PHYS_CTRL_SPD_MASK	(BIT(1) | BIT(0))
+
+#define PORT_REG_CTRL_PSTATE_SHIFT	0
+#define PORT_REG_CTRL_PSTATE_WIDTH	2
+
+#define PORT_REG_VLAN_ID_DEF_VID_SHIFT	0
+#define PORT_REG_VLAN_ID_DEF_VID_WIDTH	12
+
+#define PORT_REG_VLAN_MAP_TABLE_SHIFT	0
+#define PORT_REG_VLAN_MAP_TABLE_WIDTH	11
+
+#define SERDES_REG_CTRL_1_FORCE_LINK	BIT(10)
+
+/* Field values */
+#define PORT_REG_CTRL_PSTATE_DISABLED	0
+#define PORT_REG_CTRL_PSTATE_FORWARD	3
+
+#define PHY_REG_CTRL1_ENERGY_DET_OFF	0
+#define PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE	1
+#define PHY_REG_CTRL1_ENERGY_DET_SENSE_ONLY	2
+#define PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT	3
+
+/* PHY Status Register */
+#define PHY_REG_STATUS1_SPEED		0xc000
+#define PHY_REG_STATUS1_GBIT		0x8000
+#define PHY_REG_STATUS1_100		0x4000
+#define PHY_REG_STATUS1_DUPLEX		0x2000
+#define PHY_REG_STATUS1_SPDDONE		0x0800
+#define PHY_REG_STATUS1_LINK		0x0400
+#define PHY_REG_STATUS1_ENERGY		0x0010
+
+/*
+ * Macros for building commands for indirect addressing modes.  These are valid
+ * for both the indirect multichip addressing mode and the PHY indirection
+ * required for the writes to any PHY register.
+ */
+#define SMI_BUSY			BIT(15)
+#define SMI_CMD_CLAUSE_22		BIT(12)
+#define SMI_CMD_CLAUSE_22_OP_READ	(2 << 10)
+#define SMI_CMD_CLAUSE_22_OP_WRITE	(1 << 10)
+
+#define SMI_CMD_READ			(SMI_BUSY | SMI_CMD_CLAUSE_22 | \
+					 SMI_CMD_CLAUSE_22_OP_READ)
+#define SMI_CMD_WRITE			(SMI_BUSY | SMI_CMD_CLAUSE_22 | \
+					 SMI_CMD_CLAUSE_22_OP_WRITE)
+
+#define SMI_CMD_ADDR_SHIFT		5
+#define SMI_CMD_ADDR_WIDTH		5
+#define SMI_CMD_REG_SHIFT		0
+#define SMI_CMD_REG_WIDTH		5
+
+/* Defines for Scratch and Misc Registers */
+#define SCRATCH_UPDATE			BIT(15)
+#define SCRATCH_ADDR_SHIFT		8
+#define SCRATCH_ADDR_WIDTH		7
+
+/* Scratch registers */
+#define SCRATCH_ADDR_GPIO0DIR		0x62 /* GPIO[7:0] direction 1=input */
+#define SCRATCH_ADDR_GPIO1DIR		0x63 /* GPIO[14:8] direction 1=input */
+#define SCRATCH_ADDR_GPIO0DATA		0x64 /* GPIO[7:0] data */
+#define SCRATCH_ADDR_GPIO1DATA		0x65 /* GPIO[14:8] data */
+#define SCRATCH_ADDR_GPIO0CTRL		0x68 /* GPIO[1:0] control */
+#define SCRATCH_ADDR_GPIO1CTRL		0x69 /* GPIO[3:2] control */
+
+/* ID register values for different switch models */
+#define PORT_SWITCH_ID_6020		0x0200
+#define PORT_SWITCH_ID_6070		0x0700
+#define PORT_SWITCH_ID_6071		0x0710
+#define PORT_SWITCH_ID_6096		0x0980
+#define PORT_SWITCH_ID_6097		0x0990
+#define PORT_SWITCH_ID_6172		0x1720
+#define PORT_SWITCH_ID_6176		0x1760
+#define PORT_SWITCH_ID_6220		0x2200
+#define PORT_SWITCH_ID_6240		0x2400
+#define PORT_SWITCH_ID_6250		0x2500
+#define PORT_SWITCH_ID_6352		0x3520
+
+struct mv88e61xx_phy_priv {
+	int active_port;
+	int smi_addr;
+	int id;
+	int port_count;		/* Number of switch ports */
+	int port_reg_base;	/* Base of the switch port registers */
+//	u16 port_stat_link_mask;/* Bitmask for port link status bits */
+//	u16 port_stat_dup_mask; /* Bitmask for port duplex status bits */
+//	u8 port_stat_speed_width;/* Width of speed status bitfield */
+	u8 global1;	/* Offset of Switch Global 1 registers */
+	u8 global2;	/* Offset of Switch Global 2 registers */
+	u8 phy_ctrl1_en_det_shift; /* 'EDet' bit field offset */
+	u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
+	u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
+};
+
+static inline int smi_cmd(int cmd, int addr, int reg)
+{
+	cmd = bitfield_replace(cmd, SMI_CMD_ADDR_SHIFT, SMI_CMD_ADDR_WIDTH,
+			       addr);
+	cmd = bitfield_replace(cmd, SMI_CMD_REG_SHIFT, SMI_CMD_REG_WIDTH, reg);
+	return cmd;
+}
+
+static inline int smi_cmd_read(int addr, int reg)
+{
+	return smi_cmd(SMI_CMD_READ, addr, reg);
+}
+
+static inline int smi_cmd_write(int addr, int reg)
+{
+	return smi_cmd(SMI_CMD_WRITE, addr, reg);
+}
+
+/* Wait for the current SMI indirect command to complete */
+static int mv88e61xx_smi_wait(struct udevice *dev, int smi_addr)
+{
+	int val;
+	u32 timeout = 100;
+
+	do {
+		val = dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG);
+		if (val >= 0 && (val & SMI_BUSY) == 0)
+			return 0;
+
+		mdelay(1);
+	} while (--timeout);
+
+	dev_err(dev, "SMI busy timeout\n");
+	return -ETIMEDOUT;
+}
+
+/*
+ * The mv88e61xx has three types of addresses: the smi bus address, the device
+ * address, and the register address.  The smi bus address distinguishes it on
+ * the smi bus from other PHYs or switches.  The device address determines
+ * which on-chip register set you are reading/writing (the various PHYs, their
+ * associated ports, or global configuration registers).  The register address
+ * is the offset of the register you are reading/writing.
+ *
+ * When the mv88e61xx is hardware configured to have address zero, it behaves in
+ * single-chip addressing mode, where it responds to all SMI addresses, using
+ * the smi address as its device address.  This obviously only works when this
+ * is the only chip on the SMI bus.  This allows the driver to access device
+ * registers without using indirection.  When the chip is configured to a
+ * non-zero address, it only responds to that SMI address and requires indirect
+ * writes to access the different device addresses.
+ */
+static int mv88e61xx_reg_read(struct udevice *dev, int addr, int reg)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int smi_addr = priv->smi_addr;
+	int res;
+
+	/* In single-chip mode, the device can be addressed directly */
+	if (smi_addr == 0)
+		return dm_mdio_read(dev->parent, addr, MDIO_DEVAD_NONE, reg);
+
+	/* Wait for the bus to become free */
+	res = mv88e61xx_smi_wait(dev, smi_addr);
+	if (res < 0)
+		return res;
+
+	/* Issue the read command */
+	res = dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG,
+			    smi_cmd_read(addr, reg));
+	if (res < 0)
+		return res;
+
+	/* Wait for the read command to complete */
+	res = mv88e61xx_smi_wait(dev, smi_addr);
+	if (res < 0)
+		return res;
+
+	/* Read the data */
+	res = dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_DATA_REG);
+	if (res < 0)
+		return res;
+
+	return bitfield_extract(res, 0, 16);
+}
+
+/* See the comment above mv88e61xx_reg_read */
+static int mv88e61xx_reg_write(struct udevice *dev, int addr, int reg, u16 val)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int smi_addr = priv->smi_addr;
+	int res;
+
+	/* In single-chip mode, the device can be addressed directly */
+	if (smi_addr == 0)
+		return dm_mdio_write(dev->parent, addr, MDIO_DEVAD_NONE, reg, val);
+
+	/* Wait for the bus to become free */
+	res = mv88e61xx_smi_wait(dev, smi_addr);
+	if (res < 0)
+		return res;
+
+	/* Set the data to write */
+	res = dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE,
+			    SMI_DATA_REG, val);
+	if (res < 0)
+		return res;
+
+	/* Issue the write command */
+	res = dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG,
+			    smi_cmd_write(addr, reg));
+	if (res < 0)
+		return res;
+
+	/* Wait for the write command to complete */
+	res = mv88e61xx_smi_wait(dev, smi_addr);
+	if (res < 0)
+		return res;
+
+	return 0;
+}
+
+static int mv88e61xx_phy_wait(struct udevice *dev)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int val;
+	u32 timeout = 100;
+
+	do {
+		val = mv88e61xx_reg_read(dev, priv->global2, GLOBAL2_REG_PHY_CMD);
+		if (val >= 0 && (val & SMI_BUSY) == 0)
+			return 0;
+
+		mdelay(1);
+	} while (--timeout);
+
+	return -ETIMEDOUT;
+}
+
+static int mv88e61xx_phy_read_indirect(struct udevice *dev, int phyad, int devad, int reg)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int res;
+
+	/* Issue command to read */
+	res = mv88e61xx_reg_write(dev, priv->global2,
+				  GLOBAL2_REG_PHY_CMD,
+				  smi_cmd_read(phyad, reg));
+
+	/* Wait for data to be read */
+	res = mv88e61xx_phy_wait(dev);
+	if (res < 0)
+		return res;
+
+	/* Read retrieved data */
+	return mv88e61xx_reg_read(dev, priv->global2,
+				  GLOBAL2_REG_PHY_DATA);
+}
+
+static int mv88e61xx_phy_write_indirect(struct udevice *dev, int phyad,
+					int devad, int reg, u16 data)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int res;
+
+	/* Set the data to write */
+	res = mv88e61xx_reg_write(dev, priv->global2,
+				  GLOBAL2_REG_PHY_DATA, data);
+	if (res < 0)
+		return res;
+	/* Issue the write command */
+	res = mv88e61xx_reg_write(dev, priv->global2,
+				  GLOBAL2_REG_PHY_CMD,
+				  smi_cmd_write(phyad, reg));
+	if (res < 0)
+		return res;
+
+	/* Wait for command to complete */
+	return mv88e61xx_phy_wait(dev);
+}
+
+/* Wrapper function to make calls to phy_read_indirect simpler */
+static int mv88e61xx_phy_read(struct udevice *dev, int phy, int reg)
+{
+	return mv88e61xx_phy_read_indirect(dev, DEVADDR_PHY(phy),
+					   MDIO_DEVAD_NONE, reg);
+}
+
+/* Wrapper function to make calls to phy_write_indirect simpler */
+static int mv88e61xx_phy_write(struct udevice *dev, int phy, int reg, u16 val)
+{
+	return mv88e61xx_phy_write_indirect(dev, DEVADDR_PHY(phy),
+					    MDIO_DEVAD_NONE, reg, val);
+}
+
+static int mv88e61xx_port_read(struct udevice *dev, u8 port, u8 reg)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+
+	return mv88e61xx_reg_read(dev, priv->port_reg_base + port, reg);
+}
+
+static int mv88e61xx_port_write(struct udevice *dev, u8 port, u8 reg, u16 val)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+
+	return mv88e61xx_reg_write(dev, priv->port_reg_base + port, reg, val);
+}
+
+static int mv88e61xx_set_page(struct udevice *dev, u8 phy, u8 page)
+{
+	return mv88e61xx_phy_write(dev, phy, PHY_REG_PAGE, page);
+}
+
+static int mv88e61xx_get_switch_id(struct udevice *dev)
+{
+	int res;
+
+	res = mv88e61xx_port_read(dev, 0, PORT_REG_SWITCH_ID);
+	if (res < 0)
+		return res;
+	return res & 0xfff0;
+}
+
+static bool mv88e61xx_6352_family(struct udevice *dev)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+
+	switch (priv->id) {
+	case PORT_SWITCH_ID_6172:
+	case PORT_SWITCH_ID_6176:
+	case PORT_SWITCH_ID_6240:
+	case PORT_SWITCH_ID_6352:
+		return true;
+	}
+	return false;
+}
+
+static int mv88e61xx_get_cmode(struct udevice *dev, u8 port)
+{
+	int res;
+
+	res = mv88e61xx_port_read(dev, port, PORT_REG_STATUS);
+	if (res < 0)
+		return res;
+	return res & PORT_REG_STATUS_CMODE_MASK;
+}
+
+static int mv88e61xx_switch_reset(struct udevice *dev)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int time;
+	int val;
+	u8 port;
+
+	/* Disable all ports */
+	for (port = 0; port < priv->port_count; port++) {
+		val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL);
+		if (val < 0)
+			return val;
+		val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT,
+				       PORT_REG_CTRL_PSTATE_WIDTH,
+				       PORT_REG_CTRL_PSTATE_DISABLED);
+		val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val);
+		if (val < 0)
+			return val;
+	}
+
+	/* Wait 2 ms for queues to drain */
+	udelay(2000);
+
+	/* Reset switch */
+	val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL);
+	if (val < 0)
+		return val;
+	val |= GLOBAL1_CTRL_SWRESET;
+	val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_CTRL, val);
+	if (val < 0)
+		return val;
+
+	/* Wait up to 1 second for switch reset complete */
+	for (time = 1000; time; time--) {
+		val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL);
+		if (val >= 0 && ((val & GLOBAL1_CTRL_SWRESET) == 0))
+			break;
+		udelay(1000);
+	}
+	if (!time)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int mv88e61xx_serdes_init(struct udevice *dev)
+{
+	int val;
+
+	val = mv88e61xx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES);
+	if (val < 0)
+		return val;
+
+	/* Power up serdes module */
+	val = mv88e61xx_phy_read(dev, DEVADDR_SERDES, MII_BMCR);
+	if (val < 0)
+		return val;
+	val &= ~(BMCR_PDOWN);
+	val = mv88e61xx_phy_write(dev, DEVADDR_SERDES, MII_BMCR, val);
+	if (val < 0)
+		return val;
+
+	return 0;
+}
+
+static int mv88e61xx_port_enable(struct udevice *dev, u8 port)
+{
+	int val;
+
+	val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL);
+	if (val < 0)
+		return val;
+	val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT,
+			       PORT_REG_CTRL_PSTATE_WIDTH,
+			       PORT_REG_CTRL_PSTATE_FORWARD);
+	val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val);
+	if (val < 0)
+		return val;
+
+	return 0;
+}
+
+static int mv88e61xx_fixed_port_setup(struct udevice *dev, u8 port)
+{
+	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int val;
+
+	val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL);
+	if (val < 0)
+		return val;
+
+	val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK |
+		 PORT_REG_PHYS_CTRL_FC_VALUE |
+		 PORT_REG_PHYS_CTRL_FC_FORCE);
+	val |= PORT_REG_PHYS_CTRL_FC_FORCE |
+	       PORT_REG_PHYS_CTRL_DUPLEX_VALUE |
+	       PORT_REG_PHYS_CTRL_DUPLEX_FORCE;
+
+	if (priv->id == PORT_SWITCH_ID_6071) {
+		val |= PORT_REG_PHYS_CTRL_SPD100;
+	} else {
+		val |= PORT_REG_PHYS_CTRL_PCS_AN_EN |
+		       PORT_REG_PHYS_CTRL_PCS_AN_RST |
+		       PORT_REG_PHYS_CTRL_SPD1000;
+	}
+
+	if (port == dsa_pdata->cpu_port)
+		val |= PORT_REG_PHYS_CTRL_LINK_VALUE |
+		       PORT_REG_PHYS_CTRL_LINK_FORCE;
+
+	return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
+}
+
+static int mv88e61xx_set_cpu_port(struct udevice *dev)
+{
+	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int val;
+
+	/* Set CPUDest */
+	val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MON_CTRL);
+	if (val < 0)
+		return val;
+	val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT,
+			       GLOBAL1_MON_CTRL_CPUDEST_WIDTH,
+			       dsa_pdata->cpu_port);
+	val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val);
+	if (val < 0)
+		return val;
+
+	/* Enable CPU port */
+	val = mv88e61xx_port_enable(dev, dsa_pdata->cpu_port);
+	if (val < 0)
+		return val;
+
+	/* If CPU is connected to serdes, initialize serdes */
+	if (mv88e61xx_6352_family(dev)) {
+		val = mv88e61xx_get_cmode(dev, dsa_pdata->cpu_port);
+		if (val < 0)
+			return val;
+		if (val == PORT_REG_STATUS_CMODE_100BASE_X ||
+		    val == PORT_REG_STATUS_CMODE_1000BASE_X ||
+		    val == PORT_REG_STATUS_CMODE_SGMII) {
+			val = mv88e61xx_serdes_init(dev);
+			if (val < 0)
+				return val;
+		}
+	} else {
+		val = mv88e61xx_fixed_port_setup(dev, dsa_pdata->cpu_port);
+		if (val < 0)
+			return val;
+	}
+
+	return 0;
+}
+
+static int mv88e61xx_switch_init(struct udevice *dev)
+{
+	static int init;
+	int res;
+
+	if (init)
+		return 0;
+
+	res = mv88e61xx_switch_reset(dev);
+	if (res < 0)
+		return res;
+
+	res = mv88e61xx_set_cpu_port(dev);
+	if (res < 0)
+		return res;
+
+	init = 1;
+
+	return 0;
+}
+
+static int mv88e61xx_phy_setup(struct udevice *dev, u8 phy)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int val;
+
+	/*
+	 * Enable energy-detect sensing on PHY, used to determine when a PHY
+	 * port is physically connected
+	 */
+	val = mv88e61xx_phy_read(dev, phy, PHY_REG_CTRL1);
+	if (val < 0)
+		return val;
+	val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift,
+			       priv->phy_ctrl1_en_det_width,
+			       priv->phy_ctrl1_en_det_ctrl);
+	val = mv88e61xx_phy_write(dev, phy, PHY_REG_CTRL1, val);
+	if (val < 0)
+		return val;
+
+	return 0;
+}
+
+/*
+ * This function is used to pre-configure the required register
+ * offsets, so that the indirect register access to the PHY registers
+ * is possible. This is necessary to be able to read the PHY ID
+ * while driver probing or in get_phy_id(). The globalN register
+ * offsets must be initialized correctly for a detected switch,
+ * otherwise detection of the PHY ID won't work!
+ */
+static int mv88e61xx_priv_reg_offs_pre_init(struct udevice *dev)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+
+	/*
+	 * Initial 'port_reg_base' value must be an offset of existing
+	 * port register, then reading the ID should succeed. First, try
+	 * to read via port registers with device address 0x10 (88E6096
+	 * and compatible switches).
+	 */
+	priv->port_reg_base = 0x10;
+	priv->id = mv88e61xx_get_switch_id(dev);
+	if (priv->id != 0xfff0) {
+		priv->global1 = 0x1B;
+		priv->global2 = 0x1C;
+		return 0;
+	}
+
+	/*
+	 * Now try via port registers with device address 0x08
+	 * (88E6020 and compatible switches).
+	 */
+	priv->port_reg_base = 0x08;
+	priv->id = mv88e61xx_get_switch_id(dev);
+	if (priv->id != 0xfff0) {
+		priv->global1 = 0x0F;
+		priv->global2 = 0x07;
+		return 0;
+	}
+
+	debug("%s Unknown ID 0x%x\n", __func__, priv->id);
+	return -ENODEV;
+}
+
+static int mv88e61xx_mdio_read(struct udevice *dev, int addr, int devad, int reg)
+{
+	return mv88e61xx_phy_read_indirect(dev->parent, DEVADDR_PHY(addr),
+					   MDIO_DEVAD_NONE, reg);
+}
+
+static int mv88e61xx_mdio_write(struct udevice *dev, int addr, int devad,
+				int reg, u16 val)
+{
+	return mv88e61xx_phy_write_indirect(dev->parent, DEVADDR_PHY(addr),
+					   MDIO_DEVAD_NONE, reg, val);
+}
+
+static const struct mdio_ops mv88e61xx_mdio_ops = {
+	.read = mv88e61xx_mdio_read,
+	.write = mv88e61xx_mdio_write,
+};
+
+static int mv88e61xx_mdio_bind(struct udevice *dev)
+{
+	char name[32];
+	static int num_devices;
+
+	sprintf(name, "mv88e61xx-mdio-%d", num_devices++);
+	device_set_name(dev, name);
+
+	return 0;
+}
+
+U_BOOT_DRIVER(mv88e61xx_mdio) = {
+	.name		= "mv88e61xx_mdio",
+	.id		= UCLASS_MDIO,
+	.ops		= &mv88e61xx_mdio_ops,
+	.bind		= mv88e61xx_mdio_bind,
+	.priv_auto	= sizeof(struct mv88e61xx_phy_priv),
+	.plat_auto	= sizeof(struct mdio_perdev_priv),
+};
+
+static int mv88e61xx_dsa_setup_cpu_interface(struct udevice *dev)
+{
+	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
+	int port = dsa_pdata->cpu_port;
+	phy_interface_t phy_interface;
+	bool duplex, fixed;
+	ofnode node;
+	int speed;
+	int val;
+
+	fixed = ofnode_phy_is_fixed_link(dsa_pdata->cpu_port_node, &node);
+	if (fixed && ofnode_valid(node)) {
+		speed = ofnode_read_u32_default(node, "speed", 0);
+		duplex = ofnode_read_bool(node, "full-duplex");
+	} else {
+		duplex = false;
+		speed = 0;
+	}
+	phy_interface = phy_get_interface_by_name(ofnode_read_string(dsa_pdata->cpu_port_node,
+								     "phy-mode"));
+
+	dev_dbg(dev, "%s configuring CPU port P%d for %s fixed=%d speed=%d duplex=%d\n",
+		__func__, port, phy_string_for_interface(phy_interface),
+		fixed, speed, duplex);
+
+	/* Physical Control register: Table 62 */
+	val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL);
+	val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK ||
+		 PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK);
+	if (phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
+		val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK;
+	if (phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
+		val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK;
+	if (fixed)
+		val |= BIT(5) | BIT(4); /* Force link */
+	switch (speed) {
+	case SPEED_10:
+		break;
+	case SPEED_100:
+		val |= 0x1;
+		break;
+	case SPEED_1000:
+		val |= 0x2;
+		break;
+	default:
+		val |= 0x3; /* detect link speed */
+		break;
+	}
+	if (duplex)
+		val |= BIT(3) | BIT(2);
+
+	return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
+}
+
+static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, struct phy_device *phy)
+{
+	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	dev_dbg(dev, "%s P%d phy:0x%08x %s\n", __func__, port,
+		phy->phy_id, phy_string_for_interface(phy->interface));
+
+	ret = mv88e61xx_phy_setup(dev, port);
+	if (ret < 0) {
+		dev_err(dev, "Error setting up PHY %i\n", port);
+		return ret;
+	}
+	ret = mv88e61xx_port_enable(dev, port);
+	if (ret < 0) {
+		dev_err(dev, "Error enabling PHY %i\n", port);
+		return ret;
+	}
+	if (port != dsa_pdata->cpu_port)
+		priv->active_port = port;
+	else
+		mv88e61xx_set_cpu_port(dev);
+
+	return 0;
+}
+
+static void mv88e61xx_dsa_port_disable(struct udevice *dev, int port, struct phy_device *phy)
+{
+	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
+	int val;
+
+	dev_dbg(dev, "%s P%d phy:0x%x\n", __func__, port, phy->phy_id);
+
+	/* can't disable CPU port without re-configuring/re-starting switch */
+	if (port == dsa_pdata->cpu_port)
+		return;
+
+	val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL);
+	if (val < 0)
+		return;
+	val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT,
+			       PORT_REG_CTRL_PSTATE_WIDTH,
+			       PORT_REG_CTRL_PSTATE_DISABLED);
+	val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val);
+	if (val < 0)
+		return;
+}
+
+static int mv88e61xx_dsa_xmit(struct udevice *dev, int port, void *packet, int len)
+{
+	return 0;
+}
+
+static int mv88e61xx_dsa_recv(struct udevice *dev, int *port, void *packet, int len)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+
+	*port = priv->active_port;
+
+	return 0;
+}
+
+static const struct dsa_ops mv88e61xx_dsa_ops = {
+	.port_enable = mv88e61xx_dsa_port_enable,
+	.port_disable = mv88e61xx_dsa_port_disable,
+	.xmit = mv88e61xx_dsa_xmit,
+	.rcv = mv88e61xx_dsa_recv,
+};
+
+/* bind and probe the switch mdios */
+static int mv88e61xx_dsa_probe_mdio(struct udevice *dev)
+{
+	struct udevice *pdev;
+	ofnode node, mdios;
+	const char *name;
+	int ret;
+
+	/* bind phy ports of mdios child node to mv88e61xx_mdio device */
+	mdios = dev_read_subnode(dev, "mdios");
+	if (ofnode_valid(mdios)) {
+		ofnode_for_each_subnode(node, mdios) {
+			name = ofnode_get_name(node);
+			ret = device_bind_driver_to_node(dev,
+							 "mv88e61xx_mdio",
+							 name, node, &pdev);
+			if (ret) {
+				dev_err(dev, "failed to bind %s: %d\n", name, ret);
+				continue;
+			}
+
+			/* need to probe it as there is no compatible to do so */
+			ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &pdev);
+			if (ret) {
+				dev_err(dev, "failed to probe %s: %d\n", name, ret);
+				continue;
+			}
+		}
+	}
+
+	/* bind the mdios node to the parent mdio driver */
+	name = ofnode_get_name(mdios);
+	ret = device_bind_driver_to_node(dev,
+					 "mv88e61xx_mdio",
+					 name, mdios, &pdev);
+	if (ret)
+		dev_err(dev, "failed to bind %s: %d\n", name, ret);
+
+	/* need to probe it as there is no compatible to do so */
+	ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdios, &pdev);
+	if (ret)
+		dev_err(dev, "failed to probe %s: %d\n", name, ret);
+
+	return ret;
+}
+
+static int mv88e61xx_dsa_probe(struct udevice *dev)
+{
+	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
+	struct udevice *master = dsa_get_master(dev);
+	int ret;
+
+	if (!master)
+		return -ENODEV;
+
+	dev_dbg(dev, "%s master:%s\n", __func__, master->name);
+	dev_set_parent_priv(dev, priv);
+
+	/* probe internal mdio bus */
+	ret = mv88e61xx_dsa_probe_mdio(dev);
+	if (ret)
+		return ret;
+
+	ret = mv88e61xx_priv_reg_offs_pre_init(dev);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "ID=0x%x PORT_BASE=0x%02x GLOBAL1=0x%02x GLOBAL2=0x%02x\n",
+		priv->id, priv->port_reg_base, priv->global1, priv->global2);
+	switch (priv->id) {
+	case PORT_SWITCH_ID_6096:
+	case PORT_SWITCH_ID_6097:
+	case PORT_SWITCH_ID_6172:
+	case PORT_SWITCH_ID_6176:
+	case PORT_SWITCH_ID_6240:
+	case PORT_SWITCH_ID_6352:
+		priv->port_count = 11;
+//		priv->port_stat_link_mask = BIT(11);
+//		priv->port_stat_dup_mask = BIT(10);
+//		priv->port_stat_speed_width = 2;
+		priv->phy_ctrl1_en_det_shift = 8;
+		priv->phy_ctrl1_en_det_width = 2;
+		priv->phy_ctrl1_en_det_ctrl =
+			PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT;
+		break;
+	case PORT_SWITCH_ID_6020:
+	case PORT_SWITCH_ID_6070:
+	case PORT_SWITCH_ID_6071:
+	case PORT_SWITCH_ID_6220:
+	case PORT_SWITCH_ID_6250:
+		priv->port_count = 7;
+//		priv->port_stat_link_mask = BIT(12);
+//		priv->port_stat_dup_mask = BIT(9);
+//		priv->port_stat_speed_width = 1;
+		priv->phy_ctrl1_en_det_shift = 14;
+		priv->phy_ctrl1_en_det_width = 1;
+		priv->phy_ctrl1_en_det_ctrl =
+			PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	/* configure CPU port interface mode */
+	ret = mv88e61xx_dsa_setup_cpu_interface(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = mv88e61xx_switch_init(dev);
+	if (ret < 0)
+		return ret;
+
+	dsa_set_tagging(dev, 0, 0);
+
+	return 0;
+}
+
+static const struct udevice_id mv88e61xx_ids[] = {
+	{ .compatible = "marvell,mv88e6085" },
+	{ }
+};
+
+U_BOOT_DRIVER(mv88e61xx) = {
+	.name		= "mv88e61xx",
+	.id		= UCLASS_DSA,
+	.of_match	= mv88e61xx_ids,
+	.probe		= mv88e61xx_dsa_probe,
+	.ops		= &mv88e61xx_dsa_ops,
+	.priv_auto	= sizeof(struct mv88e61xx_phy_priv),
+};
-- 
2.17.1

[PATCH 6/6] board: gw_ventana: enable MV88E61XX DSA support

[Tim Harvey]

Add MV88E61XX DSA support:
 - update dt: U-Boot dsa driver requires different device-tree syntax
   than the linux driver in order to link the dsa ports to the mdio bus.
 - update defconfig
 - replace mv88e61xx_hw_reset weak override with board_phy_config support
   for mv88e61xx configuration that is outside the scope of the DSA driver

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/dts/imx6qdl-gw5904.dtsi        | 35 +++++++++++++++++
 board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++----------------
 configs/gwventana_gw5904_defconfig      |  7 ++--
 3 files changed, 56 insertions(+), 36 deletions(-)

diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi
index 286c7a9924c2..63590a2debc7 100644
--- a/arch/arm/dts/imx6qdl-gw5904.dtsi
+++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
@@ -219,6 +219,27 @@
 			compatible = "marvell,mv88e6085";
 			reg = <0>;
 
+			mdios {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				sw_phy0: ethernet-phy@0 {
+					reg = <0x0>;
+				};
+
+				sw_phy1: ethernet-phy@1 {
+					reg = <0x1>;
+				};
+
+				sw_phy2: ethernet-phy@2 {
+					reg = <0x2>;
+				};
+
+				sw_phy3: ethernet-phy@3 {
+					reg = <0x3>;
+				};
+			};
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -226,27 +247,41 @@
 				port@0 {
 					reg = <0>;
 					label = "lan4";
+					phy-mode = "internal";
+					phy-handle = <&sw_phy0>;
 				};
 
 				port@1 {
 					reg = <1>;
 					label = "lan3";
+					phy-mode = "internal";
+					phy-handle = <&sw_phy1>;
 				};
 
 				port@2 {
 					reg = <2>;
 					label = "lan2";
+					phy-mode = "internal";
+					phy-handle = <&sw_phy2>;
 				};
 
 				port@3 {
 					reg = <3>;
 					label = "lan1";
+					phy-mode = "internal";
+					phy-handle = <&sw_phy3>;
 				};
 
 				port@5 {
 					reg = <5>;
 					label = "cpu";
 					ethernet = <&fec>;
+					phy-mode = "rgmii-id";
+
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+					};
 				};
 			};
 		};
diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c
index 8cf791467024..fed86db14847 100644
--- a/board/gateworks/gw_ventana/gw_ventana.c
+++ b/board/gateworks/gw_ventana/gw_ventana.c
@@ -79,44 +79,30 @@ int board_phy_config(struct phy_device *phydev)
 		phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
 	}
 
+	/* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
+	else if (phydev->phy_id == 0xa5a55a5a &&
+		 ((board_type == GW5904) || (board_type == GW5909))) {
+		struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
+
+		puts("MV88E61XX ");
+		/* GPIO[0] output CLK125 for RGMII_REFCLK */
+		bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe);
+		bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7);
+
+		/* Port 0-3 LED configuration: Table 80/82 */
+		/* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
+		bus->write(bus, 0x10, 0, 0x16, 0x8088);
+		bus->write(bus, 0x11, 0, 0x16, 0x8088);
+		bus->write(bus, 0x12, 0, 0x16, 0x8088);
+		bus->write(bus, 0x13, 0, 0x16, 0x8088);
+	}
+
 	if (phydev->drv->config)
 		phydev->drv->config(phydev);
 
 	return 0;
 }
 
-#ifdef CONFIG_MV88E61XX_SWITCH
-int mv88e61xx_hw_reset(struct phy_device *phydev)
-{
-	struct mii_dev *bus = phydev->bus;
-
-	/* GPIO[0] output, CLK125 */
-	debug("enabling RGMII_REFCLK\n");
-	bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
-		   0x1a /*MV_SCRATCH_MISC*/,
-		   (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe);
-	bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
-		   0x1a /*MV_SCRATCH_MISC*/,
-		   (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7);
-
-	/* RGMII delay - Physical Control register bit[15:14] */
-	debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT);
-	/* forced 1000mbps full-duplex link */
-	bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe);
-	phydev->autoneg = AUTONEG_DISABLE;
-	phydev->speed = SPEED_1000;
-	phydev->duplex = DUPLEX_FULL;
-
-	/* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
-	bus->write(bus, 0x10, 0, 0x16, 0x8088);
-	bus->write(bus, 0x11, 0, 0x16, 0x8088);
-	bus->write(bus, 0x12, 0, 0x16, 0x8088);
-	bus->write(bus, 0x13, 0, 0x16, 0x8088);
-
-	return 0;
-}
-#endif // CONFIG_MV88E61XX_SWITCH
-
 #if defined(CONFIG_VIDEO_IPUV3)
 static void enable_hdmi(struct display_info_t const *dev)
 {
diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig
index 93fd748eabac..94c1ec34e7b6 100644
--- a/configs/gwventana_gw5904_defconfig
+++ b/configs/gwventana_gw5904_defconfig
@@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_FSL_USDHC=y
 CONFIG_MTD=y
 CONFIG_PHYLIB=y
-CONFIG_MV88E61XX_SWITCH=y
-CONFIG_MV88E61XX_CPU_PORT=5
-CONFIG_MV88E61XX_PHY_PORTS=0xf
-CONFIG_MV88E61XX_FIXED_PORTS=0x0
+CONFIG_MV88E61XX=y
+CONFIG_PHY_FIXED=y
 CONFIG_DM_ETH=y
 CONFIG_DM_MDIO=y
+CONFIG_DM_DSA=y
 CONFIG_E1000=y
 CONFIG_FEC_MXC=y
 CONFIG_MII=y
-- 
2.17.1

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Marek Behún]

On Tue, 29 Mar 2022 15:52:39 -0700
Tim Harvey <tharvey@gateworks.com> wrote:

> Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

Is this final version that should be accepted?

The drivers seems to support not only 61xx, but 6xxx (6096, 6250, 6352,
...).

Also there are some commented lines, for example

> +//	u16 port_stat_link_mask;/* Bitmask for port link status bits */
> +//	u16 port_stat_dup_mask; /* Bitmask for port duplex status bits */
> +//	u8 port_stat_speed_width;/* Width of speed status bitfield */

What is their purpose?

Why is mv88e61xx_dsa_xmit() no-op?

Marek

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Tim Harvey]

On Tue, Mar 29, 2022 at 4:22 PM Marek Behún <marek.behun@nic.cz> wrote:
>
> On Tue, 29 Mar 2022 15:52:39 -0700
> Tim Harvey <tharvey@gateworks.com> wrote:
>
> > Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>

Marek,

Thanks for looking at this.

> Is this final version that should be accepted?

This is the first submission of the new driver.

>
> The drivers seems to support not only 61xx, but 6xxx (6096, 6250, 6352,
> ...).

It should also support all the same devices that the non dsa driver it
was derived from (drivers/net/phy/mv88e61xx.c) but I only have a board
with 88E6176 to test with.

It is not completely clear to me what devices are supported in the
original driver per Kconfig but from looking over the code I would say
the following appear to be supported:
mv88e6096
mv88e6097
mv88e6020
mv88e6070
mv88e6071
mv88e6172
mv88e6176
mv88e6220
mv88e6240
mv88e6250
mv88e6352

>
> Also there are some commented lines, for example
>
> > +//   u16 port_stat_link_mask;/* Bitmask for port link status bits */
> > +//   u16 port_stat_dup_mask; /* Bitmask for port duplex status bits */
> > +//   u8 port_stat_speed_width;/* Width of speed status bitfield */
>
> What is their purpose?
>

oops... that was a mistake. I'm surprised checkpatch didn't catch
those. Those were in the non-dsa driver and not used here so will be
removed.

> Why is mv88e61xx_dsa_xmit() no-op?

For DSA dsa-uclass calls the switch master eth device send function
after calling the dsa_ops->xmit function so that a dsa driver can add
any header/footer if needed. The function is required but in my case I
don't care about header/footer tagging or vlan as only 1 port is
active at a time in U-Boot so I just return success.

Best Regards,

Tim

Re: [PATCH 0/6] Add MV88E61xx DSA driver and use on gwventana

[Tim Harvey]

On Tue, Mar 29, 2022 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> This series adds a DSA driver for the MV88E61xx based on
> drivers/net/phy/mv88e61xx and uses in on the gwventana_gw5904_defconfig.
>
> The hope is that the other three boards that use the MV88E61xx driver
> can move to this as well eventually so that we can remove the non-dm
> driver and the 4 Kconfig options it requires.
>
> The MV88E61xx has an MDIO interface this DM_MDIO must be used so support
> for a UCLASS_MDIO driver is added to the fec_mxc ethernet driver in a
> way that allows a fallback to the previous non DM_MDIO case as there are
> many boards out there using this driver that define DM_MDIO but do not
> have the required dt props for a DM_MDIO driver which would cause a
> regression.
>
> Additionally a patch to ensure MDIO children are scanned on post-bind is
> needed.
>
> I'm making use of the dm_mdio_read/dm_mdio_write wrapper from Merak that
> is pending so it is in this series as well.
>
> Best Regards,
>
> Tim
>
> Tim Harvey (6):
>   net: mdio-uclass: scan for dm mdio children on post-bind
>   net: dsa: move cpu port probe to dsa_post_probe
>   net: mdio-uclass: add wrappers for read/write/reset operations
>   net: fec: add support for DM_MDIO
>   net: add MV88E61xx DSA driver
>   board: gw_ventana: enable MV88E61XX DSA support
>
>  arch/arm/dts/imx6qdl-gw5904.dtsi        |  35 +
>  board/gateworks/gw_ventana/gw_ventana.c |  50 +-
>  configs/gwventana_gw5904_defconfig      |   7 +-
>  drivers/net/Kconfig                     |   7 +
>  drivers/net/Makefile                    |   1 +
>  drivers/net/fec_mxc.c                   | 113 ++-
>  drivers/net/fec_mxc.h                   |   1 +
>  drivers/net/mv88e61xx.c                 | 982 ++++++++++++++++++++++++
>  include/miiphy.h                        |  31 +
>  net/dsa-uclass.c                        |  14 +-
>  net/mdio-uclass.c                       |  35 +
>  11 files changed, 1227 insertions(+), 49 deletions(-)
>  create mode 100644 drivers/net/mv88e61xx.c
>
> --
> 2.17.1
>

Adding Chris and Anotolij to the thread as the maintainers of the only
other boards using drivers/net/phy/mv88e61xx.c. If they can switch to
the DSA driver it would allow us to remove the static Kconfig options
to configure the switch which should be present in the device-tree.

Best Regards,

Tim

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Marek Behún]

On Wed, 30 Mar 2022 08:46:06 -0700
Tim Harvey <tharvey@gateworks.com> wrote:

> On Tue, Mar 29, 2022 at 4:22 PM Marek Behún <marek.behun@nic.cz> wrote:
> >
> > On Tue, 29 Mar 2022 15:52:39 -0700
> > Tim Harvey <tharvey@gateworks.com> wrote:
> >  
> > > Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
> > >
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>  
> >  
> 
> Marek,
> 
> Thanks for looking at this.
> 
> > Is this final version that should be accepted?  
> 
> This is the first submission of the new driver.
> 
> >
> > The drivers seems to support not only 61xx, but 6xxx (6096, 6250, 6352,
> > ...).  
> 
> It should also support all the same devices that the non dsa driver it
> was derived from (drivers/net/phy/mv88e61xx.c) but I only have a board
> with 88E6176 to test with.
> 
> It is not completely clear to me what devices are supported in the
> original driver per Kconfig but from looking over the code I would say
> the following appear to be supported:
> mv88e6096
> mv88e6097
> mv88e6020
> mv88e6070
> mv88e6071
> mv88e6172
> mv88e6176
> mv88e6220
> mv88e6240
> mv88e6250
> mv88e6352
> 
> >
> > Also there are some commented lines, for example
> >  
> > > +//   u16 port_stat_link_mask;/* Bitmask for port link status bits */
> > > +//   u16 port_stat_dup_mask; /* Bitmask for port duplex status bits */
> > > +//   u8 port_stat_speed_width;/* Width of speed status bitfield */  
> >
> > What is their purpose?
> >  
> 
> oops... that was a mistake. I'm surprised checkpatch didn't catch
> those. Those were in the non-dsa driver and not used here so will be
> removed.
> 
> > Why is mv88e61xx_dsa_xmit() no-op?  
> 
> For DSA dsa-uclass calls the switch master eth device send function
> after calling the dsa_ops->xmit function so that a dsa driver can add
> any header/footer if needed. The function is required but in my case I
> don't care about header/footer tagging or vlan as only 1 port is
> active at a time in U-Boot so I just return success.

So if I make one port active, the other are completely disabled? They
won't even switch? Is that how DSA uclass is supposed to work in U-Boot?

I would think that it should be somehow configurable instead.

Marek

Re: [PATCH 4/6] net: fec: add support for DM_MDIO

[Vladimir Oltean]

On Tue, Mar 29, 2022 at 03:52:38PM -0700, Tim Harvey wrote:
> Add support for DM_MDIO by registering a UCLASS_MDIO driver and
> attempting to use it. This is necessary if wanting to use a DSA
> driver for example hanging off of the FEC MAC.
> 
> Care is taken to fallback to non DM_MDIO as several boards define
> DM_MDIO without having the proper device-tree configuration necessary
> such as an mdio subnode, a phy-mode prop, and either a valid phy-handle
> prop or fixed-phy subnode.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/net/fec_mxc.c | 113 ++++++++++++++++++++++++++++++++++++++----
>  drivers/net/fec_mxc.h |   1 +
>  2 files changed, 104 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index e8ebef09032a..374ef9d67d5e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -30,6 +30,8 @@
>  #include <asm/arch/imx-regs.h>
>  #include <asm/mach-imx/sys_proto.h>
>  #include <asm-generic/gpio.h>
> +#include <dm/device_compat.h>
> +#include <dm/lists.h>
>  
>  #include "fec_mxc.h"
>  #include <eth_phy.h>
> @@ -1025,6 +1027,80 @@ struct mii_dev *fec_get_miibus(ulong base_addr, int dev_id)
>  	return bus;
>  }
>  
> +#ifdef CONFIG_DM_MDIO
> +struct dm_fec_mdio_priv {
> +	struct ethernet_regs *regs;
> +};
> +
> +static int dm_fec_mdio_read(struct udevice *dev, int addr, int devad, int reg)
> +{
> +	struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
> +
> +	return fec_mdio_read(priv->regs, addr, reg);
> +}
> +
> +static int dm_fec_mdio_write(struct udevice *dev, int addr, int devad, int reg, u16 data)
> +{
> +	struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
> +
> +	return fec_mdio_write(priv->regs, addr, reg, data);
> +}
> +
> +static const struct mdio_ops dm_fec_mdio_ops = {
> +	.read = dm_fec_mdio_read,
> +	.write = dm_fec_mdio_write,
> +};
> +
> +static int dm_fec_mdio_probe(struct udevice *dev)
> +{
> +	struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
> +
> +	priv->regs = (struct ethernet_regs *)ofnode_get_addr(dev_ofnode(dev->parent));
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(fec_mdio) = {
> +	.name		= "fec_mdio",
> +	.id		= UCLASS_MDIO,
> +	.probe		= dm_fec_mdio_probe,
> +	.ops		= &dm_fec_mdio_ops,
> +	.priv_auto	= sizeof(struct dm_fec_mdio_priv),
> +};
> +
> +static int dm_fec_bind_mdio(struct udevice *dev)
> +{
> +	struct udevice *mdiodev;
> +	const char *name;
> +	ofnode mdio;
> +	int ret;
> +
> +	/* for a UCLASS_MDIO driver we need to bind and probe manually
> +	 * for an internal MDIO bus that has no dt compatible of its own
> +	 */
> +	ofnode_for_each_subnode(mdio, dev_ofnode(dev)) {
> +		name = ofnode_get_name(mdio);
> +
> +		if (strcmp(name, "mdio"))
> +			continue;
> +
> +		ret = device_bind_driver_to_node(dev, "fec_mdio",
> +						 name, mdio, &mdiodev);
> +		if (ret)
> +			return ret;
> +
> +		/* need to probe it as there is no compatible to do so */
> +		ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdiodev);
> +		if (!ret)
> +			return 0;
> +
> +		break;
> +	}
> +
> +	return ret;
> +}
> +#endif
> +

On which branch does this apply? The context above fecmxc_read_rom_hwaddr()
is different in the branches I've checked:
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/fec_mxc.c#L1276
https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/fec_mxc.c#L1276

>  static int fecmxc_read_rom_hwaddr(struct udevice *dev)
>  {
>  	struct fec_priv *priv = dev_get_priv(dev);
> @@ -1088,7 +1164,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev)
>  
>  static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
>  {
> -	struct phy_device *phydev;
> +	struct phy_device *phydev = NULL;
>  	int addr;
>  
>  	addr = device_get_phy_addr(priv, dev);
> @@ -1096,7 +1172,12 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
>  	addr = CONFIG_FEC_MXC_PHYADDR;
>  #endif
>  
> -	phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> +#ifdef CONFIG_DM_MDIO
> +	if (priv->dm_mdio)
> +		phydev = dm_eth_phy_connect(dev);
> +#endif
> +	if (!phydev)
> +		phydev = phy_connect(priv->bus, addr, dev, priv->interface);
>  	if (!phydev)
>  		return -ENODEV;
>  
> @@ -1227,11 +1308,19 @@ static int fecmxc_probe(struct udevice *dev)
>  
>  	priv->dev_id = dev_seq(dev);
>  
> +#ifdef CONFIG_DM_MDIO
> +	ret = dm_fec_bind_mdio(dev);
> +	if (!ret) {
> +		ret = fec_phy_init(priv, dev);
> +		if (!ret)
> +			priv->dm_mdio = true;
> +	}
> +#endif
>  #ifdef CONFIG_DM_ETH_PHY
>  	bus = eth_phy_get_mdio_bus(dev);
>  #endif
>  
> -	if (!bus) {
> +	if (!bus && !priv->dm_mdio) {
>  		dm_mii_bus = false;
>  #ifdef CONFIG_FEC_MXC_MDIO_BASE
>  		bus = fec_get_miibus((ulong)CONFIG_FEC_MXC_MDIO_BASE,
> @@ -1240,7 +1329,7 @@ static int fecmxc_probe(struct udevice *dev)
>  		bus = fec_get_miibus((ulong)priv->eth, dev_seq(dev));
>  #endif
>  	}
> -	if (!bus) {
> +	if (!bus && !priv->dm_mdio) {
>  		ret = -ENOMEM;
>  		goto err_mii;
>  	}
> @@ -1271,14 +1360,16 @@ static int fecmxc_probe(struct udevice *dev)
>  		break;
>  	}
>  
> -	ret = fec_phy_init(priv, dev);
> -	if (ret)
> -		goto err_phy;
> +	if (!priv->dm_mdio) {
> +		ret = fec_phy_init(priv, dev);
> +		if (ret)
> +			goto err_phy;
> +	}
>  
>  	return 0;
>  
>  err_phy:
> -	if (!dm_mii_bus) {
> +	if (!dm_mii_bus && !priv->dm_mdio) {
>  		mdio_unregister(bus);
>  		free(bus);
>  	}
> @@ -1294,8 +1385,10 @@ static int fecmxc_remove(struct udevice *dev)
>  
>  	free(priv->phydev);
>  	fec_free_descs(priv);
> -	mdio_unregister(priv->bus);
> -	mdio_free(priv->bus);
> +	if (priv->bus) {
> +		mdio_unregister(priv->bus);
> +		mdio_free(priv->bus);
> +	}
>  
>  #ifdef CONFIG_DM_REGULATOR
>  	if (priv->phy_supply)
> diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
> index 48faa33d66ec..c297880ccc54 100644
> --- a/drivers/net/fec_mxc.h
> +++ b/drivers/net/fec_mxc.h
> @@ -248,6 +248,7 @@ struct fec_priv {
>  	uint8_t *tdb_ptr;
>  	int dev_id;
>  	struct mii_dev *bus;
> +	bool dm_mdio;
>  #ifdef CONFIG_PHYLIB
>  	struct phy_device *phydev;
>  	ofnode phy_of_node;
> -- 
> 2.17.1
>

Re: [PATCH 4/6] net: fec: add support for DM_MDIO

[Tim Harvey]

On Thu, Mar 31, 2022 at 10:01 AM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> On Tue, Mar 29, 2022 at 03:52:38PM -0700, Tim Harvey wrote:
> > Add support for DM_MDIO by registering a UCLASS_MDIO driver and
> > attempting to use it. This is necessary if wanting to use a DSA
> > driver for example hanging off of the FEC MAC.
> >
> > Care is taken to fallback to non DM_MDIO as several boards define
> > DM_MDIO without having the proper device-tree configuration necessary
> > such as an mdio subnode, a phy-mode prop, and either a valid phy-handle
> > prop or fixed-phy subnode.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  drivers/net/fec_mxc.c | 113 ++++++++++++++++++++++++++++++++++++++----
> >  drivers/net/fec_mxc.h |   1 +
> >  2 files changed, 104 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> > index e8ebef09032a..374ef9d67d5e 100644
> > --- a/drivers/net/fec_mxc.c
> > +++ b/drivers/net/fec_mxc.c
> > @@ -30,6 +30,8 @@
> >  #include <asm/arch/imx-regs.h>
> >  #include <asm/mach-imx/sys_proto.h>
> >  #include <asm-generic/gpio.h>
> > +#include <dm/device_compat.h>
> > +#include <dm/lists.h>
> >
> >  #include "fec_mxc.h"
> >  #include <eth_phy.h>
> > @@ -1025,6 +1027,80 @@ struct mii_dev *fec_get_miibus(ulong base_addr, int dev_id)
> >       return bus;
> >  }
> >
> > +#ifdef CONFIG_DM_MDIO
> > +struct dm_fec_mdio_priv {
> > +     struct ethernet_regs *regs;
> > +};
> > +
> > +static int dm_fec_mdio_read(struct udevice *dev, int addr, int devad, int reg)
> > +{
> > +     struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
> > +
> > +     return fec_mdio_read(priv->regs, addr, reg);
> > +}
> > +
> > +static int dm_fec_mdio_write(struct udevice *dev, int addr, int devad, int reg, u16 data)
> > +{
> > +     struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
> > +
> > +     return fec_mdio_write(priv->regs, addr, reg, data);
> > +}
> > +
> > +static const struct mdio_ops dm_fec_mdio_ops = {
> > +     .read = dm_fec_mdio_read,
> > +     .write = dm_fec_mdio_write,
> > +};
> > +
> > +static int dm_fec_mdio_probe(struct udevice *dev)
> > +{
> > +     struct dm_fec_mdio_priv *priv = dev_get_priv(dev);
> > +
> > +     priv->regs = (struct ethernet_regs *)ofnode_get_addr(dev_ofnode(dev->parent));
> > +
> > +     return 0;
> > +}
> > +
> > +U_BOOT_DRIVER(fec_mdio) = {
> > +     .name           = "fec_mdio",
> > +     .id             = UCLASS_MDIO,
> > +     .probe          = dm_fec_mdio_probe,
> > +     .ops            = &dm_fec_mdio_ops,
> > +     .priv_auto      = sizeof(struct dm_fec_mdio_priv),
> > +};
> > +
> > +static int dm_fec_bind_mdio(struct udevice *dev)
> > +{
> > +     struct udevice *mdiodev;
> > +     const char *name;
> > +     ofnode mdio;
> > +     int ret;
> > +
> > +     /* for a UCLASS_MDIO driver we need to bind and probe manually
> > +      * for an internal MDIO bus that has no dt compatible of its own
> > +      */
> > +     ofnode_for_each_subnode(mdio, dev_ofnode(dev)) {
> > +             name = ofnode_get_name(mdio);
> > +
> > +             if (strcmp(name, "mdio"))
> > +                     continue;
> > +
> > +             ret = device_bind_driver_to_node(dev, "fec_mdio",
> > +                                              name, mdio, &mdiodev);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             /* need to probe it as there is no compatible to do so */
> > +             ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdiodev);
> > +             if (!ret)
> > +                     return 0;
> > +
> > +             break;
> > +     }
> > +
> > +     return ret;
> > +}
> > +#endif
> > +
>
> On which branch does this apply? The context above fecmxc_read_rom_hwaddr()
> is different in the branches I've checked:
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/fec_mxc.c#L1276
> https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/fec_mxc.c#L1276
>

Sorry, I should have specified in my cover letter that this is on top
of next which removes the non dm-eth support from the driver which is
quite the cleanup.

Also, after more testing I believe the dm-mdio driver code 'above' is
correct but the business of trying to use it and fallback 'below' is
wrong and needs work. It doesn't break current users of fec_mxc from
what I can tell but it also doesn't properly connect the dm_mdio
driver.

> >  static int fecmxc_read_rom_hwaddr(struct udevice *dev)
> >  {
> >       struct fec_priv *priv = dev_get_priv(dev);
> > @@ -1088,7 +1164,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev)
> >
> >  static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
> >  {
> > -     struct phy_device *phydev;
> > +     struct phy_device *phydev = NULL;
> >       int addr;
> >
> >       addr = device_get_phy_addr(priv, dev);
> > @@ -1096,7 +1172,12 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
> >       addr = CONFIG_FEC_MXC_PHYADDR;
> >  #endif
> >
> > -     phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> > +#ifdef CONFIG_DM_MDIO
> > +     if (priv->dm_mdio)
> > +             phydev = dm_eth_phy_connect(dev);
> > +#endif
> > +     if (!phydev)
> > +             phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> >       if (!phydev)
> >               return -ENODEV;
> >
> > @@ -1227,11 +1308,19 @@ static int fecmxc_probe(struct udevice *dev)
> >
> >       priv->dev_id = dev_seq(dev);
> >
> > +#ifdef CONFIG_DM_MDIO
> > +     ret = dm_fec_bind_mdio(dev);
> > +     if (!ret) {
> > +             ret = fec_phy_init(priv, dev);
> > +             if (!ret)
> > +                     priv->dm_mdio = true;
> > +     }
> > +#endif
> >  #ifdef CONFIG_DM_ETH_PHY
> >       bus = eth_phy_get_mdio_bus(dev);
> >  #endif
> >
> > -     if (!bus) {
> > +     if (!bus && !priv->dm_mdio) {
> >               dm_mii_bus = false;
> >  #ifdef CONFIG_FEC_MXC_MDIO_BASE
> >               bus = fec_get_miibus((ulong)CONFIG_FEC_MXC_MDIO_BASE,
> > @@ -1240,7 +1329,7 @@ static int fecmxc_probe(struct udevice *dev)
> >               bus = fec_get_miibus((ulong)priv->eth, dev_seq(dev));
> >  #endif
> >       }
> > -     if (!bus) {
> > +     if (!bus && !priv->dm_mdio) {
> >               ret = -ENOMEM;
> >               goto err_mii;
> >       }
> > @@ -1271,14 +1360,16 @@ static int fecmxc_probe(struct udevice *dev)
> >               break;
> >       }
> >
> > -     ret = fec_phy_init(priv, dev);
> > -     if (ret)
> > -             goto err_phy;
> > +     if (!priv->dm_mdio) {
> > +             ret = fec_phy_init(priv, dev);
> > +             if (ret)
> > +                     goto err_phy;
> > +     }
> >
> >       return 0;
> >
> >  err_phy:
> > -     if (!dm_mii_bus) {
> > +     if (!dm_mii_bus && !priv->dm_mdio) {
> >               mdio_unregister(bus);
> >               free(bus);
> >       }
> > @@ -1294,8 +1385,10 @@ static int fecmxc_remove(struct udevice *dev)
> >
> >       free(priv->phydev);
> >       fec_free_descs(priv);
> > -     mdio_unregister(priv->bus);
> > -     mdio_free(priv->bus);
> > +     if (priv->bus) {
> > +             mdio_unregister(priv->bus);
> > +             mdio_free(priv->bus);
> > +     }
> >
> >  #ifdef CONFIG_DM_REGULATOR
> >       if (priv->phy_supply)
> > diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
> > index 48faa33d66ec..c297880ccc54 100644
> > --- a/drivers/net/fec_mxc.h
> > +++ b/drivers/net/fec_mxc.h
> > @@ -248,6 +248,7 @@ struct fec_priv {
> >       uint8_t *tdb_ptr;
> >       int dev_id;
> >       struct mii_dev *bus;
> > +     bool dm_mdio;
> >  #ifdef CONFIG_PHYLIB
> >       struct phy_device *phydev;
> >       ofnode phy_of_node;
> > --
> > 2.17.1
> >

What I'm trying to accomplish here vs just simply using
dm_fec_bind_mdio(dev) and dm_eth_phy_connect(dev) is to provide a
fallback for the many users of fec_mxc that do not have a dt that
works with dm-mdio yet still have defconfig's that enable DM_MDIO.
Most of the users would not currently have an mdio subnode in the fec
node, nor have the required phy-mode 'and' phy-handle prop or
fixed-link subnode which would cause the dm_eth_phy_connect to fail.

Best Regards,

Tim

Re: [PATCH 4/6] net: fec: add support for DM_MDIO

[Vladimir Oltean]

On Thu, Mar 31, 2022 at 10:48:55AM -0700, Tim Harvey wrote:
> > On which branch does this apply? The context above fecmxc_read_rom_hwaddr()
> > is different in the branches I've checked:
> > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/fec_mxc.c#L1276
> > https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/fec_mxc.c#L1276
> >
> 
> Sorry, I should have specified in my cover letter that this is on top
> of next which removes the non dm-eth support from the driver which is
> quite the cleanup.

Ok, but now the next patch fails to apply to the 'next' branch:

Applying: net: dsa: move cpu port probe to dsa_post_probe
Applying: net: mdio-uclass: add wrappers for read/write/reset operations
Applying: net: fec: add support for DM_MDIO
Applying: net: add MV88E61xx DSA driver
error: patch failed: drivers/net/Kconfig:428
error: drivers/net/Kconfig: patch does not apply
error: patch failed: drivers/net/Makefile:66
error: drivers/net/Makefile: patch does not apply
Patch failed at 0005 net: add MV88E61xx DSA driver

> Also, after more testing I believe the dm-mdio driver code 'above' is
> correct but the business of trying to use it and fallback 'below' is
> wrong and needs work. It doesn't break current users of fec_mxc from
> what I can tell but it also doesn't properly connect the dm_mdio
> driver.

Can you spell out what is wrong about the fallback logic?

The whole thing with eth_phy_get_mdio_bus()/eth_phy_set_mdio_bus() has
me so confused that I am not really following along anymore.

> > >  static int fecmxc_read_rom_hwaddr(struct udevice *dev)
> > >  {
> > >       struct fec_priv *priv = dev_get_priv(dev);
> > > @@ -1088,7 +1164,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev)
> > >
> > >  static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
> > >  {
> > > -     struct phy_device *phydev;
> > > +     struct phy_device *phydev = NULL;
> > >       int addr;
> > >
> > >       addr = device_get_phy_addr(priv, dev);
> > > @@ -1096,7 +1172,12 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
> > >       addr = CONFIG_FEC_MXC_PHYADDR;
> > >  #endif
> > >
> > > -     phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> > > +#ifdef CONFIG_DM_MDIO
> > > +     if (priv->dm_mdio)
> > > +             phydev = dm_eth_phy_connect(dev);
> > > +#endif
> > > +     if (!phydev)
> > > +             phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> > >       if (!phydev)
> > >               return -ENODEV;
> > >
> > > @@ -1227,11 +1308,19 @@ static int fecmxc_probe(struct udevice *dev)
> > >
> > >       priv->dev_id = dev_seq(dev);
> > >
> > > +#ifdef CONFIG_DM_MDIO
> > > +     ret = dm_fec_bind_mdio(dev);
> > > +     if (!ret) {
> > > +             ret = fec_phy_init(priv, dev);
> > > +             if (!ret)
> > > +                     priv->dm_mdio = true;
> > > +     }
> > > +#endif
> > >  #ifdef CONFIG_DM_ETH_PHY
> > >       bus = eth_phy_get_mdio_bus(dev);
> > >  #endif
> > >
> > > -     if (!bus) {
> > > +     if (!bus && !priv->dm_mdio) {
> > >               dm_mii_bus = false;
> > >  #ifdef CONFIG_FEC_MXC_MDIO_BASE
> > >               bus = fec_get_miibus((ulong)CONFIG_FEC_MXC_MDIO_BASE,
> > > @@ -1240,7 +1329,7 @@ static int fecmxc_probe(struct udevice *dev)
> > >               bus = fec_get_miibus((ulong)priv->eth, dev_seq(dev));
> > >  #endif
> > >       }
> > > -     if (!bus) {
> > > +     if (!bus && !priv->dm_mdio) {
> > >               ret = -ENOMEM;
> > >               goto err_mii;
> > >       }
> > > @@ -1271,14 +1360,16 @@ static int fecmxc_probe(struct udevice *dev)
> > >               break;
> > >       }
> > >
> > > -     ret = fec_phy_init(priv, dev);
> > > -     if (ret)
> > > -             goto err_phy;
> > > +     if (!priv->dm_mdio) {
> > > +             ret = fec_phy_init(priv, dev);
> > > +             if (ret)
> > > +                     goto err_phy;
> > > +     }
> > >
> > >       return 0;
> > >
> > >  err_phy:
> > > -     if (!dm_mii_bus) {
> > > +     if (!dm_mii_bus && !priv->dm_mdio) {
> > >               mdio_unregister(bus);
> > >               free(bus);
> > >       }
> > > @@ -1294,8 +1385,10 @@ static int fecmxc_remove(struct udevice *dev)
> > >
> > >       free(priv->phydev);
> > >       fec_free_descs(priv);
> > > -     mdio_unregister(priv->bus);
> > > -     mdio_free(priv->bus);
> > > +     if (priv->bus) {
> > > +             mdio_unregister(priv->bus);
> > > +             mdio_free(priv->bus);
> > > +     }
> > >
> > >  #ifdef CONFIG_DM_REGULATOR
> > >       if (priv->phy_supply)
> > > diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
> > > index 48faa33d66ec..c297880ccc54 100644
> > > --- a/drivers/net/fec_mxc.h
> > > +++ b/drivers/net/fec_mxc.h
> > > @@ -248,6 +248,7 @@ struct fec_priv {
> > >       uint8_t *tdb_ptr;
> > >       int dev_id;
> > >       struct mii_dev *bus;
> > > +     bool dm_mdio;
> > >  #ifdef CONFIG_PHYLIB
> > >       struct phy_device *phydev;
> > >       ofnode phy_of_node;
> > > --
> > > 2.17.1
> > >
> 
> What I'm trying to accomplish here vs just simply using
> dm_fec_bind_mdio(dev) and dm_eth_phy_connect(dev) is to provide a
> fallback for the many users of fec_mxc that do not have a dt that
> works with dm-mdio yet still have defconfig's that enable DM_MDIO.
> Most of the users would not currently have an mdio subnode in the fec
> node, nor have the required phy-mode 'and' phy-handle prop or
> fixed-link subnode which would cause the dm_eth_phy_connect to fail.
> 
> Best Regards,
> 
> Tim

Re: [PATCH 0/6] Add MV88E61xx DSA driver and use on gwventana

[Chris Packham]

Hi Tim,

On 31/03/22 05:01, Tim Harvey wrote:
> On Tue, Mar 29, 2022 at 3:52 PM Tim Harvey <tharvey@gateworks.com> wrote:
>> This series adds a DSA driver for the MV88E61xx based on
>> drivers/net/phy/mv88e61xx and uses in on the gwventana_gw5904_defconfig.
>>
>> The hope is that the other three boards that use the MV88E61xx driver
>> can move to this as well eventually so that we can remove the non-dm
>> driver and the 4 Kconfig options it requires.
>>
>> The MV88E61xx has an MDIO interface this DM_MDIO must be used so support
>> for a UCLASS_MDIO driver is added to the fec_mxc ethernet driver in a
>> way that allows a fallback to the previous non DM_MDIO case as there are
>> many boards out there using this driver that define DM_MDIO but do not
>> have the required dt props for a DM_MDIO driver which would cause a
>> regression.
>>
>> Additionally a patch to ensure MDIO children are scanned on post-bind is
>> needed.
>>
>> I'm making use of the dm_mdio_read/dm_mdio_write wrapper from Merak that
>> is pending so it is in this series as well.
>>
>> Best Regards,
>>
>> Tim
>>
>> Tim Harvey (6):
>>    net: mdio-uclass: scan for dm mdio children on post-bind
>>    net: dsa: move cpu port probe to dsa_post_probe
>>    net: mdio-uclass: add wrappers for read/write/reset operations
>>    net: fec: add support for DM_MDIO
>>    net: add MV88E61xx DSA driver
>>    board: gw_ventana: enable MV88E61XX DSA support
>>
>>   arch/arm/dts/imx6qdl-gw5904.dtsi        |  35 +
>>   board/gateworks/gw_ventana/gw_ventana.c |  50 +-
>>   configs/gwventana_gw5904_defconfig      |   7 +-
>>   drivers/net/Kconfig                     |   7 +
>>   drivers/net/Makefile                    |   1 +
>>   drivers/net/fec_mxc.c                   | 113 ++-
>>   drivers/net/fec_mxc.h                   |   1 +
>>   drivers/net/mv88e61xx.c                 | 982 ++++++++++++++++++++++++
>>   include/miiphy.h                        |  31 +
>>   net/dsa-uclass.c                        |  14 +-
>>   net/mdio-uclass.c                       |  35 +
>>   11 files changed, 1227 insertions(+), 49 deletions(-)
>>   create mode 100644 drivers/net/mv88e61xx.c
>>
>> --
>> 2.17.1
>>
> Adding Chris and Anotolij to the thread as the maintainers of the only
> other boards using drivers/net/phy/mv88e61xx.c. If they can switch to
> the DSA driver it would allow us to remove the static Kconfig options
> to configure the switch which should be present in the device-tree.

I'd be keen to see DSA support for this chip. We are using the Linux DSA 
drivers on these boards so I'm reasonably sure the ported driver should 
work. I won't have much time to actually work on these boards (they're a 
bit of a pain to test remotely). If any of the code is getting in the 
way I'd be fine with removing it (effectively leaving eth0 as a 
fixed-link) and when I get some time I can add support for the 88e6097 back.

>
> Best Regards,
>
> Tim

Re: [PATCH 4/6] net: fec: add support for DM_MDIO

[Tim Harvey]

On Thu, Mar 31, 2022 at 12:36 PM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> On Thu, Mar 31, 2022 at 10:48:55AM -0700, Tim Harvey wrote:
> > > On which branch does this apply? The context above fecmxc_read_rom_hwaddr()
> > > is different in the branches I've checked:
> > > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/fec_mxc.c#L1276
> > > https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/fec_mxc.c#L1276
> > >
> >
> > Sorry, I should have specified in my cover letter that this is on top
> > of next which removes the non dm-eth support from the driver which is
> > quite the cleanup.
>
> Ok, but now the next patch fails to apply to the 'next' branch:

I based on top of origin/next, specifically:
34d2b7f20369 (origin/next) Merge tag 'v2022.04-rc5' into next

>
> Applying: net: dsa: move cpu port probe to dsa_post_probe
> Applying: net: mdio-uclass: add wrappers for read/write/reset operations
> Applying: net: fec: add support for DM_MDIO
> Applying: net: add MV88E61xx DSA driver
> error: patch failed: drivers/net/Kconfig:428
> error: drivers/net/Kconfig: patch does not apply
> error: patch failed: drivers/net/Makefile:66
> error: drivers/net/Makefile: patch does not apply
> Patch failed at 0005 net: add MV88E61xx DSA driver
>
> > Also, after more testing I believe the dm-mdio driver code 'above' is
> > correct but the business of trying to use it and fallback 'below' is
> > wrong and needs work. It doesn't break current users of fec_mxc from
> > what I can tell but it also doesn't properly connect the dm_mdio
> > driver.
>
> Can you spell out what is wrong about the fallback logic?
>

Let me revisit later today and see if I can better explain the issue
here and/or come up with a proper fix.

Can you review 'net: add MV88E61xx DSA driver' for me?

> The whole thing with eth_phy_get_mdio_bus()/eth_phy_set_mdio_bus() has
> me so confused that I am not really following along anymore.
>

I'm pretty confused at CONFIG_DM_ETH_PHY as well. I think that got
added where dm_mdio should have been added instead. see commit
5fe419ef2a61 ("net: Add eth phy generic driver for shared MDIO").

I've added Ye and Peng to the list... maybe they could comment on why
dm_mdio wasn't a viable solution for sharing an mdio bus? Its only
used in for fec_mxc dwc_eth_qos which are only on IMX boards and
believe it was added for IMX8M boards. It's enabled in a whole slew of
imx defconfigs where most of them are likely not needed.

Tim


What I'm trying to accomplish here vs just simply using
dm_fec_bind_mdio(dev) and dm_eth_phy_connect(dev) is to provide a
fallback for the many users of fec_mxc that do not have a dt that
works with dm-mdio yet still have defconfig's that enable DM_MDIO.
Most of the users would not currently have an mdio subnode in the fec
node, nor have the required phy-mode 'and' phy-handle prop or
fixed-link subnode which would cause the dm_eth_phy_connect to fail.

Best Regards,

Tim

Re: [PATCH 4/6] net: fec: add support for DM_MDIO

[Vladimir Oltean]

On Fri, Apr 01, 2022 at 10:53:14AM -0700, Tim Harvey wrote:
> Can you review 'net: add MV88E61xx DSA driver' for me?

I will. I've been thinking all day today about what to say that isn't
stupid. Give me some time and I'll provide feedback.

Re: [PATCH 1/6] net: mdio-uclass: scan for dm mdio children on post-bind

[Ramon Fried]

On Wed, Mar 30, 2022 at 1:52 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> If a DM_MDIO driver is used we need to scan the subnodes as well.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/mdio-uclass.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
> index e74e34f78f9c..190cb08b31d8 100644
> --- a/net/mdio-uclass.c
> +++ b/net/mdio-uclass.c
> @@ -59,7 +59,11 @@ static int dm_mdio_post_bind(struct udevice *dev)
>                 return -EINVAL;
>         }
>
> +#if CONFIG_IS_ENABLED(OF_REAL)
> +       return dm_scan_fdt_dev(dev);
> +#else
>         return 0;
> +#endif
>  }
>
>  /*
> --
> 2.17.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 2/6] net: dsa: move cpu port probe to dsa_post_probe

[Ramon Fried]

On Wed, Mar 30, 2022 at 1:52 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> In order to ensure that a DSA driver probe gets called before
> dsa_ops->port_probe move the port_probe of the cpu_port to
> a post-probe function.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  net/dsa-uclass.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index 9ff55a02fb23..07edc584daf3 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -466,8 +466,6 @@ 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);
> -       struct dsa_ops *ops = dsa_get_ops(dev);
> -       int err;
>
>         priv->num_ports = pdata->num_ports;
>         priv->cpu_port = pdata->cpu_port;
> @@ -480,6 +478,15 @@ static int dsa_pre_probe(struct udevice *dev)
>         uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
>                                      &priv->master_dev);
>
> +       return 0;
> +}
> +
> +static int dsa_post_probe(struct udevice *dev)
> +{
> +       struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +       struct dsa_ops *ops = dsa_get_ops(dev);
> +       int err;
> +
>         /* Simulate a probing event for the CPU port */
>         if (ops->port_probe) {
>                 err = ops->port_probe(dev, priv->cpu_port,
> @@ -489,13 +496,14 @@ static int dsa_pre_probe(struct udevice *dev)
>         }
>
>         return 0;
> -}
> +};
>
>  UCLASS_DRIVER(dsa) = {
>         .id = UCLASS_DSA,
>         .name = "dsa",
>         .post_bind = dsa_post_bind,
>         .pre_probe = dsa_pre_probe,
> +       .post_probe = dsa_post_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),
> --
> 2.17.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Tim Harvey]

On Thu, Mar 31, 2022 at 3:30 AM Marek Behún <marek.behun@nic.cz> wrote:
>
> On Wed, 30 Mar 2022 08:46:06 -0700
> Tim Harvey <tharvey@gateworks.com> wrote:
>
> > On Tue, Mar 29, 2022 at 4:22 PM Marek Behún <marek.behun@nic.cz> wrote:
> > >
> > > On Tue, 29 Mar 2022 15:52:39 -0700
> > > Tim Harvey <tharvey@gateworks.com> wrote:
> > >
> > > > Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
> > > >
> > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > >
> >
> > Marek,
> >
> > Thanks for looking at this.
> >
> > > Is this final version that should be accepted?
> >
> > This is the first submission of the new driver.
> >
> > >
> > > The drivers seems to support not only 61xx, but 6xxx (6096, 6250, 6352,
> > > ...).
> >
> > It should also support all the same devices that the non dsa driver it
> > was derived from (drivers/net/phy/mv88e61xx.c) but I only have a board
> > with 88E6176 to test with.
> >
> > It is not completely clear to me what devices are supported in the
> > original driver per Kconfig but from looking over the code I would say
> > the following appear to be supported:
> > mv88e6096
> > mv88e6097
> > mv88e6020
> > mv88e6070
> > mv88e6071
> > mv88e6172
> > mv88e6176
> > mv88e6220
> > mv88e6240
> > mv88e6250
> > mv88e6352
> >
> > >
> > > Also there are some commented lines, for example
> > >
> > > > +//   u16 port_stat_link_mask;/* Bitmask for port link status bits */
> > > > +//   u16 port_stat_dup_mask; /* Bitmask for port duplex status bits */
> > > > +//   u8 port_stat_speed_width;/* Width of speed status bitfield */
> > >
> > > What is their purpose?
> > >
> >
> > oops... that was a mistake. I'm surprised checkpatch didn't catch
> > those. Those were in the non-dsa driver and not used here so will be
> > removed.
> >
> > > Why is mv88e61xx_dsa_xmit() no-op?
> >
> > For DSA dsa-uclass calls the switch master eth device send function
> > after calling the dsa_ops->xmit function so that a dsa driver can add
> > any header/footer if needed. The function is required but in my case I
> > don't care about header/footer tagging or vlan as only 1 port is
> > active at a time in U-Boot so I just return success.
>
> So if I make one port active, the other are completely disabled? They
> won't even switch? Is that how DSA uclass is supposed to work in U-Boot?
>
> I would think that it should be somehow configurable instead.
>

Marek,

I'll let Vladimir correct me if I'm wrong but my understanding is DSA
in U-Boot is not intended to allow switches to forward packets on
their own from port to port but instead just for forwarding packets
between the active port and the MAC connected to the CPU (at least
that's what I intended when I wrote the ksz9477 dsa driver
previously).

In my opinion what a DSA driver provides is avoidance of putting
switches in forwarding mode having the potential of easily creating
bridge loops. With the existing mv88e61xx driver I've had users create
bridge loops often.

Best regards,

Tim

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Vladimir Oltean]

On Fri, Apr 01, 2022 at 01:24:48PM -0700, Tim Harvey wrote:
> > > > Why is mv88e61xx_dsa_xmit() no-op?
> > >
> > > For DSA dsa-uclass calls the switch master eth device send function
> > > after calling the dsa_ops->xmit function so that a dsa driver can add
> > > any header/footer if needed. The function is required but in my case I
> > > don't care about header/footer tagging or vlan as only 1 port is
> > > active at a time in U-Boot so I just return success.
> >
> > So if I make one port active, the other are completely disabled? They
> > won't even switch? Is that how DSA uclass is supposed to work in U-Boot?
> >
> > I would think that it should be somehow configurable instead.
> 
> Marek,
> 
> I'll let Vladimir correct me if I'm wrong but my understanding is DSA
> in U-Boot is not intended to allow switches to forward packets on
> their own from port to port but instead just for forwarding packets
> between the active port and the MAC connected to the CPU (at least
> that's what I intended when I wrote the ksz9477 dsa driver
> previously).
> 
> In my opinion what a DSA driver provides is avoidance of putting
> switches in forwarding mode having the potential of easily creating
> bridge loops. With the existing mv88e61xx driver I've had users create
> bridge loops often.

DM_DSA follows the guiding principle that the user doesn't care about
the switch more than being able to transfer a boot image through TFTP
without causing switching loops in the network. The way this is achieved
is by following the "port extender" model which is exactly the way in
which Linux DSA was first introduced, prior to having support for L2
bridging offloads via switchdev. For U-Boot, packet forwarding across
front ports is effectively a non-goal. In fact, what we want to avoid is
the proliferation of bloatware such as cmd/ethsw.c, which can and will
be abused to preconfigure a switch from the bootloader so you won't have
to manage it from Linux.

Tim, I accept that U-Boot's Ethernet usage model (a single device active
at a time, with RX in synchronous poll mode) simplifies DM_DSA to the
point that creating/parsing DSA tags (which serve the purpose of
multiplexing/demultiplexing packets to/from switch ports) on the CPU is
more or less redundant when said multiplexing can be achieved on a time
basis by disabling all the ports except the required source/destination
switch port and the CPU port. This holds true even when DM_DSA gains
support for multi-switch setups.

What I would like, however, is to avoid driver code putting pressure on
the DSA uclass code. I don't really like the tracking of the priv->active_port
as being the port on which ->port_enable was last called. The only reason
this is done is to appease the "port_index != port_pdata->index" check
from dsa_port_recv(). In turn, this is because the ->xmit() and ->rcv()
DSA ops are not optional. Let's make them optional instead, instead of
adding code to a C file that indirectly depends on the code structure
from another C file.

But ->port_enable() and ->port_disable() are optional too, and that's
not okay, because if the driver doesn't provide a way to enable/disable
ports, packets can flow anywhere.

I suggest making it clear to driver writers that it has to be one or the
other by sanitizing the ops:

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index 9ff55a02fb23..ea860fa715bb 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -342,6 +342,19 @@ U_BOOT_DRIVER(dsa_port) = {
 	.plat_auto = sizeof(struct eth_pdata),
 };
 
+static int dsa_sanitize_ops(struct udevice *dev)
+{
+	struct dsa_ops *ops = dsa_get_ops(dev);
+
+	if ((!ops->xmit || !ops->rcv) &&
+	    (!ops->port_enable && !ops->port_disable)) {
+		dev_err(dev, "Packets cannot be steered to ports\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * This function mostly deals with pulling information out of the device tree
  * into the pdata structure.
@@ -358,6 +371,10 @@ static int dsa_post_bind(struct udevice *dev)
 	if (!ofnode_valid(node))
 		return -ENODEV;
 
+	err = dsa_sanitize_ops(dev);
+	if (err)
+		return err;
+
 	pdata->master_node = ofnode_null();
 
 	node = ofnode_find_subnode(node, "ports");


Then we can properly make rcv() and xmit() optional at the level of the
DSA uclass. This way we avoid the tail chasing approach of pretending
that "return priv->active_port" _is_ the way to decode a packet for a
certain driver. It isn't, and it should just be DSA's fallback if no
such method is known. I'd like to preserve the option to implement a
rcv() and xmit() procedure, because it offers one extra layer of safety
that the CPU is really talking to the switch port it thinks it's
talking, plus inter-switch traffic is reduced in cross-chip topologies
(which are not supported now, but maybe they will be).

I think that fallback could look like this:

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index ea860fa715bb..64654ec1ad72 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -131,16 +131,15 @@ static void dsa_port_stop(struct udevice *pdev)
  * 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)
+static int dsa_port_mangle_packet(struct udevice *pdev, void *packet,
+				  int length)
 {
+	struct dsa_port_pdata *port_pdata = dev_get_parent_plat(pdev);
 	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 (length + head + tail > PKTSIZE_ALIGN)
 		return -EINVAL;
@@ -152,10 +151,21 @@ static int dsa_port_send(struct udevice *pdev, void *packet, int length)
 	/* 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 ops->xmit(dev, port_pdata->index, packet, length);
+}
+
+static int dsa_port_send(struct udevice *pdev, void *packet, int length)
+{
+	struct udevice *dev = dev_get_parent(pdev);
+	struct udevice *master = dsa_get_master(dev);
+	struct dsa_ops *ops = dsa_get_ops(dev);
+	int err;
+
+	if (ops->xmit) {
+		err = dsa_port_mangle_packet(pdev, packet, length);
+		if (err)
+			return err;
+	}
 
 	return eth_get_ops(master)->send(master, packet, length);
 }
@@ -172,7 +182,7 @@ static int dsa_port_recv(struct udevice *pdev, int flags, uchar **packetp)
 	int length, port_index, err;
 
 	length = eth_get_ops(master)->recv(master, flags, packetp);
-	if (length <= 0)
+	if (length <= 0 || !ops->rcv)
 		return length;
 
 	/*

I think part of the reason for which Tim is actively trying to avoid DSA
tag inserting and parsing is because fec_mxc.c refuses to send packets
larger than 1500 bytes. As a side note, I do think that limitation
should be lifted and do allow for a reasonable amount of extra bytes,
because as things stand, if somebody will want to implement rcv() and
xmit() for either the mv88e6xxx or ksz9477 drivers, they'll involuntarily
break the boards where fec is a DSA master, which isn't really very fair.

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Tim Harvey]

On Sat, Apr 2, 2022 at 4:17 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Apr 01, 2022 at 01:24:48PM -0700, Tim Harvey wrote:
> > > > > Why is mv88e61xx_dsa_xmit() no-op?
> > > >
> > > > For DSA dsa-uclass calls the switch master eth device send function
> > > > after calling the dsa_ops->xmit function so that a dsa driver can add
> > > > any header/footer if needed. The function is required but in my case I
> > > > don't care about header/footer tagging or vlan as only 1 port is
> > > > active at a time in U-Boot so I just return success.
> > >
> > > So if I make one port active, the other are completely disabled? They
> > > won't even switch? Is that how DSA uclass is supposed to work in U-Boot?
> > >
> > > I would think that it should be somehow configurable instead.
> >
> > Marek,
> >
> > I'll let Vladimir correct me if I'm wrong but my understanding is DSA
> > in U-Boot is not intended to allow switches to forward packets on
> > their own from port to port but instead just for forwarding packets
> > between the active port and the MAC connected to the CPU (at least
> > that's what I intended when I wrote the ksz9477 dsa driver
> > previously).
> >
> > In my opinion what a DSA driver provides is avoidance of putting
> > switches in forwarding mode having the potential of easily creating
> > bridge loops. With the existing mv88e61xx driver I've had users create
> > bridge loops often.
>
> DM_DSA follows the guiding principle that the user doesn't care about
> the switch more than being able to transfer a boot image through TFTP
> without causing switching loops in the network. The way this is achieved
> is by following the "port extender" model which is exactly the way in
> which Linux DSA was first introduced, prior to having support for L2
> bridging offloads via switchdev. For U-Boot, packet forwarding across
> front ports is effectively a non-goal. In fact, what we want to avoid is
> the proliferation of bloatware such as cmd/ethsw.c, which can and will
> be abused to preconfigure a switch from the bootloader so you won't have
> to manage it from Linux.

Vladimir,

I also want to avoid the proliferation of bloatware which is primarily
why I want to avoid having to muck with packets when there is only one
port active at a time which I feel is completely unnecessary.

>
> Tim, I accept that U-Boot's Ethernet usage model (a single device active
> at a time, with RX in synchronous poll mode) simplifies DM_DSA to the
> point that creating/parsing DSA tags (which serve the purpose of
> multiplexing/demultiplexing packets to/from switch ports) on the CPU is
> more or less redundant when said multiplexing can be achieved on a time
> basis by disabling all the ports except the required source/destination
> switch port and the CPU port. This holds true even when DM_DSA gains
> support for multi-switch setups.
>
> What I would like, however, is to avoid driver code putting pressure on
> the DSA uclass code. I don't really like the tracking of the priv->active_port
> as being the port on which ->port_enable was last called. The only reason
> this is done is to appease the "port_index != port_pdata->index" check
> from dsa_port_recv(). In turn, this is because the ->xmit() and ->rcv()
> DSA ops are not optional. Let's make them optional instead, instead of
> adding code to a C file that indirectly depends on the code structure
> from another C file.
>
> But ->port_enable() and ->port_disable() are optional too, and that's
> not okay, because if the driver doesn't provide a way to enable/disable
> ports, packets can flow anywhere.
>
> I suggest making it clear to driver writers that it has to be one or the
> other by sanitizing the ops:
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index 9ff55a02fb23..ea860fa715bb 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -342,6 +342,19 @@ U_BOOT_DRIVER(dsa_port) = {
>         .plat_auto = sizeof(struct eth_pdata),
>  };
>
> +static int dsa_sanitize_ops(struct udevice *dev)
> +{
> +       struct dsa_ops *ops = dsa_get_ops(dev);
> +
> +       if ((!ops->xmit || !ops->rcv) &&
> +           (!ops->port_enable && !ops->port_disable)) {
> +               dev_err(dev, "Packets cannot be steered to ports\n");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * This function mostly deals with pulling information out of the device tree
>   * into the pdata structure.
> @@ -358,6 +371,10 @@ static int dsa_post_bind(struct udevice *dev)
>         if (!ofnode_valid(node))
>                 return -ENODEV;
>
> +       err = dsa_sanitize_ops(dev);
> +       if (err)
> +               return err;
> +
>         pdata->master_node = ofnode_null();
>
>         node = ofnode_find_subnode(node, "ports");
>
>
> Then we can properly make rcv() and xmit() optional at the level of the
> DSA uclass. This way we avoid the tail chasing approach of pretending
> that "return priv->active_port" _is_ the way to decode a packet for a
> certain driver. It isn't, and it should just be DSA's fallback if no
> such method is known. I'd like to preserve the option to implement a
> rcv() and xmit() procedure, because it offers one extra layer of safety
> that the CPU is really talking to the switch port it thinks it's
> talking, plus inter-switch traffic is reduced in cross-chip topologies
> (which are not supported now, but maybe they will be).
>
> I think that fallback could look like this:
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index ea860fa715bb..64654ec1ad72 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -131,16 +131,15 @@ static void dsa_port_stop(struct udevice *pdev)
>   * 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)
> +static int dsa_port_mangle_packet(struct udevice *pdev, void *packet,
> +                                 int length)
>  {
> +       struct dsa_port_pdata *port_pdata = dev_get_parent_plat(pdev);
>         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 (length + head + tail > PKTSIZE_ALIGN)
>                 return -EINVAL;
> @@ -152,10 +151,21 @@ static int dsa_port_send(struct udevice *pdev, void *packet, int length)
>         /* 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 ops->xmit(dev, port_pdata->index, packet, length);
> +}
> +
> +static int dsa_port_send(struct udevice *pdev, void *packet, int length)
> +{
> +       struct udevice *dev = dev_get_parent(pdev);
> +       struct udevice *master = dsa_get_master(dev);
> +       struct dsa_ops *ops = dsa_get_ops(dev);
> +       int err;
> +
> +       if (ops->xmit) {
> +               err = dsa_port_mangle_packet(pdev, packet, length);
> +               if (err)
> +                       return err;
> +       }
>
>         return eth_get_ops(master)->send(master, packet, length);
>  }
> @@ -172,7 +182,7 @@ static int dsa_port_recv(struct udevice *pdev, int flags, uchar **packetp)
>         int length, port_index, err;
>
>         length = eth_get_ops(master)->recv(master, flags, packetp);
> -       if (length <= 0)
> +       if (length <= 0 || !ops->rcv)
>                 return length;
>
>         /*
>

I can add this to my series if I submit a follow-up.

> I think part of the reason for which Tim is actively trying to avoid DSA
> tag inserting and parsing is because fec_mxc.c refuses to send packets
> larger than 1500 bytes. As a side note, I do think that limitation
> should be lifted and do allow for a reasonable amount of extra bytes,
> because as things stand, if somebody will want to implement rcv() and

> xmit() for either the mv88e6xxx or ksz9477 drivers, they'll involuntarily
> break the boards where fec is a DSA master, which isn't really very fair.

Very likely.

I guess I'll have to invest in tagging packets if you won't accept the
simplistic approach of not having to tag frames knowing that only one
port is active at a time.

That said, I have no idea if or when I will re-visit this. Adding a
DSA version of this driver was something on my personal wish list and
not something that was necessary by any means by my employer so I may
have to just drop it as I don't have the personal time to work through
this part of it or unravelling the mii bus mess in the fec_mxc driver.
It seems to me there is an issue with trying to create DM_MDIO drivers
in general as most dt's I've seen wouldn't support the requirements
yet configure DM_MDIO anyway (meaning if you implemented it you would
break those boards as I found).

Best Regards,

Tim

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Vladimir Oltean]

On Thu, Apr 07, 2022 at 01:33:58PM -0700, Tim Harvey wrote:
> I guess I'll have to invest in tagging packets if you won't accept the
> simplistic approach of not having to tag frames knowing that only one
> port is active at a time.

I genuinely don't know where you got the impression from that I don't
accept the simplistic approach. I gave you an option to make the xmit
and receive ops actually optional at the DSA uclass level so you don't
have to come up with a make-believe tag parsing function. In the end
it goes towards the simplification of the Marvell driver. Let's let them
battle it out for a while and if tag insertion/parsing won't be
necessary even for multi-switch systems we can discuss about removing
that logic completely.

> That said, I have no idea if or when I will re-visit this. Adding a
> DSA version of this driver was something on my personal wish list and
> not something that was necessary by any means by my employer so I may
> have to just drop it as I don't have the personal time to work through
> this part of it or unravelling the mii bus mess in the fec_mxc driver.
> It seems to me there is an issue with trying to create DM_MDIO drivers
> in general as most dt's I've seen wouldn't support the requirements
> yet configure DM_MDIO anyway (meaning if you implemented it you would
> break those boards as I found).

I don't know why there are boards which set CONFIG_DM_MDIO and then
fight against the current trying to survive that config being set.
Maybe you can look into disabling that config option on boards that
aren't prepared to handle it?

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Tim Harvey]

On Thu, Apr 7, 2022 at 2:31 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Thu, Apr 07, 2022 at 01:33:58PM -0700, Tim Harvey wrote:
> > I guess I'll have to invest in tagging packets if you won't accept the
> > simplistic approach of not having to tag frames knowing that only one
> > port is active at a time.
>
> I genuinely don't know where you got the impression from that I don't
> accept the simplistic approach. I gave you an option to make the xmit
> and receive ops actually optional at the DSA uclass level so you don't
> have to come up with a make-believe tag parsing function. In the end
> it goes towards the simplification of the Marvell driver. Let's let them
> battle it out for a while and if tag insertion/parsing won't be
> necessary even for multi-switch systems we can discuss about removing
> that logic completely.

Ok... sorry I misunderstood.

>
> > That said, I have no idea if or when I will re-visit this. Adding a
> > DSA version of this driver was something on my personal wish list and
> > not something that was necessary by any means by my employer so I may
> > have to just drop it as I don't have the personal time to work through
> > this part of it or unravelling the mii bus mess in the fec_mxc driver.
> > It seems to me there is an issue with trying to create DM_MDIO drivers
> > in general as most dt's I've seen wouldn't support the requirements
> > yet configure DM_MDIO anyway (meaning if you implemented it you would
> > break those boards as I found).
>
> I don't know why there are boards which set CONFIG_DM_MDIO and then
> fight against the current trying to survive that config being set.
> Maybe you can look into disabling that config option on boards that
> aren't prepared to handle it?

There might not be many boards that would 'break'. Here's what uses
FEC_MXC and DM_MDIO:
$ grep -H CONFIG_FEC_MXC configs/* | awk -F: '{ print $1 }' | xargs grep DM_MDIO
configs/colibri_imx7_defconfig:CONFIG_DM_MDIO=y
configs/colibri_imx7_emmc_defconfig:CONFIG_DM_MDIO=y
configs/ge_b1x5v2_defconfig:CONFIG_DM_MDIO=y
configs/gwventana_emmc_defconfig:CONFIG_DM_MDIO=y
configs/gwventana_gw5904_defconfig:CONFIG_DM_MDIO=y
configs/gwventana_nand_defconfig:CONFIG_DM_MDIO=y
configs/imx7_cm_defconfig:CONFIG_DM_MDIO=y
configs/imx7_cm_defconfig:CONFIG_DM_MDIO_MUX=y
configs/imx8mm_venice_defconfig:CONFIG_DM_MDIO=y
configs/imx8mn_venice_defconfig:CONFIG_DM_MDIO=y
configs/imx8mp_venice_defconfig:CONFIG_DM_MDIO=y
configs/m53menlo_defconfig:CONFIG_DM_MDIO=y
configs/mx7dsabresd_defconfig:CONFIG_DM_MDIO=y
configs/mx7dsabresd_defconfig:CONFIG_DM_MDIO_MUX=y
configs/mx7dsabresd_qspi_defconfig:CONFIG_DM_MDIO=y
configs/mx7dsabresd_qspi_defconfig:CONFIG_DM_MDIO_MUX=y
configs/opos6uldev_defconfig:CONFIG_DM_MDIO=y
configs/pcm058_defconfig:CONFIG_DM_MDIO=y
configs/pico-dwarf-imx7d_defconfig:CONFIG_DM_MDIO=y
configs/pico-hobbit-imx7d_defconfig:CONFIG_DM_MDIO=y
configs/pico-imx7d_bl33_defconfig:CONFIG_DM_MDIO=y
configs/pico-imx7d_defconfig:CONFIG_DM_MDIO=y
configs/pico-nymph-imx7d_defconfig:CONFIG_DM_MDIO=y
configs/pico-pi-imx7d_defconfig:CONFIG_DM_MDIO=y
configs/udoo_neo_defconfig:CONFIG_DM_MDIO=y

The venice/gwventana ones are mine so I can easily test/fix. For the
others just looking at their CONFIG_DEFAULT_DEVICE_TREE and
eliminating duplicates I see:

I believe these would break:
arch/arm/dts/imx7-colibri-rawnand.dts (no mdio subnode)
arch/arm/dts/imx7-colibri-emmc.dts (no mdio subnode)
arch/arm/dts/imx6sx-udoo-neo-basic.dts (no phy-mode)

These should be ok; have phy-mode, phy-handle, mdio subnode
arch/arm/dts/imx6dl-b1x5v2.dts
arch/arm/dts/imx7-cm.dts
arch/arm/dts/imx53-m53menlo.dts
arch/arm/dts/imx6ul-opos6uldev.dts
arch/arm/dts/imx6q-phytec-mira-rdk-nand.dts
arch/arm/dts/imx7d-pico-pi.dts

These may be ok; has phy-mode, phy-handle, mdio subnode for fec1, but
missing mdio subnode for fec2 (but should use mdio from fec1)
arch/arm/dts/imx7d-sdb.dts
arch/arm/dts/imx7d-sdb-qspi.dts

I feel like I would need to get all board maintainers using FEC_MXC to
sign-off that their boards still work.

But then there is this issue of CONFIG_DM_ETH_PHY that still throws
around the concept of struct mii_dev* (which I think should have been
handled by switching to DM_MDIO). There are now three drivers using
this and I'm not sure what to do with that. There are 28 boards using
CONFIG_DM_ETH_PHY (and likely some which are not even using one of the
three drivers that even use CONFIG_DM_ETH_PHY).

Is there a move to try and move all network drivers to DM_MDIO
eliminating the need for struct mii_dev* within those drivers?

Best Regards,

Tim

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Marek Behún]

On Thu, 7 Apr 2022 16:03:12 -0700
Tim Harvey <tharvey@gateworks.com> wrote:

> Is there a move to try and move all network drivers to DM_MDIO
> eliminating the need for struct mii_dev* within those drivers?

Yes.

Marek

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Vladimir Oltean]

On Tue, Mar 29, 2022 at 03:52:39PM -0700, Tim Harvey wrote:
> Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/net/Kconfig     |   7 +
>  drivers/net/Makefile    |   1 +
>  drivers/net/mv88e61xx.c | 982 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 990 insertions(+)
>  create mode 100644 drivers/net/mv88e61xx.c
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index a6171a7c7ffd..fc018f5ba47f 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -428,6 +428,13 @@ config LPC32XX_ETH
>  	depends on ARCH_LPC32XX
>  	default y
>  
> +config MV88E61XX
> +	bool "Marvell MV88E61xx GbE switch DSA driver"
> +	depends on DM_DSA && DM_MDIO
> +	help
> +	  This driver implements a DSA switch driver for the MV88E61xx family
> +	  of GbE switches using the MDIO interface
> +
>  config MVGBE
>  	bool "Marvell Orion5x/Kirkwood network interface support"
>  	depends on ARCH_KIRKWOOD || ARCH_ORION5X
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index a6d0c23f02d3..11ada73658e9 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_MEDIATEK_ETH) += mtk_eth.o
>  obj-$(CONFIG_MPC8XX_FEC) += mpc8xx_fec.o
>  obj-$(CONFIG_MT7620_ETH) += mt7620-eth.o
>  obj-$(CONFIG_MT7628_ETH) += mt7628-eth.o
> +obj-$(CONFIG_MV88E61XX) += mv88e61xx.o
>  obj-$(CONFIG_MVGBE) += mvgbe.o
>  obj-$(CONFIG_MVMDIO) += mvmdio.o
>  obj-$(CONFIG_MVNETA) += mvneta.o
> diff --git a/drivers/net/mv88e61xx.c b/drivers/net/mv88e61xx.c
> new file mode 100644
> index 000000000000..9dd7a0c7f42e
> --- /dev/null
> +++ b/drivers/net/mv88e61xx.c
> @@ -0,0 +1,982 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2022
> + * Gateworks Corporation <www.gateworks.com>
> + * Tim Harvey <tharvey@gateworks.com>
> + *
> + * (C) Copyright 2015
> + * Elecsys Corporation <www.elecsyscorp.com>
> + * Kevin Smith <kevin.smith@elecsyscorp.com>
> + *
> + * Original driver:
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafulla@marvell.com>
> + */
> +
> +/*
> + * DSA driver for mv88e61xx ethernet switches.
> + *
> + * This driver configures the mv88e61xx for basic use as a DSA switch.
> + *
> + * This driver was adapted from drivers/net/phy/mv88e61xx and tested
> + * on the mv88e6176 via an SGMII interface.
> + */
> +
> +#include <bitfield.h>
> +#include <common.h>
> +#include <miiphy.h>
> +#include <dm/device.h>
> +#include <dm/device_compat.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <dm/of_extra.h>
> +#include <linux/delay.h>
> +#include <net/dsa.h>
> +
> +#define PORT_MASK(port_count)		((1 << (port_count)) - 1)

Not used.

> +
> +/* Device addresses */
> +#define DEVADDR_PHY(p)			(p)
> +#define DEVADDR_SERDES			0x0F
> +
> +/* SMI indirection registers for multichip addressing mode */
> +#define SMI_CMD_REG			0x00
> +#define SMI_DATA_REG			0x01
> +
> +/* Global registers */
> +#define GLOBAL1_STATUS			0x00
> +#define GLOBAL1_CTRL			0x04
> +#define GLOBAL1_MON_CTRL		0x1A
> +
> +/* Global 2 registers */
> +#define GLOBAL2_REG_PHY_CMD		0x18
> +#define GLOBAL2_REG_PHY_DATA		0x19
> +#define GLOBAL2_REG_SCRATCH		0x1A
> +
> +/* Port registers */
> +#define PORT_REG_STATUS			0x00
> +#define PORT_REG_PHYS_CTRL		0x01
> +#define PORT_REG_SWITCH_ID		0x03
> +#define PORT_REG_CTRL			0x04
> +#define PORT_REG_VLAN_MAP		0x06
> +#define PORT_REG_VLAN_ID		0x07
> +#define PORT_REG_LED_CTRL		0x16
> +
> +/* Phy registers */
> +#define PHY_REG_CTRL1			0x10
> +#define PHY_REG_STATUS1			0x11
> +#define PHY_REG_PAGE			0x16
> +
> +/* Serdes registers */
> +#define SERDES_REG_CTRL_1		0x10
> +
> +/* Phy page numbers */
> +#define PHY_PAGE_COPPER			0
> +#define PHY_PAGE_SERDES			1
> +
> +/* Register fields */
> +#define GLOBAL1_CTRL_SWRESET		BIT(15)
> +
> +#define GLOBAL1_MON_CTRL_CPUDEST_SHIFT	4
> +#define GLOBAL1_MON_CTRL_CPUDEST_WIDTH	4
> +
> +#define PORT_REG_STATUS_SPEED_SHIFT	8
> +#define PORT_REG_STATUS_SPEED_10	0
> +#define PORT_REG_STATUS_SPEED_100	1
> +#define PORT_REG_STATUS_SPEED_1000	2
> +
> +#define PORT_REG_STATUS_CMODE_MASK		0xF
> +#define PORT_REG_STATUS_CMODE_100BASE_X		0x8
> +#define PORT_REG_STATUS_CMODE_1000BASE_X	0x9
> +#define PORT_REG_STATUS_CMODE_SGMII		0xa
> +
> +#define PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK	BIT(15)
> +#define PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK	BIT(14)
> +#define PORT_REG_PHYS_CTRL_PCS_AN_EN	BIT(10)
> +#define PORT_REG_PHYS_CTRL_PCS_AN_RST	BIT(9)
> +#define PORT_REG_PHYS_CTRL_FC_VALUE	BIT(7)
> +#define PORT_REG_PHYS_CTRL_FC_FORCE	BIT(6)
> +#define PORT_REG_PHYS_CTRL_LINK_VALUE	BIT(5)
> +#define PORT_REG_PHYS_CTRL_LINK_FORCE	BIT(4)
> +#define PORT_REG_PHYS_CTRL_DUPLEX_VALUE	BIT(3)
> +#define PORT_REG_PHYS_CTRL_DUPLEX_FORCE	BIT(2)
> +#define PORT_REG_PHYS_CTRL_SPD1000	BIT(1)
> +#define PORT_REG_PHYS_CTRL_SPD100	BIT(0)
> +#define PORT_REG_PHYS_CTRL_SPD_MASK	(BIT(1) | BIT(0))
> +
> +#define PORT_REG_CTRL_PSTATE_SHIFT	0
> +#define PORT_REG_CTRL_PSTATE_WIDTH	2
> +
> +#define PORT_REG_VLAN_ID_DEF_VID_SHIFT	0
> +#define PORT_REG_VLAN_ID_DEF_VID_WIDTH	12
> +
> +#define PORT_REG_VLAN_MAP_TABLE_SHIFT	0
> +#define PORT_REG_VLAN_MAP_TABLE_WIDTH	11
> +
> +#define SERDES_REG_CTRL_1_FORCE_LINK	BIT(10)
> +
> +/* Field values */
> +#define PORT_REG_CTRL_PSTATE_DISABLED	0
> +#define PORT_REG_CTRL_PSTATE_FORWARD	3
> +
> +#define PHY_REG_CTRL1_ENERGY_DET_OFF	0
> +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE	1
> +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_ONLY	2
> +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT	3
> +
> +/* PHY Status Register */
> +#define PHY_REG_STATUS1_SPEED		0xc000
> +#define PHY_REG_STATUS1_GBIT		0x8000
> +#define PHY_REG_STATUS1_100		0x4000
> +#define PHY_REG_STATUS1_DUPLEX		0x2000
> +#define PHY_REG_STATUS1_SPDDONE		0x0800
> +#define PHY_REG_STATUS1_LINK		0x0400
> +#define PHY_REG_STATUS1_ENERGY		0x0010
> +
> +/*
> + * Macros for building commands for indirect addressing modes.  These are valid
> + * for both the indirect multichip addressing mode and the PHY indirection
> + * required for the writes to any PHY register.
> + */
> +#define SMI_BUSY			BIT(15)
> +#define SMI_CMD_CLAUSE_22		BIT(12)
> +#define SMI_CMD_CLAUSE_22_OP_READ	(2 << 10)
> +#define SMI_CMD_CLAUSE_22_OP_WRITE	(1 << 10)
> +
> +#define SMI_CMD_READ			(SMI_BUSY | SMI_CMD_CLAUSE_22 | \
> +					 SMI_CMD_CLAUSE_22_OP_READ)
> +#define SMI_CMD_WRITE			(SMI_BUSY | SMI_CMD_CLAUSE_22 | \
> +					 SMI_CMD_CLAUSE_22_OP_WRITE)
> +
> +#define SMI_CMD_ADDR_SHIFT		5
> +#define SMI_CMD_ADDR_WIDTH		5
> +#define SMI_CMD_REG_SHIFT		0
> +#define SMI_CMD_REG_WIDTH		5
> +
> +/* Defines for Scratch and Misc Registers */
> +#define SCRATCH_UPDATE			BIT(15)
> +#define SCRATCH_ADDR_SHIFT		8
> +#define SCRATCH_ADDR_WIDTH		7
> +
> +/* Scratch registers */
> +#define SCRATCH_ADDR_GPIO0DIR		0x62 /* GPIO[7:0] direction 1=input */
> +#define SCRATCH_ADDR_GPIO1DIR		0x63 /* GPIO[14:8] direction 1=input */
> +#define SCRATCH_ADDR_GPIO0DATA		0x64 /* GPIO[7:0] data */
> +#define SCRATCH_ADDR_GPIO1DATA		0x65 /* GPIO[14:8] data */
> +#define SCRATCH_ADDR_GPIO0CTRL		0x68 /* GPIO[1:0] control */
> +#define SCRATCH_ADDR_GPIO1CTRL		0x69 /* GPIO[3:2] control */
> +
> +/* ID register values for different switch models */
> +#define PORT_SWITCH_ID_6020		0x0200
> +#define PORT_SWITCH_ID_6070		0x0700
> +#define PORT_SWITCH_ID_6071		0x0710
> +#define PORT_SWITCH_ID_6096		0x0980
> +#define PORT_SWITCH_ID_6097		0x0990
> +#define PORT_SWITCH_ID_6172		0x1720
> +#define PORT_SWITCH_ID_6176		0x1760
> +#define PORT_SWITCH_ID_6220		0x2200
> +#define PORT_SWITCH_ID_6240		0x2400
> +#define PORT_SWITCH_ID_6250		0x2500
> +#define PORT_SWITCH_ID_6352		0x3520
> +
> +struct mv88e61xx_phy_priv {

Could we choose a better name than mv88e61xx_phy_priv? It is not a PHY,
it is a switch.

> +static int mv88e61xx_fixed_port_setup(struct udevice *dev, u8 port)
> +{
> +	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> +	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> +	int val;
> +
> +	val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL);
> +	if (val < 0)
> +		return val;
> +
> +	val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK |
> +		 PORT_REG_PHYS_CTRL_FC_VALUE |
> +		 PORT_REG_PHYS_CTRL_FC_FORCE);
> +	val |= PORT_REG_PHYS_CTRL_FC_FORCE |
> +	       PORT_REG_PHYS_CTRL_DUPLEX_VALUE |
> +	       PORT_REG_PHYS_CTRL_DUPLEX_FORCE;
> +
> +	if (priv->id == PORT_SWITCH_ID_6071) {
> +		val |= PORT_REG_PHYS_CTRL_SPD100;
> +	} else {
> +		val |= PORT_REG_PHYS_CTRL_PCS_AN_EN |
> +		       PORT_REG_PHYS_CTRL_PCS_AN_RST |
> +		       PORT_REG_PHYS_CTRL_SPD1000;
> +	}
> +
> +	if (port == dsa_pdata->cpu_port)
> +		val |= PORT_REG_PHYS_CTRL_LINK_VALUE |
> +		       PORT_REG_PHYS_CTRL_LINK_FORCE;
> +
> +	return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> +}
> +
> +static int mv88e61xx_set_cpu_port(struct udevice *dev)
> +{
> +	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> +	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> +	int val;
> +
> +	/* Set CPUDest */
> +	val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MON_CTRL);
> +	if (val < 0)
> +		return val;
> +	val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT,
> +			       GLOBAL1_MON_CTRL_CPUDEST_WIDTH,
> +			       dsa_pdata->cpu_port);
> +	val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val);
> +	if (val < 0)
> +		return val;
> +
> +	/* Enable CPU port */
> +	val = mv88e61xx_port_enable(dev, dsa_pdata->cpu_port);
> +	if (val < 0)
> +		return val;
> +
> +	/* If CPU is connected to serdes, initialize serdes */
> +	if (mv88e61xx_6352_family(dev)) {
> +		val = mv88e61xx_get_cmode(dev, dsa_pdata->cpu_port);
> +		if (val < 0)
> +			return val;
> +		if (val == PORT_REG_STATUS_CMODE_100BASE_X ||
> +		    val == PORT_REG_STATUS_CMODE_1000BASE_X ||
> +		    val == PORT_REG_STATUS_CMODE_SGMII) {
> +			val = mv88e61xx_serdes_init(dev);

Allow me to suggest that the SERDES init function is improperly placed
in a function called "set_cpu_port".

Don't you need to initialize the SERDES also if a front port uses the
SERDES, not just the CPU port?

What happens if you initialize it unconditionally?

> +			if (val < 0)
> +				return val;
> +		}
> +	} else {
> +		val = mv88e61xx_fixed_port_setup(dev, dsa_pdata->cpu_port);
> +		if (val < 0)
> +			return val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mv88e61xx_switch_init(struct udevice *dev)
> +{
> +	static int init;
> +	int res;
> +
> +	if (init)
> +		return 0;

Odd, why the static int?

> +
> +	res = mv88e61xx_switch_reset(dev);
> +	if (res < 0)
> +		return res;
> +
> +	res = mv88e61xx_set_cpu_port(dev);
> +	if (res < 0)
> +		return res;
> +
> +	init = 1;
> +
> +	return 0;
> +}
> +
> +static int mv88e61xx_phy_setup(struct udevice *dev, u8 phy)
> +{
> +	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> +	int val;
> +
> +	/*
> +	 * Enable energy-detect sensing on PHY, used to determine when a PHY
> +	 * port is physically connected
> +	 */
> +	val = mv88e61xx_phy_read(dev, phy, PHY_REG_CTRL1);
> +	if (val < 0)
> +		return val;
> +	val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift,
> +			       priv->phy_ctrl1_en_det_width,
> +			       priv->phy_ctrl1_en_det_ctrl);
> +	val = mv88e61xx_phy_write(dev, phy, PHY_REG_CTRL1, val);
> +	if (val < 0)
> +		return val;
> +
> +	return 0;
> +}
> +
> +/*
> + * This function is used to pre-configure the required register
> + * offsets, so that the indirect register access to the PHY registers
> + * is possible. This is necessary to be able to read the PHY ID
> + * while driver probing or in get_phy_id(). The globalN register
> + * offsets must be initialized correctly for a detected switch,
> + * otherwise detection of the PHY ID won't work!
> + */
> +static int mv88e61xx_priv_reg_offs_pre_init(struct udevice *dev)
> +{
> +	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> +
> +	/*
> +	 * Initial 'port_reg_base' value must be an offset of existing
> +	 * port register, then reading the ID should succeed. First, try
> +	 * to read via port registers with device address 0x10 (88E6096
> +	 * and compatible switches).
> +	 */
> +	priv->port_reg_base = 0x10;
> +	priv->id = mv88e61xx_get_switch_id(dev);
> +	if (priv->id != 0xfff0) {
> +		priv->global1 = 0x1B;
> +		priv->global2 = 0x1C;
> +		return 0;
> +	}
> +
> +	/*
> +	 * Now try via port registers with device address 0x08
> +	 * (88E6020 and compatible switches).
> +	 */
> +	priv->port_reg_base = 0x08;
> +	priv->id = mv88e61xx_get_switch_id(dev);
> +	if (priv->id != 0xfff0) {
> +		priv->global1 = 0x0F;
> +		priv->global2 = 0x07;
> +		return 0;
> +	}
> +
> +	debug("%s Unknown ID 0x%x\n", __func__, priv->id);

This is the kind of error that is worth more than a "debug" message, no?

> +	return -ENODEV;
> +}
> +
> +static int mv88e61xx_dsa_setup_cpu_interface(struct udevice *dev)
> +{
> +	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> +	int port = dsa_pdata->cpu_port;
> +	phy_interface_t phy_interface;
> +	bool duplex, fixed;
> +	ofnode node;
> +	int speed;
> +	int val;
> +
> +	fixed = ofnode_phy_is_fixed_link(dsa_pdata->cpu_port_node, &node);
> +	if (fixed && ofnode_valid(node)) {
> +		speed = ofnode_read_u32_default(node, "speed", 0);
> +		duplex = ofnode_read_bool(node, "full-duplex");
> +	} else {
> +		duplex = false;
> +		speed = 0;
> +	}
> +	phy_interface = phy_get_interface_by_name(ofnode_read_string(dsa_pdata->cpu_port_node,
> +								     "phy-mode"));

Isn't this something all too overcomplicated for the driver when you can
just wait until mv88e61xx_dsa_port_enable() is called on the CPU port,
and it passes the fixed PHY as argument, with PHY mode and all?

> +
> +	dev_dbg(dev, "%s configuring CPU port P%d for %s fixed=%d speed=%d duplex=%d\n",
> +		__func__, port, phy_string_for_interface(phy_interface),
> +		fixed, speed, duplex);
> +
> +	/* Physical Control register: Table 62 */
> +	val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL);
> +	val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK ||
> +		 PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK);
> +	if (phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +		val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK;
> +	if (phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +		val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK;
> +	if (fixed)
> +		val |= BIT(5) | BIT(4); /* Force link */
> +	switch (speed) {
> +	case SPEED_10:
> +		break;
> +	case SPEED_100:
> +		val |= 0x1;
> +		break;
> +	case SPEED_1000:
> +		val |= 0x2;
> +		break;
> +	default:
> +		val |= 0x3; /* detect link speed */
> +		break;
> +	}
> +	if (duplex)
> +		val |= BIT(3) | BIT(2);
> +
> +	return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> +}
> +
> +static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> +	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> +	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	dev_dbg(dev, "%s P%d phy:0x%08x %s\n", __func__, port,
> +		phy->phy_id, phy_string_for_interface(phy->interface));
> +
> +	ret = mv88e61xx_phy_setup(dev, port);
> +	if (ret < 0) {
> +		dev_err(dev, "Error setting up PHY %i\n", port);
> +		return ret;
> +	}
> +	ret = mv88e61xx_port_enable(dev, port);
> +	if (ret < 0) {
> +		dev_err(dev, "Error enabling PHY %i\n", port);
> +		return ret;
> +	}
> +	if (port != dsa_pdata->cpu_port)
> +		priv->active_port = port;
> +	else
> +		mv88e61xx_set_cpu_port(dev);

Why is there a need to call this twice, first in mv88e61xx_switch_init()
then here?

> +
> +	return 0;
> +}
> +
> +static void mv88e61xx_dsa_port_disable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> +	struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> +	int val;
> +
> +	dev_dbg(dev, "%s P%d phy:0x%x\n", __func__, port, phy->phy_id);
> +
> +	/* can't disable CPU port without re-configuring/re-starting switch */
> +	if (port == dsa_pdata->cpu_port)
> +		return;

Interesting, do you know more?

> +
> +	val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL);
> +	if (val < 0)
> +		return;
> +	val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT,
> +			       PORT_REG_CTRL_PSTATE_WIDTH,
> +			       PORT_REG_CTRL_PSTATE_DISABLED);
> +	val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val);
> +	if (val < 0)
> +		return;
> +}

> +/* bind and probe the switch mdios */
> +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev)
> +{
> +	struct udevice *pdev;
> +	ofnode node, mdios;
> +	const char *name;
> +	int ret;
> +
> +	/* bind phy ports of mdios child node to mv88e61xx_mdio device */
> +	mdios = dev_read_subnode(dev, "mdios");

The Linux driver doesn't parse the "mdios" subnode, do you intend to do
some work in that direction?

> +	if (ofnode_valid(mdios)) {
> +		ofnode_for_each_subnode(node, mdios) {
> +			name = ofnode_get_name(node);
> +			ret = device_bind_driver_to_node(dev,
> +							 "mv88e61xx_mdio",
> +							 name, node, &pdev);
> +			if (ret) {
> +				dev_err(dev, "failed to bind %s: %d\n", name, ret);
> +				continue;
> +			}
> +
> +			/* need to probe it as there is no compatible to do so */
> +			ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &pdev);
> +			if (ret) {
> +				dev_err(dev, "failed to probe %s: %d\n", name, ret);
> +				continue;
> +			}
> +		}
> +	}
> +
> +	/* bind the mdios node to the parent mdio driver */
> +	name = ofnode_get_name(mdios);
> +	ret = device_bind_driver_to_node(dev,
> +					 "mv88e61xx_mdio",
> +					 name, mdios, &pdev);
> +	if (ret)
> +		dev_err(dev, "failed to bind %s: %d\n", name, ret);
> +
> +	/* need to probe it as there is no compatible to do so */
> +	ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdios, &pdev);
> +	if (ret)
> +		dev_err(dev, "failed to probe %s: %d\n", name, ret);
> +
> +	return ret;
> +}
> +
> +static int mv88e61xx_dsa_probe(struct udevice *dev)
> +{
> +	struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> +	struct udevice *master = dsa_get_master(dev);
> +	int ret;
> +
> +	if (!master)
> +		return -ENODEV;
> +
> +	dev_dbg(dev, "%s master:%s\n", __func__, master->name);
> +	dev_set_parent_priv(dev, priv);

Copy-pasta from the ksz9477 driver?

> +
> +	/* probe internal mdio bus */
> +	ret = mv88e61xx_dsa_probe_mdio(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = mv88e61xx_priv_reg_offs_pre_init(dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "ID=0x%x PORT_BASE=0x%02x GLOBAL1=0x%02x GLOBAL2=0x%02x\n",
> +		priv->id, priv->port_reg_base, priv->global1, priv->global2);
> +	switch (priv->id) {
> +	case PORT_SWITCH_ID_6096:
> +	case PORT_SWITCH_ID_6097:
> +	case PORT_SWITCH_ID_6172:
> +	case PORT_SWITCH_ID_6176:
> +	case PORT_SWITCH_ID_6240:
> +	case PORT_SWITCH_ID_6352:
> +		priv->port_count = 11;
> +//		priv->port_stat_link_mask = BIT(11);
> +//		priv->port_stat_dup_mask = BIT(10);
> +//		priv->port_stat_speed_width = 2;

No commented code.

> +		priv->phy_ctrl1_en_det_shift = 8;
> +		priv->phy_ctrl1_en_det_width = 2;
> +		priv->phy_ctrl1_en_det_ctrl =
> +			PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT;
> +		break;
> +	case PORT_SWITCH_ID_6020:
> +	case PORT_SWITCH_ID_6070:
> +	case PORT_SWITCH_ID_6071:
> +	case PORT_SWITCH_ID_6220:
> +	case PORT_SWITCH_ID_6250:
> +		priv->port_count = 7;
> +//		priv->port_stat_link_mask = BIT(12);
> +//		priv->port_stat_dup_mask = BIT(9);
> +//		priv->port_stat_speed_width = 1;
> +		priv->phy_ctrl1_en_det_shift = 14;
> +		priv->phy_ctrl1_en_det_width = 1;
> +		priv->phy_ctrl1_en_det_ctrl =
> +			PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	/* configure CPU port interface mode */
> +	ret = mv88e61xx_dsa_setup_cpu_interface(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mv88e61xx_switch_init(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsa_set_tagging(dev, 0, 0);
> +
> +	return 0;
> +}
> +
> +static const struct udevice_id mv88e61xx_ids[] = {
> +	{ .compatible = "marvell,mv88e6085" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(mv88e61xx) = {
> +	.name		= "mv88e61xx",
> +	.id		= UCLASS_DSA,
> +	.of_match	= mv88e61xx_ids,
> +	.probe		= mv88e61xx_dsa_probe,
> +	.ops		= &mv88e61xx_dsa_ops,
> +	.priv_auto	= sizeof(struct mv88e61xx_phy_priv),
> +};
> -- 
> 2.17.1
>

Re: [PATCH 5/6] net: add MV88E61xx DSA driver

[Tim Harvey]

On Tue, Apr 12, 2022 at 7:13 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Tue, Mar 29, 2022 at 03:52:39PM -0700, Tim Harvey wrote:
> > Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  drivers/net/Kconfig     |   7 +
> >  drivers/net/Makefile    |   1 +
> >  drivers/net/mv88e61xx.c | 982 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 990 insertions(+)
> >  create mode 100644 drivers/net/mv88e61xx.c
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index a6171a7c7ffd..fc018f5ba47f 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -428,6 +428,13 @@ config LPC32XX_ETH
> >       depends on ARCH_LPC32XX
> >       default y
> >
> > +config MV88E61XX
> > +     bool "Marvell MV88E61xx GbE switch DSA driver"
> > +     depends on DM_DSA && DM_MDIO
> > +     help
> > +       This driver implements a DSA switch driver for the MV88E61xx family
> > +       of GbE switches using the MDIO interface
> > +
> >  config MVGBE
> >       bool "Marvell Orion5x/Kirkwood network interface support"
> >       depends on ARCH_KIRKWOOD || ARCH_ORION5X
> > diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> > index a6d0c23f02d3..11ada73658e9 100644
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -66,6 +66,7 @@ obj-$(CONFIG_MEDIATEK_ETH) += mtk_eth.o
> >  obj-$(CONFIG_MPC8XX_FEC) += mpc8xx_fec.o
> >  obj-$(CONFIG_MT7620_ETH) += mt7620-eth.o
> >  obj-$(CONFIG_MT7628_ETH) += mt7628-eth.o
> > +obj-$(CONFIG_MV88E61XX) += mv88e61xx.o
> >  obj-$(CONFIG_MVGBE) += mvgbe.o
> >  obj-$(CONFIG_MVMDIO) += mvmdio.o
> >  obj-$(CONFIG_MVNETA) += mvneta.o
> > diff --git a/drivers/net/mv88e61xx.c b/drivers/net/mv88e61xx.c
> > new file mode 100644
> > index 000000000000..9dd7a0c7f42e
> > --- /dev/null
> > +++ b/drivers/net/mv88e61xx.c
> > @@ -0,0 +1,982 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) Copyright 2022
> > + * Gateworks Corporation <www.gateworks.com>
> > + * Tim Harvey <tharvey@gateworks.com>
> > + *
> > + * (C) Copyright 2015
> > + * Elecsys Corporation <www.elecsyscorp.com>
> > + * Kevin Smith <kevin.smith@elecsyscorp.com>
> > + *
> > + * Original driver:
> > + * (C) Copyright 2009
> > + * Marvell Semiconductor <www.marvell.com>
> > + * Prafulla Wadaskar <prafulla@marvell.com>
> > + */
> > +
> > +/*
> > + * DSA driver for mv88e61xx ethernet switches.
> > + *
> > + * This driver configures the mv88e61xx for basic use as a DSA switch.
> > + *
> > + * This driver was adapted from drivers/net/phy/mv88e61xx and tested
> > + * on the mv88e6176 via an SGMII interface.
> > + */
> > +
> > +#include <bitfield.h>
> > +#include <common.h>
> > +#include <miiphy.h>
> > +#include <dm/device.h>
> > +#include <dm/device_compat.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/of_extra.h>
> > +#include <linux/delay.h>
> > +#include <net/dsa.h>
> > +
> > +#define PORT_MASK(port_count)                ((1 << (port_count)) - 1)
>
Vladimir,

Thanks for the review.

> Not used.
>

will remove

> > +
> > +/* Device addresses */
> > +#define DEVADDR_PHY(p)                       (p)
> > +#define DEVADDR_SERDES                       0x0F
> > +
> > +/* SMI indirection registers for multichip addressing mode */
> > +#define SMI_CMD_REG                  0x00
> > +#define SMI_DATA_REG                 0x01
> > +
> > +/* Global registers */
> > +#define GLOBAL1_STATUS                       0x00
> > +#define GLOBAL1_CTRL                 0x04
> > +#define GLOBAL1_MON_CTRL             0x1A
> > +
> > +/* Global 2 registers */
> > +#define GLOBAL2_REG_PHY_CMD          0x18
> > +#define GLOBAL2_REG_PHY_DATA         0x19
> > +#define GLOBAL2_REG_SCRATCH          0x1A
> > +
> > +/* Port registers */
> > +#define PORT_REG_STATUS                      0x00
> > +#define PORT_REG_PHYS_CTRL           0x01
> > +#define PORT_REG_SWITCH_ID           0x03
> > +#define PORT_REG_CTRL                        0x04
> > +#define PORT_REG_VLAN_MAP            0x06
> > +#define PORT_REG_VLAN_ID             0x07
> > +#define PORT_REG_LED_CTRL            0x16
> > +
> > +/* Phy registers */
> > +#define PHY_REG_CTRL1                        0x10
> > +#define PHY_REG_STATUS1                      0x11
> > +#define PHY_REG_PAGE                 0x16
> > +
> > +/* Serdes registers */
> > +#define SERDES_REG_CTRL_1            0x10
> > +
> > +/* Phy page numbers */
> > +#define PHY_PAGE_COPPER                      0
> > +#define PHY_PAGE_SERDES                      1
> > +
> > +/* Register fields */
> > +#define GLOBAL1_CTRL_SWRESET         BIT(15)
> > +
> > +#define GLOBAL1_MON_CTRL_CPUDEST_SHIFT       4
> > +#define GLOBAL1_MON_CTRL_CPUDEST_WIDTH       4
> > +
> > +#define PORT_REG_STATUS_SPEED_SHIFT  8
> > +#define PORT_REG_STATUS_SPEED_10     0
> > +#define PORT_REG_STATUS_SPEED_100    1
> > +#define PORT_REG_STATUS_SPEED_1000   2
> > +
> > +#define PORT_REG_STATUS_CMODE_MASK           0xF
> > +#define PORT_REG_STATUS_CMODE_100BASE_X              0x8
> > +#define PORT_REG_STATUS_CMODE_1000BASE_X     0x9
> > +#define PORT_REG_STATUS_CMODE_SGMII          0xa
> > +
> > +#define PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK BIT(15)
> > +#define PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK BIT(14)
> > +#define PORT_REG_PHYS_CTRL_PCS_AN_EN BIT(10)
> > +#define PORT_REG_PHYS_CTRL_PCS_AN_RST        BIT(9)
> > +#define PORT_REG_PHYS_CTRL_FC_VALUE  BIT(7)
> > +#define PORT_REG_PHYS_CTRL_FC_FORCE  BIT(6)
> > +#define PORT_REG_PHYS_CTRL_LINK_VALUE        BIT(5)
> > +#define PORT_REG_PHYS_CTRL_LINK_FORCE        BIT(4)
> > +#define PORT_REG_PHYS_CTRL_DUPLEX_VALUE      BIT(3)
> > +#define PORT_REG_PHYS_CTRL_DUPLEX_FORCE      BIT(2)
> > +#define PORT_REG_PHYS_CTRL_SPD1000   BIT(1)
> > +#define PORT_REG_PHYS_CTRL_SPD100    BIT(0)
> > +#define PORT_REG_PHYS_CTRL_SPD_MASK  (BIT(1) | BIT(0))
> > +
> > +#define PORT_REG_CTRL_PSTATE_SHIFT   0
> > +#define PORT_REG_CTRL_PSTATE_WIDTH   2
> > +
> > +#define PORT_REG_VLAN_ID_DEF_VID_SHIFT       0
> > +#define PORT_REG_VLAN_ID_DEF_VID_WIDTH       12
> > +
> > +#define PORT_REG_VLAN_MAP_TABLE_SHIFT        0
> > +#define PORT_REG_VLAN_MAP_TABLE_WIDTH        11
> > +
> > +#define SERDES_REG_CTRL_1_FORCE_LINK BIT(10)
> > +
> > +/* Field values */
> > +#define PORT_REG_CTRL_PSTATE_DISABLED        0
> > +#define PORT_REG_CTRL_PSTATE_FORWARD 3
> > +
> > +#define PHY_REG_CTRL1_ENERGY_DET_OFF 0
> > +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE 1
> > +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_ONLY  2
> > +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT  3
> > +
> > +/* PHY Status Register */
> > +#define PHY_REG_STATUS1_SPEED                0xc000
> > +#define PHY_REG_STATUS1_GBIT         0x8000
> > +#define PHY_REG_STATUS1_100          0x4000
> > +#define PHY_REG_STATUS1_DUPLEX               0x2000
> > +#define PHY_REG_STATUS1_SPDDONE              0x0800
> > +#define PHY_REG_STATUS1_LINK         0x0400
> > +#define PHY_REG_STATUS1_ENERGY               0x0010
> > +
> > +/*
> > + * Macros for building commands for indirect addressing modes.  These are valid
> > + * for both the indirect multichip addressing mode and the PHY indirection
> > + * required for the writes to any PHY register.
> > + */
> > +#define SMI_BUSY                     BIT(15)
> > +#define SMI_CMD_CLAUSE_22            BIT(12)
> > +#define SMI_CMD_CLAUSE_22_OP_READ    (2 << 10)
> > +#define SMI_CMD_CLAUSE_22_OP_WRITE   (1 << 10)
> > +
> > +#define SMI_CMD_READ                 (SMI_BUSY | SMI_CMD_CLAUSE_22 | \
> > +                                      SMI_CMD_CLAUSE_22_OP_READ)
> > +#define SMI_CMD_WRITE                        (SMI_BUSY | SMI_CMD_CLAUSE_22 | \
> > +                                      SMI_CMD_CLAUSE_22_OP_WRITE)
> > +
> > +#define SMI_CMD_ADDR_SHIFT           5
> > +#define SMI_CMD_ADDR_WIDTH           5
> > +#define SMI_CMD_REG_SHIFT            0
> > +#define SMI_CMD_REG_WIDTH            5
> > +
> > +/* Defines for Scratch and Misc Registers */
> > +#define SCRATCH_UPDATE                       BIT(15)
> > +#define SCRATCH_ADDR_SHIFT           8
> > +#define SCRATCH_ADDR_WIDTH           7
> > +
> > +/* Scratch registers */
> > +#define SCRATCH_ADDR_GPIO0DIR                0x62 /* GPIO[7:0] direction 1=input */
> > +#define SCRATCH_ADDR_GPIO1DIR                0x63 /* GPIO[14:8] direction 1=input */
> > +#define SCRATCH_ADDR_GPIO0DATA               0x64 /* GPIO[7:0] data */
> > +#define SCRATCH_ADDR_GPIO1DATA               0x65 /* GPIO[14:8] data */
> > +#define SCRATCH_ADDR_GPIO0CTRL               0x68 /* GPIO[1:0] control */
> > +#define SCRATCH_ADDR_GPIO1CTRL               0x69 /* GPIO[3:2] control */
> > +
> > +/* ID register values for different switch models */
> > +#define PORT_SWITCH_ID_6020          0x0200
> > +#define PORT_SWITCH_ID_6070          0x0700
> > +#define PORT_SWITCH_ID_6071          0x0710
> > +#define PORT_SWITCH_ID_6096          0x0980
> > +#define PORT_SWITCH_ID_6097          0x0990
> > +#define PORT_SWITCH_ID_6172          0x1720
> > +#define PORT_SWITCH_ID_6176          0x1760
> > +#define PORT_SWITCH_ID_6220          0x2200
> > +#define PORT_SWITCH_ID_6240          0x2400
> > +#define PORT_SWITCH_ID_6250          0x2500
> > +#define PORT_SWITCH_ID_6352          0x3520
> > +
> > +struct mv88e61xx_phy_priv {
>
> Could we choose a better name than mv88e61xx_phy_priv? It is not a PHY,
> it is a switch.

Good point... this was a hold-over from when I was trying to combine
the old and new drivers. Will replace with simple 'mv88e61xx_priv'

>
> > +static int mv88e61xx_fixed_port_setup(struct udevice *dev, u8 port)
> > +{
> > +     struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> > +     struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> > +     int val;
> > +
> > +     val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK |
> > +              PORT_REG_PHYS_CTRL_FC_VALUE |
> > +              PORT_REG_PHYS_CTRL_FC_FORCE);
> > +     val |= PORT_REG_PHYS_CTRL_FC_FORCE |
> > +            PORT_REG_PHYS_CTRL_DUPLEX_VALUE |
> > +            PORT_REG_PHYS_CTRL_DUPLEX_FORCE;
> > +
> > +     if (priv->id == PORT_SWITCH_ID_6071) {
> > +             val |= PORT_REG_PHYS_CTRL_SPD100;
> > +     } else {
> > +             val |= PORT_REG_PHYS_CTRL_PCS_AN_EN |
> > +                    PORT_REG_PHYS_CTRL_PCS_AN_RST |
> > +                    PORT_REG_PHYS_CTRL_SPD1000;
> > +     }
> > +
> > +     if (port == dsa_pdata->cpu_port)
> > +             val |= PORT_REG_PHYS_CTRL_LINK_VALUE |
> > +                    PORT_REG_PHYS_CTRL_LINK_FORCE;
> > +
> > +     return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> > +}
> > +
> > +static int mv88e61xx_set_cpu_port(struct udevice *dev)
> > +{
> > +     struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> > +     struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> > +     int val;
> > +
> > +     /* Set CPUDest */
> > +     val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MON_CTRL);
> > +     if (val < 0)
> > +             return val;
> > +     val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT,
> > +                            GLOBAL1_MON_CTRL_CPUDEST_WIDTH,
> > +                            dsa_pdata->cpu_port);
> > +     val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     /* Enable CPU port */
> > +     val = mv88e61xx_port_enable(dev, dsa_pdata->cpu_port);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     /* If CPU is connected to serdes, initialize serdes */
> > +     if (mv88e61xx_6352_family(dev)) {
> > +             val = mv88e61xx_get_cmode(dev, dsa_pdata->cpu_port);
> > +             if (val < 0)
> > +                     return val;
> > +             if (val == PORT_REG_STATUS_CMODE_100BASE_X ||
> > +                 val == PORT_REG_STATUS_CMODE_1000BASE_X ||
> > +                 val == PORT_REG_STATUS_CMODE_SGMII) {
> > +                     val = mv88e61xx_serdes_init(dev);
>
> Allow me to suggest that the SERDES init function is improperly placed
> in a function called "set_cpu_port".
>
> Don't you need to initialize the SERDES also if a front port uses the
> SERDES, not just the CPU port?
>
> What happens if you initialize it unconditionally?

I will have to look into this further.

>
> > +                     if (val < 0)
> > +                             return val;
> > +             }
> > +     } else {
> > +             val = mv88e61xx_fixed_port_setup(dev, dsa_pdata->cpu_port);
> > +             if (val < 0)
> > +                     return val;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mv88e61xx_switch_init(struct udevice *dev)
> > +{
> > +     static int init;
> > +     int res;
> > +
> > +     if (init)
> > +             return 0;
>
> Odd, why the static int?
>

another hold-over from the original driver and not needed now - will remove

> > +
> > +     res = mv88e61xx_switch_reset(dev);
> > +     if (res < 0)
> > +             return res;
> > +
> > +     res = mv88e61xx_set_cpu_port(dev);
> > +     if (res < 0)
> > +             return res;
> > +
> > +     init = 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int mv88e61xx_phy_setup(struct udevice *dev, u8 phy)
> > +{
> > +     struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> > +     int val;
> > +
> > +     /*
> > +      * Enable energy-detect sensing on PHY, used to determine when a PHY
> > +      * port is physically connected
> > +      */
> > +     val = mv88e61xx_phy_read(dev, phy, PHY_REG_CTRL1);
> > +     if (val < 0)
> > +             return val;
> > +     val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift,
> > +                            priv->phy_ctrl1_en_det_width,
> > +                            priv->phy_ctrl1_en_det_ctrl);
> > +     val = mv88e61xx_phy_write(dev, phy, PHY_REG_CTRL1, val);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * This function is used to pre-configure the required register
> > + * offsets, so that the indirect register access to the PHY registers
> > + * is possible. This is necessary to be able to read the PHY ID
> > + * while driver probing or in get_phy_id(). The globalN register
> > + * offsets must be initialized correctly for a detected switch,
> > + * otherwise detection of the PHY ID won't work!
> > + */
> > +static int mv88e61xx_priv_reg_offs_pre_init(struct udevice *dev)
> > +{
> > +     struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> > +
> > +     /*
> > +      * Initial 'port_reg_base' value must be an offset of existing
> > +      * port register, then reading the ID should succeed. First, try
> > +      * to read via port registers with device address 0x10 (88E6096
> > +      * and compatible switches).
> > +      */
> > +     priv->port_reg_base = 0x10;
> > +     priv->id = mv88e61xx_get_switch_id(dev);
> > +     if (priv->id != 0xfff0) {
> > +             priv->global1 = 0x1B;
> > +             priv->global2 = 0x1C;
> > +             return 0;
> > +     }
> > +
> > +     /*
> > +      * Now try via port registers with device address 0x08
> > +      * (88E6020 and compatible switches).
> > +      */
> > +     priv->port_reg_base = 0x08;
> > +     priv->id = mv88e61xx_get_switch_id(dev);
> > +     if (priv->id != 0xfff0) {
> > +             priv->global1 = 0x0F;
> > +             priv->global2 = 0x07;
> > +             return 0;
> > +     }
> > +
> > +     debug("%s Unknown ID 0x%x\n", __func__, priv->id);
>
> This is the kind of error that is worth more than a "debug" message, no?

agreed - will change to dev_warn()

>
> > +     return -ENODEV;
> > +}
> > +
> > +static int mv88e61xx_dsa_setup_cpu_interface(struct udevice *dev)
> > +{
> > +     struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> > +     int port = dsa_pdata->cpu_port;
> > +     phy_interface_t phy_interface;
> > +     bool duplex, fixed;
> > +     ofnode node;
> > +     int speed;
> > +     int val;
> > +
> > +     fixed = ofnode_phy_is_fixed_link(dsa_pdata->cpu_port_node, &node);
> > +     if (fixed && ofnode_valid(node)) {
> > +             speed = ofnode_read_u32_default(node, "speed", 0);
> > +             duplex = ofnode_read_bool(node, "full-duplex");
> > +     } else {
> > +             duplex = false;
> > +             speed = 0;
> > +     }
> > +     phy_interface = phy_get_interface_by_name(ofnode_read_string(dsa_pdata->cpu_port_node,
> > +                                                                  "phy-mode"));
>
> Isn't this something all too overcomplicated for the driver when you can
> just wait until mv88e61xx_dsa_port_enable() is called on the CPU port,
> and it passes the fixed PHY as argument, with PHY mode and all?

Agreed, I should be able to do this later. I will look into that.

>
> > +
> > +     dev_dbg(dev, "%s configuring CPU port P%d for %s fixed=%d speed=%d duplex=%d\n",
> > +             __func__, port, phy_string_for_interface(phy_interface),
> > +             fixed, speed, duplex);
> > +
> > +     /* Physical Control register: Table 62 */
> > +     val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL);
> > +     val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK ||
> > +              PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK);
> > +     if (phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +             val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK;
> > +     if (phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +             val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK;
> > +     if (fixed)
> > +             val |= BIT(5) | BIT(4); /* Force link */
> > +     switch (speed) {
> > +     case SPEED_10:
> > +             break;
> > +     case SPEED_100:
> > +             val |= 0x1;
> > +             break;
> > +     case SPEED_1000:
> > +             val |= 0x2;
> > +             break;
> > +     default:
> > +             val |= 0x3; /* detect link speed */
> > +             break;
> > +     }
> > +     if (duplex)
> > +             val |= BIT(3) | BIT(2);
> > +
> > +     return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> > +}
> > +
> > +static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, struct phy_device *phy)
> > +{
> > +     struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> > +     struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> > +     int ret;
> > +
> > +     dev_dbg(dev, "%s P%d phy:0x%08x %s\n", __func__, port,
> > +             phy->phy_id, phy_string_for_interface(phy->interface));
> > +
> > +     ret = mv88e61xx_phy_setup(dev, port);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Error setting up PHY %i\n", port);
> > +             return ret;
> > +     }
> > +     ret = mv88e61xx_port_enable(dev, port);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Error enabling PHY %i\n", port);
> > +             return ret;
> > +     }
> > +     if (port != dsa_pdata->cpu_port)
> > +             priv->active_port = port;
> > +     else
> > +             mv88e61xx_set_cpu_port(dev);
>
> Why is there a need to call this twice, first in mv88e61xx_switch_init()
> then here?

I'll refactor the init

>
> > +
> > +     return 0;
> > +}
> > +
> > +static void mv88e61xx_dsa_port_disable(struct udevice *dev, int port, struct phy_device *phy)
> > +{
> > +     struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> > +     int val;
> > +
> > +     dev_dbg(dev, "%s P%d phy:0x%x\n", __func__, port, phy->phy_id);
> > +
> > +     /* can't disable CPU port without re-configuring/re-starting switch */
> > +     if (port == dsa_pdata->cpu_port)
> > +             return;
>
> Interesting, do you know more?

this is likely a lie and some bad cut-and-paste from my ksz9447
driver. Will remove and test

>
> > +
> > +     val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL);
> > +     if (val < 0)
> > +             return;
> > +     val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT,
> > +                            PORT_REG_CTRL_PSTATE_WIDTH,
> > +                            PORT_REG_CTRL_PSTATE_DISABLED);
> > +     val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val);
> > +     if (val < 0)
> > +             return;
> > +}
>
> > +/* bind and probe the switch mdios */
> > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev)
> > +{
> > +     struct udevice *pdev;
> > +     ofnode node, mdios;
> > +     const char *name;
> > +     int ret;
> > +
> > +     /* bind phy ports of mdios child node to mv88e61xx_mdio device */
> > +     mdios = dev_read_subnode(dev, "mdios");
>
> The Linux driver doesn't parse the "mdios" subnode, do you intend to do
> some work in that direction?

We need to bind to all the subnodes or the ports won't probe right? I
have no idea how Linux does it... as we've discussed long ago Linux
doesn't require the additional dt bindings that U-Boot does (see my dt
patch) so there is a lot different.

>
> > +     if (ofnode_valid(mdios)) {
> > +             ofnode_for_each_subnode(node, mdios) {
> > +                     name = ofnode_get_name(node);
> > +                     ret = device_bind_driver_to_node(dev,
> > +                                                      "mv88e61xx_mdio",
> > +                                                      name, node, &pdev);
> > +                     if (ret) {
> > +                             dev_err(dev, "failed to bind %s: %d\n", name, ret);
> > +                             continue;
> > +                     }
> > +
> > +                     /* need to probe it as there is no compatible to do so */
> > +                     ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &pdev);
> > +                     if (ret) {
> > +                             dev_err(dev, "failed to probe %s: %d\n", name, ret);
> > +                             continue;
> > +                     }
> > +             }
> > +     }
> > +
> > +     /* bind the mdios node to the parent mdio driver */
> > +     name = ofnode_get_name(mdios);
> > +     ret = device_bind_driver_to_node(dev,
> > +                                      "mv88e61xx_mdio",
> > +                                      name, mdios, &pdev);
> > +     if (ret)
> > +             dev_err(dev, "failed to bind %s: %d\n", name, ret);
> > +
> > +     /* need to probe it as there is no compatible to do so */
> > +     ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdios, &pdev);
> > +     if (ret)
> > +             dev_err(dev, "failed to probe %s: %d\n", name, ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int mv88e61xx_dsa_probe(struct udevice *dev)
> > +{
> > +     struct mv88e61xx_phy_priv *priv = dev_get_priv(dev);
> > +     struct udevice *master = dsa_get_master(dev);
> > +     int ret;
> > +
> > +     if (!master)
> > +             return -ENODEV;
> > +
> > +     dev_dbg(dev, "%s master:%s\n", __func__, master->name);
> > +     dev_set_parent_priv(dev, priv);
>
> Copy-pasta from the ksz9477 driver?

will remove

>
> > +
> > +     /* probe internal mdio bus */
> > +     ret = mv88e61xx_dsa_probe_mdio(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = mv88e61xx_priv_reg_offs_pre_init(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     dev_dbg(dev, "ID=0x%x PORT_BASE=0x%02x GLOBAL1=0x%02x GLOBAL2=0x%02x\n",
> > +             priv->id, priv->port_reg_base, priv->global1, priv->global2);
> > +     switch (priv->id) {
> > +     case PORT_SWITCH_ID_6096:
> > +     case PORT_SWITCH_ID_6097:
> > +     case PORT_SWITCH_ID_6172:
> > +     case PORT_SWITCH_ID_6176:
> > +     case PORT_SWITCH_ID_6240:
> > +     case PORT_SWITCH_ID_6352:
> > +             priv->port_count = 11;
> > +//           priv->port_stat_link_mask = BIT(11);
> > +//           priv->port_stat_dup_mask = BIT(10);
> > +//           priv->port_stat_speed_width = 2;
>
> No commented code.

yes, will remove

Thanks for the review. I'm not sure when I will be able to get back to
this as I'm out of the office next week and have a lot of other tasks
piled up.

Best Regards,

Tim

>
> > +             priv->phy_ctrl1_en_det_shift = 8;
> > +             priv->phy_ctrl1_en_det_width = 2;
> > +             priv->phy_ctrl1_en_det_ctrl =
> > +                     PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT;
> > +             break;
> > +     case PORT_SWITCH_ID_6020:
> > +     case PORT_SWITCH_ID_6070:
> > +     case PORT_SWITCH_ID_6071:
> > +     case PORT_SWITCH_ID_6220:
> > +     case PORT_SWITCH_ID_6250:
> > +             priv->port_count = 7;
> > +//           priv->port_stat_link_mask = BIT(12);
> > +//           priv->port_stat_dup_mask = BIT(9);
> > +//           priv->port_stat_speed_width = 1;
> > +             priv->phy_ctrl1_en_det_shift = 14;
> > +             priv->phy_ctrl1_en_det_width = 1;
> > +             priv->phy_ctrl1_en_det_ctrl =
> > +                     PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE;
> > +             break;
> > +     default:
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* configure CPU port interface mode */
> > +     ret = mv88e61xx_dsa_setup_cpu_interface(dev);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = mv88e61xx_switch_init(dev);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     dsa_set_tagging(dev, 0, 0);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct udevice_id mv88e61xx_ids[] = {
> > +     { .compatible = "marvell,mv88e6085" },
> > +     { }
> > +};
> > +
> > +U_BOOT_DRIVER(mv88e61xx) = {
> > +     .name           = "mv88e61xx",
> > +     .id             = UCLASS_DSA,
> > +     .of_match       = mv88e61xx_ids,
> > +     .probe          = mv88e61xx_dsa_probe,
> > +     .ops            = &mv88e61xx_dsa_ops,
> > +     .priv_auto      = sizeof(struct mv88e61xx_phy_priv),
> > +};
> > --
> > 2.17.1
> >