| Message ID | 20240524210304.9164-2-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next,1/3] net: stmmac: Prevent RGSMIIIS IRQs flood | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-0 |
On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > into the DW GMAC controller. It's always done if the controller supports > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > interfaces support was activated during the IP-core synthesize the PCS > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > set. Based on that the RGMII in-band status detection procedure > implemented in the driver hasn't been working for the devices with the > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > interfaces available in the device. > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > statement responsible for the In-band/PCS functionality activation. If the > RGMII interface is supported by the device then the in-band link status > detection will be also supported automatically (it's always embedded into > the RGMII RTL code). If the SGMII interface is supported by the device > then the PCS block will be supported too (it's unconditionally synthesized > into the controller). The later is also correct for the TBI/RTBI PHY > interfaces. > > Note while at it drop the netdev_dbg() calls since at the moment of the > stmmac_check_pcs_mode() invocation the network device isn't registered. So > the debug prints will be for the unknown/NULL device. Thanks. As this is a fix, shouldn't it be submitted for the net tree as it seems to be fixing a bug in the driver as it stands today? Also, a build fix is required here: > - if (priv->dma_cap.pcs) { > - if ((interface == PHY_INTERFACE_MODE_RGMII) || > - (interface == PHY_INTERFACE_MODE_RGMII_ID) || > - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > - priv->hw->pcs = STMMAC_PCS_RGMII; > - } else if (interface == PHY_INTERFACE_MODE_SGMII) { > - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > - priv->hw->pcs = STMMAC_PCS_SGMII; > - } > - } > + if (phy_interface_mode_is_rgmii(interface)) > + priv->hw.pcs = STMMAC_PCS_RGMII; > + else if (interface == PHY_INTERFACE_MODE_SGMII) > + priv->hw.pcs = STMMAC_PCS_SGMII; Both of these assignments should be priv->hw->pcs not priv->hw.pcs. I think there's also another bug that needs fixing along with this. See stmmac_ethtool_set_link_ksettings(). Note that this denies the ability to disable autoneg, which (a) doesn't make sense for RGMII with an attached PHY, and (b) this code should be passing the ethtool op to phylink for it to pass on to phylib so the PHY can be appropriately configured for the users desired autoneg and link mode settings. I also don't think it makes any sense for the STMMAC_PCS_SGMII case given that it means Cisco SGMII - which implies that there is also a PHY (since Cisco SGMII with inband is designed to be coupled with something that looks like a PHY to send the inband signalling necessary to configure e.g. the SGMII link symbol replication. In both of these cases, even if the user requests autoneg to be disabled, that _shouldn't_ affect internal network driver links. This ethtool op is about configuring the externally visible media side of the network driver, not the internal links.
On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > into the DW GMAC controller. It's always done if the controller supports > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > interfaces support was activated during the IP-core synthesize the PCS > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > set. Based on that the RGMII in-band status detection procedure > > implemented in the driver hasn't been working for the devices with the > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > interfaces available in the device. > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > statement responsible for the In-band/PCS functionality activation. If the > > RGMII interface is supported by the device then the in-band link status > > detection will be also supported automatically (it's always embedded into > > the RGMII RTL code). If the SGMII interface is supported by the device > > then the PCS block will be supported too (it's unconditionally synthesized > > into the controller). The later is also correct for the TBI/RTBI PHY > > interfaces. > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > the debug prints will be for the unknown/NULL device. > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > it seems to be fixing a bug in the driver as it stands today? > > Also, a build fix is required here: > > > - if (priv->dma_cap.pcs) { > > - if ((interface == PHY_INTERFACE_MODE_RGMII) || > > - (interface == PHY_INTERFACE_MODE_RGMII_ID) || > > - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > > - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > > - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > > - priv->hw->pcs = STMMAC_PCS_RGMII; > > - } else if (interface == PHY_INTERFACE_MODE_SGMII) { > > - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > > - priv->hw->pcs = STMMAC_PCS_SGMII; > > - } > > - } > > + if (phy_interface_mode_is_rgmii(interface)) > > + priv->hw.pcs = STMMAC_PCS_RGMII; > > + else if (interface == PHY_INTERFACE_MODE_SGMII) > > + priv->hw.pcs = STMMAC_PCS_SGMII; > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs. > > I think there's also another bug that needs fixing along with this. > See stmmac_ethtool_set_link_ksettings(). Note that this denies the > ability to disable autoneg, which (a) doesn't make sense for RGMII > with an attached PHY, and (b) this code should be passing the > ethtool op to phylink for it to pass on to phylib so the PHY can > be appropriately configured for the users desired autoneg and > link mode settings. > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case > given that it means Cisco SGMII - which implies that there is also > a PHY (since Cisco SGMII with inband is designed to be coupled with > something that looks like a PHY to send the inband signalling > necessary to configure e.g. the SGMII link symbol replication. > > In both of these cases, even if the user requests autoneg to be > disabled, that _shouldn't_ affect internal network driver links. > This ethtool op is about configuring the externally visible media > side of the network driver, not the internal links. I have a concern about this patch. Have you considered dwmac-intel with its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X support. Does the dwmac-intel version of the core set priv->dma_cap.pcs? If it doesn't, then removing the test on this will cause a regression, since in Cisco SGMII mode, we end up setting priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will enable all the "integrated PCS" code paths despite XPCS clearly intending to be used for Cisco SGMII. I'm also wondering whether the same applies to the lynx PCS as well, or in the general case if we have any kind of external PCS. Hence, I think this probably needs to be: if (phy_interface_mode_is_rgmii(interface)) priv->hw->pcs = STMMAC_PCS_RGMII; else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs) priv->hw->pcs = STMMAC_PCS_SGMII; At least this is what unpicking the awful stmmac code suggests (and I do feel that my point about the shocking state of this driver is proven as details like this are extremely difficult to unpick, and not unpicking them correctly will lead to regressions.) Therefore, I would suggest that it would be wise if you also double-checked this. If my analysis is correct, then my changes to stmmac_mac_select_pcs() are also wrong.
On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > into the DW GMAC controller. It's always done if the controller supports > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > interfaces support was activated during the IP-core synthesize the PCS > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > set. Based on that the RGMII in-band status detection procedure > > implemented in the driver hasn't been working for the devices with the > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > interfaces available in the device. > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > statement responsible for the In-band/PCS functionality activation. If the > > RGMII interface is supported by the device then the in-band link status > > detection will be also supported automatically (it's always embedded into > > the RGMII RTL code). If the SGMII interface is supported by the device > > then the PCS block will be supported too (it's unconditionally synthesized > > into the controller). The later is also correct for the TBI/RTBI PHY > > interfaces. > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > the debug prints will be for the unknown/NULL device. > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > it seems to be fixing a bug in the driver as it stands today? From one point of view it could be submitted for the net tree indeed, but on the second thought are you sure we should be doing that seeing it will activate the RGMII-inband detection and the code with the netif-carrier toggling behind the phylink back? Who knows what new regressions the activated PCS-code can cause?.. > > Also, a build fix is required here: > > > - if (priv->dma_cap.pcs) { > > - if ((interface == PHY_INTERFACE_MODE_RGMII) || > > - (interface == PHY_INTERFACE_MODE_RGMII_ID) || > > - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > > - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > > - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > > - priv->hw->pcs = STMMAC_PCS_RGMII; > > - } else if (interface == PHY_INTERFACE_MODE_SGMII) { > > - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > > - priv->hw->pcs = STMMAC_PCS_SGMII; > > - } > > - } > > + if (phy_interface_mode_is_rgmii(interface)) > > + priv->hw.pcs = STMMAC_PCS_RGMII; > > + else if (interface == PHY_INTERFACE_MODE_SGMII) > > + priv->hw.pcs = STMMAC_PCS_SGMII; > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs. Ah, right. Originally I applied your patchset on top of my fixes, cleanups and platform glue-driver patchsets. One of the cleanups implied the mac_device_info instance movement to the stmmac_priv structure. When I was moving my changes onto your original series I just missed that part of the patch. Sorry about that. The rest of my patches seems free from such problem. > > I think there's also another bug that needs fixing along with this. > See stmmac_ethtool_set_link_ksettings(). Note that this denies the > ability to disable autoneg, which (a) doesn't make sense for RGMII > with an attached PHY, and This doesn't make sense for RGMII also due to DW GMAC/QoS Eth not having anything AN-related for the RGMII PHY interface. RGMII mode permits the Link status detection via the in-band signal retrieved from the PHY and nothing else. AN, if enabled, is performed on the PHY side. > (b) this code should be passing the > ethtool op to phylink for it to pass on to phylib so the PHY can > be appropriately configured for the users desired autoneg and > link mode settings. I am not that well aware of the phylink internals to be saying for 100% sure, but thinking logically it would be indeed better if phylink was aware of the PCS state changes. But in case of the STMMAC PCS implementation I guess that the original PCS-code was designed to work with no PHY being involved: e58bb43f5e43 ("stmmac: initial support to manage pcs modes") See that commit prevented the MDIO-bus registration and PHY initialization in case of the PCS/RGMII-inband being available. But in practice the implementation turned to be not that well thought through. So eventually, commit-by-commit, the implementation was effectively converted to the no longer used code. At least for the MACs with just RGMII interface and no additional SGMII/TBI/RTBI interfaces, which I guess is the vast majority of the real devices with RGMII. > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case > given that it means Cisco SGMII - which implies that there is also > a PHY (since Cisco SGMII with inband is designed to be coupled with > something that looks like a PHY to send the inband signalling > necessary to configure e.g. the SGMII link symbol replication. AFAICS the STMMAC driver supports the MAC2MAC case connected over SGMII with no intermediate PHY. In that case the speed will be just fixed to what was set in the "snps,ps-speed" property. The RAL (Rate Adapter Layer) is configured to do that by having the SGMRAL flag set (see your dwmac_pcs_config() and what is done if hw->ps is non-zero). > > In both of these cases, even if the user requests autoneg to be > disabled, that _shouldn't_ affect internal network driver links. > This ethtool op is about configuring the externally visible media > side of the network driver, not the internal links. IMO considering all the driver over-complexity (that's the most polite definition I managed to come up to.)) it would be much easier and likely safer not to try to fix the PCS-code and just convert it to something sane. At least the RGMII/In-band functionality we'll be able to test on my device. If the PCS SGMII part is still utilized by anybody, then if there are problems there the new kernel RCs will get to reveal them. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, May 27, 2024 at 12:57:02AM +0300, Serge Semin wrote: > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > > into the DW GMAC controller. It's always done if the controller supports > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > > interfaces support was activated during the IP-core synthesize the PCS > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > > set. Based on that the RGMII in-band status detection procedure > > > implemented in the driver hasn't been working for the devices with the > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > > interfaces available in the device. > > > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > > statement responsible for the In-band/PCS functionality activation. If the > > > RGMII interface is supported by the device then the in-band link status > > > detection will be also supported automatically (it's always embedded into > > > the RGMII RTL code). If the SGMII interface is supported by the device > > > then the PCS block will be supported too (it's unconditionally synthesized > > > into the controller). The later is also correct for the TBI/RTBI PHY > > > interfaces. > > > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > > the debug prints will be for the unknown/NULL device. > > > > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > > it seems to be fixing a bug in the driver as it stands today? > > From one point of view it could be submitted for the net tree indeed, > but on the second thought are you sure we should be doing that seeing > it will activate the RGMII-inband detection and the code with the > netif-carrier toggling behind the phylink back? Who knows what new > regressions the activated PCS-code can cause?.. If it's not a fix that is suitable without the remainder of the patch set, this should be stated in the commit description and it shouldn't have a Fixes: tag. The reason is because it wouldn't be stable kernel material without the other patches - if stable picks it up without the other patches then it could end up being applied without the other patches resulting in the situation you mention above. Shall I remove the Fixes: tag?
On Tue, May 28, 2024 at 11:21:39AM +0100, Russell King (Oracle) wrote: > On Mon, May 27, 2024 at 12:57:02AM +0300, Serge Semin wrote: > > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > > > into the DW GMAC controller. It's always done if the controller supports > > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > > > interfaces support was activated during the IP-core synthesize the PCS > > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > > > set. Based on that the RGMII in-band status detection procedure > > > > implemented in the driver hasn't been working for the devices with the > > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > > > interfaces available in the device. > > > > > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > > > statement responsible for the In-band/PCS functionality activation. If the > > > > RGMII interface is supported by the device then the in-band link status > > > > detection will be also supported automatically (it's always embedded into > > > > the RGMII RTL code). If the SGMII interface is supported by the device > > > > then the PCS block will be supported too (it's unconditionally synthesized > > > > into the controller). The later is also correct for the TBI/RTBI PHY > > > > interfaces. > > > > > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > > > the debug prints will be for the unknown/NULL device. > > > > > > > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > > > it seems to be fixing a bug in the driver as it stands today? > > > > From one point of view it could be submitted for the net tree indeed, > > but on the second thought are you sure we should be doing that seeing > > it will activate the RGMII-inband detection and the code with the > > netif-carrier toggling behind the phylink back? Who knows what new > > regressions the activated PCS-code can cause?.. > > If it's not a fix that is suitable without the remainder of the patch > set, this should be stated in the commit description and it shouldn't > have a Fixes: tag. > > The reason is because it wouldn't be stable kernel material without the > other patches - if stable picks it up without the other patches then > it could end up being applied without the other patches resulting in > the situation you mention above. > > Shall I remove the Fixes: tag? Let's drop it then, so not to cause confusion for the maintainers. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote: > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > > into the DW GMAC controller. It's always done if the controller supports > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > > interfaces support was activated during the IP-core synthesize the PCS > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > > set. Based on that the RGMII in-band status detection procedure > > > implemented in the driver hasn't been working for the devices with the > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > > interfaces available in the device. > > > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > > statement responsible for the In-band/PCS functionality activation. If the > > > RGMII interface is supported by the device then the in-band link status > > > detection will be also supported automatically (it's always embedded into > > > the RGMII RTL code). If the SGMII interface is supported by the device > > > then the PCS block will be supported too (it's unconditionally synthesized > > > into the controller). The later is also correct for the TBI/RTBI PHY > > > interfaces. > > > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > > the debug prints will be for the unknown/NULL device. > > > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > > it seems to be fixing a bug in the driver as it stands today? > > > > Also, a build fix is required here: > > > > > - if (priv->dma_cap.pcs) { > > > - if ((interface == PHY_INTERFACE_MODE_RGMII) || > > > - (interface == PHY_INTERFACE_MODE_RGMII_ID) || > > > - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > > > - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > > > - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > > > - priv->hw->pcs = STMMAC_PCS_RGMII; > > > - } else if (interface == PHY_INTERFACE_MODE_SGMII) { > > > - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > > > - priv->hw->pcs = STMMAC_PCS_SGMII; > > > - } > > > - } > > > + if (phy_interface_mode_is_rgmii(interface)) > > > + priv->hw.pcs = STMMAC_PCS_RGMII; > > > + else if (interface == PHY_INTERFACE_MODE_SGMII) > > > + priv->hw.pcs = STMMAC_PCS_SGMII; > > > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs. > > > > I think there's also another bug that needs fixing along with this. > > See stmmac_ethtool_set_link_ksettings(). Note that this denies the > > ability to disable autoneg, which (a) doesn't make sense for RGMII > > with an attached PHY, and (b) this code should be passing the > > ethtool op to phylink for it to pass on to phylib so the PHY can > > be appropriately configured for the users desired autoneg and > > link mode settings. > > > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case > > given that it means Cisco SGMII - which implies that there is also > > a PHY (since Cisco SGMII with inband is designed to be coupled with > > something that looks like a PHY to send the inband signalling > > necessary to configure e.g. the SGMII link symbol replication. > > > > In both of these cases, even if the user requests autoneg to be > > disabled, that _shouldn't_ affect internal network driver links. > > This ethtool op is about configuring the externally visible media > > side of the network driver, not the internal links. > > I have a concern about this patch. Have you considered dwmac-intel with > its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X > support. Does the dwmac-intel version of the core set > priv->dma_cap.pcs? If it doesn't, then removing the test on this will > cause a regression, since in Cisco SGMII mode, we end up setting > priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As > priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will > enable all the "integrated PCS" code paths despite XPCS clearly > intending to be used for Cisco SGMII. > > I'm also wondering whether the same applies to the lynx PCS as well, > or in the general case if we have any kind of external PCS. > > Hence, I think this probably needs to be: > > if (phy_interface_mode_is_rgmii(interface)) > priv->hw->pcs = STMMAC_PCS_RGMII; > else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs) > priv->hw->pcs = STMMAC_PCS_SGMII; > > At least this is what unpicking the awful stmmac code suggests (and I > do feel that my point about the shocking state of this driver is proven > as details like this are extremely difficult to unpick, and not > unpicking them correctly will lead to regressions.) Therefore, I would > suggest that it would be wise if you also double-checked this. Double-checked that part. Indeed this is what I forgot to take into account. (Just realized I had a glimpse thought about checking the DW xGMAC/XPCS for supporting the SGMII interface, but the thought got away from my mind forgotten.) DW XPCS can be synthesized with having the GMII/MII interface connected to the MAC and SGMII downstream interface over a single 1000Base-X lane. In anyway AFAICS that case has nothing to do with the PCS embedded into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW XGMAC has no embedded PCS, but could be attached to the separate DW XPCS device. About the correct implementation. Right, priv->dma_cap.pcs indicates that there is an embedded PCS and the flag can be set for DW GMAC or DW QoS Eth only. Although I would change the order: if (phy_interface_mode_is_rgmii(interface)) priv->hw->pcs = STMMAC_PCS_RGMII; else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII) priv->hw->pcs = STMMAC_PCS_SGMII; since priv->dma_cap.pcs is a primary flag. If it isn't set the interface will be irrelevant. Alternative solution could be to use the has_gmac/has_gmac4 flags instead. That will emphasize that the embedded PCS is expected to be specific for the DW GMAC and DW QoS Eth IP-cores: if (phy_interface_mode_is_rgmii(interface)) priv->hw->pcs = STMMAC_PCS_RGMII; else if ((priv->plat.has_gmac || priv->plat.has_gmac4) && interface == PHY_INTERFACE_MODE_SGMII) priv->hw->pcs = STMMAC_PCS_SGMII; -Serge(y) > > If my analysis is correct, then my changes to stmmac_mac_select_pcs() > are also wrong. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, May 28, 2024 at 04:19:49PM +0300, Serge Semin wrote: > On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote: > > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > > > into the DW GMAC controller. It's always done if the controller supports > > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > > > interfaces support was activated during the IP-core synthesize the PCS > > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > > > set. Based on that the RGMII in-band status detection procedure > > > > implemented in the driver hasn't been working for the devices with the > > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > > > interfaces available in the device. > > > > > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > > > statement responsible for the In-band/PCS functionality activation. If the > > > > RGMII interface is supported by the device then the in-band link status > > > > detection will be also supported automatically (it's always embedded into > > > > the RGMII RTL code). If the SGMII interface is supported by the device > > > > then the PCS block will be supported too (it's unconditionally synthesized > > > > into the controller). The later is also correct for the TBI/RTBI PHY > > > > interfaces. > > > > > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > > > the debug prints will be for the unknown/NULL device. > > > > > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > > > it seems to be fixing a bug in the driver as it stands today? > > > > > > Also, a build fix is required here: > > > > > > > - if (priv->dma_cap.pcs) { > > > > - if ((interface == PHY_INTERFACE_MODE_RGMII) || > > > > - (interface == PHY_INTERFACE_MODE_RGMII_ID) || > > > > - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > > > > - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > > > > - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > > > > - priv->hw->pcs = STMMAC_PCS_RGMII; > > > > - } else if (interface == PHY_INTERFACE_MODE_SGMII) { > > > > - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > > > > - priv->hw->pcs = STMMAC_PCS_SGMII; > > > > - } > > > > - } > > > > + if (phy_interface_mode_is_rgmii(interface)) > > > > + priv->hw.pcs = STMMAC_PCS_RGMII; > > > > + else if (interface == PHY_INTERFACE_MODE_SGMII) > > > > + priv->hw.pcs = STMMAC_PCS_SGMII; > > > > > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs. > > > > > > I think there's also another bug that needs fixing along with this. > > > See stmmac_ethtool_set_link_ksettings(). Note that this denies the > > > ability to disable autoneg, which (a) doesn't make sense for RGMII > > > with an attached PHY, and (b) this code should be passing the > > > ethtool op to phylink for it to pass on to phylib so the PHY can > > > be appropriately configured for the users desired autoneg and > > > link mode settings. > > > > > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case > > > given that it means Cisco SGMII - which implies that there is also > > > a PHY (since Cisco SGMII with inband is designed to be coupled with > > > something that looks like a PHY to send the inband signalling > > > necessary to configure e.g. the SGMII link symbol replication. > > > > > > In both of these cases, even if the user requests autoneg to be > > > disabled, that _shouldn't_ affect internal network driver links. > > > This ethtool op is about configuring the externally visible media > > > side of the network driver, not the internal links. > > > > > I have a concern about this patch. Have you considered dwmac-intel with > > its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X > > support. Does the dwmac-intel version of the core set > > priv->dma_cap.pcs? If it doesn't, then removing the test on this will > > cause a regression, since in Cisco SGMII mode, we end up setting > > priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As > > priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will > > enable all the "integrated PCS" code paths despite XPCS clearly > > intending to be used for Cisco SGMII. > > > > I'm also wondering whether the same applies to the lynx PCS as well, > > or in the general case if we have any kind of external PCS. > > > > Hence, I think this probably needs to be: > > > > if (phy_interface_mode_is_rgmii(interface)) > > priv->hw->pcs = STMMAC_PCS_RGMII; > > else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs) > > priv->hw->pcs = STMMAC_PCS_SGMII; > > > > At least this is what unpicking the awful stmmac code suggests (and I > > do feel that my point about the shocking state of this driver is proven > > as details like this are extremely difficult to unpick, and not > > unpicking them correctly will lead to regressions.) Therefore, I would > > suggest that it would be wise if you also double-checked this. > > Double-checked that part. Indeed this is what I forgot to take into > account. Thanks for double-checking it. > (Just realized I had a glimpse thought about checking the DW > xGMAC/XPCS for supporting the SGMII interface, but the thought got > away from my mind forgotten.) DW XPCS can be synthesized with having > the GMII/MII interface connected to the MAC and SGMII downstream > interface over a single 1000Base-X lane. > > In anyway AFAICS that case has nothing to do with the PCS embedded > into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW > XGMAC has no embedded PCS, but could be attached to the separate DW > XPCS device. This is where my head starts spinning, because identifying what "DW GMAC" and "DW QoS Eth" refer to is difficult unless one, I guess, has the documentation. The only references to QoS that I can find in the driver refer to per-DMA channel interrupts, dwmac5* and one mention for a platform driver in the Kconfig. Grepping for "DW GMAC" doesn't give anything. Conversely, I know from the code that only dwmac4 and dwmac1000 have support for the integrated PCS. So trying to put this together doesn't make much sense to me. :/ Maybe "DW QoS Eth" refers to dwmac-dwc-qos-eth.c? > About the correct implementation. Right, priv->dma_cap.pcs indicates > that there is an embedded PCS and the flag can be set for DW GMAC or DW > QoS Eth only. Although I would change the order: > > if (phy_interface_mode_is_rgmii(interface)) > priv->hw->pcs = STMMAC_PCS_RGMII; > else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII) > priv->hw->pcs = STMMAC_PCS_SGMII; > > since priv->dma_cap.pcs is a primary flag. If it isn't set the > interface will be irrelevant. As this is generic code, it probably makes sense to go with that, since priv->dma_cap.pcs indicates whether the internal PCS for SGMII is present or not rather than... > Alternative solution could be to use the has_gmac/has_gmac4 flags > instead. That will emphasize that the embedded PCS is expected to be > specific for the DW GMAC and DW QoS Eth IP-cores: > > if (phy_interface_mode_is_rgmii(interface)) > priv->hw->pcs = STMMAC_PCS_RGMII; > else if ((priv->plat.has_gmac || priv->plat.has_gmac4) && > interface == PHY_INTERFACE_MODE_SGMII) > priv->hw->pcs = STMMAC_PCS_SGMII; which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c) will always have its internal PCS if we're using SGMII mode. Does this mean it is true that these cores will never be used with an external PCS? If there is a hardware flag that indicates the PCS is implemented, then I think using that to gate whether SGMII uses the internal PCS is better rather than using the core type. Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS) is being used, the internal PCS will not have been synthesized, and thus priv->dma_cap.pcs will be false? The reason I'd like to know this is because in the future, I'd like to eliminate priv->hw->pcs, and just have dwmac1000/dwmac4's phylink_select_pcs() method make the decisions. If not, then we need to think about the behaviour that stmmac_mac_select_pcs(0 should have. Should it give priority to the internal PCS over external PCS, or external PCS first (in which case what do we need to do with the internal PCS.)
On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote: > > Alternative solution could be to use the has_gmac/has_gmac4 flags > > instead. That will emphasize that the embedded PCS is expected to be > > specific for the DW GMAC and DW QoS Eth IP-cores: > > > > if (phy_interface_mode_is_rgmii(interface)) > > priv->hw->pcs = STMMAC_PCS_RGMII; > > else if ((priv->plat.has_gmac || priv->plat.has_gmac4) && > > interface == PHY_INTERFACE_MODE_SGMII) > > priv->hw->pcs = STMMAC_PCS_SGMII; > > which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c) > will always have its internal PCS if we're using SGMII mode. Does this > mean it is true that these cores will never be used with an external > PCS? Sorry to go off on a related tangent, but I've just been looking at hw->ps which is related to this. As I understand, hw->ps comes from the "snps,ps-speed" property in DT, which is used for SGMII and MAC2MAC connections. Presumably for the SGMII case, this is used where the port is made to look like the PHY end of the SGMII link. I'm guessing MAC2MAC refers to RGMII, or does that also refer to SGMII-as-PHY? I think it would've been nice to have picked SGMII-as-PHY up in the driver earlier - we don't tend to use the "normal" PHY interface mode names, instead we have the REVxxx modes, so I think this _should_ have introduced PHY_INTERFACE_MODE_REVSGMII. In any case, moving on... in stmmac_hw_setup(), we have: /* PS and related bits will be programmed according to the speed */ if (priv->hw->pcs) { int speed = priv->plat->mac_port_sel_speed; if ((speed == SPEED_10) || (speed == SPEED_100) || (speed == SPEED_1000)) { priv->hw->ps = speed; } else { dev_warn(priv->device, "invalid port speed\n"); priv->hw->ps = 0; } } Which means that if we're using the integrated PCS, then we basically require the "snps,ps-speed" property otherwise we'll issue a warning at this point... this seems to imply that reverse mode is the only mode supported, which I'm fairly sure is false. So, maybe this shouldn't be issuing the warning if mac_port_sel_speed was zero? Moving on... hw->ps can only be 10M, 100M or 1G speeds and nothing else - which is fine since RGMII and Cisco SGMII only support these speeds. dwmac1000 tests for this against these speeds, so it is also fine. dwmac4 is basically the same as dwmac1000, so is also fine. The core code as it stands today passes this into the pcs_ctrl_ane method's rsgmi_ral argument, which sets GMAC_AN_CTRL_SGMRAL. Presumably this selects "reverse" mode for both SGMII and RGMII? Persuing this a bit futher, qcom-ethqos always calls this with rsgmi_ral clear. Presumably, qcom-ethqos never specifies "snps,ps-speed" in DT, and thus always gets the warning above? Finally, we get to the core issue, which is dwxgmac2_core.c. dwxgmac2 tests this member against 10G, 2.5G and "any other non-zero value". Out of all of these, the only possible path through that code would be the one which results in: tx |= hw->link.speed1000; Neither of the other two (2.5G and 10G) are possible because those aren't legal values for hw->ps. Moreover, it doesn't appear to have any kind of PCS, so I'm wondering whether any of this code gets used. So, I suspect some of this is "not quite right" either, and I wonder about the implications of changing how hw->pcs is set - whether we first need to fix the code above dealing with priv->hw->ps ? I'm also wondering what impact this has on my PCS conversion.
On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote: > On Tue, May 28, 2024 at 04:19:49PM +0300, Serge Semin wrote: > > On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote: > > > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote: > > > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote: > > > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized > > > > > into the DW GMAC controller. It's always done if the controller supports > > > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these > > > > > interfaces support was activated during the IP-core synthesize the PCS > > > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be > > > > > set. Based on that the RGMII in-band status detection procedure > > > > > implemented in the driver hasn't been working for the devices with the > > > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY > > > > > interfaces available in the device. > > > > > > > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional > > > > > statement responsible for the In-band/PCS functionality activation. If the > > > > > RGMII interface is supported by the device then the in-band link status > > > > > detection will be also supported automatically (it's always embedded into > > > > > the RGMII RTL code). If the SGMII interface is supported by the device > > > > > then the PCS block will be supported too (it's unconditionally synthesized > > > > > into the controller). The later is also correct for the TBI/RTBI PHY > > > > > interfaces. > > > > > > > > > > Note while at it drop the netdev_dbg() calls since at the moment of the > > > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So > > > > > the debug prints will be for the unknown/NULL device. > > > > > > > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as > > > > it seems to be fixing a bug in the driver as it stands today? > > > > > > > > Also, a build fix is required here: > > > > > > > > > - if (priv->dma_cap.pcs) { > > > > > - if ((interface == PHY_INTERFACE_MODE_RGMII) || > > > > > - (interface == PHY_INTERFACE_MODE_RGMII_ID) || > > > > > - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > > > > > - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > > > > > - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > > > > > - priv->hw->pcs = STMMAC_PCS_RGMII; > > > > > - } else if (interface == PHY_INTERFACE_MODE_SGMII) { > > > > > - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > > > > > - priv->hw->pcs = STMMAC_PCS_SGMII; > > > > > - } > > > > > - } > > > > > + if (phy_interface_mode_is_rgmii(interface)) > > > > > + priv->hw.pcs = STMMAC_PCS_RGMII; > > > > > + else if (interface == PHY_INTERFACE_MODE_SGMII) > > > > > + priv->hw.pcs = STMMAC_PCS_SGMII; > > > > > > > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs. > > > > > > > > I think there's also another bug that needs fixing along with this. > > > > See stmmac_ethtool_set_link_ksettings(). Note that this denies the > > > > ability to disable autoneg, which (a) doesn't make sense for RGMII > > > > with an attached PHY, and (b) this code should be passing the > > > > ethtool op to phylink for it to pass on to phylib so the PHY can > > > > be appropriately configured for the users desired autoneg and > > > > link mode settings. > > > > > > > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case > > > > given that it means Cisco SGMII - which implies that there is also > > > > a PHY (since Cisco SGMII with inband is designed to be coupled with > > > > something that looks like a PHY to send the inband signalling > > > > necessary to configure e.g. the SGMII link symbol replication. > > > > > > > > In both of these cases, even if the user requests autoneg to be > > > > disabled, that _shouldn't_ affect internal network driver links. > > > > This ethtool op is about configuring the externally visible media > > > > side of the network driver, not the internal links. > > > > > > > > I have a concern about this patch. Have you considered dwmac-intel with > > > its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X > > > support. Does the dwmac-intel version of the core set > > > priv->dma_cap.pcs? If it doesn't, then removing the test on this will > > > cause a regression, since in Cisco SGMII mode, we end up setting > > > priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As > > > priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will > > > enable all the "integrated PCS" code paths despite XPCS clearly > > > intending to be used for Cisco SGMII. > > > > > > I'm also wondering whether the same applies to the lynx PCS as well, > > > or in the general case if we have any kind of external PCS. > > > > > > Hence, I think this probably needs to be: > > > > > > if (phy_interface_mode_is_rgmii(interface)) > > > priv->hw->pcs = STMMAC_PCS_RGMII; > > > else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs) > > > priv->hw->pcs = STMMAC_PCS_SGMII; > > > > > > At least this is what unpicking the awful stmmac code suggests (and I > > > do feel that my point about the shocking state of this driver is proven > > > as details like this are extremely difficult to unpick, and not > > > unpicking them correctly will lead to regressions.) Therefore, I would > > > suggest that it would be wise if you also double-checked this. > > > > Double-checked that part. Indeed this is what I forgot to take into > > account. > > Thanks for double-checking it. > > > (Just realized I had a glimpse thought about checking the DW > > xGMAC/XPCS for supporting the SGMII interface, but the thought got > > away from my mind forgotten.) DW XPCS can be synthesized with having > > the GMII/MII interface connected to the MAC and SGMII downstream > > interface over a single 1000Base-X lane. > > > > In anyway AFAICS that case has nothing to do with the PCS embedded > > into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW > > XGMAC has no embedded PCS, but could be attached to the separate DW > > XPCS device. > > This is where my head starts spinning, because identifying what > "DW GMAC" and "DW QoS Eth" refer to is difficult unless one, I guess, > has the documentation. > > The only references to QoS that I can find in the driver refer to > per-DMA channel interrupts, dwmac5* and one mention for a platform > driver in the Kconfig. > > Grepping for "DW GMAC" doesn't give anything. > > Conversely, I know from the code that only dwmac4 and dwmac1000 > have support for the integrated PCS. So trying to put this together > doesn't make much sense to me. :/ > > Maybe "DW QoS Eth" refers to dwmac-dwc-qos-eth.c? DW QoS Eth is the new generation of the Synopsys Gigabit Ethernet IP-cores. Old ones are considered of version 3.74a and older: https://www.synopsys.com/dw/ipdir.php?ds=dwc_ether_mac10_100_1000_unive The new ones are of the version 4.0 and higher (the most modern DW QoS Eth IP-core is of v5.40a): https://www.synopsys.com/dw/ipdir.php?ds=dwc_ether_qos This is better summarised in the driver doc: Documentation/networking/device_drivers/ethernet/stmicro/stmmac.rst which has outdated a bit, but the summary table looks correct anyway: +-------------------------------+--------------+--------------+--------------+ | Controller Name | Min. Version | Max. Version | Abbrev. Name | +===============================+==============+==============+==============+ | Ethernet MAC Universal | N/A | 3.73a | GMAC | +-------------------------------+--------------+--------------+--------------+ | Ethernet Quality-of-Service | 4.00a | N/A | GMAC4+ | +-------------------------------+--------------+--------------+--------------+ | XGMAC - 10G Ethernet MAC | 2.10a | N/A | XGMAC2+ | +-------------------------------+--------------+--------------+--------------+ | XLGMAC - 100G Ethernet MAC | 2.00a | N/A | XLGMAC2+ | +-------------------------------+--------------+--------------+--------------+ See the abbreviation and controller names. When I say just DW GMAC then it means DW Ether MAC 10/100/1000 Universal, which driver is implemented in the dwmac1000* files. If you see DW GMAC4/GMAC5 or DW GAC4+ or DW QoE Eth, then it means DW Ethernet Quality-of-Service IP-core, which driver could be found in dwmac4*/dwmac5* files. As it inferable from the IP-core names the main difference between DW Ether MAC 10/100/1000 Universal and DW Ethernet Quality-of-Service is that the later one supports multiple queues and channels with a comprehensive list of the optional traffic scheduling features (FPE, TBS, DCB, AV-bridging, etc). DW GMAC doesn't have as many such features. The only way to have DW GMAC synthesized with the multiple DMA channels support is to enable a singly available traffic scheduling feature - AV-bridging. Note AV-bridging enabled on the DW GMAC v3.73a is the case of the Loongson GNET controller, which support is implemented in the Yanteng Si patchset recently submitted for v13 review: https://lore.kernel.org/netdev/cover.1716973237.git.siyanteng@loongson.cn/ In some extent the CSRs mapping is also different in DW GMAC v3.x and GMAC v4.x/v5.x, but the main part is in the QoS features. > > > About the correct implementation. Right, priv->dma_cap.pcs indicates > > that there is an embedded PCS and the flag can be set for DW GMAC or DW > > QoS Eth only. Although I would change the order: > > > > if (phy_interface_mode_is_rgmii(interface)) > > priv->hw->pcs = STMMAC_PCS_RGMII; > > else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII) > > priv->hw->pcs = STMMAC_PCS_SGMII; > > > > since priv->dma_cap.pcs is a primary flag. If it isn't set the > > interface will be irrelevant. > > As this is generic code, it probably makes sense to go with that, since > priv->dma_cap.pcs indicates whether the internal PCS for SGMII is > present or not rather than... Right. > > > Alternative solution could be to use the has_gmac/has_gmac4 flags > > instead. That will emphasize that the embedded PCS is expected to be > > specific for the DW GMAC and DW QoS Eth IP-cores: > > > > if (phy_interface_mode_is_rgmii(interface)) > > priv->hw->pcs = STMMAC_PCS_RGMII; > > else if ((priv->plat.has_gmac || priv->plat.has_gmac4) && > > interface == PHY_INTERFACE_MODE_SGMII) > > priv->hw->pcs = STMMAC_PCS_SGMII; > > which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c) > will always have its internal PCS if we're using SGMII mode. Right. If the DW GMAC/QoS Eth IP-core is synthesized with the SGMII/RTBI/RBI PHY interface then the internal PCS will always be available and the HWFEATURE.PCSSEL flag will be set. Here is the PCSSEL flag value definition: DW QoS Eth: DWC_EQOS_PCS_EN = DWC_EQOS_TBI_EN || DWC_EQOS_SGMII_EN || DWC_EQOS_RTBI_EN DW GMAC: if TBI, SGMII, or RTBI PHY interface is enabled. > Does this > mean it is true that these cores will never be used with an external > PCS? Sorry, I was wrong to suggest the (priv->plat.has_gmac || priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X downstream interface. Not sure why it was needed to implement that way seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of box, but AFAICS Intel mGBE is that case. Anyway the correct way to detect the internal PCS support is to check the PCSSEL flag set in the HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field). > > If there is a hardware flag that indicates the PCS is implemented, then > I think using that to gate whether SGMII uses the internal PCS is > better rather than using the core type. Right. > > Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS) > is being used, the internal PCS will not have been synthesized, and > thus priv->dma_cap.pcs will be false? Alas I can't confirm that. priv->dma_cap.pcs only indicates the internal PCS availability. External PCS is an independent entity from the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC controllers aren't aware of its existence. It's the low-level platform driver/code responsibility to somehow detect it being available ("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc). Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is synthesized with the SGMII/TBI/RTBI PHY interface support priv->dma_cap.pcs will get to be true. Note the device can be synthesized with several PHY interfaces supported. As long as SGMII/TBI/RTBI PHY interface is any of them, the flag will be set irrespective from the PHY interface activated at runtime. > The reason I'd like to know > this is because in the future, I'd like to eliminate priv->hw->pcs, > and just have dwmac1000/dwmac4's phylink_select_pcs() method make > the decisions. You can extend the priv->dma_cap.pcs flag semantics. So it could be indicating three types of the PCS'es: RGMII, SGMII, XPCS (or TBI/RTBI in future). > > If not, then we need to think about the behaviour that > stmmac_mac_select_pcs(0 should have. Should it give priority to the > internal PCS over external PCS, or external PCS first (in which case > what do we need to do with the internal PCS.) I guess the DW XPCS implementation might be more preferable. From one side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other hand the DW XPCS might be available over the MDIO-bus, which is slower to access than the internal PCS CSRs available in the DW GMAC/QoS Eth CSRs space. So the more performant link speed seems more useful feature over the faster device setup process. One thing I am not sure about is that there is a real case of having the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY interface support and being attached to the DW XPCS controller, which would have the SGMII downstream PHY interface. DW XPCS has only the XGMII or GMII/MII upstream interfaces over which the MAC can be attached. So DW GMAC/QoS Eth and DW XPCS can be connected via the GMII/MII interface only. Regarding Intel mGBE, it likely is having a setup like this: +------------+ +---------+ | | GMII/MII | | SGMII | DW QoS Eth +----------+ DW XPCS +------------ | | | | 1000Base-X +------------+ +---------+ -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, May 28, 2024 at 05:21:44PM +0100, Russell King (Oracle) wrote: > On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote: > > > Alternative solution could be to use the has_gmac/has_gmac4 flags > > > instead. That will emphasize that the embedded PCS is expected to be > > > specific for the DW GMAC and DW QoS Eth IP-cores: > > > > > > if (phy_interface_mode_is_rgmii(interface)) > > > priv->hw->pcs = STMMAC_PCS_RGMII; > > > else if ((priv->plat.has_gmac || priv->plat.has_gmac4) && > > > interface == PHY_INTERFACE_MODE_SGMII) > > > priv->hw->pcs = STMMAC_PCS_SGMII; > > > > which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c) > > will always have its internal PCS if we're using SGMII mode. Does this > > mean it is true that these cores will never be used with an external > > PCS? > > Sorry to go off on a related tangent, but I've just been looking at > hw->ps which is related to this. I was meditating around the hw->ps part for several days on the last week and just gave up in finding of how that semantics could be incorporated in the phylink pcs logic... > > As I understand, hw->ps comes from the "snps,ps-speed" property in DT, > which is used for SGMII and MAC2MAC connections. Presumably for the > SGMII case, this is used where the port is made to look like the PHY > end of the SGMII link. Right. The speed comes from the "snps,ps-speed" property and is utilized to set the particular port speed in the MAC2MAC case. But neither DW QoS Eth nor DW GMAC HW-manual explicitly describe that case. The only SGMII MAC2MAC mention there is GMAC_AN_CTRL_SGMRAL flag description: "SGMII RAL Control When set, this bit forces the SGMII RAL block to operate in the speed configured in the Speed and Port Select bits of the MAC Configuration register. This is useful when the SGMII interface is used in a direct MAC to MAC connection (without a PHY) and any MAC must reconfigure the speed. When reset, the SGMII RAL block operates according to the link speed status received on SGMII (from the PHY). This bit is reserved (and RO) if the SGMII PHY interface is not selected during core configuration." > > I'm guessing MAC2MAC refers to RGMII, or does that also refer to > SGMII-as-PHY? I guess that it can be utilized in both cases: RGMII-to-RGMII and SGMII-to-SGMII MAC2MAC setups. The only difference is that the GMAC_AN_CTRL_SGMRAL flag setting would be useless for RGMII. But originally the mac_device_info::ps field was introduced for the SGMII MAC2MAC config here: 02e57b9d7c8c ("drivers: net: stmmac: add port selection programming") and the "snps,ps-speed" property can be spotted alongside with phy-mode = "sgmii" only, here: arch/arm64/boot/dts/qcom/sa8775p-ride.dts Although AFAICS the dwmac1000_core_init()/dwmac4_core_init() methods lack of the GMAC_CONTROL_TC/GMAC_PHYIF_CTRLSTATUS_TC flags set in the (hw->ps)-related if-clause. Without that the specified speed setting won't be in-bend delivered to the other side of the MAC2MAC link and the internal PCS functionality won't work. Synopsys DW GMAC/Qos Eth databooks explicitly say that these flags need to be set for the MAC to be sending its Port speed, Duplex mode and Link Up/Down flag setting over the RGMII/SGMII in-band signal: SGMII: "The tx_config_reg[15:0] bits sent by the MAC during Auto-negotiation depend on whether the Transmit Configuration register bit is enabled for the SGMII interface." RGMII: "When the RGMII interface is configured to transmit the configuration during the IFG, then rgmii_txd[3:0] reflects the Duplex Mode, Port Select, Speed (encoded as 00 for 10 Mbps, 01 for 100 Mbps and 10 for 1000 Mbps), and Link Up/Down bits of the MAC Configuration Register," TC flag description: "Transmit Configuration in RGMII, SGMII, or SMII When set, this bit enables the transmission of duplex mode, link speed, and link up or down information to the PHY in the RGMII, SMII, or SGMII port. When this bit is reset, no such information is driven to the PHY. This bit is reserved (and RO) if the RGMII, SMII, or SGMII PHY port is not selected during core configuration." > > I think it would've been nice to have picked SGMII-as-PHY up in the > driver earlier - we don't tend to use the "normal" PHY interface > mode names, instead we have the REVxxx modes, so I think this > _should_ have introduced PHY_INTERFACE_MODE_REVSGMII. Not sure whether it would be a correct thing to do. RevMII is a real interface. DW GMAC/QoS Eth can be synthesized with RevMII PHY interface support. Mac2Mac SGMII/RGMII is a feature of the standard SGMII/RGMII interfaces. On the other hand we already have the set of the artificial modes like "rgmii-id/rgmii-txid/rgmii-rxid" indicating the MAC-side delays but describing the same interfaces. So I don't have a strong opinion against have the modes like "rev-rgmii"/"rev-sgmii". > > In any case, moving on... in stmmac_hw_setup(), we have: > > /* PS and related bits will be programmed according to the speed */ > if (priv->hw->pcs) { > int speed = priv->plat->mac_port_sel_speed; > > if ((speed == SPEED_10) || (speed == SPEED_100) || > (speed == SPEED_1000)) { > priv->hw->ps = speed; > } else { > dev_warn(priv->device, "invalid port speed\n"); > priv->hw->ps = 0; > } > } > > Which means that if we're using the integrated PCS, then we basically > require the "snps,ps-speed" property otherwise we'll issue a warning > at this point... this seems to imply that reverse mode is the only > mode supported, which I'm fairly sure is false. So, maybe this > shouldn't be issuing the warning if mac_port_sel_speed was zero? Seeing the link state could be delivered over the in-band path, I guess the "snps,ps-speed" property is supposed to be optional so the mac_port_sel_speed being zero is a possible case. Thus the warning is indeed misleading and it is totally ok to have mac_port_sel_speed being set to zero. If it is, then the link state shall be determined either over in-band or from the PHY. > > Moving on... hw->ps can only be 10M, 100M or 1G speeds and nothing else > - which is fine since RGMII and Cisco SGMII only support these speeds. > > dwmac1000 tests for this against these speeds, so it is also fine. > > dwmac4 is basically the same as dwmac1000, so is also fine. > > The core code as it stands today passes this into the pcs_ctrl_ane > method's rsgmi_ral argument, which sets GMAC_AN_CTRL_SGMRAL. Presumably > this selects "reverse" mode for both SGMII and RGMII? No, GMAC_AN_CTRL_SGMRAL flag works for SGMII only, which enables the fixed link speed (see my second comment in this email message) by forcing the SGMII RAL (Rate Adaptation Layer) working with the pre-defined speed. AFAIU RGMII interface doesn't need that flag since it always works with the pre-defined speed and has no Rate Adaptation engine. > > Persuing this a bit futher, qcom-ethqos always calls this with rsgmi_ral > clear. Presumably, qcom-ethqos never specifies "snps,ps-speed" in DT, > and thus always gets the warning above? Interesting situation. Actually no. The only DW QoS Eth device for which "snps,ps-speed = 1000" is specified is "qcom,sa8775p-ethqosi" (see arch/arm64/boot/dts/qcom/sa8775p-ride.dts), due to that no warning is printed. But on the other hand the low-level driver (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c) also sets the STMMAC_FLAG_HAS_INTEGRATED_PCS flag exactly for that device, which effectively disables the entire internal PCS functionality (except the speed setup performed in dwmac4_core_init()). Holy mother of ... > > Finally, we get to the core issue, which is dwxgmac2_core.c. > dwxgmac2 tests this member against 10G, 2.5G and "any other non-zero > value". Out of all of these, the only possible path through that code > would be the one which results in: > > tx |= hw->link.speed1000; > > Neither of the other two (2.5G and 10G) are possible because those > aren't legal values for hw->ps. Moreover, it doesn't appear to have > any kind of PCS, so I'm wondering whether any of this code gets used. I guess the (hw->ps)-related code snippet has been just dummy-copied from another dwmac*_core.c file to DW XGMAC. So IMO it can be freely dropped. After all the bindings define the snps,ps-speed as: "Port selection speed that can be passed to the core when PCS is supported. For example, this is used in case of SGMII and MAC2MAC connection." I doubt DW XGMAC could be used in the MAC2MAC setup, and it doesn't have any internal PCS (may have externally connected DW XPCS though). > > > So, I suspect some of this is "not quite right" either, and I wonder > about the implications of changing how hw->pcs is set - whether we > first need to fix the code above dealing with priv->hw->ps ? > > I'm also wondering what impact this has on my PCS conversion. My brain got blown up thinking about this one week ago. So I gave up in looking for a portable way of fixing the MAC2MAC part and sent my three patches as is to you. I thought after some time I could come up with some ideas about that. Alas the time-break didn't help.) I can't say for sure what could be a better way to align the things around the internal PCS and MAC2MAC case. But IMO seeing the code is vastly messy and unlikely has been widely used I'd suggest to preserve the semantics as required by the Qualcomm QoS Eth (dwmac-qcom-ethqos.c), and free redefining the rest of the things as you wish. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Serge, Thanks for the reply. I've attempted to deal with most of these in my v2 posting, but maybe not in the best way yet. On Fri, May 31, 2024 at 08:13:49PM +0300, Serge Semin wrote: > > Does this > > mean it is true that these cores will never be used with an external > > PCS? > > Sorry, I was wrong to suggest the (priv->plat.has_gmac || > priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW > QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X > downstream interface. Not sure why it was needed to implement that way > seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of > box, but AFAICS Intel mGBE is that case. Anyway the correct way to > detect the internal PCS support is to check the PCSSEL flag set in the > HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field). We can only wonder why! > > Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS) > > is being used, the internal PCS will not have been synthesized, and > > thus priv->dma_cap.pcs will be false? > > Alas I can't confirm that. priv->dma_cap.pcs only indicates the > internal PCS availability. External PCS is an independent entity from > the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC > controllers aren't aware of its existence. It's the low-level platform > driver/code responsibility to somehow detect it being available > ("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc). > > Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is > synthesized with the SGMII/TBI/RTBI PHY interface support > priv->dma_cap.pcs will get to be true. Note the device can be > synthesized with several PHY interfaces supported. As long as > SGMII/TBI/RTBI PHY interface is any of them, the flag will be set > irrespective from the PHY interface activated at runtime. I've been debating about this, and given your response, I'm wondering whether we should change stmmac_mac_select_pcs() to instead do: static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config, phy_interface_t interface) { struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); struct phylink_pcs *pcs; if (priv->plat->select_pcs) { pcs = priv->plat->select_pcs(priv, interface); if (!IS_ERR(pcs)) return pcs; } return stmmac_mac_phylink_select_pcs(priv, interface); } and push the problem of whether to provide a PCS that overrides the MAC internal PCS into platform code. That would mean Intel mGBE would be able to override with XPCS. rzn1 and socfpga can then do their own thing as well. I'm trying hard not to go down another rabbit hole... I've just spotted that socfpga sets mac_interface to PHY_INTERFACE_MODE_SGMII. That's another reason for pushing this down into platform drivers - if platform drivers are doing weird stuff, then we can contain their weirdness in the platform drivers moving it out of the core code. > You can extend the priv->dma_cap.pcs flag semantics. So it could > be indicating three types of the PCS'es: > RGMII, SGMII, XPCS (or TBI/RTBI in future). If TBI/RTBI gets supported, then this would have to be extended, but I get the impression that this isn't popular. > I guess the DW XPCS implementation might be more preferable. From one > side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW > GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other > hand the DW XPCS might be available over the MDIO-bus, which is slower > to access than the internal PCS CSRs available in the DW GMAC/QoS Eth > CSRs space. So the more performant link speed seems more useful > feature over the faster device setup process. I think which should be used would depend on how the hardware is wired up. This brings us back to platform specifics again, which points towards moving the decision making into platform code as per the above. > One thing I am not sure about is that there is a real case of having > the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY > interface support and being attached to the DW XPCS controller, which > would have the SGMII downstream PHY interface. DW XPCS has only the > XGMII or GMII/MII upstream interfaces over which the MAC can be > attached. That gives us another possibility, but needs platforms to be doing the right thing. If mac_interface were set to XGMII or GMII/MII, then that would exclude the internal MAC PCS. > So DW GMAC/QoS Eth and DW XPCS can be connected via the > GMII/MII interface only. Regarding Intel mGBE, it likely is having a > setup like this: > > +------------+ +---------+ > | | GMII/MII | | SGMII > | DW QoS Eth +----------+ DW XPCS +------------ > | | | | 1000Base-X > +------------+ +---------+ So as an alternative, mac_interface phy_interface XGMII/GMII/MII SGMII/1000Base-X MAC ---------------- DW XPCS ------------------ INTERNAL SGMII/TBI/RTBI MAC ---------- Internal PCS ---------------- INTERNAL RGMII MAC ---------- Internal "PCS" -------------- One of the problems here, though, is socfpga. It uses mac_interface with RGMII*, MII, GMII, SGMII and RMII. I think it's confusing mac_interface for phy_interface, but I haven't read through enough of it to be certain. So that again leads me back to my proposal above for stmmac_mac_select_pcs() as the least likely to break proposition - at least given how things are at the moment.
Hi Russel On Fri, May 31, 2024 at 08:30:27PM +0100, Russell King (Oracle) wrote: > Hi Serge, > > Thanks for the reply. I've attempted to deal with most of these in my > v2 posting, but maybe not in the best way yet. I've got your v2 series. I'll have a look at it and test it out later on the next week, sometime around Wednesday. > > On Fri, May 31, 2024 at 08:13:49PM +0300, Serge Semin wrote: > > > Does this > > > mean it is true that these cores will never be used with an external > > > PCS? > > > > Sorry, I was wrong to suggest the (priv->plat.has_gmac || > > priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW > > QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X > > downstream interface. Not sure why it was needed to implement that way > > seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of > > box, but AFAICS Intel mGBE is that case. Anyway the correct way to > > detect the internal PCS support is to check the PCSSEL flag set in the > > HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field). > > We can only wonder why! > > > > Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS) > > > is being used, the internal PCS will not have been synthesized, and > > > thus priv->dma_cap.pcs will be false? > > > > Alas I can't confirm that. priv->dma_cap.pcs only indicates the > > internal PCS availability. External PCS is an independent entity from > > the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC > > controllers aren't aware of its existence. It's the low-level platform > > driver/code responsibility to somehow detect it being available > > ("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc). > > > > Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is > > synthesized with the SGMII/TBI/RTBI PHY interface support > > priv->dma_cap.pcs will get to be true. Note the device can be > > synthesized with several PHY interfaces supported. As long as > > SGMII/TBI/RTBI PHY interface is any of them, the flag will be set > > irrespective from the PHY interface activated at runtime. > > I've been debating about this, and given your response, I'm wondering > whether we should change stmmac_mac_select_pcs() to instead do: > > static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config, > phy_interface_t interface) > { > struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > struct phylink_pcs *pcs; > > if (priv->plat->select_pcs) { > pcs = priv->plat->select_pcs(priv, interface); > if (!IS_ERR(pcs)) > return pcs; > } > > return stmmac_mac_phylink_select_pcs(priv, interface); > } > > and push the problem of whether to provide a PCS that overrides > the MAC internal PCS into platform code. That would mean Intel mGBE > would be able to override with XPCS. rzn1 and socfpga can then do > their own thing as well. Well, AFAICS the only device that currently have the DW XPCS connected to a non DW XGMAC controller is indeed the Intel mGBE with its DW QoS Eth+DW XPCS weird setup. At the same time the Intel mGBE controller can also support RGMII interface. Thus there is no internal SGMII/TBI/RTBI PCS in there. Qualcomm QoS Eth uses the internal SGMII PCS and by setting up the STMMAC_FLAG_HAS_INTEGRATED_PCS flag its driver almost completely disables the STMMAC PCS functionality (except the stmmac_pcs_ctrl_ane() being called in stmmac_hw_setup()). So from the perspective of these two devices the PCS selection looks quiet certain. It's either internal or external one. There is no device with both of them available. SoCFPGA... Well, it's another and more complicated story. Based on what said in a comment in socfpga_gen5_set_phy_mode()/socfpga_gen10_set_phy_mode() the only possibility to have some internal interface converted to the external one is when a "splitter" is available. But IMO the comment is misleading because the only thing that is then done with the "splitter" CSR is just the clock divider selection. What is actually done, if the "splitter" is available or if the SGMII/GMII/MII _MAC-interface_ is requested, then the internal interface is fixed to "GMII/MII". It looks weird because based on the "mac-mode" DT-property semantics it was supposed to indicate the internal interface only. But EMAC is never tuned to have the SGMII interface (see the values saved in the "*val" argument of socfpga_set_phy_mode_common()). So all of that makes me to conclude the next points: 1. "mac-mode" property has never been utilized for the SoG-FPGA GMAC platform. The plat_stmmacenet_data::mac_interface field has always defaulted to plat_stmmacenet_data::phy_interface. 2. SoG-FPGA GMAC IP-core itself doesn't support the native/internal SGMII interface. It's implemented by means of the so called "gmii-to-sgmii"-converter, which is the Lynx PCS. Thus unless I've missed something the SoC-FPGA network controller structure can be depicted as follows: +---- SYSMGR:PHYSEL phy_intf_sel | +------------------+ +--------------+ | RMII | | | Internal Interface | +----------+ | off +--------------------------+ | RGMII | Internal Interface | SGMII | | External Interface* | EMAC +----------+---------------------+ | +-------+ +--------+----------- | GMII/MII | | adapter | GMII/MII | Lynx | SGMII | | | +----------+ | on +----------+ +-------+ | | +--+ | | | PCS | | +------------------+ | +--------------+ +---+---+ | | | | | +------------+ | | +--------------+ Splitter** +--------------------+--------------------+ +-----+------+ | +-----+------+ | Oscillator | +------------+ * No idea whether the external interface is represented as a single IO port or as multiple interface ports handled by the same MAC. ** As I explained above, judging by the SoC-FPGA driver code the "splitter" is just the reference clock divider responsible for the clock rate adjustment based on the requested link speed. Based on the logic depicted on the sketch above, I guess that there is no internal SGMII/TBI/RTBI PCS in SoC-FPGA GMAC either. The SGMII interface is implemented by means of the Lynx PCS. > > I'm trying hard not to go down another rabbit hole... I've just > spotted that socfpga sets mac_interface to PHY_INTERFACE_MODE_SGMII. > That's another reason for pushing this down into platform drivers - > if platform drivers are doing weird stuff, then we can contain their > weirdness in the platform drivers moving it out of the core code. Oh, that damn "mac-mode" property... First of all as I already mentioned once AFAICS originally it was introduced for the SoC-FPGA GMAC, but the property has never been defined in any DT-node so far, neither in SoC-FPGA nodes nor in the rest of the DW *MAC-based nodes. Moreover based on my consideration above the SoC-FPGA internal interface is always determined based on the external one seeing plat_stmmacenet_data::mac_interface defaults to plat_stmmacenet_data::phy_interface. Secondly I also have much certainty that the rest of the glue drivers utilizing plat_stmmacenet_data::mac_interface field should in fact be using plat_stmmacenet_data::phy_interface instead. Based on the history of the mac_interface-related changes it's likely that all of them have just either been missed during the conversion to utilizing the phy_interface-field or incorrectly utilized the mac_interface field instead of phy_interface in the first place. So to speak before going further it might be worth re-checking once again the entire history of the "mac-mode" property-related change, but as an experimental A/B-test patch for net-next it may be a good idea to either drop the mac_interface field completely, or convert the driver to forgetting about the internal PCS if the external one is enabled, or, as a less invasive option, make SoC-FPGA explicitly setting up the mac_interface field to GMII/MII if it configures the internal interface to that value. Then, if these changes don't break any platform (most importantly the SoF-FPGA GMAC case), then we can go further and carefully convert the rest of the glue-drivers not using the mac_interface field. > > > You can extend the priv->dma_cap.pcs flag semantics. So it could > > be indicating three types of the PCS'es: > > RGMII, SGMII, XPCS (or TBI/RTBI in future). > > If TBI/RTBI gets supported, then this would have to be extended, but I > get the impression that this isn't popular. Irrespective from the TBI/RTBI interface support, using the priv->dma_cap.pcs field for all possible PCS'es shall also improve the code readability. Currently we have four versions of the PCS fields: dma_features::pcs mac_device_info::pcs mac_device_info::xpcs mac_device_info::lynx_pcs which are being checked here and there in the driver... > > > I guess the DW XPCS implementation might be more preferable. From one > > side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW > > GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other > > hand the DW XPCS might be available over the MDIO-bus, which is slower > > to access than the internal PCS CSRs available in the DW GMAC/QoS Eth > > CSRs space. So the more performant link speed seems more useful > > feature over the faster device setup process. > > I think which should be used would depend on how the hardware is wired > up. This brings us back to platform specifics again, which points > towards moving the decision making into platform code as per the above. > > > One thing I am not sure about is that there is a real case of having > > the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY > > interface support and being attached to the DW XPCS controller, which > > would have the SGMII downstream PHY interface. DW XPCS has only the > > XGMII or GMII/MII upstream interfaces over which the MAC can be > > attached. > > That gives us another possibility, but needs platforms to be doing > the right thing. If mac_interface were set to XGMII or GMII/MII, then > that would exclude the internal MAC PCS. > > > So DW GMAC/QoS Eth and DW XPCS can be connected via the > > GMII/MII interface only. Regarding Intel mGBE, it likely is having a > > setup like this: > > > > +------------+ +---------+ > > | | GMII/MII | | SGMII > > | DW QoS Eth +----------+ DW XPCS +------------ > > | | | | 1000Base-X > > +------------+ +---------+ > > > So as an alternative, > > mac_interface phy_interface > > XGMII/GMII/MII SGMII/1000Base-X > MAC ---------------- DW XPCS ------------------ > > INTERNAL SGMII/TBI/RTBI > MAC ---------- Internal PCS ---------------- > > INTERNAL RGMII > MAC ---------- Internal "PCS" -------------- + SoC-FPGA (presumably) GMII/MII SGMII MAC ---------------- Lynx PCS -------------- Please also note, based on the DW GMAC/QoS Eth hardware manual each internal interface block is connected to MAC by the GMII/MII interface. So the internal PCS cases more precisely could be represented as follows: GMII SGMII (AN) MAC ---------- Internal PCS ------------------ GMII TBI/RTBI (AN) MAC ---------- Internal PCS ------------------ GMII RGMII (In-band) MAC ---------- Internal "PCS" ---------------- GMII RevMII MAC ---------- RevMII block ---------------- GMII GMII MAC ------------------------------------------ MII SMII (In-band) MAC ---------- Internal "PCS" ---------------- MII RMII MAC ---------- RMII block ---------------- MII MII MAC ------------------------------------------ There is a special input signal phy_intf_sel[2:0], which tells to MAC what interface to activate (grep -i the glue drivers for "intf", "physel", etc). > > One of the problems here, though, is socfpga. It uses mac_interface > with RGMII*, MII, GMII, SGMII and RMII. I think it's confusing > mac_interface for phy_interface, but I haven't read through enough > of it to be certain. > > So that again leads me back to my proposal above for > stmmac_mac_select_pcs() as the least likely to break proposition - > at least given how things are at the moment. Please see my notes above regarding the internal interface initialization in the SoC-FPGA glue driver. I guess we could at least try to A/B-test the SoC-FPGA code in the next net-next by setting mac_interface to GMII/MII when the internal interface is enabled as GMII/MII in the glue-driver, and converting the rest of the glue driver to using phy_interface. If nothing breaks, then SoC-FPGA has never used the "mac-mode" property and we could mark the property as deprecated and could carefully covert the rest of the STMMAC platform driver to using the phy_interface field. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 90c065920af2..6c4e90b1fea3 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1146,18 +1146,10 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv) { int interface = priv->plat->mac_interface; - if (priv->dma_cap.pcs) { - if ((interface == PHY_INTERFACE_MODE_RGMII) || - (interface == PHY_INTERFACE_MODE_RGMII_ID) || - (interface == PHY_INTERFACE_MODE_RGMII_RXID) || - (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { - netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); - priv->hw->pcs = STMMAC_PCS_RGMII; - } else if (interface == PHY_INTERFACE_MODE_SGMII) { - netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); - priv->hw->pcs = STMMAC_PCS_SGMII; - } - } + if (phy_interface_mode_is_rgmii(interface)) + priv->hw.pcs = STMMAC_PCS_RGMII; + else if (interface == PHY_INTERFACE_MODE_SGMII) + priv->hw.pcs = STMMAC_PCS_SGMII; } /**
The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized into the DW GMAC controller. It's always done if the controller supports at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these interfaces support was activated during the IP-core synthesize the PCS block won't be activated either and the HWFEATURE.PCSSEL flag won't be set. Based on that the RGMII in-band status detection procedure implemented in the driver hasn't been working for the devices with the RGMII interface support and with none of the SGMII, TBI, RTBI PHY interfaces available in the device. Fix that just by dropping the dma_cap.pcs flag check from the conditional statement responsible for the In-band/PCS functionality activation. If the RGMII interface is supported by the device then the in-band link status detection will be also supported automatically (it's always embedded into the RGMII RTL code). If the SGMII interface is supported by the device then the PCS block will be supported too (it's unconditionally synthesized into the controller). The later is also correct for the TBI/RTBI PHY interfaces. Note while at it drop the netdev_dbg() calls since at the moment of the stmmac_check_pcs_mode() invocation the network device isn't registered. So the debug prints will be for the unknown/NULL device. Fixes: e58bb43f5e43 ("stmmac: initial support to manage pcs modes") Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)