[PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

[PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

[Russell King (Oracle)]

Hi,

As I noted recently in a thread (and was ignored) stmmac sucks. (I
won't hide my distain for drivers that make my life as phylink
maintainer more difficult!)

One of the contract conditions for using phylink is that the driver
will _not_ mess with the netif carrier. stmmac developers/maintainers
clearly didn't read that, because stmmac messes with the netif
carrier, which destroys phylink's guarantee that it'll make certain
calls in a particular order (e.g. it won't call mac_link_up() twice
in a row without an intervening mac_link_down().) This is clearly
stated in the phylink documentation.

Thus, this patch set attempts to fix this. Why does it mess with the
netif carrier? It has its own independent PCS implementation that
completely bypasses phylink _while_ phylink is still being used.
This is not acceptable. Either the driver uses phylink, or it doesn't
use phylink. There is no half-way house about this. Therefore, this
driver needs to either be fixed, or needs to stop using phylink.

Since I was ignored when I brought this up, I've hacked together the
following patch set - and it is hacky at the moment. It's also broken
because of recentl changes involving dwmac-qcom-ethqos.c - but there
isn't sufficient information in the driver for me to fix this. The
driver appears to use SGMII at 2500Mbps, which simply does not exist.
What interface mode (and neg_mode) does phylink pass to pcs_config()
in each of the speeds that dwmac-qcom-ethqos.c is interested in.
Without this information, I can't do that conversion. So for the
purposes of this, I've just ignored dwmac-qcom-ethqos.c (which means
it will fail to build.)

The patch splitup is not ideal, but that's not what I'm interested in
here. What I want to hear is the results of testing - does this switch
of the RGMII/SGMII "pcs" stuff to a phylink_pcs work for this driver?

Please don't review the patches, but you are welcome to send fixes to
them. Once we know that the overall implementation works, then I'll
look at how best to split the patches. In the mean time, the present
form is more convenient for making changes and fixing things.

There is still more improvement that's needed here.

Thanks.

 drivers/net/ethernet/stmicro/stmmac/Makefile       |   2 +-
 drivers/net/ethernet/stmicro/stmmac/common.h       |  12 ++-
 .../net/ethernet/stmicro/stmmac/dwmac1000_core.c   | 113 ++++++++++++---------
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  | 108 ++++++++++++--------
 .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    |   6 --
 drivers/net/ethernet/stmicro/stmmac/hwif.h         |  27 ++---
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 111 +-------------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  19 ++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c   |  57 +++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h   |  84 ++-------------
 10 files changed, 227 insertions(+), 312 deletions(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 1/6] net: stmmac: convert sgmii/rgmii "pcs" to phylink

[Russell King (Oracle)]

Convert the sgmii/rgmii "pcs" implementation in stmmac to use a
phylink_pcs so we can get rid of exceptional paths.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/Makefile  |  2 +-
 drivers/net/ethernet/stmicro/stmmac/common.h  |  9 +-
 .../ethernet/stmicro/stmmac/dwmac1000_core.c  | 77 ++++++++++++++++-
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 82 ++++++++++++++++++-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    | 16 +++-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  7 ++
 .../net/ethernet/stmicro/stmmac/stmmac_pcs.c  | 57 +++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_pcs.h  |  9 ++
 8 files changed, 252 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 26cad4344701..e6e985cf7bec 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o	\
 	      mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o	\
 	      dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \
 	      stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \
-	      stmmac_xdp.o stmmac_est.o \
+	      stmmac_xdp.o stmmac_est.o stmmac_pcs.o \
 	      $(stmmac-y)
 
 stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 9cd62b2110a1..82d0d897019c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -14,7 +14,7 @@
 #include <linux/etherdevice.h>
 #include <linux/netdevice.h>
 #include <linux/stmmac.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/pcs/pcs-xpcs.h>
 #include <linux/module.h>
 #if IS_ENABLED(CONFIG_VLAN_8021Q)
@@ -593,6 +593,7 @@ struct mac_device_info {
 	const struct stmmac_tc_ops *tc;
 	const struct stmmac_mmc_ops *mmc;
 	const struct stmmac_est_ops *est;
+	struct phylink_pcs mac_pcs;	/* The MAC's RGMII/SGMII "PCS" */
 	struct dw_xpcs *xpcs;
 	struct phylink_pcs *phylink_pcs;
 	struct mii_regs mii;	/* MII register Addresses */
@@ -613,6 +614,12 @@ struct mac_device_info {
 	bool hw_vlan_en;
 };
 
+static inline struct mac_device_info *
+phylink_pcs_to_mac_dev_info(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct mac_device_info, mac_pcs);
+}
+
 struct stmmac_rx_routing {
 	u32 reg_mask;
 	u32 reg_shift;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index 8555299443f4..4ead61886fe5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -15,7 +15,8 @@
 #include <linux/crc32.h>
 #include <linux/slab.h>
 #include <linux/ethtool.h>
-#include <asm/io.h>
+#include <linux/io.h>
+#include <linux/phylink.h>
 #include "stmmac.h"
 #include "stmmac_pcs.h"
 #include "dwmac1000.h"
@@ -335,8 +336,10 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
 
 	dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
 
-	if (intr_status & PCS_RGSMIIIS_IRQ)
+	if (intr_status & PCS_RGSMIIIS_IRQ) {
+		phylink_pcs_change(&hw->mac_pcs, false);
 		dwmac1000_rgsmii(ioaddr, x);
+	}
 
 	return ret;
 }
@@ -414,6 +417,72 @@ static void dwmac1000_get_adv_lp(void __iomem *ioaddr, struct rgmii_adv *adv)
 	dwmac_get_adv_lp(ioaddr, GMAC_PCS_BASE, adv);
 }
 
+static int dwmac1000_mii_pcs_validate(struct phylink_pcs *pcs,
+				      unsigned long *supported,
+				      const struct phylink_link_state *state)
+{
+	/* Only support in-band */
+	if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int dwmac1000_mii_pcs_config(struct phylink_pcs *pcs,
+				    unsigned int neg_mode,
+				    phy_interface_t interface,
+				    const unsigned long *advertising,
+				    bool permit_pause_to_mac)
+{
+	struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
+
+	return dwmac_pcs_config(hw, advertising, GMAC_PCS_BASE);
+}
+
+static void dwmac1000_mii_pcs_get_state(struct phylink_pcs *pcs,
+					struct phylink_link_state *state)
+{
+	struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
+	unsigned int spd_clk;
+	u32 status;
+
+	status = readl(hw->pcsr + GMAC_RGSMIIIS);
+
+	state->link = status & GMAC_RGSMIIIS_LNKSTS;
+	if (!state->link)
+		return;
+
+	spd_clk = FIELD_GET(GMAC_RGSMIIIS_SPEED, status);
+	if (spd_clk == GMAC_RGSMIIIS_SPEED_125)
+		state->speed = SPEED_1000;
+	else if (spd_clk == GMAC_RGSMIIIS_SPEED_25)
+		state->speed = SPEED_100;
+	else if (spd_clk == GMAC_RGSMIIIS_SPEED_2_5)
+		state->speed = SPEED_10;
+
+	state->duplex = status & GMAC_RGSMIIIS_LNKMOD_MASK ?
+			DUPLEX_FULL : DUPLEX_HALF;
+
+	dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE);
+}
+
+static const struct phylink_pcs_ops dwmac1000_mii_pcs_ops = {
+	.pcs_validate = dwmac1000_mii_pcs_validate,
+	.pcs_config = dwmac1000_mii_pcs_config,
+	.pcs_get_state = dwmac1000_mii_pcs_get_state,
+};
+
+static struct phylink_pcs *
+dwmac1000_phylink_select_pcs(struct stmmac_priv *priv,
+			     phy_interface_t interface)
+{
+	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
+	    priv->hw->pcs & STMMAC_PCS_SGMII)
+		return &priv->hw->mac_pcs;
+
+	return NULL;
+}
+
 static void dwmac1000_debug(struct stmmac_priv *priv, void __iomem *ioaddr,
 			    struct stmmac_extra_stats *x,
 			    u32 rx_queues, u32 tx_queues)
@@ -504,6 +573,7 @@ static void dwmac1000_set_mac_loopback(void __iomem *ioaddr, bool enable)
 
 const struct stmmac_ops dwmac1000_ops = {
 	.core_init = dwmac1000_core_init,
+	.phylink_select_pcs = dwmac1000_phylink_select_pcs,
 	.set_mac = stmmac_set_mac,
 	.rx_ipc = dwmac1000_rx_ipc_enable,
 	.dump_regs = dwmac1000_dump_regs,
@@ -555,5 +625,8 @@ int dwmac1000_setup(struct stmmac_priv *priv)
 	mac->mii.clk_csr_shift = 2;
 	mac->mii.clk_csr_mask = GENMASK(5, 2);
 
+	mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops;
+	mac->mac_pcs.neg_mode = true;
+
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index b25774d69195..3c1181b1933f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/ethtool.h>
 #include <linux/io.h>
+#include <linux/phylink.h>
 #include "stmmac.h"
 #include "stmmac_pcs.h"
 #include "dwmac4.h"
@@ -801,6 +802,77 @@ static void dwmac4_phystatus(void __iomem *ioaddr, struct stmmac_extra_stats *x)
 	}
 }
 
+static int dwmac4_mii_pcs_validate(struct phylink_pcs *pcs,
+				   unsigned long *supported,
+				   const struct phylink_link_state *state)
+{
+	/* Only support in-band */
+	if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int dwmac4_mii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+				 phy_interface_t interface,
+				 const unsigned long *advertising,
+				 bool permit_pause_to_mac)
+{
+	struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
+
+	return dwmac_pcs_config(hw, advertising, GMAC_PCS_BASE);
+}
+
+static void dwmac4_mii_pcs_get_state(struct phylink_pcs *pcs,
+				     struct phylink_link_state *state)
+{
+	struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
+	unsigned int clk_spd;
+	u32 status;
+
+	status = readl(hw->pcsr + GMAC_PHYIF_CONTROL_STATUS);
+
+	state->link = !!(status & GMAC_PHYIF_CTRLSTATUS_LNKSTS);
+	if (!state->link)
+		return;
+
+	clk_spd = FIELD_GET(GMAC_PHYIF_CTRLSTATUS_SPEED, status);
+	if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_125)
+		state->speed = SPEED_1000;
+	else if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_25)
+		state->speed = SPEED_100;
+	else if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_2_5)
+		state->speed = SPEED_10;
+
+	/* FIXME: Is this even correct?
+	 * GMAC_PHYIF_CTRLSTATUS_TC = BIT(0)
+	 * GMAC_PHYIF_CTRLSTATUS_LNKMOD = BIT(16)
+	 * GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK = 1
+	 *
+	 * The result is, we test bit 0 for the duplex setting.
+	 */
+	state->duplex = status & GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK ?
+			DUPLEX_FULL : DUPLEX_HALF;
+
+	dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE);
+}
+
+static const struct phylink_pcs_ops dwmac4_mii_pcs_ops = {
+	.pcs_validate = dwmac4_mii_pcs_validate,
+	.pcs_config = dwmac4_mii_pcs_config,
+	.pcs_get_state = dwmac4_mii_pcs_get_state,
+};
+
+static struct phylink_pcs *
+dwmac4_phylink_select_pcs(struct stmmac_priv *priv, phy_interface_t interface)
+{
+	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
+	    priv->hw->pcs & STMMAC_PCS_SGMII)
+		return &priv->hw->mac_pcs;
+
+	return NULL;
+}
+
 static int dwmac4_irq_mtl_status(struct stmmac_priv *priv,
 				 struct mac_device_info *hw, u32 chan)
 {
@@ -872,8 +944,10 @@ static int dwmac4_irq_status(struct mac_device_info *hw,
 	}
 
 	dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
-	if (intr_status & PCS_RGSMIIIS_IRQ)
+	if (intr_status & PCS_RGSMIIIS_IRQ) {
+		phylink_pcs_change(&hw->mac_pcs, false);
 		dwmac4_phystatus(ioaddr, x);
+	}
 
 	return ret;
 }
@@ -1191,6 +1265,7 @@ static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw)
 const struct stmmac_ops dwmac4_ops = {
 	.core_init = dwmac4_core_init,
 	.update_caps = dwmac4_update_caps,
+	.phylink_select_pcs = dwmac4_phylink_select_pcs,
 	.set_mac = stmmac_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1236,6 +1311,7 @@ const struct stmmac_ops dwmac4_ops = {
 const struct stmmac_ops dwmac410_ops = {
 	.core_init = dwmac4_core_init,
 	.update_caps = dwmac4_update_caps,
+	.phylink_select_pcs = dwmac4_phylink_select_pcs,
 	.set_mac = stmmac_dwmac4_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1285,6 +1361,7 @@ const struct stmmac_ops dwmac410_ops = {
 const struct stmmac_ops dwmac510_ops = {
 	.core_init = dwmac4_core_init,
 	.update_caps = dwmac4_update_caps,
+	.phylink_select_pcs = dwmac4_phylink_select_pcs,
 	.set_mac = stmmac_dwmac4_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1399,5 +1476,8 @@ int dwmac4_setup(struct stmmac_priv *priv)
 	mac->mii.clk_csr_mask = GENMASK(11, 8);
 	mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr);
 
+	mac->mac_pcs.ops = &dwmac4_mii_pcs_ops;
+	mac->mac_pcs.neg_mode = true;
+
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 90384db228b5..e106f57b8b66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -5,6 +5,7 @@
 #ifndef __STMMAC_HWIF_H__
 #define __STMMAC_HWIF_H__
 
+#include <linux/err.h>
 #include <linux/netdevice.h>
 #include <linux/stmmac.h>
 
@@ -17,13 +18,17 @@
 	} \
 	__result; \
 })
-#define stmmac_do_callback(__priv, __module, __cname,  __arg0, __args...) \
+#define stmmac_do_typed_callback(__type, __fail_ret, __priv, __module, \
+				 __cname,  __arg0, __args...) \
 ({ \
-	int __result = -EINVAL; \
+	__type __result = __fail_ret; \
 	if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) \
 		__result = (__priv)->hw->__module->__cname((__arg0), ##__args); \
 	__result; \
 })
