[PATCH net-next v2 0/10] stmmac cleanups

[PATCH net-next v2 0/10] stmmac cleanups

[Russell King (Oracle)]

Hi,

One of the comments I had on Feiyang Chen's series was concerning the
initialisation of phylink... and so I've decided to do something about
it, cleaning it up a bit.

This series:

1) adds a new phylink function to limit the MAC capabilities according
   to a maximum speed. This allows us to greatly simplify stmmac's
   initialisation of phylink's mac capabilities.

2) everywhere that uses priv->plat->phylink_node first converts this
   to a fwnode before doing anything with it. This is silly. Let's
   instead store it as a fwnode to eliminate these conversions in
   multiple places.

3) clean up passing the fwnode to phylink - it might as well happen
   at the phylink_create() callsite, rather than being scattered
   throughout the entire function.

4) same for mdio_bus_data

5) use phylink_limit_mac_speed() to handle the priv->plat->max_speed
   restriction.

6) add a method to get the MAC-specific capabilities from the code
   dealing with the MACs, and arrange to call it at an appropriate
   time.

7) convert the gmac4 users to use the MAC specific method.

8) same for xgmac.

9) group all the simple phylink_config initialisations together.

10) convert half-duplex logic to being positive logic.

While looking into all of this, this raised eyebrows:

        if (priv->plat->tx_queues_to_use > 1)
                priv->phylink_config.mac_capabilities &=
                        ~(MAC_10HD | MAC_100HD | MAC_1000HD);

priv->plat->tx_queues_to_use is initialised by platforms to either 1,
4 or 8, and can be controlled from userspace via the --set-channels
ethtool op. The implementation of this op in this driver limits the
number of channels to priv->dma_cap.number_tx_queues, which is derived
from the DMA hwcap.

So, the obvious questions are:

1) what guarantees that the static initialisation of tx_queues_to_use
will always be less than or equal to number_tx_queues from the DMA hw
cap?

2) tx_queues_to_use starts off as 1, but number_tx_queues is larger,
we will leave the half-duplex capabilities in place, but userspace can
increase tx_queues_to_use above 1. Does that mean half-duplex is then
not supported?

3) Should we be basing the decision whether half-duplex is supported
off the DMA capabilities?

4) What about priv->dma_cap.half_duplex? Doesn't that get a say in
whether half-duplex is supported or not? Why isn't this used? Why is
it only reported via debugfs? If it's not being used by the driver,
what's the point of reporting it via debugfs?

 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  |  8 +++
 .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 10 ++++
 drivers/net/ethernet/stmicro/stmmac/hwif.h         |  4 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 60 +++++++++-------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  |  3 +-
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  2 +-
 drivers/net/phy/phylink.c                          | 18 +++++++
 include/linux/phylink.h                            |  2 +
 include/linux/stmmac.h                             |  2 +-
 9 files changed, 70 insertions(+), 39 deletions(-)

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

[PATCH net-next 01/10] net: phylink: add phylink_limit_mac_speed()

[Russell King (Oracle)]

Add a function which can be used to limit the phylink MAC capabilities
to an upper speed limit.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 18 ++++++++++++++++++
 include/linux/phylink.h   |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 160bce608c34..0d7354955d62 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -426,6 +426,24 @@ static struct {
 	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF },
 };
 
