[1/2] ARM: imx: drop uneccessary of_platform_default_populate
diff mbox series

Message ID 20190313091757.16410-1-peng.fan@nxp.com
State New
Headers show
Series
  • [1/2] ARM: imx: drop uneccessary of_platform_default_populate
Related show

Commit Message

Peng Fan March 13, 2019, 9:05 a.m. UTC
"arch_initcall_sync(of_platform_default_populate_init);" could be
used to populate the device tree, there is no need to call
of_platform_default_populate in machine code.

Tested on i.MX6Q-SDB i.MX6SL-EVK i.MX6UL-EVK board.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/mach-imx/mach-imx6q.c  | 2 --
 arch/arm/mach-imx/mach-imx6sl.c | 2 --
 arch/arm/mach-imx/mach-imx6sx.c | 2 --
 arch/arm/mach-imx/mach-imx6ul.c | 1 -
 4 files changed, 7 deletions(-)

Comments

Aisheng Dong March 15, 2019, 10:16 a.m. UTC | #1
> From: Peng Fan
> 
> "arch_initcall_sync(of_platform_default_populate_init);" could be used to
> populate the device tree, there is no need to call of_platform_default_populate
> in machine code.
> 
> Tested on i.MX6Q-SDB i.MX6SL-EVK i.MX6UL-EVK board.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c  | 2 --
>  arch/arm/mach-imx/mach-imx6sl.c | 2 --
>  arch/arm/mach-imx/mach-imx6sx.c | 2 --
>  arch/arm/mach-imx/mach-imx6ul.c | 1 -
>  4 files changed, 7 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c
> b/arch/arm/mach-imx/mach-imx6q.c index 7d80a0ae723c..655398c20256
> 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -278,8 +278,6 @@ static void __init imx6q_init_machine(void)
> 
>  	imx6q_enet_phy_init();
> 
> -	of_platform_default_populate(NULL, NULL, parent);
> -

This slightly change the device register sequence as well as possible driver probe
sequence as a consequence.

Originally devices are registered in arch_initcall. Now it will be a bit later
in arch_initcall_sync and this may cause a bit risk if the code under the
default_populate want to access the device service provided by early probe.

Probably it's more safe to leave as it is unless you can double confirm 
there're no such code depends on accessing early probed devices as follows
before we can make the change.

>  	imx_anatop_init();
>  	cpu_is_imx6q() ?  imx6q_pm_init() : imx6dl_pm_init();
>  	imx6q_1588_init();

Regards
Dong Aisheng

> diff --git a/arch/arm/mach-imx/mach-imx6sl.c
> b/arch/arm/mach-imx/mach-imx6sl.c index 99be4225297a..9743bdbb68fa
> 100644
> --- a/arch/arm/mach-imx/mach-imx6sl.c
> +++ b/arch/arm/mach-imx/mach-imx6sl.c
> @@ -56,8 +56,6 @@ static void __init imx6sl_init_machine(void)
>  	if (parent == NULL)
>  		pr_warn("failed to initialize soc device\n");
> 
> -	of_platform_default_populate(NULL, NULL, parent);
> -
>  	if (cpu_is_imx6sl())
>  		imx6sl_fec_init();
>  	imx_anatop_init();
> diff --git a/arch/arm/mach-imx/mach-imx6sx.c
> b/arch/arm/mach-imx/mach-imx6sx.c index 7f52d9b1e8a4..19b9f2dd309e
> 100644
> --- a/arch/arm/mach-imx/mach-imx6sx.c
> +++ b/arch/arm/mach-imx/mach-imx6sx.c
> @@ -72,8 +72,6 @@ static void __init imx6sx_init_machine(void)
>  	if (parent == NULL)
>  		pr_warn("failed to initialize soc device\n");
> 
> -	of_platform_default_populate(NULL, NULL, parent);
> -
>  	imx6sx_enet_init();
>  	imx_anatop_init();
>  	imx6sx_pm_init();
> diff --git a/arch/arm/mach-imx/mach-imx6ul.c
> b/arch/arm/mach-imx/mach-imx6ul.c index 6cb8a22b617d..c57b9df791b1
> 100644
> --- a/arch/arm/mach-imx/mach-imx6ul.c
> +++ b/arch/arm/mach-imx/mach-imx6ul.c
> @@ -65,7 +65,6 @@ static void __init imx6ul_init_machine(void)
>  	if (parent == NULL)
>  		pr_warn("failed to initialize soc device\n");
> 
> -	of_platform_default_populate(NULL, NULL, parent);
>  	imx6ul_enet_init();
>  	imx_anatop_init();
>  	imx6ul_pm_init();
> --
> 2.16.4
Peng Fan March 15, 2019, 12:56 p.m. UTC | #2
Hi Aisheng,

