| Message ID | E1qZAXd-005pUP-JL@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stmmac cleanups | expand |
On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote: > Move the xgmac specific phylink capabilities to the dwxgmac2 support > core. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ---------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > index 34e1b0c3f346..f352be269deb 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, > writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); > } > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv) > +{ > + priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | > + MAC_10000FD | MAC_25000FD | > + MAC_40000FD | MAC_50000FD | > + MAC_100000FD; > +} > + > static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) > { > u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq, > > const struct stmmac_ops dwxgmac210_ops = { > .core_init = dwxgmac2_core_init, > + .phylink_get_caps = xgmac_phylink_get_caps, This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps speeds. > .set_mac = dwxgmac2_set_mac, > .rx_ipc = dwxgmac2_rx_ipc, > .rx_queue_enable = dwxgmac2_rx_queue_enable, > @@ -1551,6 +1560,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, > > const struct stmmac_ops dwxlgmac2_ops = { > .core_init = dwxgmac2_core_init, > + .phylink_get_caps = xgmac_phylink_get_caps, This is ok. -Serge(y) > .set_mac = dwxgmac2_set_mac, > .rx_ipc = dwxgmac2_rx_ipc, > .rx_queue_enable = dwxlgmac2_rx_queue_enable, > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 0b02845e7e9d..5cf8304564c6 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1227,16 +1227,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) > /* Get the MAC specific capabilities */ > stmmac_mac_phylink_get_caps(priv); > > - if (priv->plat->has_xgmac) { > - priv->phylink_config.mac_capabilities |= MAC_2500FD; > - priv->phylink_config.mac_capabilities |= MAC_5000FD; > - priv->phylink_config.mac_capabilities |= MAC_10000FD; > - priv->phylink_config.mac_capabilities |= MAC_25000FD; > - priv->phylink_config.mac_capabilities |= MAC_40000FD; > - priv->phylink_config.mac_capabilities |= MAC_50000FD; > - priv->phylink_config.mac_capabilities |= MAC_100000FD; > - } > - > /* Half-Duplex can only work with single queue */ > if (priv->plat->tx_queues_to_use > 1) > priv->phylink_config.mac_capabilities &= > -- > 2.30.2 > >
On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote: > Move the xgmac specific phylink capabilities to the dwxgmac2 support > core. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ---------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > index 34e1b0c3f346..f352be269deb 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, > writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); > } > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv) Also after splitting this method up into DW XGMAC v2.x and DW XLGMAC v2.x specific functions please preserve the local naming convention: use dwxgmac2_ and dwxlgmac2_ prefixes. -Serge(y) > +{ > + priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | > + MAC_10000FD | MAC_25000FD | > + MAC_40000FD | MAC_50000FD | > + MAC_100000FD; > +} > + > static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) > { > u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq, > > const struct stmmac_ops dwxgmac210_ops = { > .core_init = dwxgmac2_core_init, > + .phylink_get_caps = xgmac_phylink_get_caps, > .set_mac = dwxgmac2_set_mac, > .rx_ipc = dwxgmac2_rx_ipc, > .rx_queue_enable = dwxgmac2_rx_queue_enable, > @@ -1551,6 +1560,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, > > const struct stmmac_ops dwxlgmac2_ops = { > .core_init = dwxgmac2_core_init, > + .phylink_get_caps = xgmac_phylink_get_caps, > .set_mac = dwxgmac2_set_mac, > .rx_ipc = dwxgmac2_rx_ipc, > .rx_queue_enable = dwxlgmac2_rx_queue_enable, > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 0b02845e7e9d..5cf8304564c6 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1227,16 +1227,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) > /* Get the MAC specific capabilities */ > stmmac_mac_phylink_get_caps(priv); > > - if (priv->plat->has_xgmac) { > - priv->phylink_config.mac_capabilities |= MAC_2500FD; > - priv->phylink_config.mac_capabilities |= MAC_5000FD; > - priv->phylink_config.mac_capabilities |= MAC_10000FD; > - priv->phylink_config.mac_capabilities |= MAC_25000FD; > - priv->phylink_config.mac_capabilities |= MAC_40000FD; > - priv->phylink_config.mac_capabilities |= MAC_50000FD; > - priv->phylink_config.mac_capabilities |= MAC_100000FD; > - } > - > /* Half-Duplex can only work with single queue */ > if (priv->plat->tx_queues_to_use > 1) > priv->phylink_config.mac_capabilities &= > -- > 2.30.2 > >
On Sat, Aug 26, 2023 at 04:32:15PM +0300, Serge Semin wrote: > On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote: > > Move the xgmac specific phylink capabilities to the dwxgmac2 support > > core. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++ > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ---------- > > 2 files changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > index 34e1b0c3f346..f352be269deb 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, > > writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); > > } > > > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv) > > +{ > > + priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | > > + MAC_10000FD | MAC_25000FD | > > + MAC_40000FD | MAC_50000FD | > > + MAC_100000FD; > > +} > > + > > static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) > > { > > u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); > > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq, > > > > const struct stmmac_ops dwxgmac210_ops = { > > .core_init = dwxgmac2_core_init, > > > + .phylink_get_caps = xgmac_phylink_get_caps, > > This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps > speeds. So the reason this got added is to keep the code compatible with how things work today. When priv->plat->has_xgmac is true, the old code in stmmac_phy_setup() would enable speeds from 2.5G up to 100G, limiting them if priv->plat->max_speed is set non-zero. The table in hwif.c matches when: entry->gmac == priv->plat->has_gmac, entry->gmac4 == priv->plat->has_gmac4 and entry->xgmac == priv->plat->has_xgmac The entries in the table which patch on has_xgmac = true contain the following: .mac = &dwxgmac210_ops, .mac = &dwxlgmac2_ops, Therefore, to keep things compatible, I've effectively moved this initialisation into the new .phylink_get_caps method that is part of those two ops, and since they have has_xgmac true, this means that all these speeds need to be set. We do this without regard to max_speed, which we apply separately, after the .phylink_get_caps method has returned. So, the code is functionally identical to what happens in the driver, even if it is the case that xgmac210 doesn't actually support the speeds. If those extra speeds that the hardware doesn't support were present before, they're present after. If those extra speeds are limited by the max_speed, then they will be similarly limited. While it may look odd, since the specifications for Synopsys are all behind closed doors, all I can do is transform the code - I can't know that such-and-such a core doesn't actually support stuff. So my only option is to keep the code bug-compatible. I think all I've done here is make it glaringly obvious what the old code is doing and you've spotted "but that isn't right!" - which is actually a good thing! Feel free to submit patches to correct the functionality as bugs in the driver become more obvious!
On Sat, Aug 26, 2023 at 04:36:46PM +0300, Serge Semin wrote: > On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote: > > Move the xgmac specific phylink capabilities to the dwxgmac2 support > > core. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++ > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ---------- > > 2 files changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > index 34e1b0c3f346..f352be269deb 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, > > writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); > > } > > > > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv) > > Also after splitting this method up into DW XGMAC v2.x and DW XLGMAC > v2.x specific functions please preserve the local naming convention: > use dwxgmac2_ and dwxlgmac2_ prefixes. The only possibility I have would be to implement two functions with different names but are functionally identical, since I have no further information. The new code is functionally identical to the code it replaces - as explained in my previous response.
On Sat, Aug 26, 2023 at 03:51:53PM +0100, Russell King (Oracle) wrote: > On Sat, Aug 26, 2023 at 04:32:15PM +0300, Serge Semin wrote: > > On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote: > > > Move the xgmac specific phylink capabilities to the dwxgmac2 support > > > core. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++ > > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ---------- > > > 2 files changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > > index 34e1b0c3f346..f352be269deb 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, > > > writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); > > > } > > > > > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv) > > > +{ > > > + priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | > > > + MAC_10000FD | MAC_25000FD | > > > + MAC_40000FD | MAC_50000FD | > > > + MAC_100000FD; > > > +} > > > + > > > static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) > > > { > > > u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); > > > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq, > > > > > > const struct stmmac_ops dwxgmac210_ops = { > > > .core_init = dwxgmac2_core_init, > > > > > + .phylink_get_caps = xgmac_phylink_get_caps, > > > > This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps > > speeds. > > So the reason this got added is to keep the code compatible with how > things work today. > > When priv->plat->has_xgmac is true, the old code in stmmac_phy_setup() > would enable speeds from 2.5G up to 100G, limiting them if > priv->plat->max_speed is set non-zero. Indeed. I didn't consider it has been coded like that long before this discussion. > > The table in hwif.c matches when: > entry->gmac == priv->plat->has_gmac, > entry->gmac4 == priv->plat->has_gmac4 and > entry->xgmac == priv->plat->has_xgmac > > The entries in the table which patch on has_xgmac = true contain the > following: > > .mac = &dwxgmac210_ops, > .mac = &dwxlgmac2_ops, > > Therefore, to keep things compatible, I've effectively moved this > initialisation into the new .phylink_get_caps method that is part of > those two ops, and since they have has_xgmac true, this means that > all these speeds need to be set. > > We do this without regard to max_speed, which we apply separately, > after the .phylink_get_caps method has returned. > > So, the code is functionally identical to what happens in the driver, > even if it is the case that xgmac210 doesn't actually support the > speeds. If those extra speeds that the hardware doesn't support were > present before, they're present after. If those extra speeds are > limited by the max_speed, then they will be similarly limited. > > While it may look odd, since the specifications for Synopsys are all > behind closed doors, all I can do is transform the code - I can't > know that such-and-such a core doesn't actually support stuff. So > my only option is to keep the code bug-compatible. > If I see odd things and have no specs at hand I normally dig into the git log in order to find a respective commit, then get to finding the corresponding lkml discussion which may have some clue of possible oddness justification. In this case the problematic part was added in the commit 8a880936e902 ("net: stmmac: Add XLGMII support"). So before having the XLGMAC support added the DW XGMAC had had standard speeds support. Seeing there were no discussion concerning that part of the code within the review it was very likely wrong to add the higher speeds support to the DW XMGAC controller. But of course with no databook at hand it gets to be still an assumption but with high probability to be true though. > I think all I've done here is make it glaringly obvious what the old > code is doing and you've spotted "but that isn't right!" - which is > actually a good thing! > > Feel free to submit patches to correct the functionality as bugs in > the driver become more obvious! I see your point. Ok I'll add such fix to my extensive collection of the STMMAC driver bug-fixes.) I'll send it out to the community eventually when I get to have more spare time for review. -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/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index 34e1b0c3f346..f352be269deb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw, writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN); } +static void xgmac_phylink_get_caps(struct stmmac_priv *priv) +{ + priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD | + MAC_10000FD | MAC_25000FD | + MAC_40000FD | MAC_50000FD | + MAC_100000FD; +} + static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable) { u32 tx = readl(ioaddr + XGMAC_TX_CONFIG); @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq, const struct stmmac_ops dwxgmac210_ops = { .core_init = dwxgmac2_core_init, + .phylink_get_caps = xgmac_phylink_get_caps, .set_mac = dwxgmac2_set_mac, .rx_ipc = dwxgmac2_rx_ipc, .rx_queue_enable = dwxgmac2_rx_queue_enable, @@ -1551,6 +1560,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, const struct stmmac_ops dwxlgmac2_ops = { .core_init = dwxgmac2_core_init, + .phylink_get_caps = xgmac_phylink_get_caps, .set_mac = dwxgmac2_set_mac, .rx_ipc = dwxgmac2_rx_ipc, .rx_queue_enable = dwxlgmac2_rx_queue_enable, diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 0b02845e7e9d..5cf8304564c6 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1227,16 +1227,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) /* Get the MAC specific capabilities */ stmmac_mac_phylink_get_caps(priv); - if (priv->plat->has_xgmac) { - priv->phylink_config.mac_capabilities |= MAC_2500FD; - priv->phylink_config.mac_capabilities |= MAC_5000FD; - priv->phylink_config.mac_capabilities |= MAC_10000FD; - priv->phylink_config.mac_capabilities |= MAC_25000FD; - priv->phylink_config.mac_capabilities |= MAC_40000FD; - priv->phylink_config.mac_capabilities |= MAC_50000FD; - priv->phylink_config.mac_capabilities |= MAC_100000FD; - } - /* Half-Duplex can only work with single queue */ if (priv->plat->tx_queues_to_use > 1) priv->phylink_config.mac_capabilities &=
Move the xgmac specific phylink capabilities to the dwxgmac2 support core. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++ drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ---------- 2 files changed, 10 insertions(+), 10 deletions(-)