Re: [PATCH net-next v4 1/2] net: dsa: microchip: ksz8: Make flow control, speed, and duplex on CPU port configurable

On Sat, May 20, 2023 at 02:28:02AM +0300, Vladimir Oltean wrote:

quoted

+	/* This hardware only supports SPEED_10 and SPEED_100. For SPEED_10
+	 * we need to set the SW_10_MBIT bit. Otherwise, we can leave it 0.
+	 */
+	if (speed == SPEED_10)
+		ctrl |= SW_10_MBIT;
+
+	ksz_rmw8(dev, REG_SW_CTRL_4, SW_HALF_DUPLEX | SW_FLOW_CTRL |
+		 SW_10_MBIT, ctrl);

REG_SW_CTRL_4 ... S_REPLACE_VID_CTRL ... dev->info->regs[P_XMII_CTRL_1] ...
at some point we will need one more consolidation effort here, since we
have at least 3 ways of reaching the same register.

Agree, the register access is a bit messy now. Your idea about the
regfield API sounds good. We should try it.

Should i convert this patch to use dev->info->regs?

quoted

 	.mirror_del = ksz8_port_mirror_del,
 	.get_caps = ksz8_get_caps,
+	.phylink_mac_link_up = ksz8_phylink_mac_link_up,

Another future consolidation to consider: since all ksz_dev_ops now
provide .phylink_mac_link_up(), the "if" condition here is no longer
necessary:

static void ksz_phylink_mac_link_up(struct dsa_switch *ds, int port,
				    unsigned int mode,
				    phy_interface_t interface,
				    struct phy_device *phydev, int speed,
				    int duplex, bool tx_pause, bool rx_pause)
{
	struct ksz_device *dev = ds->priv;

	if (dev->dev_ops->phylink_mac_link_up)
		dev->dev_ops->phylink_mac_link_up(dev, port, mode, interface,
						  phydev, speed, duplex,
						  tx_pause, rx_pause);
}

which reminds me of the fact that I also had a patch to remove
dev->dev_ops->phylink_mac_config():
https://patchwork.kernel.org/project/netdevbpf/patch/20230316161250.3286055-5-vladimir.oltean@nxp.com/

I give up with that patch set now, since there's zero reviewer interest.
If you want and you think it's useful, you might want to adapt it for
KSZ8873.

Sounds good. I'll take it in to my mainlining queue for KSZ8873.

quoted

 	.config_cpu_port = ksz8_config_cpu_port,
 	.enable_stp_addr = ksz8_enable_stp_addr,
 	.reset = ksz8_reset_switch,
-- 
2.39.2

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |