Messages in this thread | | | From | Sean Anderson <> | Subject | Re: [PATCH] net: phylink: Pass state to pcs_config | Date | Thu, 16 Dec 2021 14:00:32 -0500 |
| |
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
| |