lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <D3Z1U6IHATAI.2Z2L37F10HOSE@protonmail.com> Date: Fri, 06 Sep 2024 08:13:35 +0000 From: Harry Austen <hpausten@...tonmail.com> To: Stephen Boyd <sboyd@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>, Michal Simek <michal.simek@....com>, Rob Herring <robh@...nel.org> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@....com>, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org Subject: Re: [PATCH 6/6] clk: clocking-wizard: move dynamic reconfig setup behind flag 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
Powered by blists - more mailing lists