Re: [PATCH v1] of: platform: Batch fwnode parsing in the init_machine() path

[Saravana Kannan]

On Fri, Oct 2, 2020 at 8:03 AM 'Grygorii Strashko' via kernel-team
<kernel-team@xxxxxxxxxxx> wrote:
>
>
>
> On 02/10/2020 14:40, Grygorii Strashko wrote:
> >
> >
> > On 02/10/2020 02:19, Laurent Pinchart wrote:
> >> Hi Saravana,
> >>
> >> Thank you for the patch.
> >>
> >> On Thu, Oct 01, 2020 at 03:59:51PM -0700, Saravana Kannan wrote:
> >>> When commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when
> >>> adding all top level devices") optimized the fwnode parsing when all top
> >>> level devices are added, it missed out optimizing this for platform
> >>> where the top level devices are added through the init_machine() path.
> >>>
> >>> This commit does the optimization for all paths by simply moving the
> >>> fw_devlink_pause/resume() inside of_platform_default_populate().
> >>
> >> Based on v5.9-rc5, before the patch:
> >>
> >> [    0.652887] cpuidle: using governor menu
> >> [   12.349476] No ATAGs?
> >>
> >> After the patch:
> >>
> >> [    0.650460] cpuidle: using governor menu
> >> [   12.262101] No ATAGs?
> >>
> >> :-(
> >
> > This is kinda expected :( because omap2 arch doesn't call of_platform_default_populate()
> >
> > Call path:
> > board-generic.c
> >   DT_MACHINE_START()
> >     .init_machine    = omap_generic_init,
> >
> >   omap_generic_init()
> >     pdata_quirks_init(omap_dt_match_table);
> >          of_platform_populate(NULL, omap_dt_match_table,
> >                   omap_auxdata_lookup, NULL);
> >
> > Other affected platforms
> > arm: mach-ux500
> > some mips
> > some powerpc
> >
> > there are also case when a lot of devices placed under bus node, in such case
> >   of_platform_populate() calls from bus drivers will also suffer from this issue.
> >
> > I think one option could be to add some parameter to _populate() or introduce new api.
> >
> > By the way, is there option to disable this feature at all?
> > Is there Kconfig option?
> > Is there any reasons why such complex and time consuming code added to the kernel and not implemented on DTC level?
> >
> >
> > Also, I've came with another diff, pls check.
> >
> > [    0.000000] Booting Linux on physical CPU 0x0
> > [    0.000000] Linux version 5.9.0-rc6-01791-g9acba6b38757-dirty (grygorii@grygorii-XPS-13-9370) (arm-linux-gnueabihf-gcc (GNU Toolcha0
> > [    0.000000] CPU: ARMv7 Processor [412fc0f2] revision 2 (ARMv7), cr=10c5387d
> > [    0.000000] CPU: div instructions available: patching division code
> > [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
> > [    0.000000] OF: fdt: Machine model: TI AM5718 IDK
> > ...
> > [    0.053443] cpuidle: using governor ladder
> > [    0.053470] cpuidle: using governor menu
> > [    0.089304] No ATAGs?
> > ...
> > [    3.092291] devtmpfs: mounted
> > [    3.095804] Freeing unused kernel memory: 1024K
> > [    3.100483] Run /sbin/init as init process
> >
> >
> >
> > ------ >< ---
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 071f04da32c8..4521b26e7745 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -514,6 +514,12 @@ static const struct of_device_id reserved_mem_matches[] = {
> >          {}
> >   };
> >
> > +static int __init of_platform_fw_devlink_pause(void)
> > +{
> > +       fw_devlink_pause();
> > +}
> > +core_initcall(of_platform_fw_devlink_pause);
> > +
> >   static int __init of_platform_default_populate_init(void)
> >   {
> >          struct device_node *node;
> > @@ -538,9 +544,7 @@ static int __init of_platform_default_populate_init(void)
> >          }
> >
> >          /* Populate everything else. */
> > -       fw_devlink_pause();
> >          of_platform_default_populate(NULL, NULL, NULL);
> > -       fw_devlink_resume();
> >
> >          return 0;
> >   }
> > @@ -548,6 +552,7 @@ arch_initcall_sync(of_platform_default_populate_init);
> >
> >   static int __init of_platform_sync_state_init(void)
> >   {
> > +       fw_devlink_resume();
>
> ^ it seems has to be done earlier, like
> +static int __init of_platform_fw_devlink_resume(void)
> +{
> +       fw_devlink_resume();
> +       return 0;
> +}
> +device_initcall_sync(of_platform_fw_devlink_resume);

This will mean no device will probe until device_initcall_sync().
Unfortunately, I don't think we can make such a sweeping assumption.

-Saravana