> -----Original Message-----
> From: Aisheng Dong
> Sent: 2019年3月15日 18:17
> To: Peng Fan <peng.fan@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; Anson Huang <anson.huang@nxp.com>;
> arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Cc: van.freenix@gmail.com
> Subject: RE: [PATCH 1/2] ARM: imx: drop uneccessary
> of_platform_default_populate
> 
> > From: Peng Fan
> >
> > "arch_initcall_sync(of_platform_default_populate_init);" could be used
> > to populate the device tree, there is no need to call
> > of_platform_default_populate in machine code.
> >
> > Tested on i.MX6Q-SDB i.MX6SL-EVK i.MX6UL-EVK board.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/arm/mach-imx/mach-imx6q.c  | 2 --
> > arch/arm/mach-imx/mach-imx6sl.c | 2 --
> > arch/arm/mach-imx/mach-imx6sx.c | 2 --
> > arch/arm/mach-imx/mach-imx6ul.c | 1 -
> >  4 files changed, 7 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c
> > b/arch/arm/mach-imx/mach-imx6q.c index 7d80a0ae723c..655398c20256
> > 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -278,8 +278,6 @@ static void __init imx6q_init_machine(void)
> >
> >  	imx6q_enet_phy_init();
> >
> > -	of_platform_default_populate(NULL, NULL, parent);
> > -
> 
> This slightly change the device register sequence as well as possible driver
> probe sequence as a consequence.

This patch is inspired from patch:

commit 850bea2335e42780a0752a75860d3fbcc3d12d6e
Author: Kefeng Wang <wangkefeng.wang@huawei.com>
Date:   Wed Jun 1 14:52:56 2016 +0800

    arm: Remove unnecessary of_platform_populate with default match table

    After patch "of/platform: Add common method to populate default bus",
    it is possible for arch code to remove unnecessary callers of
    of_platform_populate with default match table.

> 
> Originally devices are registered in arch_initcall. Now it will be a bit later in
> arch_initcall_sync and this may cause a bit risk if the code under the
> default_populate want to access the device service provided by early probe.
> 
> Probably it's more safe to leave as it is unless you can double confirm there're
> no such code depends on accessing early probed devices as follows before we
> can make the change.

This has been boot tested on 6Q-SDB/6UL-EVK/6SL-EVK board.
For i.MX, I only see pinctrl driver use arch_initcall from the link,
https://elixir.bootlin.com/linux/latest/ident/arch_initcall

From my boot test, the pinctrl driver probe will be delayed a bit, but I do
not see issues.

The common code is introduced by this patch, one thing need to consider
is the of_iommu_init and swiotlb_late_init on aarch64. My patch
is only for i.MX6/7, so I think there is no such issue.
commit 44a7185c2ae6383c88ff5b1ef2e2969f35d7b8b7
Author: Kefeng Wang <wangkefeng.wang@huawei.com>
Date:   Wed Jun 1 14:52:54 2016 +0800

    of/platform: Add common method to populate default bus

    The arch code calls of_platform_populate() with default match table
    when it wants to populate default bus.

    This patch introduce a new of_platform_default_populate_init() and make it
    arch_initcall_sync(it should be later than some iommu configration, eg,
    of_iommu_init() and swiotlb_late_init in arm64), then we can finish above
    job in common method.

    In order to avoid the default bus being populated twice, simply checking
    the flag of bus node whether has be set OF_POPULATED_BUS or not.

    After that, we can safely remove the caller in arch code.

    Btw, add debug print in of_platform_populate(), and use __func__ to
    print function's name of of_platform_bus_create().


Thanks,
Peng.

