| Message ID | d0ca47778a424a142abbfa7947d8413dfbffc104.1714046812.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
On Thu, Apr 25, 2024 at 09:06:12PM +0800, Yanteng Si wrote: > The mac_interface of gmac is PHY_INTERFACE_MODE_RGMII_ID. No it isn't! > + plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID; You don't touch mac_interface here. Please read the big comment I put in include/linux/stmmac.h above these fields in struct plat_stmmacenet_data to indicate what the difference between mac_interface and phy_interface are, and then correct which-ever of the above needs to be corrected. Thanks.
Hi Russell, 在 2024/4/25 22:36, Russell King (Oracle) 写道: >> The mac_interface of gmac is PHY_INTERFACE_MODE_RGMII_ID. > No it isn't! Ok, that's a typo. > >> + plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID; > You don't touch mac_interface here. Please read the big comment I put > in include/linux/stmmac.h above these fields in struct > plat_stmmacenet_data to indicate what the difference between > mac_interface and phy_interface are, and then correct which-ever > of the above needs to be corrected. Copy your big comment here: int phy_addr; /* MAC ----- optional PCS ----- SerDes ----- optional PHY ----- Media * ^ ^ * mac_interface phy_interface * * mac_interface is the MAC-side interface, which may be the same * as phy_interface if there is no intervening PCS. If there is a * PCS, then mac_interface describes the interface mode between the * MAC and PCS, and phy_interface describes the interface mode * between the PCS and PHY. */ phy_interface_t mac_interface; /* phy_interface is the PHY-side interface - the interface used by * an attached PHY. */ Our hardware engineer said we don't support pcs, and if I understand your comment correctly, our mac_interface and phy_interface should be the same, right? Thanks, Yanteng
On Fri, Apr 26, 2024 at 06:16:42PM +0800, Yanteng Si wrote: > Hi Russell, > > 在 2024/4/25 22:36, Russell King (Oracle) 写道: > > > The mac_interface of gmac is PHY_INTERFACE_MODE_RGMII_ID. > > No it isn't! > Ok, that's a typo. > > > > > + plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID; > > You don't touch mac_interface here. Please read the big comment I put > > in include/linux/stmmac.h above these fields in struct > > plat_stmmacenet_data to indicate what the difference between > > mac_interface and phy_interface are, and then correct which-ever > > of the above needs to be corrected. > > Copy your big comment here: > > int phy_addr; > /* MAC ----- optional PCS ----- SerDes ----- optional PHY ----- Media > * ^ ^ > * mac_interface phy_interface > * > * mac_interface is the MAC-side interface, which may be the same > * as phy_interface if there is no intervening PCS. If there is a > * PCS, then mac_interface describes the interface mode between the > * MAC and PCS, and phy_interface describes the interface mode > * between the PCS and PHY. > */ > phy_interface_t mac_interface; > /* phy_interface is the PHY-side interface - the interface used by > * an attached PHY. > */ > > Our hardware engineer said we don't support pcs, and if I understand > > your comment correctly, our mac_interface and phy_interface should > > be the same, right? It only matters to the core code if priv->dma_cap.pcs is set true. If it isn't, then mac_interface doesn't seem to be relevent (it does get used by a truck load of platform specific code though which I haven't looked at to answer this.) I would suggest that if priv->dma_cap.pcs is false, then setting mac_interface to PHY_INTERFACE_MODE_NA to make it explicit that it's not used would be a good idea. While looking at this, however, I've come across the fact that stmmac manipulates the netif carrier via netif_carrier_off() and netif_carrier_on(), which is a _big_ no no when using phylink. Phylink manages the carrier for the driver, and its part of phylink's state. Fiddling with the carrier totally breaks the guarantee that phylink will make calls to mac_link_down() and mac_link_up(). If a driver wants to fiddle with the netif carrier, it must NOT use phylink. If it wants to use phylink, it must NOT fiddle with the netif carrier state. The two are mutually exclusive. stmmac is quickly becoming a driver I don't care about whether my changes to phylink end up breaking it or not because of abuses like this.
On Fri, Apr 26, 2024 at 12:00:25PM +0100, Russell King (Oracle) wrote: > On Fri, Apr 26, 2024 at 06:16:42PM +0800, Yanteng Si wrote: > > Hi Russell, > > > > 在 2024/4/25 22:36, Russell King (Oracle) 写道: > > > > The mac_interface of gmac is PHY_INTERFACE_MODE_RGMII_ID. > > > No it isn't! > > Ok, that's a typo. > > > > > > > + plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID; > > > You don't touch mac_interface here. Please read the big comment I put > > > in include/linux/stmmac.h above these fields in struct > > > plat_stmmacenet_data to indicate what the difference between > > > mac_interface and phy_interface are, and then correct which-ever > > > of the above needs to be corrected. > > > > Copy your big comment here: > > > > int phy_addr; > > /* MAC ----- optional PCS ----- SerDes ----- optional PHY ----- Media > > * ^ ^ > > * mac_interface phy_interface > > * > > * mac_interface is the MAC-side interface, which may be the same > > * as phy_interface if there is no intervening PCS. If there is a > > * PCS, then mac_interface describes the interface mode between the > > * MAC and PCS, and phy_interface describes the interface mode > > * between the PCS and PHY. > > */ > > phy_interface_t mac_interface; > > /* phy_interface is the PHY-side interface - the interface used by > > * an attached PHY. > > */ > > > > Our hardware engineer said we don't support pcs, and if I understand > > > > your comment correctly, our mac_interface and phy_interface should > > > > be the same, right? > > It only matters to the core code if priv->dma_cap.pcs is set true. > If it isn't, then mac_interface doesn't seem to be relevent Absolutely correct. Moreover AFAICS from the databooks the PCS module is only available for the DW GMACs with the TBI, RTBI and SGMII interfaces. So the STMMAC_PCS_RGMII PCS flag seems misleading. The only info about the link state that could be runtime detected on the MAC side is: link speed, duplex mode and link status. It's done by means of the RGMII in-band status signals. That's what is implemented in the dwmac1000_rgsmii() method. I've checked it works as soon as the GMAC_INT_DISABLE_RGMII IRQ is unmask. But the RGMII-capable DW GMACs don't support the Auto-negotiation feature, because it has no PCS inside. Thus what has been implemented for the RGMII case in stmmac_ethtool_get_link_ksettings() and stmmac_ethtool_set_link_ksettings() seems incorrect to me. > (it > does get used by a truck load of platform specific code though > which I haven't looked at to answer this.) IMO the most of them are using the plat_stmmacenet_data::mac_interface field as just an additional storage of the PHY-interface type. The only cases which I would consider as validly using the field are the ones with the SGMII interface support (and TBI/RTBI). BTW I see the "mac-mode" DT-property support was introduced in 2019 by the commit 0060c8783330 ("net: stmmac: implement support for passive mode converters via dt"). The commit didn't touch any of the platform drivers, but merely changed the plat_stmmacenet_data::mac_interface semantics to containing the intermediate interface type if the property was specified. So IMO all the platform drivers which had been available by the time the change was introduced can be converted to using the plat_stmmacenet_data::phy_interface field with a small probability to break things. I can't help but notice that since that commit there have been no any DW MAC DT-node introduced with the "mac-mode" property specified. So all of that makes me thinking that the code implemented around the mac_interface data has been mainly unused and most likely was needless overcomplication. Sigh... > > I would suggest that if priv->dma_cap.pcs is false, then setting > mac_interface to PHY_INTERFACE_MODE_NA to make it explicit that > it's not used would be a good idea. I bet it will be false. But Yanteng, could you please double check that? If it is could you convert this patch to setting plat_stmmacenet_data::mac_interface with PHY_INTERFACE_MODE_NA by default for all your device and setting the plat_stmmacenet_data::phy_interface with PHY_INTERFACE_MODE_RGMII_ID for the Loongson GMAC devices? BTW Yanteng, are you sure it's supposed to PHY_INTERFACE_MODE_RGMII_ID? AFAICS from the Loongson DTS files (loongson64-2k1000.dtsi, ls7a-pch.dtsi) the phy-mode is just PHY_INTERFACE_MODE_RGMII with no internal delays. -Serge(y) > > While looking at this, however, I've come across the fact that > stmmac manipulates the netif carrier via netif_carrier_off() and > netif_carrier_on(), which is a _big_ no no when using phylink. > Phylink manages the carrier for the driver, and its part of > phylink's state. Fiddling with the carrier totally breaks the > guarantee that phylink will make calls to mac_link_down() and > mac_link_up(). > > If a driver wants to fiddle with the netif carrier, it must NOT > use phylink. If it wants to use phylink, it must NOT fiddle with > the netif carrier state. The two are mutually exclusive. > > stmmac is quickly becoming a driver I don't care about whether my > changes to phylink end up breaking it or not because of abuses > like this. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sat, May 04, 2024 at 12:01:42AM +0300, Serge Semin wrote: > Absolutely correct. Moreover AFAICS from the databooks the PCS module > is only available for the DW GMACs with the TBI, RTBI and SGMII > interfaces. So the STMMAC_PCS_RGMII PCS flag seems misleading. The only > info about the link state that could be runtime detected on the MAC > side is: link speed, duplex mode and link status. It's done by means > of the RGMII in-band status signals. That's what is implemented in the > dwmac1000_rgsmii() method. ... which you've now made me look at, look at the whole pcs_link/pcs_speed/pcs_duplex and made me wonder why the hell stmmac is making this so figgin difficult, messing with the carrier behind phylink's back (which is a no-no), and why it needs to have separate paths for this. It doesn't. Just because it doesn't have something called an official "PCS" does not mean you _can't_ use a phylink_pcs to represent something that has PCS-like properties - such as the RGMII in-band status which is no different to what Cisco SGMII does. This isn't something new... this is something that mvneta has supported since before phylink was a thing, and so phylink had to allow it as well.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index f7618edf4a3a..e989cb835340 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -49,6 +49,7 @@ static int loongson_gmac_data(struct plat_stmmacenet_data *plat) loongson_default_data(plat); plat->mdio_bus_data->phy_mask = 0; + plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID; return 0; }