RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> Sent: 09 July 2018 12:56 PM
> To: Rajan Vaja <RAJANV@xxxxxxxxxx>
> Cc: linux-clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jolly Shah
> <JOLLYS@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>;
> mturquette@xxxxxxxxxxxx
> Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> EXTERNAL EMAIL
> 
> Quoting Rajan Vaja (2018-06-03 20:41:27)
> > > > > > > On the other hand, even if registration failed, that node will be
> > > > > > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also
> not
> > > > > > > be called and that DT fixed-factor clock would never be registered.
> > > > > > >
> > > > > > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> > > > > >
> > > > > > Ok. I believe the answer is to fix the DT to describe the parent chain
> > > > > > properly with clock-output-names. Otherwise, we have no good way
> of
> > > > > > figuring out what the name should be.
> > > > > [Rajan] clock DT binding doc says that clock-output-names property
> > > > > is optional and sometimes not recommended.
> > > > > I think this patch fixes the issue we have which mandates to add clock-
> > > output-
> > > > > names
> > > > > property (for this particular case). Also, IIUC platform driver probe in clk-
> > > fixed-
> > > > > factor.c
> > > > > will never be called unless we use CLK_OF_DECLARE_DRIVER.
> > > > > I completely agree that proper solution would be to stop using strings to
> > > > > describe clock topology.
> > > > [Rajan] Any comments on this?
> > > > Unless we have proper solution ready, we need to have some mechanism
> to
> > > handle this scenario.
> > > > clock-output-names is optional and without this, it mandates to use clock-
> > > output-names.
> > > >
> > >
> > > I couldn't read your outlook sent email and I was too busy to go
> > > translate it. Some bug in my MUA it seems.
> > >
> > > Can you add the output-names property? In this case it's practically
> > > mandatory, so if you can't do it I'd like to hear why not.
> > [Rajan] In our case, we are firmware maintains clock database and driver
> query clocks
> > from firmware and registers clock based on information received from
> firmware. So
> > output clock names are not fixed.
> > https://patchwork.kernel.org/patch/10439893/ - dt-bindings: clock: Add
> bindings for ZynqMP clock driver
> > https://patchwork.kernel.org/patch/10439891/ - drivers: clk: Add ZynqMP
> clock driver
> 
> output-names are fixed by the time firmware is made. Is the DT also
> fixed by the time firmware is made? Why can't the DT be made to match
> what the firmware is describing by having the firmware update the DT to
> describe it's output names?
[Rajan] The idea is to have a generic driver which queries clock for the variant from
Firmware. So if some clock changes, driver remains same.
> 
> And what is this fixed factor clk in DT for?
> 
> I'm beginning to think that maybe we should make the fixed factor setup
> a little worse by having it unmark itself as OF_POPULATED if it finds
> that the DT node has a clocks property that it can't find a name for.
> Hopefully we can get by with it registering later on because it isn't
> critical for the system to bootstrap interrupts and timers.
[Rajan] I have submitted separate change for this https://patchwork.kernel.org/patch/10529387/. 

> 
> I take it you understand that changing the code to be
> CLK_OF_DECLARE_DRIVER will be actively bad and cause the clk to be
> registered potentially twice so we really need to fix the real issue
> which is that the parent name can't be figured out until later on, so
> the CLK_OF_DECLARE path needs to fail when it can't find the parent it
> needs and then manually mark itself as not populated in that case so
> that it probes later on with the platform device driver. Unmarking the
> node can actually be used to flag a failure in the init function so that
> we don't keep trying to do things with the node until driver time too.
[Rajan] I understood that making it CLK_OF_DECLARE_DRIVER will try to register clock twice.