Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2024-10-25 at 17:47 +0100, Conor Dooley wrote:
> 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@microchip.com>
> > 
> > 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.

Ok, so IIUC, you mean leaving old DTs as of today and relying on someone else to
enable the axi clock (if it was not enabled they would have noticed by now). And only
take care of the bus clock when clock-names is provided?

- Nuno Sá