Re: [PATCH] clk: fixed: handle failed clk setup

[Emil Abildgaard Svendsen <EMAS@bang-olufsen.dk>]

On Wed, Oct 25, 2023 at 02:57:50PM -0700, Stephen Boyd wrote:
> Quoting Emil Abildgaard Svendsen (2023-10-25 02:34:49)
> > On Tue, Oct 24, 2023 at 11:41:05AM -0700, Stephen Boyd wrote:
> > > Quoting Emil Abildgaard Svendsen (2023-10-24 07:54:28)
> > > > On Mon, Oct 23, 2023 at 08:14:01PM -0700, Stephen Boyd wrote:
> > > > > Quoting Emil Abildgaard Svendsen (2023-04-14 05:52:16)
> > > > > > When initializing clock providers "of_clk_init" will try and init
> > > > > > parents first. But if parent clock is provided by a platform driver it
> > > > > > can't. Then clocks will be forced on and OF_POPULATED flag will be set
> > > > > > blindly. So if setup failes e.g. with -EPROBE_DEFER the clock will not
> > > > > > be probed later on.
> > > > > > 
> > > > > > This patch will clear the OF_POPULATED falg if clock setup failes.
> > > > > 
> > > > > Are you actually running into a problem with of_clk_init() failing to
> > > > > register a fixed rate of fixed factor clk from DT?
> 
> Sorry, this was "fixed rate or fixed factor"

Fixed factor

> > > 
> > > Can you answer this question?
> > 
> > I wouldn't call it a problem with of_clk_init() because how does it know
> > when fixed-factor-clk fails to register a clock when the setup function
> > returns void? 
> 
> Let's not debate the semantics of the question. Does the clk get
> registered with the framework during of_clk_init()?

No the clk doesn't get registered by of_clk_init().

> > 
> > > > > I don't understand
> > > > > why that is happening. I agree it's good hygiene to clear the flag, but
> > > > > I don't see why this is failing.
> > > > 
> > > > The problem is of_clk_init() will not wait for clock providers
> > > > registered by a platform driver that returns -EPROBE_DEFER. Which can
> > > > result in final probe to be delayed quite a bit. If this happens and
> > > > CLK_OF_DECLARE tries to setup a fixed-factor-clock with a deferred
> > > > clock parent, it will fail.
> > > 
> > > Doesn't the 'force' local variable get set to true in this case, and
> > > then the fixed factor clk is registered anyway? The clk framework tries
> > > hard to let clks be registered in any order so that parents don't need
> > > to be registered before children. From what I remember, we want to wait
> > > for parents because of_clk_set_defaults() may fail otherwise.
> > 
> > Yes, the 'force' local variable get set. But it's the setup function of
> > fixed-factor-clk that fails to register the clock.
> 
> Ok. What part of __clk_hw_register_fixed_factor() fails? Or is it the
> caller, _of_fixed_factor_clk_setup()?

It's the caller _of_fixed_factor_clk_setup() in of_clk_add_hw_provider()
after __clk_hw_register_fixed_factor().