ACPI MDIO support for Marvell controllers

Hi,

The second version of the patchset addresses all comments received
during v1 review and introduces a couple of new patches that
were requested.

fwnode_mdiobus_register() helper routine was added and it is used
now by 2 drivers (xgmac_mdio and mvmdio). In the latter a clock
handling was significantly simplified by a switch to
a devm_clk_bulk_get_optional().

Last but not least two additional MAC configuration modes ACPI
desctiption were documented ("managed" and "fixed-link") - they
can be processed by the existing fwnode_ phylink helpers and
comply with the standard _DSD properties and hierarchical
data extension. ACPI Maintainers are therefore added to reviewers' list.

More details can be found in the patches and their commit messages.

As before, the feature was verified with ACPI on MacchiatoBin, CN913x-DB
and Armada 8040 DB (fixed-link handling).
Moreover regression tests were performed (old firmware with updated kernel,
new firmware with old kernel and the operation with DT).

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:
v1->v2
* 1/7 - new patch
* 2/7 - new patch
* 3/7 - new patch
* 4/7 - new patch

26 more lines

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 | 55 ++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
index 7d01ae8b3cc6..839710b94a7f 100644
--- a/Documentation/firmware-guide/acpi/dsd/phy.rst
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst

@@ -49,6 +49,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 the 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

@@ -121,6 +136,44 @@ phy-mode and phy-handle are used as explained earlier.
 	  })
 	}
 
+MAC node example where "managed" property is specified.
+-------------------------------------------------------
+::
+	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.
+---------------------------------------------
+::
+	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
 ==========
 

@@ -131,3 +184,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 e96766da8de4..b6576af89fa1 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

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 ++---------
 1 file changed, 2 insertions(+), 9 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;

-- 
2.29.0

Because the mvmdio driver supports a wide range of Marvell
SoC families it must support multiple types of the
HW description and required resources.

Thanks to the devm_clk_bulk_get_optional() helper routine
the clock handling could be significantly simplified,
as it is compatible with all possible variants within
a single call.

Suggested-by: Andy Shevchenko <redacted>
Signed-off-by: Marcin Wojtas <redacted>
---
 drivers/net/ethernet/marvell/mvmdio.c | 61 ++++++--------------
 1 file changed, 18 insertions(+), 43 deletions(-)

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

@@ -62,9 +62,11 @@
 #define MVMDIO_XSMI_POLL_INTERVAL_MIN	150
 #define MVMDIO_XSMI_POLL_INTERVAL_MAX	160
 