+#define stmmac_do_callback(__priv, __module, __cname,  __arg0, __args...) \
+	stmmac_do_typed_callback(int, -EINVAL, __priv, __module, __cname, \
+				 __arg0, ##__args)
 
 struct stmmac_extra_stats;
 struct stmmac_priv;
@@ -310,6 +315,9 @@ struct stmmac_ops {
 	void (*core_init)(struct mac_device_info *hw, struct net_device *dev);
 	/* Update MAC capabilities */
 	void (*update_caps)(struct stmmac_priv *priv);
+	/* Get phylink PCS (for MAC */
+	struct phylink_pcs *(*phylink_select_pcs)(struct stmmac_priv *priv,
+						  phy_interface_t interface);
 	/* Enable the MAC RX/TX */
 	void (*set_mac)(void __iomem *ioaddr, bool enable);
 	/* Enable and verify that the IPC module is supported */
@@ -432,6 +440,10 @@ struct stmmac_ops {
 	stmmac_do_void_callback(__priv, mac, core_init, __args)
 #define stmmac_mac_update_caps(__priv) \
 	stmmac_do_void_callback(__priv, mac, update_caps, __priv)
+#define stmmac_mac_phylink_select_pcs(__priv, __interface) \
+	stmmac_do_typed_callback(struct phylink_pcs *, ERR_PTR(-EOPNOTSUPP), \
+				 __priv, mac, phylink_select_pcs, __priv,\
+				 __interface)
 #define stmmac_mac_set(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, set_mac, __args)
 #define stmmac_rx_ipc(__priv, __args...) \
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 142a7c598efe..79364b60ac6b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -956,6 +956,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
 						 phy_interface_t interface)
 {
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
+	struct phylink_pcs *pcs;
+
+	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)) {
+		pcs = stmmac_mac_phylink_select_pcs(priv, interface);
+		if (!IS_ERR(pcs))
+			return pcs;
+	}
 
 	if (priv->hw->xpcs)
 		return &priv->hw->xpcs->pcs;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
new file mode 100644
index 000000000000..a16c5636ad05
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
@@ -0,0 +1,57 @@
+#include "common.h"
+#include "stmmac_pcs.h"
+
+int dwmac_pcs_config(struct mac_device_info *hw,
+		     const unsigned long *advertising,
+		     unsigned int reg_base)
+{
+	u32 val;
+
+	val = readl(hw->pcsr + GMAC_AN_CTRL(reg_base));
+
+	val |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
+
+	if (hw->ps)
+		val |= GMAC_AN_CTRL_SGMRAL;
+
+	writel(val, hw->pcsr + GMAC_AN_CTRL(reg_base));
+
+	return 0;
+}
+
+void dwmac_pcs_get_state(struct mac_device_info *hw,
+			 struct phylink_link_state *state,
+			 unsigned int reg_base)
+{
+	u32 val;
+
+	val = readl(hw->pcsr + GMAC_ANE_LPA(reg_base));
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+			 state->lp_advertising);
+
+	if (val & GMAC_ANE_FD) {
+		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+				 state->lp_advertising);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+				 state->lp_advertising);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+				 state->lp_advertising);
+	}
+
+	if (val & GMAC_ANE_HD) {
+		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+				 state->lp_advertising);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+				 state->lp_advertising);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+				 state->lp_advertising);
+	}
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+			 state->lp_advertising,
+			 FIELD_GET(GMAC_ANE_PSE, val) & STMMAC_PCS_PAUSE);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+			 state->lp_advertising,
+			 FIELD_GET(GMAC_ANE_PSE, val) & STMMAC_PCS_ASYM_PAUSE);
+}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h
index 13a30e6df4c1..c3ff12a6859b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h
@@ -154,4 +154,13 @@ static inline void dwmac_get_adv_lp(void __iomem *ioaddr, u32 reg,
 
 	adv_lp->lp_pause = (value & GMAC_ANE_PSE) >> GMAC_ANE_PSE_SHIFT;
 }
+
+int dwmac_pcs_config(struct mac_device_info *hw,
+		     const unsigned long *advertising,
+		     unsigned int reg_base);
+
+void dwmac_pcs_get_state(struct mac_device_info *hw,
+			 struct phylink_link_state *state,
+			 unsigned int reg_base);
+
 #endif /* __STMMAC_PCS_H__ */
-- 
2.30.2

Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

[Serge Semin]

Hi Russell

On Sun, May 12, 2024 at 05:28:20PM +0100, Russell King (Oracle) wrote:
> Hi,
> 

> As I noted recently in a thread (and was ignored)

Sorry about that. I didn't mean to ignore. Your message reached me
right when I caught a cold, which made me AFK for the rest of the
week.(

> As I noted recently in a thread (and was ignored) stmmac sucks.

Can't argue with that. There are much more aspects in what it sucks
than just the netif's. One glimpse at the plat_stmmacenet_data
structure causes million questions aka why, how come, what the hell...
I'll start submitting my cleanup patch sets after my another
networking work (DW XPCS wise) is finally done, re-submitted, reviewed
and merged in.

> (I
> won't hide my distain for drivers that make my life as phylink
> maintainer more difficult!)
> 
> One of the contract conditions for using phylink is that the driver
> will _not_ mess with the netif carrier. stmmac developers/maintainers
> clearly didn't read that, because stmmac messes with the netif
> carrier, which destroys phylink's guarantee that it'll make certain
> calls in a particular order (e.g. it won't call mac_link_up() twice
> in a row without an intervening mac_link_down().) This is clearly
> stated in the phylink documentation.
> 
> Thus, this patch set attempts to fix this. Why does it mess with the
> netif carrier? It has its own independent PCS implementation that
> completely bypasses phylink _while_ phylink is still being used.
> This is not acceptable. Either the driver uses phylink, or it doesn't
> use phylink. There is no half-way house about this. Therefore, this
> driver needs to either be fixed, or needs to stop using phylink.

Thanks for submitting the series, especially for making the RGMII
in-band status well-implemented in the driver. When I was studying the
STMMAC internals I was surprised that it wasn't actually utilized for
something useful. Furthermore at some point afterwards even the
RGSMIIIS IRQ turned to be disabled. So the RGMII-part of the code has
been completely unused after that. But even before that the RGMII
in-band status change IRQ was utilized just to print the link state
into the system log. 

> 
> Since I was ignored when I brought this up, I've hacked together the
> following patch set - and it is hacky at the moment. It's also broken
> because of recentl changes involving dwmac-qcom-ethqos.c - but there
> isn't sufficient information in the driver for me to fix this. The
> driver appears to use SGMII at 2500Mbps, which simply does not exist.
> What interface mode (and neg_mode) does phylink pass to pcs_config()
> in each of the speeds that dwmac-qcom-ethqos.c is interested in.
> Without this information, I can't do that conversion. So for the
> purposes of this, I've just ignored dwmac-qcom-ethqos.c (which means
> it will fail to build.)
> 
> The patch splitup is not ideal, but that's not what I'm interested in
> here. What I want to hear is the results of testing - does this switch
> of the RGMII/SGMII "pcs" stuff to a phylink_pcs work for this driver?
> 
> Please don't review the patches, but you are welcome to send fixes to
> them. Once we know that the overall implementation works, then I'll
> look at how best to split the patches. In the mean time, the present
> form is more convenient for making changes and fixing things.

I'll give your series a try later on this week on my DW GMAC with the
RGMII interface (alas I don't have an SGMII capable device, so can't
help with the AN-part testing). Today I've made a brief glance on it
and already noted a few places which may require a fix to make the
change working for RGMII (at least the RGSMIIIS IRQ must be got back
enabled). After making the patch set working for my device in what
form would you prefer me to submit the fixes? As incremental patches
in-reply to this thread?

-Serge(y)

> 
> There is still more improvement that's needed here.
> 
> Thanks.
> 
>  drivers/net/ethernet/stmicro/stmmac/Makefile       |   2 +-
>  drivers/net/ethernet/stmicro/stmmac/common.h       |  12 ++-
>  .../net/ethernet/stmicro/stmmac/dwmac1000_core.c   | 113 ++++++++++++---------
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  | 108 ++++++++++++--------
>  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    |   6 --
>  drivers/net/ethernet/stmicro/stmmac/hwif.h         |  27 ++---
>  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 111 +-------------------
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  19 ++--
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c   |  57 +++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h   |  84 ++-------------
>  10 files changed, 227 insertions(+), 312 deletions(-)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

[Russell King (Oracle)]

On Tue, May 14, 2024 at 02:04:00AM +0300, Serge Semin wrote:
> Hi Russell
> 
> I'll give your series a try later on this week on my DW GMAC with the
> RGMII interface (alas I don't have an SGMII capable device, so can't
> help with the AN-part testing).

Thanks!

> Today I've made a brief glance on it
> and already noted a few places which may require a fix to make the
> change working for RGMII (at least the RGSMIIIS IRQ must be got back
> enabled). After making the patch set working for my device in what
> form would you prefer me to submit the fixes? As incremental patches
> in-reply to this thread?

I think it depends on where the issues are.

If they are addressing issues that are also present in the existing
code, then it would make sense to get those patched as the driver
stands today, because backporting them to stable would be easier.

If they are for "new" issues, given that this patch series is more
or less experimental, I would prefer to roll them into these
patches. As mentioned, when it comes to submitting these patches,
the way I've split them wouldn't make much sense, but it does
make sense for where I am with it. Hence, I'll want to resplit
the series into something better for submission than it currently
is. If you want to reply to this thread, that is fine.

There's still a few netif_carrier_off()/netif_carrier_on()s left
in the driver even after this patch series, but I think they're in
more obscure paths, but I will also want to address those as well.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

[Serge Semin]

On Tue, May 14, 2024 at 12:21:42AM +0100, Russell King (Oracle) wrote:
> On Tue, May 14, 2024 at 02:04:00AM +0300, Serge Semin wrote:
> > Hi Russell
> > 
> > I'll give your series a try later on this week on my DW GMAC with the
> > RGMII interface (alas I don't have an SGMII capable device, so can't
> > help with the AN-part testing).
> 
> Thanks!
> 
> > Today I've made a brief glance on it
> > and already noted a few places which may require a fix to make the
> > change working for RGMII (at least the RGSMIIIS IRQ must be got back
> > enabled). After making the patch set working for my device in what
> > form would you prefer me to submit the fixes? As incremental patches
> > in-reply to this thread?
> 
> I think it depends on where the issues are.
> 

> If they are addressing issues that are also present in the existing
> code, then it would make sense to get those patched as the driver
> stands today, because backporting them to stable would be easier.
> 

Sure. If I get to find any problem with the existing code I'll submit
a fix as an independent patch.

> If they are for "new" issues, given that this patch series is more
> or less experimental, I would prefer to roll them into these
> patches. As mentioned, when it comes to submitting these patches,
> the way I've split them wouldn't make much sense, but it does
> make sense for where I am with it. Hence, I'll want to resplit
> the series into something better for submission than it currently
> is. If you want to reply to this thread, that is fine.

I was meaning the "new" issues. Ok then. I'll prepare the fixes as
incremental patches either on top of the entire series or on top of
the respective patches, so the altered parts could be spotted right
out of the patches body. Then I'll submit them in-reply to the
respective messages in this patch set. After that you'll be able to
squash them up into the commits in your repo and re-shuffle the
changes as would be more appropriate.

> 
> There's still a few netif_carrier_off()/netif_carrier_on()s left
> in the driver even after this patch series, but I think they're in
> more obscure paths, but I will also want to address those as well.

Yeah, I noticed that. I was going to discuss this matter after getting
the changes tested.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink

[Serge Semin]

Hi Russell

On Tue, May 14, 2024 at 12:21:42AM +0100, Russell King (Oracle) wrote:
> On Tue, May 14, 2024 at 02:04:00AM +0300, Serge Semin wrote:
> > Hi Russell
> > 
> > I'll give your series a try later on this week on my DW GMAC with the
> > RGMII interface (alas I don't have an SGMII capable device, so can't
> > help with the AN-part testing).
> 
> Thanks!
> 
> > Today I've made a brief glance on it
> > and already noted a few places which may require a fix to make the
> > change working for RGMII (at least the RGSMIIIS IRQ must be got back
> > enabled). After making the patch set working for my device in what
> > form would you prefer me to submit the fixes? As incremental patches
> > in-reply to this thread?
> 
> I think it depends on where the issues are.
> 
> If they are addressing issues that are also present in the existing
> code, then it would make sense to get those patched as the driver
> stands today, because backporting them to stable would be easier.
> 
> If they are for "new" issues, given that this patch series is more
> or less experimental, I would prefer to roll them into these
> patches. As mentioned, when it comes to submitting these patches,
> the way I've split them wouldn't make much sense, but it does
> make sense for where I am with it. Hence, I'll want to resplit
> the series into something better for submission than it currently
> is. If you want to reply to this thread, that is fine.

I've just submitted the fixes for your series
https://lore.kernel.org/netdev/20240524210304.9164-1-fancer.lancer@gmail.com/
which make it working well on my hardware: DW GMAC v3.73 with RGMII
PHY interface connected to the Micrel KSZ9031RNX PHY. (For a lucky
coincident the PHY happen to support the link status sent in-band up
to the MAC.) So as long as the managed="in-band-status" property is
specified the PCS subsystem gets the link-status by means of the
pcs_get_state() callback. The status change is signaled by means of
the RGSMIIIS IRQ.  For that to work the Patch 2 of my series was
required (and of course Patch 1 which prevents the IRQs flood).

