Date:   Wed, 15 Dec 2021 00:49:14 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Sean Anderson <sean.anderson@seco.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Marcin Wojtas <mw@semihalf.com>, UNGLinuxDriver@microchip.com,
        "David S . Miller" <davem@davemloft.net>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Jose Abreu <Jose.Abreu@synopsys.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH] net: phylink: Pass state to pcs_config
Message-ID: <Ybk7iuxdin69MjTo@shell.armlinux.org.uk>
References: <20211214233450.1488736-1-sean.anderson@seco.com>
 <YbkshnqgXP7Gd188@shell.armlinux.org.uk>
 <de1f7214-58c8-cdc6-1d29-08c979ce68f1@seco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <de1f7214-58c8-cdc6-1d29-08c979ce68f1@seco.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Tue, Dec 14, 2021 at 07:16:53PM -0500, Sean Anderson wrote:
> Ok, so let me clarify my understanding. Perhaps this can be eliminated
> through a different approach.
> 
> When I read the datasheet for mvneta (which hopefully has the same
> logic here, since I could not find a datasheet for an mvpp2 device), I
> noticed that the Pause_Adv bit said
> 
> > It is valid only if flow control mode is defined by Auto-Negotiation
> > (as defined by the <AnFcEn> bit).
> 
> Which I interpreted to mean that if AnFcEn was clear, then no flow
> control was advertised. But perhaps it instead means that the logic is
> something like
> 
> if (AnFcEn)
> 	Config_Reg.PAUSE = Pause_Adv;
> else
> 	Config_Reg.PAUSE = SetFcEn;
> 
> which would mean that we can just clear AnFcEn in link_up if the
> autonegotiated pause settings are different from the configured pause
> settings.

Having actually played with this hardware quite a bit and observed what
it sends, what it implements for advertising is:

	Config_Reg.PAUSE = Pause_Adv;

Config_Reg gets sent over the 1000BASE-X link to the link partner, and
we receive Remote_Reg from the link partner.

Then, the hardware implements:

	if (AnFcEn)
		MAC_PAUSE = Config_Reg.PAUSE & Remote_Reg.PAUSE;
	else
		MAC_PAUSE = SetFcEn;

In otherwords, AnFcEn controls whether the result of autonegotiation
or the value of SetFcEn controls whether the MAC enables symmetric
pause mode.

Pause_Adv comes from the advertisement, and this is controlled from
ethtool -s and somewhat by ethtool -A.

AnFcEn is controlled purely and only by ethtool -A ... autoneg on|off.
You can't derive this from "state".

SetFcEn comes from ethtool -A ... tx and rx parameters (which must
both be on or both be off.)

Since we have no knowledge what Remote_Reg contains (it's not made
accessible by the hardware), it's impossible to derive whether the
autonegotiated pause settings are different from the configured pause
settings.

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