> 
> >  	imx_anatop_init();
> >  	cpu_is_imx6q() ?  imx6q_pm_init() : imx6dl_pm_init();
> >  	imx6q_1588_init();
> 
> Regards
> Dong Aisheng
> 
> > diff --git a/arch/arm/mach-imx/mach-imx6sl.c
> > b/arch/arm/mach-imx/mach-imx6sl.c index 99be4225297a..9743bdbb68fa
> > 100644
> > --- a/arch/arm/mach-imx/mach-imx6sl.c
> > +++ b/arch/arm/mach-imx/mach-imx6sl.c
> > @@ -56,8 +56,6 @@ static void __init imx6sl_init_machine(void)
> >  	if (parent == NULL)
> >  		pr_warn("failed to initialize soc device\n");
> >
> > -	of_platform_default_populate(NULL, NULL, parent);
> > -
> >  	if (cpu_is_imx6sl())
> >  		imx6sl_fec_init();
> >  	imx_anatop_init();
> > diff --git a/arch/arm/mach-imx/mach-imx6sx.c
> > b/arch/arm/mach-imx/mach-imx6sx.c index 7f52d9b1e8a4..19b9f2dd309e
> > 100644
> > --- a/arch/arm/mach-imx/mach-imx6sx.c
> > +++ b/arch/arm/mach-imx/mach-imx6sx.c
> > @@ -72,8 +72,6 @@ static void __init imx6sx_init_machine(void)
> >  	if (parent == NULL)
> >  		pr_warn("failed to initialize soc device\n");
> >
> > -	of_platform_default_populate(NULL, NULL, parent);
> > -
> >  	imx6sx_enet_init();
> >  	imx_anatop_init();
> >  	imx6sx_pm_init();
> > diff --git a/arch/arm/mach-imx/mach-imx6ul.c
> > b/arch/arm/mach-imx/mach-imx6ul.c index 6cb8a22b617d..c57b9df791b1
> > 100644
> > --- a/arch/arm/mach-imx/mach-imx6ul.c
> > +++ b/arch/arm/mach-imx/mach-imx6ul.c
> > @@ -65,7 +65,6 @@ static void __init imx6ul_init_machine(void)
> >  	if (parent == NULL)
> >  		pr_warn("failed to initialize soc device\n");
> >
> > -	of_platform_default_populate(NULL, NULL, parent);
> >  	imx6ul_enet_init();
> >  	imx_anatop_init();
> >  	imx6ul_pm_init();
> > --
> > 2.16.4
Aisheng Dong March 15, 2019, 1:38 p.m. UTC | #3
[...]

> > Originally devices are registered in arch_initcall. Now it will be a
> > bit later in arch_initcall_sync and this may cause a bit risk if the
> > code under the default_populate want to access the device service provided
> by early probe.
> >
> > Probably it's more safe to leave as it is unless you can double
> > confirm there're no such code depends on accessing early probed
> > devices as follows before we can make the change.
> 
> This has been boot tested on 6Q-SDB/6UL-EVK/6SL-EVK board.
> For i.MX, I only see pinctrl driver use arch_initcall from the link,
> https://elixir.bootlin.com/linux/latest/ident/arch_initcall
> 
> From my boot test, the pinctrl driver probe will be delayed a bit, but I do not
> see issues.
> 

From what I saw, imx6q_1588_init() and imx6q_axi_init() will use syscon which is
registered with postcore_initcall. Without having syscon devices populated,
I wonder those calls may fall.
Can you double check it?

Regards
Dong Aisheng
Peng Fan March 18, 2019, 2:31 a.m. UTC | #4
> -----Original Message-----
> From: Aisheng Dong
> Sent: 2019年3月15日 21:39
> To: Peng Fan <peng.fan@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; Anson Huang <anson.huang@nxp.com>;
> arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Cc: van.freenix@gmail.com
> Subject: RE: [PATCH 1/2] ARM: imx: drop uneccessary
> of_platform_default_populate
> 
> [...]
> 
> > > Originally devices are registered in arch_initcall. Now it will be a
> > > bit later in arch_initcall_sync and this may cause a bit risk if the
> > > code under the default_populate want to access the device service
> > > provided
> > by early probe.
> > >
> > > Probably it's more safe to leave as it is unless you can double
> > > confirm there're no such code depends on accessing early probed
> > > devices as follows before we can make the change.
> >
> > This has been boot tested on 6Q-SDB/6UL-EVK/6SL-EVK board.
> > For i.MX, I only see pinctrl driver use arch_initcall from the link,
> > https://elixir.bootlin.com/linux/latest/ident/arch_initcall
> >
> > From my boot test, the pinctrl driver probe will be delayed a bit, but
> > I do not see issues.
> >
> 
> From what I saw, imx6q_1588_init() and imx6q_axi_init() will use syscon which
> is registered with postcore_initcall. Without having syscon devices populated,
> I wonder those calls may fall.
> Can you double check it?

