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 <[email protected]>
---
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
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!
> +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)
<[email protected]> napisał(a):
>
> 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?
>
It does. I'll update in v2.
Thanks,
Marcin
Hi,
niedz., 13 cze 2021 o 21:47 Andrew Lunn <[email protected]> napisał(a):
>
> > +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 <[email protected]> napisał(a):
>
> > 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 <[email protected]> napisał(a):
> >
> > > 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.
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