Messages in this thread | | | From | Saravana Kannan <> | Date | Wed, 1 Mar 2023 15:52:41 -0800 | Subject | Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros |
| |
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
| |