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#L12... https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/...

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

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#L12... https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/...

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

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.

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