+/**
+ * phylink_limit_mac_speed - limit the phylink_config to a maximum speed
+ * @config: pointer to a &struct phylink_config
+ * @max_speed: maximum speed
+ *
+ * Mask off MAC capabilities for speeds higher than the @max_speed parameter.
+ * Any further motifications of config.mac_capabilities will override this.
+ */
+void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(phylink_caps_params) &&
+		    phylink_caps_params[i].speed > max_speed; i++)
+		config->mac_capabilities &= ~phylink_caps_params[i].mask;
+}
+EXPORT_SYMBOL_GPL(phylink_limit_mac_speed);
+
 /**
  * phylink_cap_from_speed_duplex - Get mac capability from speed/duplex
  * @speed: the speed to search for
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 789c516c6b4a..7d07f8736431 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -223,6 +223,8 @@ struct phylink_config {
 	unsigned long mac_capabilities;
 };
 
+void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
+
 /**
  * struct phylink_mac_ops - MAC operations structure.
  * @validate: Validate and update the link configuration.
-- 
2.30.2

[PATCH net-next 02/10] net: stmmac: convert plat->phylink_node to fwnode

[Russell King (Oracle)]

All users of plat->phylink_node first convert it to a fwnode. Rather
than repeatedly convert to a fwnode, store it as a fwnode. To reflect
this change, call it plat->port_node instead - it is used for more
than just phylink.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 4 ++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c     | 3 ++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +-
 include/linux/stmmac.h                                | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7a9bbcf03ea5..a2dfb624e10c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1153,7 +1153,7 @@ static int stmmac_init_phy(struct net_device *dev)
 	if (!phylink_expects_phy(priv->phylink))
 		return 0;
 
-	fwnode = of_fwnode_handle(priv->plat->phylink_node);
+	fwnode = priv->plat->port_node;
 	if (!fwnode)
 		fwnode = dev_fwnode(priv->device);
 
@@ -1200,7 +1200,7 @@ static int stmmac_init_phy(struct net_device *dev)
 static int stmmac_phy_setup(struct stmmac_priv *priv)
 {
 	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
-	struct fwnode_handle *fwnode = of_fwnode_handle(priv->plat->phylink_node);
+	struct fwnode_handle *fwnode = priv->plat->port_node;
 	int max_speed = priv->plat->max_speed;
 	int mode = priv->plat->phy_interface;
 	struct phylink *phylink;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index dd9e2fec5328..fa9e7e7040b9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -533,11 +533,11 @@ int stmmac_mdio_register(struct net_device *ndev)
 	int err = 0;
 	struct mii_bus *new_bus;
 	struct stmmac_priv *priv = netdev_priv(ndev);
-	struct fwnode_handle *fwnode = of_fwnode_handle(priv->plat->phylink_node);
 	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
 	struct device_node *mdio_node = priv->plat->mdio_node;
 	struct device *dev = ndev->dev.parent;
 	struct fwnode_handle *fixed_node;
+	struct fwnode_handle *fwnode;
 	int addr, found, max_addr;
 
 	if (!mdio_bus_data)
@@ -601,6 +601,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 		stmmac_xgmac2_mdio_read_c45(new_bus, 0, 0, 0);
 
 	/* If fixed-link is set, skip PHY scanning */
+	fwnode = priv->plat->port_node;
 	if (!fwnode)
 		fwnode = dev_fwnode(priv->device);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index be8e79c7aa34..ff330423ee66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -428,7 +428,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
 
 	/* PHYLINK automatically parses the phy-handle property */
