| Message ID | 20230517121034.3801640-2-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fine-Tune Flow Control and Speed Configurations in Microchip KSZ8xxx DSA Driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 75 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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. 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? 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?
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
On Wed, May 17, 2023 at 02:10:33PM +0200, Oleksij Rempel wrote: > Allow flow control, speed, and duplex settings on the CPU port to be > configurable. Previously, the speed and duplex relied on default switch > values, which limited flexibility. Additionally, flow control was > hardcoded and only functional in duplex mode. This update enhances the > configurability of these parameters. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz8.h | 4 +++ > drivers/net/dsa/microchip/ksz8795.c | 47 ++++++++++++++++++++++++-- > drivers/net/dsa/microchip/ksz_common.c | 1 + > 3 files changed, 50 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h > index e68465fdf6b9..ec02baca726f 100644 > --- a/drivers/net/dsa/microchip/ksz8.h > +++ b/drivers/net/dsa/microchip/ksz8.h > @@ -58,5 +58,9 @@ int ksz8_switch_detect(struct ksz_device *dev); > int ksz8_switch_init(struct ksz_device *dev); > void ksz8_switch_exit(struct ksz_device *dev); > int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu); > +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); > > #endif > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > index f56fca1b1a22..75b98a5d53af 100644 > --- a/drivers/net/dsa/microchip/ksz8795.c > +++ b/drivers/net/dsa/microchip/ksz8795.c > @@ -1371,6 +1371,51 @@ void ksz8_config_cpu_port(struct dsa_switch *ds) > } > } > > +/** > + * ksz8_upstram_link_up - Configures the CPU/upstream port of the switch. Looks like a typ0: upstream > +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; > + } > + > + if (speed == SPEED_10) > + ctrl |= SW_10_MBIT; Other speeds don't need to be handled? Maybe a comment why 10 is special? Andrew --- pw-bot: cr
diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h index e68465fdf6b9..ec02baca726f 100644 --- a/drivers/net/dsa/microchip/ksz8.h +++ b/drivers/net/dsa/microchip/ksz8.h @@ -58,5 +58,9 @@ int ksz8_switch_detect(struct ksz_device *dev); int ksz8_switch_init(struct ksz_device *dev); void ksz8_switch_exit(struct ksz_device *dev); int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu); +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); #endif diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index f56fca1b1a22..75b98a5d53af 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -1371,6 +1371,51 @@ void ksz8_config_cpu_port(struct dsa_switch *ds) } } +/** + * 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; + } + + if (speed == SPEED_10) + ctrl |= SW_10_MBIT; + + ksz_rmw8(dev, REG_SW_CTRL_4, SW_HALF_DUPLEX_FLOW_CTRL | SW_HALF_DUPLEX | + SW_FLOW_CTRL | SW_10_MBIT, ctrl); +} + +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) +{ + if (dsa_is_upstream_port(dev->ds, port)) + ksz8_upstram_link_up(dev, port, speed, duplex, tx_pause, + rx_pause); +} + static int ksz8_handle_global_errata(struct dsa_switch *ds) { struct ksz_device *dev = ds->priv; @@ -1419,8 +1464,6 @@ int ksz8_setup(struct dsa_switch *ds) */ ds->vlan_filtering_is_global = true; - ksz_cfg(dev, S_REPLACE_VID_CTRL, SW_FLOW_CTRL, true); - /* Enable automatic fast aging when link changed detected. */ ksz_cfg(dev, S_LINK_AGING_CTRL, SW_LINK_AUTO_AGING, true); diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index a4428be5f483..6e19ad70c671 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -210,6 +210,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = { .mirror_add = ksz8_port_mirror_add, .mirror_del = ksz8_port_mirror_del, .get_caps = ksz8_get_caps, + .phylink_mac_link_up = ksz8_phylink_mac_link_up, .config_cpu_port = ksz8_config_cpu_port, .enable_stp_addr = ksz8_enable_stp_addr, .reset = ksz8_reset_switch,
Allow flow control, speed, and duplex settings on the CPU port to be configurable. Previously, the speed and duplex relied on default switch values, which limited flexibility. Additionally, flow control was hardcoded and only functional in duplex mode. This update enhances the configurability of these parameters. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz8.h | 4 +++ drivers/net/dsa/microchip/ksz8795.c | 47 ++++++++++++++++++++++++-- drivers/net/dsa/microchip/ksz_common.c | 1 + 3 files changed, 50 insertions(+), 2 deletions(-)