+#define MVMDIO_CLOCK_COUNT		4
+
 struct orion_mdio_dev {
 	void __iomem *regs;
-	struct clk *clk[4];
+	struct clk_bulk_data clks[MVMDIO_CLOCK_COUNT];
 	/*
 	 * If we have access to the error interrupt pin (which is
 	 * somewhat misnamed as it not only reflects internal errors

@@ -279,7 +281,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	struct resource *r;
 	struct mii_bus *bus;
 	struct orion_mdio_dev *dev;
-	int i, ret;
+	int ret;
 
 	type = (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
 

@@ -319,33 +321,20 @@ static int orion_mdio_probe(struct platform_device *pdev)
 
 	init_waitqueue_head(&dev->smi_busy_wait);
 
-	if (pdev->dev.of_node) {
-		for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
-			dev->clk[i] = of_clk_get(pdev->dev.of_node, i);
-			if (PTR_ERR(dev->clk[i]) == -EPROBE_DEFER) {
-				ret = -EPROBE_DEFER;
-				goto out_clk;
-			}
-			if (IS_ERR(dev->clk[i]))
-				break;
-			clk_prepare_enable(dev->clk[i]);
-		}
-
-		if (!IS_ERR(of_clk_get(pdev->dev.of_node,
-				       ARRAY_SIZE(dev->clk))))
-			dev_warn(&pdev->dev,
-				 "unsupported number of clocks, limiting to the first "
-				 __stringify(ARRAY_SIZE(dev->clk)) "\n");
-	} else {
-		dev->clk[0] = clk_get(&pdev->dev, NULL);
-		if (PTR_ERR(dev->clk[0]) == -EPROBE_DEFER) {
-			ret = -EPROBE_DEFER;
-			goto out_clk;
-		}
-		if (!IS_ERR(dev->clk[0]))
-			clk_prepare_enable(dev->clk[0]);
-	}
+	dev->clks[0].id = "core";
+	dev->clks[1].id = "mg";
+	dev->clks[2].id = "mg_core";
+	dev->clks[3].id = "axi";
+	ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT,
+					 dev->clks);
+	if (ret)
+		return ret;
 
+	ret = clk_bulk_prepare_enable(MVMDIO_CLOCK_COUNT, dev->clks);
+	if (ret) {
+		dev_err(&pdev->dev, "Cannot enable clocks\n");
+		return ret;
+	}
 
 	dev->err_interrupt = platform_get_irq_optional(pdev, 0);
 	if (dev->err_interrupt > 0 &&

@@ -383,14 +372,6 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	if (dev->err_interrupt > 0)
 		writel(0, dev->regs + MVMDIO_ERR_INT_MASK);
 
-out_clk:
-	for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
-		if (IS_ERR(dev->clk[i]))
-			break;
-		clk_disable_unprepare(dev->clk[i]);
-		clk_put(dev->clk[i]);
-	}
-
 	return ret;
 }
 

@@ -398,18 +379,12 @@ static int orion_mdio_remove(struct platform_device *pdev)
 {
 	struct mii_bus *bus = platform_get_drvdata(pdev);
 	struct orion_mdio_dev *dev = bus->priv;
-	int i;
 
 	if (dev->err_interrupt > 0)
 		writel(0, dev->regs + MVMDIO_ERR_INT_MASK);
 	mdiobus_unregister(bus);
 
-	for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
-		if (IS_ERR(dev->clk[i]))
-			break;
-		clk_disable_unprepare(dev->clk[i]);
-		clk_put(dev->clk[i]);
-	}
+	clk_bulk_disable_unprepare(MVMDIO_CLOCK_COUNT, dev->clks);
 
 	return 0;
 }

-- 
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

Also clk enabling is skipped, because the tables do not contain
such data and clock maintenance relies on the firmware.

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 ce3ddc867898..4fe6428b8119 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>

@@ -283,7 +285,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	struct orion_mdio_dev *dev;
 	int 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) {

@@ -358,7 +360,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;

@@ -396,12 +398,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

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>
---
 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

On Wed, Jun 16, 2021 at 09:07:54PM +0200, Marcin Wojtas wrote:

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.

I'm not sure this fallback is correct.

Any driver which decides to use fwmode is going to select it. If it is
not selected, you want a link time error, or a compiler time error to
tell you, you are missing FWNODE_MDIO. Calling mdiobus_register() is
unlikely to work, or the driver would of done that directly.

	 Andrew

On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:

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

You need to add depends on FWNODE_MDIO

    Andrew

Kirkwood:

                mdio: mdio-bus@72004 {
                        compatible = "marvell,orion-mdio";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0x72004 0x84>;
                        interrupts = <46>;
                        clocks = <&gate_clk 0>;
                        status = "disabled";

Does this work? There is no clock-names in DT.

     Andrew

On Wed, Jun 16, 2021 at 09:07:57PM +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

Also clk enabling is skipped, because the tables do not contain
such data and clock maintenance relies on the firmware.

This last part seems to be no longer true.

     Andrew

On Wed, Jun 16, 2021 at 09:07:59PM +0200, Marcin Wojtas wrote:

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>

    Andrew

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

On Wed, Jun 16, 2021 at 09:07:57PM +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

Also clk enabling is skipped, because the tables do not contain
such data and clock maintenance relies on the firmware.

This last part seems to be no longer true.

Well, it is still relies on firmware (no clocks are passed via ACPI),
but skipping this enablement is hidden in the internals of
devm_clk_bulk_get_optional() and clk_bulk_prepare_enable().

Best regards,
Marcin

On Thu, Jun 17, 2021 at 12:37:06AM +0200, Marcin Wojtas wrote:

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

quoted

On Wed, Jun 16, 2021 at 09:07:57PM +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

Also clk enabling is skipped, because the tables do not contain
such data and clock maintenance relies on the firmware.

This last part seems to be no longer true.

Well, it is still relies on firmware (no clocks are passed via ACPI),
but skipping this enablement is hidden in the internals of
devm_clk_bulk_get_optional() and clk_bulk_prepare_enable().

A quick grep in driver/clk does not reveal any ACPI code. Nor did i
spot any generic clock code in drivers/acpi. So even if you did add
clocks to the tables, i don't see how they would be used.

       Andrew

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

quoted

+     dev->clks[0].id = "core";
+     dev->clks[1].id = "mg";
+     dev->clks[2].id = "mg_core";
+     dev->clks[3].id = "axi";
+     ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT,
+                                      dev->clks);

Kirkwood:

                mdio: mdio-bus@72004 {
                        compatible = "marvell,orion-mdio";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0x72004 0x84>;
                        interrupts = <46>;
                        clocks = <&gate_clk 0>;
                        status = "disabled";

Does this work? There is no clock-names in DT.

Neither are the clocks in Armada 7k8k / CN913x:

                CP11X_LABEL(mdio): mdio@12a200 {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        compatible = "marvell,orion-mdio";
                        reg = <0x12a200 0x10>;
                        clocks = <&CP11X_LABEL(clk) 1 9>,
<&CP11X_LABEL(clk) 1 5>,
                                 <&CP11X_LABEL(clk) 1 6>,
<&CP11X_LABEL(clk) 1 18>;
                        status = "disabled";
                };

Apparently I misread the code and got convinced that contrary to
devm_clk_get_optional(), the devm_clk_get_bulk_optional() obtains the
clocks directly by index, not name (on the tested boards, the same
clocks are enabled by the other interfaces, so the problems

I will drop this patch. Thank you for spotting the issue.

Best regards,
Marcin

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

On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:

quoted

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

You need to add depends on FWNODE_MDIO

Do you mean something like this?

--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig

@@ -68,8 +68,8 @@ config FSL_PQ_MDIO
 config FSL_XGMAC_MDIO
        tristate "Freescale XGMAC MDIO"
        select PHYLIB
-       depends on OF
-       select OF_MDIO
+       depends on ACPI || OF
+       select FWNODE_MDIO
        help

Best regards,
Marcin

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

On Wed, Jun 16, 2021 at 09:07:54PM +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.

quoted

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.
+      */

11 more quoted lines

This kind of fallback is done in of_mdiobus_register and acpi_mdiobus_register.

Actually mvmdio driver is using this fallback for non-dt platforms
(e.g. Orion). Therefore I would prefer to keep the current behavior.

Best regards,
Marcin

On Thu, Jun 17, 2021 at 2:25 AM Marcin Wojtas [off-list ref] wrote:

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

quoted

quoted

+     dev->clks[0].id = "core";
+     dev->clks[1].id = "mg";
+     dev->clks[2].id = "mg_core";
+     dev->clks[3].id = "axi";
+     ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT,
+                                      dev->clks);