-	plat->phylink_node = np;
+	plat->port_node = of_fwnode_handle(np);
 
 	/* Get max speed of operation from device tree */
 	of_property_read_u32(np, "max-speed", &plat->max_speed);
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 784277d666eb..b2ccd827bb80 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -227,7 +227,7 @@ struct plat_stmmacenet_data {
 	phy_interface_t phy_interface;
 	struct stmmac_mdio_bus_data *mdio_bus_data;
 	struct device_node *phy_node;
-	struct device_node *phylink_node;
+	struct fwnode_handle *port_node;
 	struct device_node *mdio_node;
 	struct stmmac_dma_cfg *dma_cfg;
 	struct stmmac_est *est;
-- 
2.30.2

[PATCH net-next 03/10] net: stmmac: clean up passing fwnode to phylink

[Russell King (Oracle)]

Move the initialisation of the fwnode variable closer to its use
site, rather than scattered throughout stmmac_phy_setup().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a2dfb624e10c..7d97166194b5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1200,9 +1200,9 @@ static int stmmac_init_phy(struct net_device *dev)
 static int stmmac_phy_setup(struct stmmac_priv *priv)
 {
 	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
-	struct fwnode_handle *fwnode = priv->plat->port_node;
 	int max_speed = priv->plat->max_speed;
 	int mode = priv->plat->phy_interface;
+	struct fwnode_handle *fwnode;
 	struct phylink *phylink;
 
 	priv->phylink_config.dev = &priv->dev->dev;
@@ -1211,9 +1211,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 		priv->phylink_config.ovr_an_inband =
 			mdio_bus_data->xpcs_an_inband;
 
-	if (!fwnode)
-		fwnode = dev_fwnode(priv->device);
-
 	/* Set the platform/firmware specified interface mode */
 	__set_bit(mode, priv->phylink_config.supported_interfaces);
 
@@ -1254,6 +1251,10 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	fwnode = priv->plat->port_node;
+	if (!fwnode)
+		fwnode = dev_fwnode(priv->device);
+
 	phylink = phylink_create(&priv->phylink_config, fwnode,
 				 mode, &stmmac_phylink_mac_ops);
 	if (IS_ERR(phylink))
-- 
2.30.2

[PATCH net-next 04/10] net: stmmac: use "mdio_bus_data" local variable

[Russell King (Oracle)]

We have a local variable for priv->plat->mdio_bus_data, which we use
later in the conditional if() block, but we evaluate the above within
the conditional expression. Use mdio_bus_data instead. Since these
will be the only two users of this local variable, move its assignment
just before the if().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7d97166194b5..fe733a9f5fe4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1199,7 +1199,7 @@ static int stmmac_init_phy(struct net_device *dev)
 
 static int stmmac_phy_setup(struct stmmac_priv *priv)
 {
-	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
+	struct stmmac_mdio_bus_data *mdio_bus_data;
 	int max_speed = priv->plat->max_speed;
 	int mode = priv->plat->phy_interface;
 	struct fwnode_handle *fwnode;
@@ -1207,7 +1207,9 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 
 	priv->phylink_config.dev = &priv->dev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
-	if (priv->plat->mdio_bus_data)
+
+	mdio_bus_data = priv->plat->mdio_bus_data;
+	if (mdio_bus_data)
 		priv->phylink_config.ovr_an_inband =
 			mdio_bus_data->xpcs_an_inband;
 
-- 
2.30.2

[PATCH net-next 05/10] net: stmmac: use phylink_limit_mac_speed()

[Russell King (Oracle)]

Use phylink_limit_mac_speed() to limit the MAC capabilities rather
than coding this for each speed.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++-----------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fe733a9f5fe4..30a085f2b8fa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1200,10 +1200,10 @@ static int stmmac_init_phy(struct net_device *dev)
 static int stmmac_phy_setup(struct stmmac_priv *priv)
 {
 	struct stmmac_mdio_bus_data *mdio_bus_data;
-	int max_speed = priv->plat->max_speed;
 	int mode = priv->plat->phy_interface;
 	struct fwnode_handle *fwnode;
 	struct phylink *phylink;
+	int max_speed;
 
 	priv->phylink_config.dev = &priv->dev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
@@ -1222,29 +1222,18 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 				    priv->phylink_config.supported_interfaces);
 
 	priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
-		MAC_10 | MAC_100;
-
-	if (!max_speed || max_speed >= 1000)
-		priv->phylink_config.mac_capabilities |= MAC_1000;
+		MAC_10 | MAC_100 | MAC_1000;
 
 	if (priv->plat->has_gmac4) {
-		if (!max_speed || max_speed >= 2500)
-			priv->phylink_config.mac_capabilities |= MAC_2500FD;
+		priv->phylink_config.mac_capabilities |= MAC_2500FD;
 	} else if (priv->plat->has_xgmac) {
-		if (!max_speed || max_speed >= 2500)
-			priv->phylink_config.mac_capabilities |= MAC_2500FD;
-		if (!max_speed || max_speed >= 5000)
-			priv->phylink_config.mac_capabilities |= MAC_5000FD;
-		if (!max_speed || max_speed >= 10000)
-			priv->phylink_config.mac_capabilities |= MAC_10000FD;
-		if (!max_speed || max_speed >= 25000)
-			priv->phylink_config.mac_capabilities |= MAC_25000FD;
-		if (!max_speed || max_speed >= 40000)
-			priv->phylink_config.mac_capabilities |= MAC_40000FD;
-		if (!max_speed || max_speed >= 50000)
-			priv->phylink_config.mac_capabilities |= MAC_50000FD;
-		if (!max_speed || max_speed >= 100000)
-			priv->phylink_config.mac_capabilities |= MAC_100000FD;
+		priv->phylink_config.mac_capabilities |= MAC_2500FD;
+		priv->phylink_config.mac_capabilities |= MAC_5000FD;
+		priv->phylink_config.mac_capabilities |= MAC_10000FD;
+		priv->phylink_config.mac_capabilities |= MAC_25000FD;
+		priv->phylink_config.mac_capabilities |= MAC_40000FD;
+		priv->phylink_config.mac_capabilities |= MAC_50000FD;
+		priv->phylink_config.mac_capabilities |= MAC_100000FD;
 	}
 
 	/* Half-Duplex can only work with single queue */
@@ -1253,6 +1242,10 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	max_speed = priv->plat->max_speed;
+	if (max_speed)
+		phylink_limit_mac_speed(&priv->phylink_config, max_speed);
+
 	fwnode = priv->plat->port_node;
 	if (!fwnode)
 		fwnode = dev_fwnode(priv->device);
-- 
2.30.2

[PATCH net-next 06/10] net: stmmac: provide stmmac_mac_phylink_get_caps()

[Russell King (Oracle)]

Allow MACs to provide their own capabilities via the MAC operations
struct.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h        | 4 ++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 238f17c50a1e..b95d3e137813 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -300,6 +300,8 @@ struct stmmac_est;
 struct stmmac_ops {
 	/* MAC core initialization */
 	void (*core_init)(struct mac_device_info *hw, struct net_device *dev);
+	/* Get phylink capabilities */
+	void (*phylink_get_caps)(struct stmmac_priv *priv);
 	/* Enable the MAC RX/TX */
 	void (*set_mac)(void __iomem *ioaddr, bool enable);
 	/* Enable and verify that the IPC module is supported */
@@ -419,6 +421,8 @@ struct stmmac_ops {
 
 #define stmmac_core_init(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, core_init, __args)
+#define stmmac_mac_phylink_get_caps(__priv) \
+	stmmac_do_void_callback(__priv, mac, phylink_get_caps, __priv)
 #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 30a085f2b8fa..a9cf6aecdddf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1224,6 +1224,9 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100 | MAC_1000;
 
+	/* Get the MAC specific capabilities */
+	stmmac_mac_phylink_get_caps(priv);
+
 	if (priv->plat->has_gmac4) {
 		priv->phylink_config.mac_capabilities |= MAC_2500FD;
 	} else if (priv->plat->has_xgmac) {
-- 
2.30.2

[PATCH net-next 07/10] net: stmmac: move gmac4 specific phylink capabilities to gmac4

[Russell King (Oracle)]

Move the setup of gmac4 speicifc phylink capabilities into gmac4 code.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 8 ++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index 03b1c5a97826..c6ff1fa0e04d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -68,6 +68,11 @@ static void dwmac4_core_init(struct mac_device_info *hw,
 		init_waitqueue_head(&priv->tstamp_busy_wait);
 }
 
+static void dwmac4_phylink_get_caps(struct stmmac_priv *priv)
+{
+	priv->phylink_config.mac_capabilities |= MAC_2500FD;
+}
+
 static void dwmac4_rx_queue_enable(struct mac_device_info *hw,
 				   u8 mode, u32 queue)
 {
@@ -1131,6 +1136,7 @@ static int dwmac4_config_l4_filter(struct mac_device_info *hw, u32 filter_no,
 
 const struct stmmac_ops dwmac4_ops = {
 	.core_init = dwmac4_core_init,
+	.phylink_get_caps = dwmac4_phylink_get_caps,
 	.set_mac = stmmac_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1173,6 +1179,7 @@ const struct stmmac_ops dwmac4_ops = {
 
 const struct stmmac_ops dwmac410_ops = {
 	.core_init = dwmac4_core_init,
+	.phylink_get_caps = dwmac4_phylink_get_caps,
 	.set_mac = stmmac_dwmac4_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1221,6 +1228,7 @@ const struct stmmac_ops dwmac410_ops = {
 
 const struct stmmac_ops dwmac510_ops = {
 	.core_init = dwmac4_core_init,
+	.phylink_get_caps = dwmac4_phylink_get_caps,
 	.set_mac = stmmac_dwmac4_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a9cf6aecdddf..0b02845e7e9d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1227,9 +1227,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	/* Get the MAC specific capabilities */
 	stmmac_mac_phylink_get_caps(priv);
 
-	if (priv->plat->has_gmac4) {
-		priv->phylink_config.mac_capabilities |= MAC_2500FD;
-	} else if (priv->plat->has_xgmac) {
+	if (priv->plat->has_xgmac) {
 		priv->phylink_config.mac_capabilities |= MAC_2500FD;
 		priv->phylink_config.mac_capabilities |= MAC_5000FD;
 		priv->phylink_config.mac_capabilities |= MAC_10000FD;
-- 
2.30.2

[PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

[Russell King (Oracle)]

Move the xgmac specific phylink capabilities to the dwxgmac2 support
core.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 34e1b0c3f346..f352be269deb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
 	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
 }
 
+static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
+{
+	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
+						 MAC_10000FD | MAC_25000FD |
+						 MAC_40000FD | MAC_50000FD |
+						 MAC_100000FD;
+}
+
 static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
 {
 	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
@@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
 
 const struct stmmac_ops dwxgmac210_ops = {
 	.core_init = dwxgmac2_core_init,
+	.phylink_get_caps = xgmac_phylink_get_caps,
 	.set_mac = dwxgmac2_set_mac,
 	.rx_ipc = dwxgmac2_rx_ipc,
 	.rx_queue_enable = dwxgmac2_rx_queue_enable,
@@ -1551,6 +1560,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
 
 const struct stmmac_ops dwxlgmac2_ops = {
 	.core_init = dwxgmac2_core_init,
+	.phylink_get_caps = xgmac_phylink_get_caps,
 	.set_mac = dwxgmac2_set_mac,
 	.rx_ipc = dwxgmac2_rx_ipc,
 	.rx_queue_enable = dwxlgmac2_rx_queue_enable,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0b02845e7e9d..5cf8304564c6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1227,16 +1227,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	/* Get the MAC specific capabilities */
 	stmmac_mac_phylink_get_caps(priv);
 
-	if (priv->plat->has_xgmac) {
-		priv->phylink_config.mac_capabilities |= MAC_2500FD;
-		priv->phylink_config.mac_capabilities |= MAC_5000FD;
-		priv->phylink_config.mac_capabilities |= MAC_10000FD;
-		priv->phylink_config.mac_capabilities |= MAC_25000FD;
-		priv->phylink_config.mac_capabilities |= MAC_40000FD;
-		priv->phylink_config.mac_capabilities |= MAC_50000FD;
-		priv->phylink_config.mac_capabilities |= MAC_100000FD;
-	}
-
 	/* Half-Duplex can only work with single queue */
 	if (priv->plat->tx_queues_to_use > 1)
 		priv->phylink_config.mac_capabilities &=
-- 
2.30.2

[PATCH net-next 09/10] net: stmmac: move priv->phylink_config.mac_managed_pm

[Russell King (Oracle)]

Move priv->phylink_config.mac_managed_pm to be along side the other
phylink initialisations.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 5cf8304564c6..7cfc2918c913 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1207,6 +1207,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 
 	priv->phylink_config.dev = &priv->dev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
+	priv->phylink_config.mac_managed_pm = true;
 
 	mdio_bus_data = priv->plat->mdio_bus_data;
 	if (mdio_bus_data)
@@ -1231,7 +1232,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	if (priv->plat->tx_queues_to_use > 1)
 		priv->phylink_config.mac_capabilities &=
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
-	priv->phylink_config.mac_managed_pm = true;
 
 	max_speed = priv->plat->max_speed;
 	if (max_speed)
-- 
2.30.2

[PATCH net-next 10/10] net: stmmac: convert half-duplex support to positive logic

[Russell King (Oracle)]

Rather than detecting when half-duplex is not supported, and clearing
the MAC capabilities, reverse the if() condition and use it to set the
capabilities instead.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7cfc2918c913..33ca5c50bdcd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1223,16 +1223,17 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 				    priv->phylink_config.supported_interfaces);
 
 	priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
-		MAC_10 | MAC_100 | MAC_1000;
+						MAC_10FD | MAC_100FD |
+						MAC_1000FD;
+
+	/* Half-Duplex can only work with single queue */
+	if (priv->plat->tx_queues_to_use <= 1)
+		priv->phylink_config.mac_capabilities |= MAC_10HD | MAC_100HD |
+							 MAC_1000HD;
 
 	/* Get the MAC specific capabilities */
 	stmmac_mac_phylink_get_caps(priv);
 
-	/* Half-Duplex can only work with single queue */
-	if (priv->plat->tx_queues_to_use > 1)
-		priv->phylink_config.mac_capabilities &=
-			~(MAC_10HD | MAC_100HD | MAC_1000HD);
-
 	max_speed = priv->plat->max_speed;
 	if (max_speed)
 		phylink_limit_mac_speed(&priv->phylink_config, max_speed);
-- 
2.30.2

Re: [PATCH net-next v2 0/10] stmmac cleanups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 24 Aug 2023 14:37:24 +0100 you wrote:
> Hi,
> 
> One of the comments I had on Feiyang Chen's series was concerning the
> initialisation of phylink... and so I've decided to do something about
> it, cleaning it up a bit.
> 
> This series:
> 
> [...]

Here is the summary with links:
  - [net-next,01/10] net: phylink: add phylink_limit_mac_speed()
    https://git.kernel.org/netdev/net-next/c/70934c7c99ad
  - [net-next,02/10] net: stmmac: convert plat->phylink_node to fwnode
    https://git.kernel.org/netdev/net-next/c/e80af2acdef7
  - [net-next,03/10] net: stmmac: clean up passing fwnode to phylink
    https://git.kernel.org/netdev/net-next/c/1a37c1c19832
  - [net-next,04/10] net: stmmac: use "mdio_bus_data" local variable
    https://git.kernel.org/netdev/net-next/c/2b070cdd3afd
  - [net-next,05/10] net: stmmac: use phylink_limit_mac_speed()
    https://git.kernel.org/netdev/net-next/c/a4ac612bd345
  - [net-next,06/10] net: stmmac: provide stmmac_mac_phylink_get_caps()
    https://git.kernel.org/netdev/net-next/c/d42ca04e0448
  - [net-next,07/10] net: stmmac: move gmac4 specific phylink capabilities to gmac4
    https://git.kernel.org/netdev/net-next/c/f1dae3d222c6
  - [net-next,08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core
    https://git.kernel.org/netdev/net-next/c/bedf9b81233d
  - [net-next,09/10] net: stmmac: move priv->phylink_config.mac_managed_pm
    https://git.kernel.org/netdev/net-next/c/64961f1b8ca1
  - [net-next,10/10] net: stmmac: convert half-duplex support to positive logic
    https://git.kernel.org/netdev/net-next/c/76649fc93f09

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

[Serge Semin]

On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> Move the xgmac specific phylink capabilities to the dwxgmac2 support
> core.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index 34e1b0c3f346..f352be269deb 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
>  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
>  }
>  
> +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> +{
> +	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> +						 MAC_10000FD | MAC_25000FD |
> +						 MAC_40000FD | MAC_50000FD |
> +						 MAC_100000FD;
> +}
> +
>  static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
>  {
>  	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
>  
>  const struct stmmac_ops dwxgmac210_ops = {
>  	.core_init = dwxgmac2_core_init,

> +	.phylink_get_caps = xgmac_phylink_get_caps,

This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps
speeds.

>  	.set_mac = dwxgmac2_set_mac,
>  	.rx_ipc = dwxgmac2_rx_ipc,
>  	.rx_queue_enable = dwxgmac2_rx_queue_enable,
> @@ -1551,6 +1560,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
>  
>  const struct stmmac_ops dwxlgmac2_ops = {
>  	.core_init = dwxgmac2_core_init,

> +	.phylink_get_caps = xgmac_phylink_get_caps,

This is ok.

-Serge(y)

>  	.set_mac = dwxgmac2_set_mac,
>  	.rx_ipc = dwxgmac2_rx_ipc,
>  	.rx_queue_enable = dwxlgmac2_rx_queue_enable,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0b02845e7e9d..5cf8304564c6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1227,16 +1227,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
>  	/* Get the MAC specific capabilities */
>  	stmmac_mac_phylink_get_caps(priv);
>  
> -	if (priv->plat->has_xgmac) {
> -		priv->phylink_config.mac_capabilities |= MAC_2500FD;
> -		priv->phylink_config.mac_capabilities |= MAC_5000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_10000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_25000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_40000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_50000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_100000FD;
> -	}
> -
>  	/* Half-Duplex can only work with single queue */
>  	if (priv->plat->tx_queues_to_use > 1)
>  		priv->phylink_config.mac_capabilities &=
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

[Serge Semin]

On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> Move the xgmac specific phylink capabilities to the dwxgmac2 support
> core.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index 34e1b0c3f346..f352be269deb 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
>  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
>  }
>  

> +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)

Also after splitting this method up into DW XGMAC v2.x and DW XLGMAC
v2.x specific functions please preserve the local naming convention:
use dwxgmac2_ and dwxlgmac2_ prefixes.

-Serge(y)

> +{
> +	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> +						 MAC_10000FD | MAC_25000FD |
> +						 MAC_40000FD | MAC_50000FD |
> +						 MAC_100000FD;
> +}
> +
>  static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
>  {
>  	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
>  
>  const struct stmmac_ops dwxgmac210_ops = {
>  	.core_init = dwxgmac2_core_init,
> +	.phylink_get_caps = xgmac_phylink_get_caps,
>  	.set_mac = dwxgmac2_set_mac,
>  	.rx_ipc = dwxgmac2_rx_ipc,
>  	.rx_queue_enable = dwxgmac2_rx_queue_enable,
> @@ -1551,6 +1560,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
>  
>  const struct stmmac_ops dwxlgmac2_ops = {
>  	.core_init = dwxgmac2_core_init,
> +	.phylink_get_caps = xgmac_phylink_get_caps,
>  	.set_mac = dwxgmac2_set_mac,
>  	.rx_ipc = dwxgmac2_rx_ipc,
>  	.rx_queue_enable = dwxlgmac2_rx_queue_enable,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0b02845e7e9d..5cf8304564c6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1227,16 +1227,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
>  	/* Get the MAC specific capabilities */
>  	stmmac_mac_phylink_get_caps(priv);
>  
> -	if (priv->plat->has_xgmac) {
> -		priv->phylink_config.mac_capabilities |= MAC_2500FD;
> -		priv->phylink_config.mac_capabilities |= MAC_5000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_10000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_25000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_40000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_50000FD;
> -		priv->phylink_config.mac_capabilities |= MAC_100000FD;
> -	}
> -
>  	/* Half-Duplex can only work with single queue */
>  	if (priv->plat->tx_queues_to_use > 1)
>  		priv->phylink_config.mac_capabilities &=
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

[Russell King (Oracle)]

On Sat, Aug 26, 2023 at 04:32:15PM +0300, Serge Semin wrote:
> On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> > Move the xgmac specific phylink capabilities to the dwxgmac2 support
> > core.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index 34e1b0c3f346..f352be269deb 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
> >  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
> >  }
> >  
> > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> > +{
> > +	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> > +						 MAC_10000FD | MAC_25000FD |
> > +						 MAC_40000FD | MAC_50000FD |
> > +						 MAC_100000FD;
> > +}
> > +
> >  static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
> >  {
> >  	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
> >  
> >  const struct stmmac_ops dwxgmac210_ops = {
> >  	.core_init = dwxgmac2_core_init,
> 
> > +	.phylink_get_caps = xgmac_phylink_get_caps,
> 
> This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps
> speeds.

So the reason this got added is to keep the code compatible with how
things work today.

When priv->plat->has_xgmac is true, the old code in stmmac_phy_setup()
would enable speeds from 2.5G up to 100G, limiting them if
priv->plat->max_speed is set non-zero.

The table in hwif.c matches when:
	entry->gmac == priv->plat->has_gmac,
	entry->gmac4 == priv->plat->has_gmac4 and
	entry->xgmac == priv->plat->has_xgmac

The entries in the table which patch on has_xgmac = true contain the
following:

                .mac = &dwxgmac210_ops,
                .mac = &dwxlgmac2_ops,

Therefore, to keep things compatible, I've effectively moved this
initialisation into the new .phylink_get_caps method that is part of
those two ops, and since they have has_xgmac true, this means that
all these speeds need to be set.

We do this without regard to max_speed, which we apply separately,
after the .phylink_get_caps method has returned.

So, the code is functionally identical to what happens in the driver,
even if it is the case that xgmac210 doesn't actually support the
speeds. If those extra speeds that the hardware doesn't support were
present before, they're present after. If those extra speeds are
limited by the max_speed, then they will be similarly limited.

While it may look odd, since the specifications for Synopsys are all
behind closed doors, all I can do is transform the code - I can't
know that such-and-such a core doesn't actually support stuff. So
my only option is to keep the code bug-compatible.

I think all I've done here is make it glaringly obvious what the old
code is doing and you've spotted "but that isn't right!" - which is
actually a good thing!

Feel free to submit patches to correct the functionality as bugs in
the driver become more obvious!

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

Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

[Russell King (Oracle)]

On Sat, Aug 26, 2023 at 04:36:46PM +0300, Serge Semin wrote:
> On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> > Move the xgmac specific phylink capabilities to the dwxgmac2 support
> > core.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index 34e1b0c3f346..f352be269deb 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
> >  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
> >  }
> >  
> 
> > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> 
> Also after splitting this method up into DW XGMAC v2.x and DW XLGMAC
> v2.x specific functions please preserve the local naming convention:
> use dwxgmac2_ and dwxlgmac2_ prefixes.

The only possibility I have would be to implement two functions with
different names but are functionally identical, since I have no further
information. The new code is functionally identical to the code it
replaces - as explained in my previous response.

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

Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink caps to dwxgmac2 core

[Serge Semin]

On Sat, Aug 26, 2023 at 03:51:53PM +0100, Russell King (Oracle) wrote:
> On Sat, Aug 26, 2023 at 04:32:15PM +0300, Serge Semin wrote:
> > On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> > > Move the xgmac specific phylink capabilities to the dwxgmac2 support
> > > core.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
> > >  2 files changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > index 34e1b0c3f346..f352be269deb 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
> > >  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
> > >  }
> > >  
> > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> > > +{
> > > +	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> > > +						 MAC_10000FD | MAC_25000FD |
> > > +						 MAC_40000FD | MAC_50000FD |
> > > +						 MAC_100000FD;
> > > +}
> > > +
> > >  static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
> > >  {
> > >  	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> > > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
> > >  
> > >  const struct stmmac_ops dwxgmac210_ops = {
> > >  	.core_init = dwxgmac2_core_init,
> > 
> > > +	.phylink_get_caps = xgmac_phylink_get_caps,
> > 
> > This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps
> > speeds.
> 
> So the reason this got added is to keep the code compatible with how
> things work today.
> 
> When priv->plat->has_xgmac is true, the old code in stmmac_phy_setup()
> would enable speeds from 2.5G up to 100G, limiting them if
> priv->plat->max_speed is set non-zero.

Indeed. I didn't consider it has been coded like that long before this
discussion.

> 
> The table in hwif.c matches when:
> 	entry->gmac == priv->plat->has_gmac,
> 	entry->gmac4 == priv->plat->has_gmac4 and
> 	entry->xgmac == priv->plat->has_xgmac
> 
> The entries in the table which patch on has_xgmac = true contain the
> following:
> 
>                 .mac = &dwxgmac210_ops,
>                 .mac = &dwxlgmac2_ops,
> 
> Therefore, to keep things compatible, I've effectively moved this
> initialisation into the new .phylink_get_caps method that is part of
> those two ops, and since they have has_xgmac true, this means that
> all these speeds need to be set.
> 
> We do this without regard to max_speed, which we apply separately,
> after the .phylink_get_caps method has returned.
> 
> So, the code is functionally identical to what happens in the driver,
> even if it is the case that xgmac210 doesn't actually support the
> speeds. If those extra speeds that the hardware doesn't support were
> present before, they're present after. If those extra speeds are
> limited by the max_speed, then they will be similarly limited.

> 
> While it may look odd, since the specifications for Synopsys are all
> behind closed doors, all I can do is transform the code - I can't
> know that such-and-such a core doesn't actually support stuff. So
> my only option is to keep the code bug-compatible.
> 

If I see odd things and have no specs at hand I normally dig into the
git log in order to find a respective commit, then get to finding the
corresponding lkml discussion which may have some clue of possible
oddness justification. In this case the problematic part was added in
the commit 8a880936e902 ("net: stmmac: Add XLGMII support"). So before
having the XLGMAC support added the DW XGMAC had had standard speeds
support. Seeing there were no discussion concerning that part of the
code within the review it was very likely wrong to add the higher
speeds support to the DW XMGAC controller. But of course with no
databook at hand it gets to be still an assumption but with high
probability to be true though.

> I think all I've done here is make it glaringly obvious what the old
> code is doing and you've spotted "but that isn't right!" - which is
> actually a good thing!
> 
> Feel free to submit patches to correct the functionality as bugs in
> the driver become more obvious!

I see your point. Ok I'll add such fix to my extensive collection of
the STMMAC driver bug-fixes.) I'll send it out to the community
eventually when I get to have more spare time for review.

-Serge(y)

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