I'm sorry for submitting the series only today. First I had to dig
deeper into the way the RGMII In-band/PCS-block of the controller
works. Then I needed some time to study the STMMAC PCS-code and to
debug the problems fixed in my series. So I finished testing your
patchset on this Monday. Then I decided to spend sometime for making
the PCS implementation looking more optimized based on the knowledge I
gained during the debugging. But as it's normal for the STMMAC driver
(which sucks indeed) a few days wasn't enough for that, because due to
the driver being overwhelmed with duty hacks any more-or-less invasive
refactoring may lead to regressions here or there. So I stuck right at
the first step of getting the "snps,ps-speed" and the MAC2MAC mode
well implemented...(

Anyway here is the key points regarding the RGMII In-band and
PCS-interface implemented in the DW GMAC and DW QoS Eth
controllers/drivers:

1. Intermediate PCS for which the plat_stmmacenet_data::mac_interface
field and the "mac-mode" property was introduced isn't the case of the
PCS embedded into the DW GMAC and DW QoS Eth IP-cores by Synopsys.
That was some another PCS likely specific for the Altera SoC FPGA
platform (dwmac-socfpga.c).

2. RGMII: There is no any PCS-block utilized in case of the RGMII PHY
interface (that's why HWFEATURE.PCSSEL flag isn't set). The networking
controller provides a way to pick up the RGMII In-band status
delivered from the attached PHY. The in-band status is updated in the
GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW QoS Eth)
registers and signalled via the RGSMIIIS MAC IRQ.

3. SGMII: The interface implementation has a PCS-block so the
HWFEATURE.PCSSEL flag is set. But the auto-negotiation procedure
complies to the SGMII specification: no ability advertisement. SGMII
PHY sends the control information back to the MAC by means of the
tx_config_Reg[15:0] register. MAC either acknowledges the update or
not. The control information retrieved from the PHY is reflected in
the GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW QoS
Eth) registers. The only AN-related CSR available for the SGMII
interface are GMAC_AN_CTRL(x) and GMAC_AN_STATUS(x) since no
advertisement implied by the specification.

4. RGMII/SGMII/SMII: Note CSR-wise the tx_config_Reg[15:0] register
mapping is the same for all of these interfaces. It's available in the
GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW QoS Eth)
CSRs: in case of the DW GMAC it's GMAC_RGSMIIIS[0:15] bits, but in
case of DW QoS Eth it's GMAC_PHYIF_CTRLSTATUS[16:31]. (This info can
be useful to create a common dwmac_inband_pcs_get_state() method
implementation in the stmmac_pcs.c module.)

5. TBI/RTBI: It has a traditional auto-negotiation procedure fully
complying to the IEEE 802.3z C37 specification with the link abilities
advertisement. RBI/RTBI don't imply any in-band link status detection
so the GMAC_RGSMIIIS (DW GMAC) and in the GMAC_PHYIF_CTRLSTATUS (DW
QoS Eth) CSRs aren't available for these interfaces.

6. RGMII/SGMII/SMII: In order to have the Link Speed
(GMAC_CONTROL.{PS,FES}), Duplex mode (GMAC_CONTROL.DM) and Link
Up/Down bit (GMAC_CONTROL.LUD or GMAC_PHYIF_CTRLSTATUS.LUD)
transferred from MAC to the attached PHY or to another MAC via
tx_config_Reg[15:0], the GMAC_CONTROL.TC (DW GMAC) or
GMAC_PHYIF_CTRLSTATUS.TC (DW QoS Eth) flags must be set. Just a note
seeing the current PCS implementation doesn't do that even in case of
the fixed Port-select speed setup (when snps,ps-speed property is
specified).


Based on the info above I was going to extend your stmmac_pcs.c module
with the inband link status (retrieved via the tx_config_Reg[15:0])
parsing method; create more basic PCS-ops in the framework of the
dwmac1000_core.c and dwmac4_core.c modules, and the common
phylink_pcs_ops in the stmmac_main.c module using those basic PCS-ops.
But as I mentioned before I was stuck on the fixed Port-select speed
implementation. It's activated via the "snps,ps-speed" property. If
it's specified the AN_Control.SGMRAL flag will be set which makes the
SGMII interface working with a fixed speed pre-initialized in the
MAC_CONFIG.{PS,FES} fields. First of all I wasn't sure whether the
MAC2MAC functionality it's utilized for, can be applicable for
non-SGMII interface. Secondly the port speed fixing is performed
behind the phylink back. It's possible to have the speed setting being
updated later in framework of the mac_link_up() callback. So I have
some doubts that the current "snps,ps-speed"-based fixed port speed
implementation is fully correct.

That's the stage at which I decided to stop further researches and
sent my series of fixes to you.

-Serge(y)

> 
> There's still a few netif_carrier_off()/netif_carrier_on()s left
> in the driver even after this patch series, but I think they're in
> more obscure paths, but I will also want to address those as well.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 1/3] net: stmmac: Prevent RGSMIIIS IRQs flood

[Serge Semin]

