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

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

[Russell King (Oracle)]

On Fri, Apr 07, 2023 at 04:25:57PM +0200, Andrew Lunn wrote:
> Hi Russell
> 
> It looks like you were not Cc:ed.

Thanks Andrew.

It really would help if people Cc'd the right folk! Because they failed
to, sorry, this isn't going to be a reply that's threaded properly...

> > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> > index d0e3f6e2db1d..8917f22f90d2 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -1321,12 +1321,52 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
> >  			if (remote & KSZ8_PORT_FIBER_MODE)
> >  				p->fiber = 1;
> >  		}
> > -		if (p->fiber)
> > -			ksz_port_cfg(dev, i, regs[P_STP_CTRL],
> > -				     PORT_FORCE_FLOW_CTRL, true);
> > -		else
> > -			ksz_port_cfg(dev, i, regs[P_STP_CTRL],
> > -				     PORT_FORCE_FLOW_CTRL, false);
> > +	}
> > +}
> > +
> > +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?
2) If yes, are you *not* providing MAC_ASYM_PAUSE in the MAC
   capabilities? If not, why not?
3) If yes, then please do as others do and use tx_pause || rx_pause
   here.

Lastly, a more general question for ethtool/network folk - as for half
duplex and back pressure, is that a recognised facility for the MAC
to control via the ethtool pause parameter API?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Oleksij Rempel]

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)

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

Suddenly I do not have ksz87xx variants to test it.

> 2) If yes, are you *not* providing MAC_ASYM_PAUSE in the MAC
>    capabilities? If not, why not?

Current code looks as follow:

  1402 void ksz8_get_caps(struct ksz_device *dev, int port,                                                                           
  1403 ┊       ┊          struct phylink_config *config)                                                                              
  1404 {                                                                                                                              
  1405 ┊       config->mac_capabilities = MAC_10 | MAC_100;                                                                           
  1406 ┊                                                                                                                              
  1407 ┊       /* Silicon Errata Sheet (DS80000830A):                                                                                 
  1408 ┊        * "Port 1 does not respond to received flow control PAUSE frames"                                                     
  1409 ┊        * So, disable Pause support on "Port 1" (port == 0) for all ksz88x3                                                   
  1410 ┊        * switches.                                                                                                           
  1411 ┊        */                                                                                                                    
  1412 ┊       if (!ksz_is_ksz88x3(dev) || port)                                                                                      
  1413 ┊       ┊       config->mac_capabilities |= MAC_SYM_PAUSE;                                                                     
  1414 ┊                                                                                                                              
  1415 ┊       /* Asym pause is not supported on KSZ8863 and KSZ8873 */                                                               
  1416 ┊       if (!ksz_is_ksz88x3(dev))                                                                                              
  1417 ┊       ┊       config->mac_capabilities |= MAC_ASYM_PAUSE;                                                                    
  1418 } 

wich will set SYM support for all ksz87xx ports and all except of port == 0
on ksz87xx.

but SYM and ASYM modes depend on global switch configuration, so this
code is probably correct as long as global defaults are not changed. The
Autoneg based MAC_ASYM_PAUSE is still not clear for me.

> 3) If yes, then please do as others do and use tx_pause || rx_pause
>    here.

Hm... you are right.

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 |

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

[Russell King (Oracle)]

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?
It's weird that it seems there's no way to force flow control
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.

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

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

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?

Thanks for the clarification from the register set.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Oleksij Rempel]

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 |

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

[Vladimir Oltean]

On Tue, Apr 11, 2023 at 10:17:47AM +0100, Russell King (Oracle) wrote:
> 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,

This implies restarting autoneg on all the other switch ports when one
port's flow control mode is changed?

> 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.)
> 
> 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?

I don't object to documenting that manually forcing flow control off is
broken and leaving it at that (and the way to force it off would be to
not advertise any of the 2 bits).

