On Fri, Oct 25, 2024 at 08:56:34AM +0200, Nuno Sá wrote: > On Thu, 2024-10-24 at 17:13 +0100, Conor Dooley wrote: > > On Thu, Oct 24, 2024 at 02:35:37PM +0200, Nuno Sá wrote: > > > On Wed, 2024-10-23 at 17:30 +0100, Conor Dooley wrote: > > > > On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote: > > > > > In order to access the registers of the HW, we need to make sure that > > > > > the AXI bus clock is enabled. Hence let's increase the number of clocks > > > > > by one. > > > > > > > > > > In order to keep backward compatibility, the new axi clock must be the > > > > > last phandle in the array. To make the intent clear, a non mandatory > > > > > clock-names property is also being added. > > > > > > > > Hmm, I'm not sure. I think clock-names actually may need to be mandatory > > > > here, as otherwise you'll not what the second clock is. The driver would > > > > have to interpret no clock-names meaning clock 2 was clkin2. > > > > > > > > > > > > > > So the way things are now is that we just get the parents count with > > > of_clk_get_parent_count() and then get the names with of_clk_get_parent_name() > > > and > > > this is given into 'struct clk_init_data'. So they are effectively clk_parents of > > > the > > > clock we're registering and as you can see clock-names does not really matter. > > > What > > > I'm trying to do is to keep this and still allow to get the AXI bus clock which > > > is > > > something we should get and enable and not rely on others to do it. The idea is > > > then > > > to add the axi bus clock as the last one in the clocks property and I will get it > > > by > > > index with of_clk_get(). The rest pretty much remains the same and we just need > > > to > > > decrement by one the number of parent clocks as the axi clock is not really a > > > parent > > > of our output clock. > > > > I mean, if it works, and you can always disambiguate between whether or > > not someone has two clkins or one clkin and the axi clock, then > > Acked-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > > The assumption is that the axi clock is the last one in the phandle array. But your > comment made me think a bit more about this and I do see a possible problem if we run > old DTs against a kernel with this patch. We have two possibilities: > > 1) DT only with one parent clock; > 2) DT with two parent clocks; > > 1) is "fine" as it would now fail to probe. 2) is more problematic as we would assume > the second parent to be the axi_bus clock so effectively not fixing anything and > silently probing with a broken setup. > > So yeah, I think I overthinked the backward compatibility thing. I mean, in theory, > all old DTs are not correct and should be fixed by including the axi_clk. And if we > now enforce clock-names we at least get probe errors right away making it clear > (which is far better from silently breaking after probe). > > Given the above, it should be fine to just enforce clock-names now, right? I think you need to enforce clock-names in the binding and take !clock-names and 2 clocks to mean that the second one is a clkin. I think that's a better solution than failing to probe for all extant devicestrees.
Attachment:
signature.asc
Description: PGP signature
