| Message ID | 20210613183520.2247415-2-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 4 of 4 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: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 75 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> - 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
> @@ -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; 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. > - 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; > + 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
Hi, niedz., 13 cze 2021 o 22:08 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a): > > > > On Sunday, June 13, 2021, Andrew Lunn <andrew@lunn.ch> wrote: >> >> > @@ -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; >> >> 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. > Indeed, I'll double check if it works and will keep the if {} else {} intact. > > > The better approach is to switch to devm_get_clk_optional() as I have done in several drivers, IIRC recently in mvpp2 > > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=cf3399b731d36bc780803ff63e4d480a1efa33ac > Yes, this would be a nice improvement, however the devm_get_clk_optional requires clock name (type char *) - mvmdio uses raw indexes, so this helper unfortunately seems to be not applicable. >> >> > - 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; >> > + >> >> 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. Clock routines silently accept NULL pointers, so it will be safe to drop this addition in v2. Best regards, Marcin
Hi, niedz., 13 cze 2021 o 21:20 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > - 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:09 PM Marcin Wojtas <mw@semihalf.com> wrote: > niedz., 13 cze 2021 o 22:08 Andy Shevchenko > <andy.shevchenko@gmail.com> napisał(a): > > On Sunday, June 13, 2021, Andrew Lunn <andrew@lunn.ch> wrote: > > The better approach is to switch to devm_get_clk_optional() as I have done in several drivers, IIRC recently in mvpp2 > > > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=cf3399b731d36bc780803ff63e4d480a1efa33ac > > Yes, this would be a nice improvement, however the > devm_get_clk_optional requires clock name (type char *) - mvmdio uses > raw indexes, so this helper unfortunately seems to be not applicable. As far as I can read the code it smells like devm_clk_bulk_get_optional(). Am I mistaken?
On Tue, Jun 15, 2021 at 6:14 PM Marcin Wojtas <mw@semihalf.com> wrote: > Hi, > niedz., 13 cze 2021 o 21:20 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > > - 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)
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), }, };
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 <mw@semihalf.com> --- drivers/net/ethernet/marvell/mvmdio.c | 27 +++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)