linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Tudor Ambarus <tudor.ambarus@microchip.com>,
	Michael Turquette <mturquette@baylibre.com>,
	nicolas.ferre@microchip.com,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Ludovic Desroches <ludovic.desroches@microchip.com>,
	Claudiu Beznea <claudiu.beznea@microchip.com>,
	mirq-linux@rere.qmqm.pl,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] clk: at91: sama5d2: Mark device OF_POPULATED after setup
Date: Tue, 9 Feb 2021 01:11:17 -0800	[thread overview]
Message-ID: <CAGETcx9C+sH-GKz61GfCxd9qk=E-AMueHSwPgp5Z_5QcOzXJiQ@mail.gmail.com> (raw)
In-Reply-To: <161285731192.418021.10555916396092570051@swboyd.mtv.corp.google.com>

On Mon, Feb 8, 2021 at 11:55 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Saravana Kannan (2021-01-28 09:01:41)
> > On Thu, Jan 28, 2021 at 2:45 AM Tudor Ambarus
> > <tudor.ambarus@microchip.com> wrote:
> > >
> > > The sama5d2 requires the clock provider initialized before timers.
> > > We can't use a platform driver for the sama5d2-pmc driver, as the
> > > platform_bus_init() is called later on, after time_init().
> > >
> > > As fw_devlink considers only devices, it does not know that the
> > > pmc is ready. Hence probing of devices that depend on it fail:
> > > probe deferral - supplier f0014000.pmc not ready
> > >
> > > Fix this by setting the OF_POPULATED flag for the sama5d2_pmc
> > > device node after successful setup. This will make
> > > of_link_to_phandle() ignore the sama5d2_pmc device node as a
> > > dependency, and consumer devices will be probed again.
> > >
> > > Fixes: e590474768f1cc04 ("driver core: Set fw_devlink=on by default")
> > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > > ---
> > > I'll be out of office, will check the rest of the at91 SoCs
> > > at the begining of next week.
> > >
> > >  drivers/clk/at91/sama5d2.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/clk/at91/sama5d2.c b/drivers/clk/at91/sama5d2.c
> > > index 9a5cbc7cd55a..5eea2b4a63dd 100644
> > > --- a/drivers/clk/at91/sama5d2.c
> > > +++ b/drivers/clk/at91/sama5d2.c
> > > @@ -367,6 +367,8 @@ static void __init sama5d2_pmc_setup(struct device_node *np)
> > >
> > >         of_clk_add_hw_provider(np, of_clk_hw_pmc_get, sama5d2_pmc);
> > >
> > > +       of_node_set_flag(np, OF_POPULATED);
> > > +
> > >         return;
> >
> > Hi Tudor,
> >
> > Thanks for looking into this.
> >
> > I already accounted for early clocks like this when I designed
> > fw_devlink. Each driver shouldn't need to set OF_POPULATED.
> > drivers/clk/clk.c already does this for you.
> >
> > I think the problem is that your driver is using
> > CLK_OF_DECLARE_DRIVER() instead of CLK_OF_DECLARE(). The comments for
> > CLK_OF_DECLARE_DRIVER() says:
> > /*
> >  * Use this macro when you have a driver that requires two initialization
> >  * routines, one at of_clk_init(), and one at platform device probe
> >  */
> >
> > In your case, you are explicitly NOT having a driver bind to this
> > clock later. So you shouldn't be using CLK_OF_DECLARE() instead.
> >
>
> I see
>
> drivers/power/reset/at91-sama5d2_shdwc.c:       { .compatible = "atmel,sama5d2-pmc" },
>
> so isn't that the driver that wants to bind to the same device node
> again? First at of_clk_init() time here and then second for the reset
> driver?

You are right. I assumed that when Tudor was setting OF_POPULATED,
they didn't want to create a struct device and they knew it was right
for their platform.

However...
$ git grep "atmel,sama5d2-pmc"
arch/arm/boot/dts/sama5d2.dtsi:                         compatible =
"atmel,sama5d2-pmc", "syscon";
arch/arm/mach-at91/pm.c:        { .compatible = "atmel,sama5d2-pmc",
.data = &pmc_infos[1] },
drivers/clk/at91/pmc.c: { .compatible = "atmel,sama5d2-pmc" },
drivers/clk/at91/sama5d2.c:CLK_OF_DECLARE_DRIVER(sama5d2_pmc,
"atmel,sama5d2-pmc", sama5d2_pmc_setup);
drivers/power/reset/at91-sama5d2_shdwc.c:       { .compatible =
"atmel,sama5d2-pmc" },

Geez! How many drivers are there for this one device. Clearly not all
of them are going to bind. But I'm not going to dig into this. You can
reject this patch. I expect this series [1] to take care of the issue
Tudor was trying to fix.

Tudor,

Want to give this series [1] a shot?

[1] - https://lore.kernel.org/lkml/20210205222644.2357303-1-saravanak@google.com/
-Saravana

  reply	other threads:[~2021-02-09  9:15 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-28 10:44 [PATCH] clk: at91: sama5d2: Mark device OF_POPULATED after setup Tudor Ambarus
2021-01-28 17:01 ` Saravana Kannan
2021-02-01 10:54   ` Geert Uytterhoeven
2021-02-01 17:16     ` Saravana Kannan
2021-02-09  7:55   ` Stephen Boyd
2021-02-09  9:11     ` Saravana Kannan [this message]
2021-02-09 15:21       ` Tudor.Ambarus
2021-02-09 19:06         ` Saravana Kannan
2021-02-10  8:09           ` Tudor.Ambarus
2021-02-09  9:42     ` Tudor.Ambarus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGETcx9C+sH-GKz61GfCxd9qk=E-AMueHSwPgp5Z_5QcOzXJiQ@mail.gmail.com' \
    --to=saravanak@google.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=claudiu.beznea@microchip.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ludovic.desroches@microchip.com \
    --cc=mirq-linux@rere.qmqm.pl \
    --cc=mturquette@baylibre.com \
    --cc=nicolas.ferre@microchip.com \
    --cc=sboyd@kernel.org \
    --cc=tudor.ambarus@microchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).