But why advertise only 11 (Asym_Pause | Pause) when the PHYs integrated
here have the advertisement configurable (presumably also through the
micrel.c PHY driver)? They would advertise in accordance with ethtool, no?

I may have missed something.

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

[Vladimir Oltean]

On Tue, Apr 11, 2023 at 02:16:09PM +0300, Vladimir Oltean wrote:
> I may have missed something.

Maybe I'm wrong, but my blind intuition says that when autoneg is
disabled in the integrated PHYs, flow control _is_ by default forced off
per port, unless the "Force Flow Control" bit from Port N Control 2
registers is set. So that can be used to still support:
- ethtool --pause swp0 autoneg off rx on tx on
- ethtool --pause swp0 autoneg off rx off tx off
- ethtool --pause swp0 autoneg on # asymmetric RX/TX combinations depend upon autoneg

I may be wrong; I don't have the hardware and the ethtool pause autoneg
bit is not 100% clear to me.

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

[Russell King (Oracle)]

On Tue, Apr 11, 2023 at 02:16:09PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 11, 2023 at 10:17:47AM +0100, Russell King (Oracle) wrote:
> > 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,
> 
> This implies restarting autoneg on all the other switch ports when one
> port's flow control mode is changed?

From my reading of these global register descriptions, no it doesn't,
and even if we did restart aneg, it would have no overall system effect
because the advertisements for each port haven't been changed. It's
mad hardware.

What I was meaning above is that we configure the entire switch to
either do autonegotiated flow control at setup time, or we configure
the switch to never do flow control.

> > 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.)
> > 
> > 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?
> 
> I don't object to documenting that manually forcing flow control off is
> broken and leaving it at that (and the way to force it off would be to
> not advertise any of the 2 bits).
> 
> But why advertise only 11 (Asym_Pause | Pause) when the PHYs integrated
> here have the advertisement configurable (presumably also through the
> micrel.c PHY driver)? They would advertise in accordance with ethtool, no?
> 
> I may have missed something.

I think you have. I'm only talking about the ability to control flow
control manually via ethtool -A. Changing it via the advertisement
(ethtool -s) would still work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Tue, Apr 11, 2023 at 02:35:16PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 11, 2023 at 02:16:09PM +0300, Vladimir Oltean wrote:
> > I may have missed something.
> 
> Maybe I'm wrong, but my blind intuition says that when autoneg is
> disabled in the integrated PHYs, flow control _is_ by default forced off
> per port, unless the "Force Flow Control" bit from Port N Control 2
> registers is set. So that can be used to still support:
> - ethtool --pause swp0 autoneg off rx on tx on
> - ethtool --pause swp0 autoneg off rx off tx off
> - ethtool --pause swp0 autoneg on # asymmetric RX/TX combinations depend upon autoneg
> 
> I may be wrong; I don't have the hardware and the ethtool pause autoneg
> bit is not 100% clear to me.

Stage 1 (per port, force bit):
- If zero, the flow control result from aneg is used, and thus depends on
  what both ends advertise.
- If one, flow control is force-enabled.

Stage 2 (global):
Transmit and receive flow control can be masked off.

Basically, the best we could do is:

	ethtool --pause ... autoneg on

depends on the negotiation result (correct).

	ethtool --pause ... autoneg off rx off tx off

if we *only* program the local advertisement to 00, but leave the
force bit as 0, then this can work.

	ethtool --pause ... autoneg off rx on tx on

if we program the force bit to 1, then this can work, and it doesn't
matter what we do with the advertisement.

Anything else wouldn't give the result the user wants, because there's
no way to independently force rx and tx flow control per port.

That said, phylink doesn't give enough information to make the above
possible since the force bit depends on (tx && rx &&!permit_pause_to_mac)

So, because this hardware is that crazy, I suggest that it *doesn't*
even attempt to support ethtool --pause, and either is programmed
at setup time to use autonegotiated pause (with the negotiation state
programmed via ethtool -s) or it's programmed to have pause globally
disabled. Essentially, I'm saying the hardware is too broken in its
design to be worth bothering trying to work around its weirdness.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Andrew Lunn]

