From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2025-09-24 18:17:53
This series is radical - it takes the brave step of ripping out much of
the existing PCS support code and throwing it all away.
I have discussed the introduction of the STMMAC_FLAG_HAS_INTEGRATED_PCS
flag with Bartosz Golaszewski, and the conclusion I came to is that
this is to workaround the breakage that I've been going on about
concerning the phylink conversion for the last five or six years.
The problem is that the stmmac PCS code manipulates the netif carrier
state, which confuses phylink.
There is a way of testing this out on the Jetson Xavier NX platform as
the "PCS" code paths can be exercised while in RGMII mode - because
RGMII also has in-band status and the status register is shared with
SGMII. Testing this out confirms my long held theory: the interrupt
handler manipulates the netif carrier state before phylink gets a
look-in, which means that the mac_link_up() and mac_link_down() methods
are never called, resulting in the device being non-functional.
Moreover, on dwmac4 cores, ethtool reports incorrect information -
despite having a full-duplex link, ethtool reports that it is
half-dupex.
Thus, this code is completely broken - anyone using it will not have
a functional platform, and thus it doesn't deserve to live any longer,
especially as it's a thorn in phylink.
Rip all this out, leaving just the bare bones initialisation in place.
However, this is not the last of what's broken. We have this hw->ps
integer which is really not descriptive, and the DT property from
which it comes from does little to help understand what's going on.
Putting all the clues together:
- early configuration of the GMAC configuration register for the
speed.
- setting the SGMII rate adapter layer to take its speed from the
GMAC configuration register.
Lastly, setting the transmit enable (TE) bit, which is a typo that puts
the nail in the coffin of this code. It should be the transmit
configuration (TC) bit. Given that when the link comes up, phylink
will call mac_link_up() which will overwrite the speed in the GMAC
configuration register, the only part of this that is functional is
changing where the SGMII rate adapter layer gets its speed from,
which is a boolean.
From what I've found so far, everyone who sets the snps,ps-speed
property which configures this mode also configures a fixed link,
so the pre-configuration is unnecessary - the link will come up
anyway.
So, this series rips that out the preconfiguration as well, and
replaces hw->ps with a boolean hw->reverse_sgmii_enable flag.
We then move the sole PCS configuration into a phylink_pcs instance,
which configures the PCS control register in the same way as is done
during the probe function.
Thus, we end up with much easier and simpler conversion to phylink PCS
than previous attempts.
Even so, this still results in inband mode always being enabled at the
moment in the new .pcs_config() method to reflect what the probe
function was doing. The next stage will be to change that to allow
phylink to correctly configure the PCS. This needs fixing to allow
platform glue maintainers who are currently blocked to progress.
Please note, however, that this has not been tested with any SGMII
platform.
I've tried to get as many people into the Cc list with get_maintainers,
I hope that's sufficient to get enough eyeballs on this.
drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
drivers/net/ethernet/stmicro/stmmac/common.h | 5 +-
.../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 6 +-
drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 6 +-
.../net/ethernet/stmicro/stmmac/dwmac1000_core.c | 65 ++-------------------
drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 3 +-
drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 66 ++-------------------
.../net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 25 +-------
drivers/net/ethernet/stmicro/stmmac/hwif.h | 4 +-
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 4 ++
.../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 68 +---------------------
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 ++++----
drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c | 47 +++++++++++++++
drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h | 23 ++++++--
include/linux/stmmac.h | 1 -
15 files changed, 104 insertions(+), 245 deletions(-)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:20:22
Changing the netif_carrier_*() state behind phylink's back has always
been prohibited because it messes up with phylinks state tracking, and
means that phylink no longer guarantees to call the mac_link_down()
and mac_link_up() methods at the appropriate times. This was later
documented in the sfp-phylink network driver conversion guide.
stmmac was converted to phylink in 2019, but nothing was done with the
"PCS" code. Since then, apart from the updates as part of phylink
development, nothing has happened with stmmac to improve its use of
phylink, or even to address this point.
A couple of years ago, a has_integrated_pcs boolean was added by Bart,
which later became the STMMAC_FLAG_HAS_INTEGRATED_PCS flag, to avoid
manipulating the netif_carrier_*() state. This flag is mis-named,
because whenever the stmmac is synthesized for its native SGMII, TBI
or RTBI interfaces, it has an "integrated PCS". This boolean/flag
actually means "ignore the status from the integrated PCS".
Discussing with Bart, the reasons for this are lost to the winds of
time (which is why we should always document the reasons in the commit
message.)
RGMII also has in-band status, and the dwmac cores and stmmac code
supports this but with one bug that saves the day.
When dwmac cores are synthesised for RGMII only, they do not contain
an integrated PCS, and so priv->dma_cap.pcs is clear, which prevents
(incorrectly) the "RGMII PCS" being used, meaning we don't read the
in-band status. However, a core synthesised for RGMII and also SGMII,
TBI or RTBI will have this capability bit set, thus making these
code paths reachable.
The Jetson Xavier NX uses RGMII mode to talk to its PHY, and removing
the incorrect check for priv->dma_cap.pcs reveals the theortical issue
with netif_carrier_*() manipulation is real:
dwc-eth-dwmac 2490000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
dwc-eth-dwmac 2490000.ethernet eth0: PHY [stmmac-0:00] driver [RTL8211F Gigabit Ethernet] (irq=141)
dwc-eth-dwmac 2490000.ethernet eth0: No Safety Features support found
dwc-eth-dwmac 2490000.ethernet eth0: IEEE 1588-2008 Advanced Timestamp supported
dwc-eth-dwmac 2490000.ethernet eth0: registered PTP clock
dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii-id link mode
8021q: adding VLAN 0 to HW filter on device eth0
dwc-eth-dwmac 2490000.ethernet eth0: Adding VLAN ID 0 is not supported
Link is Up - 1000/Full
Link is Down
Link is Up - 1000/Full
This looks good until one realises that the phylink "Link" status
messages are missing, even when the RJ45 cable is reconnected. Nothing
one can do results in the interface working. The interrupt handler
(which prints those "Link is" messages) always wins over phylink's
resolve worker, meaning phylink never calls the mac_link_up() nor
mac_link_down() methods.
eth0 also sees no traffic received, and is unable to obtain a DHCP
address:
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group defa
ult qlen 1000
link/ether e6:d3:6a:e6:92:de brd ff:ff:ff:ff:ff:ff
RX: bytes packets errors dropped overrun mcast
0 0 0 0 0 0
TX: bytes packets errors dropped carrier collsns
27686 149 0 0 0 0
With the STMMAC_FLAG_HAS_INTEGRATED_PCS flag set, which disables the
netif_carrier_*() manipulation then stmmac works normally:
dwc-eth-dwmac 2490000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
dwc-eth-dwmac 2490000.ethernet eth0: PHY [stmmac-0:00] driver [RTL8211F Gigabit Ethernet] (irq=141)
dwc-eth-dwmac 2490000.ethernet eth0: No Safety Features support found
dwc-eth-dwmac 2490000.ethernet eth0: IEEE 1588-2008 Advanced Timestamp supported
dwc-eth-dwmac 2490000.ethernet eth0: registered PTP clock
dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii-id link mode
8021q: adding VLAN 0 to HW filter on device eth0
dwc-eth-dwmac 2490000.ethernet eth0: Adding VLAN ID 0 is not supported
Link is Up - 1000/Full
dwc-eth-dwmac 2490000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
and packets can be transferred.
This clearly shows that when priv->hw->pcs is set, but
STMMAC_FLAG_HAS_INTEGRATED_PCS is clear, the driver reliably fails.
Discovering whether a platform falls into this is impossible as
parsing all the dtsi and dts files to find out which use the stmmac
driver, whether any of them use RGMII or SGMII and also depends
whether an external interface is being used. The kernel likely
doesn't contain all dts files either.
The only driver that sets this flag uses the qcom,sa8775p-ethqos
compatible, and uses SGMII or 2500BASE-X.
but these are saved from this problem by the incorrect check for
priv->dma_cap.pcs.
So, we have to assume that for every other platform that uses SGMII
with stmmac is using an external PCS.
Moreover, ethtool output can be incorrect. With the full-duplex link
negotiated, ethtool reports:
Speed: 1000Mb/s
Duplex: Half
because with dwmac4, the full-duplex bit is in bit 16 of the status,
priv->xstats.pcs_duplex becomes BIT(16) for full duplex, but the
ethtool ksettings duplex member is u8 - so becomes zero. Moreover,
the supported, advertised and link partner modes are all "not
reported".
Finally, ksettings_set() won't be able to set the advertisement on
a PHY if this PCS code is activated, which is incorrect when SGMII
is used with a PHY.
Thus, remove:
1. the incorrect netif_carrier_*() manipulation.
2. the broken ethtool ksettings code.
Given that all uses of STMMAC_FLAG_HAS_INTEGRATED_PCS are now gone,
remove the flag from stmmac.h and dwmac-qcom-ethqos.c.
Signed-off-by: Russell King (Oracle) <redacted>
---
.../stmicro/stmmac/dwmac-qcom-ethqos.c | 4 --
.../ethernet/stmicro/stmmac/stmmac_ethtool.c | 55 -------------------
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 9 ---
include/linux/stmmac.h | 1 -
4 files changed, 69 deletions(-)
@@ -322,47 +322,6 @@ static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,{structstmmac_priv*priv=netdev_priv(dev);-if(!(priv->plat->flags&STMMAC_FLAG_HAS_INTEGRATED_PCS)&&-(priv->hw->pcs&STMMAC_PCS_RGMII||-priv->hw->pcs&STMMAC_PCS_SGMII)){-u32supported,advertising,lp_advertising;--if(!priv->xstats.pcs_link){-cmd->base.speed=SPEED_UNKNOWN;-cmd->base.duplex=DUPLEX_UNKNOWN;-return0;-}-cmd->base.duplex=priv->xstats.pcs_duplex;--cmd->base.speed=priv->xstats.pcs_speed;--/* Encoding of PSE bits is defined in 802.3z, 37.2.1.4 */--ethtool_convert_link_mode_to_legacy_u32(-&supported,cmd->link_modes.supported);-ethtool_convert_link_mode_to_legacy_u32(-&advertising,cmd->link_modes.advertising);-ethtool_convert_link_mode_to_legacy_u32(-&lp_advertising,cmd->link_modes.lp_advertising);--/* Reg49[3] always set because ANE is always supported */-cmd->base.autoneg=ADVERTISED_Autoneg;-supported|=SUPPORTED_Autoneg;-advertising|=ADVERTISED_Autoneg;-lp_advertising|=ADVERTISED_Autoneg;--cmd->base.port=PORT_OTHER;--ethtool_convert_legacy_u32_to_link_mode(-cmd->link_modes.supported,supported);-ethtool_convert_legacy_u32_to_link_mode(-cmd->link_modes.advertising,advertising);-ethtool_convert_legacy_u32_to_link_mode(-cmd->link_modes.lp_advertising,lp_advertising);--return0;-}-returnphylink_ethtool_ksettings_get(priv->phylink,cmd);}
@@ -372,20 +331,6 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev,{structstmmac_priv*priv=netdev_priv(dev);-if(!(priv->plat->flags&STMMAC_FLAG_HAS_INTEGRATED_PCS)&&-(priv->hw->pcs&STMMAC_PCS_RGMII||-priv->hw->pcs&STMMAC_PCS_SGMII)){-/* Only support ANE */-if(cmd->base.autoneg!=AUTONEG_ENABLE)-return-EINVAL;--mutex_lock(&priv->lock);-stmmac_pcs_ctrl_ane(priv,1,priv->hw->ps,0);-mutex_unlock(&priv->lock);--return0;-}-returnphylink_ethtool_ksettings_set(priv->phylink,cmd);}
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:20:39
As a result of the previous commit, the pcs_link, pcs_duplex and
pcs_speed members are not used outside of the interrupt handling code,
and are only used to print their status using the misleading "Link is"
messages that bear no relation to the actual status of the link.
Remove the printing of these messages, these members, and the code
that decodes them from the hardware.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/stmicro/stmmac/common.h | 3 --
.../ethernet/stmicro/stmmac/dwmac1000_core.c | 28 +------------------
.../net/ethernet/stmicro/stmmac/dwmac4_core.c | 28 +------------------
3 files changed, 2 insertions(+), 57 deletions(-)
@@ -266,34 +266,8 @@ static void dwmac1000_pmt(struct mac_device_info *hw, unsigned long mode)/* RGMII or SMII interface */staticvoiddwmac1000_rgsmii(void__iomem*ioaddr,structstmmac_extra_stats*x){-u32status;--status=readl(ioaddr+GMAC_RGSMIIIS);+readl(ioaddr+GMAC_RGSMIIIS);x->irq_rgmii_n++;--/* Check the link status */-if(status&GMAC_RGSMIIIS_LNKSTS){-intspeed_value;--x->pcs_link=1;--speed_value=((status&GMAC_RGSMIIIS_SPEED)>>-GMAC_RGSMIIIS_SPEED_SHIFT);-if(speed_value==GMAC_RGSMIIIS_SPEED_125)-x->pcs_speed=SPEED_1000;-elseif(speed_value==GMAC_RGSMIIIS_SPEED_25)-x->pcs_speed=SPEED_100;-else-x->pcs_speed=SPEED_10;--x->pcs_duplex=(status&GMAC_RGSMIIIS_LNKMOD_MASK);--pr_info("Link is Up - %d/%s\n",(int)x->pcs_speed,-x->pcs_duplex?"Full":"Half");-}else{-x->pcs_link=0;-pr_info("Link is Down\n");-}}staticintdwmac1000_irq_status(structmac_device_info*hw,
@@ -592,34 +592,8 @@ static void dwmac4_ctrl_ane(struct stmmac_priv *priv, bool ane, bool srgmi_ral,/* RGMII or SMII interface */staticvoiddwmac4_phystatus(void__iomem*ioaddr,structstmmac_extra_stats*x){-u32status;--status=readl(ioaddr+GMAC_PHYIF_CONTROL_STATUS);+readl(ioaddr+GMAC_PHYIF_CONTROL_STATUS);x->irq_rgmii_n++;--/* Check the link status */-if(status&GMAC_PHYIF_CTRLSTATUS_LNKSTS){-intspeed_value;--x->pcs_link=1;--speed_value=((status&GMAC_PHYIF_CTRLSTATUS_SPEED)>>-GMAC_PHYIF_CTRLSTATUS_SPEED_SHIFT);-if(speed_value==GMAC_PHYIF_CTRLSTATUS_SPEED_125)-x->pcs_speed=SPEED_1000;-elseif(speed_value==GMAC_PHYIF_CTRLSTATUS_SPEED_25)-x->pcs_speed=SPEED_100;-else-x->pcs_speed=SPEED_10;--x->pcs_duplex=(status&GMAC_PHYIF_CTRLSTATUS_LNKMOD);--pr_info("Link is Up - %d/%s\n",(int)x->pcs_speed,-x->pcs_duplex?"Full":"Half");-}else{-x->pcs_link=0;-pr_info("Link is Down\n");-}}staticintdwmac4_irq_mtl_status(structstmmac_priv*priv,
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:20:49
Now that the only use for the interrupt is to clear it and increment a
statistic counter (which is not that relevant anymore) remove all this
code and ensure that the interrupt remains disabled to avoid a stuck
interrupt.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 6 +++---
drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 10 ----------
drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 3 +--
drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 9 ---------
4 files changed, 4 insertions(+), 24 deletions(-)
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:21:02
Remove the "we always autoneg pause" forcing when the stmmac driver
decides that a "PCS" is present, which blocks passing the ethtool
pause calls to phylink when using SGMII mode.
This prevents the pause results being reported when a PHY is attached
using SGMII mode, or the pause settings being changed in SGMII mode.
There is no reason to prevent this.
Signed-off-by: Russell King (Oracle) <redacted>
---
.../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:21:13
After a lot of digging, it seems that the oddly named hw->ps member is
all about configuring the core for reverse SGMII. This member is set to
one of 0, SPEED_10, SPEED_100 or SPEED_1000 depending on
priv->plat->mac_port_sel_speed. On DT systems, this comes from the
"snps,ps-speed" DT property.
When set to a non-zero value, it:
1. Configures the MAC at initialisation time to operate at a specific
speed. However, this will be overwritten by mac_link_up() when the
link comes up (e.g. with the fixed-link parameters.)
Note that dwxgmac2 wants to also support SPEED_2500 and SPEED_10000,
but both these values are impossible.
2. It _incorrectly_ enables the transmitter (GMAC_CONFIG_TE) which
makes no sense, rather than enabling the "transmit configuration"
bit (GMAC_CONFIG_TC). Likely a typo.
3. It configures the SGMII rate adapter layer to retrieve its speed
setting from the MAC configuration register rather than the PHY.
There are two ways forward here:
a) fixing (2) so that we set GMAC_CONFIG_TC. However, we have platform
that set the "snps,ps-speed" property and that work today. Fixing
this will cause the RGMII, SGMII or SMII inband configuration to be
transmitted, which will be a functional change which could cause a
regression.
b) ripping out (1) and (2) as they are ineffective. This also has the
possibility of regressions, but the patch author believes this risk
is much lower than (a).
Therefore, this commit takes the approach in (b).
Signed-off-by: Russell King (Oracle) <redacted>
---
.../ethernet/stmicro/stmmac/dwmac1000_core.c | 23 +++--------------
.../net/ethernet/stmicro/stmmac/dwmac4_core.c | 24 +++---------------
.../ethernet/stmicro/stmmac/dwxgmac2_core.c | 25 ++-----------------
3 files changed, 8 insertions(+), 64 deletions(-)
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:25:45
The internal PCS registers only exist if the core is synthesized with
SGMII, TBI or RTBI support. They have no relevance for RGMII.
However, priv->hw->pcs contains a STMMAC_PCS_RGMII flag, which is set
if a PCS has been synthesized but we are operating in RGMII mode. As
the register has no effect for RGMII, there is no point calling
stmmac_pcs_ctrl_ane() in this case. Add a comment describing this
and make it conditional on STMMAC_PCS_SGMII.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -3519,7 +3519,11 @@ static int stmmac_hw_setup(struct net_device *dev)}}-if(priv->hw->pcs)+/* The PCS control register is only relevant for SGMII, TBI and RTBI+*modes.WenolongersupportTBIorRTBI,soonlyconfigurethis+*registerwhenoperatinginSGMIImodewiththeintegratedPCS.+*/+if(priv->hw->pcs&STMMAC_PCS_SGMII)stmmac_pcs_ctrl_ane(priv,1,priv->hw->reverse_sgmii_enable);/* set TX and RX rings length */
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:26:30
Now that stmmac's PCS support is much more simple - just a matter of
configuring the control register - the basic conversion to phylink PCS
support becomes straight forward.
Create the infrastructure to setup a phylink_pcs instance for the
integrated PCS:
- add a struct stmmac_pcs to encapsulate the phylink_pcs structure,
pointer to stmmac_priv, and the core-specific base address of the
PCS registers.
- modify stmmac_priv and stmmac_mac_select_pcs() to return the
embedded phylink_pcs structure when setup and STMMAC_PCS_SGMII is
in use, and move the comment from stmmac_hw_setup() to here.
- create stmmac_pcs.c, which contains the phylink_pcs_ops structure,
a dummy .pcs_get_state() method which always reports link-down, and
.pcs_config() method, moving the call to stmmac_pcs_ctrl_ane() here,
but without indirecting through the dwmac specific core code.
This will ensure that the PCS control register is configured to the
same settings as before, but will now happen when the netdev is
opened or reusmed rather than only during probe time. However, this
will be before the .fix_mac_speed() method is called, which is
critical for the behaviour in dwmac-qcom-ethqos's
ethqos_configure_sgmii() function to be maintained.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
.../ethernet/stmicro/stmmac/dwmac1000_core.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac4_core.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 4 ++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++---
.../net/ethernet/stmicro/stmmac/stmmac_pcs.c | 47 +++++++++++++++++++
.../net/ethernet/stmicro/stmmac/stmmac_pcs.h | 17 +++++++
7 files changed, 79 insertions(+), 10 deletions(-)
create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
@@ -883,6 +884,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,returnpcs;}+/* The PCS control register is only relevant for SGMII, TBI and RTBI+*modes.WenolongersupportTBIorRTBI,soonlyconfigurethis+*registerwhenoperatinginSGMIImodewiththeintegratedPCS.+*/+if(priv->hw->pcs&STMMAC_PCS_SGMII&&priv->integrated_pcs)+return&priv->integrated_pcs->pcs;+returnNULL;}
@@ -3519,13 +3527,6 @@ static int stmmac_hw_setup(struct net_device *dev)}}-/* The PCS control register is only relevant for SGMII, TBI and RTBI-*modes.WenolongersupportTBIorRTBI,soonlyconfigurethis-*registerwhenoperatinginSGMIImodewiththeintegratedPCS.-*/-if(priv->hw->pcs&STMMAC_PCS_SGMII)-stmmac_pcs_ctrl_ane(priv,1,priv->hw->reverse_sgmii_enable);-/* set TX and RX rings length */stmmac_set_rings_length(priv);
From: Russell King (Oracle) <hidden> Date: 2025-09-24 18:27:03
After a lot of digging, it seems that the oddly named hw->ps member
is all about setting the core into reverse SGMII speed. When set to
a non-zero value, it:
1. Configures the MAC at initialisation time to operate at a specific
speed.
2. It _incorrectly_ enables the transmitter (GMAC_CONFIG_TE) which
makes no sense, rather than enabling the "transmit configuration"
bit (GMAC_CONFIG_TC).
3. It configures the SGMII rate adapter layer to retrieve its speed
setting from the MAC configuration register rather than the PHY.
In the previous commit, we removed (1) and (2) as phylink overwrites
the configuration set at that step.
Thus, the only functional aspect is (3), which is a boolean operation.
This means there is no need to store the actual speed, and just have a
boolean flag.
Convert the priv->ps member to a boolean, and rename it to
priv->reverse_sgmii_enable to make it more understandable.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/stmicro/stmmac/common.h | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2025-09-24 19:13:28
On 9/24/2025 11:17 AM, Russell King (Oracle) wrote:
This series is radical - it takes the brave step of ripping out much of
the existing PCS support code and throwing it all away.
I have discussed the introduction of the STMMAC_FLAG_HAS_INTEGRATED_PCS
flag with Bartosz Golaszewski, and the conclusion I came to is that
this is to workaround the breakage that I've been going on about
concerning the phylink conversion for the last five or six years.
The problem is that the stmmac PCS code manipulates the netif carrier
state, which confuses phylink.
There is a way of testing this out on the Jetson Xavier NX platform as
the "PCS" code paths can be exercised while in RGMII mode - because
RGMII also has in-band status and the status register is shared with
SGMII. Testing this out confirms my long held theory: the interrupt
handler manipulates the netif carrier state before phylink gets a
look-in, which means that the mac_link_up() and mac_link_down() methods
are never called, resulting in the device being non-functional.
Moreover, on dwmac4 cores, ethtool reports incorrect information -
despite having a full-duplex link, ethtool reports that it is
half-dupex.
Thus, this code is completely broken - anyone using it will not have
a functional platform, and thus it doesn't deserve to live any longer,
especially as it's a thorn in phylink.
Rip all this out, leaving just the bare bones initialisation in place.
However, this is not the last of what's broken. We have this hw->ps
integer which is really not descriptive, and the DT property from
which it comes from does little to help understand what's going on.
Putting all the clues together:
- early configuration of the GMAC configuration register for the
speed.
- setting the SGMII rate adapter layer to take its speed from the
GMAC configuration register.
Lastly, setting the transmit enable (TE) bit, which is a typo that puts
the nail in the coffin of this code. It should be the transmit
configuration (TC) bit. Given that when the link comes up, phylink
will call mac_link_up() which will overwrite the speed in the GMAC
configuration register, the only part of this that is functional is
changing where the SGMII rate adapter layer gets its speed from,
which is a boolean.
From what I've found so far, everyone who sets the snps,ps-speed
property which configures this mode also configures a fixed link,
so the pre-configuration is unnecessary - the link will come up
anyway.
So, this series rips that out the preconfiguration as well, and
replaces hw->ps with a boolean hw->reverse_sgmii_enable flag.
We then move the sole PCS configuration into a phylink_pcs instance,
which configures the PCS control register in the same way as is done
during the probe function.
Thus, we end up with much easier and simpler conversion to phylink PCS
than previous attempts.
Even so, this still results in inband mode always being enabled at the
moment in the new .pcs_config() method to reflect what the probe
function was doing. The next stage will be to change that to allow
phylink to correctly configure the PCS. This needs fixing to allow
platform glue maintainers who are currently blocked to progress.
Please note, however, that this has not been tested with any SGMII
platform.
I've tried to get as many people into the Cc list with get_maintainers,
I hope that's sufficient to get enough eyeballs on this.
I'm no expert with this hardware or driver, but all of your explanations
seem reasonable to me.
I'd guess the real step is to try and get this tested against the
variety of hardware supported by stmmac?
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2025-09-24 19:31:44
On Wed, Sep 24, 2025 at 12:13:18PM -0700, Jacob Keller wrote:
On 9/24/2025 11:17 AM, Russell King (Oracle) wrote:
quoted
This series is radical - it takes the brave step of ripping out much of
the existing PCS support code and throwing it all away.
I have discussed the introduction of the STMMAC_FLAG_HAS_INTEGRATED_PCS
flag with Bartosz Golaszewski, and the conclusion I came to is that
this is to workaround the breakage that I've been going on about
concerning the phylink conversion for the last five or six years.
The problem is that the stmmac PCS code manipulates the netif carrier
state, which confuses phylink.
There is a way of testing this out on the Jetson Xavier NX platform as
the "PCS" code paths can be exercised while in RGMII mode - because
RGMII also has in-band status and the status register is shared with
SGMII. Testing this out confirms my long held theory: the interrupt
handler manipulates the netif carrier state before phylink gets a
look-in, which means that the mac_link_up() and mac_link_down() methods
are never called, resulting in the device being non-functional.
Moreover, on dwmac4 cores, ethtool reports incorrect information -
despite having a full-duplex link, ethtool reports that it is
half-dupex.
Thus, this code is completely broken - anyone using it will not have
a functional platform, and thus it doesn't deserve to live any longer,
especially as it's a thorn in phylink.
Rip all this out, leaving just the bare bones initialisation in place.
However, this is not the last of what's broken. We have this hw->ps
integer which is really not descriptive, and the DT property from
which it comes from does little to help understand what's going on.
Putting all the clues together:
- early configuration of the GMAC configuration register for the
speed.
- setting the SGMII rate adapter layer to take its speed from the
GMAC configuration register.
Lastly, setting the transmit enable (TE) bit, which is a typo that puts
the nail in the coffin of this code. It should be the transmit
configuration (TC) bit. Given that when the link comes up, phylink
will call mac_link_up() which will overwrite the speed in the GMAC
configuration register, the only part of this that is functional is
changing where the SGMII rate adapter layer gets its speed from,
which is a boolean.
From what I've found so far, everyone who sets the snps,ps-speed
property which configures this mode also configures a fixed link,
so the pre-configuration is unnecessary - the link will come up
anyway.
So, this series rips that out the preconfiguration as well, and
replaces hw->ps with a boolean hw->reverse_sgmii_enable flag.
We then move the sole PCS configuration into a phylink_pcs instance,
which configures the PCS control register in the same way as is done
during the probe function.
Thus, we end up with much easier and simpler conversion to phylink PCS
than previous attempts.
Even so, this still results in inband mode always being enabled at the
moment in the new .pcs_config() method to reflect what the probe
function was doing. The next stage will be to change that to allow
phylink to correctly configure the PCS. This needs fixing to allow
platform glue maintainers who are currently blocked to progress.
Please note, however, that this has not been tested with any SGMII
platform.
I've tried to get as many people into the Cc list with get_maintainers,
I hope that's sufficient to get enough eyeballs on this.
I'm no expert with this hardware or driver, but all of your explanations
seem reasonable to me.
I'd guess the real step is to try and get this tested against the
variety of hardware supported by stmmac?
Yes please, that would be very helpful, as I don't want to regress
anyone's setup. I'm hoping that this series is going to be the low-
risk change.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
From: Maxime Chevallier <maxime.chevallier@bootlin.com> Date: 2025-09-25 11:57:01
Hi Russell,
On 24/09/2025 23:47, Russell King (Oracle) wrote:
This series is radical - it takes the brave step of ripping out much of
the existing PCS support code and throwing it all away.
I have discussed the introduction of the STMMAC_FLAG_HAS_INTEGRATED_PCS
flag with Bartosz Golaszewski, and the conclusion I came to is that
this is to workaround the breakage that I've been going on about
concerning the phylink conversion for the last five or six years.
The problem is that the stmmac PCS code manipulates the netif carrier
state, which confuses phylink.
There is a way of testing this out on the Jetson Xavier NX platform as
the "PCS" code paths can be exercised while in RGMII mode - because
RGMII also has in-band status and the status register is shared with
SGMII. Testing this out confirms my long held theory: the interrupt
handler manipulates the netif carrier state before phylink gets a
look-in, which means that the mac_link_up() and mac_link_down() methods
are never called, resulting in the device being non-functional.
Moreover, on dwmac4 cores, ethtool reports incorrect information -
despite having a full-duplex link, ethtool reports that it is
half-dupex.
Thus, this code is completely broken - anyone using it will not have
a functional platform, and thus it doesn't deserve to live any longer,
especially as it's a thorn in phylink.
Rip all this out, leaving just the bare bones initialisation in place.
However, this is not the last of what's broken. We have this hw->ps
integer which is really not descriptive, and the DT property from
which it comes from does little to help understand what's going on.
Putting all the clues together:
- early configuration of the GMAC configuration register for the
speed.
- setting the SGMII rate adapter layer to take its speed from the
GMAC configuration register.
Lastly, setting the transmit enable (TE) bit, which is a typo that puts
the nail in the coffin of this code. It should be the transmit
configuration (TC) bit. Given that when the link comes up, phylink
will call mac_link_up() which will overwrite the speed in the GMAC
configuration register, the only part of this that is functional is
changing where the SGMII rate adapter layer gets its speed from,
which is a boolean.
From what I've found so far, everyone who sets the snps,ps-speed
property which configures this mode also configures a fixed link,
so the pre-configuration is unnecessary - the link will come up
anyway.
So, this series rips that out the preconfiguration as well, and
replaces hw->ps with a boolean hw->reverse_sgmii_enable flag.
We then move the sole PCS configuration into a phylink_pcs instance,
which configures the PCS control register in the same way as is done
during the probe function.
Thus, we end up with much easier and simpler conversion to phylink PCS
than previous attempts.
Even so, this still results in inband mode always being enabled at the
moment in the new .pcs_config() method to reflect what the probe
function was doing. The next stage will be to change that to allow
phylink to correctly configure the PCS. This needs fixing to allow
platform glue maintainers who are currently blocked to progress.
Please note, however, that this has not been tested with any SGMII
platform.
Thanks for that.
I'll give this a test on socfpga next week, as I don't have access to
the HW right now. It may not be the best platform to test this on, as it
has a lynx PCS and no internal PCS :/
Maxime
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2025-09-25 13:32:23
On Thu, Sep 25, 2025 at 05:26:01PM +0530, Maxime Chevallier wrote:
Hi Russell,
On 24/09/2025 23:47, Russell King (Oracle) wrote:
quoted
This series is radical - it takes the brave step of ripping out much of
the existing PCS support code and throwing it all away.
I have discussed the introduction of the STMMAC_FLAG_HAS_INTEGRATED_PCS
flag with Bartosz Golaszewski, and the conclusion I came to is that
this is to workaround the breakage that I've been going on about
concerning the phylink conversion for the last five or six years.
The problem is that the stmmac PCS code manipulates the netif carrier
state, which confuses phylink.
There is a way of testing this out on the Jetson Xavier NX platform as
the "PCS" code paths can be exercised while in RGMII mode - because
RGMII also has in-band status and the status register is shared with
SGMII. Testing this out confirms my long held theory: the interrupt
handler manipulates the netif carrier state before phylink gets a
look-in, which means that the mac_link_up() and mac_link_down() methods
are never called, resulting in the device being non-functional.
Moreover, on dwmac4 cores, ethtool reports incorrect information -
despite having a full-duplex link, ethtool reports that it is
half-dupex.
Thus, this code is completely broken - anyone using it will not have
a functional platform, and thus it doesn't deserve to live any longer,
especially as it's a thorn in phylink.
Rip all this out, leaving just the bare bones initialisation in place.
However, this is not the last of what's broken. We have this hw->ps
integer which is really not descriptive, and the DT property from
which it comes from does little to help understand what's going on.
Putting all the clues together:
- early configuration of the GMAC configuration register for the
speed.
- setting the SGMII rate adapter layer to take its speed from the
GMAC configuration register.
Lastly, setting the transmit enable (TE) bit, which is a typo that puts
the nail in the coffin of this code. It should be the transmit
configuration (TC) bit. Given that when the link comes up, phylink
will call mac_link_up() which will overwrite the speed in the GMAC
configuration register, the only part of this that is functional is
changing where the SGMII rate adapter layer gets its speed from,
which is a boolean.
From what I've found so far, everyone who sets the snps,ps-speed
property which configures this mode also configures a fixed link,
so the pre-configuration is unnecessary - the link will come up
anyway.
So, this series rips that out the preconfiguration as well, and
replaces hw->ps with a boolean hw->reverse_sgmii_enable flag.
We then move the sole PCS configuration into a phylink_pcs instance,
which configures the PCS control register in the same way as is done
during the probe function.
Thus, we end up with much easier and simpler conversion to phylink PCS
than previous attempts.
Even so, this still results in inband mode always being enabled at the
moment in the new .pcs_config() method to reflect what the probe
function was doing. The next stage will be to change that to allow
phylink to correctly configure the PCS. This needs fixing to allow
platform glue maintainers who are currently blocked to progress.
Please note, however, that this has not been tested with any SGMII
platform.
Thanks for that.
I'll give this a test on socfpga next week, as I don't have access to the HW
right now. It may not be the best platform to test this on, as it has a lynx
PCS and no internal PCS :/
Thanks for the offer of testing.
Do you know how the stmmac core has been synthesized as far as the
MII interface from it?
If not, if it's using gmac1000, possibly later cores as well, then
DMA_HW_FEATURE (or FEATURE0) bits 30:28 should give that information.
I'd guess GMII, so probably contains 0. The driver doesn't actually
use these, or even look at them.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
From: Maxime Chevallier <maxime.chevallier@bootlin.com> Date: 2025-10-03 13:25:15
Hi Russell,
Thanks for the offer of testing.
Do you know how the stmmac core has been synthesized as far as the
MII interface from it?
If not, if it's using gmac1000, possibly later cores as well, then
DMA_HW_FEATURE (or FEATURE0) bits 30:28 should give that information.
I'd guess GMII, so probably contains 0. The driver doesn't actually
use these, or even look at them.
When synthesized with Lynx, this reads 0 indeed. On my device there are
2 instances of socfpga, the other instance doesn't include Lynx and uses
RGMII, so in that case bits 30:28 read 1.
I hope that helps :)
Maxime