Re: [RFC net-next v2] net:dsa:yt922x: Add support for Motorcomm YT922x

[Kyle Switch]

On 8/20/26 22:47, Andrew Lunn wrote:
> >   drivers/net/dsa/Kconfig  |   7 +-
> >   drivers/net/dsa/Makefile |   2 +-
> >   drivers/net/dsa/yt921x.c | 819 ++++++++++++++++++++++++++++++++++++++-
> That is a lot of new code. Please try to break it up into a number of
> smaller patches, with good commit messages, which are obviously
> correct.
>
> Since this is a new device, it does not need to work with a big
> bang. You can slowly add the needed bits, such that when the last
> patch is merged the device is functional.

Ans: I have now split the original patch into two separate patches

       — one for the tag and the other for the YT922x DSA switch.

> > +#define YT922X_INTERNAL_SDS1_PHYADDR 0
> > +#define YT922X_INTERNAL_UTP0_PHYADDR 4
> > +#define YT922X_INTERNAL_UTP1_PHYADDR 5
> > +#define YT922X_INTERNAL_UTP2_PHYADDR 6
> > +#define YT922X_INTERNAL_UTP3_PHYADDR 7
> > +#define YT922X_INTERNAL_SDS0_PHYADDR 8
> These don't seem to be used. Generally, a MAC driver does not need to
> know the mapping to PHY addresses, the phandles in the DT indicates
> it.
Ans: fix done in v3 to remove the unnecessary macros.
> >   static int yt921x_mbus_int_read(struct mii_bus *mbus, int port, int reg)
> >   {
> >   	struct yt921x_priv *priv = mbus->priv;
> > +	int max_ports;
> >   	u16 val;
> >   	int res;
> >   
> > -	if (port >= YT921X_PORT_NUM)
> > +	max_ports = priv->series_info->ports;
> > +	if (port >= max_ports)
> This sort of code change would make a nice simple patch. Change all
> current instances of YT921X_PORT_NUM to priv->series_info->ports. Easy
> to review.
>
> > @@ -4748,6 +4766,13 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
> >   	struct device_node *child;
> >   	int res;
> >   
> > +	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
> > +		struct yt921x_port *pp = &priv->ports[i];
> > +
> > +		pp->index = i;
> > +		INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
> > +	}
> This should be a patch, with an explanation why it is needed. Also
> please take a look at ethtool -c stats-block-usecs, which indicates
> how out of date the statistics are. It is currently not well
> supported, i only learned of it recently, but setting it will help you
> get the self tests passing.

Ans: The initialization logic was already there. I've moved it from probe()

        to yt921x_dsa_setup() because the YT922x doesn't support MIB stats

        in this series, and the MIB support between the two chips is 
different.

> > +static void
> > +yt922x_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 yt921x_priv *priv = to_yt921x_priv(dp->ds);
> > +	int port = dp->index;
> > +	int res;
> > +
> > +	mutex_lock(&priv->reg_lock);
> > +	res = yt922x_port_down(priv, port);
> > +	mutex_unlock(&priv->reg_lock);
> > +
> > +	if (res)
> > +		dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring down",
> > +			port, res);
> > +}
> This appears to be a cut/paste of
> yt921x_phylink_mac_link_down(). Don't do that. Refactor existing
> functions to make them generic.
>
> > +static int yt922x_port_sds_init(struct yt921x_priv *priv, int port,
> > +				phy_interface_t interface)
> > +{
> > +	int addr;
> > +	u16 data;
> > +	int res;
> > +
> > +	addr = yt922x_sds_phyaddr_get(port,
> > +				      YT922X_PHY_REG_TYPE_SDS_COMMON_EXT,
> > +				      YT922X_PHY_REG_SPACE_SGMII);
> > +	if (addr < 0)
> > +		return -EINVAL;
> > +	/* write protect */
> > +	res = yt921x_intif_ext_write(priv, addr, 0x4be, 0xd);
> > +	if (res)
> > +		return res;
> > +	/* CDR */
> > +	if (interface == PHY_INTERFACE_MODE_100BASEX) {
> > +		res = yt921x_intif_ext_write(priv, addr, 0x406, 0x0);
> > +		if (res)
> > +			return res;
> > +		res = yt921x_intif_ext_write(priv, addr, 0x416, 0x3458);
> > +		if (res)
> > +			return res;
> > +	} else {
> > +		res = yt921x_intif_ext_write(priv, addr, 0x406, 0x800);
> > +		if (res)
> > +			return res;
> > +		res = yt921x_intif_ext_write(priv, addr, 0x416, 0x4558);
> > +		if (res)
> > +			return res;
> > +	}
> > +	/* PLL */
> > +	if (interface == PHY_INTERFACE_MODE_USXGMII) {
> > +		res = yt921x_intif_ext_write(priv, addr, 0x43a, 0x1006);
> > +		if (res)
> > +			return res;
> > +		res = yt921x_intif_ext_write(priv, addr, 0x43f, 0x3029);
> > +		if (res)
> What is this function doing?
Ans: Complete some init configuration according to serdes interface mode.
>      Andrew
>
> ---
> pw-bot: cr