Without reading the GMAC_RGSMIIIS/MAC_PHYIF_Control_Status the IRQ line
won't be de-asserted causing interrupt handler executed over and over. As
a quick-fix let's just dummy-read the CSR for now.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 2 ++
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c    | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index adb872d5719f..2ae8467c588e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -304,6 +304,8 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
 	dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
 
 	if (intr_status & PCS_RGSMIIIS_IRQ) {
+		/* TODO Dummy-read to clear the IRQ status */
+		readl(ioaddr + GMAC_RGSMIIIS);
 		phylink_pcs_change(&hw->mac_pcs, false);
 		x->irq_rgmii_n++;
 	}
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index a892d361a4e4..cd2ca1d0222c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -896,6 +896,8 @@ static int dwmac4_irq_status(struct mac_device_info *hw,
 
 	dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
 	if (intr_status & PCS_RGSMIIIS_IRQ) {
+		/* TODO Dummy-read to clear the IRQ status */
+		readl(ioaddr + GMAC_PHYIF_CONTROL_STATUS);
 		phylink_pcs_change(&hw->mac_pcs, false);
 		x->irq_rgmii_n++;
 	}
-- 
2.43.0

[PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
into the DW GMAC controller. It's always done if the controller supports
at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
interfaces support was activated during the IP-core synthesize the PCS
block won't be activated either and the HWFEATURE.PCSSEL flag won't be
set. Based on that the RGMII in-band status detection procedure
implemented in the driver hasn't been working for the devices with the
RGMII interface support and with none of the SGMII, TBI, RTBI PHY
interfaces available in the device.

Fix that just by dropping the dma_cap.pcs flag check from the conditional
statement responsible for the In-band/PCS functionality activation. If the
RGMII interface is supported by the device then the in-band link status
detection will be also supported automatically (it's always embedded into
the RGMII RTL code). If the SGMII interface is supported by the device
then the PCS block will be supported too (it's unconditionally synthesized
into the controller). The later is also correct for the TBI/RTBI PHY
interfaces.

Note while at it drop the netdev_dbg() calls since at the moment of the
stmmac_check_pcs_mode() invocation the network device isn't registered. So
the debug prints will be for the unknown/NULL device.

Fixes: e58bb43f5e43 ("stmmac: initial support to manage pcs modes")
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 90c065920af2..6c4e90b1fea3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1146,18 +1146,10 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
 {
 	int interface = priv->plat->mac_interface;
 
-	if (priv->dma_cap.pcs) {
-		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
-		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
-		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
-		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
-			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
-			priv->hw->pcs = STMMAC_PCS_RGMII;
-		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
-			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
-			priv->hw->pcs = STMMAC_PCS_SGMII;
-		}
-	}
+	if (phy_interface_mode_is_rgmii(interface))
+		priv->hw.pcs = STMMAC_PCS_RGMII;
+	else if (interface == PHY_INTERFACE_MODE_SGMII)
+		priv->hw.pcs = STMMAC_PCS_SGMII;
 }
 
 /**
-- 
2.43.0

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> into the DW GMAC controller. It's always done if the controller supports
> at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> interfaces support was activated during the IP-core synthesize the PCS
> block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> set. Based on that the RGMII in-band status detection procedure
> implemented in the driver hasn't been working for the devices with the
> RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> interfaces available in the device.
> 
> Fix that just by dropping the dma_cap.pcs flag check from the conditional
> statement responsible for the In-band/PCS functionality activation. If the
> RGMII interface is supported by the device then the in-band link status
> detection will be also supported automatically (it's always embedded into
> the RGMII RTL code). If the SGMII interface is supported by the device
> then the PCS block will be supported too (it's unconditionally synthesized
> into the controller). The later is also correct for the TBI/RTBI PHY
> interfaces.
> 
> Note while at it drop the netdev_dbg() calls since at the moment of the
> stmmac_check_pcs_mode() invocation the network device isn't registered. So
> the debug prints will be for the unknown/NULL device.

Thanks. As this is a fix, shouldn't it be submitted for the net tree as
it seems to be fixing a bug in the driver as it stands today?

Also, a build fix is required here:

> -	if (priv->dma_cap.pcs) {
> -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> -			priv->hw->pcs = STMMAC_PCS_RGMII;
> -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> -			priv->hw->pcs = STMMAC_PCS_SGMII;
> -		}
> -	}
> +	if (phy_interface_mode_is_rgmii(interface))
> +		priv->hw.pcs = STMMAC_PCS_RGMII;
> +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> +		priv->hw.pcs = STMMAC_PCS_SGMII;

Both of these assignments should be priv->hw->pcs not priv->hw.pcs.

I think there's also another bug that needs fixing along with this.
See stmmac_ethtool_set_link_ksettings(). Note that this denies the
ability to disable autoneg, which (a) doesn't make sense for RGMII
with an attached PHY, and (b) this code should be passing the
ethtool op to phylink for it to pass on to phylib so the PHY can
be appropriately configured for the users desired autoneg and
link mode settings.

I also don't think it makes any sense for the STMMAC_PCS_SGMII case
given that it means Cisco SGMII - which implies that there is also
a PHY (since Cisco SGMII with inband is designed to be coupled with
something that looks like a PHY to send the inband signalling
necessary to configure e.g. the SGMII link symbol replication.

In both of these cases, even if the user requests autoneg to be
disabled, that _shouldn't_ affect internal network driver links.
This ethtool op is about configuring the externally visible media
side of the network driver, not the internal links.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > into the DW GMAC controller. It's always done if the controller supports
> > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > interfaces support was activated during the IP-core synthesize the PCS
> > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > set. Based on that the RGMII in-band status detection procedure
> > implemented in the driver hasn't been working for the devices with the
> > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > interfaces available in the device.
> > 
> > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > statement responsible for the In-band/PCS functionality activation. If the
> > RGMII interface is supported by the device then the in-band link status
> > detection will be also supported automatically (it's always embedded into
> > the RGMII RTL code). If the SGMII interface is supported by the device
> > then the PCS block will be supported too (it's unconditionally synthesized
> > into the controller). The later is also correct for the TBI/RTBI PHY
> > interfaces.
> > 
> > Note while at it drop the netdev_dbg() calls since at the moment of the
> > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > the debug prints will be for the unknown/NULL device.
> 
> Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> it seems to be fixing a bug in the driver as it stands today?
> 
> Also, a build fix is required here:
> 
> > -	if (priv->dma_cap.pcs) {
> > -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> > -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> > -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> > -			priv->hw->pcs = STMMAC_PCS_RGMII;
> > -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> > -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> > -			priv->hw->pcs = STMMAC_PCS_SGMII;
> > -		}
> > -	}
> > +	if (phy_interface_mode_is_rgmii(interface))
> > +		priv->hw.pcs = STMMAC_PCS_RGMII;
> > +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> > +		priv->hw.pcs = STMMAC_PCS_SGMII;
> 
> Both of these assignments should be priv->hw->pcs not priv->hw.pcs.
> 
> I think there's also another bug that needs fixing along with this.
> See stmmac_ethtool_set_link_ksettings(). Note that this denies the
> ability to disable autoneg, which (a) doesn't make sense for RGMII
> with an attached PHY, and (b) this code should be passing the
> ethtool op to phylink for it to pass on to phylib so the PHY can
> be appropriately configured for the users desired autoneg and
> link mode settings.
> 
> I also don't think it makes any sense for the STMMAC_PCS_SGMII case
> given that it means Cisco SGMII - which implies that there is also
> a PHY (since Cisco SGMII with inband is designed to be coupled with
> something that looks like a PHY to send the inband signalling
> necessary to configure e.g. the SGMII link symbol replication.
> 
> In both of these cases, even if the user requests autoneg to be
> disabled, that _shouldn't_ affect internal network driver links.
> This ethtool op is about configuring the externally visible media
> side of the network driver, not the internal links.

I have a concern about this patch. Have you considered dwmac-intel with
its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X
support. Does the dwmac-intel version of the core set
priv->dma_cap.pcs? If it doesn't, then removing the test on this will
cause a regression, since in Cisco SGMII mode, we end up setting
priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As
priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will
enable all the "integrated PCS" code paths despite XPCS clearly
intending to be used for Cisco SGMII.

I'm also wondering whether the same applies to the lynx PCS as well,
or in the general case if we have any kind of external PCS.

Hence, I think this probably needs to be:

	if (phy_interface_mode_is_rgmii(interface))
		priv->hw->pcs = STMMAC_PCS_RGMII;
	else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs)
		priv->hw->pcs = STMMAC_PCS_SGMII;

At least this is what unpicking the awful stmmac code suggests (and I
do feel that my point about the shocking state of this driver is proven
as details like this are extremely difficult to unpick, and not
unpicking them correctly will lead to regressions.) Therefore, I would
suggest that it would be wise if you also double-checked this.

If my analysis is correct, then my changes to stmmac_mac_select_pcs()
are also wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote:
> On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > > into the DW GMAC controller. It's always done if the controller supports
> > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > > interfaces support was activated during the IP-core synthesize the PCS
> > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > > set. Based on that the RGMII in-band status detection procedure
> > > implemented in the driver hasn't been working for the devices with the
> > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > > interfaces available in the device.
> > > 
> > > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > > statement responsible for the In-band/PCS functionality activation. If the
> > > RGMII interface is supported by the device then the in-band link status
> > > detection will be also supported automatically (it's always embedded into
> > > the RGMII RTL code). If the SGMII interface is supported by the device
> > > then the PCS block will be supported too (it's unconditionally synthesized
> > > into the controller). The later is also correct for the TBI/RTBI PHY
> > > interfaces.
> > > 
> > > Note while at it drop the netdev_dbg() calls since at the moment of the
> > > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > > the debug prints will be for the unknown/NULL device.
> > 
> > Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> > it seems to be fixing a bug in the driver as it stands today?
> > 
> > Also, a build fix is required here:
> > 
> > > -	if (priv->dma_cap.pcs) {
> > > -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> > > -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > > -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> > > -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > > -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> > > -			priv->hw->pcs = STMMAC_PCS_RGMII;
> > > -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> > > -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> > > -			priv->hw->pcs = STMMAC_PCS_SGMII;
> > > -		}
> > > -	}
> > > +	if (phy_interface_mode_is_rgmii(interface))
> > > +		priv->hw.pcs = STMMAC_PCS_RGMII;
> > > +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> > > +		priv->hw.pcs = STMMAC_PCS_SGMII;
> > 
> > Both of these assignments should be priv->hw->pcs not priv->hw.pcs.
> > 
> > I think there's also another bug that needs fixing along with this.
> > See stmmac_ethtool_set_link_ksettings(). Note that this denies the
> > ability to disable autoneg, which (a) doesn't make sense for RGMII
> > with an attached PHY, and (b) this code should be passing the
> > ethtool op to phylink for it to pass on to phylib so the PHY can
> > be appropriately configured for the users desired autoneg and
> > link mode settings.
> > 
> > I also don't think it makes any sense for the STMMAC_PCS_SGMII case
> > given that it means Cisco SGMII - which implies that there is also
> > a PHY (since Cisco SGMII with inband is designed to be coupled with
> > something that looks like a PHY to send the inband signalling
> > necessary to configure e.g. the SGMII link symbol replication.
> > 
> > In both of these cases, even if the user requests autoneg to be
> > disabled, that _shouldn't_ affect internal network driver links.
> > This ethtool op is about configuring the externally visible media
> > side of the network driver, not the internal links.
> 

> I have a concern about this patch. Have you considered dwmac-intel with
> its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X
> support. Does the dwmac-intel version of the core set
> priv->dma_cap.pcs? If it doesn't, then removing the test on this will
> cause a regression, since in Cisco SGMII mode, we end up setting
> priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As
> priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will
> enable all the "integrated PCS" code paths despite XPCS clearly
> intending to be used for Cisco SGMII.
> 
> I'm also wondering whether the same applies to the lynx PCS as well,
> or in the general case if we have any kind of external PCS.
> 
> Hence, I think this probably needs to be:
> 
> 	if (phy_interface_mode_is_rgmii(interface))
> 		priv->hw->pcs = STMMAC_PCS_RGMII;
> 	else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs)
> 		priv->hw->pcs = STMMAC_PCS_SGMII;
> 
> At least this is what unpicking the awful stmmac code suggests (and I
> do feel that my point about the shocking state of this driver is proven
> as details like this are extremely difficult to unpick, and not
> unpicking them correctly will lead to regressions.) Therefore, I would
> suggest that it would be wise if you also double-checked this.

Double-checked that part. Indeed this is what I forgot to take into
account. (Just realized I had a glimpse thought about checking the DW
xGMAC/XPCS for supporting the SGMII interface, but the thought got
away from my mind forgotten.) DW XPCS can be synthesized with having
the GMII/MII interface connected to the MAC and SGMII downstream
interface over a single 1000Base-X lane.

In anyway AFAICS that case has nothing to do with the PCS embedded
into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW
XGMAC has no embedded PCS, but could be attached to the separate DW
XPCS device.

About the correct implementation. Right, priv->dma_cap.pcs indicates
that there is an embedded PCS and the flag can be set for DW GMAC or DW
QoS Eth only. Although I would change the order:

       if (phy_interface_mode_is_rgmii(interface))
               priv->hw->pcs = STMMAC_PCS_RGMII;
       else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
               priv->hw->pcs = STMMAC_PCS_SGMII;

since priv->dma_cap.pcs is a primary flag. If it isn't set the
interface will be irrelevant.

Alternative solution could be to use the has_gmac/has_gmac4 flags
instead. That will emphasize that the embedded PCS is expected to be
specific for the DW GMAC and DW QoS Eth IP-cores:

       if (phy_interface_mode_is_rgmii(interface))
               priv->hw->pcs = STMMAC_PCS_RGMII;
       else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
		interface == PHY_INTERFACE_MODE_SGMII)
               priv->hw->pcs = STMMAC_PCS_SGMII;

-Serge(y)

> 
> If my analysis is correct, then my changes to stmmac_mac_select_pcs()
> are also wrong.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

On Tue, May 28, 2024 at 04:19:49PM +0300, Serge Semin wrote:
> On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote:
> > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > > > into the DW GMAC controller. It's always done if the controller supports
> > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > > > interfaces support was activated during the IP-core synthesize the PCS
> > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > > > set. Based on that the RGMII in-band status detection procedure
> > > > implemented in the driver hasn't been working for the devices with the
> > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > > > interfaces available in the device.
> > > > 
> > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > > > statement responsible for the In-band/PCS functionality activation. If the
> > > > RGMII interface is supported by the device then the in-band link status
> > > > detection will be also supported automatically (it's always embedded into
> > > > the RGMII RTL code). If the SGMII interface is supported by the device
> > > > then the PCS block will be supported too (it's unconditionally synthesized
> > > > into the controller). The later is also correct for the TBI/RTBI PHY
> > > > interfaces.
> > > > 
> > > > Note while at it drop the netdev_dbg() calls since at the moment of the
> > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > > > the debug prints will be for the unknown/NULL device.
> > > 
> > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> > > it seems to be fixing a bug in the driver as it stands today?
> > > 
> > > Also, a build fix is required here:
> > > 
> > > > -	if (priv->dma_cap.pcs) {
> > > > -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> > > > -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > > > -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> > > > -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > > > -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> > > > -			priv->hw->pcs = STMMAC_PCS_RGMII;
> > > > -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> > > > -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> > > > -			priv->hw->pcs = STMMAC_PCS_SGMII;
> > > > -		}
> > > > -	}
> > > > +	if (phy_interface_mode_is_rgmii(interface))
> > > > +		priv->hw.pcs = STMMAC_PCS_RGMII;
> > > > +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> > > > +		priv->hw.pcs = STMMAC_PCS_SGMII;
> > > 
> > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs.
> > > 
> > > I think there's also another bug that needs fixing along with this.
> > > See stmmac_ethtool_set_link_ksettings(). Note that this denies the
> > > ability to disable autoneg, which (a) doesn't make sense for RGMII
> > > with an attached PHY, and (b) this code should be passing the
> > > ethtool op to phylink for it to pass on to phylib so the PHY can
> > > be appropriately configured for the users desired autoneg and
> > > link mode settings.
> > > 
> > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case
> > > given that it means Cisco SGMII - which implies that there is also
> > > a PHY (since Cisco SGMII with inband is designed to be coupled with
> > > something that looks like a PHY to send the inband signalling
> > > necessary to configure e.g. the SGMII link symbol replication.
> > > 
> > > In both of these cases, even if the user requests autoneg to be
> > > disabled, that _shouldn't_ affect internal network driver links.
> > > This ethtool op is about configuring the externally visible media
> > > side of the network driver, not the internal links.
> > 
> 
> > I have a concern about this patch. Have you considered dwmac-intel with
> > its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X
> > support. Does the dwmac-intel version of the core set
> > priv->dma_cap.pcs? If it doesn't, then removing the test on this will
> > cause a regression, since in Cisco SGMII mode, we end up setting
> > priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As
> > priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will
> > enable all the "integrated PCS" code paths despite XPCS clearly
> > intending to be used for Cisco SGMII.
> > 
> > I'm also wondering whether the same applies to the lynx PCS as well,
> > or in the general case if we have any kind of external PCS.
> > 
> > Hence, I think this probably needs to be:
> > 
> > 	if (phy_interface_mode_is_rgmii(interface))
> > 		priv->hw->pcs = STMMAC_PCS_RGMII;
> > 	else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs)
> > 		priv->hw->pcs = STMMAC_PCS_SGMII;
> > 
> > At least this is what unpicking the awful stmmac code suggests (and I
> > do feel that my point about the shocking state of this driver is proven
> > as details like this are extremely difficult to unpick, and not
> > unpicking them correctly will lead to regressions.) Therefore, I would
> > suggest that it would be wise if you also double-checked this.
> 
> Double-checked that part. Indeed this is what I forgot to take into
> account.

Thanks for double-checking it.

> (Just realized I had a glimpse thought about checking the DW
> xGMAC/XPCS for supporting the SGMII interface, but the thought got
> away from my mind forgotten.) DW XPCS can be synthesized with having
> the GMII/MII interface connected to the MAC and SGMII downstream
> interface over a single 1000Base-X lane.
> 
> In anyway AFAICS that case has nothing to do with the PCS embedded
> into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW
> XGMAC has no embedded PCS, but could be attached to the separate DW
> XPCS device.

This is where my head starts spinning, because identifying what
"DW GMAC" and "DW QoS Eth" refer to is difficult unless one, I guess,
has the documentation.

The only references to QoS that I can find in the driver refer to
per-DMA channel interrupts, dwmac5* and one mention for a platform
driver in the Kconfig.

Grepping for "DW GMAC" doesn't give anything.

Conversely, I know from the code that only dwmac4 and dwmac1000
have support for the integrated PCS. So trying to put this together
doesn't make much sense to me. :/

Maybe "DW QoS Eth" refers to dwmac-dwc-qos-eth.c?

> About the correct implementation. Right, priv->dma_cap.pcs indicates
> that there is an embedded PCS and the flag can be set for DW GMAC or DW
> QoS Eth only. Although I would change the order:
> 
>        if (phy_interface_mode_is_rgmii(interface))
>                priv->hw->pcs = STMMAC_PCS_RGMII;
>        else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
>                priv->hw->pcs = STMMAC_PCS_SGMII;
> 
> since priv->dma_cap.pcs is a primary flag. If it isn't set the
> interface will be irrelevant.

As this is generic code, it probably makes sense to go with that, since
priv->dma_cap.pcs indicates whether the internal PCS for SGMII is
present or not rather than...

> Alternative solution could be to use the has_gmac/has_gmac4 flags
> instead. That will emphasize that the embedded PCS is expected to be
> specific for the DW GMAC and DW QoS Eth IP-cores:
> 
>        if (phy_interface_mode_is_rgmii(interface))
>                priv->hw->pcs = STMMAC_PCS_RGMII;
>        else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
> 		interface == PHY_INTERFACE_MODE_SGMII)
>                priv->hw->pcs = STMMAC_PCS_SGMII;

which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c)
will always have its internal PCS if we're using SGMII mode. Does this
mean it is true that these cores will never be used with an external
PCS?

If there is a hardware flag that indicates the PCS is implemented, then
I think using that to gate whether SGMII uses the internal PCS is
better rather than using the core type.

Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS)
is being used, the internal PCS will not have been synthesized, and
thus priv->dma_cap.pcs will be false? The reason I'd like to know
this is because in the future, I'd like to eliminate priv->hw->pcs,
and just have dwmac1000/dwmac4's phylink_select_pcs() method make
the decisions.

If not, then we need to think about the behaviour that
stmmac_mac_select_pcs(0 should have. Should it give priority to the
internal PCS over external PCS, or external PCS first (in which case
what do we need to do with the internal PCS.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote:
> > Alternative solution could be to use the has_gmac/has_gmac4 flags
> > instead. That will emphasize that the embedded PCS is expected to be
> > specific for the DW GMAC and DW QoS Eth IP-cores:
> > 
> >        if (phy_interface_mode_is_rgmii(interface))
> >                priv->hw->pcs = STMMAC_PCS_RGMII;
> >        else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
> > 		interface == PHY_INTERFACE_MODE_SGMII)
> >                priv->hw->pcs = STMMAC_PCS_SGMII;
> 
> which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c)
> will always have its internal PCS if we're using SGMII mode. Does this
> mean it is true that these cores will never be used with an external
> PCS?

Sorry to go off on a related tangent, but I've just been looking at
hw->ps which is related to this.

As I understand, hw->ps comes from the "snps,ps-speed" property in DT,
which is used for SGMII and MAC2MAC connections. Presumably for the
SGMII case, this is used where the port is made to look like the PHY
end of the SGMII link.

I'm guessing MAC2MAC refers to RGMII, or does that also refer to
SGMII-as-PHY?

I think it would've been nice to have picked SGMII-as-PHY up in the
driver earlier - we don't tend to use the "normal" PHY interface
mode names, instead we have the REVxxx modes, so I think this
_should_ have introduced PHY_INTERFACE_MODE_REVSGMII.

In any case, moving on... in stmmac_hw_setup(), we have:

        /* PS and related bits will be programmed according to the speed */
        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;
                }
        }

