| Message ID | 20230301012506.1401883-1-saravanak@google.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros | expand |
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
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.
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
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?
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
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?
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
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)
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(+)