On Tue, 14 Nov 2023, Andrew Lunn wrote: ... > > + phy_support_asym_pause(phy); > > + > > + ipqess_port_set_state_now(port, BR_STATE_FORWARDING, false); > > + > > + if (port->pl) > > + phylink_start(port->pl); > > That looks odd. You unconditionally call phy_support_asym_pause() yet > conditionally call phylink_start(). I would expect there to always be > a phylink instance. > > Also, you should be telling phylink about the pause capabilities in > config->mac_capabilities. It is then phylinks problem to tell the PHY, > or the PCS driving the SFP etc about pause. You are correct. I probably fumbled this when splitting the calibration code. > > + if (tx_pause || port->index == 0) > > + reg |= QCA8K_PORT_STATUS_TXFLOW; > > + } > > + > > + reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC; > > + > > + qca8k_write(priv, QCA8K_REG_PORT_STATUS(port->index), reg); > > +} > > qca8k_phylink_mac_link_up() with some refactoring can be > reused. Please look through the driver and find other instances like > this where you can reuse more code. I tried to be conservative with modifying qca8k-common.c when it required modifying qca8k-8xxx.c. But I'll factor this code more aggressively since you think it is preferable. Best, -- Romain Gantois, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
