| Message ID | 20210621173028.3541424-6-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ACPI MDIO support for Marvell controllers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6 this patch: 6 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6 this patch: 6 |
| netdev/header_inline | success | Link |
> +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, śr., 23 cze 2021 o 22:37 Andrew Lunn <andrew@lunn.ch> 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")); > > 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
On Wed, Jun 23, 2021 at 11:45:04PM +0200, Marcin Wojtas wrote: > Hi, > > śr., 23 cze 2021 o 22:37 Andrew Lunn <andrew@lunn.ch> 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")); > > > > 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
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; }
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 <mw@semihalf.com> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 22 +++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)