diff mbox series

[net-next,v5,1/3] net: dsa: microchip: ksz8: Make flow control, speed, and duplex on CPU port configurable

Message ID 20231122092545.2895635-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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
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: 1127 this patch: 1127
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 1154 this patch: 1154
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: 1154 this patch: 1154
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 82 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Oleksij Rempel Nov. 22, 2023, 9:25 a.m. UTC 
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>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/microchip/ksz8.h       |  4 ++
 drivers/net/dsa/microchip/ksz8795.c    | 54 +++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.c |  1 +
 3 files changed, 57 insertions(+), 2 deletions(-)

Comments

Russell King (Oracle) Nov. 22, 2023, 9:38 a.m. UTC | #1 
On Wed, Nov 22, 2023 at 10:25:43AM +0100, 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>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  drivers/net/dsa/microchip/ksz8.h       |  4 ++
>  drivers/net/dsa/microchip/ksz8795.c    | 54 +++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz_common.c |  1 +
>  3 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
> index ef653bbfde75..571c26ce71e4 100644
> --- a/drivers/net/dsa/microchip/ksz8.h
> +++ b/drivers/net/dsa/microchip/ksz8.h
> @@ -54,5 +54,9 @@ int ksz8_reset_switch(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 8deb217638d3..3504e7238eba 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -1528,6 +1528,58 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
>  	}
>  }
>  
> +/**
> + * ksz8_cpu_port_link_up - Configures the CPU port of the switch.
> + * @dev: The KSZ device instance.
> + * @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
> + * port of the switch based on the desired settings, current duplex mode, and
> + * speed.
> + */
> +static void ksz8_cpu_port_link_up(struct ksz_device *dev, int speed, int duplex,
> +				  bool tx_pause, bool rx_pause)
> +{
> +	const u16 *regs = dev->info->regs;
> +	u8 ctrl = 0;
> +
> +	/* SW_FLOW_CTRL, SW_HALF_DUPLEX, and SW_10_MBIT bits are bootstrappable
> +	 * at least on KSZ8873. They can have different values depending on your
> +	 * board setup.
> +	 */
> +	if (duplex) {
> +		if (tx_pause || rx_pause)
> +			ctrl |= SW_FLOW_CTRL;

Please don't make this dependent on duplex, and allow phylink to enforce
that. For example, phylink_resolve_an_pause() will only enable tx/rx
pause if in full duplex mode.

> +	} else {
> +		ctrl |= SW_HALF_DUPLEX;
> +	}
> +
> +	/* This hardware only supports SPEED_10 and SPEED_100. For SPEED_10
> +	 * we need to set the SW_10_MBIT bit. Otherwise, we can leave it 0.
> +	 */
> +	if (speed == SPEED_10)
> +		ctrl |= SW_10_MBIT;
> +
> +	ksz_rmw8(dev, regs[S_BROADCAST_CTRL], SW_HALF_DUPLEX | SW_FLOW_CTRL |
> +		 SW_10_MBIT, ctrl);

So this is using SW_FLOW_CTRL with the S_BROADCAST_CTRL register...

> @@ -1576,8 +1628,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);

