Re: [PATCH RFC net-next 3/3] net: dsa: add basic initial driver for MxL862xx switches

[Daniel Golle <daniel@makrotopia.org>]

Hi Andrew,

thank you for the detailed review.
See my replies inline below

On Wed, Dec 03, 2025 at 03:07:20AM +0100, Andrew Lunn wrote:
> > [...]
> > +struct mxl862xx_ctp_port_assignment {
> > +	u8 logical_port_id;
> > +	__le16 first_ctp_port_id;
> > +	__le16 number_of_ctp_port;
> > +	enum mxl862xx_logical_port_mode mode;
> > +	__le16 bridge_port_id;
> > +} __packed;
> 
> Does the C standard define the size of an enum? Do you assume this is
> a byte?

Yes, it needs to be a byte, and I guess the best is to then just use
u8 type here and cast to u8 on assignment.

> > + * struct mxl862xx_sys_fw_image_version - VLAN counter mapping configuration
> 
> The text seems wrong, probably cut/paste error?

Yes, I agree ;)

> 
> > +#define MXL862XX_MMD_DEV 30
> 
> Please use MDIO_MMD_VEND1

Ack.

> 
> > +#define MXL862XX_MMD_REG_CTRL 0
> > +#define MXL862XX_MMD_REG_LEN_RET 1
> > +#define MXL862XX_MMD_REG_DATA_FIRST 2
> > +#define MXL862XX_MMD_REG_DATA_LAST 95
> > +#define MXL862XX_MMD_REG_DATA_MAX_SIZE \
> > +		(MXL862XX_MMD_REG_DATA_LAST - MXL862XX_MMD_REG_DATA_FIRST + 1)
> > +
> > +#define MMD_API_SET_DATA_0 (0x0 + 0x2)
> > +#define MMD_API_GET_DATA_0 (0x0 + 0x5)
> > +#define MMD_API_RST_DATA (0x0 + 0x8)
> 
> What is the significant of these numbers? Can you use #defines to make
> it clearer?

I'll simplify this to be

#define MMD_API_SET_DATA		2
#define MMD_API_GET_DATA		5
#define MMD_API_RST_DATA		8

which makes it more clear I hope.

> 
> > +
> > +static int mxl862xx_busy_wait(struct mxl862xx_priv *dev)
> > +{
> > +	int ret, i;
> > +
> > +	for (i = 0; i < MAX_BUSY_LOOP; i++) {
> > +		ret = __mdiobus_c45_read(dev->bus, dev->sw_addr,
> > +					 MXL862XX_MMD_DEV,
> > +					 MXL862XX_MMD_REG_CTRL);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret & CTRL_BUSY_MASK)
> > +			usleep_range(10, 15);
> > +		else
> > +			return 0;
> > +	}
> > +
> > +	return -ETIMEDOUT;
> 
> We already have phy_read_mmd_poll_timeout(). Maybe you should add a
> __mdiobus_c45_read_poll_timeout()?
> 
> Also, as far as i can see, __mdiobus_c45_read() is always called with
> the same first three parameters. Maybe add a mxl862xx_reg_read()?  and
> a mxl862xx_reg_write()?

Imho it would be nice to introduce unlock __mdiodev_c45_* helpers in
include/linux/mdio.h, ie.

static inline int __mdiodev_c45_read(struct mdio_device *mdiodev, int devad,
				     u16 regnum)
{
	return __mdiobus_c45_read(mdiodev->bus, mdiodev->addr, devad, regnum);
}

static inline int __mdiodev_c45_write(struct mdio_device *mdiodev, u32 devad,
				      u16 regnum, u16 val)
{
	return __mdiobus_c45_write(mdiodev->bus, mdiodev->addr, devad, regnum,
				   val);
}

and then have something like this in mxl862xx-host.c:

static int mxl862xx_reg_read(struct mxl862xx_priv *priv, u32 addr)
{
	return __mdiodev_c45_read(priv->mdiodev, MDIO_MMD_VEND1, addr);
}

