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
