Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Thu, 23 Nov 2023 11:55:18 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Andrew Lunn <andrew@lunn.ch>,
        Eric Dumazet <edumazet@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Woojung Huh <woojung.huh@microchip.com>,
        Arun Ramadoss <arun.ramadoss@microchip.com>,
        Simon Horman <simon.horman@corigine.com>,
        kernel@pengutronix.de, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net-next v6 2/3] net: dsa: microchip: ksz8: Add function
 to configure ports with integrated PHYs
Message-ID: <ZV89pgQlYPxJGZdR@shell.armlinux.org.uk>
References: <20231123112051.713142-1-o.rempel@pengutronix.de>
 <20231123112051.713142-3-o.rempel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231123112051.713142-3-o.rempel@pengutronix.de>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Nov 23, 2023 at 12:20:50PM +0100, Oleksij Rempel wrote:
> This patch introduces the function 'ksz8_phy_port_link_up' to the
> Microchip KSZ8xxx driver. This function is responsible for setting up
> flow control and duplex settings for the ports that are integrated with
> PHYs.
> 
> The KSZ8795 switch supports asymmetric pause control, which can't be
> fully utilized since a single bit controls both RX and TX pause. Despite
> this, the flow control can be adjusted based on the auto-negotiation
> process, taking into account the capabilities of both link partners.
> 
> On the other hand, the KSZ8873's PORT_FORCE_FLOW_CTRL bit can be set by
> the hardware bootstrap, which ignores the auto-negotiation result.
> Therefore, even in auto-negotiation mode, we need to ensure that this
> bit is correctly set.
> 
> When auto-negotiation isn't in use, we enforce symmetric pause control
> for the KSZ8795 switch.

This doesn't sound right.

I would suggest that if PORT_FORCE_FLOW_CTRL is cleared, and autoneg is
disabled, the result will be that flow control will be disabled.

Also, I think PORT_FORCE_FLOW_CTRL should be used in combination with
permit_pause_to_mac, which you can get at in mac_config() using
state->pause & MLO_PAUSE_AN.

So, what I would suggest is that in mac_config(), the driver stores
in its private data something like:

	dev->manual_flow[port] = !(state->pause & MLO_PAUSE_AN);

And then in mac_link_up(), you can examine this, and if manual_flow
for the port is set _and_ tx_pause is also set, then you can set
PORT_FORCE_FLOW_CTRL.

I think this will give a better approximation to the intent of the
user API (assuming symmetric pause only):

Autoneg		Pause Autoneg	rx,tx	PORT_FORCE_FLOW_CTRL
1		1		x	0
0		1		x	0 (flow control probably disabled)
x		0		1	1 (flow control force enabled)
1		0		0	0 (flow control still depends on
					   aneg result due to hardware)
0		0		0	0 (flow control probably disabled)

Whereas, what I think you're proposing is:

Autoneg		Pause Autoneg	rx,tx	PORT_FORCE_FLOW_CTRL
1		1		x	0
1		0		x	0 (flow control still depends on
					   aneg result)
0		x		1	1 (flow control force enabled)
0		x		0	0 (flow control probably disabled)

The difference is that flow control can be forced with my suggestion
when Autoneg is still enabled, but the pause autoneg bit is cleared
and we want flow control enabled.

Thanks.

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