[PATCH v5 7/8] net: add MV88E61xx DSA driver
Vladimir Oltean
vladimir.oltean at nxp.com
Wed Oct 5 01:48:24 CEST 2022
Please also rewrite "61xx" from the commit message into "6xxx".
On Tue, Oct 04, 2022 at 09:49:17AM -0700, Tim Harvey wrote:
> Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches.
>
> Cc: Marek BehĂșn <marek.behun at nic.cz>
> Cc: Vladimir Oltean <vladimir.oltean at nxp.com>
> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> Reviewed-by: Vladimir Oltean <vladimir.oltean at nxp.com>
> Reviewed-by: Fabio Estevam <festevam at denx.de>
> ---
> v5:
> - added support for MV88E6320
> - added Fabio's rb tag
> v4:
> - rename to mv88e6xxx
> - sort includes alphabetically
> - remove dsa term from function names
> - reduce indentation level and remove unecessary code in of probe_mdio function
> - rename pdev to mdev to represent mdio device
> v3:
> - Added Vladimir's rb tag
> v2:
> - rebase on v2022.07-rc1 (use ofnode_get_phy_node)
> - remove unused commented out fields from struct
> - remove unused PORT_MASK macro
> - remove phy from priv struct name
> - refactor code from original drivers/net/phy/mv88e61xx with
> suggestions from review to consolidate some functions
> into mv88e61xx_dsa_port_enable
> - remove unecessary skiping of disabling of CPU port
> - remove unecessary dev_set_parent_priv
> - remove unnecessary static init flag
> - replace debug with a dev_warn if switch device-id unsupported
> - remove unnecessary xmit/recv functions as we rely on the fact that
> only a single port is active instead of mangling packets
> ---
> drivers/net/Kconfig | 7 +
> drivers/net/Makefile | 1 +
> drivers/net/mv88e6xxx.c | 833 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 841 insertions(+)
> create mode 100644 drivers/net/mv88e6xxx.c
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 6bbbadc5eef3..5508dacbcc49 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -438,6 +438,13 @@ config KSZ9477
> This driver implements a DSA switch driver for the KSZ9477 family
> of GbE switches using the I2C interface.
>
> +config MV88E6XXX
> + bool "Marvell MV88E6xxx GbE switch DSA driver"
> + depends on DM_DSA && DM_MDIO
> + help
> + This driver implements a DSA switch driver for the MV88E6xxx family
> + of GbE switches using the MDIO interface
I don't think they are all gigabit.
> +
> 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 96b7678e9882..f05fd58e3fa7 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -59,6 +59,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_MV88E6XXX) += mv88e6xxx.o
> obj-$(CONFIG_MVGBE) += mvgbe.o
> obj-$(CONFIG_MVMDIO) += mvmdio.o
> obj-$(CONFIG_MVNETA) += mvneta.o
> diff --git a/drivers/net/mv88e6xxx.c b/drivers/net/mv88e6xxx.c
> new file mode 100644
> index 000000000000..e59e2464c641
> --- /dev/null
> +++ b/drivers/net/mv88e6xxx.c
> +static int mv88e6xxx_switch_reset(struct udevice *dev)
> +{
> + struct mv88e6xxx_priv *priv = dev_get_priv(dev);
> + int time;
> + int val;
> + u8 port;
> +
> + /* Disable all ports */
> + for (port = 0; port < priv->port_count; port++) {
> + val = mv88e6xxx_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 = mv88e6xxx_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 = mv88e6xxx_reg_read(dev, priv->global1, GLOBAL1_CTRL);
> + if (val < 0)
> + return val;
> + val |= GLOBAL1_CTRL_SWRESET;
> + val = mv88e6xxx_reg_write(dev, priv->global1, GLOBAL1_CTRL, val);
> + if (val < 0)
> + return val;
> +
> + /* Wait up to 1 second for switch reset complete */
for switch reset *to* complete
> + for (time = 1000; time; time--) {
Maybe "time_ms"? Or "retries"?
> + val = mv88e6xxx_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 mv88e6xxx_serdes_init(struct udevice *dev)
> +{
> + int val;
> +
> + val = mv88e6xxx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES);
> + if (val < 0)
> + return val;
> +
> + /* Power up serdes module */
> + val = mv88e6xxx_phy_read(dev, DEVADDR_SERDES, MII_BMCR);
> + if (val < 0)
> + return val;
> + val &= ~(BMCR_PDOWN);
> + val = mv88e6xxx_phy_write(dev, DEVADDR_SERDES, MII_BMCR, val);
> + if (val < 0)
> + return val;
> +
> + return 0;
> +}
> +
> +static int mv88e6xxx_fixed_port_setup(struct udevice *dev, u8 port)
> +{
> + struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> + struct mv88e6xxx_priv *priv = dev_get_priv(dev);
> + int val;
> +
> + val = mv88e6xxx_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;
> + }
The speed and duplex are available in struct phy_device which is passed
to mv88e6xxx_port_enable() for all ports, so why hack them?
> +
> + if (port == dsa_pdata->cpu_port)
> + val |= PORT_REG_PHYS_CTRL_LINK_VALUE |
> + PORT_REG_PHYS_CTRL_LINK_FORCE;
> +
> + return mv88e6xxx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> +}
> +
> +/*
> + * 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 mv88e6xxx_priv_reg_offs_pre_init(struct udevice *dev)
> +{
> + struct mv88e6xxx_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 = mv88e6xxx_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 = mv88e6xxx_get_switch_id(dev);
> + if (priv->id != 0xfff0) {
> + priv->global1 = 0x0F;
> + priv->global2 = 0x07;
> + return 0;
> + }
> +
> + dev_warn(dev, "%s Unknown ID 0x%x\n", __func__, priv->id);
> +
> + return -ENODEV;
> +}
> +
> +static int mv88e6xxx_mdio_read(struct udevice *dev, int addr, int devad, int reg)
> +{
> + return mv88e6xxx_phy_read_indirect(dev->parent, DEVADDR_PHY(addr),
> + MDIO_DEVAD_NONE, reg);
> +}
> +
> +static int mv88e6xxx_mdio_write(struct udevice *dev, int addr, int devad,
> + int reg, u16 val)
> +{
> + return mv88e6xxx_phy_write_indirect(dev->parent, DEVADDR_PHY(addr),
> + MDIO_DEVAD_NONE, reg, val);
Please align MDIO_DEVAD_NONE to the open bracket.
> +}
> +
> +static const struct mdio_ops mv88e6xxx_mdio_ops = {
> + .read = mv88e6xxx_mdio_read,
> + .write = mv88e6xxx_mdio_write,
> +};
> +
> +static int mv88e6xxx_mdio_bind(struct udevice *dev)
> +{
> + char name[32];
> + static int num_devices;
> +
> + sprintf(name, "mv88e6xxx-mdio-%d", num_devices++);
> + device_set_name(dev, name);
> +
> + return 0;
> +}
> +
> +U_BOOT_DRIVER(mv88e6xxx_mdio) = {
> + .name = "mv88e6xxx_mdio",
> + .id = UCLASS_MDIO,
> + .ops = &mv88e6xxx_mdio_ops,
> + .bind = mv88e6xxx_mdio_bind,
> + .priv_auto = sizeof(struct mv88e6xxx_priv),
You are not using dev_get_priv(dev) with dev == the MDIO controller
udevice. You are always calling phy_read|write_indirect with dev->parent,
and calling dev_get_priv() on that. So you are allocating private data
for nothing.
> + .plat_auto = sizeof(struct mdio_perdev_priv),
> +};
> +
> +static int mv88e6xxx_port_enable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> + struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> + struct mv88e6xxx_priv *priv = dev_get_priv(dev);
> + int val, ret;
> +
> + dev_dbg(dev, "%s P%d phy:0x%08x %s\n", __func__, port,
> + phy->phy_id, phy_string_for_interface(phy->interface));
> +
> + /*
> + * Enable energy-detect sensing on PHY, used to determine when a PHY
> + * port is physically connected
> + */
What if it's not a PHY port?
> + val = mv88e6xxx_phy_read(dev, port, 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 = mv88e6xxx_phy_write(dev, port, PHY_REG_CTRL1, val);
> + if (val < 0)
> + return val;
> +
> + /* enable port */
> + val = mv88e6xxx_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 = mv88e6xxx_port_write(dev, port, PORT_REG_CTRL, val);
> + if (val < 0)
> + return val;
> +
> + /* configure RGMII delays for fixed link */
> + if (phy->phy_id == PHY_FIXED_ID) {
> + /* Physical Control register: Table 62 */
> + val = mv88e6xxx_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;
> + val |= BIT(5) | BIT(4); /* Force link */
The 5 and the 4 are PORT_REG_PHYS_CTRL_LINK_VALUE | PORT_REG_PHYS_CTRL_LINK_FORCE,
right? This makes either this or this assignment from mv88e6xxx_fixed_port_setup()
redundant:
if (port == dsa_pdata->cpu_port)
val |= PORT_REG_PHYS_CTRL_LINK_VALUE |
PORT_REG_PHYS_CTRL_LINK_FORCE;
I'd prefer you keep the macros as opposed to the magic values, but force
the link unconditionally for all fixed ports, rather than just for the
CPU port. There might come support for cascaded DSA ports in the future,
and this entire shebang will need to be patched otherwise.
> + ret = mv88e6xxx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> + if (ret < 0)
> + return ret;
All of this logic from the "if" branch can be moved into mv88e6xxx_fixed_port_setup().
> +
> + ret = mv88e6xxx_fixed_port_setup(dev, port);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (port == dsa_pdata->cpu_port) {
> + /* Set CPUDest for cpu port */
> + val = mv88e6xxx_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, port);
> + val = mv88e6xxx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val);
> + if (val < 0)
> + return val;
Since you don't use DSA/EDSA tags and you are quite averse to them,
I don't think you need to set the CPUDest.
> +
> + if (mv88e6xxx_6352_family(dev)) {
> + /* initialize serdes */
> + val = mv88e6xxx_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 = mv88e6xxx_serdes_init(dev);
> + if (val < 0)
> + return val;
> + }
You can/should initialize the SERDES regardless of whether this is the
CPU port or not, if the C_MODE dictates so. Otherwise said, the
"if (port == dsa_pdata->cpu_port)" check should disappear.
Additionally, you should validate that phy->interface matches what the
C_MODE reports for this port (I understand that changing the C_MODE is
possible but not exactly without its issues), and report an error on
mismatch, to avoid people scratching their heads.
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void mv88e6xxx_port_disable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> + int val;
> +
> + dev_dbg(dev, "%s P%d phy:0x%x\n", __func__, port, phy->phy_id);
> +
> + val = mv88e6xxx_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 = mv88e6xxx_port_write(dev, port, PORT_REG_CTRL, val);
> + if (val < 0)
> + return;
The assignment of "val" and then its checking is quite useless here,
either print an error or remove it.
> +}
> +
> +static const struct dsa_ops mv88e6xxx_dsa_ops = {
> + .port_enable = mv88e6xxx_port_enable,
> + .port_disable = mv88e6xxx_port_disable,
> +};
> +
> +/* bind and probe the switch mdios */
> +static int mv88e6xxx_probe_mdio(struct udevice *dev)
> +{
> + struct udevice *mdev;
> + ofnode node, mdios;
> + const char *name;
> + int ret;
> +
> + /* bind phy ports of mdios child node to mv88e6xxx_mdio device */
> + mdios = dev_read_subnode(dev, "mdios");
> + if (!ofnode_valid(mdios))
> + return 0;
I wrote a long, angry comment about the parsing of this "mdios"
container node. Please squash the fixup from patch 8/8 into the code
here, I just wasted 10 minutes which I'm not getting back.
> +
> + ofnode_for_each_subnode(node, mdios) {
> + name = ofnode_get_name(node);
> + ret = device_bind_driver_to_node(dev,
> + "mv88e6xxx_mdio",
> + name, node, NULL);
> + 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, &mdev);
> + if (ret) {
> + dev_err(dev, "failed to probe %s: %d\n", name, ret);
> + continue;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int mv88e6xxx_probe(struct udevice *dev)
> +{
> + struct mv88e6xxx_priv *priv = dev_get_priv(dev);
> + struct udevice *master = dsa_get_master(dev);
> + int ret;
> +
> + if (!master)
> + return -ENODEV;
Remove this.
You could consider treating the 'status = "disabled";' case, however.
Look at sja1105_probe() for example.
> +
> + dev_dbg(dev, "%s master:%s\n", __func__, master->name);
Useless variable, please remove it and stop looking at it in probe().
> +
> + /* probe internal mdio bus */
> + ret = mv88e6xxx_probe_mdio(dev);
> + if (ret)
> + return ret;
> +
> + ret = mv88e6xxx_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->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:
> + case PORT_SWITCH_ID_6320:
> + priv->port_count = 7;
> + 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;
> + }
> +
> + ret = mv88e6xxx_switch_reset(dev);
> + if (ret < 0)
> + return ret;
> +
> + dsa_set_tagging(dev, 0, 0);
Just don't call this, it doesn't do anything.
> +
> + return 0;
> +}
> +
> +static const struct udevice_id mv88e6xxx_ids[] = {
> + { .compatible = "marvell,mv88e6085" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(mv88e6xxx) = {
> + .name = "mv88e6xxx",
> + .id = UCLASS_DSA,
> + .of_match = mv88e6xxx_ids,
> + .probe = mv88e6xxx_probe,
> + .ops = &mv88e6xxx_dsa_ops,
> + .priv_auto = sizeof(struct mv88e6xxx_priv),
> +};
> --
> 2.25.1
>
More information about the U-Boot
mailing list