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

[Oleksij Rempel]

On Wed, May 17, 2023 at 01:45:46PM +0100, Russell King (Oracle) wrote:
> On Wed, May 17, 2023 at 02:10:33PM +0200, Oleksij Rempel wrote:
> > +/**
> > + * ksz8_upstram_link_up - Configures the CPU/upstream port of the switch.
> > + * @dev: The KSZ device instance.
> > + * @port: The port number to configure.
> > + * @speed: The desired link speed.
> > + * @duplex: The desired duplex mode.
> > + * @tx_pause: If true, enables transmit pause.
> > + * @rx_pause: If true, enables receive pause.
> > + *
> > + * Description:
> > + * The function configures flow control and speed settings for the CPU/upstream
> > + * port of the switch based on the desired settings, current duplex mode, and
> > + * speed.
> > + */
> > +static void ksz8_upstram_link_up(struct ksz_device *dev, int port, int speed,
> > +				 int duplex, bool tx_pause, bool rx_pause)
> > +{
> > +	u8 ctrl = 0;
> > +
> > +	if (duplex) {
> > +		if (tx_pause || rx_pause)
> > +			ctrl |= SW_FLOW_CTRL;
> > +	} else {
> > +		ctrl |= SW_HALF_DUPLEX;
> > +		if (tx_pause || rx_pause)
> > +			ctrl |= SW_HALF_DUPLEX_FLOW_CTRL;
> 
> It's come up before whether the pause settings should be used to control
> half-duplex flow control, and I believe the decision was they shouldn't.

Got it, back pressure and pause for flow control are two different
things. I'll remove the half-duplex back pressure control using pause
settings from the patch.

> The other thing I find slightly weird is that this is only being done
> for upstream ports - why would a port that's between switches or the
> switch and the CPU be in half duplex mode?

As for the CPU port half-duplex mode, it's currently configurable via
the device tree. I don't have a specific use case for it, but it's there
if needed. If it's causing confusion though, I'm open to removing it.
What do you think?

> Also, why would such a port want to use some kind of flow control? If
> the CPU starts sending pause frames because its got stuck, doesn't
> that eventually kill the entire network? Also doesn't it limit the
> network bandwidth to the ability of the host CPU *not* to send
> pause frames?

Before this patch, flow control on the CPU port was indeed hard-coded.
This patch lets us disable it if we want to, giving us a bit more
flexibility.

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 |