Kirkwood:

                mdio: mdio-bus@72004 {
                        compatible = "marvell,orion-mdio";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0x72004 0x84>;
                        interrupts = <46>;
                        clocks = <&gate_clk 0>;
                        status = "disabled";

Does this work? There is no clock-names in DT.

Neither are the clocks in Armada 7k8k / CN913x:

                CP11X_LABEL(mdio): mdio@12a200 {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        compatible = "marvell,orion-mdio";
                        reg = <0x12a200 0x10>;
                        clocks = <&CP11X_LABEL(clk) 1 9>,
<&CP11X_LABEL(clk) 1 5>,
                                 <&CP11X_LABEL(clk) 1 6>,
<&CP11X_LABEL(clk) 1 18>;
                        status = "disabled";
                };

Apparently I misread the code and got convinced that contrary to
devm_clk_get_optional(), the devm_clk_get_bulk_optional() obtains the

2 more quoted lines

Me too. Sorry for the wrong suggestion. I think we need something that
actually gets clocks by indices in a bulk, but this is another story.

I will drop this patch. Thank you for spotting the issue.

I'm fine with this.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Jun 17, 2021 at 01:39:40AM +0200, Marcin Wojtas wrote:

You should not need depends on ACPI || OF. FWNODE_MDIO implies
that. And there are no direct calls to of_ functions, so you can drop
the depends on OF.

    Andrew

Actually mvmdio driver is using this fallback for non-dt platforms
(e.g. Orion). Therefore I would prefer to keep the current behavior.

A quick look at Orion5x, it is now a multi arch MACH. It selects
ARCH_MULTI_V5. Which seems to imply ARCH_MULTIPLATFORM which selects
USE_OF which selects OF.

At least for ARM, i'm not sure you can realistically disable OF.

Having said that acpi_mdiobus_register() also falls back to
mdiobus_register(mdio). So it is symmetric. And
fwmode_mdiobus_register() falling back would keep with the
symmetry. So, O.K.

	  Andrew

czw., 17 cze 2021 o 14:28 Andrew Lunn [off-list ref] napisał(a):

On Thu, Jun 17, 2021 at 01:39:40AM +0200, Marcin Wojtas wrote:

quoted

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

quoted

On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:

quoted

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

You need to add depends on FWNODE_MDIO

Do you mean something like this?

--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig

@@ -68,8 +68,8 @@ config FSL_PQ_MDIO
 config FSL_XGMAC_MDIO
        tristate "Freescale XGMAC MDIO"
        select PHYLIB
-       depends on OF
-       select OF_MDIO
+       depends on ACPI || OF
+       select FWNODE_MDIO
        help

You should not need depends on ACPI || OF. FWNODE_MDIO implies
that. And there are no direct calls to of_ functions, so you can drop
the depends on OF.

Ok, I'll leave:
depends on FWNODE_MDIO
only.

Thanks,
Marcin