On Wed, Oct 12, 2022 at 12:50 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
On Mon, Oct 10, 2022 at 09:39:43AM -0700, Tim Harvey wrote:
Add a DSA driver for the MV88E6xxx compatible Ethernet switches.
Cc: Marek BehĂșn marek.behun@nic.cz Cc: Vladimir Oltean vladimir.oltean@nxp.com Signed-off-by: Tim Harvey tharvey@gateworks.com Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com Reviewed-by: Fabio Estevam festevam@denx.de
v6:
- update commit msg (s/MV88E61xx/MV88E6xxx)
really?
will fix in v7 - I updated the long msg and missed updating the short msg
- remove GbE from commit msg and Kconfig desc
- squash accidental change from patch 7 into patch 6
- added error print on failure to read switch id
- mv88e6xxx_probe:
- check for switch enabled
- remove unused variable enabled
- remove unnecessary call to dsa_set_tagging
- port_probe:
- new function to configure phy features by calling phy_config
- port_enable:
- only enable energy-detect sensing on phy ports
- add phy/cmode verification mistmatch warning
- remove mv88e6xxx_fixed_port_setup()
- always force link up for fixed ports
- always set SERDES mode regardless of cpu port
really?
The SERDES configuration on the 88E6352/88E6240/88E6176/88E6172 (which I have documentation for) is not in port specific registers so I think I should move that to the probe function for the switch.
+struct mv88e6xxx_priv {
int smi_addr;int id;int port_count; /* Number of switch ports */int port_reg_base; /* Base of the switch port registers */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)
If the convention in U-Boot is the same as in Linux (which it well may be, except for the fact that more things escape review), then you should avoid manually defining functions as inline in C files. The compiler will decide whether to inline or not regardless of what you specify here.
Although I wonder if these couldn't be in fact expressed more clearly using plain, boring old logical | and << operators as part of a macro.
The bitfield_replace came from the original drivers/net/phy/mv88e61xx.c but I also find it easier to read logical | and << operators so I'll change to that throughout for v7
+{
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 mv88e6xxx_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;+}
+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_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*/if (port != dsa_pdata->cpu_port) {Can you test based on phy->interface == PHY_INTERFACE_MODE_INTERNAL? Support may come later for cascade ports, and those are ports without an internal PHY which are not CPU ports.
In my config there are no ports with interface == PHY_INTERFACE_MODE_INTERNAL so I'm not sure what you mean here. What situations does that come up in?
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) {I believe this code should really be executed only for RGMII ports. In other words, move it inside a function that gets called inside the switch (phy->interface) block you added below.
It makes sense to only do this for RGMII modes but I'll need to take it out of the if (mv88e61xx_6352_famiily()) as it looks like other switches that could be supported by this driver support RGMII interfaces.
/* Physical Control register: Table 62 */val = mv88e6xxx_port_read(dev, port, PORT_REG_PHYS_CTRL);/* RGMII delays */val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK ||PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK);I find the style a bit chaotic when in other places you use bitfield_replace() but not everywhere.
I'll get rid of bitfield_replace and leave these as is
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;/* Force Link */val |= PORT_REG_PHYS_CTRL_LINK_VALUE |PORT_REG_PHYS_CTRL_LINK_FORCE;ret = mv88e6xxx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);if (ret < 0)return ret;}if (mv88e6xxx_6352_family(dev)) {val = mv88e6xxx_get_cmode(dev, dsa_pdata->cpu_port);What gets handled by dsa_ops :: port_enable() should be a function of "int port", whereas dsa_pdata->cpu_port is an invariant of that.
Otherwise said, you check for the cmode of the CPU port for as many times as ports there are, and never for the other ports. Probably not what you intend.
The original mv88e61xx driver initializes SERDES in mv88e61xx_set_cpu_port() called from mv88e61xx_switch_init() called from mv88e61xx_phy_config() for the cpu port PHY, only does this for the 6172/6176/6240/6352, and the registers are not port specific:
/* If CPU is connected to serdes, initialize serdes */ if (mv88e61xx_6352_family(phydev)) { val = mv88e61xx_get_cmode(phydev, CONFIG_MV88E61XX_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(phydev); if (val < 0) return val; } }
static int mv88e61xx_serdes_init(struct phy_device *phydev) { int val;
val = mv88e61xx_set_page(phydev, DEVADDR_SERDES, PHY_PAGE_SERDES); if (val < 0) return val;
/* Power up serdes module */ val = mv88e61xx_phy_read(phydev, DEVADDR_SERDES, MII_BMCR); if (val < 0) return val; val &= ~(BMCR_PDOWN); val = mv88e61xx_phy_write(phydev, DEVADDR_SERDES, MII_BMCR, val); if (val < 0) return val;
return 0; }
So I think the proper thing to do is move this SERDES init into my mv88e6xxx_probe function.
if (val < 0)return val;/* initialize serdes */if (val == PORT_REG_STATUS_CMODE_100BASE_X ||val == PORT_REG_STATUS_CMODE_1000BASE_X ||val == PORT_REG_STATUS_CMODE_SGMII) {ret = mv88e6xxx_serdes_init(dev);Again, you call mv88e6xxx_serdes_init() 10 times on the CPU port. The point of getting the C_Mode per port was to avoid directly checking for dsa_pdata->cpu_port.
if (ret < 0)return ret;}/* validate interface type */if (phy->phy_id == PHY_FIXED_ID) {I think you mean here to validate the C_Mode only for ports not connected to internal PHYs. But SERDES/RGMII ports may also have PHYs, albeit external. You want to validate the C_Mode for them too. So this check must go. Please check for PHY_INTERFACE_MODE_INTERNAL, and avoid comparing it to the C_Mode in that case.
I think my flaw here was that I checked c_mode for the cpu port above as you pointed out:
val = mv88e6xxx_get_cmode(dev, dsa_pdata->cpu_port);
Changing that to mv88e6xxx_get_cmode(dev, port) should resolve that but again I'm not sure where I would be seeing an interface mode of PHY_INTERFACE_MODE_INTERNAL
switch (phy->interface) {case PHY_INTERFACE_MODE_RGMII:case PHY_INTERFACE_MODE_RGMII_RXID:case PHY_INTERFACE_MODE_RGMII_TXID:case PHY_INTERFACE_MODE_RGMII_ID:if (val != PORT_REG_STATUS_CMODE_RGMII)goto mismatch;break;case PHY_INTERFACE_MODE_1000BASEX:if (val != PORT_REG_STATUS_CMODE_1000BASE_X)goto mismatch;break;+mismatch:
default:dev_err(dev, "Mismatched PHY mode %s on port %d!\n",phy_string_for_interface(phy->interface), port);break;}}}return phy_startup(phy);+}
Overall it looks better than v5, but I believe that it could use a bit more attention to detail.
Also, I don't work with Marvell hardware, so I'm not all that familiar with it. I understand this only supports the SERDES as seen on the 6176 (where there is a single SERDES port), but I would appreciate if someone could take a second look at the hardware configuration in the physical interface area.
The only hardware I have access to for testing has a MV88E6176 and the only register documentionat I have is from the '88E6352/88E6240/88E6176/88E6172 Functional Specification' which is what the driver considers the '6352 family' of Gigabit Ethernet switches. I did manage to find a Marvell 2020 Product Selector Guide [1] that does give an outline of the various switches between pages 15 and 17 but without register definitions for the other switches all I have to go off of is the original u-boot driver and perhaps the Linux driver.
Best Regards,
Tim [1] https://jp.marvell.com/content/dam/marvell/en/psg/marvell_psg.pdf