| Message ID | 20240624132802.14238-6-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: convert stmmac "pcs" to phylink | expand |
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; > +static void dwmac_pcs_get_state(struct phylink_pcs *pcs, > + struct phylink_link_state *state) > { > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); > struct stmmac_priv *priv = hw->priv; > u32 val; > > + val = stmmac_pcs_get_config_reg(priv, hw); > + > + /* TODO The next is SGMII/RGMII/SMII-specific */ > + state->link = !!(val & PCS_CFG_LNKSTS); > + if (!state->link) > + return; > + > + switch (FIELD_GET(PCS_CFG_LNKSPEED, val)) { > + case PCS_CFG_LNKSPEED_2_5: > + state->speed = SPEED_10; > + break; > + case PCS_CFG_LNKSPEED_25: > + state->speed = SPEED_100; > + break; > + case PCS_CFG_LNKSPEED_250: > + state->speed = SPEED_1000; > + break; > + default: > + netdev_err(priv->dev, "Unknown speed detected\n"); > + break; > + } > + > + state->duplex = val & PCS_CFG_LNKMOD ? DUPLEX_FULL : DUPLEX_HALF; > + > + /* 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 ? 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? > > - /* 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. 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.
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. > > > +static void dwmac_pcs_get_state(struct phylink_pcs *pcs, > > + struct phylink_link_state *state) > > { > > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); > > struct stmmac_priv *priv = hw->priv; > > u32 val; > > > > + val = stmmac_pcs_get_config_reg(priv, hw); > > + > > + /* TODO The next is SGMII/RGMII/SMII-specific */ > > + state->link = !!(val & PCS_CFG_LNKSTS); > > + if (!state->link) > > + return; > > + > > + switch (FIELD_GET(PCS_CFG_LNKSPEED, val)) { > > + case PCS_CFG_LNKSPEED_2_5: > > + state->speed = SPEED_10; > > + break; > > + case PCS_CFG_LNKSPEED_25: > > + state->speed = SPEED_100; > > + break; > > + case PCS_CFG_LNKSPEED_250: > > + state->speed = SPEED_1000; > > + break; > > + default: > > + netdev_err(priv->dev, "Unknown speed detected\n"); > > + break; > > + } > > + > > + state->duplex = val & PCS_CFG_LNKMOD ? DUPLEX_FULL : DUPLEX_HALF; > > + > > + /* 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. > 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. > > > > > - /* 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)? > > 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. -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 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.
On Wed, Jul 03, 2024 at 09:07:22PM +0100, Russell King (Oracle) wrote: > 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. Actually I did mean mac->mac_pcs.ops. AFAICS the stmmac_hwif_init() method semantics implies that the plat_stmmacenet_data::setup() function responsibility is to allocate the mac_device_info instance and pre-initialize it' fields with the data specific for the particular device including the DW MAC HW-interface ops. Like it's done in the dwmac-sun8i.c driver (and in the currently being reviewed Loongson GMAC/GNET series). So I suppose it should also concern the internal PCS ops implementation being added by you. In case if some particular controller has some internal PCS peculiarities required to be fixed on the PHY-link PCS ops implementation level. No? > > > > > + /* 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. Fully agree. > > 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). Agreed. Good idea. > > > > 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 */ Got it. Thanks. > > > > 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. Ok. Let me know what is going to be my part in the next patch set revision preparation and when my help is needed. I think I'll be able to allocate some evenings and a few weekend days for that in this month. I very much hope my work schedule will be less occupied in the next month. -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/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h index f3a95d27298c..94be66e794be 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h @@ -87,19 +87,7 @@ enum power_event { /* SGMII/RGMII status register */ #define GMAC_RGSMIIIS_CONFIG_REG GENMASK(15, 0) -#define GMAC_RGSMIIIS_LNKMODE BIT(0) -#define GMAC_RGSMIIIS_SPEED GENMASK(2, 1) -#define GMAC_RGSMIIIS_SPEED_SHIFT 1 -#define GMAC_RGSMIIIS_LNKSTS BIT(3) -#define GMAC_RGSMIIIS_JABTO BIT(4) -#define GMAC_RGSMIIIS_FALSECARDET BIT(5) #define GMAC_RGSMIIIS_SMIDRXS BIT(16) -/* LNKMOD */ -#define GMAC_RGSMIIIS_LNKMOD_MASK 0x1 -/* LNKSPEED */ -#define GMAC_RGSMIIIS_SPEED_125 0x2 -#define GMAC_RGSMIIIS_SPEED_25 0x1 -#define GMAC_RGSMIIIS_SPEED_2_5 0x0 /* GMAC Configuration defines */ #define GMAC_CONTROL_2K 0x08000000 /* IEEE 802.3as 2K packets */ diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c index 9511ea753da7..332018ecd624 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c @@ -395,79 +395,6 @@ static u16 dwmac1000_pcs_get_config_reg(struct mac_device_info *hw) return FIELD_GET(GMAC_RGSMIIIS_CONFIG_REG, val); } -static int dwmac1000_mii_pcs_validate(struct phylink_pcs *pcs, - unsigned long *supported, - const struct phylink_link_state *state) -{ - /* Only support in-band */ - if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising)) - return -EINVAL; - - return 0; -} - -static int dwmac1000_mii_pcs_enable(struct phylink_pcs *pcs) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - - dwmac1000_pcs_enable_irq(hw); - - return 0; -} - -static void dwmac1000_mii_pcs_disable(struct phylink_pcs *pcs) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - - dwmac1000_pcs_disable_irq(hw); -} - -static int dwmac1000_mii_pcs_config(struct phylink_pcs *pcs, - unsigned int neg_mode, - phy_interface_t interface, - const unsigned long *advertising, - bool permit_pause_to_mac) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - - return dwmac_pcs_config(hw, neg_mode, advertising, advertising); -} - -static void dwmac1000_mii_pcs_get_state(struct phylink_pcs *pcs, - struct phylink_link_state *state) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - unsigned int spd_clk; - u32 status; - - status = readl(hw->pcsr + GMAC_RGSMIIIS); - - state->link = status & GMAC_RGSMIIIS_LNKSTS; - if (!state->link) - return; - - spd_clk = FIELD_GET(GMAC_RGSMIIIS_SPEED, status); - if (spd_clk == GMAC_RGSMIIIS_SPEED_125) - state->speed = SPEED_1000; - else if (spd_clk == GMAC_RGSMIIIS_SPEED_25) - state->speed = SPEED_100; - else if (spd_clk == GMAC_RGSMIIIS_SPEED_2_5) - state->speed = SPEED_10; - - state->duplex = status & GMAC_RGSMIIIS_LNKMOD_MASK ? - DUPLEX_FULL : DUPLEX_HALF; - - dwmac_pcs_get_state(hw, state); -} - -static const struct phylink_pcs_ops dwmac1000_mii_pcs_ops = { - .pcs_validate = dwmac1000_mii_pcs_validate, - .pcs_enable = dwmac1000_mii_pcs_enable, - .pcs_disable = dwmac1000_mii_pcs_disable, - .pcs_config = dwmac1000_mii_pcs_config, - .pcs_get_state = dwmac1000_mii_pcs_get_state, -}; - static struct phylink_pcs * dwmac1000_phylink_select_pcs(struct stmmac_priv *priv, phy_interface_t interface) @@ -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; return 0; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h index bb2997191f08..5c765e16bc13 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h @@ -568,18 +568,6 @@ static inline u32 mtl_low_credx_base_addr(const struct dwmac4_addrs *addrs, #define GMAC_PHYIF_CTRLSTATUS_LUD BIT(1) #define GMAC_PHYIF_CTRLSTATUS_SMIDRXS BIT(4) #define GMAC_PHYIF_CTRLSTATUS_CONFIG_REG GENMASK(31, 16) -#define GMAC_PHYIF_CTRLSTATUS_LNKMOD BIT(16) -#define GMAC_PHYIF_CTRLSTATUS_SPEED GENMASK(18, 17) -#define GMAC_PHYIF_CTRLSTATUS_SPEED_SHIFT 17 -#define GMAC_PHYIF_CTRLSTATUS_LNKSTS BIT(19) -#define GMAC_PHYIF_CTRLSTATUS_JABTO BIT(20) -#define GMAC_PHYIF_CTRLSTATUS_FALSECARDET BIT(21) -/* LNKMOD */ -#define GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK 0x1 -/* LNKSPEED */ -#define GMAC_PHYIF_CTRLSTATUS_SPEED_125 0x2 -#define GMAC_PHYIF_CTRLSTATUS_SPEED_25 0x1 -#define GMAC_PHYIF_CTRLSTATUS_SPEED_2_5 0x0 extern const struct stmmac_dma_ops dwmac4_dma_ops; extern const struct stmmac_dma_ops dwmac410_dma_ops; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index 1e73c14f36ce..1487f5cc5249 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c @@ -780,85 +780,6 @@ static void dwmac4_flow_ctrl(struct mac_device_info *hw, unsigned int duplex, } } -static int dwmac4_mii_pcs_validate(struct phylink_pcs *pcs, - unsigned long *supported, - const struct phylink_link_state *state) -{ - /* Only support in-band */ - if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising)) - return -EINVAL; - - return 0; -} - -static int dwmac4_mii_pcs_enable(struct phylink_pcs *pcs) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - - dwmac4_pcs_enable_irq(hw); - - return 0; -} - -static void dwmac4_mii_pcs_disable(struct phylink_pcs *pcs) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - - dwmac4_pcs_disable_irq(hw); -} - -static int dwmac4_mii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, - phy_interface_t interface, - const unsigned long *advertising, - bool permit_pause_to_mac) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - - return dwmac_pcs_config(hw, advertising, interface, advertising); -} - -static void dwmac4_mii_pcs_get_state(struct phylink_pcs *pcs, - struct phylink_link_state *state) -{ - struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); - unsigned int clk_spd; - u32 status; - - status = readl(hw->pcsr + GMAC_PHYIF_CONTROL_STATUS); - - state->link = !!(status & GMAC_PHYIF_CTRLSTATUS_LNKSTS); - if (!state->link) - return; - - clk_spd = FIELD_GET(GMAC_PHYIF_CTRLSTATUS_SPEED, status); - if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_125) - state->speed = SPEED_1000; - else if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_25) - state->speed = SPEED_100; - else if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_2_5) - state->speed = SPEED_10; - - /* FIXME: Is this even correct? - * GMAC_PHYIF_CTRLSTATUS_TC = BIT(0) - * GMAC_PHYIF_CTRLSTATUS_LNKMOD = BIT(16) - * GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK = 1 - * - * The result is, we test bit 0 for the duplex setting. - */ - state->duplex = status & GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK ? - DUPLEX_FULL : DUPLEX_HALF; - - dwmac_pcs_get_state(hw, state); -} - -static const struct phylink_pcs_ops dwmac4_mii_pcs_ops = { - .pcs_validate = dwmac4_mii_pcs_validate, - .pcs_enable = dwmac4_mii_pcs_enable, - .pcs_disable = dwmac4_mii_pcs_disable, - .pcs_config = dwmac4_mii_pcs_config, - .pcs_get_state = dwmac4_mii_pcs_get_state, -}; - static struct phylink_pcs * dwmac4_phylink_select_pcs(struct stmmac_priv *priv, phy_interface_t interface) { @@ -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; return 0; 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; mac->priv = priv; priv->hw = mac; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c index 41b99f7e36e6..24b95d1fdb64 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c @@ -1,17 +1,54 @@ +#include <linux/phylink.h> + #include "common.h" #include "stmmac_pcs.h" -int dwmac_pcs_config(struct mac_device_info *hw, unsigned int neg_mode, - phy_interface_t interface, - const unsigned long *advertising) +static int dwmac_pcs_validate(struct phylink_pcs *pcs, unsigned long *supported, + const struct phylink_link_state *state) +{ + /* Only support in-band */ + if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising)) + return -EINVAL; + + return 0; +} + +static int dwmac_pcs_enable(struct phylink_pcs *pcs) +{ + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); + + stmmac_pcs_enable_irq(hw->priv, hw); + + return 0; +} + +static void dwmac_pcs_disable(struct phylink_pcs *pcs) { + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); + + stmmac_pcs_disable_irq(hw->priv, hw); +} + +static int dwmac_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) +{ + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); struct stmmac_priv *priv = hw->priv; u32 val; + /* TODO Think about this: + * + En/dis SGMII/RGMII IRQs based on the neg_mode value? + * + Do we need to set PCS_CONTROL.TC?.. For SGMII MAC2MAC? + * + The next is SGMII/RTBI/TBI-specific + */ + val = readl(priv->pcsaddr + PCS_AN_CTRL); val |= PCS_AN_CTRL_ANE | PCS_AN_CTRL_RAN; + /* + The SGMRAL flag is SGMII-specific */ if (hw->ps) val |= PCS_AN_CTRL_SGMRAL; @@ -20,12 +57,40 @@ int dwmac_pcs_config(struct mac_device_info *hw, unsigned int neg_mode, return 0; } -void dwmac_pcs_get_state(struct mac_device_info *hw, - struct phylink_link_state *state) +static void dwmac_pcs_get_state(struct phylink_pcs *pcs, + struct phylink_link_state *state) { + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); struct stmmac_priv *priv = hw->priv; u32 val; + val = stmmac_pcs_get_config_reg(priv, hw); + + /* TODO The next is SGMII/RGMII/SMII-specific */ + state->link = !!(val & PCS_CFG_LNKSTS); + if (!state->link) + return; + + switch (FIELD_GET(PCS_CFG_LNKSPEED, val)) { + case PCS_CFG_LNKSPEED_2_5: + state->speed = SPEED_10; + break; + case PCS_CFG_LNKSPEED_25: + state->speed = SPEED_100; + break; + case PCS_CFG_LNKSPEED_250: + state->speed = SPEED_1000; + break; + default: + netdev_err(priv->dev, "Unknown speed detected\n"); + break; + } + + state->duplex = val & PCS_CFG_LNKMOD ? DUPLEX_FULL : DUPLEX_HALF; + + /* 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); linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, @@ -49,7 +114,10 @@ void dwmac_pcs_get_state(struct mac_device_info *hw, state->lp_advertising); } - /* 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); @@ -59,4 +127,10 @@ void dwmac_pcs_get_state(struct mac_device_info *hw, } const struct phylink_pcs_ops dwmac_pcs_ops = { + .pcs_validate = dwmac_pcs_validate, + .pcs_enable = dwmac_pcs_enable, + .pcs_disable = dwmac_pcs_disable, + .pcs_config = dwmac_pcs_config, + .pcs_get_state = dwmac_pcs_get_state, + }; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h index 76badfd208b6..2baebb92bea7 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h @@ -47,6 +47,16 @@ #define PCS_ANE_RFE_SHIFT 12 #define PCS_ANE_ACK BIT(14) /* AN Base-page acknowledge */ +/* SGMII/RGMII/SMII link status register */ +#define PCS_CFG_LNKMOD BIT(0) /* Link Duplex Mode */ +#define PCS_CFG_LNKSPEED GENMASK(2, 1) /* Link Speed: */ +#define PCS_CFG_LNKSPEED_2_5 0x0 /* 2.5 MHz - 10 Mbps */ +#define PCS_CFG_LNKSPEED_25 0x1 /* 25 MHz - 100 Mbps */ +#define PCS_CFG_LNKSPEED_250 0x2 /* 250 MHz - 1000 Mbps */ +#define PCS_CFG_LNKSTS BIT(3) /* Link Up/Down Status */ +#define PCS_CFG_JABTO BIT(4) /* Jabber Timeout (SMII only) */ +#define PCS_CFG_FALSCARDET BIT(5) /* False Carrier (SMII only) */ + /** * dwmac_pcs_isr - TBI, RTBI, or SGMII PHY ISR * @ioaddr: IO registers pointer @@ -76,11 +86,4 @@ static inline void dwmac_pcs_isr(void __iomem *pcsaddr, } } -int dwmac_pcs_config(struct mac_device_info *hw, unsigned int neg_mode, - phy_interface_t interface, - const unsigned long *advertising); - -void dwmac_pcs_get_state(struct mac_device_info *hw, - struct phylink_link_state *state); - #endif /* __STMMAC_PCS_H__ */
The PCS-related code is now ready to be consolidated in the PCS-specific module. Let's move the PHYLINK PCS operations implementation to the stmmac_pcs.c file. No semantics has changed. The same functionality has been re-implemented in stammac_pcs.c by using the generic link status register macros and the phylink_pcs_ops instance has been populated with the new callbacks. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- Note the code has been equipped with some TODO-notes to think about on the RFC review stage. --- .../net/ethernet/stmicro/stmmac/dwmac1000.h | 12 --- .../ethernet/stmicro/stmmac/dwmac1000_core.c | 74 ---------------- drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 12 --- .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 80 ----------------- drivers/net/ethernet/stmicro/stmmac/hwif.c | 1 + .../net/ethernet/stmicro/stmmac/stmmac_pcs.c | 86 +++++++++++++++++-- .../net/ethernet/stmicro/stmmac/stmmac_pcs.h | 17 ++-- 7 files changed, 91 insertions(+), 191 deletions(-)