Re: [PATCH net-next v14 5/5] net: dsa: motorcomm: Add support for Motorcomm YT922x

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

On 10/13/26 09:51, Andrew Lunn wrote:
>> +++ b/drivers/net/dsa/motorcomm/mdio_bus.c
>> @@ -299,3 +299,42 @@ int yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp)
>>   
>>   	return 0;
>>   }
>> +
>> +/* intif extend register read/write api */
>> +int
>> +yt921x_intif_ext_write(struct yt921x_priv *priv, int port, int reg, u16 val)
>> +{
>> +	int res;
>> +
>> +	if (port > priv->series->max_ports)
>> +		return -ENODEV;
>> +
>> +	res = yt921x_intif_write(priv, port, YT92XX_PAGE_SELECT, reg);
>> +	if (res)
>> +		return res;
>> +
>> +	res = yt921x_intif_write(priv, port, YT92XX_PAGE, val);
>> +	if (res)
>> +		return res;
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +yt921x_intif_ext_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
>> +{
>> +	int res;
>> +
>> +	if (port > priv->series->max_ports)
>> +		return -ENODEV;
>> +
>> +	res = yt921x_intif_write(priv, port, YT92XX_PAGE_SELECT, reg);
>> +	if (res)
>> +		return res;
>> +
>> +	res = yt921x_intif_read(priv, port, YT92XX_PAGE, valp);
>> +	if (res)
>> +		return res;
>> +
>> +	return 0;
>> +}
> Why are these in mdio_bus? They don't have anything to do with MDIO.
>
> Since they build on yt921x_intif_read they seem to fit in yt921x.c
> better? Or am i missing something?

These two interfaces access the extend registers of the Phy/serdes.

Following the existing code style-where the PHY/SerDes MII registers

were placed in the mdio_bus.c,these two interfaces are also placed

in the mdio_bus.c.

>> +static void yt922x_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>> +				 struct phylink_link_state *state)
>> +{
>> +	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
>> +	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
>> +	int port = pp->index;
>> +	int res = 0;
>> +	u16 data;
>> +	int addr;
>> +	u16 lp;
>> +
>> +	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII);
>> +	if (addr < 0) {
>> +		state->link = false;
>> +		return;
>> +	}
>> +
>> +	mutex_lock(&priv->reg_lock);
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>> +	case PHY_INTERFACE_MODE_2500BASEX:
>> +		res = yt921x_intif_read(priv, addr, MII_BMSR, &data);
>> +		if (res)
>> +			goto err;
>> +		res = yt921x_intif_read(priv, addr, MII_LPA, &lp);
>> +		if (res)
>> +			goto err;
>> +		phylink_mii_c22_pcs_decode_state(state, neg_mode, data, lp);
>
> Rather than data, call it bmsr. It then fits:
>
> void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
> 				      unsigned int neg_mode, u16 bmsr, u16 lpa)
Okay.
>
>> +static void yt922x_pcs_an_restart(struct phylink_pcs *pcs)
>> +{
>> +	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
>> +	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
>> +	struct device *dev = to_device(priv);
>> +	int port = pp->index;
>> +	u16 data;
>> +	int addr;
>> +	int res;
>> +
>> +	mutex_lock(&priv->reg_lock);
>> +	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII);
>> +	if (addr < 0) {
>> +		res = addr;
>> +		goto err;
>> +	}
>> +	res = yt921x_intif_read(priv, addr, MII_BMCR, &data);
>> +	if (res)
>> +		goto err;
>> +	data |= BMCR_ANRESTART;
>> +	res = yt921x_intif_write(priv, addr, MII_BMCR, data);
> Again, bmcr. It then looks a lot like
> phylink_mii_c22_pcs_an_restart().
Okay.
>> +	if (res)
>> +		goto err;
>> +	mutex_unlock(&priv->reg_lock);
>> +	return;
>> +
>> +err:
>> +	mutex_unlock(&priv->reg_lock);
>> +	if (res)
>> +		dev_err(dev, "Failed to %s PCS port %d: %i\n", "an restart",
>> +			port, res);
>> +}
>> +
>> +static int yt922x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>> +			     phy_interface_t interface,
>> +			     const unsigned long *advertising,
>> +			     bool permit_pause_to_mac)
>> +{
> advertising does not appear to be used here. Does this pcs not support
> it? That would be odd, because you looked at lpa in earlier functions.

advertising and neg_mode will been handled in next patch version.

>
> 	Andrew