[PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Saravana Kannan]

The CLK_OF_DECLARE macros sometimes prevent the creation of struct
devices for the device node being handled. It does this by
setting/clearing OF_POPULATED flag. This can block the probing of some
devices because fw_devlink will block the consumers of this node till a
struct device is created and probed.

Set the appropriate fwnode flags when these device nodes are initialized
by the clock framework and when OF_POPULATED flag is set/cleared. This
will allow fw_devlink to handle the dependencies correctly.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: Linus Walleij <linus.walleij@linaro.org>
Link: https://lore.kernel.org/lkml/CACRpkdamxDX6EBVjKX5=D3rkHp17f5pwGdBVhzFU90-0MHY6dQ@mail.gmail.com/
Fixes: 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---

This is fixing a patch that landed through driver core. So, should Greg
be pulling in this fix?

-Saravana

 drivers/clk/clk.c            | 4 ++++
 include/linux/clk-provider.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ae07685c7588..8bd5b9841993 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -5361,6 +5361,10 @@ void __init of_clk_init(const struct of_device_id *matches)
 				clk_provider->clk_init_cb(clk_provider->np);
 				of_clk_set_defaults(clk_provider->np, true);
 
+				fwnode_dev_initialized(
+					of_fwnode_handle(clk_provider->np),
+					true);
+
 				list_del(&clk_provider->node);
 				of_node_put(clk_provider->np);
 				kfree(clk_provider);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 842e72a5348f..93c0b06a0f2b 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1373,6 +1373,7 @@ struct clk_hw_onecell_data {
 	static void __init name##_of_clk_init_driver(struct device_node *np) \
 	{								\
 		of_node_clear_flag(np, OF_POPULATED);			\
+		fwnode_dev_initialized(of_fwnode_handle(np), false);	\
 		fn(np);							\
 	}								\
 	OF_DECLARE_1(clk, name, compat, name##_of_clk_init_driver)
-- 
2.39.2.722.g9855ee24e9-goog

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Linus Walleij]

On Wed, Mar 1, 2023 at 2:25 AM Saravana Kannan <saravanak@google.com> wrote:

> The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> devices for the device node being handled. It does this by
> setting/clearing OF_POPULATED flag. This can block the probing of some
> devices because fw_devlink will block the consumers of this node till a
> struct device is created and probed.
>
> Set the appropriate fwnode flags when these device nodes are initialized
> by the clock framework and when OF_POPULATED flag is set/cleared. This
> will allow fw_devlink to handle the dependencies correctly.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Link: https://lore.kernel.org/lkml/CACRpkdamxDX6EBVjKX5=D3rkHp17f5pwGdBVhzFU90-0MHY6dQ@mail.gmail.com/
> Fixes: 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
> Signed-off-by: Saravana Kannan <saravanak@google.com>

Excellent Saravana, this fixes my issue!
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Stephen Boyd]

Quoting Saravana Kannan (2023-02-28 17:25:06)
> The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> devices for the device node being handled. It does this by
> setting/clearing OF_POPULATED flag. This can block the probing of some
> devices because fw_devlink will block the consumers of this node till a
> struct device is created and probed.

Why can't you use CLK_OF_DECLARE_DRIVER()?

> 
> Set the appropriate fwnode flags when these device nodes are initialized
> by the clock framework and when OF_POPULATED flag is set/cleared. This
> will allow fw_devlink to handle the dependencies correctly.

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Saravana Kannan]

On Wed, Mar 1, 2023 at 12:40 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Saravana Kannan (2023-02-28 17:25:06)
> > The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> > devices for the device node being handled. It does this by
> > setting/clearing OF_POPULATED flag. This can block the probing of some
> > devices because fw_devlink will block the consumers of this node till a
> > struct device is created and probed.
>
> Why can't you use CLK_OF_DECLARE_DRIVER()?

Not sure if you looked at the patch. This is a fix for the macros, not
this specific user of CLK_OF_DECLARE(). Even if Linus fixes his
specific case, this change is still needed for other users of
CLK_OF_DECLARE().

