[PATCH net] net: stmmac: Fix false "invalid port speed" warning

[PATCH net] net: stmmac: Fix false "invalid port speed" warning

[Serge Semin]

If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
instance and there is no "snps,ps-speed" property specified (or the
plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
next warning will be printed to the system log:

> [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed

By the original intention the "snps,ps-speed" property was supposed to be
utilized on the platforms with the MAC2MAC link setup to fix the link
speed with the specified value. But since it's possible to have a device
with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
the property is actually optional (which is also confirmed by the DW MAC
DT-bindings). Thus it's absolutely normal to have the
plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
that there is no need in the MAC-speed fixing and the denoted warning is
false.

Fix the warning by permitting the plat_stmmacenet_data::mac_port_sel_speed
field to have the zero value in case if the internal PCS is available.

Fixes: 02e57b9d7c8c ("drivers: net: stmmac: add port selection programming")
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---

Note this fix will get to be mainly actual after the next patch is merged
in:
https://lore.kernel.org/netdev/E1sauuS-000tvz-6E@rmk-PC.armlinux.org.uk/

Cc: Russell King (Oracle) <linux@armlinux.org.uk>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f3a1b179aaea..fb63df1b99c0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3422,7 +3422,7 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 		if ((speed == SPEED_10) || (speed == SPEED_100) ||
 		    (speed == SPEED_1000)) {
 			priv->hw->ps = speed;
-		} else {
+		} else if (speed) {
 			dev_warn(priv->device, "invalid port speed\n");
 			priv->hw->ps = 0;
 		}
-- 
2.43.0

Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

[Andrew Halaney]

On Fri, Aug 09, 2024 at 10:21:39PM GMT, Serge Semin wrote:
> If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
> instance and there is no "snps,ps-speed" property specified (or the
> plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
> next warning will be printed to the system log:
> 
> > [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed
> 
> By the original intention the "snps,ps-speed" property was supposed to be
> utilized on the platforms with the MAC2MAC link setup to fix the link
> speed with the specified value. But since it's possible to have a device
> with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
> the property is actually optional (which is also confirmed by the DW MAC
> DT-bindings). Thus it's absolutely normal to have the
> plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
> that there is no need in the MAC-speed fixing and the denoted warning is
> false.

Can you help me understand what snps,ps-speed actually does? Its turned
into a bool and pushed down into srgmi_ral right now:

	/**
	 * dwmac_ctrl_ane - To program the AN Control Register.
	 * @ioaddr: IO registers pointer
	 * @reg: Base address of the AN Control Register.
	 * @ane: to enable the auto-negotiation
	 * @srgmi_ral: to manage MAC-2-MAC SGMII connections.
	 * @loopback: to cause the PHY to loopback tx data into rx path.
	 * Description: this is the main function to configure the AN control register
	 * and init the ANE, select loopback (usually for debugging purpose) and
	 * configure SGMII RAL.
	 */
	static inline void dwmac_ctrl_ane(void __iomem *ioaddr, u32 reg, bool ane,
					  bool srgmi_ral, bool loopback)
	{
		u32 value = readl(ioaddr + GMAC_AN_CTRL(reg));

		/* Enable and restart the Auto-Negotiation */
		if (ane)
			value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
		else
			value &= ~GMAC_AN_CTRL_ANE;

		/* In case of MAC-2-MAC connection, block is configured to operate
		 * according to MAC conf register.
		 */
		if (srgmi_ral)
			value |= GMAC_AN_CTRL_SGMRAL;

		if (loopback)
			value |= GMAC_AN_CTRL_ELE;

		writel(value, ioaddr + GMAC_AN_CTRL(reg));
	}

What is that bit doing exactly? The only user upstream I see is
sa8775p-ride variants, but they're all using a phy right now (not
fixed-link aka MAC2MAC iiuc)... so I should probably remove it from
there too.

I feel like that property really (if I'm following right) should be just
taken from a proper fixed-link devicetree description? i.e. we already
specify a speed in that case. Maybe this predates that (or reinvents it)
and should be marked as deprecated in the dt-bindings.

But I'm struggling to understand what the bit is really doing based
on the original commit that added it, so I don't know if my logic is
solid. i.e., what's different in the phy case vs mac2mac with this
particular bit?

Thanks for your never ending patience about my questions wrt the
hardware and this driver.

- Andrew

Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

[Serge Semin]

Hi Andrew

On Fri, Aug 09, 2024 at 03:41:04PM -0500, Andrew Halaney wrote:
> On Fri, Aug 09, 2024 at 10:21:39PM GMT, Serge Semin wrote:
> > If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
> > instance and there is no "snps,ps-speed" property specified (or the
> > plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
> > next warning will be printed to the system log:
> > 
> > > [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed
> > 
> > By the original intention the "snps,ps-speed" property was supposed to be
> > utilized on the platforms with the MAC2MAC link setup to fix the link
> > speed with the specified value. But since it's possible to have a device
> > with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
> > the property is actually optional (which is also confirmed by the DW MAC
> > DT-bindings). Thus it's absolutely normal to have the
> > plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
> > that there is no need in the MAC-speed fixing and the denoted warning is
> > false.
> 

> Can you help me understand what snps,ps-speed actually does? Its turned
> into a bool and pushed down into srgmi_ral right now:
> 
> 	/**
> 	 * dwmac_ctrl_ane - To program the AN Control Register.
> 	 * @ioaddr: IO registers pointer
> 	 * @reg: Base address of the AN Control Register.
> 	 * @ane: to enable the auto-negotiation
> 	 * @srgmi_ral: to manage MAC-2-MAC SGMII connections.
> 	 * @loopback: to cause the PHY to loopback tx data into rx path.
> 	 * Description: this is the main function to configure the AN control register
> 	 * and init the ANE, select loopback (usually for debugging purpose) and
> 	 * configure SGMII RAL.
> 	 */
> 	static inline void dwmac_ctrl_ane(void __iomem *ioaddr, u32 reg, bool ane,
> 					  bool srgmi_ral, bool loopback)
> 	{
> 		u32 value = readl(ioaddr + GMAC_AN_CTRL(reg));
> 
> 		/* Enable and restart the Auto-Negotiation */
> 		if (ane)
> 			value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
> 		else
> 			value &= ~GMAC_AN_CTRL_ANE;
> 
> 		/* In case of MAC-2-MAC connection, block is configured to operate
> 		 * according to MAC conf register.
> 		 */
> 		if (srgmi_ral)
> 			value |= GMAC_AN_CTRL_SGMRAL;
> 
> 		if (loopback)
> 			value |= GMAC_AN_CTRL_ELE;
> 
> 		writel(value, ioaddr + GMAC_AN_CTRL(reg));
> 	}
> 
> 

In addition to the method above there are three more places related to
the SGMRAL flag (SGMII Rate Adaptation Layer Control flag) setting up:

2. drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
stmmac_probe_config_dt():
  of_property_read_u32(np, "snps,ps-speed", &plat->mac_port_sel_speed);

Description: Retrieve the fixed speed of the MAC-2-MAC SGMII
connection from DT-file.

3. drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
stmmac_hw_setup():
   if (priv->hw->pcs) {
	int speed = priv->plat->mac_port_sel_speed;

	if ((speed == SPEED_10) || (speed == SPEED_100) ||
	    (speed == SPEED_1000)) {
		priv->hw->ps = speed;
	} else {
		dev_warn(priv->device, "invalid port speed\n");
		priv->hw->ps = 0;
	}
   }

Description: Parse the speed specified via the "snps,ps-speed"
property to make sure it's of one of the permitted values. Note it's
executed only if the priv->hw->pcs flag is set which due to the
current stmmac_check_pcs_mode() implementation is only possible if
the DW GMAC/QoS Eth supports the SGMII PHY interface.

4. drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
   drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
dwmac*_core_init():
   if (hw->ps) {
	value |= GMAC_CONTROL_TE;

	value &= ~hw->link.speed_mask;
	switch (hw->ps) {
	case SPEED_1000:
		value |= hw->link.speed1000;
		break;
	case SPEED_100:
		value |= hw->link.speed100;
		break;
	case SPEED_10:
		value |= hw->link.speed10;
		break;
	}
   }

Description: Pre-initialize the MAC speed with the value retrieved from
the "snps,ps-speed" property. The speed is then fixed in the SGMII
Rate adaptation Layer by setting up the SGMIRAL flag performed in the
dwmac_ctrl_ane() method you cited. 
Note the same register fields (MAC_CONTROL.{PS,FES}) are touched in
the stmmac_mac_link_up() method in the framework of the MAC-link up
procedure (phylink_mac_ops::mac_link_up). But AFAICS the procedure is
done too late, and after the SGMIRAL flag is set in the framework of
the stmmac_open()->stmmac_hw_setup()->dwmac_ctrl_ane() call. That's
probably why the MAC2MAC mode developer needed the speed being
pre-initialized earlier in the dwmac*_core_init() functions.


To sum up the MAC-2-MAC mode implementation can be described as
follows:
1. The platform firmware defines the fixed link speed by means of the
"snps,ps-speed" property (or by pre-initializing the
plat_stmmacenet_data::mac_port_sel_speed field in the glue driver).
2. If the SGMII PHY interface is supported by the DW GMAC/QoS Eth
controller, the speed will be fixed right at the network interface
open() procedure execution by subsequently calling the
stmmac_core_init() and stmmac_pcs_ctrl_ane() methods.

Please note I can't immediately tell of how that functionality gets to
live with the phylink-based link setup procedure, in the framework of
which the link speed is also setup in stmmac_mac_link_up(). Alas I
don't have any SGMII-based device to test it out. But I guess that it
lives well until the pre-initialized in the "snps,ps-speed"
speed matches to the speed negotiated between the connected PHYs. If
the speeds don't match, then I can't tell for sure what would happen
without a hardware at hands.

> What is that bit doing exactly?

Here is what the DW MAC databook says about the bit:

"SGMII RAL Control

When set, this bit forces the SGMII RAL block to operate in the speed
configured in the MAC Configuration register's Speed and Port Select
bits. This is useful when the SGMII interface is used in a direct
MAC-MAC connection (without a PHY) and any MAC must reconfigure the
speed.

When reset, the SGMII RAL block operates according to the link speed
status received on the SGMII (from the PHY)."

Shortly speaking the flag enables the SGMII Rate adaptation layer
(SGMII RAL) to stop selecting the speed based on the info retrieved
from the PHY and instead to fix the speed with the value specified in
the MAC_CTRL_REG register.

> The only user upstream I see is
> sa8775p-ride variants, but they're all using a phy right now (not
> fixed-link aka MAC2MAC iiuc)... so I should probably remove it from
> there too.

Right, there is only a single user of that property in the kernel. But
I don't know what was the actual reason of having the "snps,ps-speed"
specified in that case. From one side it seems contradicting to the
original MAC-2-MAC semantics since the phy-handle property is also
specified in the ethernet controller DT-node, but from another side it
might have been caused by some HW-related problem so there was a need
to fix the speed. It would be better to ask Bartosz Golaszewski about
that since it was him who submitted the patch adding the sa8775p-ride
Ethernet DT-nodes.

> 

> I feel like that property really (if I'm following right) should be just
> taken from a proper fixed-link devicetree description? i.e. we already
> specify a speed in that case. Maybe this predates that (or reinvents it)
> and should be marked as deprecated in the dt-bindings.

Exactly my thoughts of the way it should have been implemented in the
first place. The fixed-linked semantics was exactly suitable for the
MAC-2-MAC HW-setup since both of them implies the link-speed being
fixed to a single value with no PHY-node required.

Note if the respective code conversion is planned to be done then I
guess it would be better to do after the Russell' series
https://lore.kernel.org/netdev/ZrCoQZKo74zvKMhT@shell.armlinux.org.uk/
is merged in, since the changes are related.

> 
> But I'm struggling to understand what the bit is really doing based
> on the original commit that added it, so I don't know if my logic is
> solid. i.e., what's different in the phy case vs mac2mac with this
> particular bit?

Please see my notes above for more details about the SGMRAL bit
semantics. Shortly speaking The difference is basically in the way the
link-speed is established:
- If the SGMRAL bit is set the MAC' SGMII RAL will operate with the
  speed specified in the Speed and Port Select bits of the
  MAC_CTRL_REG register.
- If the SGMRAL bit is reset the SGMII RAL will operate according to
  the link speed status received on SGMII (from the PHY).

> 
> Thanks for your never ending patience about my questions wrt the
> hardware and this driver.

Always welcome. I'm glad to be helpful, especially after I myself
have spent endless time deciphering down the STMMAC' driver guts and
studying the hardware design of the Synopsys Ethernet MACs.

-Serge(y)

> 
> - Andrew
> 

Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

[Andrew Halaney]

On Mon, Aug 12, 2024 at 07:48:34PM GMT, Serge Semin wrote:
> Hi Andrew
> 
> On Fri, Aug 09, 2024 at 03:41:04PM -0500, Andrew Halaney wrote:
> > On Fri, Aug 09, 2024 at 10:21:39PM GMT, Serge Semin wrote:
> > > If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
> > > instance and there is no "snps,ps-speed" property specified (or the
> > > plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
> > > next warning will be printed to the system log:
> > > 
> > > > [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed
> > > 
> > > By the original intention the "snps,ps-speed" property was supposed to be
> > > utilized on the platforms with the MAC2MAC link setup to fix the link
> > > speed with the specified value. But since it's possible to have a device
> > > with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
> > > the property is actually optional (which is also confirmed by the DW MAC
> > > DT-bindings). Thus it's absolutely normal to have the
> > > plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
> > > that there is no need in the MAC-speed fixing and the denoted warning is
> > > false.
> > 
> 
> > Can you help me understand what snps,ps-speed actually does? Its turned
> > into a bool and pushed down into srgmi_ral right now:
> > 
> > 	/**
> > 	 * dwmac_ctrl_ane - To program the AN Control Register.
> > 	 * @ioaddr: IO registers pointer
> > 	 * @reg: Base address of the AN Control Register.
> > 	 * @ane: to enable the auto-negotiation
> > 	 * @srgmi_ral: to manage MAC-2-MAC SGMII connections.
> > 	 * @loopback: to cause the PHY to loopback tx data into rx path.
> > 	 * Description: this is the main function to configure the AN control register
> > 	 * and init the ANE, select loopback (usually for debugging purpose) and
> > 	 * configure SGMII RAL.
> > 	 */
> > 	static inline void dwmac_ctrl_ane(void __iomem *ioaddr, u32 reg, bool ane,
> > 					  bool srgmi_ral, bool loopback)
> > 	{
> > 		u32 value = readl(ioaddr + GMAC_AN_CTRL(reg));
> > 
> > 		/* Enable and restart the Auto-Negotiation */
> > 		if (ane)
> > 			value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
> > 		else
> > 			value &= ~GMAC_AN_CTRL_ANE;
> > 
> > 		/* In case of MAC-2-MAC connection, block is configured to operate
> > 		 * according to MAC conf register.
> > 		 */
> > 		if (srgmi_ral)
> > 			value |= GMAC_AN_CTRL_SGMRAL;
> > 
> > 		if (loopback)
> > 			value |= GMAC_AN_CTRL_ELE;
> > 
> > 		writel(value, ioaddr + GMAC_AN_CTRL(reg));
> > 	}
> > 
> > 
> 
> In addition to the method above there are three more places related to
> the SGMRAL flag (SGMII Rate Adaptation Layer Control flag) setting up:
> 
> 2. drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> stmmac_probe_config_dt():
>   of_property_read_u32(np, "snps,ps-speed", &plat->mac_port_sel_speed);
> 
> Description: Retrieve the fixed speed of the MAC-2-MAC SGMII
> connection from DT-file.
> 
> 3. drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
> stmmac_hw_setup():
>    if (priv->hw->pcs) {
> 	int speed = priv->plat->mac_port_sel_speed;
> 
> 	if ((speed == SPEED_10) || (speed == SPEED_100) ||
> 	    (speed == SPEED_1000)) {
> 		priv->hw->ps = speed;
> 	} else {
> 		dev_warn(priv->device, "invalid port speed\n");
> 		priv->hw->ps = 0;
> 	}
>    }
> 
> Description: Parse the speed specified via the "snps,ps-speed"
> property to make sure it's of one of the permitted values. Note it's
> executed only if the priv->hw->pcs flag is set which due to the
> current stmmac_check_pcs_mode() implementation is only possible if
> the DW GMAC/QoS Eth supports the SGMII PHY interface.
> 
> 4. drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
>    drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
>    drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
> dwmac*_core_init():
>    if (hw->ps) {
> 	value |= GMAC_CONTROL_TE;

Might have missed it today when looking at this, but what's the
GMAC_CONTROL_TE bit about? Otherwise, I agree that mac_link_up()
basically does similar work at different times in the driver.

> 
> 	value &= ~hw->link.speed_mask;
> 	switch (hw->ps) {
> 	case SPEED_1000:
> 		value |= hw->link.speed1000;
> 		break;
> 	case SPEED_100:
> 		value |= hw->link.speed100;
> 		break;
> 	case SPEED_10:
> 		value |= hw->link.speed10;
> 		break;
> 	}
>    }
> 
> Description: Pre-initialize the MAC speed with the value retrieved from
> the "snps,ps-speed" property. The speed is then fixed in the SGMII
> Rate adaptation Layer by setting up the SGMIRAL flag performed in the
> dwmac_ctrl_ane() method you cited. 
> Note the same register fields (MAC_CONTROL.{PS,FES}) are touched in
> the stmmac_mac_link_up() method in the framework of the MAC-link up
> procedure (phylink_mac_ops::mac_link_up). But AFAICS the procedure is
> done too late, and after the SGMIRAL flag is set in the framework of
> the stmmac_open()->stmmac_hw_setup()->dwmac_ctrl_ane() call. That's
> probably why the MAC2MAC mode developer needed the speed being
> pre-initialized earlier in the dwmac*_core_init() functions.
> 
> 
> To sum up the MAC-2-MAC mode implementation can be described as
> follows:
> 1. The platform firmware defines the fixed link speed by means of the
> "snps,ps-speed" property (or by pre-initializing the
> plat_stmmacenet_data::mac_port_sel_speed field in the glue driver).
> 2. If the SGMII PHY interface is supported by the DW GMAC/QoS Eth
> controller, the speed will be fixed right at the network interface
> open() procedure execution by subsequently calling the
> stmmac_core_init() and stmmac_pcs_ctrl_ane() methods.
> 
> Please note I can't immediately tell of how that functionality gets to
> live with the phylink-based link setup procedure, in the framework of
> which the link speed is also setup in stmmac_mac_link_up(). Alas I
> don't have any SGMII-based device to test it out. But I guess that it
> lives well until the pre-initialized in the "snps,ps-speed"
> speed matches to the speed negotiated between the connected PHYs. If
> the speeds don't match, then I can't tell for sure what would happen
> without a hardware at hands.
> 
> > What is that bit doing exactly?
> 
> Here is what the DW MAC databook says about the bit:
> 
> "SGMII RAL Control
> 
> When set, this bit forces the SGMII RAL block to operate in the speed
> configured in the MAC Configuration register's Speed and Port Select
> bits. This is useful when the SGMII interface is used in a direct
> MAC-MAC connection (without a PHY) and any MAC must reconfigure the
> speed.
> 
> When reset, the SGMII RAL block operates according to the link speed
> status received on the SGMII (from the PHY)."
> 
> Shortly speaking the flag enables the SGMII Rate adaptation layer
> (SGMII RAL) to stop selecting the speed based on the info retrieved
> from the PHY and instead to fix the speed with the value specified in
> the MAC_CTRL_REG register.
> 
> > The only user upstream I see is
> > sa8775p-ride variants, but they're all using a phy right now (not
> > fixed-link aka MAC2MAC iiuc)... so I should probably remove it from
> > there too.
> 
> Right, there is only a single user of that property in the kernel. But
> I don't know what was the actual reason of having the "snps,ps-speed"
> specified in that case. From one side it seems contradicting to the
> original MAC-2-MAC semantics since the phy-handle property is also
> specified in the ethernet controller DT-node, but from another side it
> might have been caused by some HW-related problem so there was a need
> to fix the speed. It would be better to ask Bartosz Golaszewski about
> that since it was him who submitted the patch adding the sa8775p-ride
> Ethernet DT-nodes.
> 
> > 
> 
> > I feel like that property really (if I'm following right) should be just
> > taken from a proper fixed-link devicetree description? i.e. we already
> > specify a speed in that case. Maybe this predates that (or reinvents it)
> > and should be marked as deprecated in the dt-bindings.
> 
> Exactly my thoughts of the way it should have been implemented in the
> first place. The fixed-linked semantics was exactly suitable for the
> MAC-2-MAC HW-setup since both of them implies the link-speed being
> fixed to a single value with no PHY-node required.
> 
> Note if the respective code conversion is planned to be done then I
> guess it would be better to do after the Russell' series
> https://lore.kernel.org/netdev/ZrCoQZKo74zvKMhT@shell.armlinux.org.uk/
> is merged in, since the changes are related.
> 
> > 
> > But I'm struggling to understand what the bit is really doing based
> > on the original commit that added it, so I don't know if my logic is
> > solid. i.e., what's different in the phy case vs mac2mac with this
> > particular bit?
> 
> Please see my notes above for more details about the SGMRAL bit
> semantics. Shortly speaking The difference is basically in the way the
> link-speed is established:
> - If the SGMRAL bit is set the MAC' SGMII RAL will operate with the
>   speed specified in the Speed and Port Select bits of the
>   MAC_CTRL_REG register.
> - If the SGMRAL bit is reset the SGMII RAL will operate according to
>   the link speed status received on SGMII (from the PHY).

I think with all of that in mind, long term it might make sense to:

    1. Remove ps-speed handling
    2. Program GMAC_AN_CTRL in mac_link_up() as specified in the
       kerneldoc (i.e. iiuc only in !phylink_autoneg_inband(@mode) case)
    3. Program SGMRAL bit in pcs_config as specified in the kernel doc
       (iiuc mode == PHYLINK_PCS_NEG_OUTBAND)

I'm probably misunderstanding the phylink interactions while reading
on the plane today (and hence may have misunderstood what callbacks
should configure that and when), but in general sounds like the forward
looking step would be to build on top of Russell's PCS changes and rip out the
ps-speed stuff, relying on phylink to indicate what the speed / port
select stuff needs to be in the PCS and MAC in the various configs
possible (i.e. stop treating fixed-link special here).

Maybe Qualcomm can help with testing the SGMII fixed-link setup, as I
think they're the only folks I know of with a board with that at least.
I have acess to a sa8775p-ride, but that has SGMII with a phy so its not
going to cover all testing.

FYI.. I'm traveling the next two weeks, sorry in advance if I disappear
for a bit :)

Thanks,
Andrew

Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

[Serge Semin]

On Tue, Aug 13, 2024 at 08:05:36PM -0500, Andrew Halaney wrote:
> On Mon, Aug 12, 2024 at 07:48:34PM GMT, Serge Semin wrote:
> > Hi Andrew
> > 
> > On Fri, Aug 09, 2024 at 03:41:04PM -0500, Andrew Halaney wrote:
> > > On Fri, Aug 09, 2024 at 10:21:39PM GMT, Serge Semin wrote:
> > > > If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
> > > > instance and there is no "snps,ps-speed" property specified (or the
> > > > plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
> > > > next warning will be printed to the system log:
> > > > 
> > > > > [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed
> > > > 
> > > > By the original intention the "snps,ps-speed" property was supposed to be
> > > > utilized on the platforms with the MAC2MAC link setup to fix the link
> > > > speed with the specified value. But since it's possible to have a device
> > > > with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
> > > > the property is actually optional (which is also confirmed by the DW MAC
> > > > DT-bindings). Thus it's absolutely normal to have the
> > > > plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
> > > > that there is no need in the MAC-speed fixing and the denoted warning is
> > > > false.
> > > 
> > 
> > > Can you help me understand what snps,ps-speed actually does? Its turned
> > > into a bool and pushed down into srgmi_ral right now:
> > > 
> > > 	/**
> > > 	 * dwmac_ctrl_ane - To program the AN Control Register.
> > > 	 * @ioaddr: IO registers pointer
> > > 	 * @reg: Base address of the AN Control Register.
> > > 	 * @ane: to enable the auto-negotiation
> > > 	 * @srgmi_ral: to manage MAC-2-MAC SGMII connections.
> > > 	 * @loopback: to cause the PHY to loopback tx data into rx path.
> > > 	 * Description: this is the main function to configure the AN control register
> > > 	 * and init the ANE, select loopback (usually for debugging purpose) and
> > > 	 * configure SGMII RAL.
> > > 	 */
> > > 	static inline void dwmac_ctrl_ane(void __iomem *ioaddr, u32 reg, bool ane,
> > > 					  bool srgmi_ral, bool loopback)
> > > 	{
> > > 		u32 value = readl(ioaddr + GMAC_AN_CTRL(reg));
> > > 
> > > 		/* Enable and restart the Auto-Negotiation */
> > > 		if (ane)
> > > 			value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
> > > 		else
> > > 			value &= ~GMAC_AN_CTRL_ANE;
> > > 
> > > 		/* In case of MAC-2-MAC connection, block is configured to operate
> > > 		 * according to MAC conf register.
> > > 		 */
> > > 		if (srgmi_ral)
> > > 			value |= GMAC_AN_CTRL_SGMRAL;
> > > 
> > > 		if (loopback)
> > > 			value |= GMAC_AN_CTRL_ELE;
> > > 
> > > 		writel(value, ioaddr + GMAC_AN_CTRL(reg));
> > > 	}
> > > 
> > > 
> > 
> > In addition to the method above there are three more places related to
> > the SGMRAL flag (SGMII Rate Adaptation Layer Control flag) setting up:
> > 
> > 2. drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > stmmac_probe_config_dt():
> >   of_property_read_u32(np, "snps,ps-speed", &plat->mac_port_sel_speed);
> > 
> > Description: Retrieve the fixed speed of the MAC-2-MAC SGMII
> > connection from DT-file.
> > 
> > 3. drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
> > stmmac_hw_setup():
> >    if (priv->hw->pcs) {
> > 	int speed = priv->plat->mac_port_sel_speed;
> > 
> > 	if ((speed == SPEED_10) || (speed == SPEED_100) ||
> > 	    (speed == SPEED_1000)) {
> > 		priv->hw->ps = speed;
> > 	} else {
> > 		dev_warn(priv->device, "invalid port speed\n");
> > 		priv->hw->ps = 0;
> > 	}
> >    }
> > 
> > Description: Parse the speed specified via the "snps,ps-speed"
> > property to make sure it's of one of the permitted values. Note it's
> > executed only if the priv->hw->pcs flag is set which due to the
> > current stmmac_check_pcs_mode() implementation is only possible if
> > the DW GMAC/QoS Eth supports the SGMII PHY interface.
> > 
> > 4. drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
> >    drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
> >    drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
> > dwmac*_core_init():
> >    if (hw->ps) {
> > 	value |= GMAC_CONTROL_TE;
> 

> Might have missed it today when looking at this, but what's the
> GMAC_CONTROL_TE bit about? Otherwise, I agree that mac_link_up()
> basically does similar work at different times in the driver.

This bit enables the DW GMAC Transmission engine: "When this bit is
set, the transmit state machine of the MAC is enabled for transmission
on the GMII or MII." So by setting that flag the driver actually
_actiavates_ the MAC to transmission the data to the link. I have very
much doubt whether it's legitimate to do that at this stage.

Honestly this is the most mysterious part for me in the MAC2MAC
implementation, because I failed to find any reason of why it was
necessary to activate the transmissions so early before any of the
following up configs are done. This is normally done at the tail of
the MAC-link up initialization procedure (see the stmmac_mac_set()
method calls). So there is a good chance that the code author confused
the GMAC_CONTROL.TE flag with GMAC_CONTROL.TC flag. The later flag is
actually responsible for the GMAC duplex, speed and link up/down
setups transmission to the RGMII/SGMII/TBI/RTBI link-partner (PHY or
another MAC in the MAC2MAC config). So IMO the GMAC_CONTROL.TC flag
should have be set here instead. But I don't have a device to try it
out.

> 
> > 
> > 	value &= ~hw->link.speed_mask;
> > 	switch (hw->ps) {
> > 	case SPEED_1000:
> > 		value |= hw->link.speed1000;
> > 		break;
> > 	case SPEED_100:
> > 		value |= hw->link.speed100;
> > 		break;
> > 	case SPEED_10:
> > 		value |= hw->link.speed10;
> > 		break;
> > 	}
> >    }
> > 
> > Description: Pre-initialize the MAC speed with the value retrieved from
> > the "snps,ps-speed" property. The speed is then fixed in the SGMII
> > Rate adaptation Layer by setting up the SGMIRAL flag performed in the
> > dwmac_ctrl_ane() method you cited. 
> > Note the same register fields (MAC_CONTROL.{PS,FES}) are touched in
> > the stmmac_mac_link_up() method in the framework of the MAC-link up
> > procedure (phylink_mac_ops::mac_link_up). But AFAICS the procedure is
> > done too late, and after the SGMIRAL flag is set in the framework of
> > the stmmac_open()->stmmac_hw_setup()->dwmac_ctrl_ane() call. That's
> > probably why the MAC2MAC mode developer needed the speed being
> > pre-initialized earlier in the dwmac*_core_init() functions.
> > 
> > 
> > To sum up the MAC-2-MAC mode implementation can be described as
> > follows:
> > 1. The platform firmware defines the fixed link speed by means of the
> > "snps,ps-speed" property (or by pre-initializing the
> > plat_stmmacenet_data::mac_port_sel_speed field in the glue driver).
> > 2. If the SGMII PHY interface is supported by the DW GMAC/QoS Eth
> > controller, the speed will be fixed right at the network interface
> > open() procedure execution by subsequently calling the
> > stmmac_core_init() and stmmac_pcs_ctrl_ane() methods.
> > 
> > Please note I can't immediately tell of how that functionality gets to
> > live with the phylink-based link setup procedure, in the framework of
> > which the link speed is also setup in stmmac_mac_link_up(). Alas I
> > don't have any SGMII-based device to test it out. But I guess that it
> > lives well until the pre-initialized in the "snps,ps-speed"
> > speed matches to the speed negotiated between the connected PHYs. If
> > the speeds don't match, then I can't tell for sure what would happen
> > without a hardware at hands.
> > 
> > > What is that bit doing exactly?
> > 
> > Here is what the DW MAC databook says about the bit:
> > 
> > "SGMII RAL Control
> > 
> > When set, this bit forces the SGMII RAL block to operate in the speed
> > configured in the MAC Configuration register's Speed and Port Select
> > bits. This is useful when the SGMII interface is used in a direct
> > MAC-MAC connection (without a PHY) and any MAC must reconfigure the
> > speed.
> > 
> > When reset, the SGMII RAL block operates according to the link speed
> > status received on the SGMII (from the PHY)."
> > 
> > Shortly speaking the flag enables the SGMII Rate adaptation layer
> > (SGMII RAL) to stop selecting the speed based on the info retrieved
> > from the PHY and instead to fix the speed with the value specified in
> > the MAC_CTRL_REG register.
> > 
> > > The only user upstream I see is
> > > sa8775p-ride variants, but they're all using a phy right now (not
> > > fixed-link aka MAC2MAC iiuc)... so I should probably remove it from
> > > there too.
> > 
> > Right, there is only a single user of that property in the kernel. But
> > I don't know what was the actual reason of having the "snps,ps-speed"
> > specified in that case. From one side it seems contradicting to the
> > original MAC-2-MAC semantics since the phy-handle property is also
> > specified in the ethernet controller DT-node, but from another side it
> > might have been caused by some HW-related problem so there was a need
> > to fix the speed. It would be better to ask Bartosz Golaszewski about
> > that since it was him who submitted the patch adding the sa8775p-ride
> > Ethernet DT-nodes.
> > 
> > > 
> > 
> > > I feel like that property really (if I'm following right) should be just
> > > taken from a proper fixed-link devicetree description? i.e. we already
> > > specify a speed in that case. Maybe this predates that (or reinvents it)
> > > and should be marked as deprecated in the dt-bindings.
> > 
> > Exactly my thoughts of the way it should have been implemented in the
> > first place. The fixed-linked semantics was exactly suitable for the
> > MAC-2-MAC HW-setup since both of them implies the link-speed being
> > fixed to a single value with no PHY-node required.
> > 
> > Note if the respective code conversion is planned to be done then I
> > guess it would be better to do after the Russell' series
> > https://lore.kernel.org/netdev/ZrCoQZKo74zvKMhT@shell.armlinux.org.uk/
> > is merged in, since the changes are related.
> > 
> > > 
> > > But I'm struggling to understand what the bit is really doing based
> > > on the original commit that added it, so I don't know if my logic is
> > > solid. i.e., what's different in the phy case vs mac2mac with this
> > > particular bit?
> > 
> > Please see my notes above for more details about the SGMRAL bit
> > semantics. Shortly speaking The difference is basically in the way the
> > link-speed is established:
> > - If the SGMRAL bit is set the MAC' SGMII RAL will operate with the
> >   speed specified in the Speed and Port Select bits of the
> >   MAC_CTRL_REG register.
> > - If the SGMRAL bit is reset the SGMII RAL will operate according to
> >   the link speed status received on SGMII (from the PHY).
> 
> I think with all of that in mind, long term it might make sense to:
> 

>     1. Remove ps-speed handling

I guess this can't be completely dropped due to the DT backward
compatibility requirement.

>     2. Program GMAC_AN_CTRL in mac_link_up() as specified in the
>        kerneldoc (i.e. iiuc only in !phylink_autoneg_inband(@mode) case)
>     3. Program SGMRAL bit in pcs_config as specified in the kernel doc
>        (iiuc mode == PHYLINK_PCS_NEG_OUTBAND)

(mode == MLO_AN_FIXED) seems more appropriate.

> 
> I'm probably misunderstanding the phylink interactions while reading
> on the plane today (and hence may have misunderstood what callbacks
> should configure that and when), but in general sounds like the forward
> looking step would be to build on top of Russell's PCS changes and rip out the
> ps-speed stuff, relying on phylink to indicate what the speed / port
> select stuff needs to be in the PCS and MAC in the various configs
> possible (i.e. stop treating fixed-link special here).

Sounds like a plan. But please note that it's normally prohibited to
break the DT properties functionality so the kernel would work on the
older platforms (although I have doubts there is any real device with
the MAC-2-MAC setup). In anyway it's better to wait for the Russell'
series to be merged in, and then think of the "snps,ps-speed"-related
code refactoring (perhaps in a dedicated emailing thread).

> 
> Maybe Qualcomm can help with testing the SGMII fixed-link setup, as I
> think they're the only folks I know of with a board with that at least.
> I have acess to a sa8775p-ride, but that has SGMII with a phy so its not
> going to cover all testing.

Very much hope they will, but there is a good chance they don't have
an appropriate device either (with the MAC-2-MAC setup). The original
code author was Giuseppe Cavallaro - former driver maintainer from
STmicroelectronics, who hasn't been activate in the driver support for
years. Alas there is no info left in the mailing list archive for what
device he has introduced the ps-speed property functionality.

> 
> FYI.. I'm traveling the next two weeks, sorry in advance if I disappear
> for a bit :)

No worries. I am quite busy currently too.)

-Serge(y)

> 
> Thanks,
> Andrew
> 

Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

[Andrew Halaney]

On Fri, Aug 09, 2024 at 10:21:39PM GMT, Serge Semin wrote:
> If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
> instance and there is no "snps,ps-speed" property specified (or the
> plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
> next warning will be printed to the system log:
> 
> > [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed
> 
> By the original intention the "snps,ps-speed" property was supposed to be
> utilized on the platforms with the MAC2MAC link setup to fix the link
> speed with the specified value. But since it's possible to have a device
> with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
> the property is actually optional (which is also confirmed by the DW MAC
> DT-bindings). Thus it's absolutely normal to have the
> plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
> that there is no need in the MAC-speed fixing and the denoted warning is
> false.
> 
> Fix the warning by permitting the plat_stmmacenet_data::mac_port_sel_speed
> field to have the zero value in case if the internal PCS is available.
> 
> Fixes: 02e57b9d7c8c ("drivers: net: stmmac: add port selection programming")
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>