I do not see failure in imx6q_1588_init by adding a return value check of
regmap_update_bits, But the imx6q_pm_init will find ocram device for
suspend usage, postpone the device population will make imx6q_pm_init
fail, so drop this patch and leave the code as it is.

Thanks,
Peng.

> 
> Regards
> Dong Aisheng
Shawn Guo March 21, 2019, 9:10 a.m. UTC | #5
On Wed, Mar 13, 2019 at 09:05:52AM +0000, Peng Fan wrote:
> "arch_initcall_sync(of_platform_default_populate_init);" could be

The code change looks good.  But can you please mention commit
44a7185c2ae6 ("of/platform: Add common method to populate default bus")
in the commit log, so that we will be more clear about the story.

Shawn

> used to populate the device tree, there is no need to call
> of_platform_default_populate in machine code.
> 
> Tested on i.MX6Q-SDB i.MX6SL-EVK i.MX6UL-EVK board.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c  | 2 --
>  arch/arm/mach-imx/mach-imx6sl.c | 2 --
>  arch/arm/mach-imx/mach-imx6sx.c | 2 --
>  arch/arm/mach-imx/mach-imx6ul.c | 1 -
>  4 files changed, 7 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 7d80a0ae723c..655398c20256 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -278,8 +278,6 @@ static void __init imx6q_init_machine(void)
>  
>  	imx6q_enet_phy_init();
>  
> -	of_platform_default_populate(NULL, NULL, parent);
> -
>  	imx_anatop_init();
>  	cpu_is_imx6q() ?  imx6q_pm_init() : imx6dl_pm_init();
>  	imx6q_1588_init();
> diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
> index 99be4225297a..9743bdbb68fa 100644
> --- a/arch/arm/mach-imx/mach-imx6sl.c
> +++ b/arch/arm/mach-imx/mach-imx6sl.c
> @@ -56,8 +56,6 @@ static void __init imx6sl_init_machine(void)
>  	if (parent == NULL)
>  		pr_warn("failed to initialize soc device\n");
>  
> -	of_platform_default_populate(NULL, NULL, parent);
> -
>  	if (cpu_is_imx6sl())
>  		imx6sl_fec_init();
>  	imx_anatop_init();
> diff --git a/arch/arm/mach-imx/mach-imx6sx.c b/arch/arm/mach-imx/mach-imx6sx.c
> index 7f52d9b1e8a4..19b9f2dd309e 100644
> --- a/arch/arm/mach-imx/mach-imx6sx.c
> +++ b/arch/arm/mach-imx/mach-imx6sx.c
> @@ -72,8 +72,6 @@ static void __init imx6sx_init_machine(void)
>  	if (parent == NULL)
>  		pr_warn("failed to initialize soc device\n");
>  
> -	of_platform_default_populate(NULL, NULL, parent);
> -
>  	imx6sx_enet_init();
>  	imx_anatop_init();
>  	imx6sx_pm_init();
> diff --git a/arch/arm/mach-imx/mach-imx6ul.c b/arch/arm/mach-imx/mach-imx6ul.c
> index 6cb8a22b617d..c57b9df791b1 100644
> --- a/arch/arm/mach-imx/mach-imx6ul.c
> +++ b/arch/arm/mach-imx/mach-imx6ul.c
> @@ -65,7 +65,6 @@ static void __init imx6ul_init_machine(void)
>  	if (parent == NULL)
>  		pr_warn("failed to initialize soc device\n");
>  
> -	of_platform_default_populate(NULL, NULL, parent);
>  	imx6ul_enet_init();
>  	imx_anatop_init();
>  	imx6ul_pm_init();
> -- 
> 2.16.4
>
Shawn Guo March 21, 2019, 9:15 a.m. UTC | #6
On Mon, Mar 18, 2019 at 02:31:34AM +0000, Peng Fan wrote:
> 
> 
> > -----Original Message-----
> > From: Aisheng Dong
> > Sent: 2019年3月15日 21:39
> > To: Peng Fan <peng.fan@nxp.com>; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > dl-linux-imx <linux-imx@nxp.com>; Anson Huang <anson.huang@nxp.com>;
> > arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org
> > Cc: van.freenix@gmail.com
> > Subject: RE: [PATCH 1/2] ARM: imx: drop uneccessary
> > of_platform_default_populate
> > 
> > [...]
> > 
> > > > Originally devices are registered in arch_initcall. Now it will be a
> > > > bit later in arch_initcall_sync and this may cause a bit risk if the
> > > > code under the default_populate want to access the device service
> > > > provided
> > > by early probe.
> > > >
> > > > Probably it's more safe to leave as it is unless you can double
> > > > confirm there're no such code depends on accessing early probed
> > > > devices as follows before we can make the change.
> > >
> > > This has been boot tested on 6Q-SDB/6UL-EVK/6SL-EVK board.
> > > For i.MX, I only see pinctrl driver use arch_initcall from the link,
> > > https://elixir.bootlin.com/linux/latest/ident/arch_initcall
> > >
> > > From my boot test, the pinctrl driver probe will be delayed a bit, but
> > > I do not see issues.
> > >
> > 
> > From what I saw, imx6q_1588_init() and imx6q_axi_init() will use syscon which
> > is registered with postcore_initcall. Without having syscon devices populated,
> > I wonder those calls may fall.
> > Can you double check it?
> 
> I do not see failure in imx6q_1588_init by adding a return value check of
> regmap_update_bits, But the imx6q_pm_init will find ocram device for
> suspend usage, postpone the device population will make imx6q_pm_init
> fail, so drop this patch and leave the code as it is.