static int mxl862xx_reg_write(struct mxl862xx_priv *priv, u32 addr, u16 data)
{
	return __mdiodev_c45_write(priv->mdiodev, MDIO_MMD_VEND1, addr, data);
}

static int mxl862xx_ctrl_read(struct mxl862xx_priv *priv)
{
	return mxl862xx_reg_read(priv, MXL862XX_MMD_REG_CTRL);
}

static int mxl862xx_busy_wait(struct mxl862xx_priv *priv)
{
	int val;

	return readx_poll_timeout(mxl862xx_ctrl_read, priv, val,
				  !(val & CTRL_BUSY_MASK), 15, 10000);
}

Do you agree?

> > [...]
> > +	if (result < 0) {
> > +		ret = result;
> > +		goto out;
> > +	}
> 
> If i'm reading mxl862xx_send_cmd() correct, result is the value of a
> register. It seems unlikely this is a Linux error code?

Only someone with insights into the use of error codes by the uC
firmware can really answer that. However, as also Russell pointed out,
the whole use of s16 here with negative values being interpreted as
errors is fishy here, because in the end this is also used to read
registers from external MDIO connected PHYs which may return arbitrary
16-bit values...
Someone in MaxLinear will need to clarify here.

> 
> > +
> > +	for (i = 0; i < max && read; i++) {
> 
> That is an unusual way to use read.

Ack. I suggest

if (!read)
	goto out;

> > +#define MXL862XX_API_WRITE(dev, cmd, data) \
> > +	mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), false)
> > +#define MXL862XX_API_READ(dev, cmd, data) \
> > +	mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true)
> 
> > +/* PHY access via firmware relay */
> > +static int mxl862xx_phy_read_mmd(struct mxl862xx_priv *priv, int port,
> > +				 int devadd, int reg)
> > +{
> > +	struct mdio_relay_data param = {
> > +		.phy = port,
> > +		.mmd = devadd,
> > +		.reg = reg & 0xffff,
> > +	};
> > +	int ret;
> > +
> > +	ret = MXL862XX_API_READ(priv, INT_GPHY_READ, param);
> 
> That looks a bit ugly, using a macro as a function name. I would
> suggest tiny functions rather than macros. The compiler should do the
> right thing.

The thing is that the macro way allows to use MXL862XX_API_* on
arbitrary types, such as the packed structs. Using a function would
require the type of the parameter to be defined, which would result
in a lot of code duplication in this case.

> 
> > +/* Configure CPU tagging */
> > +static int mxl862xx_configure_tag_proto(struct dsa_switch *ds, int port,
> > +					bool enable)
> > +{
> > +	struct mxl862xx_ss_sp_tag tag = {
> > +		.pid = DSA_MXL_PORT(port),
> > +		.mask = BIT(0) | BIT(1),
> > +		.rx = enable ? 2 : 1,
> > +		.tx = enable ? 2 : 3,
> > +	};
> 
> There is a bit comment at the beginning of the patch about these, but
> it does not help much here. Please could you add some #defines for
> these magic numbers.

Ack, I'll do that.

> 
> > +/* Reset switch via MMD write */
> > +static int mxl862xx_mmd_write(struct dsa_switch *ds, int reg, u16 data)
> 
> The comment does not fit what the function does.

Ooops...

> 
> > +{
> > +	struct mxl862xx_priv *priv = ds->priv;
> > +	int ret;
> > +
> > +	mutex_lock(&priv->bus->mdio_lock);
> > +	ret = __mdiobus_c45_write(priv->bus, priv->sw_addr, MXL862XX_MMD_DEV,
> > +				  reg, data);
> > +	mutex_unlock(&priv->bus->mdio_lock);
> 
> There is no point using the unlocked version if you wrap it in
> lock/unlock...

I'll throw all this out and instead introduce a high-level reset
function in mxl8622xx-host.c, in that way all MDIO host access is kept
in mxl8622xx-host.c.

> > [...]
> > +	/* Software reset */
> > +	ret = mxl862xx_mmd_write(ds, 1, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mxl862xx_mmd_write(ds, 0, 0x9907);
> 
> More magic numbers.

Maybe someone in MaxLinear can demystify this?