On Wed, Jul 03, 2024 at 10:08:16PM +0300, Serge Semin wrote: > On Fri, Jun 28, 2024 at 04:07:46PM +0100, Russell King (Oracle) wrote: > > On Mon, Jun 24, 2024 at 04:26:31PM +0300, Serge Semin wrote: > > > @@ -621,7 +548,6 @@ int dwmac1000_setup(struct stmmac_priv *priv) > > > mac->mii.clk_csr_shift = 2; > > > mac->mii.clk_csr_mask = GENMASK(5, 2); > > > > > > - mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops; > > > mac->mac_pcs.neg_mode = true; > > > > "mac->mac_pcs.neg_mode = true;" is a property of the "ops" so should > > move with it. > > > > > @@ -1475,7 +1396,6 @@ int dwmac4_setup(struct stmmac_priv *priv) > > > mac->mii.clk_csr_mask = GENMASK(11, 8); > > > mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr); > > > > > > - mac->mac_pcs.ops = &dwmac4_mii_pcs_ops; > > > mac->mac_pcs.neg_mode = true; > > > > Also applies here. > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c > > > index 3666893acb69..c42fb2437948 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c > > > @@ -363,6 +363,7 @@ int stmmac_hwif_init(struct stmmac_priv *priv) > > > mac->tc = mac->tc ? : entry->tc; > > > mac->mmc = mac->mmc ? : entry->mmc; > > > mac->est = mac->est ? : entry->est; > > > + mac->mac_pcs.ops = mac->mac_pcs.ops ?: entry->pcs; > > > > > Removing both of the above means that mac->mac_pcs.ops won't ever be set > > prior to this, so this whole thing should just be: > > > > mac->mac_pcs.ops = entry->pcs; > > mac->mac_pcs.neg_mode = true; > > Actually, no. mac->mac_pcs.ops can be set by the platform-specific > plat_stmmacenet_data::setup() method. mac->mac_pcs is there for the _internal_ MAC only, not for platforms to fiddle around with (remember, my patch set adds this!) I think you're thinking of mac->phylink_pcs which platforms can and do fiddle with. > > > + /* TODO Check the PCS_AN_STATUS.Link status here?.. Note the flag is latched-low */ > > > + > > > + /* TODO The next is the TBI/RTBI-specific and seems to be valid if PCS_AN_STATUS.ANC */ > > > val = readl(priv->pcsaddr + PCS_ANE_LPA); > > > > > I thought these registers only existed of dma_cap.pcs is true ? > > Right. The AN-registers are SGMII/TBI/RTBI-specific. Therefore, I suggest that if state->interface is RGMII, then these registers should not be accessed. My idea is to provide two PCS per MAC: One simple one which covers RGMII which only reads the PHYIF/RGSMIIIS register, does no configuration, but does implement the .pcs_enable/ .pcs_disable etc. The .pcs_validate method should also be empty for this because the AutoNeg ethtool capability does not refer to the inband signalling, but to the media PHY. Then a more complex PCS implementation that does everything the RGMII one does, but also the bits for SGMII (and TBI/RTBI). > > If we > > start checking PCS_AN_STATUS.Link here, and this register reads as > > zeros, doesn't it mean that RMGII inband mode won't ever signal link > > up? > > Right. The PCS_AN_STATUS.Link should be checked for the SGMII (and > TBI/RTBI) only. The databooks defines the flag as follows: > > DW GMAC v3.73a: > Link Status This bit indicates whether the data channel (link) is up or > R_SS_SC_LLO down. For the TBI, RTBI or SGMII interfaces, if ANEG is going > on, data cannot be transferred across the link and hence the > link is given as down. > > DW QoS Eth: > Link Status When this bit is set, it indicates that the link is up between > Read-only the MAC and the TBI, RTBI, or SGMII interface. When this bit is > reset, it indicates that the link is down between the MAC and > the TBI, RTBI, or SGMII interface. > > I guess that in fact the flag semantics is the same on both devices. > But the Access-status for some reason different. Although DW QoS Eth > databook doesn't define any latched-low CSR. So there is a chance that > some of the databooks might be wrong in the flag access status. Yes, it sounds like it. > > > - /* TODO Make sure that STMMAC_PCS_PAUSE STMMAC_PCS_ASYM_PAUSE usage is legitimate */ > > > + /* TODO The databook says the encoding is defined in IEEE 802.3z, > > > + * Section 37.2.1.4. Do we need the STMMAC_PCS_PAUSE and > > > + * STMMAC_PCS_ASYM_PAUSE mask here? > > > + */ > > > linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, > > > state->lp_advertising, > > > FIELD_GET(PCS_ANE_PSE, val) & STMMAC_PCS_PAUSE); > > > > > If it's 802.3z aka 1000base-X format, then yes, we should be using > > these bits if we are getting state from this register. > > I meant that should we be using the driver-specific macro in here > seeing the field encoding is defined by the IEEE 802.3z? Is there any > ready-to-use macros/constants defined in the network subsystem core > for the standard Pause encoding (IEEE 802.3z Section 37.2.1.4)? include/uapi/linux/mii.h: #define ADVERTISE_1000XFULL 0x0020 /* Try for 1000BASE-X full-duplex */ /* GMAC_ANE_FD */ #define ADVERTISE_1000XHALF 0x0040 /* Try for 1000BASE-X half-duplex */ /* GMAC_ANE_HD */ #define ADVERTISE_1000XPAUSE 0x0080 /* Try for 1000BASE-X pause */ /* GMAC_ANE_PSE bit 0 */ #define ADVERTISE_1000XPSE_ASYM 0x0100 /* Try for 1000BASE-X asym pause */ /* GMAC_ANE_PSE bit 1 */ #define ADVERTISE_LPACK 0x4000 /* Ack link partners response */ /* GMAC_ANE_ACK */ #define LPA_1000XFULL 0x0020 /* Can do 1000BASE-X full-duplex */ /* GMAC_ANE_FD */ #define LPA_1000XHALF 0x0040 /* Can do 1000BASE-X half-duplex */ /* GMAC_ANE_HD */ #define LPA_1000XPAUSE 0x0080 /* Can do 1000BASE-X pause */ /* GMAC_ANE_PSE bit 0 */ #define LPA_1000XPAUSE_ASYM 0x0100 /* Can do 1000BASE-X pause asym*/ /* GMAC_ANE_PSE bit 1 */ #define LPA_RESV 0x1000 /* Unused... */ /* GMAC_ANE_RFE bit 0 */ #define LPA_RFAULT 0x2000 /* Link partner faulted */ /* GMAC_ANE_RFE bit 1 */ #define LPA_LPACK 0x4000 /* Link partner acked us */ /* GMAC_ANE_ACK */ > > If TBI/RTBI is ever used, rather than trying to shoe-horn it all into > > these functions, please consider splitting them into separate PCSes, > > and sharing code between them e.g. using common functions called from > > the method functions or shared method functions where appropriate. > > Ok. Sounds reasonable. > > I guess your message also means that the patchset re-spinning will be > on me from now, right?) If so, please note, I can't promise I'll be > able to do that soonish. I am quite busy at the moment. I'll be > more-or-less free for that in a month or so. Not necessarily - some good news today, the high priority issue I was working on is lower priority at last, which means I've more time to look at mainline again. Bad news... I need a break after about 2.5 months of frustrations, which could be from this weekend! Given the fix for the LNKMOD issue, I suspect that won't be merged into net-next until after the weekend, but I'll see whether I can sneak a respin of the patch set once that's happened. That said, given that we'll be at -rc7, it's likely too late to be thinking about getting the PCS changes queued up for this coming merge window. In any case, I don't think even if I did post a series, we're at the point where we have something that would be ready. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