Which means that if we're using the integrated PCS, then we basically
require the "snps,ps-speed" property otherwise we'll issue a warning
at this point... this seems to imply that reverse mode is the only
mode supported, which I'm fairly sure is false. So, maybe this
shouldn't be issuing the warning if mac_port_sel_speed was zero?

Moving on... hw->ps can only be 10M, 100M or 1G speeds and nothing else
- which is fine since RGMII and Cisco SGMII only support these speeds.

dwmac1000 tests for this against these speeds, so it is also fine.

dwmac4 is basically the same as dwmac1000, so is also fine.

The core code as it stands today passes this into the pcs_ctrl_ane
method's rsgmi_ral argument, which sets GMAC_AN_CTRL_SGMRAL. Presumably
this selects "reverse" mode for both SGMII and RGMII?

Persuing this a bit futher, qcom-ethqos always calls this with rsgmi_ral
clear. Presumably, qcom-ethqos never specifies "snps,ps-speed" in DT,
and thus always gets the warning above?

Finally, we get to the core issue, which is dwxgmac2_core.c.
dwxgmac2 tests this member against 10G, 2.5G and "any other non-zero
value". Out of all of these, the only possible path through that code
would be the one which results in:

	tx |= hw->link.speed1000;

Neither of the other two (2.5G and 10G) are possible because those
aren't legal values for hw->ps. Moreover, it doesn't appear to have
any kind of PCS, so I'm wondering whether any of this code gets used.


So, I suspect some of this is "not quite right" either, and I wonder
about the implications of changing how hw->pcs is set - whether we
first need to fix the code above dealing with priv->hw->ps ?

I'm also wondering what impact this has on my PCS conversion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

On Tue, May 28, 2024 at 05:21:44PM +0100, Russell King (Oracle) wrote:
> On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote:
> > > Alternative solution could be to use the has_gmac/has_gmac4 flags
> > > instead. That will emphasize that the embedded PCS is expected to be
> > > specific for the DW GMAC and DW QoS Eth IP-cores:
> > > 
> > >        if (phy_interface_mode_is_rgmii(interface))
> > >                priv->hw->pcs = STMMAC_PCS_RGMII;
> > >        else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
> > > 		interface == PHY_INTERFACE_MODE_SGMII)
> > >                priv->hw->pcs = STMMAC_PCS_SGMII;
> > 
> > which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c)
> > will always have its internal PCS if we're using SGMII mode. Does this
> > mean it is true that these cores will never be used with an external
> > PCS?
> 

> Sorry to go off on a related tangent, but I've just been looking at
> hw->ps which is related to this.

I was meditating around the hw->ps part for several days on the last
week and just gave up in finding of how that semantics could be
incorporated in the phylink pcs logic...

> 
> As I understand, hw->ps comes from the "snps,ps-speed" property in DT,
> which is used for SGMII and MAC2MAC connections. Presumably for the
> SGMII case, this is used where the port is made to look like the PHY
> end of the SGMII link.

Right. The speed comes from the "snps,ps-speed" property and is
utilized to set the particular port speed in the MAC2MAC case. But
neither DW QoS Eth nor DW GMAC HW-manual explicitly describe that
case. The only SGMII MAC2MAC mention there is GMAC_AN_CTRL_SGMRAL flag
description:

"SGMII RAL Control

When set, this bit forces the SGMII RAL block to operate in the speed
configured in the Speed and Port Select bits of the MAC Configuration
register. This is useful when the SGMII interface is used in a direct
MAC to 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 SGMII (from the PHY).

This bit is reserved (and RO) if the SGMII PHY interface is not
selected during core configuration."

> 
> I'm guessing MAC2MAC refers to RGMII, or does that also refer to
> SGMII-as-PHY?

I guess that it can be utilized in both cases: RGMII-to-RGMII and
SGMII-to-SGMII MAC2MAC setups. The only difference is that the
GMAC_AN_CTRL_SGMRAL flag setting would be useless for RGMII. But
originally the mac_device_info::ps field was introduced for the SGMII
MAC2MAC config here:
02e57b9d7c8c ("drivers: net: stmmac: add port selection programming")
and the "snps,ps-speed" property can be spotted alongside with 
phy-mode = "sgmii" only, here:
arch/arm64/boot/dts/qcom/sa8775p-ride.dts

Although AFAICS the dwmac1000_core_init()/dwmac4_core_init() methods
lack of the GMAC_CONTROL_TC/GMAC_PHYIF_CTRLSTATUS_TC flags set in the
(hw->ps)-related if-clause. Without that the specified speed setting
won't be in-bend delivered to the other side of the MAC2MAC link and
the internal PCS functionality won't work. Synopsys DW GMAC/Qos Eth
databooks explicitly say that these flags need to be set for the MAC
to be sending its Port speed, Duplex mode and Link Up/Down flag
setting over the RGMII/SGMII in-band signal:

SGMII: "The tx_config_reg[15:0] bits sent by the MAC during
Auto-negotiation depend on whether the Transmit Configuration register
bit is enabled for the SGMII interface."

RGMII: "When the RGMII interface is configured to transmit the
configuration during the IFG, then rgmii_txd[3:0] reflects the Duplex
Mode, Port Select, Speed (encoded as 00 for 10 Mbps, 01 for 100 Mbps
and 10 for 1000 Mbps), and Link Up/Down bits of the MAC Configuration
Register,"

TC flag description:
"Transmit Configuration in RGMII, SGMII, or SMII

When set, this bit enables the transmission of duplex mode, link
speed, and link up or down information to the PHY in the RGMII, SMII,
or SGMII port. When this bit is reset, no such information is driven
to the PHY. This bit is reserved (and RO) if the RGMII, SMII, or SGMII
PHY port is not selected during core configuration."

> 
> I think it would've been nice to have picked SGMII-as-PHY up in the
> driver earlier - we don't tend to use the "normal" PHY interface
> mode names, instead we have the REVxxx modes, so I think this
> _should_ have introduced PHY_INTERFACE_MODE_REVSGMII.

Not sure whether it would be a correct thing to do. RevMII is a real
interface. DW GMAC/QoS Eth can be synthesized with RevMII PHY
interface support. Mac2Mac SGMII/RGMII is a feature of the standard
SGMII/RGMII interfaces.

On the other hand we already have the set of the artificial modes like
"rgmii-id/rgmii-txid/rgmii-rxid" indicating the MAC-side delays but
describing the same interfaces. So I don't have a strong opinion
against have the modes like "rev-rgmii"/"rev-sgmii".

> 
> In any case, moving on... in stmmac_hw_setup(), we have:
> 
>         /* PS and related bits will be programmed according to the speed */
>         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;
>                 }
>         }
> 

> Which means that if we're using the integrated PCS, then we basically
> require the "snps,ps-speed" property otherwise we'll issue a warning
> at this point... this seems to imply that reverse mode is the only
> mode supported, which I'm fairly sure is false. So, maybe this
> shouldn't be issuing the warning if mac_port_sel_speed was zero?

Seeing the link state could be delivered over the in-band path, I
guess the "snps,ps-speed" property is supposed to be optional so the
mac_port_sel_speed being zero is a possible case. Thus the warning is
indeed misleading and it is totally ok to have mac_port_sel_speed
being set to zero. If it is, then the link state shall be determined
either over in-band or from the PHY.

> 
> Moving on... hw->ps can only be 10M, 100M or 1G speeds and nothing else
> - which is fine since RGMII and Cisco SGMII only support these speeds.
> 
> dwmac1000 tests for this against these speeds, so it is also fine.
> 
> dwmac4 is basically the same as dwmac1000, so is also fine.
> 
> The core code as it stands today passes this into the pcs_ctrl_ane
> method's rsgmi_ral argument, which sets GMAC_AN_CTRL_SGMRAL. Presumably
> this selects "reverse" mode for both SGMII and RGMII?

No, GMAC_AN_CTRL_SGMRAL flag works for SGMII only, which enables the
fixed link speed (see my second comment in this email message) by
forcing the SGMII RAL (Rate Adaptation Layer) working with the
pre-defined speed. AFAIU RGMII interface doesn't need that flag since
it always works with the pre-defined speed and has no Rate Adaptation
engine.

> 
> Persuing this a bit futher, qcom-ethqos always calls this with rsgmi_ral
> clear. Presumably, qcom-ethqos never specifies "snps,ps-speed" in DT,
> and thus always gets the warning above?

Interesting situation. Actually no. The only DW QoS Eth device for
which "snps,ps-speed = 1000" is specified is "qcom,sa8775p-ethqosi"
(see arch/arm64/boot/dts/qcom/sa8775p-ride.dts), due to that no
warning is printed. But on the other hand the low-level driver
(drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c) also sets
the STMMAC_FLAG_HAS_INTEGRATED_PCS flag exactly for that device, which
effectively disables the entire internal PCS functionality (except the
speed setup performed in dwmac4_core_init()).

Holy mother of ...

> 
> Finally, we get to the core issue, which is dwxgmac2_core.c.
> dwxgmac2 tests this member against 10G, 2.5G and "any other non-zero
> value". Out of all of these, the only possible path through that code
> would be the one which results in:
> 
> 	tx |= hw->link.speed1000;
> 
> Neither of the other two (2.5G and 10G) are possible because those
> aren't legal values for hw->ps. Moreover, it doesn't appear to have
> any kind of PCS, so I'm wondering whether any of this code gets used.

I guess the (hw->ps)-related code snippet has been just dummy-copied from
another dwmac*_core.c file to DW XGMAC. So IMO it can be freely
dropped. After all the bindings define the snps,ps-speed as:

      "Port selection speed that can be passed to the core when PCS
      is supported. For example, this is used in case of SGMII and
      MAC2MAC connection."

I doubt DW XGMAC could be used in the MAC2MAC setup, and it doesn't
have any internal PCS (may have externally connected DW XPCS though).

> 
> 
> So, I suspect some of this is "not quite right" either, and I wonder
> about the implications of changing how hw->pcs is set - whether we
> first need to fix the code above dealing with priv->hw->ps ?
> 
> I'm also wondering what impact this has on my PCS conversion.

My brain got blown up thinking about this one week ago. So I gave up
in looking for a portable way of fixing the MAC2MAC part and sent my
three patches as is to you. I thought after some time I could come up
with some ideas about that. Alas the time-break didn't help.)

I can't say for sure what could be a better way to align the things
around the internal PCS and MAC2MAC case. But IMO seeing the code is
vastly messy and unlikely has been widely used I'd suggest to preserve
the semantics as required by the Qualcomm QoS Eth
(dwmac-qcom-ethqos.c), and free redefining the rest of the
things as you wish.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote:
> On Tue, May 28, 2024 at 04:19:49PM +0300, Serge Semin wrote:
> > On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote:
> > > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> > > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > > > > into the DW GMAC controller. It's always done if the controller supports
> > > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > > > > interfaces support was activated during the IP-core synthesize the PCS
> > > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > > > > set. Based on that the RGMII in-band status detection procedure
> > > > > implemented in the driver hasn't been working for the devices with the
> > > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > > > > interfaces available in the device.
> > > > > 
> > > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > > > > statement responsible for the In-band/PCS functionality activation. If the
> > > > > RGMII interface is supported by the device then the in-band link status
> > > > > detection will be also supported automatically (it's always embedded into
> > > > > the RGMII RTL code). If the SGMII interface is supported by the device
> > > > > then the PCS block will be supported too (it's unconditionally synthesized
> > > > > into the controller). The later is also correct for the TBI/RTBI PHY
> > > > > interfaces.
> > > > > 
> > > > > Note while at it drop the netdev_dbg() calls since at the moment of the
> > > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > > > > the debug prints will be for the unknown/NULL device.
> > > > 
> > > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> > > > it seems to be fixing a bug in the driver as it stands today?
> > > > 
> > > > Also, a build fix is required here:
> > > > 
> > > > > -	if (priv->dma_cap.pcs) {
> > > > > -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> > > > > -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > > > > -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> > > > > -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > > > > -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> > > > > -			priv->hw->pcs = STMMAC_PCS_RGMII;
> > > > > -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> > > > > -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> > > > > -			priv->hw->pcs = STMMAC_PCS_SGMII;
> > > > > -		}
> > > > > -	}
> > > > > +	if (phy_interface_mode_is_rgmii(interface))
> > > > > +		priv->hw.pcs = STMMAC_PCS_RGMII;
> > > > > +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> > > > > +		priv->hw.pcs = STMMAC_PCS_SGMII;
> > > > 
> > > > Both of these assignments should be priv->hw->pcs not priv->hw.pcs.
> > > > 
> > > > I think there's also another bug that needs fixing along with this.
> > > > See stmmac_ethtool_set_link_ksettings(). Note that this denies the
> > > > ability to disable autoneg, which (a) doesn't make sense for RGMII
> > > > with an attached PHY, and (b) this code should be passing the
> > > > ethtool op to phylink for it to pass on to phylib so the PHY can
> > > > be appropriately configured for the users desired autoneg and
> > > > link mode settings.
> > > > 
> > > > I also don't think it makes any sense for the STMMAC_PCS_SGMII case
> > > > given that it means Cisco SGMII - which implies that there is also
> > > > a PHY (since Cisco SGMII with inband is designed to be coupled with
> > > > something that looks like a PHY to send the inband signalling
> > > > necessary to configure e.g. the SGMII link symbol replication.
> > > > 
> > > > In both of these cases, even if the user requests autoneg to be
> > > > disabled, that _shouldn't_ affect internal network driver links.
> > > > This ethtool op is about configuring the externally visible media
> > > > side of the network driver, not the internal links.
> > > 
> > 
> > > I have a concern about this patch. Have you considered dwmac-intel with
> > > its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X
> > > support. Does the dwmac-intel version of the core set
> > > priv->dma_cap.pcs? If it doesn't, then removing the test on this will
> > > cause a regression, since in Cisco SGMII mode, we end up setting
> > > priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As
> > > priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will
> > > enable all the "integrated PCS" code paths despite XPCS clearly
> > > intending to be used for Cisco SGMII.
> > > 
> > > I'm also wondering whether the same applies to the lynx PCS as well,
> > > or in the general case if we have any kind of external PCS.
> > > 
> > > Hence, I think this probably needs to be:
> > > 
> > > 	if (phy_interface_mode_is_rgmii(interface))
> > > 		priv->hw->pcs = STMMAC_PCS_RGMII;
> > > 	else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs)
> > > 		priv->hw->pcs = STMMAC_PCS_SGMII;
> > > 
> > > At least this is what unpicking the awful stmmac code suggests (and I
> > > do feel that my point about the shocking state of this driver is proven
> > > as details like this are extremely difficult to unpick, and not
> > > unpicking them correctly will lead to regressions.) Therefore, I would
> > > suggest that it would be wise if you also double-checked this.
> > 
> > Double-checked that part. Indeed this is what I forgot to take into
> > account.
> 
> Thanks for double-checking it.
> 
> > (Just realized I had a glimpse thought about checking the DW
> > xGMAC/XPCS for supporting the SGMII interface, but the thought got
> > away from my mind forgotten.) DW XPCS can be synthesized with having
> > the GMII/MII interface connected to the MAC and SGMII downstream
> > interface over a single 1000Base-X lane.
> > 
> > In anyway AFAICS that case has nothing to do with the PCS embedded
> > into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW
> > XGMAC has no embedded PCS, but could be attached to the separate DW
> > XPCS device.
> 

