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

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

On Thu, Oct 26, 2023 at 12:22:20PM -0700, Stephen Boyd wrote:
> Quoting Emil Abildgaard Svendsen (2023-10-26 02:40:04)
> > 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().
> 
> Thanks!
> 
> > 
> > > > 
> > > > > > > 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().
> 
> Ok, so is of_clk_set_defaults() failing for some reason?

It's __set_clk_rates() that failes with EPROBE_DEFER when calling
of_clk_get_from_provider().

> Can you provide
> more details about the DTB, or DT node being used?

The clock parent is the master clock from a SAI where the MCLK gets
registered in the SAI driver. Where fixed-factor-clock's will create
bit clk and frame clk.

Extra details: SAI can provide BCLK and FCLK but the codec needs FCLK
running all the time and the SAI can only keep the MCLK running. Like on
pause/XRUNS.