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