> This is where my head starts spinning, because identifying what
> "DW GMAC" and "DW QoS Eth" refer to is difficult unless one, I guess,
> has the documentation.
> 
> The only references to QoS that I can find in the driver refer to
> per-DMA channel interrupts, dwmac5* and one mention for a platform
> driver in the Kconfig.
> 
> Grepping for "DW GMAC" doesn't give anything.
> 
> Conversely, I know from the code that only dwmac4 and dwmac1000
> have support for the integrated PCS. So trying to put this together
> doesn't make much sense to me. :/
> 
> Maybe "DW QoS Eth" refers to dwmac-dwc-qos-eth.c?

DW QoS Eth is the new generation of the Synopsys Gigabit Ethernet
IP-cores. Old ones are considered of version 3.74a and older:
https://www.synopsys.com/dw/ipdir.php?ds=dwc_ether_mac10_100_1000_unive
The new ones are of the version 4.0 and higher (the most modern
DW QoS Eth IP-core is of v5.40a):
https://www.synopsys.com/dw/ipdir.php?ds=dwc_ether_qos

This is better summarised in the driver doc:

Documentation/networking/device_drivers/ethernet/stmicro/stmmac.rst

which has outdated a bit, but the summary table looks correct anyway:

+-------------------------------+--------------+--------------+--------------+
| Controller Name               | Min. Version | Max. Version | Abbrev. Name |
+===============================+==============+==============+==============+
| Ethernet MAC Universal        | N/A          | 3.73a        | GMAC         |
+-------------------------------+--------------+--------------+--------------+
| Ethernet Quality-of-Service   | 4.00a        | N/A          | GMAC4+       |
+-------------------------------+--------------+--------------+--------------+
| XGMAC - 10G Ethernet MAC      | 2.10a        | N/A          | XGMAC2+      |
+-------------------------------+--------------+--------------+--------------+
| XLGMAC - 100G Ethernet MAC    | 2.00a        | N/A          | XLGMAC2+     |
+-------------------------------+--------------+--------------+--------------+

See the abbreviation and controller names. When I say just DW GMAC
then it means DW Ether MAC 10/100/1000 Universal, which driver is
implemented in the dwmac1000* files. If you see DW GMAC4/GMAC5 or DW
GAC4+ or DW QoE Eth, then it means DW Ethernet Quality-of-Service
IP-core, which driver could be found in dwmac4*/dwmac5* files.

As it inferable from the IP-core names the main difference between DW
Ether MAC 10/100/1000 Universal and DW Ethernet Quality-of-Service is
that the later one supports multiple queues and channels with a
comprehensive list of the optional traffic scheduling features (FPE,
TBS, DCB, AV-bridging, etc). DW GMAC doesn't have as many such
features. The only way to have DW GMAC synthesized with the multiple
DMA channels support is to enable a singly available traffic
scheduling feature - AV-bridging. Note AV-bridging enabled on the DW
GMAC v3.73a is the case of the Loongson GNET controller, which support
is implemented in the Yanteng Si patchset recently submitted for v13
review:
https://lore.kernel.org/netdev/cover.1716973237.git.siyanteng@loongson.cn/

In some extent the CSRs mapping is also different in DW GMAC v3.x and
GMAC v4.x/v5.x, but the main part is in the QoS features.

> 
> > About the correct implementation. Right, priv->dma_cap.pcs indicates
> > that there is an embedded PCS and the flag can be set for DW GMAC or DW
> > QoS Eth only. Although I would change the order:
> > 
> >        if (phy_interface_mode_is_rgmii(interface))
> >                priv->hw->pcs = STMMAC_PCS_RGMII;
> >        else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
> >                priv->hw->pcs = STMMAC_PCS_SGMII;
> > 
> > since priv->dma_cap.pcs is a primary flag. If it isn't set the
> > interface will be irrelevant.
> 

> As this is generic code, it probably makes sense to go with that, since
> priv->dma_cap.pcs indicates whether the internal PCS for SGMII is
> present or not rather than...

Right.

> 
> > Alternative solution could be to use the has_gmac/has_gmac4 flags
> > instead. That will emphasize that the embedded PCS is expected to be
> > specific for the DW GMAC and DW QoS Eth IP-cores:
> > 
> >        if (phy_interface_mode_is_rgmii(interface))
> >                priv->hw->pcs = STMMAC_PCS_RGMII;
> >        else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
> > 		interface == PHY_INTERFACE_MODE_SGMII)
> >                priv->hw->pcs = STMMAC_PCS_SGMII;
> 

> which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c)
> will always have its internal PCS if we're using SGMII mode.

Right. If the DW GMAC/QoS Eth IP-core is synthesized with the
SGMII/RTBI/RBI PHY interface then the internal PCS will always be
available and the HWFEATURE.PCSSEL flag will be set. Here is the
PCSSEL flag value definition:
DW QoS Eth: DWC_EQOS_PCS_EN = DWC_EQOS_TBI_EN || DWC_EQOS_SGMII_EN || DWC_EQOS_RTBI_EN
DW GMAC: if TBI, SGMII, or RTBI PHY interface is enabled.

> Does this
> mean it is true that these cores will never be used with an external
> PCS?

Sorry, I was wrong to suggest the (priv->plat.has_gmac ||
priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW
QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X
downstream interface. Not sure why it was needed to implement that way
seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of
box, but AFAICS Intel mGBE is that case. Anyway the correct way to
detect the internal PCS support is to check the PCSSEL flag set in the
HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field).

> 
> If there is a hardware flag that indicates the PCS is implemented, then
> I think using that to gate whether SGMII uses the internal PCS is
> better rather than using the core type.

Right.

> 
> Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS)
> is being used, the internal PCS will not have been synthesized, and
> thus priv->dma_cap.pcs will be false?

Alas I can't confirm that. priv->dma_cap.pcs only indicates the
internal PCS availability. External PCS is an independent entity from
the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC
controllers aren't aware of its existence. It's the low-level platform
driver/code responsibility to somehow detect it being available
("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc).

Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is
synthesized with the SGMII/TBI/RTBI PHY interface support
priv->dma_cap.pcs will get to be true. Note the device can be
synthesized with several PHY interfaces supported. As long as
SGMII/TBI/RTBI PHY interface is any of them, the flag will be set
irrespective from the PHY interface activated at runtime. 

> The reason I'd like to know
> this is because in the future, I'd like to eliminate priv->hw->pcs,
> and just have dwmac1000/dwmac4's phylink_select_pcs() method make
> the decisions.

You can extend the priv->dma_cap.pcs flag semantics. So it could
be indicating three types of the PCS'es:
RGMII, SGMII, XPCS (or TBI/RTBI in future).

> 
> If not, then we need to think about the behaviour that
> stmmac_mac_select_pcs(0 should have. Should it give priority to the
> internal PCS over external PCS, or external PCS first (in which case
> what do we need to do with the internal PCS.)

I guess the DW XPCS implementation might be more preferable. From one
side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW
GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other
hand the DW XPCS might be available over the MDIO-bus, which is slower
to access than the internal PCS CSRs available in the DW GMAC/QoS Eth
CSRs space. So the more performant link speed seems more useful
feature over the faster device setup process.

One thing I am not sure about is that there is a real case of having
the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY
interface support and being attached to the DW XPCS controller, which
would have the SGMII downstream PHY interface. DW XPCS has only the
XGMII or GMII/MII upstream interfaces over which the MAC can be
attached. So DW GMAC/QoS Eth and DW XPCS can be connected via the
GMII/MII interface only. Regarding Intel mGBE, it likely is having a
setup like this:

+------------+          +---------+
|            | GMII/MII |         |   SGMII
| DW QoS Eth +----------+ DW XPCS +------------
|            |          |         | 1000Base-X
+------------+          +---------+

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

Hi Serge,

Thanks for the reply. I've attempted to deal with most of these in my
v2 posting, but maybe not in the best way yet.

On Fri, May 31, 2024 at 08:13:49PM +0300, Serge Semin wrote:
> > Does this
> > mean it is true that these cores will never be used with an external
> > PCS?
> 
> Sorry, I was wrong to suggest the (priv->plat.has_gmac ||
> priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW
> QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X
> downstream interface. Not sure why it was needed to implement that way
> seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of
> box, but AFAICS Intel mGBE is that case. Anyway the correct way to
> detect the internal PCS support is to check the PCSSEL flag set in the
> HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field).

We can only wonder why!

> > Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS)
> > is being used, the internal PCS will not have been synthesized, and
> > thus priv->dma_cap.pcs will be false?
> 
> Alas I can't confirm that. priv->dma_cap.pcs only indicates the
> internal PCS availability. External PCS is an independent entity from
> the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC
> controllers aren't aware of its existence. It's the low-level platform
> driver/code responsibility to somehow detect it being available
> ("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc).
> 
> Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is
> synthesized with the SGMII/TBI/RTBI PHY interface support
> priv->dma_cap.pcs will get to be true. Note the device can be
> synthesized with several PHY interfaces supported. As long as
> SGMII/TBI/RTBI PHY interface is any of them, the flag will be set
> irrespective from the PHY interface activated at runtime. 

I've been debating about this, and given your response, I'm wondering
whether we should change stmmac_mac_select_pcs() to instead do:

static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
						 phy_interface_t interface)
{
	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
	struct phylink_pcs *pcs;

	if (priv->plat->select_pcs) {
		pcs = priv->plat->select_pcs(priv, interface);
		if (!IS_ERR(pcs))
			return pcs;
	}

	return stmmac_mac_phylink_select_pcs(priv, interface);
}

and push the problem of whether to provide a PCS that overrides
the MAC internal PCS into platform code. That would mean Intel mGBE
would be able to override with XPCS. rzn1 and socfpga can then do
their own thing as well.

I'm trying hard not to go down another rabbit hole... I've just
spotted that socfpga sets mac_interface to PHY_INTERFACE_MODE_SGMII.
That's another reason for pushing this down into platform drivers -
if platform drivers are doing weird stuff, then we can contain their
weirdness in the platform drivers moving it out of the core code.

> You can extend the priv->dma_cap.pcs flag semantics. So it could
> be indicating three types of the PCS'es:
> RGMII, SGMII, XPCS (or TBI/RTBI in future).

If TBI/RTBI gets supported, then this would have to be extended, but I
get the impression that this isn't popular.

> I guess the DW XPCS implementation might be more preferable. From one
> side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW
> GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other
> hand the DW XPCS might be available over the MDIO-bus, which is slower
> to access than the internal PCS CSRs available in the DW GMAC/QoS Eth
> CSRs space. So the more performant link speed seems more useful
> feature over the faster device setup process.

I think which should be used would depend on how the hardware is wired
up. This brings us back to platform specifics again, which points
towards moving the decision making into platform code as per the above.

> One thing I am not sure about is that there is a real case of having
> the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY
> interface support and being attached to the DW XPCS controller, which
> would have the SGMII downstream PHY interface. DW XPCS has only the
> XGMII or GMII/MII upstream interfaces over which the MAC can be
> attached.

That gives us another possibility, but needs platforms to be doing
the right thing. If mac_interface were set to XGMII or GMII/MII, then
that would exclude the internal MAC PCS.

> So DW GMAC/QoS Eth and DW XPCS can be connected via the
> GMII/MII interface only. Regarding Intel mGBE, it likely is having a
> setup like this:
> 
> +------------+          +---------+
> |            | GMII/MII |         |   SGMII
> | DW QoS Eth +----------+ DW XPCS +------------
> |            |          |         | 1000Base-X
> +------------+          +---------+


So as an alternative, 

     mac_interface            phy_interface

     XGMII/GMII/MII           SGMII/1000Base-X
MAC ---------------- DW XPCS ------------------

     INTERNAL                SGMII/TBI/RTBI
MAC ---------- Internal PCS ----------------

     INTERNAL                  RGMII
MAC ---------- Internal "PCS" --------------

One of the problems here, though, is socfpga. It uses mac_interface
with RGMII*, MII, GMII, SGMII and RMII. I think it's confusing
mac_interface for phy_interface, but I haven't read through enough
of it to be certain.

So that again leads me back to my proposal above for
stmmac_mac_select_pcs() as the least likely to break proposition -
at least given how things are at the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

Hi Russel

On Fri, May 31, 2024 at 08:30:27PM +0100, Russell King (Oracle) wrote:
> Hi Serge,
> 
> Thanks for the reply. I've attempted to deal with most of these in my
> v2 posting, but maybe not in the best way yet.

I've got your v2 series. I'll have a look at it and test it out later
on the next week, sometime around Wednesday.

> 
> On Fri, May 31, 2024 at 08:13:49PM +0300, Serge Semin wrote:
> > > Does this
> > > mean it is true that these cores will never be used with an external
> > > PCS?
> > 
> > Sorry, I was wrong to suggest the (priv->plat.has_gmac ||
> > priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW
> > QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X
> > downstream interface. Not sure why it was needed to implement that way
> > seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of
> > box, but AFAICS Intel mGBE is that case. Anyway the correct way to
> > detect the internal PCS support is to check the PCSSEL flag set in the
> > HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field).
> 
> We can only wonder why!
> 
> > > Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS)
> > > is being used, the internal PCS will not have been synthesized, and
> > > thus priv->dma_cap.pcs will be false?
> > 
> > Alas I can't confirm that. priv->dma_cap.pcs only indicates the
> > internal PCS availability. External PCS is an independent entity from
> > the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC
> > controllers aren't aware of its existence. It's the low-level platform
> > driver/code responsibility to somehow detect it being available
> > ("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc).
> > 
> > Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is
> > synthesized with the SGMII/TBI/RTBI PHY interface support
> > priv->dma_cap.pcs will get to be true. Note the device can be
> > synthesized with several PHY interfaces supported. As long as
> > SGMII/TBI/RTBI PHY interface is any of them, the flag will be set
> > irrespective from the PHY interface activated at runtime. 
> 
> I've been debating about this, and given your response, I'm wondering
> whether we should change stmmac_mac_select_pcs() to instead do:
> 
> static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
> 						 phy_interface_t interface)
> {
> 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> 	struct phylink_pcs *pcs;
> 
> 	if (priv->plat->select_pcs) {
> 		pcs = priv->plat->select_pcs(priv, interface);
> 		if (!IS_ERR(pcs))
> 			return pcs;
> 	}
> 
> 	return stmmac_mac_phylink_select_pcs(priv, interface);
> }
> 
> and push the problem of whether to provide a PCS that overrides
> the MAC internal PCS into platform code. That would mean Intel mGBE
> would be able to override with XPCS. rzn1 and socfpga can then do
> their own thing as well.