Honestly, a ton of existing CLK_OF_DECLARE() users should really be
switching to CLK_OF_DECLARE_DRIVER() or better yet, implement them as
normal platform drivers when the specific clock control doesn't
provide any early clocks.

-Saravana

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Stephen Boyd]

Quoting Stephen Boyd (2023-03-01 12:40:03)
> Quoting Saravana Kannan (2023-02-28 17:25:06)
> > The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> > devices for the device node being handled. It does this by
> > setting/clearing OF_POPULATED flag. This can block the probing of some
> > devices because fw_devlink will block the consumers of this node till a
> > struct device is created and probed.
> 
> Why can't you use CLK_OF_DECLARE_DRIVER()?

Ah I misunderstood. CLK_OF_DECLARE() _always_ prevents the creation of a
struct device for the device node being handled. The 'sometimes' threw
me off.

> 
> > 
> > Set the appropriate fwnode flags when these device nodes are initialized
> > by the clock framework and when OF_POPULATED flag is set/cleared. This
> > will allow fw_devlink to handle the dependencies correctly.

How is this different from commit 3c9ea42802a1 ("clk: Mark fwnodes when
their clock provider is added/removed")? Do you have some user of
CLK_OF_DECLARE() that isn't registering an OF clk provider?

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Saravana Kannan]

On Wed, Mar 1, 2023 at 12:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Stephen Boyd (2023-03-01 12:40:03)
> > Quoting Saravana Kannan (2023-02-28 17:25:06)
> > > The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> > > devices for the device node being handled. It does this by
> > > setting/clearing OF_POPULATED flag. This can block the probing of some
> > > devices because fw_devlink will block the consumers of this node till a
> > > struct device is created and probed.
> >
> > Why can't you use CLK_OF_DECLARE_DRIVER()?
>
> Ah I misunderstood. CLK_OF_DECLARE() _always_ prevents the creation of a
> struct device for the device node being handled. The 'sometimes' threw
> me off.

The "sometimes" is because dependending on the macro we go back and
clear the flag.

> >
> > >
> > > Set the appropriate fwnode flags when these device nodes are initialized
> > > by the clock framework and when OF_POPULATED flag is set/cleared. This
> > > will allow fw_devlink to handle the dependencies correctly.
>
> How is this different from commit 3c9ea42802a1 ("clk: Mark fwnodes when
> their clock provider is added/removed")? Do you have some user of
> CLK_OF_DECLARE() that isn't registering an OF clk provider?

So it looks like drivers don't always register the same node used for
CLK_OF_DECLARE() as the clock provider. So, this is covering for the
case when that's not true.

-Saravana

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Stephen Boyd]

Quoting Saravana Kannan (2023-03-01 13:25:13)
> On Wed, Mar 1, 2023 at 12:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Stephen Boyd (2023-03-01 12:40:03)
> > > Quoting Saravana Kannan (2023-02-28 17:25:06)
> > > > The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> > > > devices for the device node being handled. It does this by
> > > > setting/clearing OF_POPULATED flag. This can block the probing of some
> > > > devices because fw_devlink will block the consumers of this node till a
> > > > struct device is created and probed.
> > >
> > > Why can't you use CLK_OF_DECLARE_DRIVER()?
> >
> > Ah I misunderstood. CLK_OF_DECLARE() _always_ prevents the creation of a
> > struct device for the device node being handled. The 'sometimes' threw
> > me off.
> 
> The "sometimes" is because dependending on the macro we go back and
> clear the flag.

Ok. Maybe instead of this paragraph you can explain the problem being
fixed, specifically ux500 container node not being marked as
"initialized" and that preventing consumer devices from probing. That
would help the reviewer understand the specific problem you're solving.

> 
> > >
> > > >
> > > > Set the appropriate fwnode flags when these device nodes are initialized
> > > > by the clock framework and when OF_POPULATED flag is set/cleared. This
> > > > will allow fw_devlink to handle the dependencies correctly.

This is the "what" and not the "why".

