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

[Kyle Switch <kyle.switch@motor-comm.com>]

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