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

On Wed, Dec 15, 2021 at 12:49:14AM +0000, Russell King (Oracle) wrote:
> 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.

I should also note that in the Port Status register,

	TxFcEn = RxFcEn = MAC_PAUSE;

So, the status register bits follow SetFcEn when AnFcEn is disabled.

However, these bits are the only way to report the result of the
negotiation, which is why we use them to report back whether flow
control was enabled in mvneta_pcs_get_state(). These bits will be
ignored by phylink when ethtool -A has disabled pause negotiation,
and in that situation there is no way as I said to be able to read
the negotiation result.

permit_pause_to_mac exists precisely because of the limitions of this
hardware, and having it costs virtually nothing to other network
drivers... except a parameter that others ignore.

If we don't have permit_pause_to_mac in pcs_config() then we need to
have it passed to the link_up() methods instead. There is no other
option (other than breaking mvneta and mvpp2) here than to make the
state of ethtool -A ... autoneg on|off known to the hardware.

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