> >
> > How is this different from commit 3c9ea42802a1 ("clk: Mark fwnodes when
> > their clock provider is added/removed")? Do you have some user of
> > CLK_OF_DECLARE() that isn't registering an OF clk provider?
> 
> So it looks like drivers don't always register the same node used for
> CLK_OF_DECLARE() as the clock provider. So, this is covering for the
> case when that's not true.

Please add this information to the commit text. Otherwise the patch
looks entirely unnecessary.

If the node used for CLK_OF_DECLARE() isn't the same as the node as the
clock provider then how are we certain that the CLK_OF_DECLARE() probe
function has actually registered a clk provider?

Should we simply remove the calls to fwnode_dev_initialized() in the OF
clk provider functions and put the call in CLK_OF_DECLARE() (and
specifically _not_ CLK_OF_DECLARE_DRIVER) as this patch does? What about
bindings that are registering clks early with CLK_OF_DECLARE_DRIVER()
and then probing something like a reset controller later with a platform
device created by an MFD matching the same compatible as the
CLK_OF_DECLARE_DRIVER() compatible?

Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros

[Saravana Kannan]

On Wed, Mar 1, 2023 at 2:45 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Saravana Kannan (2023-03-01 13:25:13)
> > On Wed, Mar 1, 2023 at 12:48 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Stephen Boyd (2023-03-01 12:40:03)
> > > > Quoting Saravana Kannan (2023-02-28 17:25:06)
> > > > > The CLK_OF_DECLARE macros sometimes prevent the creation of struct
> > > > > devices for the device node being handled. It does this by
> > > > > setting/clearing OF_POPULATED flag. This can block the probing of some
> > > > > devices because fw_devlink will block the consumers of this node till a
> > > > > struct device is created and probed.
> > > >
> > > > Why can't you use CLK_OF_DECLARE_DRIVER()?
> > >
> > > Ah I misunderstood. CLK_OF_DECLARE() _always_ prevents the creation of a
> > > struct device for the device node being handled. The 'sometimes' threw
> > > me off.
> >
> > The "sometimes" is because dependending on the macro we go back and
> > clear the flag.
>
> Ok. Maybe instead of this paragraph you can explain the problem being
> fixed, specifically ux500 container node not being marked as
> "initialized" and that preventing consumer devices from probing. That
> would help the reviewer understand the specific problem you're solving.

Ack

>
> >
> > > >
> > > > >
> > > > > Set the appropriate fwnode flags when these device nodes are initialized
> > > > > by the clock framework and when OF_POPULATED flag is set/cleared. This
> > > > > will allow fw_devlink to handle the dependencies correctly.
>
> This is the "what" and not the "why".
>
> > >
> > > How is this different from commit 3c9ea42802a1 ("clk: Mark fwnodes when
> > > their clock provider is added/removed")? Do you have some user of
> > > CLK_OF_DECLARE() that isn't registering an OF clk provider?
> >
> > So it looks like drivers don't always register the same node used for
> > CLK_OF_DECLARE() as the clock provider. So, this is covering for the
> > case when that's not true.
>
> Please add this information to the commit text. Otherwise the patch
> looks entirely unnecessary.

Ack.

> If the node used for CLK_OF_DECLARE() isn't the same as the node as the
> clock provider then how are we certain that the CLK_OF_DECLARE() probe
> function has actually registered a clk provider?

Whether it's registered or not, we can't wait for a struct device to
be created for it. That's what the flag is about. device link can't
work without devices.

> Should we simply remove the calls to fwnode_dev_initialized() in the OF
> clk provider functions

Not all clock providers are going through CLK_OF_DECLARE(). There are
so many ways to register a clock provider. So it's good to cover all
those cases and leave those existing calls in.

> and put the call in CLK_OF_DECLARE() (and
> specifically _not_ CLK_OF_DECLARE_DRIVER) as this patch does?

Between the time the clk provider is initialized and a new struct
device being created, we still don't want to block consumers from
probing because they might be dependent on the already registered
early clocks. So, we should set the flag in the DRIVER case too. And
then we clear it once it has been initialized because we allow the
struct device to be populated and it's okay to wait for those.

> What about
> bindings that are registering clks early with CLK_OF_DECLARE_DRIVER()
> and then probing something like a reset controller later with a platform
> device created by an MFD matching the same compatible as the
> CLK_OF_DECLARE_DRIVER() compatible?

I think I answered it above?

-Saravana