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

From: Sean Anderson
Date: Thu Dec 16 2021 - 14:00:43 EST



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


On 12/16/21 1:05 PM, Russell King (Oracle) wrote:
On Thu, Dec 16, 2021 at 12:51:33PM -0500, Sean Anderson wrote:
On 12/16/21 12:26 PM, Russell King (Oracle) wrote:
> On Thu, Dec 16, 2021 at 12:02:55PM -0500, Sean Anderson wrote:
> > On 12/14/21 8:12 PM, Russell King (Oracle) wrote:
> > > 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;
> >
> > So the above note from the datasheet about Pause_Adv not being valid is
> > incorrect?
> >
> > > > 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