Ah, okay.  Thanks for the careful review and the check, Aisheng, Peng.

Shawn

Patch
diff mbox series

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 7d80a0ae723c..655398c20256 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -278,8 +278,6 @@  static void __init imx6q_init_machine(void)
 
 	imx6q_enet_phy_init();
 
-	of_platform_default_populate(NULL, NULL, parent);
-
 	imx_anatop_init();
 	cpu_is_imx6q() ?  imx6q_pm_init() : imx6dl_pm_init();
 	imx6q_1588_init();
diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
index 99be4225297a..9743bdbb68fa 100644
--- a/arch/arm/mach-imx/mach-imx6sl.c
+++ b/arch/arm/mach-imx/mach-imx6sl.c
@@ -56,8 +56,6 @@  static void __init imx6sl_init_machine(void)
 	if (parent == NULL)
 		pr_warn("failed to initialize soc device\n");
 
-	of_platform_default_populate(NULL, NULL, parent);
-
 	if (cpu_is_imx6sl())
 		imx6sl_fec_init();
 	imx_anatop_init();
diff --git a/arch/arm/mach-imx/mach-imx6sx.c b/arch/arm/mach-imx/mach-imx6sx.c
index 7f52d9b1e8a4..19b9f2dd309e 100644
--- a/arch/arm/mach-imx/mach-imx6sx.c
+++ b/arch/arm/mach-imx/mach-imx6sx.c
@@ -72,8 +72,6 @@  static void __init imx6sx_init_machine(void)
 	if (parent == NULL)
 		pr_warn("failed to initialize soc device\n");
 
-	of_platform_default_populate(NULL, NULL, parent);
-
 	imx6sx_enet_init();
 	imx_anatop_init();
 	imx6sx_pm_init();
diff --git a/arch/arm/mach-imx/mach-imx6ul.c b/arch/arm/mach-imx/mach-imx6ul.c
index 6cb8a22b617d..c57b9df791b1 100644
--- a/arch/arm/mach-imx/mach-imx6ul.c
+++ b/arch/arm/mach-imx/mach-imx6ul.c
@@ -65,7 +65,6 @@  static void __init imx6ul_init_machine(void)
 	if (parent == NULL)
 		pr_warn("failed to initialize soc device\n");
 
-	of_platform_default_populate(NULL, NULL, parent);
 	imx6ul_enet_init();
 	imx_anatop_init();
 	imx6ul_pm_init();