[PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Saravana Kannan]

We already mark fwnodes as initialized when they are registered as clock
providers. We do this so that fw_devlink can tell when a clock driver
doesn't use the driver core framework to probe/initialize its device.
This ensures fw_devlink doesn't block the consumers of such a clock
provider indefinitely.

However, some users of CLK_OF_DECLARE() macros don't use the same node
that matches the macro as the node for the clock provider, but they
initialize the entire node. To cover these cases, also mark the nodes
that match the macros as initialized when the init callback function is
called.

An example of this is "stericsson,u8500-clks" that's handled using
CLK_OF_DECLARE() and looks something like this:

clocks {
	compatible = "stericsson,u8500-clks";

	prcmu_clk: prcmu-clock {
		#clock-cells = <1>;
	};

	prcc_pclk: prcc-periph-clock {
		#clock-cells = <2>;
	};

	prcc_kclk: prcc-kernel-clock {
		#clock-cells = <2>;
	};

	prcc_reset: prcc-reset-controller {
		#reset-cells = <2>;
	};
	...
	...
};

This patch makes sure that "clocks" is marked as initialized so that
fw_devlink knows that all nodes under it have been initialized.

If the driver creates struct devices for some of the subnodes,
fw_devlink is smart enough to know to wait for those devices to probe.
So, no special handling is required for those cases.

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>
---
 include/linux/clk-provider.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 842e72a5348f..c9f5276006a0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1363,7 +1363,13 @@ struct clk_hw_onecell_data {
 	struct clk_hw *hws[];
 };
 
