| Message ID | 20190313091757.16410-1-peng.fan@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ARM: imx: drop uneccessary of_platform_default_populate | expand |
> 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
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
[...] > > 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
> -----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
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 >
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
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();
"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(-)