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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]