| Message ID | 20210613183520.2247415-3-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 |
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?
> +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) <linux@armlinux.org.uk> 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 <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")); > > 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 <andrew@lunn.ch> 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 <andrew@lunn.ch> 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
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; }
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 | 21 ++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)