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

Hi,

The MDIO ACPI binding has been established and merged to the
Linux tree, hence it is now possible to use it on the platforms
that base on the Marvell SoCs.

This short patchset adds ACPI support for the mvmdio controller.
mvpp2 driver is also updated in order to use the phylink in
ACPI world. For the latter a backward compatibility is ensured
- in case an older firmware is used, the driver would fall back to the
hitherto link interrupt handling.

The feature was verified with ACPI on MacchiatoBin and CN913x-DB.
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


Marcin Wojtas (3):
  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 ---
 drivers/net/ethernet/marvell/mvmdio.c           | 27 +++++++++++++++++---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 22 ++++++++++++----
 3 files changed, 41 insertions(+), 11 deletions(-)

-- 
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 | 21 ++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 9bca8c8f9f8d..ca1f0464e746 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,

@@ -6922,7 +6934,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 	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 +6946,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

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 | 27 +++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

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

@@ -17,6 +17,8 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/interrupt.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) {

@@ -336,7 +338,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
 			dev_warn(&pdev->dev,
 				 "unsupported number of clocks, limiting to the first "
 				 __stringify(ARRAY_SIZE(dev->clk)) "\n");
-	} else {
+	} else if (!has_acpi_companion(&pdev->dev)) {
 		dev->clk[0] = clk_get(&pdev->dev, NULL);
 		if (PTR_ERR(dev->clk[0]) == -EPROBE_DEFER) {
 			ret = -EPROBE_DEFER;

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

@@ -383,6 +390,9 @@ static int orion_mdio_probe(struct platform_device *pdev)
 	if (dev->err_interrupt > 0)
 		writel(0, dev->regs + MVMDIO_ERR_INT_MASK);
 
+	if (has_acpi_companion(&pdev->dev))
+		return ret;
+
 out_clk:
 	for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
 		if (IS_ERR(dev->clk[i]))

@@ -404,6 +414,9 @@ static int orion_mdio_remove(struct platform_device *pdev)
 		writel(0, dev->regs + MVMDIO_ERR_INT_MASK);
 	mdiobus_unregister(bus);
 
+	if (has_acpi_companion(&pdev->dev))
+		return 0;
+
 	for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
 		if (IS_ERR(dev->clk[i]))
 			break;

@@ -421,12 +434,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

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 ca1f0464e746..c87752999485 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 Sun, Jun 13, 2021 at 08:35:19PM +0200, Marcin Wojtas wrote:

 
 	/* Phylink isn't used w/ ACPI as of now */
-	if (port_node) {
+	if (!mvpp2_use_acpi_compat_mode(port_fwnode)) {

Does this comment need to be updated?

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

-	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	if (pdev->dev.of_node)
+		ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	else if (is_acpi_node(pdev->dev.fwnode))
+		ret = acpi_mdiobus_register(bus, pdev->dev.fwnode);
+	else
+		ret = -EINVAL;

This seems like something which could be put into fwnode_mdio.c.

     Andrew

Is this needed? As you said, there are no clocks when ACPI is used, So
doesn't clk_get() return -ENODEV? Since this is not EPRODE_DEFER, it
keeps going. The clk_prepare_enable() won't be called.

I think this can also be removed for the same reason.

We should try to avoid adding has_acpi_companion() and
!pdev->dev.of_node whenever we can. It makes the driver code too much
of a maze.

   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"));

fixed-link and managed are not documented in
Documentation/firmware-guide/acpi/dsd/phy.rst.

Also, should you be looking for phy-mode?

      Andrew

Hi,


niedz., 13 cze 2021 o 20:44 Russell King (Oracle)
[off-list ref] napisał(a):

On Sun, Jun 13, 2021 at 08:35:19PM +0200, Marcin Wojtas wrote:

quoted

      /* Phylink isn't used w/ ACPI as of now */
-     if (port_node) {
+     if (!mvpp2_use_acpi_compat_mode(port_fwnode)) {

Does this comment need to be updated?

It does. I'll update in v2.

Thanks,
Marcin

Hi,

niedz., 13 cze 2021 o 21:47 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"));

fixed-link and managed are not documented in
Documentation/firmware-guide/acpi/dsd/phy.rst.

True. I picked the port type properties that are interpreted by
phylink. Basically, I think that everything that's described in:
devicetree/bindings/net/ethernet-controller.yaml
is valid for the ACPI as well - the kernel already is using 'fwnode_'
in most (if not all) cases.

Would you like me to add "managed" and "fixed-link"
description/examples to the mentioned file?

Also, should you be looking for phy-mode?

In the beginning of the mvpp2_port_probe, there's:

        phy_mode = fwnode_get_phy_mode(port_fwnode);
        if (phy_mode < 0) {
                dev_err(&pdev->dev, "incorrect phy mode\n");
                err = phy_mode;
                goto err_free_netdev;
        }

So we won't reach further checks in case anything is wrong with it.

Best regards,
Marcin

True. I picked the port type properties that are interpreted by
phylink. Basically, I think that everything that's described in:
devicetree/bindings/net/ethernet-controller.yaml
is valid for the ACPI as well

So you are saying ACPI is just DT stuff into tables? Then why bother
with ACPI? Just use DT.

Right, O.K. Please document anything which phylink already supports:

hylink.c:		ret = fwnode_property_read_u32(fixed_node, "speed", &speed);
phylink.c:		if (fwnode_property_read_bool(fixed_node, "full-duplex"))
phylink.c:		if (fwnode_property_read_bool(fixed_node, "pause"))
phylink.c:		if (fwnode_property_read_bool(fixed_node, "asym-pause"))
phylink.c:		ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:		ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:	if (dn || fwnode_property_present(fwnode, "fixed-link"))
phylink.c:	if ((fwnode_property_read_string(fwnode, "managed", &managed) == 0 &&

If you are adding new properties, please do that In a separate patch,
which needs an ACPI maintainer to ACK it before it gets merged.

	 Andrew

<Adding ACPI Maintainers>

Hi Andrew,

niedz., 13 cze 2021 o 23:35 Andrew Lunn [off-list ref] napisał(a):

quoted

True. I picked the port type properties that are interpreted by
phylink. Basically, I think that everything that's described in:
devicetree/bindings/net/ethernet-controller.yaml
is valid for the ACPI as well

So you are saying ACPI is just DT stuff into tables? Then why bother
with ACPI? Just use DT.

Any user is free to use whatever they like, however apparently there
must have been valid reasons, why ARM is choosing ACPI as the
preferred way of describing the hardware over DT. In such
circumstances, we all work to improve adoption and its usability for
existing devices.

Regarding the properties in _DSD package, please refer to
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/DSD-properties-rules.html,
especially to two fragments:
"The _DSD (Device Specific Data) configuration object, introduced in
ACPI 5.1, allows any type of device configuration data to be provided
via the ACPI namespace. In principle, the format of the data may be
arbitrary [...]"
"It often is useful to make _DSD return property sets that follow
Device Tree bindings."
Therefore what I understand is that (within some constraints) simple
reusing existing sets of nodes' properties, should not violate ACPI
spec. In this patchset no new extension/interfaces/method is
introduced.

Right, O.K. Please document anything which phylink already supports:

hylink.c:               ret = fwnode_property_read_u32(fixed_node, "speed", &speed);
phylink.c:              if (fwnode_property_read_bool(fixed_node, "full-duplex"))
phylink.c:              if (fwnode_property_read_bool(fixed_node, "pause"))
phylink.c:              if (fwnode_property_read_bool(fixed_node, "asym-pause"))
phylink.c:              ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:              ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:      if (dn || fwnode_property_present(fwnode, "fixed-link"))
phylink.c:      if ((fwnode_property_read_string(fwnode, "managed", &managed) == 0 &&

If you are adding new properties, please do that In a separate patch,
which needs an ACPI maintainer to ACK it before it gets merged.

Ok, I can extend the documentation.

Best regards,
Marcin

On Mon, Jun 14, 2021 at 01:46:06AM +0200, Marcin Wojtas wrote:

<Adding ACPI Maintainers>

Hi Andrew,

niedz., 13 cze 2021 o 23:35 Andrew Lunn [off-list ref] napisał(a):

quoted

quoted

True. I picked the port type properties that are interpreted by
phylink. Basically, I think that everything that's described in:
devicetree/bindings/net/ethernet-controller.yaml
is valid for the ACPI as well

So you are saying ACPI is just DT stuff into tables? Then why bother
with ACPI? Just use DT.

Any user is free to use whatever they like, however apparently there
must have been valid reasons, why ARM is choosing ACPI as the
preferred way of describing the hardware over DT. In such
circumstances, we all work to improve adoption and its usability for
existing devices.

Regarding the properties in _DSD package, please refer to
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/DSD-properties-rules.html,
especially to two fragments:
"The _DSD (Device Specific Data) configuration object, introduced in
ACPI 5.1, allows any type of device configuration data to be provided
via the ACPI namespace. In principle, the format of the data may be
arbitrary [...]"
"It often is useful to make _DSD return property sets that follow
Device Tree bindings."
Therefore what I understand is that (within some constraints) simple
reusing existing sets of nodes' properties, should not violate ACPI
spec. In this patchset no new extension/interfaces/method is
introduced.

quoted

Right, O.K. Please document anything which phylink already supports:

hylink.c:               ret = fwnode_property_read_u32(fixed_node, "speed", &speed);
phylink.c:              if (fwnode_property_read_bool(fixed_node, "full-duplex"))
phylink.c:              if (fwnode_property_read_bool(fixed_node, "pause"))
phylink.c:              if (fwnode_property_read_bool(fixed_node, "asym-pause"))
phylink.c:              ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:              ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:      if (dn || fwnode_property_present(fwnode, "fixed-link"))
phylink.c:      if ((fwnode_property_read_string(fwnode, "managed", &managed) == 0 &&

If you are adding new properties, please do that In a separate patch,
which needs an ACPI maintainer to ACK it before it gets merged.

Ok, I can extend the documentation.

My real fear is snowflakes. Each ACPI implementation is unique. That
is going to be a maintenance nightmare, and it will make it very hard
to change the APIs between phylib/phylink and MAC drivers. To avoid
that, we need to push are much as possible into the core, document as
much as possible, and NACK anything does looks like a snowflake.

I actually like what you pointed out above. It makes it possible to
say, ACPI for phylink/phylib needs to follow device tree, 1 to 1.
It also means we should be able to remove a lot of the

if (is_of()) {}
else if (is_acpi() {}
else
	return -EINVAL;

in drivers, and put it into the core.

   Andrew

Hi,

niedz., 13 cze 2021 o 21:20 Andrew Lunn [off-list ref] napisał(a):

quoted

-     ret = of_mdiobus_register(bus, pdev->dev.of_node);
+     if (pdev->dev.of_node)
+             ret = of_mdiobus_register(bus, pdev->dev.of_node);
+     else if (is_acpi_node(pdev->dev.fwnode))
+             ret = acpi_mdiobus_register(bus, pdev->dev.fwnode);
+     else
+             ret = -EINVAL;

This seems like something which could be put into fwnode_mdio.c.

Agree - I'll create a simple fwnode_mdiobus_register() helper there.

Best regards,
Marcin

On Tue, Jun 15, 2021 at 6:14 PM Marcin Wojtas [off-list ref] wrote:

Hi,
niedz., 13 cze 2021 o 21:20 Andrew Lunn [off-list ref] napisał(a):

quoted

quoted

-     ret = of_mdiobus_register(bus, pdev->dev.of_node);
+     if (pdev->dev.of_node)
+             ret = of_mdiobus_register(bus, pdev->dev.of_node);
+     else if (is_acpi_node(pdev->dev.fwnode))
+             ret = acpi_mdiobus_register(bus, pdev->dev.fwnode);
+     else
+             ret = -EINVAL;

This seems like something which could be put into fwnode_mdio.c.

Agree - I'll create a simple fwnode_mdiobus_register() helper there.

Please, also convert the users that we will not have again some
open-coded examples here and there
https://lore.kernel.org/netdev/162344280835.13501.16334655818490594799.git-patchwork-notify@kernel.org/T/#mff706861dea5d3be037d1546fa9c362b27d5839b

(Btw, note the is_of_node() usage there, so should
fwnode_mdiobus_register() have)

-- 
With Best Regards,
Andy Shevchenko