Re: [PATCH RFC net-next 2/2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

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

On Mon, Jan 27, 2025 at 07:32:26PM -0800, Tristram.Ha@microchip.com wrote:
> @@ -161,6 +161,113 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
>  					10, 1000);
>  }
>  
> +static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 len)
> +{
> +	u32 data;
> +
> +	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
> +	data |= reg;
> +	if (len > 1)
> +		data |= PORT_SGMII_AUTO_INCR;
> +	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
> +}
> +
> +static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 *buf, u16 len)
> +{
> +	u32 data;
> +
> +	mutex_lock(&dev->sgmii_mutex);

You won't need sgmii_mutex if all accesses go through the MDIO bus
accessors (please do so and get rid of this mutex.)

> +	port_sgmii_s(dev, port, devid, reg, len);
> +	while (len) {
> +		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
> +		*buf++ = (u16)data;
> +		len--;
> +	}
> +	mutex_unlock(&dev->sgmii_mutex);
> +}
> +
> +static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 *buf, u16 len)
> +{
> +	u32 data;
> +
> +	mutex_lock(&dev->sgmii_mutex);
> +	port_sgmii_s(dev, port, devid, reg, len);
> +	while (len) {
> +		data = *buf++;
> +		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
> +		len--;
> +	}
> +	mutex_unlock(&dev->sgmii_mutex);
> +}

I don't see any need for any of the above complexity for writing
multiple registers. All your calls to the above two functions only
pass a value of 1 for len.

> +
> +static void port_sgmii_reset(struct ksz_device *dev, uint p)
> +{
> +	u16 ctrl = BMCR_RESET;
> +
> +	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +}

doesn't xpcs_create() result in xpcs->need_reset being set true, and
thus when the XPCS is used, cause xpcs_pre_config() to do a soft reset
of the XPCS? More importantly, the XPCS driver waits for the reset
to complete...

> @@ -978,6 +1085,13 @@ void ksz9477_get_caps(struct ksz_device *dev, int port,
>  
>  	if (dev->info->gbit_capable[port])
>  		config->mac_capabilities |= MAC_1000FD;
> +
> +	if (ksz_is_sgmii_port(dev, port)) {
> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +	}

	f (ksz_is_sgmii_port(dev, port))
		phy_interface_or(config->supported_interfaces,
				 config->supported_interfaces,
				 xpcs_to_phylink_pcs(port->xpcs)->supported_interfaces);

If xpcs_to_phylink_pcs(port->xpcs)->supported_interfaces does not
accurately indicate the interfaces that are supported on your XPCS
model, then you need further xpcs driver changes.

> +static inline int ksz_get_sgmii_port(struct ksz_device *dev)
> +{
> +	return (dev->info->sgmii_port - 1);
> +}
> +
> +static inline bool ksz_has_sgmii_port(struct ksz_device *dev)
> +{
> +	return (dev->info->sgmii_port > 0);
> +}
> +
> +static inline bool ksz_is_sgmii_port(struct ksz_device *dev, int port)
> +{
> +	return (dev->info->sgmii_port == port + 1);
> +}

No need for the parens in any of the above three.

Thanks.

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