Well, AFAICS the only device that currently have the DW XPCS
connected to a non DW XGMAC controller is indeed the Intel mGBE with its
DW QoS Eth+DW XPCS weird setup. At the same time the Intel mGBE
controller can also support RGMII interface. Thus there is no internal
SGMII/TBI/RTBI PCS in there.

Qualcomm QoS Eth uses the internal SGMII PCS and by setting up the
STMMAC_FLAG_HAS_INTEGRATED_PCS flag its driver almost completely
disables the STMMAC PCS functionality (except the
stmmac_pcs_ctrl_ane() being called in stmmac_hw_setup()).

So from the perspective of these two devices the PCS selection looks
quiet certain. It's either internal or external one. There is no
device with both of them available.

SoCFPGA... Well, it's another and more complicated story. Based on
what said in a comment in
socfpga_gen5_set_phy_mode()/socfpga_gen10_set_phy_mode() the only
possibility to have some internal interface converted to the external
one is when a "splitter" is available. But IMO the comment is
misleading because the only thing that is then done with the
"splitter" CSR is just the clock divider selection. What is actually
done, if the "splitter" is available or if the SGMII/GMII/MII
_MAC-interface_ is requested, then the internal interface is fixed to
"GMII/MII". It looks weird because based on the "mac-mode" DT-property
semantics it was supposed to indicate the internal interface only. But
EMAC is never tuned to have the SGMII interface (see the values saved
in the "*val" argument of socfpga_set_phy_mode_common()). So all of
that makes me to conclude the next points:

1. "mac-mode" property has never been utilized for the SoG-FPGA GMAC
platform. The plat_stmmacenet_data::mac_interface field has always
defaulted to plat_stmmacenet_data::phy_interface.

2. SoG-FPGA GMAC IP-core itself doesn't support the native/internal
SGMII interface.  It's implemented by means of the so called
"gmii-to-sgmii"-converter, which is the Lynx PCS.

Thus unless I've missed something the SoC-FPGA network controller
structure can be depicted as follows:


                   +---- SYSMGR:PHYSEL
      phy_intf_sel |
+------------------+                     +--------------+ 
|          RMII    |                     |              | Internal Interface
|       +----------+                     |          off +--------------------------+
|          RGMII   | Internal Interface  | SGMII        |                          | External Interface*
| EMAC  +----------+---------------------+              |          +-------+       +--------+-----------
|         GMII/MII |                     | adapter      | GMII/MII | Lynx  | SGMII |        |
|       +----------+                     |           on +----------+       +-------+        |
|                  +--+                  |              |          |  PCS  |                |
+------------------+  |                  +--------------+          +---+---+                |
                      |                                                |                    |
                      |              +------------+                    |                    |
                      +--------------+ Splitter** +--------------------+--------------------+
                                     +-----+------+
                                           |
                                     +-----+------+
                                     | Oscillator |
                                     +------------+

* No idea whether the external interface is represented as a single IO
port or as multiple interface ports handled by the same MAC.

** As I explained above, judging by the SoC-FPGA driver code the
"splitter" is just the reference clock divider responsible for the
clock rate adjustment based on the requested link speed.

Based on the logic depicted on the sketch above, I guess that there is
no internal SGMII/TBI/RTBI PCS in SoC-FPGA GMAC either. The SGMII
interface is implemented by means of the Lynx PCS.

> 
> I'm trying hard not to go down another rabbit hole... I've just
> spotted that socfpga sets mac_interface to PHY_INTERFACE_MODE_SGMII.
> That's another reason for pushing this down into platform drivers -
> if platform drivers are doing weird stuff, then we can contain their
> weirdness in the platform drivers moving it out of the core code.

Oh, that damn "mac-mode" property... First of all as I already
mentioned once AFAICS originally it was introduced for the SoC-FPGA
GMAC, but the property has never been defined in any DT-node so far,
neither in SoC-FPGA nodes nor in the rest of the DW *MAC-based nodes.
Moreover based on my consideration above the SoC-FPGA internal
interface is always determined based on the external one seeing
plat_stmmacenet_data::mac_interface defaults to
plat_stmmacenet_data::phy_interface. Secondly I also have much
certainty that the rest of the glue drivers utilizing
plat_stmmacenet_data::mac_interface field should in fact be using
plat_stmmacenet_data::phy_interface instead. Based on the history of
the mac_interface-related changes it's likely that all of them have
just either been missed during the conversion to utilizing the
phy_interface-field or incorrectly utilized the mac_interface field
instead of phy_interface in the first place.

So to speak before going further it might be worth re-checking once
again the entire history of the "mac-mode" property-related change,
but as an experimental A/B-test patch for net-next it may be a good
idea to either drop the mac_interface field completely, or convert the
driver to forgetting about the internal PCS if the external one is
enabled, or, as a less invasive option, make SoC-FPGA explicitly
setting up the mac_interface field to GMII/MII if it configures the
internal interface to that value. Then, if these changes don't break
any platform (most importantly the SoF-FPGA GMAC case), then we can go
further and carefully convert the rest of the glue-drivers not using
the mac_interface field.

> 
> > You can extend the priv->dma_cap.pcs flag semantics. So it could
> > be indicating three types of the PCS'es:
> > RGMII, SGMII, XPCS (or TBI/RTBI in future).
> 
> If TBI/RTBI gets supported, then this would have to be extended, but I
> get the impression that this isn't popular.

Irrespective from the TBI/RTBI interface support, using the
priv->dma_cap.pcs field for all possible PCS'es shall also improve the
code readability. Currently we have four versions of the PCS fields:
dma_features::pcs
mac_device_info::pcs
mac_device_info::xpcs
mac_device_info::lynx_pcs
which are being checked here and there in the driver...

> 
> > I guess the DW XPCS implementation might be more preferable. From one
> > side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW
> > GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other
> > hand the DW XPCS might be available over the MDIO-bus, which is slower
> > to access than the internal PCS CSRs available in the DW GMAC/QoS Eth
> > CSRs space. So the more performant link speed seems more useful
> > feature over the faster device setup process.
> 
> I think which should be used would depend on how the hardware is wired
> up. This brings us back to platform specifics again, which points
> towards moving the decision making into platform code as per the above.
> 
> > One thing I am not sure about is that there is a real case of having
> > the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY
> > interface support and being attached to the DW XPCS controller, which
> > would have the SGMII downstream PHY interface. DW XPCS has only the
> > XGMII or GMII/MII upstream interfaces over which the MAC can be
> > attached.
> 
> That gives us another possibility, but needs platforms to be doing
> the right thing. If mac_interface were set to XGMII or GMII/MII, then
> that would exclude the internal MAC PCS.
> 
> > So DW GMAC/QoS Eth and DW XPCS can be connected via the
> > GMII/MII interface only. Regarding Intel mGBE, it likely is having a
> > setup like this:
> > 
> > +------------+          +---------+
> > |            | GMII/MII |         |   SGMII
> > | DW QoS Eth +----------+ DW XPCS +------------
> > |            |          |         | 1000Base-X
> > +------------+          +---------+
> 
> 
> So as an alternative, 
> 
>      mac_interface            phy_interface
> 
>      XGMII/GMII/MII           SGMII/1000Base-X
> MAC ---------------- DW XPCS ------------------
> 
>      INTERNAL                SGMII/TBI/RTBI
> MAC ---------- Internal PCS ----------------
> 
>      INTERNAL                  RGMII
> MAC ---------- Internal "PCS" --------------

+ SoC-FPGA (presumably)

       GMII/MII                  SGMII
  MAC ---------------- Lynx PCS --------------

Please also note, based on the DW GMAC/QoS Eth hardware manual each
internal interface block is connected to MAC by the GMII/MII
interface. So the internal PCS cases more precisely could be
represented as follows:

       GMII                     SGMII (AN)
  MAC ---------- Internal PCS ------------------

       GMII                     TBI/RTBI (AN)
  MAC ---------- Internal PCS ------------------
  
       GMII                      RGMII (In-band)
  MAC ---------- Internal "PCS" ----------------

       GMII                      RevMII
  MAC ----------  RevMII block  ----------------

       GMII                      GMII
  MAC ------------------------------------------

       MII                       SMII (In-band)
  MAC ---------- Internal "PCS" ----------------

       MII                       RMII
  MAC ----------   RMII block   ----------------

       MII                       MII
  MAC ------------------------------------------

There is a special input signal phy_intf_sel[2:0], which tells to MAC
what interface to activate (grep -i the glue drivers for "intf",
"physel", etc).

> 
> One of the problems here, though, is socfpga. It uses mac_interface
> with RGMII*, MII, GMII, SGMII and RMII. I think it's confusing
> mac_interface for phy_interface, but I haven't read through enough
> of it to be certain.
> 
> So that again leads me back to my proposal above for
> stmmac_mac_select_pcs() as the least likely to break proposition -
> at least given how things are at the moment.

Please see my notes above regarding the internal interface
initialization in the SoC-FPGA glue driver. I guess we could at least
try to A/B-test the SoC-FPGA code in the next net-next by setting
mac_interface to GMII/MII when the internal interface is enabled as
GMII/MII in the glue-driver, and converting the rest of the glue
driver to using phy_interface. If nothing breaks, then SoC-FPGA has
never used the "mac-mode" property and we could mark the property as
deprecated and could carefully covert the rest of the STMMAC platform
driver to using the phy_interface field.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > into the DW GMAC controller. It's always done if the controller supports
> > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > interfaces support was activated during the IP-core synthesize the PCS
> > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > set. Based on that the RGMII in-band status detection procedure
> > implemented in the driver hasn't been working for the devices with the
> > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > interfaces available in the device.
> > 
> > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > statement responsible for the In-band/PCS functionality activation. If the
> > RGMII interface is supported by the device then the in-band link status
> > detection will be also supported automatically (it's always embedded into
> > the RGMII RTL code). If the SGMII interface is supported by the device
> > then the PCS block will be supported too (it's unconditionally synthesized
> > into the controller). The later is also correct for the TBI/RTBI PHY
> > interfaces.
> > 
> > Note while at it drop the netdev_dbg() calls since at the moment of the
> > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > the debug prints will be for the unknown/NULL device.
> 

> Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> it seems to be fixing a bug in the driver as it stands today?

From one point of view it could be submitted for the net tree indeed,
but on the second thought are you sure we should be doing that seeing
it will activate the RGMII-inband detection and the code with the
netif-carrier toggling behind the phylink back? Who knows what new
regressions the activated PCS-code can cause?..

> 
> Also, a build fix is required here:
> 
> > -	if (priv->dma_cap.pcs) {
> > -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> > -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> > -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> > -			priv->hw->pcs = STMMAC_PCS_RGMII;
> > -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> > -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> > -			priv->hw->pcs = STMMAC_PCS_SGMII;
> > -		}
> > -	}
> > +	if (phy_interface_mode_is_rgmii(interface))
> > +		priv->hw.pcs = STMMAC_PCS_RGMII;
> > +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> > +		priv->hw.pcs = STMMAC_PCS_SGMII;
> 
> Both of these assignments should be priv->hw->pcs not priv->hw.pcs.

Ah, right. Originally I applied your patchset on top of my fixes,
cleanups and platform glue-driver patchsets. One of the cleanups
implied the mac_device_info instance movement to the stmmac_priv
structure. When I was moving my changes onto your original series I
just missed that part of the patch. Sorry about that. The rest of my
patches seems free from such problem.

