Message-ID: <20230411095524.GB19711@pengutronix.de>
Date:   Tue, 11 Apr 2023 11:55:24 +0200
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Woojung Huh <woojung.huh@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, kernel@...gutronix.de,
        Jakub Kicinski <kuba@...nel.org>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: FWD: Re: [PATCH net-next v1 1/1] net: dsa: microchip: ksz8: Make
 flow control, speed, and duplex on CPU port configurable

On Tue, Apr 11, 2023 at 10:17:47AM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 11, 2023 at 10:56:26AM +0200, Oleksij Rempel wrote:
> > On Fri, Apr 07, 2023 at 06:44:20PM +0100, Russell King (Oracle) wrote:
> > > On Fri, Apr 07, 2023 at 04:25:57PM +0200, Andrew Lunn wrote:
> > > > > +void ksz8_phylink_mac_link_up(struct ksz_device *dev, int port,
> > > > > +			      unsigned int mode, phy_interface_t interface,
> > > > > +			      struct phy_device *phydev, int speed, int duplex,
> > > > > +			      bool tx_pause, bool rx_pause)
> > > > > +{
> > > > > +	struct dsa_switch *ds = dev->ds;
> > > > > +	struct ksz_port *p;
> > > > > +	u8 ctrl = 0;
> > > > > +
> > > > > +	p = &dev->ports[port];
> > > > > +
> > > > > +	if (dsa_upstream_port(ds, port)) {
> > > > > +		u8 mask = SW_HALF_DUPLEX_FLOW_CTRL | SW_HALF_DUPLEX |
> > > > > +			SW_FLOW_CTRL | SW_10_MBIT;
> > > > > +
> > > > > +		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;
> > > > > +		}
> > > > > +
> > > > > +		if (speed == SPEED_10)
> > > > > +			ctrl |= SW_10_MBIT;
> > > > > +
> > > > > +		ksz_rmw8(dev, REG_SW_CTRL_4, mask, ctrl);
> > > > > +
> > > > > +		p->phydev.speed = speed;
> > > > > +	} else {
> > > > > +		const u16 *regs = dev->info->regs;
> > > > > +
> > > > > +		if (duplex) {
> > > > > +			if (tx_pause && rx_pause)
> > > > > +				ctrl |= PORT_FORCE_FLOW_CTRL;
> > > > > +		} else {
> > > > > +			if (tx_pause && rx_pause)
> > > > > +				ctrl |= PORT_BACK_PRESSURE;
> > > > > +		}
> > > > > +
> > > > > +		ksz_rmw8(dev, regs[P_STP_CTRL], PORT_FORCE_FLOW_CTRL |
> > > > > +			 PORT_BACK_PRESSURE, ctrl);
> > > 
> > > So, I guess the idea here is to enable some form of flow control when
> > > both tx and rx pause are enabled.
> > > 
> > > Here's a bunch of questions I would like answered before I give a tag:
> > > 
> > > 1) It looks like the device only supports symmetric pause?
> > 
> > This part of driver supports two family of switches: ksz88xx and
> > ksz87xx.
> > 
> > According to KSZ8765CLX  datasheet:
> > Per port, we control pause rx and tx with one bit:
> >   Register 18 (0x12): Port 1 Control 2
> >   Bit 4 - Force Flow Control
> >     1 = Enables Rx and Tx flow control on the port, regardless of the AN result.
> >     0 = Flow control is enabled based on the AN result (Default)
> 
> Is this more in the MAC register set than the PCS register set?

I assume, it is MAC register, since it contains following bits:
- Learning Disable
- Receive Enable
- Transmit Enable
- Ingress VLAN Filtering
- Enable 2 Queue Split of Tx Queue

> It's weird that it seems there's no way to force flow control
> off.

In case autoneg is disabled, then it is kind of force off.

> If it's the PCS register set, then this is partly what the
> permit_pause_to_mac boolean in pcs_config() should be used to
> control - but that assumes that when !permit_pause_to_mac it
> merely stops forwarding the flow control settings to the MAC.
> 
> If it's in the MAC register set, this should be controlled by
> the MLO_PAUSE_AN bit in state->pause.
> 
> However, when those are false, we expect the tx_pause and
> rx_pause in mac_link_up() to be respected by the hardware.

In case advertisement is synced with tx_pause/rx_pause, it will be
accidentally in sync. In case autoneg is disabled, then it is respected.
Some of KSZ8765CLX ports are 100baseFX ports - in this case Aneg seems
to be not used and Force Flow Control bit is really forcing on and off
states.

> > Globally, pause tx and/or rx can be disabled:
> > 
> >   Register 3 (0x03): Global Control 1
> >   Bit 5 - IEEE 802.3x Transmit Flow Control Disable
> >     0 = Enables transmit flow control based on AN result.
> >     1 = Will not enable transmit flow control regardless of the AN result.
> >   Bit 4 - IEEE 802.3x Receive Flow Control Disable
> >     0 = Enables receive flow control based on AN result.
> >     1 = Will not enable receive flow control regardless of the AN result.
> > 
> > So, it is possible to configure the entire switch in SYNC or ASYNC mode
> > only.
> 
> Well, that's a very strange setup. So basically we have no software
> control over manually setting the flow control, and we must use
> the hardware to pass the AN result to the MAC to have everything
> configured correctly.

Not sure if HW will advertise proper configuration if one of this global
bits is set to 0. Probably it will need SW assistance to set it right.

> > Still not sure what role plays autoneg in this configuration:
> > 
> >   Register 55 (0x37): Port 3 Control 7 (only for ports 3 and 4)
> >   Bits 5 - 4 - Advertised_Flow_Control _Capability
> >     00 = No pause
> >     01 = Symmetric PAUSE
> >     10 = Asymmetric PAUSE
> >     11 = Both Symmetric PAUSE and Asymmetric
> > 
> > According to this bits, it is possible to announce both Symmetric
> > and Asymmetric PAUSE, but will the switch enable asymmetric mode
> > properly if link partner advertise asymmetric too?
> 
> These two bits correspond directly with:
> ETHTOOL_LINK_MODE_Pause_BIT (for bit 4)
> ETHTOOL_LINK_MODE_Asym_Pause_BIT (for bit 5)
> 
> IEEE 802.3 has a table in it of the possible resolutions given the
> advertisement from both ends. In the case of advertising both, then
> the resolutions can be:
> - Pause disabled
> - Asymmetric pause towards local device (rx enabled, tx disabled)
> - Symmetric pause (rx and tx enabled)
> 
> It is the responsibility of both ends of the link to implement the
> decoding as per 802.3 table 28B-3.
> 
> Since we can't manually control the tx and rx pause enables, I think
> the only sensible way forward with this would be to either globally
> disable pause on the device, and not report support for any pause
> modes, or report support for all pause modes, advertise '11' and
> let the hardware control it (which means the ethtool configuration
> for pause would not be functional.)

Do it make sense to make it conditional, depending on global switch
configuration and PHY integrated in the port - 100BaseT vs 100BaseFX?

> This needs to be commented in the driver so that in the future we
> remember why this has been done.
> 
> Maybe Andrew and/or Vladimir also have an opinion to share about the
> best approach here?


-- 
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 |

Powered by blists - more mailing lists