| Message ID | 20240831111056.3864-7-hpausten@protonmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | clk: clocking-wizard: modernize probe | expand |
Quoting Harry Austen (2024-08-31 04:13:26) > diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > index 1a65a7d153c35..967eacc28050d 100644 > --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > @@ -1146,20 +1146,6 @@ static int clk_wzrd_probe(struct platform_device *pdev) > if (IS_ERR(clk_wzrd->base)) > return PTR_ERR(clk_wzrd->base); > > - ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade); > - if (!ret) { > - if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) { > - dev_warn(&pdev->dev, "invalid speed grade '%d'\n", > - clk_wzrd->speed_grade); > - clk_wzrd->speed_grade = 0; > - } > - } > - > - clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1"); > - if (IS_ERR(clk_wzrd->clk_in1)) > - return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1), > - "clk_in1 not found\n"); > - > clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > if (IS_ERR(clk_wzrd->axi_clk)) > return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk), > @@ -1170,31 +1156,48 @@ static int clk_wzrd_probe(struct platform_device *pdev) > return -EINVAL; > } > > - ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs); > - if (ret) > - return ret; > - > - clk_wzrd->clk_data.num = nr_outputs; > - ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data); > - if (ret) { > - dev_err(&pdev->dev, "unable to register clock provider\n"); > - return ret; > - } > + if (of_property_read_bool(np, "xlnx,dynamic-reconfig")) { Is this going to break the existing DTBs? Before the property existed, the driver seems to have always tried to read xlnx,speed-grade and then setup a notifier, etc. Why would xlnx,speed-grade be set if xlnx,dynamic-reconfig wasn't set? The binding has implicitly had xlnx,dynamic-reconfig set before this patch, and we should strive to maintain that. Perhaps the property should be inverted, i.e. xlnx,static-config, and the absence of that property can imply the reconfig property.
On Thu Sep 5, 2024 at 8:06 PM BST, Stephen Boyd wrote: > Quoting Harry Austen (2024-08-31 04:13:26) > > diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > > index 1a65a7d153c35..967eacc28050d 100644 > > --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > > +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > > @@ -1146,20 +1146,6 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > if (IS_ERR(clk_wzrd->base)) > > return PTR_ERR(clk_wzrd->base); > > > > - ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade); > > - if (!ret) { > > - if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) { > > - dev_warn(&pdev->dev, "invalid speed grade '%d'\n", > > - clk_wzrd->speed_grade); > > - clk_wzrd->speed_grade = 0; > > - } > > - } > > - > > - clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1"); > > - if (IS_ERR(clk_wzrd->clk_in1)) > > - return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1), > > - "clk_in1 not found\n"); > > - > > clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > > if (IS_ERR(clk_wzrd->axi_clk)) > > return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk), > > @@ -1170,31 +1156,48 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs); > > - if (ret) > > - return ret; > > - > > - clk_wzrd->clk_data.num = nr_outputs; > > - ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data); > > - if (ret) { > > - dev_err(&pdev->dev, "unable to register clock provider\n"); > > - return ret; > > - } > > + if (of_property_read_bool(np, "xlnx,dynamic-reconfig")) { > > Is this going to break the existing DTBs? Before the property existed, > the driver seems to have always tried to read xlnx,speed-grade and then > setup a notifier, etc. Why would xlnx,speed-grade be set if > xlnx,dynamic-reconfig wasn't set? I did wonder about this. What is the kernel's policy on breaking DT ABI? Especially in this case where there are no such DTs in the kernel source tree (AMD/Xilinx have their own tools for devicetree generation, e.g. see the clocking wizard DT node generation TCL script on GitHub [1]). Agree it would be better to maintain compatibility with existing DTs if it makes sense to do so though. In terms of speed grade, as you say it currently only affects how you would want to interact with the dynamic reconfiguration registers. But that's not to say that it might be relevant to some other register added in future (since it is just a generic property of the chip). So using presence of the xlnx,speed-grade property to indicate dynamic reconfiguration enablement feels like potentially a bad idea as well. > > The binding has implicitly had xlnx,dynamic-reconfig set before this > patch, and we should strive to maintain that. Perhaps the property > should be inverted, i.e. xlnx,static-config, and the absence of that > property can imply the reconfig property. I don't mind this. A bit of a shame that it's then inverted to how the IP core is configured through Vivado, but that's not the end of the world. Unless anyone can think of a better idea, will probably go with this in v2 of this patchset. Thanks for the review! Harry [1]: https://github.com/Xilinx/device-tree-xlnx/blob/master/axi_clk_wiz/data/axi_clk_wiz.tcl
diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c index 1a65a7d153c35..967eacc28050d 100644 --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c @@ -1146,20 +1146,6 @@ static int clk_wzrd_probe(struct platform_device *pdev) if (IS_ERR(clk_wzrd->base)) return PTR_ERR(clk_wzrd->base); - ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade); - if (!ret) { - if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) { - dev_warn(&pdev->dev, "invalid speed grade '%d'\n", - clk_wzrd->speed_grade); - clk_wzrd->speed_grade = 0; - } - } - - clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1"); - if (IS_ERR(clk_wzrd->clk_in1)) - return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1), - "clk_in1 not found\n"); - clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); if (IS_ERR(clk_wzrd->axi_clk)) return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk), @@ -1170,31 +1156,48 @@ static int clk_wzrd_probe(struct platform_device *pdev) return -EINVAL; } - ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs); - if (ret) - return ret; - - clk_wzrd->clk_data.num = nr_outputs; - ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, &clk_wzrd->clk_data); - if (ret) { - dev_err(&pdev->dev, "unable to register clock provider\n"); - return ret; - } + if (of_property_read_bool(np, "xlnx,dynamic-reconfig")) { + ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade); + if (!ret) { + if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) { + dev_warn(&pdev->dev, "invalid speed grade '%d'\n", + clk_wzrd->speed_grade); + clk_wzrd->speed_grade = 0; + } + } - if (clk_wzrd->speed_grade) { - clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier; + clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1"); + if (IS_ERR(clk_wzrd->clk_in1)) + return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1), + "clk_in1 not found\n"); - ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1, - &clk_wzrd->nb); + ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs); if (ret) - dev_warn(&pdev->dev, - "unable to register clock notifier\n"); + return ret; + + clk_wzrd->clk_data.num = nr_outputs; + ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get, + &clk_wzrd->clk_data); + if (ret) { + dev_err(&pdev->dev, "unable to register clock provider\n"); + return ret; + } - ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk, - &clk_wzrd->nb); - if (ret) - dev_warn(&pdev->dev, - "unable to register clock notifier\n"); + if (clk_wzrd->speed_grade) { + clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier; + + ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1, + &clk_wzrd->nb); + if (ret) + dev_warn(&pdev->dev, + "unable to register clock notifier\n"); + + ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk, + &clk_wzrd->nb); + if (ret) + dev_warn(&pdev->dev, + "unable to register clock notifier\n"); + } } return 0;
Xilinx clocking wizard IP core's dynamic reconfiguration support is optionally enabled at build time. Use the new boolean devicetree property to indicate whether the hardware supports this feature or not. Signed-off-by: Harry Austen <hpausten@protonmail.com> --- drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 73 +++++++++++----------- 1 file changed, 38 insertions(+), 35 deletions(-)