Re: [PATCH net-next v3 2/2] net: dsa: microchip: ksz8: Add function to configure downstream ports for KSZ8xxx

[Simon Horman]

On Thu, May 18, 2023 at 11:29:13AM +0200, Oleksij Rempel wrote:
> This patch introduces the function 'ksz8_downstream_link_up' to the
> Microchip KSZ8xxx driver. This function configures the flow control settings
> for the downstream ports of the switch based on desired settings and the
> current duplex mode.
> 
> The KSZ8795 switch, unlike the KSZ8873, supports asynchronous pause control.
> However, a single bit controls both RX and TX pause, so we can't enforce
> asynchronous pause control. The flow control can be set based on the
> auto-negotiation process, depending on the capabilities of both link partners.
> 
> For the KSZ8873, the PORT_FORCE_FLOW_CTRL bit can be set by the hardware
> bootstrap, ignoring the auto-negotiation result. Therefore, even in
> auto-negotiation mode, we need to ensure that the PORT_FORCE_FLOW_CTRL bit is
> correctly set.
> 
> In the absence of auto-negotiation, we will enforce synchronous pause control
> for the KSZ8795 switch.
> 
> Note: It is currently not possible to force disable flow control on a port if
> we still advertise pause support. This configuration is not currently supported
> by Linux, and it may not make practical sense. However, it's essential to
> understand this limitation when working with the KSZ8873 and similar devices.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>

Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>

> +static void ksz8_downstream_link_up(struct ksz_device *dev, int port,
> +				   int duplex, bool tx_pause, bool rx_pause)
> +{
> +	const u16 *regs = dev->info->regs;
> +	u8 ctrl = 0;
> +	int ret;
> +
> +	/*
> +	 * The KSZ8795 switch differs from the KSZ8873 by supporting
> +	 * asynchronous pause control. However, since a single bit is used to
> +	 * control both RX and TX pause, we can't enforce asynchronous pause
> +	 * control - both TX and RX pause will be either enabled or disabled
> +	 * together.
> +	 *
> +	 * If auto-negotiation is enabled, we usually allow the flow control to
> +	 * be determined by the auto-negotiation process based on the
> +	 * capabilities of both link partners. However, for KSZ8873, the
> +	 * PORT_FORCE_FLOW_CTRL bit may be set by the hardware bootstrap,
> +	 * ignoring the auto-negotiation result. Thus, even in auto-negotiatio
> +	 * mode, we need to ensure that the PORT_FORCE_FLOW_CTRL bit is
> +	 * properly cleared.
> +	 *
> +	 * In the absence of auto-negotiation, we will enforce synchronous
> +	 * pause control for both variants of switches - KSZ8873 and KSZ8795.
> +	 */

nit: multi-line comments in networking code are like this:

	/* This is
	 * a wrap.
	 */

...