Message-ID: <CAGETcx8+RFPM51hixbpmroHB17iMpxeyUrFe6hOLaoQNNp-zKg@mail.gmail.com>
Date:   Tue, 9 Feb 2021 11:06:32 -0800
From:   Saravana Kannan <saravanak@...gle.com>
To:     Tudor Ambarus <Tudor.Ambarus@...rochip.com>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Nicolas.Ferre@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Ludovic Desroches <Ludovic.Desroches@...rochip.com>,
        Claudiu Beznea <Claudiu.Beznea@...rochip.com>,
        mirq-linux@...e.qmqm.pl,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: at91: sama5d2: Mark device OF_POPULATED after setup

On Tue, Feb 9, 2021 at 7:21 AM <Tudor.Ambarus@...rochip.com> wrote:
>
> Hi, Saravana,
>
> On 2/9/21 11:11 AM, Saravana Kannan wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Mon, Feb 8, 2021 at 11:55 PM Stephen Boyd <sboyd@...nel.org> wrote:
> >>
> >> Quoting Saravana Kannan (2021-01-28 09:01:41)
> >>> On Thu, Jan 28, 2021 at 2:45 AM Tudor Ambarus
> >>> <tudor.ambarus@...rochip.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@...rochip.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,
>
> No, there's a single driver that binds to that compatible.
>
> > 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
>
> From this entire list only the drivers/clk/at91/sama5d2.c driver binds to the
> "atmel,sama5d2-pmc" compatible, the rest are just using the compatible to
> map the PMC memory.
>
> > 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?
>
> The series at [1] doesn't apply clean neither on next-20210209, nor on
> driver-core-next. On top of which sha1 should I apply them?

It's on top of driver-core-next:
4731210c09f5 gpiolib: Bind gpio_device to a driver to enable
fw_devlink=on by default

> Anyway, I think the patch at [2] is still needed, regardless of the outcome
> of [1].

Right, [2] is still a good clean up based on your comment above.

-Saravana

> >
> > [1] - https://lore.kernel.org/lkml/20210205222644.2357303-1-saravanak@google.com/
>
> [2] https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.ambarus@microchip.com/
>
> Cheers,
> ta
>

Powered by blists - more mailing lists