On Tue, Apr 11, 2023 at 01:00:43PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 11, 2023 at 02:35:16PM +0300, Vladimir Oltean wrote:
> > On Tue, Apr 11, 2023 at 02:16:09PM +0300, Vladimir Oltean wrote:
> > > I may have missed something.
> > 
> > Maybe I'm wrong, but my blind intuition says that when autoneg is
> > disabled in the integrated PHYs, flow control _is_ by default forced off
> > per port, unless the "Force Flow Control" bit from Port N Control 2
> > registers is set. So that can be used to still support:
> > - ethtool --pause swp0 autoneg off rx on tx on
> > - ethtool --pause swp0 autoneg off rx off tx off
> > - ethtool --pause swp0 autoneg on # asymmetric RX/TX combinations depend upon autoneg
> > 
> > I may be wrong; I don't have the hardware and the ethtool pause autoneg
> > bit is not 100% clear to me.
> 
> Stage 1 (per port, force bit):
> - If zero, the flow control result from aneg is used, and thus depends on
>   what both ends advertise.
> - If one, flow control is force-enabled.
> 
> Stage 2 (global):
> Transmit and receive flow control can be masked off.
> 
> Basically, the best we could do is:
> 
> 	ethtool --pause ... autoneg on

This hardware design does seem messed up. My experience reviewing
ethtool patches for flow control is that most developers also get it
wrong. Plus it is not very well documented. phylink has made that
better since it moves a lot of the code into the core.

I suggest you follow Russells suggestion, only support autoneg on.
Have ethtool set return -EOPNOTSUPP for anything else. And ethtool get
should return that autoneg is on, and the result of the autoneg, which
you should be able to get.

Only implementing a subset of ethtool is fine.  We have drivers
implementing various subsets, mostly when firmware is controlling the
hardware, and there are firmware limitations.

	  Andrew

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

[Vladimir Oltean]

On Tue, Apr 11, 2023 at 12:50:28PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 11, 2023 at 02:16:09PM +0300, Vladimir Oltean wrote:
> > On Tue, Apr 11, 2023 at 10:17:47AM +0100, Russell King (Oracle) wrote:
> > > 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,
> > 
> > This implies restarting autoneg on all the other switch ports when one
> > port's flow control mode is changed?
> 
> From my reading of these global register descriptions, no it doesn't,
> and even if we did restart aneg, it would have no overall system effect
> because the advertisements for each port haven't been changed. It's
> mad hardware.
> 
> What I was meaning above is that we configure the entire switch to
> either do autonegotiated flow control at setup time, or we configure
> the switch to never do flow control.

I was thinking you were suggesting to also modify the advertisement in
software from those other ports to 00 when the flow control was forced
off on one. Otherwise (it seems you weren't), I think it's a bit
counter-productive to configure the switch to never do flow control,
when the only problem seems to be with the forced modes but autoneg is fine.

> > I don't object to documenting that manually forcing flow control off is
> > broken and leaving it at that (and the way to force it off would be to
> > not advertise any of the 2 bits).
> > 
> > But why advertise only 11 (Asym_Pause | Pause) when the PHYs integrated
> > here have the advertisement configurable (presumably also through the
> > micrel.c PHY driver)? They would advertise in accordance with ethtool, no?
> > 
> > I may have missed something.
> 
> I think you have. I'm only talking about the ability to control flow
> control manually via ethtool -A. Changing it via the advertisement
> (ethtool -s) would still work.

That part was understood.

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

[Vladimir Oltean]

On Tue, Apr 11, 2023 at 01:00:43PM +0100, Russell King (Oracle) wrote:
> 	ethtool --pause ... autoneg on
> 	ethtool --pause ... autoneg off rx off tx off
> 	ethtool --pause ... autoneg off rx on tx on
> 
> Anything else wouldn't give the result the user wants, because there's
> no way to independently force rx and tx flow control per port.

Right.

> That said, phylink doesn't give enough information to make the above
> possible since the force bit depends on (tx && rx &&!permit_pause_to_mac)

So, since the "permit_pause_to_mac" information is missing here, I guess
the next logical step based on what you're saying is that it's a matter
of not using the pcs_config() API, or am I misunderstanding again? :)