> 
> I think there's also another bug that needs fixing along with this.
> See stmmac_ethtool_set_link_ksettings(). Note that this denies the
> ability to disable autoneg, which (a) doesn't make sense for RGMII
> with an attached PHY, and 

This doesn't make sense for RGMII also due to DW GMAC/QoS Eth not having
anything AN-related for the RGMII PHY interface. RGMII mode permits
the Link status detection via the in-band signal retrieved from the
PHY and nothing else. AN, if enabled, is performed on the PHY side.

> (b) this code should be passing the
> ethtool op to phylink for it to pass on to phylib so the PHY can
> be appropriately configured for the users desired autoneg and
> link mode settings.

I am not that well aware of the phylink internals to be saying for
100% sure, but thinking logically it would be indeed better if phylink
was aware of the PCS state changes. But in case of the STMMAC PCS
implementation I guess that the original PCS-code was designed to work
with no PHY being involved:
e58bb43f5e43 ("stmmac: initial support to manage pcs modes")
See that commit prevented the MDIO-bus registration and PHY
initialization in case of the PCS/RGMII-inband being available. But in
practice the implementation turned to be not that well thought
through. So eventually, commit-by-commit, the implementation was
effectively converted to the no longer used code. At least for the
MACs with just RGMII interface and no additional SGMII/TBI/RTBI
interfaces, which I guess is the vast majority of the real devices
with RGMII.

> 
> I also don't think it makes any sense for the STMMAC_PCS_SGMII case
> given that it means Cisco SGMII - which implies that there is also
> a PHY (since Cisco SGMII with inband is designed to be coupled with
> something that looks like a PHY to send the inband signalling
> necessary to configure e.g. the SGMII link symbol replication.

AFAICS the STMMAC driver supports the MAC2MAC case connected over
SGMII with no intermediate PHY. In that case the speed will be just
fixed to what was set in the "snps,ps-speed" property. The RAL (Rate
Adapter Layer) is configured to do that by having the SGMRAL flag set
(see your dwmac_pcs_config() and what is done if hw->ps is non-zero).

> 
> In both of these cases, even if the user requests autoneg to be
> disabled, that _shouldn't_ affect internal network driver links.
> This ethtool op is about configuring the externally visible media
> side of the network driver, not the internal links.

IMO considering all the driver over-complexity (that's the most polite
definition I managed to come up to.)) it would be much easier and
likely safer not to try to fix the PCS-code and just convert it to
something sane. At least the RGMII/In-band functionality we'll be able
to test on my device. If the PCS SGMII part is still utilized by
anybody, then if there are problems there the new kernel RCs will get
to reveal them.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

On Mon, May 27, 2024 at 12:57:02AM +0300, Serge Semin wrote:
> On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > > into the DW GMAC controller. It's always done if the controller supports
> > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > > interfaces support was activated during the IP-core synthesize the PCS
> > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > > set. Based on that the RGMII in-band status detection procedure
> > > implemented in the driver hasn't been working for the devices with the
> > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > > interfaces available in the device.
> > > 
> > > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > > statement responsible for the In-band/PCS functionality activation. If the
> > > RGMII interface is supported by the device then the in-band link status
> > > detection will be also supported automatically (it's always embedded into
> > > the RGMII RTL code). If the SGMII interface is supported by the device
> > > then the PCS block will be supported too (it's unconditionally synthesized
> > > into the controller). The later is also correct for the TBI/RTBI PHY
> > > interfaces.
> > > 
> > > Note while at it drop the netdev_dbg() calls since at the moment of the
> > > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > > the debug prints will be for the unknown/NULL device.
> > 
> 
> > Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> > it seems to be fixing a bug in the driver as it stands today?
> 
> From one point of view it could be submitted for the net tree indeed,
> but on the second thought are you sure we should be doing that seeing
> it will activate the RGMII-inband detection and the code with the
> netif-carrier toggling behind the phylink back? Who knows what new
> regressions the activated PCS-code can cause?..

If it's not a fix that is suitable without the remainder of the patch
set, this should be stated in the commit description and it shouldn't
have a Fixes: tag.

The reason is because it wouldn't be stable kernel material without the
other patches - if stable picks it up without the other patches then
it could end up being applied without the other patches resulting in
the situation you mention above.

Shall I remove the Fixes: tag?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

On Tue, May 28, 2024 at 11:21:39AM +0100, Russell King (Oracle) wrote:
> On Mon, May 27, 2024 at 12:57:02AM +0300, Serge Semin wrote:
> > On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> > > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > > > into the DW GMAC controller. It's always done if the controller supports
> > > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > > > interfaces support was activated during the IP-core synthesize the PCS
> > > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > > > set. Based on that the RGMII in-band status detection procedure
> > > > implemented in the driver hasn't been working for the devices with the
> > > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > > > interfaces available in the device.
> > > > 
> > > > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > > > statement responsible for the In-band/PCS functionality activation. If the
> > > > RGMII interface is supported by the device then the in-band link status
> > > > detection will be also supported automatically (it's always embedded into
> > > > the RGMII RTL code). If the SGMII interface is supported by the device
> > > > then the PCS block will be supported too (it's unconditionally synthesized
> > > > into the controller). The later is also correct for the TBI/RTBI PHY
> > > > interfaces.
> > > > 
> > > > Note while at it drop the netdev_dbg() calls since at the moment of the
> > > > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > > > the debug prints will be for the unknown/NULL device.
> > > 
> > 
> > > Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> > > it seems to be fixing a bug in the driver as it stands today?
> > 
> > From one point of view it could be submitted for the net tree indeed,
> > but on the second thought are you sure we should be doing that seeing
> > it will activate the RGMII-inband detection and the code with the
> > netif-carrier toggling behind the phylink back? Who knows what new
> > regressions the activated PCS-code can cause?..
> 
> If it's not a fix that is suitable without the remainder of the patch
> set, this should be stated in the commit description and it shouldn't
> have a Fixes: tag.
> 
> The reason is because it wouldn't be stable kernel material without the
> other patches - if stable picks it up without the other patches then
> it could end up being applied without the other patches resulting in
> the situation you mention above.
> 
> Shall I remove the Fixes: tag?

Let's drop it then, so not to cause confusion for the maintainers.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 3/3] net: stmmac: Drop TBI/RTBI PCS flags

[Serge Semin]

First of all the flags are never set by any of the driver parts. If nobody
have them set then the respective statements will always have the same
result. Thus the statements can be simplified or even dropped with no risk
to break things.

Secondly shall any of the TBI or RTBI flag is set the MDIO-bus
registration will be bypassed. Why? It really seems weird. It's perfectly
fine to have a TBI/RTBI-capable PHY configured over the MDIO bus
interface.

Based on the notes above the TBI/RTBI PCS flags can be freely dropped thus
simplifying the driver code.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h  |  2 -
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 37 ++++++-------------
 2 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index b02b905bc892..40a930ea4ff3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -268,8 +268,6 @@ struct stmmac_safety_stats {
 /* PCS defines */
 #define STMMAC_PCS_RGMII	(1 << 0)
 #define STMMAC_PCS_SGMII	(1 << 1)
-#define STMMAC_PCS_TBI		(1 << 2)
-#define STMMAC_PCS_RTBI		(1 << 3)
 
 #define SF_DMA_MODE 1		/* DMA STORE-AND-FORWARD Operation Mode */
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6c4e90b1fea3..06f95dfdf09e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -471,13 +471,6 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
 {
 	int eee_tw_timer = priv->eee_tw_timer;
 
-	/* Using PCS we cannot dial with the phy registers at this stage
-	 * so we do not support extra feature like EEE.
-	 */
-	if (priv->hw->pcs == STMMAC_PCS_TBI ||
-	    priv->hw->pcs == STMMAC_PCS_RTBI)
-		return false;
-
 	/* Check if MAC core supports the EEE feature. */
 	if (!priv->dma_cap.eee)
 		return false;
@@ -3945,9 +3938,7 @@ static int __stmmac_open(struct net_device *dev,
 	if (ret < 0)
 		return ret;
 
-	if (priv->hw->pcs != STMMAC_PCS_TBI &&
-	    priv->hw->pcs != STMMAC_PCS_RTBI &&
-	    (!priv->hw->xpcs ||
+	if ((!priv->hw->xpcs ||
 	     xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73) &&
 	    !priv->hw->lynx_pcs) {
 		ret = stmmac_init_phy(dev);
@@ -7724,16 +7715,12 @@ int stmmac_dvr_probe(struct device *device,
 	if (!pm_runtime_enabled(device))
 		pm_runtime_enable(device);
 
-	if (priv->hw->pcs != STMMAC_PCS_TBI &&
-	    priv->hw->pcs != STMMAC_PCS_RTBI) {
-		/* MDIO bus Registration */
-		ret = stmmac_mdio_register(ndev);
-		if (ret < 0) {
-			dev_err_probe(priv->device, ret,
-				      "%s: MDIO bus (id: %d) registration failed\n",
-				      __func__, priv->plat->bus_id);
-			goto error_mdio_register;
-		}
+	ret = stmmac_mdio_register(ndev);
+	if (ret < 0) {
+		dev_err_probe(priv->device, ret,
+			      "MDIO bus (id: %d) registration failed\n",
+			      priv->plat->bus_id);
+		goto error_mdio_register;
 	}
 
 	if (priv->plat->speed_mode_2500)
@@ -7776,9 +7763,7 @@ int stmmac_dvr_probe(struct device *device,
 	phylink_destroy(priv->phylink);
 error_xpcs_setup:
 error_phy_setup:
-	if (priv->hw->pcs != STMMAC_PCS_TBI &&
-	    priv->hw->pcs != STMMAC_PCS_RTBI)
-		stmmac_mdio_unregister(ndev);
+	stmmac_mdio_unregister(ndev);
 error_mdio_register:
 	stmmac_napi_del(ndev);
 error_hw_init:
@@ -7817,9 +7802,9 @@ void stmmac_dvr_remove(struct device *dev)
 	if (priv->plat->stmmac_rst)
 		reset_control_assert(priv->plat->stmmac_rst);
 	reset_control_assert(priv->plat->stmmac_ahb_rst);
-	if (priv->hw->pcs != STMMAC_PCS_TBI &&
-	    priv->hw->pcs != STMMAC_PCS_RTBI)
-		stmmac_mdio_unregister(ndev);
+
+	stmmac_mdio_unregister(ndev);
+
 	destroy_workqueue(priv->wq);
 	mutex_destroy(&priv->lock);
 	bitmap_free(priv->af_xdp_zc_qps);
-- 
2.43.0

Re: [PATCH RFC net-next 3/3] net: stmmac: Drop TBI/RTBI PCS flags

[Russell King (Oracle)]

On Sat, May 25, 2024 at 12:02:59AM +0300, Serge Semin wrote:
> First of all the flags are never set by any of the driver parts. If nobody
> have them set then the respective statements will always have the same
> result. Thus the statements can be simplified or even dropped with no risk
> to break things.
> 
> Secondly shall any of the TBI or RTBI flag is set the MDIO-bus
> registration will be bypassed. Why? It really seems weird. It's perfectly
> fine to have a TBI/RTBI-capable PHY configured over the MDIO bus
> interface.
> 
> Based on the notes above the TBI/RTBI PCS flags can be freely dropped thus
> simplifying the driver code.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

I think this patch can come first in the series, along with another
few patches that remove stuff. Any objection?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/3] net: stmmac: Drop TBI/RTBI PCS flags

[Serge Semin]

On Tue, May 28, 2024 at 11:23:05AM +0100, Russell King (Oracle) wrote:
> On Sat, May 25, 2024 at 12:02:59AM +0300, Serge Semin wrote:
> > First of all the flags are never set by any of the driver parts. If nobody
> > have them set then the respective statements will always have the same
> > result. Thus the statements can be simplified or even dropped with no risk
> > to break things.
> > 
> > Secondly shall any of the TBI or RTBI flag is set the MDIO-bus
> > registration will be bypassed. Why? It really seems weird. It's perfectly
> > fine to have a TBI/RTBI-capable PHY configured over the MDIO bus
> > interface.
> > 
> > Based on the notes above the TBI/RTBI PCS flags can be freely dropped thus
> > simplifying the driver code.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> I think this patch can come first in the series, along with another
> few patches that remove stuff. Any objection?

No objection.

-Serge(y)

> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 1/3] net: stmmac: Prevent RGSMIIIS IRQs flood

[Russell King (Oracle)]

On Sat, May 25, 2024 at 12:02:57AM +0300, Serge Semin wrote:
> Without reading the GMAC_RGSMIIIS/MAC_PHYIF_Control_Status the IRQ line
> won't be de-asserted causing interrupt handler executed over and over. As
> a quick-fix let's just dummy-read the CSR for now.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

I think it would make sense to merge these into the patches that do the
conversion to avoid a git bisect hitting on a patch that causes an
interrupt storm. Any objection?

(I'm now converting these two in separate patches, so would need to
split this patch...)

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 1/3] net: stmmac: Prevent RGSMIIIS IRQs flood

[Serge Semin]

On Tue, May 28, 2024 at 11:24:30AM +0100, Russell King (Oracle) wrote:
> On Sat, May 25, 2024 at 12:02:57AM +0300, Serge Semin wrote:
> > Without reading the GMAC_RGSMIIIS/MAC_PHYIF_Control_Status the IRQ line
> > won't be de-asserted causing interrupt handler executed over and over. As
> > a quick-fix let's just dummy-read the CSR for now.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> I think it would make sense to merge these into the patches that do the
> conversion to avoid a git bisect hitting on a patch that causes an
> interrupt storm. Any objection?

Of course, no objection. This patch content was intended to be merged
into yours.

-Serge(y)

> 
> (I'm now converting these two in separate patches, so would need to
> split this patch...)
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!