From: Russell King (Oracle) <hidden> Date: 2025-10-29 10:18:44
When operating in "SGMII" mode (Cisco SGMII or 2500BASE-X), qcom-ethqos
modifies the MAC control register in its ethqos_configure_sgmii()
function, which is only called from one path:
stmmac_mac_link_up()
+- reads MAC_CTRL_REG
+- masks out priv->hw->link.speed_mask
+- sets bits according to speed (2500, 1000, 100, 10) from priv->hw.link.speed*
+- ethqos_fix_mac_speed()
| +- qcom_ethqos_set_sgmii_loopback(false)
| +- ethqos_update_link_clk(speed)
| `- ethqos_configure(speed)
| `- ethqos_configure_sgmii(speed)
| +- reads MAC_CTRL_REG,
| +- configures PS/FES bits according to speed
| `- writes MAC_CTRL_REG as the last operation
+- sets duplex bit(s)
+- stmmac_mac_flow_ctrl()
+- writes MAC_CTRL_REG if changed from original read
...
As can be seen, the modification of the control register that
stmmac_mac_link_up() overwrites the changes that ethqos_fix_mac_speed()
does to the register. This makes ethqos_configure_sgmii()'s
modification questionable at best.
Analysing the values written, GMAC4 sets the speed bits as:
speed_mask = GMAC_CONFIG_FES | GMAC_CONFIG_PS
speed2500 = GMAC_CONFIG_FES B14=1 B15=0
speed1000 = 0 B14=0 B15=0
speed100 = GMAC_CONFIG_FES | GMAC_CONFIG_PS B14=1 B15=1
speed10 = GMAC_CONFIG_PS B14=0 B15=1
Whereas ethqos_configure_sgmii():
2500: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
1000: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
100: sets ETHQOS_MAC_CTRL_PORT_SEL | B14=1 B15=1
ETHQOS_MAC_CTRL_SPEED_MODE
10: sets ETHQOS_MAC_CTRL_PORT_SEL B14=0 B15=1
clears ETHQOS_MAC_CTRL_SPEED_MODE
Thus, they appear to be doing very similar, with the exception of the
FES bit (bit 14) for 1G and 2.5G speeds.
Given that stmmac_mac_link_up() will write the MAC_CTRL_REG after
ethqos_configure_sgmii(), remove the unnecessary update in the
glue driver's ethqos_configure_sgmii() method, simplifying the code.
Signed-off-by: Russell King (Oracle) <redacted>
---
Ayaan, please can you also test this patch? I believe that this
code is unnecessary as per the analysis in the commit message.
Thanks.
.../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
From: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com> Date: 2025-10-30 09:56:34
On Wed, Oct 29, 2025 at 10:18:36AM +0000, Russell King (Oracle) wrote:
When operating in "SGMII" mode (Cisco SGMII or 2500BASE-X), qcom-ethqos
modifies the MAC control register in its ethqos_configure_sgmii()
function, which is only called from one path:
stmmac_mac_link_up()
+- reads MAC_CTRL_REG
+- masks out priv->hw->link.speed_mask
+- sets bits according to speed (2500, 1000, 100, 10) from priv->hw.link.speed*
+- ethqos_fix_mac_speed()
| +- qcom_ethqos_set_sgmii_loopback(false)
| +- ethqos_update_link_clk(speed)
| `- ethqos_configure(speed)
| `- ethqos_configure_sgmii(speed)
| +- reads MAC_CTRL_REG,
| +- configures PS/FES bits according to speed
| `- writes MAC_CTRL_REG as the last operation
+- sets duplex bit(s)
+- stmmac_mac_flow_ctrl()
+- writes MAC_CTRL_REG if changed from original read
...
As can be seen, the modification of the control register that
stmmac_mac_link_up() overwrites the changes that ethqos_fix_mac_speed()
does to the register. This makes ethqos_configure_sgmii()'s
modification questionable at best.
Analysing the values written, GMAC4 sets the speed bits as:
speed_mask = GMAC_CONFIG_FES | GMAC_CONFIG_PS
speed2500 = GMAC_CONFIG_FES B14=1 B15=0
speed1000 = 0 B14=0 B15=0
speed100 = GMAC_CONFIG_FES | GMAC_CONFIG_PS B14=1 B15=1
speed10 = GMAC_CONFIG_PS B14=0 B15=1
Whereas ethqos_configure_sgmii():
2500: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
1000: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
100: sets ETHQOS_MAC_CTRL_PORT_SEL | B14=1 B15=1
ETHQOS_MAC_CTRL_SPEED_MODE
10: sets ETHQOS_MAC_CTRL_PORT_SEL B14=0 B15=1
clears ETHQOS_MAC_CTRL_SPEED_MODE
Thus, they appear to be doing very similar, with the exception of the
FES bit (bit 14) for 1G and 2.5G speeds.
Makes sense.
Given that stmmac_mac_link_up() will write the MAC_CTRL_REG after
ethqos_configure_sgmii(), remove the unnecessary update in the
glue driver's ethqos_configure_sgmii() method, simplifying the code.
Signed-off-by: Russell King (Oracle) <redacted>
---
Ayaan, please can you also test this patch? I believe that this
code is unnecessary as per the analysis in the commit message.
Thanks.
.../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
Tested on top of net-next on the Qualcomm QCS9100 Ride R3 board and
found no issues, so:
Tested-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
From: Konrad Dybcio <hidden> Date: 2025-10-30 10:01:53
On 10/29/25 11:18 AM, Russell King (Oracle) wrote:
When operating in "SGMII" mode (Cisco SGMII or 2500BASE-X), qcom-ethqos
modifies the MAC control register in its ethqos_configure_sgmii()
function, which is only called from one path:
stmmac_mac_link_up()
+- reads MAC_CTRL_REG
+- masks out priv->hw->link.speed_mask
+- sets bits according to speed (2500, 1000, 100, 10) from priv->hw.link.speed*
+- ethqos_fix_mac_speed()
| +- qcom_ethqos_set_sgmii_loopback(false)
| +- ethqos_update_link_clk(speed)
| `- ethqos_configure(speed)
| `- ethqos_configure_sgmii(speed)
| +- reads MAC_CTRL_REG,
| +- configures PS/FES bits according to speed
| `- writes MAC_CTRL_REG as the last operation
+- sets duplex bit(s)
+- stmmac_mac_flow_ctrl()
+- writes MAC_CTRL_REG if changed from original read
...
As can be seen, the modification of the control register that
stmmac_mac_link_up() overwrites the changes that ethqos_fix_mac_speed()
does to the register. This makes ethqos_configure_sgmii()'s
modification questionable at best.
Analysing the values written, GMAC4 sets the speed bits as:
speed_mask = GMAC_CONFIG_FES | GMAC_CONFIG_PS
speed2500 = GMAC_CONFIG_FES B14=1 B15=0
speed1000 = 0 B14=0 B15=0
speed100 = GMAC_CONFIG_FES | GMAC_CONFIG_PS B14=1 B15=1
speed10 = GMAC_CONFIG_PS B14=0 B15=1
Whereas ethqos_configure_sgmii():
2500: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
1000: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
100: sets ETHQOS_MAC_CTRL_PORT_SEL | B14=1 B15=1
ETHQOS_MAC_CTRL_SPEED_MODE
10: sets ETHQOS_MAC_CTRL_PORT_SEL B14=0 B15=1
clears ETHQOS_MAC_CTRL_SPEED_MODE
Thus, they appear to be doing very similar, with the exception of the
FES bit (bit 14) for 1G and 2.5G speeds.
Without any additional knowledge, the register description says:
2500: B14=1 B15=0
1000: B14=0 B15=0
100: B14=1 B15=1
10: B14=0 B15=1
(so the current state of this driver)
and it indeed seems to match the values set in dwmac4_setup()
Reviewed-by: Konrad Dybcio <redacted>
Konrad
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2025-10-30 10:17:09
On Thu, Oct 30, 2025 at 11:01:48AM +0100, Konrad Dybcio wrote:
On 10/29/25 11:18 AM, Russell King (Oracle) wrote:
quoted
Whereas ethqos_configure_sgmii():
2500: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
1000: clears ETHQOS_MAC_CTRL_PORT_SEL B14=X B15=0
100: sets ETHQOS_MAC_CTRL_PORT_SEL | B14=1 B15=1
ETHQOS_MAC_CTRL_SPEED_MODE
10: sets ETHQOS_MAC_CTRL_PORT_SEL B14=0 B15=1
clears ETHQOS_MAC_CTRL_SPEED_MODE
Thus, they appear to be doing very similar, with the exception of the
FES bit (bit 14) for 1G and 2.5G speeds.
Without any additional knowledge, the register description says:
2500: B14=1 B15=0
1000: B14=0 B15=0
100: B14=1 B15=1
10: B14=0 B15=1
(so the current state of this driver)
and it indeed seems to match the values set in dwmac4_setup()
Reviewed-by: Konrad Dybcio <redacted>
Thanks Konrad. I've included an extract from your reply in the commit
description as well, thanks for referring back to the documentation.
Note that B14 is not explicitly configured by ethqos_configure_sgmii()
for 1G and 2.5G speeds which is a harmless bug in this glue driver,
and an additional reason to get rid of this code!
--
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)" <linux@armlinux.org.uk> Date: 2025-10-30 12:17:31
Konrad, Ayaan,
Can you shed any light on the manipulation of the RGMII_IO_MACRO_CONFIG
and RGMII_IO_MACRO_CONFIG2 registers in ethqos_configure_sgmii()?
Specifically:
- why would RGMII_CONFIG2_RGMII_CLK_SEL_CFG be set for 2.5G and 1G
speeds, but never be cleared for any other speed?
- why is RGMII_CONFIG_SGMII_CLK_DVDR set to SGMII_10M_RX_CLK_DVDR
for 10M, but never set to any other value for other speeds?
To me, this code looks very suspicious.
If you have time, please test with a connection capable of 1000BASE-T,
100BASE-TX and 10BASE-T, modifying the advertisement to make it
negotiate each of these, and checking that packet transfer is still
possible.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
From: Konrad Dybcio <hidden> Date: 2025-10-30 13:08:47
On 10/30/25 1:17 PM, Russell King (Oracle) wrote:
Konrad, Ayaan,
Can you shed any light on the manipulation of the RGMII_IO_MACRO_CONFIG
and RGMII_IO_MACRO_CONFIG2 registers in ethqos_configure_sgmii()?
Specifically:
- why would RGMII_CONFIG2_RGMII_CLK_SEL_CFG be set for 2.5G and 1G
speeds, but never be cleared for any other speed?
BIT(16) - "enable to transmit delayed clock in RGMII 100/10 ID Mode"
- why is RGMII_CONFIG_SGMII_CLK_DVDR set to SGMII_10M_RX_CLK_DVDR
for 10M, but never set to any other value for other speeds?
[18:10] - In short, it configures a divider. The expected value is 0x13
for 10 Mbps / RMII mode
which seems to have been problematic given:
https://lore.kernel.org/all/20231212092208.22393-1-quic_snehshah@quicinc.com/
But it didn't say what hardware had this issue.. whether it concerns a
specific SoC or all of them..
A programming guide mentions the new 0x31 value for 10 Mbps in a
SoC-common paragraph so I suppose it's indeed better-er.. Perhaps issues
could arise if you switch back to a faster mode?
To me, this code looks very suspicious.
If you have time, please test with a connection capable of 1000BASE-T,
100BASE-TX and 10BASE-T, modifying the advertisement to make it
negotiate each of these, and checking that packet transfer is still
possible.
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2025-10-30 14:05:36
On Thu, Oct 30, 2025 at 02:08:41PM +0100, Konrad Dybcio wrote:
On 10/30/25 1:17 PM, Russell King (Oracle) wrote:
quoted
Konrad, Ayaan,
Can you shed any light on the manipulation of the RGMII_IO_MACRO_CONFIG
and RGMII_IO_MACRO_CONFIG2 registers in ethqos_configure_sgmii()?
Specifically:
- why would RGMII_CONFIG2_RGMII_CLK_SEL_CFG be set for 2.5G and 1G
speeds, but never be cleared for any other speed?
BIT(16) - "enable to transmit delayed clock in RGMII 100/10 ID Mode"
I guess that means that changing this bit is not relevant for the SGMII
path, and thus can be removed:
switch (speed) {
case SPEED_2500:
- rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_IO_MACRO_CONFIG2);
ethqos_set_serdes_speed(ethqos, SPEED_2500);
ethqos_pcs_set_inband(priv, false);
break;
case SPEED_1000:
- rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_IO_MACRO_CONFIG2);
ethqos_set_serdes_speed(ethqos, SPEED_1000);
ethqos_pcs_set_inband(priv, true);
quoted
- why is RGMII_CONFIG_SGMII_CLK_DVDR set to SGMII_10M_RX_CLK_DVDR
for 10M, but never set to any other value for other speeds?
[18:10] - In short, it configures a divider. The expected value is 0x13
for 10 Mbps / RMII mode
This gets confusing. Is the "/" meaning "10Mbps in RMII mode" or "10Mbps
or RMII mode".
which seems to have been problematic given:
https://lore.kernel.org/all/20231212092208.22393-1-quic_snehshah@quicinc.com/
But it didn't say what hardware had this issue.. whether it concerns a
specific SoC or all of them..
A programming guide mentions the new 0x31 value for 10 Mbps in a
SoC-common paragraph so I suppose it's indeed better-er.. Perhaps issues
could arise if you switch back to a faster mode?
Could the 0x13 be a typo? Its suspicious that the two values are 0x13
vs 0x31. 0x13 = 19 vs 0x31 = 49. 0x31 makes more sense than 19.
The platform glue is required to supply clk_rx_i to the dwmac's MAC
receive path, deriving it from the 125MHz SGMII rx link clock divided
by 1, 5 or 50. Normally, this would be done by hardware signals output
from the dwmac.
This suggests that the value programmed is one less than the actual
divisor.
There's two possibilities why this value needs to be programmed:
1. the hardware doesn't divide the SGMII rx link clock according to
the hardware signals output from the dwmac, and needs the divisor to
be manually programmed. This would require the divisor to also be
programmed to 4 for 100M (but the driver doesn't do this.)
2. the hardware selects the clk_rx_i depending on the hardware
signals, and while 1G and 100M use a fixed divisor of 1 and 5, the
10M divisor needs to be manually programmed.
Any ideas what's really going on here?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
From: Konrad Dybcio <hidden> Date: 2025-11-07 11:34:40
On 10/30/25 3:05 PM, Russell King (Oracle) wrote:
On Thu, Oct 30, 2025 at 02:08:41PM +0100, Konrad Dybcio wrote:
quoted
On 10/30/25 1:17 PM, Russell King (Oracle) wrote:
quoted
Konrad, Ayaan,
Can you shed any light on the manipulation of the RGMII_IO_MACRO_CONFIG
and RGMII_IO_MACRO_CONFIG2 registers in ethqos_configure_sgmii()?
Specifically:
- why would RGMII_CONFIG2_RGMII_CLK_SEL_CFG be set for 2.5G and 1G
speeds, but never be cleared for any other speed?
BIT(16) - "enable to transmit delayed clock in RGMII 100/10 ID Mode"
I guess that means that changing this bit is not relevant for the SGMII
path, and thus can be removed:
switch (speed) {
case SPEED_2500:
- rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_IO_MACRO_CONFIG2);
ethqos_set_serdes_speed(ethqos, SPEED_2500);
ethqos_pcs_set_inband(priv, false);
break;
case SPEED_1000:
- rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
- RGMII_IO_MACRO_CONFIG2);
ethqos_set_serdes_speed(ethqos, SPEED_1000);
ethqos_pcs_set_inband(priv, true);
quoted
quoted
- why is RGMII_CONFIG_SGMII_CLK_DVDR set to SGMII_10M_RX_CLK_DVDR
for 10M, but never set to any other value for other speeds?
[18:10] - In short, it configures a divider. The expected value is 0x13
for 10 Mbps / RMII mode
This gets confusing. Is the "/" meaning "10Mbps in RMII mode" or "10Mbps
or RMII mode".
quoted
which seems to have been problematic given:
https://lore.kernel.org/all/20231212092208.22393-1-quic_snehshah@quicinc.com/
But it didn't say what hardware had this issue.. whether it concerns a
specific SoC or all of them..
A programming guide mentions the new 0x31 value for 10 Mbps in a
SoC-common paragraph so I suppose it's indeed better-er.. Perhaps issues
could arise if you switch back to a faster mode?
Could the 0x13 be a typo? Its suspicious that the two values are 0x13
vs 0x31. 0x13 = 19 vs 0x31 = 49. 0x31 makes more sense than 19.
The platform glue is required to supply clk_rx_i to the dwmac's MAC
receive path, deriving it from the 125MHz SGMII rx link clock divided
by 1, 5 or 50. Normally, this would be done by hardware signals output
from the dwmac.
This suggests that the value programmed is one less than the actual
divisor.
There's two possibilities why this value needs to be programmed:
1. the hardware doesn't divide the SGMII rx link clock according to
the hardware signals output from the dwmac, and needs the divisor to
be manually programmed. This would require the divisor to also be
programmed to 4 for 100M (but the driver doesn't do this.)
2. the hardware selects the clk_rx_i depending on the hardware
signals, and while 1G and 100M use a fixed divisor of 1 and 5, the
10M divisor needs to be manually programmed.
Any ideas what's really going on here?
The computer says:
RGMII ID mode - speed == 10 ? 0x13 : dontcare?
RMII Bypass ID mode - - speed == (10 || 100) ? 0x13 : dontcare?
(*the 100 above says "default" but that's again 0x13)
RMII mode (100 Mbps) - default (0x13)
RMII mode (10 MBps) - 0x13
SGMII mode - speed == 10 ? 0x31 : 0x13
Make of that what you will, I would *guess* there may be something like
2. going on
Konrad