> So, because this hardware is that crazy, I suggest that it *doesn't*
> even attempt to support ethtool --pause, and either is programmed
> at setup time to use autonegotiated pause (with the negotiation state
> programmed via ethtool -s) or it's programmed to have pause globally
> disabled. Essentially, I'm saying the hardware is too broken in its
> design to be worth bothering trying to work around its weirdness.

Ok. How can this driver reject changes made through ethtool --pause?

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

[Russell King (Oracle)]

On Tue, Apr 11, 2023 at 04:12:15PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 11, 2023 at 12:50:28PM +0100, Russell King (Oracle) wrote:
> > On Tue, Apr 11, 2023 at 02:16:09PM +0300, Vladimir Oltean wrote:
> > > On Tue, Apr 11, 2023 at 10:17:47AM +0100, Russell King (Oracle) wrote:
> > > > 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,
> > > 
> > > This implies restarting autoneg on all the other switch ports when one
> > > port's flow control mode is changed?
> > 
> > From my reading of these global register descriptions, no it doesn't,
> > and even if we did restart aneg, it would have no overall system effect
> > because the advertisements for each port haven't been changed. It's
> > mad hardware.
> > 
> > What I was meaning above is that we configure the entire switch to
> > either do autonegotiated flow control at setup time, or we configure
> > the switch to never do flow control.
> 
> I was thinking you were suggesting to also modify the advertisement in
> software from those other ports to 00 when the flow control was forced
> off on one. Otherwise (it seems you weren't), I think it's a bit
> counter-productive to configure the switch to never do flow control,
> when the only problem seems to be with the forced modes but autoneg is fine.

As I see it, there's essentially two possible options with this
hardware:

1. treat the switch as something that can negotiate pause modes.
2. treat the switch as something that doesn't support pause modes.

You are absolutely right that if (1) then its possible through the
advertisement to have (2) but the reverse is not true, so clearly
(1) is the better approach here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Tue, Apr 11, 2023 at 04:26:17PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 11, 2023 at 01:00:43PM +0100, Russell King (Oracle) wrote:
> > 	ethtool --pause ... autoneg on
> > 	ethtool --pause ... autoneg off rx off tx off
> > 	ethtool --pause ... autoneg off rx on tx on
> > 
> > Anything else wouldn't give the result the user wants, because there's
> > no way to independently force rx and tx flow control per port.
> 
> Right.
> 
> > That said, phylink doesn't give enough information to make the above
> > possible since the force bit depends on (tx && rx &&!permit_pause_to_mac)
> 
> So, since the "permit_pause_to_mac" information is missing here, I guess
> the next logical step based on what you're saying is that it's a matter
> of not using the pcs_config() API, or am I misunderstanding again? :)

pcs_config() doesn't get the "tx" and "rx" above. mac_link_up() does,
but doesn't get the "permit_pause_to_mac" (since that's supposed to
be a "configuration" thing.)

Anyway, I think this is now moot since I think we've agreed on a way
forward for this hardware.

> > So, because this hardware is that crazy, I suggest that it *doesn't*
> > even attempt to support ethtool --pause, and either is programmed
> > at setup time to use autonegotiated pause (with the negotiation state
> > programmed via ethtool -s) or it's programmed to have pause globally
> > disabled. Essentially, I'm saying the hardware is too broken in its
> > design to be worth bothering trying to work around its weirdness.
> 
> Ok. How can this driver reject changes made through ethtool --pause?

