| Message ID | 20210616190759.2832033-5-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, 99 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> + dev->clks[0].id = "core"; > + dev->clks[1].id = "mg"; > + dev->clks[2].id = "mg_core"; > + dev->clks[3].id = "axi"; > + ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT, > + dev->clks); Kirkwood: mdio: mdio-bus@72004 { compatible = "marvell,orion-mdio"; #address-cells = <1>; #size-cells = <0>; reg = <0x72004 0x84>; interrupts = <46>; clocks = <&gate_clk 0>; status = "disabled"; Does this work? There is no clock-names in DT. Andrew
śr., 16 cze 2021 o 21:48 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > + dev->clks[0].id = "core"; > > + dev->clks[1].id = "mg"; > > + dev->clks[2].id = "mg_core"; > > + dev->clks[3].id = "axi"; > > + ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT, > > + dev->clks); > > Kirkwood: > > mdio: mdio-bus@72004 { > compatible = "marvell,orion-mdio"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0x72004 0x84>; > interrupts = <46>; > clocks = <&gate_clk 0>; > status = "disabled"; > > Does this work? There is no clock-names in DT. > Neither are the clocks in Armada 7k8k / CN913x: CP11X_LABEL(mdio): mdio@12a200 { #address-cells = <1>; #size-cells = <0>; compatible = "marvell,orion-mdio"; reg = <0x12a200 0x10>; clocks = <&CP11X_LABEL(clk) 1 9>, <&CP11X_LABEL(clk) 1 5>, <&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>; status = "disabled"; }; Apparently I misread the code and got convinced that contrary to devm_clk_get_optional(), the devm_clk_get_bulk_optional() obtains the clocks directly by index, not name (on the tested boards, the same clocks are enabled by the other interfaces, so the problems I will drop this patch. Thank you for spotting the issue. Best regards, Marcin
On Thu, Jun 17, 2021 at 2:25 AM Marcin Wojtas <mw@semihalf.com> wrote: > > śr., 16 cze 2021 o 21:48 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > > + dev->clks[0].id = "core"; > > > + dev->clks[1].id = "mg"; > > > + dev->clks[2].id = "mg_core"; > > > + dev->clks[3].id = "axi"; > > > + ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT, > > > + dev->clks); > > > > Kirkwood: > > > > mdio: mdio-bus@72004 { > > compatible = "marvell,orion-mdio"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0x72004 0x84>; > > interrupts = <46>; > > clocks = <&gate_clk 0>; > > status = "disabled"; > > > > Does this work? There is no clock-names in DT. > > > > Neither are the clocks in Armada 7k8k / CN913x: > > CP11X_LABEL(mdio): mdio@12a200 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "marvell,orion-mdio"; > reg = <0x12a200 0x10>; > clocks = <&CP11X_LABEL(clk) 1 9>, > <&CP11X_LABEL(clk) 1 5>, > <&CP11X_LABEL(clk) 1 6>, > <&CP11X_LABEL(clk) 1 18>; > status = "disabled"; > }; > > Apparently I misread the code and got convinced that contrary to > devm_clk_get_optional(), the devm_clk_get_bulk_optional() obtains the > clocks directly by index, not name (on the tested boards, the same > clocks are enabled by the other interfaces, so the problems Me too. Sorry for the wrong suggestion. I think we need something that actually gets clocks by indices in a bulk, but this is another story. > I will drop this patch. Thank you for spotting the issue. I'm fine with this.
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c index d14762d93640..ce3ddc867898 100644 --- a/drivers/net/ethernet/marvell/mvmdio.c +++ b/drivers/net/ethernet/marvell/mvmdio.c @@ -62,9 +62,11 @@ #define MVMDIO_XSMI_POLL_INTERVAL_MIN 150 #define MVMDIO_XSMI_POLL_INTERVAL_MAX 160 +#define MVMDIO_CLOCK_COUNT 4 + struct orion_mdio_dev { void __iomem *regs; - struct clk *clk[4]; + struct clk_bulk_data clks[MVMDIO_CLOCK_COUNT]; /* * If we have access to the error interrupt pin (which is * somewhat misnamed as it not only reflects internal errors @@ -279,7 +281,7 @@ static int orion_mdio_probe(struct platform_device *pdev) struct resource *r; struct mii_bus *bus; struct orion_mdio_dev *dev; - int i, ret; + int ret; type = (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev); @@ -319,33 +321,20 @@ static int orion_mdio_probe(struct platform_device *pdev) init_waitqueue_head(&dev->smi_busy_wait); - if (pdev->dev.of_node) { - for (i = 0; i < ARRAY_SIZE(dev->clk); i++) { - dev->clk[i] = of_clk_get(pdev->dev.of_node, i); - if (PTR_ERR(dev->clk[i]) == -EPROBE_DEFER) { - ret = -EPROBE_DEFER; - goto out_clk; - } - if (IS_ERR(dev->clk[i])) - break; - clk_prepare_enable(dev->clk[i]); - } - - if (!IS_ERR(of_clk_get(pdev->dev.of_node, - ARRAY_SIZE(dev->clk)))) - dev_warn(&pdev->dev, - "unsupported number of clocks, limiting to the first " - __stringify(ARRAY_SIZE(dev->clk)) "\n"); - } else { - dev->clk[0] = clk_get(&pdev->dev, NULL); - if (PTR_ERR(dev->clk[0]) == -EPROBE_DEFER) { - ret = -EPROBE_DEFER; - goto out_clk; - } - if (!IS_ERR(dev->clk[0])) - clk_prepare_enable(dev->clk[0]); - } + dev->clks[0].id = "core"; + dev->clks[1].id = "mg"; + dev->clks[2].id = "mg_core"; + dev->clks[3].id = "axi"; + ret = devm_clk_bulk_get_optional(&pdev->dev, MVMDIO_CLOCK_COUNT, + dev->clks); + if (ret) + return ret; + ret = clk_bulk_prepare_enable(MVMDIO_CLOCK_COUNT, dev->clks); + if (ret) { + dev_err(&pdev->dev, "Cannot enable clocks\n"); + return ret; + } dev->err_interrupt = platform_get_irq_optional(pdev, 0); if (dev->err_interrupt > 0 && @@ -383,14 +372,6 @@ static int orion_mdio_probe(struct platform_device *pdev) if (dev->err_interrupt > 0) writel(0, dev->regs + MVMDIO_ERR_INT_MASK); -out_clk: - for (i = 0; i < ARRAY_SIZE(dev->clk); i++) { - if (IS_ERR(dev->clk[i])) - break; - clk_disable_unprepare(dev->clk[i]); - clk_put(dev->clk[i]); - } - return ret; } @@ -398,18 +379,12 @@ static int orion_mdio_remove(struct platform_device *pdev) { struct mii_bus *bus = platform_get_drvdata(pdev); struct orion_mdio_dev *dev = bus->priv; - int i; if (dev->err_interrupt > 0) writel(0, dev->regs + MVMDIO_ERR_INT_MASK); mdiobus_unregister(bus); - for (i = 0; i < ARRAY_SIZE(dev->clk); i++) { - if (IS_ERR(dev->clk[i])) - break; - clk_disable_unprepare(dev->clk[i]); - clk_put(dev->clk[i]); - } + clk_bulk_disable_unprepare(MVMDIO_CLOCK_COUNT, dev->clks); return 0; }
Because the mvmdio driver supports a wide range of Marvell SoC families it must support multiple types of the HW description and required resources. Thanks to the devm_clk_bulk_get_optional() helper routine the clock handling could be significantly simplified, as it is compatible with all possible variants within a single call. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- drivers/net/ethernet/marvell/mvmdio.c | 61 ++++++-------------- 1 file changed, 18 insertions(+), 43 deletions(-)