Message-ID: <a3bde7aa1793c20638cbf2749f3209a2.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAGETcx82r-YC7cBUY5xa57FCEOUP_BeGNp2zURG=HUJkUMSVPA@mail.gmail.com>
References: <20230301012506.1401883-1-saravanak@google.com> <31ae9957edf319416d4551f14eba2071.sboyd@kernel.org> <7cca9a24b24d849565cd6a4f40ddbee9.sboyd@kernel.org> <CAGETcx82r-YC7cBUY5xa57FCEOUP_BeGNp2zURG=HUJkUMSVPA@mail.gmail.com>
Subject: Re: [PATCH v1] clk: Mark a fwnode as initialized when using CLK_OF_DECLARE* macros
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        kernel-team@android.com, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
To:     Saravana Kannan <saravanak@google.com>
Date:   Wed, 01 Mar 2023 14:45:12 -0800
User-Agent: alot/0.10
Precedence: bulk

Quoting Saravana Kannan (2023-03-01 13:25:13)
> On Wed, Mar 1, 2023 at 12:48=E2=80=AFPM Stephen Boyd <sboyd@kernel.org> w=
rote:
> >
> > 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 s=
ome
> > > > devices because fw_devlink will block the consumers of this node ti=
ll 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.
>=20
> 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.

>=20
> > >
> > > >
> > > > Set the appropriate fwnode flags when these device nodes are initia=
lized
> > > > by the clock framework and when OF_POPULATED flag is set/cleared. T=
his
> > > > 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?
>=20
> 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?