Hi Stephen, > -----Original Message----- > From: Stephen Boyd [mailto:sb...@kernel.org] > Sent: 09 July 2018 12:56 PM > To: Rajan Vaja <raj...@xilinx.com> > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah > <jol...@xilinx.com>; Michal Simek <mich...@xilinx.com>; > mturque...@baylibre.com > 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.