But previously it was being used with the S_REPLACE_VID_CTRL register.
Doesn't make sense to me.
Oleksij Rempel Nov. 22, 2023, 9:45 a.m. UTC | #2 
On Wed, Nov 22, 2023 at 09:38:09AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 22, 2023 at 10:25:43AM +0100, 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>
> > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  drivers/net/dsa/microchip/ksz8.h       |  4 ++
> >  drivers/net/dsa/microchip/ksz8795.c    | 54 +++++++++++++++++++++++++-
> >  drivers/net/dsa/microchip/ksz_common.c |  1 +
> >  3 files changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
> > index ef653bbfde75..571c26ce71e4 100644
> > --- a/drivers/net/dsa/microchip/ksz8.h
> > +++ b/drivers/net/dsa/microchip/ksz8.h
> > @@ -54,5 +54,9 @@ int ksz8_reset_switch(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 8deb217638d3..3504e7238eba 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -1528,6 +1528,58 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
> >  	}
> >  }
> >  
> > +/**
> > + * ksz8_cpu_port_link_up - Configures the CPU port of the switch.
> > + * @dev: The KSZ device instance.
> > + * @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
> > + * port of the switch based on the desired settings, current duplex mode, and
> > + * speed.
> > + */
> > +static void ksz8_cpu_port_link_up(struct ksz_device *dev, int speed, int duplex,
> > +				  bool tx_pause, bool rx_pause)
> > +{
> > +	const u16 *regs = dev->info->regs;
> > +	u8 ctrl = 0;
> > +
> > +	/* SW_FLOW_CTRL, SW_HALF_DUPLEX, and SW_10_MBIT bits are bootstrappable
> > +	 * at least on KSZ8873. They can have different values depending on your
> > +	 * board setup.
> > +	 */
> > +	if (duplex) {
> > +		if (tx_pause || rx_pause)
> > +			ctrl |= SW_FLOW_CTRL;
> 
> Please don't make this dependent on duplex, and allow phylink to enforce
> that. For example, phylink_resolve_an_pause() will only enable tx/rx
> pause if in full duplex mode.

OK, thx.

> > +	} else {
> > +		ctrl |= SW_HALF_DUPLEX;
> > +	}
> > +
> > +	/* This hardware only supports SPEED_10 and SPEED_100. For SPEED_10
> > +	 * we need to set the SW_10_MBIT bit. Otherwise, we can leave it 0.
> > +	 */
> > +	if (speed == SPEED_10)
> > +		ctrl |= SW_10_MBIT;
> > +
> > +	ksz_rmw8(dev, regs[S_BROADCAST_CTRL], SW_HALF_DUPLEX | SW_FLOW_CTRL |
> > +		 SW_10_MBIT, ctrl);
> 
> So this is using SW_FLOW_CTRL with the S_BROADCAST_CTRL register...
> 
> > @@ -1576,8 +1628,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);
> 
> But previously it was being used with the S_REPLACE_VID_CTRL register.
> Doesn't make sense to me.

Yes, it looks suspicious. There are multiple ways to access same
register. Vladimir pointed to in in the v4 review. S_BROADCAST_CTRL is
used in most recent code. So, i decided to settle down to the variant.

Regards,
Oleksij
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index ef653bbfde75..571c26ce71e4 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -54,5 +54,9 @@  int ksz8_reset_switch(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 8deb217638d3..3504e7238eba 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1528,6 +1528,58 @@  void ksz8_config_cpu_port(struct dsa_switch *ds)
 	}
 }
 
+/**
+ * ksz8_cpu_port_link_up - Configures the CPU port of the switch.
+ * @dev: The KSZ device instance.
+ * @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
+ * port of the switch based on the desired settings, current duplex mode, and
+ * speed.
+ */
+static void ksz8_cpu_port_link_up(struct ksz_device *dev, int speed, int duplex,
+				  bool tx_pause, bool rx_pause)
+{
+	const u16 *regs = dev->info->regs;
+	u8 ctrl = 0;
+
+	/* SW_FLOW_CTRL, SW_HALF_DUPLEX, and SW_10_MBIT bits are bootstrappable
+	 * at least on KSZ8873. They can have different values depending on your
+	 * board setup.
+	 */
+	if (duplex) {
+		if (tx_pause || rx_pause)
+			ctrl |= SW_FLOW_CTRL;
+	} else {
+		ctrl |= SW_HALF_DUPLEX;
+	}
+
+	/* This hardware only supports SPEED_10 and SPEED_100. For SPEED_10
+	 * we need to set the SW_10_MBIT bit. Otherwise, we can leave it 0.
+	 */
+	if (speed == SPEED_10)
+		ctrl |= SW_10_MBIT;
+
+	ksz_rmw8(dev, regs[S_BROADCAST_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 the port is the CPU port, apply special handling. Only the CPU
+	 * port is configured via global registers.
+	 */
+	if (dev->cpu_port == port)
+		ksz8_cpu_port_link_up(dev, speed, duplex, tx_pause, rx_pause);
+}
+
 static int ksz8_handle_global_errata(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
@@ -1576,8 +1628,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 3fed406fb46a..0ee7cfb8d4bd 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -277,6 +277,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,