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@xxxxxxxxxx> wrote:
>
> Quoting Saravana Kannan (2023-03-01 13:25:13)
> > On Wed, Mar 1, 2023 at 12:48 PM Stephen Boyd <sboyd@xxxxxxxxxx> 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