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

Tim Harvey tharvey at gateworks.com
Thu Apr 14 23:30:25 CEST 2022


On Tue, Apr 12, 2022 at 7:13 AM Vladimir Oltean <vladimir.oltean at 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 at 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 at gateworks.com>
> > + *
> > + * (C) Copyright 2015
> > + * Elecsys Corporation <www.elecsyscorp.com>
> > + * Kevin Smith <kevin.smith at elecsyscorp.com>
> > + *
> > + * Original driver:
> > + * (C) Copyright 2009
> > + * Marvell Semiconductor <www.marvell.com>
> > + * Prafulla Wadaskar <prafulla at 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
> >


More information about the U-Boot mailing list