Re: [PATCH net-next v6 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

["Russell King (Oracle)" <linux@armlinux.org.uk>]

Hi,

Thanks for fixing the major phylink implementation errors. There are
further issues that need addressing.

On Sun, Aug 24, 2025 at 08:51:11AM +0800, David Yang wrote:
> +/******** hardware definitions ********/

...

> +#define YT921X_REG_END			0x400000  /* as long as reg space is below this */

Please consider moving the above register definitions to, e.g.
drivers/net/dsa/yt921x-hw.h Also consider whether some of the below
should also be moved there.

> +#define YT921X_TAG_LEN			8
> +
> +#define YT921X_EDATA_EXTMODE		0xfb
> +#define YT921X_EDATA_LEN		0x100
> +
> +#define YT921X_FDB_NUM		4096
> 
> +enum yt921x_fdb_entry_status {
> +	YT921X_FDB_ENTRY_STATUS_INVALID = 0,
> +	YT921X_FDB_ENTRY_STATUS_MIN_TIME = 1,
> +	YT921X_FDB_ENTRY_STATUS_MOVE_AGING_MAX_TIME = 3,
> +	YT921X_FDB_ENTRY_STATUS_MAX_TIME = 5,
> +	YT921X_FDB_ENTRY_STATUS_PENDING = 6,
> +	YT921X_FDB_ENTRY_STATUS_STATIC = 7,
> +};
> +
> +#define YT921X_PVID_DEFAULT		1
> +
> +#define YT921X_FRAME_SIZE_MAX		0x2400  /* 9216 */
> +
> +#define YT921X_RST_DELAY_US		10000
> +
> +struct yt921x_mib_desc {
> +	unsigned int size;
> +	unsigned int offset;
> +	const char *name;
> +};

Maybe consider moving the struct definitions (of which there are
several) to drivers/net/dsa/yt921x.h ?

> +/******** eee ********/
> +
> +static int
> +yt921x_set_eee(struct yt921x_priv *priv, int port, struct ethtool_keee *e)
> +{
> +	struct device *dev = to_device(priv);
> +	bool enable = e->eee_enabled;
> +	u16 new_mask;
> +	int res;
> +
> +	dev_dbg(dev, "%s: port %d, enable %d\n", __func__, port, enable);
> +
> +	/* Enable / disable global EEE */
> +	new_mask = priv->eee_ports_mask;
> +	new_mask &= ~BIT(port);
> +	new_mask |= !enable ? 0 : BIT(port);
> +
> +	if (!!new_mask != !!priv->eee_ports_mask) {
> +		dev_dbg(dev, "%s: toggle %d\n", __func__, !!new_mask);
> +
> +		res = yt921x_smi_toggle_bits(priv, YT921X_PON_STRAP_FUNC,
> +					     YT921X_PON_STRAP_EEE, !!new_mask);
> +		if (res)
> +			return res;
> +		res = yt921x_smi_toggle_bits(priv, YT921X_PON_STRAP_VAL,
> +					     YT921X_PON_STRAP_EEE, !!new_mask);
> +		if (res)
> +			return res;

Here, if EEE is completely disabled, you clear the YT921X_PON_STRAP_EEE
bit...

> +static bool yt921x_dsa_support_eee(struct dsa_switch *ds, int port)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +
> +	return (priv->pon_strap_cap & YT921X_PON_STRAP_EEE) != 0;

... and if this bit is clear, you report that EEE is unsupported by the
device - which means the device has no hardware for EEE support, and
the ethtool EEE operations will be blocked and return -EOPNOTSUPP. This
means that once all ports have EEE disabled, EEE can not be re-enabled
except through hardware reset.

Please see the code in net/dsa/user.c::dsa_user_set_eee().

> +/******** port ********/
> +
> +static int yt921x_port_down(struct yt921x_priv *priv, int port)
> +{
> +	u32 mask;
> +	u32 ctrl;
> +	int res;
> +
> +	ctrl = YT921X_PORT_LINK | YT921X_PORT_RX_MAC_EN | YT921X_PORT_TX_MAC_EN;
> +	res = yt921x_smi_clear_bits(priv, YT921X_PORTn_CTRL(port), ctrl);
> +	if (res)
> +		return res;
> +
> +	if (yt921x_port_is_external(port)) {
> +		ctrl = YT921X_SGMII_LINK;
> +		res = yt921x_smi_clear_bits(priv, YT921X_SGMIIn(port), ctrl);
> +		if (res)
> +			return res;
> +
> +		mask = YT921X_XMII_LINK;
> +		res = yt921x_smi_clear_bits(priv, YT921X_XMIIn(port), ctrl);
> +		if (res)
> +			return res;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
> +	       phy_interface_t interface, int speed, int duplex,
> +	       bool tx_pause, bool rx_pause)
> +{
> +	struct device *dev = to_device(priv);
> +	u32 mask;
> +	u32 ctrl;
> +	int res;
> +
> +	switch (speed) {
> +	case SPEED_10:
> +		ctrl = YT921X_PORT_SPEED_10;
> +		break;
> +	case SPEED_100:
> +		ctrl = YT921X_PORT_SPEED_100;
> +		break;
> +	case SPEED_1000:
> +		ctrl = YT921X_PORT_SPEED_1000;
> +		break;
> +	case SPEED_10000:
> +		ctrl = YT921X_PORT_SPEED_10000;
> +		break;
> +	case SPEED_2500:
> +		ctrl = YT921X_PORT_SPEED_2500;
> +		break;
> +	default:
> +		if (speed >= 0)
> +			dev_err(dev, "Unsupported speed %d\n", speed);
> +		break;
> +	}
> +	if (duplex == DUPLEX_FULL)
> +		ctrl |= YT921X_PORT_DUPLEX_FULL;
> +	if (tx_pause)
> +		ctrl |= YT921X_PORT_TX_PAUSE;
> +	if (rx_pause)
> +		ctrl |= YT921X_PORT_RX_PAUSE;
> +	ctrl |= YT921X_PORT_LINK | YT921X_PORT_RX_MAC_EN |
> +		YT921X_PORT_TX_MAC_EN;
> +	res = yt921x_smi_write(priv, YT921X_PORTn_CTRL(port), ctrl);
> +	if (res)
> +		return res;
> +
> +	if (yt921x_port_is_external(port)) {
> +		mask = YT921X_SGMII_SPEED_M;
> +		switch (speed) {
> +		case SPEED_10:
> +			ctrl = YT921X_SGMII_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			ctrl = YT921X_SGMII_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			ctrl = YT921X_SGMII_SPEED_1000;
> +			break;
> +		case SPEED_10000:
> +			ctrl = YT921X_SGMII_SPEED_10000;
> +			break;
> +		case SPEED_2500:
> +			ctrl = YT921X_SGMII_SPEED_2500;
> +			break;
> +		}
> +		mask |= YT921X_SGMII_DUPLEX_FULL;
> +		if (duplex == DUPLEX_FULL)
> +			ctrl |= YT921X_SGMII_DUPLEX_FULL;
> +		mask |= YT921X_SGMII_TX_PAUSE;
> +		if (tx_pause)
> +			ctrl |= YT921X_SGMII_TX_PAUSE;
> +		mask |= YT921X_SGMII_RX_PAUSE;
> +		if (rx_pause)
> +			ctrl |= YT921X_SGMII_RX_PAUSE;
> +		mask |= YT921X_SGMII_LINK;
> +		ctrl |= YT921X_SGMII_LINK;
> +		res = yt921x_smi_update_bits(priv, YT921X_SGMIIn(port),
> +					     mask, ctrl);
> +		if (res)
> +			return res;
> +
> +		ctrl = YT921X_XMII_LINK;
> +		res = yt921x_smi_set_bits(priv, YT921X_XMIIn(port), ctrl);
> +		if (res)
> +			return res;
> +
> +		switch (speed) {
> +		case SPEED_10:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_1000;
> +			break;
> +		case SPEED_10000:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_10000;
> +			break;
> +		case SPEED_2500:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_2500;
> +			break;
> +		}
> +		if (duplex == DUPLEX_FULL)
> +			ctrl |= YT921X_MDIO_POLLING_DUPLEX_FULL;
> +		ctrl |= YT921X_MDIO_POLLING_LINK;
> +		res = yt921x_smi_write(priv, YT921X_MDIO_POLLINGn(port), ctrl);
> +		if (res)
> +			return res;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +yt921x_port_config(struct yt921x_priv *priv, int port, unsigned int mode,
> +		   phy_interface_t interface)
> +{
> +	struct device *dev = to_device(priv);
> +	u32 mask;
> +	u32 ctrl;
> +	int res;
> +
> +	if (!yt921x_port_is_external(port)) {
> +		if (interface != PHY_INTERFACE_MODE_INTERNAL) {
> +			dev_err(dev, "Wrong mode %d on port %d\n",
> +				interface, port);
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}
> +
> +	switch (interface) {
> +	/* SGMII */
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_100BASEX:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		res = yt921x_smi_set_bits(priv, YT921X_SGMII_CTRL,
> +					  YT921X_SGMII_CTRL_PORTn(port));
> +		if (res)
> +			return res;
> +		res = yt921x_smi_clear_bits(priv, YT921X_XMII_CTRL,
> +					    YT921X_XMII_CTRL_PORTn(port));
> +		if (res)
> +			return res;
> +
> +		mask = YT921X_SGMII_MODE_M;
> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_SGMII:
> +			ctrl = YT921X_SGMII_MODE_SGMII_PHY;
> +			break;
> +		case PHY_INTERFACE_MODE_100BASEX:
> +			ctrl = YT921X_SGMII_MODE_100BASEX;
> +			break;
> +		case PHY_INTERFACE_MODE_1000BASEX:
> +			ctrl = YT921X_SGMII_MODE_1000BASEX;
> +			break;
> +		case PHY_INTERFACE_MODE_2500BASEX:
> +			ctrl = YT921X_SGMII_MODE_2500BASEX;
> +			break;
> +		default:
> +			WARN_ON(1);
> +			break;
> +		}
> +		res = yt921x_smi_update_bits(priv, YT921X_SGMIIn(port),
> +					     mask, ctrl);
> +		if (res)
> +			return res;
> +
> +		break;
> +	/* XMII */
> +	case PHY_INTERFACE_MODE_MII:
> +	case PHY_INTERFACE_MODE_REVMII:
> +	case PHY_INTERFACE_MODE_RMII:
> +	case PHY_INTERFACE_MODE_REVRMII:
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +	case PHY_INTERFACE_MODE_INTERNAL:
> +		/* TODO */
> +		dev_err(dev, "Untested mode %d\n", interface);
> +		return -EINVAL;
> +
> +		res = yt921x_smi_clear_bits(priv, YT921X_SGMII_CTRL,
> +					    YT921X_SGMII_CTRL_PORTn(port));
> +		if (res)
> +			return res;
> +		res = yt921x_smi_set_bits(priv, YT921X_XMII_CTRL,
> +					  YT921X_XMII_CTRL_PORTn(port));
> +		if (res)
> +			return res;
> +
> +		mask = YT921X_XMII_EN | YT921X_XMII_MODE_M;
> +		ctrl = YT921X_XMII_EN;
> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_MII:
> +			ctrl |= YT921X_XMII_MODE_MII;
> +			break;
> +		case PHY_INTERFACE_MODE_REVMII:
> +			ctrl |= YT921X_XMII_MODE_REVMII;
> +			break;
> +		case PHY_INTERFACE_MODE_RMII:
> +			ctrl |= YT921X_XMII_MODE_RMII;
> +			break;
> +		case PHY_INTERFACE_MODE_REVRMII:
> +			ctrl |= YT921X_XMII_MODE_REVRMII;
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII:
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +		case PHY_INTERFACE_MODE_RGMII_RXID:
> +		case PHY_INTERFACE_MODE_RGMII_TXID:
> +			ctrl |= YT921X_XMII_MODE_RGMII;
> +			break;
> +		case PHY_INTERFACE_MODE_INTERNAL:
> +			ctrl |= YT921X_XMII_MODE_DISABLE;
> +			break;
> +		default:
> +			WARN_ON(1);
> +			break;
> +		}
> +		res = yt921x_smi_update_bits(priv, YT921X_XMIIn(port),
> +					     mask, ctrl);
> +		if (res)
> +			return res;
> +
> +		/* TODO: RGMII delay */
> +
> +		break;
> +	default:
> +		WARN_ON(1);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +yt921x_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> +			     phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	int port = dp->index;
> +	int res;
> +
> +	dev_dbg(dev, "%s: port %d\n", __func__, port);
> +
> +	yt921x_smi_acquire(priv);
> +	res = yt921x_port_down(priv, port);
> +	yt921x_smi_release(priv);
> +
> +	if (res)
> +		dev_err(dev, "%s: port %d: %i\n", __func__, port, res);
> +}
> +
> +static void
> +yt921x_phylink_mac_link_up(struct phylink_config *config,
> +			   struct phy_device *phydev, unsigned int mode,
> +			   phy_interface_t interface, int speed, int duplex,
> +			   bool tx_pause, bool rx_pause)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	int port = dp->index;
> +	int res;
> +
> +	dev_dbg(dev,
> +		"%s: port %d, mode %u, interface %d, speed %d, duplex %d, "
> +		"tx_pause %d, rx_pause %d\n", __func__, port, mode, interface,
> +		speed, duplex, tx_pause, rx_pause);
> +
> +	yt921x_smi_acquire(priv);
> +	res = yt921x_port_up(priv, port, mode, interface, speed, duplex,
> +			     tx_pause, rx_pause);
> +	yt921x_smi_release(priv);
> +
> +	if (res)
> +		dev_err(dev, "%s: port %d: %i\n", __func__, port, res);
> +}
> +
> +static void
> +yt921x_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> +			  const struct phylink_link_state *state)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	int port = dp->index;
> +	int res;
> +
> +	dev_dbg(dev, "%s: port %d, mode %u, interface %d\n",
> +		__func__, port, mode, state->interface);
> +
> +	yt921x_smi_acquire(priv);
> +	res = yt921x_port_config(priv, port, mode, state->interface);
> +	yt921x_smi_release(priv);
> +	if (res)
> +		dev_err(dev, "%s: port %d: %i\n", __func__, port, res);
> +}
> +
> +static void
> +yt921x_dsa_phylink_get_caps(struct dsa_switch *ds, int port,
> +			    struct phylink_config *config)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	const struct yt921x_info *info = priv->info;
> +
> +	if (yt921x_info_port_is_internal(info, port)) {
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		config->mac_capabilities = 0;
> +	} else if (yt921x_info_port_is_external(info, port)) {
> +		/* TODO: external port may support SGMII only, XMII only, or
> +		 * SGMII + XMII depending on the chip. However, we can't get
> +		 * the accurate config table due to lack of document, thus
> +		 * we simply declare SGMII + XMII and rely on the correctness
> +		 * of devicetree for now.
> +		 */
> +
> +		/* SGMII */
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_100BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			  config->supported_interfaces);
> +		config->mac_capabilities = MAC_2500FD;
> +
> +		/* XMII */
> +		__set_bit(PHY_INTERFACE_MODE_MII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_REVMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_REVRMII,
> +			  config->supported_interfaces);
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +	} else {
> +		return;
> +	}
> +
> +	config->mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +				    MAC_10 | MAC_100 | MAC_1000;

You can set this at the top of the function, which will eliminate the
need for the final "else { return; }" and the need for the weird
looking "config->mac_capabilities = 0;" in the internal case. Don't
forget to update the external case to use |=.

This is safe because if config->supported_interfaces is empty, phylink
will report an error.

> +static void yt921x_mdio_shutdown(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct yt921x_priv *priv = dev_get_drvdata(dev);

	struct yt921x_priv *priv = mdiodev_get_drvdata(mdiodev0;

> +	struct dsa_switch *ds = &priv->ds;
> +
> +	dsa_switch_shutdown(ds);
> +}
> +
> +static void yt921x_mdio_remove(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct yt921x_priv *priv = dev_get_drvdata(dev);

	struct yt921x_priv *priv = mdiodev_get_drvdata(mdiodev0;

> +	struct dsa_switch *ds = &priv->ds;
> +
> +	dsa_unregister_switch(ds);
> +}
> +
> +static int yt921x_mdio_probe(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct yt921x_smi_mdio *mdio;
> +	struct yt921x_priv *priv;
> +	struct dsa_switch *ds;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mdio = devm_kzalloc(dev, sizeof(*mdio), GFP_KERNEL);
> +	if (!mdio)
> +		return -ENOMEM;
> +
> +	mdio->bus = mdiodev->bus;
> +	mdio->addr = mdiodev->addr;
> +	mdio->switchid = 0;
> +
> +	priv->smi_ops = &yt921x_smi_ops_mdio;
> +	priv->smi_ctx = mdio;
> +
> +	ds = &priv->ds;
> +	ds->dev = dev;
> +	ds->configure_vlan_while_not_filtering = true;
> +	ds->assisted_learning_on_cpu_port = true;
> +	ds->priv = priv;
> +	ds->ops = &yt921x_dsa_switch_ops;
> +	ds->phylink_mac_ops = &yt921x_phylink_mac_ops;
> +	ds->num_ports = YT921X_PORT_NUM;
> +
> +	dev_set_drvdata(dev, priv);

	mdiodev_set_drvdata(mdiodev, priv);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!