Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAGETcx8owDP_Bu4oNCyHEsME8XpKygxghm8+yNc2RyMA4wyjCA@mail.gmail.com>
 <20201001225952.3676755-1-saravanak@google.com> <20201001231922.GG3722@pendragon.ideasonboard.com>
 <17bdc3f0-d816-151a-fef2-88cd38fc8621@ti.com> <e0ef8816-11ea-3a1a-cac6-14b9f6c92bcf@ti.com>
In-Reply-To: <e0ef8816-11ea-3a1a-cac6-14b9f6c92bcf@ti.com>
From:   Saravana Kannan <saravanak@google.com>
Date:   Fri, 2 Oct 2020 10:48:22 -0700
Message-ID: <CAGETcx-wkZZQyfnkc59e2ECg_kho-O_c2ms4OOtM4=-Hd125+Q@mail.gmail.com>
Subject: Re: [PATCH v1] of: platform: Batch fwnode parsing in the
 init_machine() path
To:     Grygorii Strashko <grygorii.strashko@ti.com>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux-OMAP <linux-omap@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Peter Ujfalusi <peter.ujfalusi@ti.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Tomi Valkeinen <tomi.valkeinen@ti.com>,
        Tony Lindgren <tony@atomide.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Android Kernel Team <kernel-team@android.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 2, 2020 at 8:03 AM 'Grygorii Strashko' via kernel-team
<kernel-team@android.com> 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