Messages in this thread | | | Date | Thu, 16 Dec 2021 18:58:56 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH] net: phylink: Pass state to pcs_config |
| |
On Thu, Dec 16, 2021 at 01:29:20PM -0500, 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. > > > > > > The toggling is not really a problem, since we always correct the pause > > > mode as soon as we notice. > > > > When do we "notice" ? We don't regularly poll on these platforms, we > > rely on interrupts. > > When the link comes back up again.
So we end up with link-down-link-up and the settings are wrong. The next time the link goes down and up, the settings are corrected as I described. The next time the link goes down and up, the settings are again wrong. etc. I don't consider this acceptable.
> > > The real issue would be if we don't notice > > > because the link went down and back up in between calls to > > > phylink_resolve. > > > > Err no. If the link goes down and back up, one of the things the code > > is structured to ensure is that phylink_resolve gets called, _and_ that > > we will see a link-down-link-up. > > Great. Then the above will work fine. Because we always set the pause > mode in mac_link_up, it's OK to have the pause be incorrect in the time > between when it comes up and when mac_link_up is called. The result of > the pause from get_state will not be what was negotiated, but that is OK > because we discard it anyway.
I can't tell whether you are intentionally misinterpreting to try and get your way or not, but I've now said several times that your proposal will _not_ work, yet you keep with some weird idea that I'm somehow agreeing with you.
I'm not.
> I think my primary issue with the name is that it is named after its > purpose in the marvell hardware, and not after the user setting. That > is, we have something like > > if (pause_autonegotiation_enabled()) > permit_pause_to_mac() > > So the generic interface should be named after the condition and not > the body. > > If this parameter got moved to mac_link_up, I think the following would > be good: > > @autonegotiated_pause: This indicates whether the pause settings are a > result of autonegotiation or whether they were manually configured. Some > MACs are tightly coupled to their PCSs and have a hardware > implementation of linkmode_resolve_pause() which sets their pause mode > based on the autonegotiated pause mode. For these MACs, disabling this > hardware implementation may inhibit their ability to determine the > autonegotiated pause mode, so it should only be disabled when the pause > mode was manually set. MACs which do not have this feature/limitation > should ignore this parameter.
I might agree with that, but I haven't read it because I'm feeling way too frazzled this evening to do so (not your fault, just an afternoon of pressure.)
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |