[PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[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);
-
 	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

[PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

[Peng Fan]

Follow other i.MX6/7 machince code to check return value
of imx_soc_device_init and warn when fail.

Also drop of_platform_default_populate, because
"arch_initcall_sync(of_platform_default_populate_init);" could be
used to populate the device tree.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/mach-imx/mach-imx7ulp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/mach-imx7ulp.c b/arch/arm/mach-imx/mach-imx7ulp.c
index 11ac71aaf965..6d823f05d9aa 100644
--- a/arch/arm/mach-imx/mach-imx7ulp.c
+++ b/arch/arm/mach-imx/mach-imx7ulp.c
@@ -53,11 +53,16 @@ static void __init imx7ulp_set_revision(void)
 
 static void __init imx7ulp_init_machine(void)
 {
+	struct device *soc_dev;
+
+	soc_dev = imx_soc_device_init();
+	if (soc_dev == NULL)
+		pr_warn("failed to initialize soc device\n");
+
 	imx7ulp_pm_init();
 
 	mxc_set_cpu_type(MXC_CPU_IMX7ULP);
 	imx7ulp_set_revision();
-	of_platform_default_populate(NULL, NULL, imx_soc_device_init());
 }
 
 static const char *const imx7ulp_dt_compat[] __initconst = {
-- 
2.16.4

RE: [PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[Aisheng Dong]

> 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

RE: [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

[Aisheng Dong]

> From: Peng Fan
>
> [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

ARM: imx: imx7ulp: ...

> Follow other i.MX6/7 machince code to check return value of
> imx_soc_device_init and warn when fail.
> 
> Also drop of_platform_default_populate, because
> "arch_initcall_sync(of_platform_default_populate_init);" could be used to
> populate the device tree.
> 

This could be in separate patch.

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/mach-imx/mach-imx7ulp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx7ulp.c
> b/arch/arm/mach-imx/mach-imx7ulp.c
> index 11ac71aaf965..6d823f05d9aa 100644
> --- a/arch/arm/mach-imx/mach-imx7ulp.c
> +++ b/arch/arm/mach-imx/mach-imx7ulp.c
> @@ -53,11 +53,16 @@ static void __init imx7ulp_set_revision(void)
> 
>  static void __init imx7ulp_init_machine(void)  {
> +	struct device *soc_dev;
> +
> +	soc_dev = imx_soc_device_init();
> +	if (soc_dev == NULL)
> +		pr_warn("failed to initialize soc device\n");
> +

Should this be under set revision?

Regards
Dong Aisheng

>  	imx7ulp_pm_init();
> 
>  	mxc_set_cpu_type(MXC_CPU_IMX7ULP);
>  	imx7ulp_set_revision();
> -	of_platform_default_populate(NULL, NULL, imx_soc_device_init());
>  }
> 
>  static const char *const imx7ulp_dt_compat[] __initconst = {
> --
> 2.16.4

RE: [PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[Peng Fan]

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

RE: [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

[Peng Fan]

> -----Original Message-----
> From: Aisheng Dong
> Sent: 2019年3月15日 18:22
> 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 2/2] ARM: imx: mach-imx7ulp: warn when
> imx_soc_device_init fail
> 
> > From: Peng Fan
> >
> > [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail
> 
> ARM: imx: imx7ulp: ...

Ok.
> 
> > Follow other i.MX6/7 machince code to check return value of
> > imx_soc_device_init and warn when fail.
> >
> > Also drop of_platform_default_populate, because
> > "arch_initcall_sync(of_platform_default_populate_init);" could be used
> > to populate the device tree.
> >
> 
> This could be in separate patch.

I'll do it in v2 after we agree the change in the first patch.

> 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/arm/mach-imx/mach-imx7ulp.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-imx/mach-imx7ulp.c
> > b/arch/arm/mach-imx/mach-imx7ulp.c
> > index 11ac71aaf965..6d823f05d9aa 100644
> > --- a/arch/arm/mach-imx/mach-imx7ulp.c
> > +++ b/arch/arm/mach-imx/mach-imx7ulp.c
> > @@ -53,11 +53,16 @@ static void __init imx7ulp_set_revision(void)
> >
> >  static void __init imx7ulp_init_machine(void)  {
> > +	struct device *soc_dev;
> > +
> > +	soc_dev = imx_soc_device_init();
> > +	if (soc_dev == NULL)
> > +		pr_warn("failed to initialize soc device\n");
> > +
> 
> Should this be under set revision?

Just follow other i.MX6/7 platforms practice, I could move
this under set revision in v2 if you prefer.

Thanks,
Peng.

> 
> Regards
> Dong Aisheng
> 
> >  	imx7ulp_pm_init();
> >
> >  	mxc_set_cpu_type(MXC_CPU_IMX7ULP);
> >  	imx7ulp_set_revision();
> > -	of_platform_default_populate(NULL, NULL, imx_soc_device_init());
> >  }
> >
> >  static const char *const imx7ulp_dt_compat[] __initconst = {
> > --
> > 2.16.4

RE: [PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[Aisheng Dong]

[...]

> > 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

RE: [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

[Aisheng Dong]

[...]

> > > Follow other i.MX6/7 machince code to check return value of
> > > imx_soc_device_init and warn when fail.
> > >
> > > Also drop of_platform_default_populate, because
> > > "arch_initcall_sync(of_platform_default_populate_init);" could be
> > > used to populate the device tree.
> > >
> >
> > This could be in separate patch.
> 
> I'll do it in v2 after we agree the change in the first patch.
> 

I think imx7ulp does not have the issue in patch 1.

Regards
Dong Aisheng

> >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  arch/arm/mach-imx/mach-imx7ulp.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/mach-imx/mach-imx7ulp.c
> > > b/arch/arm/mach-imx/mach-imx7ulp.c
> > > index 11ac71aaf965..6d823f05d9aa 100644
> > > --- a/arch/arm/mach-imx/mach-imx7ulp.c
> > > +++ b/arch/arm/mach-imx/mach-imx7ulp.c
> > > @@ -53,11 +53,16 @@ static void __init imx7ulp_set_revision(void)
> > >
> > >  static void __init imx7ulp_init_machine(void)  {
> > > +	struct device *soc_dev;
> > > +
> > > +	soc_dev = imx_soc_device_init();
> > > +	if (soc_dev == NULL)
> > > +		pr_warn("failed to initialize soc device\n");
> > > +
> >
> > Should this be under set revision?
> 
> Just follow other i.MX6/7 platforms practice, I could move this under set
> revision in v2 if you prefer.
> 
> Thanks,
> Peng.
> 
> >
> > Regards
> > Dong Aisheng
> >
> > >  	imx7ulp_pm_init();
> > >
> > >  	mxc_set_cpu_type(MXC_CPU_IMX7ULP);
> > >  	imx7ulp_set_revision();
> > > -	of_platform_default_populate(NULL, NULL, imx_soc_device_init());
> > >  }
> > >
> > >  static const char *const imx7ulp_dt_compat[] __initconst = {
> > > --
> > > 2.16.4

RE: [PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[Peng Fan]

> -----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

RE: [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

[Peng Fan]

> -----Original Message-----
> From: Aisheng Dong
> Sent: 2019年3月15日 21:40
> 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 2/2] ARM: imx: mach-imx7ulp: warn when
> imx_soc_device_init fail
> 
> [...]
> 
> > > > Follow other i.MX6/7 machince code to check return value of
> > > > imx_soc_device_init and warn when fail.
> > > >
> > > > Also drop of_platform_default_populate, because
> > > > "arch_initcall_sync(of_platform_default_populate_init);" could be
> > > > used to populate the device tree.
> > > >
> > >
> > > This could be in separate patch.
> >
> > I'll do it in v2 after we agree the change in the first patch.
> >
> 
> I think imx7ulp does not have the issue in patch 1.

Missed to reply you.
So do you agree to split this patch into two
patches ? or leave the code as it is?

The first is follow other i.mx6/7 to check return value of
imx_soc_device_init. 
The second is drop the call to of_platform_default_populate.

Thanks,
Peng

> 
> Regards
> Dong Aisheng
> 
> > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  arch/arm/mach-imx/mach-imx7ulp.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm/mach-imx/mach-imx7ulp.c
> > > > b/arch/arm/mach-imx/mach-imx7ulp.c
> > > > index 11ac71aaf965..6d823f05d9aa 100644
> > > > --- a/arch/arm/mach-imx/mach-imx7ulp.c
> > > > +++ b/arch/arm/mach-imx/mach-imx7ulp.c
> > > > @@ -53,11 +53,16 @@ static void __init imx7ulp_set_revision(void)
> > > >
> > > >  static void __init imx7ulp_init_machine(void)  {
> > > > +	struct device *soc_dev;
> > > > +
> > > > +	soc_dev = imx_soc_device_init();
> > > > +	if (soc_dev == NULL)
> > > > +		pr_warn("failed to initialize soc device\n");
> > > > +
> > >
> > > Should this be under set revision?
> >
> > Just follow other i.MX6/7 platforms practice, I could move this under
> > set revision in v2 if you prefer.
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Dong Aisheng
> > >
> > > >  	imx7ulp_pm_init();
> > > >
> > > >  	mxc_set_cpu_type(MXC_CPU_IMX7ULP);
> > > >  	imx7ulp_set_revision();
> > > > -	of_platform_default_populate(NULL, NULL, imx_soc_device_init());
> > > >  }
> > > >
> > > >  static const char *const imx7ulp_dt_compat[] __initconst = {
> > > > --
> > > > 2.16.4

RE: [PATCH 2/2] ARM: imx: mach-imx7ulp: warn when imx_soc_device_init fail

[Aisheng Dong]

> From: Peng Fan
>
> > > > > Follow other i.MX6/7 machince code to check return value of
> > > > > imx_soc_device_init and warn when fail.
> > > > >
> > > > > Also drop of_platform_default_populate, because
> > > > > "arch_initcall_sync(of_platform_default_populate_init);" could
> > > > > be used to populate the device tree.
> > > > >
> > > >
> > > > This could be in separate patch.
> > >
> > > I'll do it in v2 after we agree the change in the first patch.
> > >
> >
> > I think imx7ulp does not have the issue in patch 1.
> 
> Missed to reply you.
> So do you agree to split this patch into two patches ? or leave the code as it is?
> 

A bit more thinking, i wonder that warning may not be quite necessary
for a new patch. 
So probably leave as it is unless we have more strong reasons.
Sorry about that.

Regards
Dong Aisheng

> The first is follow other i.mx6/7 to check return value of imx_soc_device_init.
> The second is drop the call to of_platform_default_populate.
> 
> Thanks,
> Peng
> 
> >
> > Regards
> > Dong Aisheng
> >
> > > >
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >  arch/arm/mach-imx/mach-imx7ulp.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm/mach-imx/mach-imx7ulp.c
> > > > > b/arch/arm/mach-imx/mach-imx7ulp.c
> > > > > index 11ac71aaf965..6d823f05d9aa 100644
> > > > > --- a/arch/arm/mach-imx/mach-imx7ulp.c
> > > > > +++ b/arch/arm/mach-imx/mach-imx7ulp.c
> > > > > @@ -53,11 +53,16 @@ static void __init
> > > > > imx7ulp_set_revision(void)
> > > > >
> > > > >  static void __init imx7ulp_init_machine(void)  {
> > > > > +	struct device *soc_dev;
> > > > > +
> > > > > +	soc_dev = imx_soc_device_init();
> > > > > +	if (soc_dev == NULL)
> > > > > +		pr_warn("failed to initialize soc device\n");
> > > > > +
> > > >
> > > > Should this be under set revision?
> > >
> > > Just follow other i.MX6/7 platforms practice, I could move this
> > > under set revision in v2 if you prefer.
> > >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > Regards
> > > > Dong Aisheng
> > > >
> > > > >  	imx7ulp_pm_init();
> > > > >
> > > > >  	mxc_set_cpu_type(MXC_CPU_IMX7ULP);
> > > > >  	imx7ulp_set_revision();
> > > > > -	of_platform_default_populate(NULL, NULL, imx_soc_device_init());
> > > > >  }
> > > > >
> > > > >  static const char *const imx7ulp_dt_compat[] __initconst = {
> > > > > --
> > > > > 2.16.4

Re: [PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[Shawn Guo]

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
> 

Re: [PATCH 1/2] ARM: imx: drop uneccessary of_platform_default_populate

[Shawn Guo]

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