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

[Rajan Vaja]

Hi Stephen,

Thanks for the reply. 

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> Sent: 02 June 2018 12:11 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
> 
> Quoting Rajan Vaja (2018-05-03 02:18:30)
> > Hi Stephen,
> >
> > > -----Original Message-----
> > > From: Rajan Vaja
> > > Sent: 16 March 2018 05:20 PM
> > > To: 'Stephen Boyd' <sboyd@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
> > >
> > > Hi Stephen,
> > >
> > > Thanks for the comment.
> > >
> > > > -----Original Message-----
> > > > From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> > > > Sent: 16 March 2018 12:17 AM
> > > > 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
> > > >
> > > > Quoting Rajan Vaja (2018-03-09 11:27:40)
> > > > > > From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> > > > > >
> > > > > > Is the intent to register the clk twice? I believe things are working as
> > > > > > intended without this patch, so maybe you can explain a little more
> what
> > > > > > you're trying to fix.
> > > > > [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> > > > > parent which is neither mentioned in output-clock-names of clock
> > > > > controller nor registered as clock provider, of_clk_init() will try to
> > > > > forcefully register in second loop.
> > > > >
> > > > >                         if (force || parent_ready(clk_provider->np)) {
> > > > >
> > > > >                                 /* Don't populate platform devices */
> > > > >                                 of_node_set_flag(clk_provider->np,
> > > > >                                                  OF_POPULATED);
> > > > >
> > > > > So registration of this DT fixed-factor clock would fail as parent
> > > > > would be NULL as below (called from _of_fixed_factor_clk_setup()):
> > > > > parent_name = of_clk_get_parent_name(node, 0);
> > > > >
> > > > > 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