On Tue, Feb 9, 2021 at 7:21 AM <tudor.amba...@microchip.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 <sb...@kernel.org> wrote: > >> > >> Quoting Saravana Kannan (2021-01-28 09:01:41) > >>> On Thu, Jan 28, 2021 at 2:45 AM Tudor Ambarus > >>> <tudor.amba...@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.amba...@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, > > 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-sarava...@google.com/ > > [2] > https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.amba...@microchip.com/ > > Cheers, > ta >