Re: [PATCH] net: phylink: Pass state to pcs_config
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2021-12-16 18:59:15
Also in:
lkml, netdev
On Thu, Dec 16, 2021 at 01:29:20PM -0500, 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 saidquoted
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 aboutdiff --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.
quoted
quoted
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! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel