Re: [PATCH] net: phylink: Pass state to pcs_config

On 12/16/21 1:29 PM, Sean Anderson wrote:

On 12/16/21 1:05 PM, Russell King (Oracle) wrote:

quoted

On Thu, Dec 16, 2021 at 12:51:33PM -0500, Sean Anderson wrote:

quoted

On 12/16/21 12:26 PM, Russell King (Oracle) wrote:

quoted

On Thu, Dec 16, 2021 at 12:02:55PM -0500, Sean Anderson wrote:

quoted

On 12/14/21 8:12 PM, Russell King (Oracle) wrote:

quoted

On Wed, Dec 15, 2021 at 12:49:14AM +0000, Russell King (Oracle) wrote:

quoted

On Tue, Dec 14, 2021 at 07:16:53PM -0500, Sean Anderson wrote:

quoted

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

quoted

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;

So the above note from the datasheet about Pause_Adv not being valid is
incorrect?

quoted

quoted

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.

Ok, how about

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index b1cce4425296..9b41d8ee71fb 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

@@ -6226,8 +6226,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
                         * automatically or the bits in MVPP22_GMAC_CTRL_4_REG
                         * manually controls the GMAC pause modes.
                         */
-                       if (permit_pause_to_mac)
-                               val |= MVPP2_GMAC_FLOW_CTRL_AUTONEG;
+                       val |= MVPP2_GMAC_FLOW_CTRL_AUTONEG;

                        /* Configure advertisement bits */
                        mask |= MVPP2_GMAC_FC_ADV_EN | MVPP2_GMAC_FC_ADV_ASM_EN;

@@ -6525,6 +6524,9 @@ static void mvpp2_mac_link_up(struct phylink_config *config,
                }
        } else {
                if (!phylink_autoneg_inband(mode)) {
+                       bool cur_tx_pause, cur_rx_pause;
+                       u32 status0 = readl(port->base + MVPP2_GMAC_STATUS0);
+
                        val = MVPP2_GMAC_FORCE_LINK_PASS;

                        if (speed == SPEED_1000 || speed == SPEED_2500)

@@ -6535,11 +6537,18 @@ static void mvpp2_mac_link_up(struct phylink_config *config,
                        if (duplex == DUPLEX_FULL)
                                val |= MVPP2_GMAC_CONFIG_FULL_DUPLEX;

+                       cur_tx_pause = status0 & MVPP2_GMAC_STATUS0_TX_PAUSE;
+                       cur_rx_pause = status0 & MVPP2_GMAC_STATUS0_RX_PAUSE;

I think you haven't understood everything I've said. These status bits
report what the MAC is doing. They do not reflect what was negotiated
_unless_ MVPP2_GMAC_FLOW_CTRL_AUTONEG was set.

So, if we clear MVPP2_GMAC_FLOW_CTRL_AUTONEG, these bits will follow
MVPP22_XLG_CTRL0_TX_FLOW_CTRL_EN and MVPP22_XLG_CTRL0_RX_FLOW_CTRL_EN.

Let's say we apply this patch. tx/rx pause are negotiated and enabled.
So cur_tx_pause and cur_rx_pause are both true.

We change the pause settings, forcing tx pause only. This causes
pcs_config to be called which sets MVPP2_GMAC_FLOW_CTRL_AUTONEG, and
then link_up gets called with the differing settings. We clear
MVPP2_GMAC_FLOW_CTRL_AUTONEG and force the pause settings. We now
have the status register containing MVPP2_GMAC_STATUS0_TX_PAUSE set
but MVPP2_GMAC_STATUS0_RX_PAUSE clear.

The link goes down e.g. because the remote end has changed and comes
back. We read the status register and see MVPP2_GMAC_STATUS0_TX_PAUSE
is set and MVPP2_GMAC_STATUS0_RX_PAUSE is still clear. tx_pause is
true and rx_pause is false. These agree with the settings, so we
then set MVPP2_GMAC_FLOW_CTRL_AUTONEG.

If the link goes down and up again, then this cycle repeats - the
status register will now have both MVPP2_GMAC_STATUS0_TX_PAUSE and
MVPP2_GMAC_STATUS0_RX_PAUSE set, so we clear
MVPP2_GMAC_FLOW_CTRL_AUTONEG. If the link goes down/up again, we flip
back to re-enabling MVPP2_GMAC_FLOW_CTRL_AUTONEG.

Ok, so I think I see what you're getting at here. If we set
MVPP2_GMAC_FLOW_CTRL_AUTONEG after examining the pause mode, then the
pause mode will revert to what was negotiated. But since this platform
uses interrupts, we can clear it in link_up and set it in link_down.

  --Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel