[net-next: PATCH v3 0/6] ACPI MDIO support for Marvell controllers

Hi,

The third version of the patchset main change is
dropping a clock handling optimisation patch
for mvmdio driver. Other than that it sets
explicit dependency on FWNODE_MDIO for CONFIG_FSL_XGMAC_MDIO
and applies minor cosmetic improvements (please see the
'Changelog' below).

The firmware ACPI description is exposed in the public github branch:
https://github.com/semihalf-wojtas-marcin/edk2-platforms/commits/acpi-mdio-r20210613
There is also MacchiatoBin firmware binary available for testing:
https://drive.google.com/file/d/1eigP_aeM4wYQpEaLAlQzs3IN_w1-kQr0

I'm looking forward to the comments or remarks.

Best regards,
Marcin

Changelog:
v2->v3
* Rebase on top of net-next/master.
* Drop "net: mvmdio: simplify clock handling" patch.
* 1/6 - fix code block comments.
* 2/6 - unchanged
* 3/6 - add "depends on FWNODE_MDIO" for CONFIG_FSL_XGMAC_MDIO
* 4/6 - drop mention about the clocks from the commit message.
* 5/6 - unchanged
* 6/6 - add Andrew's RB.

v1->v2
* 1/7 - new patch
* 2/7 - new patch
* 3/7 - new patch
* 4/7 - new patch
* 5/7 - remove unnecessary `if (has_acpi_companion())` and rebase onto
        the new clock handling
* 6/7 - remove deprecated comment
* 7/7 - no changes

Marcin Wojtas (6):
  Documentation: ACPI: DSD: describe additional MAC configuration
  net: mdiobus: Introduce fwnode_mdbiobus_register()
  net/fsl: switch to fwnode_mdiobus_register
  net: mvmdio: add ACPI support
  net: mvpp2: enable using phylink with ACPI
  net: mvpp2: remove unused 'has_phy' field

 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      |  3 -
 include/linux/fwnode_mdio.h                     | 12 ++++
 drivers/net/ethernet/freescale/xgmac_mdio.c     | 11 +---
 drivers/net/ethernet/marvell/mvmdio.c           | 14 ++++-
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 23 ++++++--
 drivers/net/mdio/fwnode_mdio.c                  | 22 ++++++++
 Documentation/firmware-guide/acpi/dsd/phy.rst   | 59 ++++++++++++++++++++
 drivers/net/ethernet/freescale/Kconfig          |  4 +-
 8 files changed, 125 insertions(+), 23 deletions(-)

-- 
2.29.0

Document additional MAC configuration modes which can be processed
by the existing fwnode_ phylink helpers:

* "managed" standard ACPI _DSD property [1]
* "fixed-link" data-only subnode linked in the _DSD package via
  generic mechanism of the hierarchical data extension [2]

[1] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
[2] https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.pdf

Signed-off-by: Marcin Wojtas <redacted>
---
 Documentation/firmware-guide/acpi/dsd/phy.rst | 59 ++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
index 0d49bad2ea9c..680ad179e5f9 100644
--- a/Documentation/firmware-guide/acpi/dsd/phy.rst
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst

@@ -50,6 +50,21 @@ phy-mode
 The "phy-mode" _DSD property is used to describe the connection to
 the PHY. The valid values for "phy-mode" are defined in [4].
 
+managed
+-------
+Optional property, which specifies the PHY management type.
+The valid values for "managed" are defined in [4].
+
+fixed-link
+----------
+The "fixed-link" is described by a data-only subnode of the
+MAC port, which is linked in the _DSD package via
+hierarchical data extension (UUID dbb8e3e6-5886-4ba6-8795-1319f52a966b
+in accordance with [5] "_DSD Implementation Guide" document).
+The subnode should comprise a required property ("speed") and
+possibly the optional ones - complete list of parameters and
+their values are specified in [4].
+
 The following ASL example illustrates the usage of these properties.
 
 DSDT entry for MDIO node

@@ -128,6 +143,48 @@ phy-mode and phy-handle are used as explained earlier.
 	  })
 	}
 
+MAC node example where "managed" property is specified.
+-------------------------------------------------------
+
+.. code-block:: none
+
+	Scope(\_SB.PP21.ETH0)
+	{
+	  Name (_DSD, Package () {
+	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package () {"phy-mode", "sgmii"},
+		     Package () {"managed", "in-band-status"}
+		 }
+	   })
+	}
+
+MAC node example with a "fixed-link" subnode.
+---------------------------------------------
+
+.. code-block:: none
+
+	Scope(\_SB.PP21.ETH1)
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package () {"phy-mode", "sgmii"},
+		 },
+	    ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		 Package () {
+		     Package () {"fixed-link", "LNK0"}
+		 }
+	  })
+	  Name (LNK0, Package(){ // Data-only subnode of port
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package () {"speed", 1000},
+		     Package () {"full-duplex", 1}
+		 }
+	  })
+	}
+
 References
 ==========
 

@@ -138,3 +195,5 @@ References
 [3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst
 
 [4] Documentation/devicetree/bindings/net/ethernet-controller.yaml
+
+[5] https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.pdf

-- 
2.29.0

This patch introduces a new helper function that
wraps acpi_/of_ mdiobus_register() and allows its
usage via common fwnode_ interface.

Fall back to raw mdiobus_register() in case CONFIG_FWNODE_MDIO
is not enabled, in order to satisfy compatibility
in all future user drivers.

Signed-off-by: Marcin Wojtas <redacted>
---
 include/linux/fwnode_mdio.h    | 12 +++++++++++
 drivers/net/mdio/fwnode_mdio.c | 22 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
index faf603c48c86..13d4ae8fee0a 100644
--- a/include/linux/fwnode_mdio.h
+++ b/include/linux/fwnode_mdio.h

@@ -16,6 +16,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 				struct fwnode_handle *child, u32 addr);
 
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode);
 #else /* CONFIG_FWNODE_MDIO */
 int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 				       struct phy_device *phy,

@@ -30,6 +31,17 @@ static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 {
 	return -EINVAL;
 }
+
+static inline int fwnode_mdiobus_register(struct mii_bus *bus,
+					  struct fwnode_handle *fwnode)
+{
+	/*
+	 * Fall back to mdiobus_register() function to register a bus.
+	 * This way, we don't have to keep compat bits around in drivers.
+	 */
+
+	return mdiobus_register(mdio);
+}
 #endif
 
 #endif /* __LINUX_FWNODE_MDIO_H */

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1becb1a731f6..ae0bf71a9932 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c

@@ -7,8 +7,10 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include <linux/fwnode_mdio.h>
 #include <linux/of.h>
+#include <linux/of_mdio.h>
 #include <linux/phy.h>
 
 MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>");

@@ -142,3 +144,23 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	return 0;
 }
 EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
+/**
+ * fwnode_mdiobus_register - bring up all the PHYs on a given MDIO bus and
+ *	attach them to it.
+ * @bus: Target MDIO bus.
+ * @fwnode: Pointer to fwnode of the MDIO controller.
+ *
+ * Return values are determined accordingly to acpi_/of_ mdiobus_register()
+ * operation.
+ */
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
+{
+	if (is_acpi_node(fwnode))
+		return acpi_mdiobus_register(bus, fwnode);
+	else if (is_of_node(fwnode))
+		return of_mdiobus_register(bus, to_of_node(fwnode));
+	else
+		return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register);

-- 
2.29.0

This patch introducing ACPI support for the mvmdio driver by adding
acpi_match_table with two entries:

* "MRVL0100" for the SMI operation
* "MRVL0101" for the XSMI mode

Signed-off-by: Marcin Wojtas <redacted>
---
 drivers/net/ethernet/marvell/mvmdio.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index d14762d93640..7537ee3f6622 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c

@@ -17,8 +17,10 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/fwnode_mdio.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>

@@ -281,7 +283,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	struct orion_mdio_dev *dev;
 	int i, ret;
 
-	type = (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+	type = (enum orion_mdio_bus_type)device_get_match_data(&pdev->dev);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r) {

@@ -369,7 +371,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 		goto out_mdio;
 	}
 
-	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
 		goto out_mdio;

@@ -421,12 +423,20 @@ static const struct of_device_id orion_mdio_match[] = {
 };
 MODULE_DEVICE_TABLE(of, orion_mdio_match);
 
+static const struct acpi_device_id orion_mdio_acpi_match[] = {
+	{ "MRVL0100", BUS_TYPE_SMI },
+	{ "MRVL0101", BUS_TYPE_XSMI },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, orion_mdio_acpi_match);
+
 static struct platform_driver orion_mdio_driver = {
 	.probe = orion_mdio_probe,
 	.remove = orion_mdio_remove,
 	.driver = {
 		.name = "orion-mdio",
 		.of_match_table = orion_mdio_match,
+		.acpi_match_table = ACPI_PTR(orion_mdio_acpi_match),
 	},
 };
 

-- 
2.29.0

Utilize the newly added helper routine
for registering the MDIO bus via fwnode_
interface.

Signed-off-by: Marcin Wojtas <redacted>
---
 drivers/net/ethernet/freescale/xgmac_mdio.c | 11 ++---------
 drivers/net/ethernet/freescale/Kconfig      |  4 +---
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 0b68852379da..2d99edc8a647 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c

@@ -13,7 +13,7 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/acpi_mdio.h>
+#include <linux/fwnode_mdio.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mdio.h>

@@ -246,7 +246,6 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 
 static int xgmac_mdio_probe(struct platform_device *pdev)
 {
-	struct fwnode_handle *fwnode;
 	struct mdio_fsl_priv *priv;
 	struct resource *res;
 	struct mii_bus *bus;

@@ -291,13 +290,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
 
-	fwnode = pdev->dev.fwnode;
-	if (is_of_node(fwnode))
-		ret = of_mdiobus_register(bus, to_of_node(fwnode));
-	else if (is_acpi_node(fwnode))
-		ret = acpi_mdiobus_register(bus, fwnode);
-	else
-		ret = -EINVAL;
+	ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot register MDIO bus\n");
 		goto err_registration;

diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
index 2d1abdd58fab..92a390576b88 100644
--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig

@@ -67,9 +67,7 @@ config FSL_PQ_MDIO
 
 config FSL_XGMAC_MDIO
 	tristate "Freescale XGMAC MDIO"
-	select PHYLIB
-	depends on OF
-	select OF_MDIO
+	depends on FWNODE_MDIO
 	help
 	  This driver supports the MDIO bus on the Fman 10G Ethernet MACs, and
 	  on the FMan mEMAC (which supports both Clauses 22 and 45)

-- 
2.29.0

Now that the MDIO and phylink are supported in the ACPI
world, enable to use them in the mvpp2 driver. Ensure a backward
compatibility with the firmware whose ACPI description does
not contain the necessary elements for the proper phy handling
and fall back to relying on the link interrupts instead.

Signed-off-by: Marcin Wojtas <redacted>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 22 +++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 9bca8c8f9f8d..a66ed3194015 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

@@ -4793,9 +4793,8 @@ static int mvpp2_open(struct net_device *dev)
 		goto err_cleanup_txqs;
 	}
 
-	/* Phylink isn't supported yet in ACPI mode */
-	if (port->of_node) {
-		err = phylink_of_phy_connect(port->phylink, port->of_node, 0);
+	if (port->phylink) {
+		err = phylink_fwnode_phy_connect(port->phylink, port->fwnode, 0);
 		if (err) {
 			netdev_err(port->dev, "could not attach PHY (%d)\n",
 				   err);

@@ -6703,6 +6702,19 @@ static void mvpp2_acpi_start(struct mvpp2_port *port)
 			  SPEED_UNKNOWN, DUPLEX_UNKNOWN, false, false);
 }
 
+/* In order to ensure backward compatibility for ACPI, check if the port
+ * firmware node comprises the necessary description allowing to use phylink.
+ */
+static bool mvpp2_use_acpi_compat_mode(struct fwnode_handle *port_fwnode)
+{
+	if (!is_acpi_node(port_fwnode))
+		return false;
+
+	return (!fwnode_property_present(port_fwnode, "phy-handle") &&
+		!fwnode_property_present(port_fwnode, "managed") &&
+		!fwnode_get_named_child_node(port_fwnode, "fixed-link"));
+}
+
 /* Ports initialization */
 static int mvpp2_port_probe(struct platform_device *pdev,
 			    struct fwnode_handle *port_fwnode,

@@ -6921,8 +6933,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 	dev->max_mtu = MVPP2_BM_JUMBO_PKT_SIZE;
 	dev->dev.of_node = port_node;
 
-	/* Phylink isn't used w/ ACPI as of now */
-	if (port_node) {
+	if (!mvpp2_use_acpi_compat_mode(port_fwnode)) {
 		port->phylink_config.dev = &dev->dev;
 		port->phylink_config.type = PHYLINK_NETDEV;
 

@@ -6934,6 +6945,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 		}
 		port->phylink = phylink;
 	} else {
+		dev_warn(&pdev->dev, "Use link irqs for port#%d. FW update required\n", port->id);
 		port->phylink = NULL;
 	}
 

-- 
2.29.0

The 'has_phy' field from struct mvpp2_port is no longer used.
Remove it.

Signed-off-by: Marcin Wojtas <redacted>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      | 3 ---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 1 -
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 4a61c90003b5..b9fbc9f000f2 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h

@@ -1197,9 +1197,6 @@ struct mvpp2_port {
 	/* Firmware node associated to the port */
 	struct fwnode_handle *fwnode;
 
-	/* Is a PHY always connected to the port */
-	bool has_phy;
-
 	/* Per-port registers' base address */
 	void __iomem *base;
 	void __iomem *stats_base;

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index a66ed3194015..8362e64a3b28 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

@@ -6790,7 +6790,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 	port = netdev_priv(dev);
 	port->dev = dev;
 	port->fwnode = port_fwnode;
-	port->has_phy = !!of_find_property(port_node, "phy", NULL);
 	port->ntxqs = ntxqs;
 	port->nrxqs = nrxqs;
 	port->priv = priv;

-- 
2.29.0

Hi,

pon., 21 cze 2021 o 19:31 Marcin Wojtas [off-list ref] napisał(a):

Hi,

The third version of the patchset main change is
dropping a clock handling optimisation patch
for mvmdio driver. Other than that it sets
explicit dependency on FWNODE_MDIO for CONFIG_FSL_XGMAC_MDIO
and applies minor cosmetic improvements (please see the
'Changelog' below).

The firmware ACPI description is exposed in the public github branch:
https://github.com/semihalf-wojtas-marcin/edk2-platforms/commits/acpi-mdio-r20210613
There is also MacchiatoBin firmware binary available for testing:
https://drive.google.com/file/d/1eigP_aeM4wYQpEaLAlQzs3IN_w1-kQr0

I'm looking forward to the comments or remarks.

I would really appreciate if this third revision can possibly get
attention some in coming days. There's still some buffer for
improvements, so that in case of no objections, it can be queued for
v5.14 via net-next tree.

Thanks,
Marcin

Marcin

Changelog:
v2->v3
* Rebase on top of net-next/master.
* Drop "net: mvmdio: simplify clock handling" patch.
* 1/6 - fix code block comments.
* 2/6 - unchanged
* 3/6 - add "depends on FWNODE_MDIO" for CONFIG_FSL_XGMAC_MDIO
* 4/6 - drop mention about the clocks from the commit message.
* 5/6 - unchanged
* 6/6 - add Andrew's RB.

v1->v2
* 1/7 - new patch
* 2/7 - new patch
* 3/7 - new patch
* 4/7 - new patch
* 5/7 - remove unnecessary `if (has_acpi_companion())` and rebase onto
        the new clock handling
* 6/7 - remove deprecated comment
* 7/7 - no changes

Marcin Wojtas (6):
  Documentation: ACPI: DSD: describe additional MAC configuration
  net: mdiobus: Introduce fwnode_mdbiobus_register()
  net/fsl: switch to fwnode_mdiobus_register
  net: mvmdio: add ACPI support
  net: mvpp2: enable using phylink with ACPI
  net: mvpp2: remove unused 'has_phy' field

 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      |  3 -
 include/linux/fwnode_mdio.h                     | 12 ++++
 drivers/net/ethernet/freescale/xgmac_mdio.c     | 11 +---
 drivers/net/ethernet/marvell/mvmdio.c           | 14 ++++-
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 23 ++++++--
 drivers/net/mdio/fwnode_mdio.c                  | 22 ++++++++
 Documentation/firmware-guide/acpi/dsd/phy.rst   | 59 ++++++++++++++++++++
 drivers/net/ethernet/freescale/Kconfig          |  4 +-
 8 files changed, 125 insertions(+), 23 deletions(-)

--
2.29.0

+MAC node example with a "fixed-link" subnode.
+---------------------------------------------
+
+.. code-block:: none
+
+	Scope(\_SB.PP21.ETH1)
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package () {"phy-mode", "sgmii"},
+		 },
+	    ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		 Package () {
+		     Package () {"fixed-link", "LNK0"}
+		 }
+	  })

At least in the DT world, it is pretty unusual to see both fixed-link
and phy-mode. You might have one of the four RGMII modes, in order to
set the delays when connecting to a switch. But sgmii and fixed link
seems very unlikely, how is sgmii autoneg going to work?

+	  Name (LNK0, Package(){ // Data-only subnode of port
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package () {"speed", 1000},
+		     Package () {"full-duplex", 1}
+		 }
+	  })
+	}
+

  Andrew

On Mon, Jun 21, 2021 at 07:30:24PM +0200, Marcin Wojtas wrote:

I looked at this some more, and in the end i decided it was O.K.

+/**
+ * fwnode_mdiobus_register - bring up all the PHYs on a given MDIO bus and
+ *	attach them to it.
+ * @bus: Target MDIO bus.
+ * @fwnode: Pointer to fwnode of the MDIO controller.
+ *
+ * Return values are determined accordingly to acpi_/of_ mdiobus_register()
+ * operation.
+ */
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
+{
+	if (is_acpi_node(fwnode))
+		return acpi_mdiobus_register(bus, fwnode);
+	else if (is_of_node(fwnode))
+		return of_mdiobus_register(bus, to_of_node(fwnode));
+	else
+		return -EINVAL;

I wounder if here you should call mdiobus_register(mdio), rather than
-EINVAL?

I don't have a strong opinion.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Mon, Jun 21, 2021 at 07:30:25PM +0200, Marcin Wojtas wrote:

Utilize the newly added helper routine
for registering the MDIO bus via fwnode_
interface.

Signed-off-by: Marcin Wojtas <redacted>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Mon, Jun 21, 2021 at 07:30:26PM +0200, Marcin Wojtas wrote:

This patch introducing ACPI support for the mvmdio driver by adding
acpi_match_table with two entries:

* "MRVL0100" for the SMI operation
* "MRVL0101" for the XSMI mode

Same as the freescale MDIO bus driver, you should add

depends on FWNODE_MDIO

Otherwise you might find randconfig builds end up with it disabled,
and then linker errors.

       Andrew

+static bool mvpp2_use_acpi_compat_mode(struct fwnode_handle *port_fwnode)
+{
+	if (!is_acpi_node(port_fwnode))
+		return false;
+
+	return (!fwnode_property_present(port_fwnode, "phy-handle") &&
+		!fwnode_property_present(port_fwnode, "managed") &&
+		!fwnode_get_named_child_node(port_fwnode, "fixed-link"));

I'm not too sure about this last one. You only use fixed-link when
connecting to an Ethernet switch. I doubt anybody will try ACPI and a
switch. It has been agreed, ACPI is for simple hardware, and you need
to use DT for advanced hardware configurations.

What is your use case for fixed-link?

     Andrew

Hi Andrew,

śr., 23 cze 2021 o 22:18 Andrew Lunn [off-list ref] napisał(a):

quoted

+MAC node example with a "fixed-link" subnode.
+---------------------------------------------
+
+.. code-block:: none
+
+     Scope(\_SB.PP21.ETH1)
+     {
+       Name (_DSD, Package () {
+         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+              Package () {
+                  Package () {"phy-mode", "sgmii"},
+              },
+         ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+              Package () {
+                  Package () {"fixed-link", "LNK0"}
+              }
+       })

At least in the DT world, it is pretty unusual to see both fixed-link
and phy-mode.

I did a quick experiment:
git grep -C 8 fixed-link arch/arm64/boot/dts/
git grep -C 8 fixed-link arch/arm/boot/dts/
almost all MAC nodes (i.e. not switch ports) containing 'fixed-link'
have an adjacent 'phy-mode' property defined. How else would the
drivers know how to configure the HW connection type in its registers?

You might have one of the four RGMII modes, in order to
set the delays when connecting to a switch. But sgmii and fixed link
seems very unlikely, how is sgmii autoneg going to work?

Indeed most cases in the tree are "rgmii*", but we can also see e.g.
10gbase-r, sgmii and 2500base-x. You can find sgmii + fixed-link on
the eth1 of the armada-388-clearfog. Regarding the autoneg - I'm
mostly familiar with the mvneta/mvpp2, but in this mode the
autonegotiation is disabled and the link is forcibly set up/down in
MAC registers during the netedev_open/close accordingly. FYI, along
with the 10G ports on CN913x-DB, I tested fixed-link on Macchiatobin
sgmii port.

Anyway - all above is a bit side discussion to the actual DSDT
description and how the fixed-link subnode looks like. I think
phy-mode set to "sgmii" is not incorrect, but we can change it to
whatever other type of your preference, as well.

Best regards,
Marcin

quoted

+       Name (LNK0, Package(){ // Data-only subnode of port
+         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+              Package () {
+                  Package () {"speed", 1000},
+                  Package () {"full-duplex", 1}
+              }
+       })
+     }
+

  Andrew

Anyway - all above is a bit side discussion to the actual DSDT
description and how the fixed-link subnode looks like. I think
phy-mode set to "sgmii" is not incorrect, but we can change it to
whatever other type of your preference, as well.

rgmii would be better, so we side step the whole auto-neg discussion.

      Andrew

Hi Andrew,

On Wed, Jun 23, 2021 at 10:18:02PM +0200, Andrew Lunn wrote:

quoted

+MAC node example with a "fixed-link" subnode.
+---------------------------------------------
+
+.. code-block:: none
+
+	Scope(\_SB.PP21.ETH1)
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package () {"phy-mode", "sgmii"},
+		 },
+	    ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		 Package () {
+		     Package () {"fixed-link", "LNK0"}
+		 }
+	  })

At least in the DT world, it is pretty unusual to see both fixed-link
and phy-mode. You might have one of the four RGMII modes, in order to
set the delays when connecting to a switch. But sgmii and fixed link
seems very unlikely, how is sgmii autoneg going to work?

SGMII autoneg is supposed to be disabled if you have a fixed-link, and
there is nothing unusual in that kind of setup.
There are 3 types of phylink setups:

MLO_AN_INBAND: there might or might not be a phy-handle, but SGMII
               autoneg should be enabled and the MAC should be
               reconfigured automatically (in hardware) to the right
               speed/duplex based on that
MLO_AN_PHY: there is a phy-handle but SGMII autoneg should be disabled*
            and the MAC should be reconfigured (forced) in software to
            the speed/duplex determined by reading the PHY MDIO
            registers
MLO_AN_FIXED: there is no phy-handle or phy_device, but the driver
              should do the same thing, the speed/duplex is configured
              by management (in this case DT/ACPI)

*there appears to be some debate here, since the "managed" property is
phylink-specific and therefore a phylib driver will not necessarily
disable its in-band autoneg, but this is what the existing phylink_pcs
drivers in drivers/net/pcs/ do and I think there's nothing wrong with
settling on that if phylink is being used. It does create some interesting
questions though when a driver is being converted from phylib to phylink,
since the meaning of the existing firmware bindings suddenly changes.

An SGMII link with MLO_AN_FIXED is nothing unusual, it is in fact very
widespread as a way to reduce pin count compared to the parallel RGMII.
I suspect there are more DSA setups in the field with an SGMII fixed-link
than with RGMII fixed-link, due to practicality.

The 2 characteristic features of SGMII compared to 1000base-X are:
- customization of the 16-bit configuration word communicated via the
  clause 37 state machines. Those are bypassed in MLO_AN_PHY and
  MLO_AN_FIXED modes, true
- symbol replication at 10/100 speeds.

So since it is equally valid to have an SGMII fixed-link at 100Mbps or
10Mbps, it is just as valid to have an SGMII fixed-link at 1Gbps with
in-band autoneg disabled.

Hi,

śr., 23 cze 2021 o 22:37 Andrew Lunn [off-list ref] napisał(a):

quoted

+static bool mvpp2_use_acpi_compat_mode(struct fwnode_handle *port_fwnode)
+{
+     if (!is_acpi_node(port_fwnode))
+             return false;
+
+     return (!fwnode_property_present(port_fwnode, "phy-handle") &&
+             !fwnode_property_present(port_fwnode, "managed") &&
+             !fwnode_get_named_child_node(port_fwnode, "fixed-link"));

I'm not too sure about this last one. You only use fixed-link when
connecting to an Ethernet switch. I doubt anybody will try ACPI and a
switch. It has been agreed, ACPI is for simple hardware, and you need
to use DT for advanced hardware configurations.

What is your use case for fixed-link?

Regardless of the "simple hardware" definition or whether DSA + ACPI
feasibility, you can still have e.g. the switch left in "unmanaged"
mode (or whatever the firmware configures), connected via fixed-link
to the MAC. The same effect as booting with DT, but not loading the
DSA/switch driver - the "CPU port" can be used as a normal netdev
interface.

I'd also prefer to have all 3 major interface types supported in
phylink, explicitly checked in the driver - it has not been supported
yet, but can be in the future, so let's have them covered in the
backward compatibility check.

Best regards,
Marcin

śr., 23 cze 2021 o 22:28 Andrew Lunn [off-list ref] napisał(a):

On Mon, Jun 21, 2021 at 07:30:26PM +0200, Marcin Wojtas wrote:

quoted

This patch introducing ACPI support for the mvmdio driver by adding
acpi_match_table with two entries:

* "MRVL0100" for the SMI operation
* "MRVL0101" for the XSMI mode

Same as the freescale MDIO bus driver, you should add

depends on FWNODE_MDIO

Otherwise you might find randconfig builds end up with it disabled,
and then linker errors.

The CONFIG_MVMDIO is selected by CONFIG_MV643XX_ETH and actually there
is a real example of the previously discussed fallback to the
mdiobus_register() (without DT/ACPI and now FWNODE_MDIO). I just
checked and successfully built the kernel out of the dove_defconfig. I
only needed below fix, that will be submitted in v4:

--- a/include/linux/fwnode_mdio.h
+++ b/include/linux/fwnode_mdio.h

@@ -40,7 +40,7 @@ static inline int fwnode_mdiobus_register(struct mii_bus *bus,
         * This way, we don't have to keep compat bits around in drivers.
         */

-       return mdiobus_register(mdio);
+       return mdiobus_register(bus);
 }
 #endif

In order to leave dove_defconfig intact, I'd keep the current Kconfig
shape for this driver.

Thanks,
Marcin

Hi,

śr., 23 cze 2021 o 22:22 Andrew Lunn [off-list ref] napisał(a):

On Mon, Jun 21, 2021 at 07:30:24PM +0200, Marcin Wojtas wrote:

quoted

This patch introduces a new helper function that
wraps acpi_/of_ mdiobus_register() and allows its
usage via common fwnode_ interface.

Fall back to raw mdiobus_register() in case CONFIG_FWNODE_MDIO
is not enabled, in order to satisfy compatibility
in all future user drivers.

Signed-off-by: Marcin Wojtas <redacted>
---
 include/linux/fwnode_mdio.h    | 12 +++++++++++
 drivers/net/mdio/fwnode_mdio.c | 22 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
index faf603c48c86..13d4ae8fee0a 100644
--- a/include/linux/fwnode_mdio.h
+++ b/include/linux/fwnode_mdio.h

@@ -16,6 +16,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
                              struct fwnode_handle *child, u32 addr);

+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode);
 #else /* CONFIG_FWNODE_MDIO */
 int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
                                     struct phy_device *phy,

@@ -30,6 +31,17 @@ static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 {
      return -EINVAL;
 }
+
+static inline int fwnode_mdiobus_register(struct mii_bus *bus,
+                                       struct fwnode_handle *fwnode)
+{
+     /*
+      * Fall back to mdiobus_register() function to register a bus.
+      * This way, we don't have to keep compat bits around in drivers.
+      */
+
+     return mdiobus_register(mdio);
+}
 #endif

I looked at this some more, and in the end i decided it was O.K.

quoted

+/**
+ * fwnode_mdiobus_register - bring up all the PHYs on a given MDIO bus and
+ *   attach them to it.
+ * @bus: Target MDIO bus.
+ * @fwnode: Pointer to fwnode of the MDIO controller.
+ *
+ * Return values are determined accordingly to acpi_/of_ mdiobus_register()
+ * operation.
+ */
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
+{
+     if (is_acpi_node(fwnode))
+             return acpi_mdiobus_register(bus, fwnode);
+     else if (is_of_node(fwnode))
+             return of_mdiobus_register(bus, to_of_node(fwnode));
+     else
+             return -EINVAL;

I wounder if here you should call mdiobus_register(mdio), rather than
-EINVAL?

I don't have a strong opinion.

Currently (and in foreseeable future) we support only DT/ACPI as a
firmware description, reaching the last "else" means something really
wrong. The case of lack of DT/ACPI and the fallback is handled on the
include/linux/fwnode_mdio.h level.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks,
Marcin

On Wed, Jun 23, 2021 at 11:45:04PM +0200, Marcin Wojtas wrote:

Hi,

śr., 23 cze 2021 o 22:37 Andrew Lunn [off-list ref] napisał(a):

quoted

quoted

+static bool mvpp2_use_acpi_compat_mode(struct fwnode_handle *port_fwnode)
+{
+     if (!is_acpi_node(port_fwnode))
+             return false;
+
+     return (!fwnode_property_present(port_fwnode, "phy-handle") &&
+             !fwnode_property_present(port_fwnode, "managed") &&
+             !fwnode_get_named_child_node(port_fwnode, "fixed-link"));

I'm not too sure about this last one. You only use fixed-link when
connecting to an Ethernet switch. I doubt anybody will try ACPI and a
switch. It has been agreed, ACPI is for simple hardware, and you need
to use DT for advanced hardware configurations.

What is your use case for fixed-link?

Regardless of the "simple hardware" definition or whether DSA + ACPI
feasibility, you can still have e.g. the switch left in "unmanaged"
mode (or whatever the firmware configures), connected via fixed-link
to the MAC. The same effect as booting with DT, but not loading the
DSA/switch driver - the "CPU port" can be used as a normal netdev
interface.

You can do this, but i would not recommend it. Without having STP,
your network is going to be vulnerable to broadcast storms killing
your network.

I'd also prefer to have all 3 major interface types supported in
phylink, explicitly checked in the driver - it has not been supported
yet, but can be in the future, so let's have them covered in the
backward compatibility check.

Maybe i'm not understanding this correctly, but isn't this condition
enforcing there must be a fixed link in order to use the new ACPI
binding? But i expect most boards never need a fixed-link, it is
optional after all.

	 Andrew

On Wed, Jun 23, 2021 at 11:58:14PM +0200, Marcin Wojtas wrote:

śr., 23 cze 2021 o 22:28 Andrew Lunn [off-list ref] napisał(a):

quoted

On Mon, Jun 21, 2021 at 07:30:26PM +0200, Marcin Wojtas wrote:

quoted

This patch introducing ACPI support for the mvmdio driver by adding
acpi_match_table with two entries:

* "MRVL0100" for the SMI operation
* "MRVL0101" for the XSMI mode

Same as the freescale MDIO bus driver, you should add

depends on FWNODE_MDIO

Otherwise you might find randconfig builds end up with it disabled,
and then linker errors.

The CONFIG_MVMDIO is selected by CONFIG_MV643XX_ETH and actually there
is a real example of the previously discussed fallback to the
mdiobus_register() (without DT/ACPI and now FWNODE_MDIO). I just
checked and successfully built the kernel out of the dove_defconfig. I
only needed below fix, that will be submitted in v4:

You could be correct, but i've seen randconfig builds find issues. So
i tend to add dependencies to avoid possible problems.  Such problem
reports tend to come weeks later, when Arnd does such builds.

	Andrew

Hi,

czw., 24 cze 2021 o 00:10 Marcin Wojtas [off-list ref] napisał(a):

Hi,

śr., 23 cze 2021 o 22:22 Andrew Lunn [off-list ref] napisał(a):

quoted

On Mon, Jun 21, 2021 at 07:30:24PM +0200, Marcin Wojtas wrote:

quoted

This patch introduces a new helper function that
wraps acpi_/of_ mdiobus_register() and allows its
usage via common fwnode_ interface.

Fall back to raw mdiobus_register() in case CONFIG_FWNODE_MDIO
is not enabled, in order to satisfy compatibility
in all future user drivers.

Signed-off-by: Marcin Wojtas <redacted>
---
 include/linux/fwnode_mdio.h    | 12 +++++++++++
 drivers/net/mdio/fwnode_mdio.c | 22 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
index faf603c48c86..13d4ae8fee0a 100644
--- a/include/linux/fwnode_mdio.h
+++ b/include/linux/fwnode_mdio.h

@@ -16,6 +16,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
                              struct fwnode_handle *child, u32 addr);

+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode);
 #else /* CONFIG_FWNODE_MDIO */
 int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
                                     struct phy_device *phy,

@@ -30,6 +31,17 @@ static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 {
      return -EINVAL;
 }
+
+static inline int fwnode_mdiobus_register(struct mii_bus *bus,
+                                       struct fwnode_handle *fwnode)
+{
+     /*
+      * Fall back to mdiobus_register() function to register a bus.
+      * This way, we don't have to keep compat bits around in drivers.
+      */
+
+     return mdiobus_register(mdio);
+}
 #endif

I looked at this some more, and in the end i decided it was O.K.

quoted

+/**
+ * fwnode_mdiobus_register - bring up all the PHYs on a given MDIO bus and
+ *   attach them to it.
+ * @bus: Target MDIO bus.
+ * @fwnode: Pointer to fwnode of the MDIO controller.
+ *
+ * Return values are determined accordingly to acpi_/of_ mdiobus_register()
+ * operation.
+ */
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
+{
+     if (is_acpi_node(fwnode))
+             return acpi_mdiobus_register(bus, fwnode);
+     else if (is_of_node(fwnode))
+             return of_mdiobus_register(bus, to_of_node(fwnode));
+     else
+             return -EINVAL;

I wounder if here you should call mdiobus_register(mdio), rather than
-EINVAL?

I don't have a strong opinion.

Currently (and in foreseeable future) we support only DT/ACPI as a
firmware description, reaching the last "else" means something really
wrong. The case of lack of DT/ACPI and the fallback is handled on the
include/linux/fwnode_mdio.h level.

quoted

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Unfortunately I have to withdraw this patch, as well as xgmac_mdio
one. In case the fwnode_/of_/acpi_mdio are built as modules, we get a
cycle dependency during depmod phase of modules_install, eg.:

depmod: ERROR: Cycle detected: fwnode_mdio -> of_mdio -> fwnode_mdio
depmod: ERROR: Found 2 modules in dependency cycles!

OR:

depmod: ERROR: Cycle detected: acpi_mdio -> fwnode_mdio -> acpi_mdio
depmod: ERROR: Found 2 modules in dependency cycles!

The proper solution would be to merge contents of
acpi_mdiobus_register and of_mdiobus_register inside the common
fwnode_mdiobus_register (so that the former would only call the
latter). However this change seems feasible, but I'd expect it to be a
patchset bigger than this one alone and deserves its own thorough
review and testing, as it would affect huge amount of current
of_mdiobus_register users.

Given above, for now I will resubmit this patchset in the shape as
proposed in v1, i.e. using the 'if' condition explicitly in mvmdio
driver.

Best regards,
Marcin