[PATCH] clk: fixed: handle failed clk setup

[PATCH] clk: fixed: handle failed clk setup

[Emil Abildgaard Svendsen]

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.

Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>
---
 drivers/clk/clk-fixed-factor.c | 19 +++++++++++--------
 drivers/clk/clk-fixed-rate.c   | 11 ++++++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index f734e34735a9..d00837daca9e 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -268,14 +268,8 @@ static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 
 	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, NULL, 0,
 					    0, mult, div, false);
-	if (IS_ERR(hw)) {
-		/*
-		 * Clear OF_POPULATED flag so that clock registration can be
-		 * attempted again from probe function.
-		 */
-		of_node_clear_flag(node, OF_POPULATED);
+	if (IS_ERR(hw))
 		return ERR_CAST(hw);
-	}
 
 	ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw);
 	if (ret) {
@@ -292,7 +286,16 @@ static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
  */
 void __init of_fixed_factor_clk_setup(struct device_node *node)
 {
-	_of_fixed_factor_clk_setup(node);
+	struct clk_hw *hw;
+
+	hw = _of_fixed_factor_clk_setup(node);
+	if (IS_ERR(hw)) {
+		/*
+		 * Clear OF_POPULATED flag so that clock registration can be
+		 * attempted again from probe function.
+		 */
+		of_node_clear_flag(node, OF_POPULATED);
+	}
 }
 CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
 		of_fixed_factor_clk_setup);
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 7d775954e26d..e69f4285759e 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -192,7 +192,16 @@ static struct clk_hw *_of_fixed_clk_setup(struct device_node *node)
  */
 void __init of_fixed_clk_setup(struct device_node *node)
 {
-	_of_fixed_clk_setup(node);
+	struct clk_hw *hw;
+
+	hw = _of_fixed_clk_setup(node);
+	if (IS_ERR(hw)) {
+		/*
+		 * Clear OF_POPULATED flag so that clock registration can be
+		 * attempted again from probe function.
+		 */
+		of_node_clear_flag(node, OF_POPULATED);
+	}
 }
 CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
 
-- 
2.34.1

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

[Emil Abildgaard Svendsen]

On 4/18/23 02:34, 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.
> 
> Did something break this recently? For example, commit c28cd1f3433c
> ("clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro")?

No it's also in v6.1 and looking back it could have been there since the
platform driver was added or close to.

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

[Emil Abildgaard Svendsen]

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? 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. Clearing the flag will allow the
fixed-factor-clock to be probed after the clock provider has been
successfully created.

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

[Emil Abildgaard Svendsen]

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?
> 
> 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? 

> > > 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.

> > Clearing the flag will allow the
> > fixed-factor-clock to be probed after the clock provider has been
> > successfully created.
> 
> Got it. By clock provider you mean the parent, right?

Yes, you're correct.

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

[Emil Abildgaard Svendsen]

On Mon, Apr 17, 2023 at 05:34:45PM -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.
> 
> Did something break this recently? For example, commit c28cd1f3433c
> ("clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro")?

Sorry for missing this mail. But no nothing broke I just added support
for some HW in a DT where the clock parent has a lot of dependencies and
has a likelihood of getting deferred until all the dependencies has been
probed. 

> > 
> > This patch will clear the OF_POPULATED falg if clock setup failes.
> 
> s/falg/flag/

Thank you! I will update the commit message. I'm still new to the
mailing list. What is the preferred way? Sending PATCH v2 or just resend
the updated patch to the mailing list?

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

[Emil Abildgaard Svendsen]

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().

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

[Emil Abildgaard Svendsen]

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.

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

[Emil Abildgaard Svendsen]

On Fri, Oct 27, 2023 at 07:55:55PM -0700, Stephen Boyd wrote:
> Quoting Emil Abildgaard Svendsen (2023-10-27 01:56:00)
> > 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:
> > > > > > > > > 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().
> 
> Thank you for fully describing the problem. These sorts of details
> should be in the commit text. It would have helped avoid this back and
> forth.

I agree. I will remember that next time and thank you for your patience.

> Coming to the patch, I'll have to think about this problem more. I see
> that we call of_clk_set_defaults() from multiple places, and sometimes
> multiple times (e.g. if a platform driver registers a clk provider it
> may be run twice). And we don't allow of_clk_init() to know that things
> have failed to register. It's quite a mess! We also don't know if
> failing to register clks during of_clk_init() is fatal, or if it will be
> ok to wait for a platform driver to probe later. And we blindly call
> of_clk_set_defaults() during of_clk_init() even if it fails. Ugh.
> 
> > 
> > > 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.
> 
> Having assigned-clock-rates on the fixed factor clk node is rare. I only
> see arch/arm/boot/dts/nxp/imx/imx6q-bosch-acc.dts which does this in
> mainline. Why do you do that instead of putting the assigned-clock-rates
> property in the consumer device node for the fixed factor clk?

Because there's two consumers. But I have just confirmed it's indeed
enough to just have it in the consumers instead. Thank you!