We would need either something in DSA (dsa_slave_set_pauseparam()) to
prevent success, or something in phylink_ethtool_set_pauseparam() to
do the same.

At the phylink level, that could be a boolean in struct phylink_config.
Something like "disable_ethtool_set_pauseparam" (I'd prefer something
a tad shorter) which if set would make phylink_ethtool_set_pauseparam()
return -EOPNOTSUPP.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Vladimir Oltean]

On Tue, Apr 11, 2023 at 02:33:00PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 11, 2023 at 04:26:17PM +0300, Vladimir Oltean wrote:
> > On Tue, Apr 11, 2023 at 01:00:43PM +0100, Russell King (Oracle) wrote:
> > > 	ethtool --pause ... autoneg on
> > > 	ethtool --pause ... autoneg off rx off tx off
> > > 	ethtool --pause ... autoneg off rx on tx on
> > > 
> > > Anything else wouldn't give the result the user wants, because there's
> > > no way to independently force rx and tx flow control per port.
> > 
> > Right.
> > 
> > > That said, phylink doesn't give enough information to make the above
> > > possible since the force bit depends on (tx && rx &&!permit_pause_to_mac)
> > 
> > So, since the "permit_pause_to_mac" information is missing here, I guess
> > the next logical step based on what you're saying is that it's a matter
> > of not using the pcs_config() API, or am I misunderstanding again? :)
> 
> pcs_config() doesn't get the "tx" and "rx" above. mac_link_up() does,
> but doesn't get the "permit_pause_to_mac" (since that's supposed to
> be a "configuration" thing.)

Ah, ok. So it would be more complicated to plumb all information through
in a reliable way.

> Anyway, I think this is now moot since I think we've agreed on a way
> forward for this hardware.

I would say we've agreed on something that does not impose practical
limitations for internal PHY ports.

The xMII port (usually fixed-link) has its own flow control
configuration, through Register 6 (0x06): Global Control 4, and that
seems to not distinguish between TX and RX either. We're okay with
setting that single bit based on "rx_pause || tx_pause", right?

> > > So, because this hardware is that crazy, I suggest that it *doesn't*
> > > even attempt to support ethtool --pause, and either is programmed
> > > at setup time to use autonegotiated pause (with the negotiation state
> > > programmed via ethtool -s) or it's programmed to have pause globally
> > > disabled. Essentially, I'm saying the hardware is too broken in its
> > > design to be worth bothering trying to work around its weirdness.
> > 
> > Ok. How can this driver reject changes made through ethtool --pause?
> 
> We would need either something in DSA (dsa_slave_set_pauseparam()) to
> prevent success, or something in phylink_ethtool_set_pauseparam() to
> do the same.
> 
> At the phylink level, that could be a boolean in struct phylink_config.
> Something like "disable_ethtool_set_pauseparam" (I'd prefer something
> a tad shorter) which if set would make phylink_ethtool_set_pauseparam()
> return -EOPNOTSUPP.

Since the configuration "ethtool --pause swp0 autoneg on" should be
trivially accepted, then I suggest a "bool no_forced_pause" based on
which struct ethtool_pauseparam :: autoneg gets restricted?
This could live either in phylink_config or in struct dsa_port,
depending on what you believe to be the most appropriate scope for this
workaround. I would slightly prefer it to live in phylink, since that's
the entity who keeps track of MLO_PAUSE_AN in general.

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

[Vladimir Oltean]

On Fri, Apr 07, 2023 at 06:44:20PM +0100, Russell King (Oracle) wrote:
> Lastly, a more general question for ethtool/network folk - as for half
> duplex and back pressure, is that a recognised facility for the MAC
> to control via the ethtool pause parameter API?

AFAIU, the ethtool pause API is for PAUSE-based flow control (as defined
in IEEE 802.3-2018 annex 31B), not half duplex collision based flow control.
That being said, I don't know what facility could be used to enable
collision based flow control. I'd say it is an abuse of the API.