-#define CLK_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
+#define CLK_OF_DECLARE(name, compat, fn) \
+	static void __init name##_of_clk_init_declare(struct device_node *np) \
+	{								\
+		fn(np);							\
+		fwnode_dev_initialized(of_fwnode_handle(np), true);	\
+	}								\
+	OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
 
 /*
  * Use this macro when you have a driver that requires two initialization
-- 
2.39.2.722.g9855ee24e9-goog

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Saravana Kannan]

On Wed, Mar 1, 2023 at 5:46 PM Saravana Kannan <saravanak@google.com> wrote:
>
> We already mark fwnodes as initialized when they are registered as clock
> providers. We do this so that fw_devlink can tell when a clock driver
> doesn't use the driver core framework to probe/initialize its device.
> This ensures fw_devlink doesn't block the consumers of such a clock
> provider indefinitely.
>
> However, some users of CLK_OF_DECLARE() macros don't use the same node
> that matches the macro as the node for the clock provider, but they
> initialize the entire node. To cover these cases, also mark the nodes
> that match the macros as initialized when the init callback function is
> called.
>
> An example of this is "stericsson,u8500-clks" that's handled using
> CLK_OF_DECLARE() and looks something like this:
>
> clocks {
>         compatible = "stericsson,u8500-clks";
>
>         prcmu_clk: prcmu-clock {
>                 #clock-cells = <1>;
>         };
>
>         prcc_pclk: prcc-periph-clock {
>                 #clock-cells = <2>;
>         };
>
>         prcc_kclk: prcc-kernel-clock {
>                 #clock-cells = <2>;
>         };
>
>         prcc_reset: prcc-reset-controller {
>                 #reset-cells = <2>;
>         };
>         ...
>         ...
> };
>
> This patch makes sure that "clocks" is marked as initialized so that
> fw_devlink knows that all nodes under it have been initialized.
>
> If the driver creates struct devices for some of the subnodes,
> fw_devlink is smart enough to know to wait for those devices to probe.
> So, no special handling is required for those cases.
>
> 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>

Linus, I changed the code a bit and I think this should work too. Can
you give it a shot please?

-Saravana

> ---
>  include/linux/clk-provider.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 842e72a5348f..c9f5276006a0 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -1363,7 +1363,13 @@ struct clk_hw_onecell_data {
>         struct clk_hw *hws[];
>  };
>
> -#define CLK_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn)
> +#define CLK_OF_DECLARE(name, compat, fn) \
> +       static void __init name##_of_clk_init_declare(struct device_node *np) \
> +       {                                                               \
> +               fn(np);                                                 \
> +               fwnode_dev_initialized(of_fwnode_handle(np), true);     \
> +       }                                                               \
> +       OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
>
>  /*
>   * Use this macro when you have a driver that requires two initialization
> --
> 2.39.2.722.g9855ee24e9-goog
>

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Linus Walleij]

On Thu, Mar 2, 2023 at 2:46 AM Saravana Kannan <saravanak@google.com> wrote:

> We already mark fwnodes as initialized when they are registered as clock
> providers. We do this so that fw_devlink can tell when a clock driver
> doesn't use the driver core framework to probe/initialize its device.
> This ensures fw_devlink doesn't block the consumers of such a clock
> provider indefinitely.
>
> However, some users of CLK_OF_DECLARE() macros don't use the same node
> that matches the macro as the node for the clock provider, but they
> initialize the entire node. To cover these cases, also mark the nodes
> that match the macros as initialized when the init callback function is
> called.
>
> An example of this is "stericsson,u8500-clks" that's handled using
> CLK_OF_DECLARE() and looks something like this:
>
> clocks {
>         compatible = "stericsson,u8500-clks";
>
>         prcmu_clk: prcmu-clock {
>                 #clock-cells = <1>;
>         };
>
>         prcc_pclk: prcc-periph-clock {
>                 #clock-cells = <2>;
>         };
>
>         prcc_kclk: prcc-kernel-clock {
>                 #clock-cells = <2>;
>         };
>
>         prcc_reset: prcc-reset-controller {
>                 #reset-cells = <2>;
>         };
>         ...
>         ...
> };
>
> This patch makes sure that "clocks" is marked as initialized so that
> fw_devlink knows that all nodes under it have been initialized.
>
> If the driver creates struct devices for some of the subnodes,
> fw_devlink is smart enough to know to wait for those devices to probe.
> So, no special handling is required for those cases.
>
> 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>

Works like a charm on U8500!
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Saravana Kannan]

On Thu, Mar 2, 2023 at 5:14 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Mar 2, 2023 at 2:46 AM Saravana Kannan <saravanak@google.com> wrote:
>
> > We already mark fwnodes as initialized when they are registered as clock
> > providers. We do this so that fw_devlink can tell when a clock driver
> > doesn't use the driver core framework to probe/initialize its device.
> > This ensures fw_devlink doesn't block the consumers of such a clock
> > provider indefinitely.
> >
> > However, some users of CLK_OF_DECLARE() macros don't use the same node
> > that matches the macro as the node for the clock provider, but they
> > initialize the entire node. To cover these cases, also mark the nodes
> > that match the macros as initialized when the init callback function is
> > called.
> >
> > An example of this is "stericsson,u8500-clks" that's handled using
> > CLK_OF_DECLARE() and looks something like this:
> >
> > clocks {
> >         compatible = "stericsson,u8500-clks";
> >
> >         prcmu_clk: prcmu-clock {
> >                 #clock-cells = <1>;
> >         };
> >
> >         prcc_pclk: prcc-periph-clock {
> >                 #clock-cells = <2>;
> >         };
> >
> >         prcc_kclk: prcc-kernel-clock {
> >                 #clock-cells = <2>;
> >         };
> >
> >         prcc_reset: prcc-reset-controller {
> >                 #reset-cells = <2>;
> >         };
> >         ...
> >         ...
> > };
> >
> > This patch makes sure that "clocks" is marked as initialized so that
> > fw_devlink knows that all nodes under it have been initialized.
> >
> > If the driver creates struct devices for some of the subnodes,
> > fw_devlink is smart enough to know to wait for those devices to probe.
> > So, no special handling is required for those cases.
> >
> > 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>
>
> Works like a charm on U8500!
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Stephen, Does this look good? Can we have Greg pull this in?

-Saravana

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Stephen Boyd]

Quoting Saravana Kannan (2023-03-01 17:46:38)
> We already mark fwnodes as initialized when they are registered as clock
> providers. We do this so that fw_devlink can tell when a clock driver
> doesn't use the driver core framework to probe/initialize its device.
> This ensures fw_devlink doesn't block the consumers of such a clock
> provider indefinitely.
> 
> However, some users of CLK_OF_DECLARE() macros don't use the same node
> that matches the macro as the node for the clock provider, but they
> initialize the entire node. To cover these cases, also mark the nodes
> that match the macros as initialized when the init callback function is
> called.
> 
> An example of this is "stericsson,u8500-clks" that's handled using
> CLK_OF_DECLARE() and looks something like this:
> 
> clocks {
>         compatible = "stericsson,u8500-clks";
> 
>         prcmu_clk: prcmu-clock {
>                 #clock-cells = <1>;
>         };
> 
>         prcc_pclk: prcc-periph-clock {
>                 #clock-cells = <2>;
>         };
> 
>         prcc_kclk: prcc-kernel-clock {
>                 #clock-cells = <2>;
>         };
> 
>         prcc_reset: prcc-reset-controller {
>                 #reset-cells = <2>;
>         };
>         ...
>         ...
> };
> 
> This patch makes sure that "clocks" is marked as initialized so that
> fw_devlink knows that all nodes under it have been initialized.
> 
> If the driver creates struct devices for some of the subnodes,
> fw_devlink is smart enough to know to wait for those devices to probe.
> So, no special handling is required for those cases.
> 
> 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>
> ---

Applied to clk-fixes

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Saravana Kannan]

On Mon, Mar 6, 2023 at 11:14 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Saravana Kannan (2023-03-01 17:46:38)
> > We already mark fwnodes as initialized when they are registered as clock
> > providers. We do this so that fw_devlink can tell when a clock driver
> > doesn't use the driver core framework to probe/initialize its device.
> > This ensures fw_devlink doesn't block the consumers of such a clock
> > provider indefinitely.
> >
> > However, some users of CLK_OF_DECLARE() macros don't use the same node
> > that matches the macro as the node for the clock provider, but they
> > initialize the entire node. To cover these cases, also mark the nodes
> > that match the macros as initialized when the init callback function is
> > called.
> >
> > An example of this is "stericsson,u8500-clks" that's handled using
> > CLK_OF_DECLARE() and looks something like this:
> >
> > clocks {
> >         compatible = "stericsson,u8500-clks";
> >
> >         prcmu_clk: prcmu-clock {
> >                 #clock-cells = <1>;
> >         };
> >
> >         prcc_pclk: prcc-periph-clock {
> >                 #clock-cells = <2>;
> >         };
> >
> >         prcc_kclk: prcc-kernel-clock {
> >                 #clock-cells = <2>;
> >         };
> >
> >         prcc_reset: prcc-reset-controller {
> >                 #reset-cells = <2>;
> >         };
> >         ...
> >         ...
> > };
> >
> > This patch makes sure that "clocks" is marked as initialized so that
> > fw_devlink knows that all nodes under it have been initialized.
> >
> > If the driver creates struct devices for some of the subnodes,
> > fw_devlink is smart enough to know to wait for those devices to probe.
> > So, no special handling is required for those cases.
> >
> > 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>
> > ---
>
> Applied to clk-fixes

Thanks!

-Saravana

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Stephen Boyd]

Quoting Saravana Kannan (2023-03-03 13:24:00)
> On Thu, Mar 2, 2023 at 5:14 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > Works like a charm on U8500!
> > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Stephen, Does this look good? Can we have Greg pull this in?
> 

I picked it up for fixes. Will send it off later this week.

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Nathan Chancellor]

On Mon, Mar 06, 2023 at 11:15:54AM -0800, Stephen Boyd wrote:
> Quoting Saravana Kannan (2023-03-03 13:24:00)
> > On Thu, Mar 2, 2023 at 5:14 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > Works like a charm on U8500!
> > > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > Stephen, Does this look good? Can we have Greg pull this in?
> > 
> 
> I picked it up for fixes. Will send it off later this week.

This change causes a build regression:

  drivers/clk/mvebu/kirkwood.c:358:1: error: expected identifier or '('
  CLK_OF_DECLARE(98dx1135_clk, "marvell,mv98dx1135-core-clock",
  ^
  include/linux/clk-provider.h:1367:21: note: expanded from macro 'CLK_OF_DECLARE'
          static void __init name##_of_clk_init_declare(struct device_node *np) \
                             ^
  <scratch space>:124:1: note: expanded from here
  98dx1135_clk_of_clk_init_declare
  ^
  drivers/clk/mvebu/kirkwood.c:358:1: error: invalid digit 'd' in decimal constant
  include/linux/clk-provider.h:1372:34: note: expanded from macro 'CLK_OF_DECLARE'
          OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
                                          ^
  <scratch space>:125:3: note: expanded from here
  98dx1135_clk_of_clk_init_declare
    ^
  drivers/clk/mvebu/kirkwood.c:358:1: error: invalid digit 'd' in decimal constant
  include/linux/clk-provider.h:1372:34: note: expanded from macro 'CLK_OF_DECLARE'
          OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
                                          ^
  <scratch space>:125:3: note: expanded from here
  98dx1135_clk_of_clk_init_declare
    ^
  drivers/clk/mvebu/kirkwood.c:358:1: error: invalid digit 'd' in decimal constant
  include/linux/clk-provider.h:1372:34: note: expanded from macro 'CLK_OF_DECLARE'
          OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
                                          ^
  <scratch space>:125:3: note: expanded from here
  98dx1135_clk_of_clk_init_declare
    ^
  4 errors generated.

The clock's name starts with a number, resulting in an invalid C
function name. Are clock names ABI? Should this be fixed by adjusting
the name like so or adjusting where the name appears in the function
defined by CLK_OF_DECLARE()?

diff --git a/drivers/clk/mvebu/kirkwood.c b/drivers/clk/mvebu/kirkwood.c
index 8bc893df4736..5d0a7e3bfde5 100644
--- a/drivers/clk/mvebu/kirkwood.c
+++ b/drivers/clk/mvebu/kirkwood.c
@@ -355,5 +355,5 @@ CLK_OF_DECLARE(kirkwood_clk, "marvell,kirkwood-core-clock",
 	       kirkwood_clk_init);
 CLK_OF_DECLARE(mv88f6180_clk, "marvell,mv88f6180-core-clock",
 	       kirkwood_clk_init);
-CLK_OF_DECLARE(98dx1135_clk, "marvell,mv98dx1135-core-clock",
+CLK_OF_DECLARE(mv98dx1135_clk, "marvell,mv98dx1135-core-clock",
 	       kirkwood_clk_init);

Cheers,
Nathan

Re: [PATCH v2] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE() macro

[Saravana Kannan]

On Wed, Mar 8, 2023 at 8:01 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Mon, Mar 06, 2023 at 11:15:54AM -0800, Stephen Boyd wrote:
> > Quoting Saravana Kannan (2023-03-03 13:24:00)
> > > On Thu, Mar 2, 2023 at 5:14 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > >
> > > > Works like a charm on U8500!
> > > > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> > > Stephen, Does this look good? Can we have Greg pull this in?
> > >
> >
> > I picked it up for fixes. Will send it off later this week.
>
> This change causes a build regression:
>
>   drivers/clk/mvebu/kirkwood.c:358:1: error: expected identifier or '('
>   CLK_OF_DECLARE(98dx1135_clk, "marvell,mv98dx1135-core-clock",
>   ^
>   include/linux/clk-provider.h:1367:21: note: expanded from macro 'CLK_OF_DECLARE'
>           static void __init name##_of_clk_init_declare(struct device_node *np) \
>                              ^
>   <scratch space>:124:1: note: expanded from here
>   98dx1135_clk_of_clk_init_declare
>   ^
>   drivers/clk/mvebu/kirkwood.c:358:1: error: invalid digit 'd' in decimal constant
>   include/linux/clk-provider.h:1372:34: note: expanded from macro 'CLK_OF_DECLARE'
>           OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
>                                           ^
>   <scratch space>:125:3: note: expanded from here
>   98dx1135_clk_of_clk_init_declare
>     ^
>   drivers/clk/mvebu/kirkwood.c:358:1: error: invalid digit 'd' in decimal constant
>   include/linux/clk-provider.h:1372:34: note: expanded from macro 'CLK_OF_DECLARE'
>           OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
>                                           ^
>   <scratch space>:125:3: note: expanded from here
>   98dx1135_clk_of_clk_init_declare
>     ^
>   drivers/clk/mvebu/kirkwood.c:358:1: error: invalid digit 'd' in decimal constant
>   include/linux/clk-provider.h:1372:34: note: expanded from macro 'CLK_OF_DECLARE'
>           OF_DECLARE_1(clk, name, compat, name##_of_clk_init_declare)
>                                           ^
>   <scratch space>:125:3: note: expanded from here
>   98dx1135_clk_of_clk_init_declare
>     ^
>   4 errors generated.
>
> The clock's name starts with a number, resulting in an invalid C
> function name. Are clock names ABI? Should this be fixed by adjusting
> the name like so or adjusting where the name appears in the function
> defined by CLK_OF_DECLARE()?
>

Not sure if the name matters, but it's just simpler to fix the macro
than add churn to all those drivers. Can you send a patch that just
adds __ in the front of the function name in the macro please?

Thanks,
Saravana

> diff --git a/drivers/clk/mvebu/kirkwood.c b/drivers/clk/mvebu/kirkwood.c
> index 8bc893df4736..5d0a7e3bfde5 100644
> --- a/drivers/clk/mvebu/kirkwood.c
> +++ b/drivers/clk/mvebu/kirkwood.c
> @@ -355,5 +355,5 @@ CLK_OF_DECLARE(kirkwood_clk, "marvell,kirkwood-core-clock",
>                kirkwood_clk_init);
>  CLK_OF_DECLARE(mv88f6180_clk, "marvell,mv88f6180-core-clock",
>                kirkwood_clk_init);
> -CLK_OF_DECLARE(98dx1135_clk, "marvell,mv98dx1135-core-clock",
> +CLK_OF_DECLARE(mv98dx1135_clk, "marvell,mv98dx1135-core-clock",
>                kirkwood_clk_init);
>
> Cheers,
> Nathan