| Message ID | 20240925220552.149551-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] soc: imx8m: Probe the SoC driver as platform driver | expand |
On Wed, Sep 25, 2024 at 3:06 PM Marek Vasut <marex@denx.de> wrote: > > With driver_async_probe=* on kernel command line, the following trace is > produced because on i.MX8M Plus hardware because the soc-imx8m.c driver > calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock > driver is not yet probed. This was not detected during regular testing > without driver_async_probe. > > Convert the SoC code to platform driver and instantiate a platform device > in its current device_initcall() to probe the platform driver. Rework > .soc_revision callback to always return valid error code and return SoC > revision via parameter. This way, if anything in the .soc_revision callback > return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get > retried later. I'm assuming you tested this patch and it works? Did you see any other issues with driver_async_probe=* ? Does this improve your probe/boot time? Some stats on that would be nice. > > " > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 1 at drivers/soc/imx/soc-imx8m.c:115 imx8mm_soc_revision+0xdc/0x180 > CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-next-20240924-00002-g2062bb554dea #603 > Hardware name: DH electronics i.MX8M Plus DHCOM Premium Developer Kit (3) (DT) > pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : imx8mm_soc_revision+0xdc/0x180 > lr : imx8mm_soc_revision+0xd0/0x180 > sp : ffff8000821fbcc0 > x29: ffff8000821fbce0 x28: 0000000000000000 x27: ffff800081810120 > x26: ffff8000818a9970 x25: 0000000000000006 x24: 0000000000824311 > x23: ffff8000817f42c8 x22: ffff0000df8be210 x21: fffffffffffffdfb > x20: ffff800082780000 x19: 0000000000000001 x18: ffffffffffffffff > x17: ffff800081fff418 x16: ffff8000823e1000 x15: ffff0000c03b65e8 > x14: ffff0000c00051b0 x13: ffff800082790000 x12: 0000000000000801 > x11: ffff80008278ffff x10: ffff80008209d3a6 x9 : ffff80008062e95c > x8 : ffff8000821fb9a0 x7 : 0000000000000000 x6 : 00000000000080e3 > x5 : ffff0000df8c03d8 x4 : 0000000000000000 x3 : 0000000000000000 > x2 : 0000000000000000 x1 : fffffffffffffdfb x0 : fffffffffffffdfb > Call trace: > imx8mm_soc_revision+0xdc/0x180 > imx8_soc_init+0xb0/0x1e0 > do_one_initcall+0x94/0x1a8 > kernel_init_freeable+0x240/0x2a8 > kernel_init+0x28/0x140 > ret_from_fork+0x10/0x20 > ---[ end trace 0000000000000000 ]--- > SoC: i.MX8MP revision 1.1 > " > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Jeff Johnson <quic_jjohnson@quicinc.com> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Saravana Kannan <saravanak@google.com> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: imx@lists.linux.dev > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > V2: Use the platform device approach instead of late_initcall > --- > drivers/soc/imx/soc-imx8m.c | 92 ++++++++++++++++++++++++++++--------- > 1 file changed, 70 insertions(+), 22 deletions(-) > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c > index fe111bae38c8e..170970d4955c6 100644 > --- a/drivers/soc/imx/soc-imx8m.c > +++ b/drivers/soc/imx/soc-imx8m.c > @@ -30,7 +30,7 @@ > > struct imx8_soc_data { > char *name; > - u32 (*soc_revision)(void); > + int (*soc_revision)(u32 *socrev); > }; > > static u64 soc_uid; > @@ -51,24 +51,29 @@ static u32 imx8mq_soc_revision_from_atf(void) > static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; }; > #endif > > -static u32 __init imx8mq_soc_revision(void) > +static int imx8mq_soc_revision(u32 *socrev) > { > struct device_node *np; > void __iomem *ocotp_base; > u32 magic; > u32 rev; > struct clk *clk; > + int ret; > > np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); > if (!np) > - return 0; > + return -EINVAL; > > ocotp_base = of_iomap(np, 0); > - WARN_ON(!ocotp_base); > + if (!ocotp_base) { > + ret = -EINVAL; > + goto err_iomap; > + } > + > clk = of_clk_get_by_name(np, NULL); > if (IS_ERR(clk)) { > - WARN_ON(IS_ERR(clk)); > - return 0; > + ret = PTR_ERR(clk); > + goto err_clk; > } > > clk_prepare_enable(clk); > @@ -88,32 +93,45 @@ static u32 __init imx8mq_soc_revision(void) > soc_uid <<= 32; > soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); > > + *socrev = rev; > + > clk_disable_unprepare(clk); > clk_put(clk); > iounmap(ocotp_base); > of_node_put(np); > > - return rev; > + return 0; > + > +err_clk: > + iounmap(ocotp_base); > +err_iomap: > + of_node_put(np); > + return ret; > } > > -static void __init imx8mm_soc_uid(void) > +static int imx8mm_soc_uid(void) > { > void __iomem *ocotp_base; > struct device_node *np; > struct clk *clk; > + int ret = 0; > u32 offset = of_machine_is_compatible("fsl,imx8mp") ? > IMX8MP_OCOTP_UID_OFFSET : 0; > > np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-ocotp"); > if (!np) > - return; > + return -EINVAL; > > ocotp_base = of_iomap(np, 0); > - WARN_ON(!ocotp_base); > + if (!ocotp_base) { > + ret = -EINVAL; > + goto err_iomap; > + } > + > clk = of_clk_get_by_name(np, NULL); > if (IS_ERR(clk)) { > - WARN_ON(IS_ERR(clk)); > - return; > + ret = PTR_ERR(clk); > + goto err_clk; > } > > clk_prepare_enable(clk); > @@ -124,31 +142,41 @@ static void __init imx8mm_soc_uid(void) > > clk_disable_unprepare(clk); > clk_put(clk); > + > +err_clk: > iounmap(ocotp_base); > +err_iomap: > of_node_put(np); > + > + return ret; > } > > -static u32 __init imx8mm_soc_revision(void) > +static int imx8mm_soc_revision(u32 *socrev) > { > struct device_node *np; > void __iomem *anatop_base; > - u32 rev; > + int ret; > > np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop"); > if (!np) > - return 0; > + return -EINVAL; > > anatop_base = of_iomap(np, 0); > - WARN_ON(!anatop_base); > + if (!anatop_base) { > + ret = -EINVAL; > + goto err_iomap; > + } > > - rev = readl_relaxed(anatop_base + ANADIG_DIGPROG_IMX8MM); > + *socrev = readl_relaxed(anatop_base + ANADIG_DIGPROG_IMX8MM); > > iounmap(anatop_base); > of_node_put(np); > > - imx8mm_soc_uid(); > + return imx8mm_soc_uid(); > > - return rev; > +err_iomap: > + of_node_put(np); > + return ret; > } > > static const struct imx8_soc_data imx8mq_soc_data = { > @@ -184,7 +212,7 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = { > kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev & 0xf) : \ > "unknown" > > -static int __init imx8_soc_init(void) > +static int imx8m_soc_probe(struct platform_device *pdev) > { > struct soc_device_attribute *soc_dev_attr; > struct soc_device *soc_dev; > @@ -212,8 +240,11 @@ static int __init imx8_soc_init(void) > data = id->data; > if (data) { > soc_dev_attr->soc_id = data->name; > - if (data->soc_revision) > - soc_rev = data->soc_revision(); > + if (data->soc_revision) { > + ret = data->soc_revision(&soc_rev); > + if (ret) > + goto free_soc; > + } > } > > soc_dev_attr->revision = imx8_revision(soc_rev); > @@ -251,6 +282,23 @@ static int __init imx8_soc_init(void) > kfree(soc_dev_attr); > return ret; > } > + > +static struct platform_driver imx8m_soc_driver = { > + .probe = imx8m_soc_probe, > + .driver = { > + .name = "imx8m-soc", > + }, > +}; > +module_platform_driver(imx8m_soc_driver); This just translates to a module_init() when compiled as a module. > > + > +static int __init imx8_soc_init(void) > +{ > + struct platform_device *pdev; > + > + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); > + > + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; > +} > device_initcall(imx8_soc_init); This also translates to a module_init() when compiled as a module. I've never seen a module with two module_init()s and I'm pretty sure that doesn't work. I'm guessing this driver doesn't support tristate in its current state. But with this patch, it should work as a module too. Why not add support for that too? Why not just do both of these things in one initcall? platform_create_bundle() doesn't work with deferred probing though. So just do one initcall that adds the device and registers the platform driver. -Saravana > MODULE_DESCRIPTION("NXP i.MX8M SoC driver"); > MODULE_LICENSE("GPL"); > -- > 2.45.2 >
On Wed, Sep 25, 2024, at 22:04, Marek Vasut wrote: > With driver_async_probe=* on kernel command line, the following trace is > produced because on i.MX8M Plus hardware because the soc-imx8m.c driver > calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock > driver is not yet probed. This was not detected during regular testing > without driver_async_probe. > > Convert the SoC code to platform driver and instantiate a platform device > in its current device_initcall() to probe the platform driver. Rework > .soc_revision callback to always return valid error code and return SoC > revision via parameter. This way, if anything in the .soc_revision callback > return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get > retried later. Thanks for the new version, that was quick! > +static struct platform_driver imx8m_soc_driver = { > + .probe = imx8m_soc_probe, > + .driver = { > + .name = "imx8m-soc", > + }, > +}; > +module_platform_driver(imx8m_soc_driver); > + > +static int __init imx8_soc_init(void) > +{ > + struct platform_device *pdev; > + > + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); > + > + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; > +} > device_initcall(imx8_soc_init); Did you run into problems with the method I suggested first? I don't like the way that this version still registers both the device and driver regardless of the hardware it runs on, I'd prefer to leave the platform check in the initcall and only register them if we are actually on an imx8 machine. Having two initcalls also makes it impossible to build this as a loadable module, which is why I suggested platform_create_bundle(). I think you can keep the of_device_id lookup and pass the imx8_soc_data pointer as the platform_data to platform_create_bundle. Arnd
On Wed, Sep 25, 2024 at 11:28 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Sep 25, 2024, at 22:04, Marek Vasut wrote: > > With driver_async_probe=* on kernel command line, the following trace is > > produced because on i.MX8M Plus hardware because the soc-imx8m.c driver > > calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock > > driver is not yet probed. This was not detected during regular testing > > without driver_async_probe. > > > > Convert the SoC code to platform driver and instantiate a platform device > > in its current device_initcall() to probe the platform driver. Rework > > .soc_revision callback to always return valid error code and return SoC > > revision via parameter. This way, if anything in the .soc_revision callback > > return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get > > retried later. > > Thanks for the new version, that was quick! > > > +static struct platform_driver imx8m_soc_driver = { > > + .probe = imx8m_soc_probe, > > + .driver = { > > + .name = "imx8m-soc", > > + }, > > +}; > > +module_platform_driver(imx8m_soc_driver); > > + > > +static int __init imx8_soc_init(void) > > +{ > > + struct platform_device *pdev; > > + > > + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); > > + > > + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; > > +} > > device_initcall(imx8_soc_init); > > Did you run into problems with the method I suggested first? > > I don't like the way that this version still registers both the > device and driver regardless of the hardware it runs on, I'd > prefer to leave the platform check in the initcall and > only register them if we are actually on an imx8 machine. > > Having two initcalls also makes it impossible to build this > as a loadable module, which is why I suggested > platform_create_bundle(). That particular helper doesn't seem to like probe deferral because it allows for some of the code to be in init sections. So, it gets dropped after module load is complete. So, Marek will still have to register the device and driver separately. But I agree that it might be better to NOT register the device and driver if the basic conditions aren't met. -Saravana > I think you can keep the > of_device_id lookup and pass the imx8_soc_data pointer > as the platform_data to platform_create_bundle. > > Arnd
On 9/26/24 7:36 PM, Saravana Kannan wrote: > On Wed, Sep 25, 2024 at 11:28 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Wed, Sep 25, 2024, at 22:04, Marek Vasut wrote: >>> With driver_async_probe=* on kernel command line, the following trace is >>> produced because on i.MX8M Plus hardware because the soc-imx8m.c driver >>> calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock >>> driver is not yet probed. This was not detected during regular testing >>> without driver_async_probe. >>> >>> Convert the SoC code to platform driver and instantiate a platform device >>> in its current device_initcall() to probe the platform driver. Rework >>> .soc_revision callback to always return valid error code and return SoC >>> revision via parameter. This way, if anything in the .soc_revision callback >>> return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get >>> retried later. >> >> Thanks for the new version, that was quick! >> >>> +static struct platform_driver imx8m_soc_driver = { >>> + .probe = imx8m_soc_probe, >>> + .driver = { >>> + .name = "imx8m-soc", >>> + }, >>> +}; >>> +module_platform_driver(imx8m_soc_driver); >>> + >>> +static int __init imx8_soc_init(void) >>> +{ >>> + struct platform_device *pdev; >>> + >>> + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); >>> + >>> + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; >>> +} >>> device_initcall(imx8_soc_init); >> >> Did you run into problems with the method I suggested first? >> >> I don't like the way that this version still registers both the >> device and driver regardless of the hardware it runs on, I'd >> prefer to leave the platform check in the initcall and >> only register them if we are actually on an imx8 machine. >> >> Having two initcalls also makes it impossible to build this >> as a loadable module, which is why I suggested >> platform_create_bundle(). > > That particular helper doesn't seem to like probe deferral because it > allows for some of the code to be in init sections. So, it gets > dropped after module load is complete. So, Marek will still have to > register the device and driver separately. But I agree that it might > be better to NOT register the device and driver if the basic > conditions aren't met. Fixed in V3.
On 9/26/24 1:03 AM, Saravana Kannan wrote: > On Wed, Sep 25, 2024 at 3:06 PM Marek Vasut <marex@denx.de> wrote: >> >> With driver_async_probe=* on kernel command line, the following trace is >> produced because on i.MX8M Plus hardware because the soc-imx8m.c driver >> calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock >> driver is not yet probed. This was not detected during regular testing >> without driver_async_probe. >> >> Convert the SoC code to platform driver and instantiate a platform device >> in its current device_initcall() to probe the platform driver. Rework >> .soc_revision callback to always return valid error code and return SoC >> revision via parameter. This way, if anything in the .soc_revision callback >> return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get >> retried later. > > I'm assuming you tested this patch and it works? I tested it both with and without driver_async_probe=* > Did you see any other > issues with driver_async_probe=* ? Nope, just this one. > Does this improve your probe/boot time? Some stats on that would be nice. It does improve the boot time from 4.5 to 2.9 seconds (measured by looking at the kernel log, so imprecise, but noticeable). With 'quiet' on kernel command line, boot time drops from 2.99s to 2.34s . >> " >> ------------[ cut here ]------------ >> WARNING: CPU: 1 PID: 1 at drivers/soc/imx/soc-imx8m.c:115 imx8mm_soc_revision+0xdc/0x180 >> CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-next-20240924-00002-g2062bb554dea #603 >> Hardware name: DH electronics i.MX8M Plus DHCOM Premium Developer Kit (3) (DT) >> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> pc : imx8mm_soc_revision+0xdc/0x180 >> lr : imx8mm_soc_revision+0xd0/0x180 >> sp : ffff8000821fbcc0 >> x29: ffff8000821fbce0 x28: 0000000000000000 x27: ffff800081810120 >> x26: ffff8000818a9970 x25: 0000000000000006 x24: 0000000000824311 >> x23: ffff8000817f42c8 x22: ffff0000df8be210 x21: fffffffffffffdfb >> x20: ffff800082780000 x19: 0000000000000001 x18: ffffffffffffffff >> x17: ffff800081fff418 x16: ffff8000823e1000 x15: ffff0000c03b65e8 >> x14: ffff0000c00051b0 x13: ffff800082790000 x12: 0000000000000801 >> x11: ffff80008278ffff x10: ffff80008209d3a6 x9 : ffff80008062e95c >> x8 : ffff8000821fb9a0 x7 : 0000000000000000 x6 : 00000000000080e3 >> x5 : ffff0000df8c03d8 x4 : 0000000000000000 x3 : 0000000000000000 >> x2 : 0000000000000000 x1 : fffffffffffffdfb x0 : fffffffffffffdfb >> Call trace: >> imx8mm_soc_revision+0xdc/0x180 >> imx8_soc_init+0xb0/0x1e0 >> do_one_initcall+0x94/0x1a8 >> kernel_init_freeable+0x240/0x2a8 >> kernel_init+0x28/0x140 >> ret_from_fork+0x10/0x20 >> ---[ end trace 0000000000000000 ]--- >> SoC: i.MX8MP revision 1.1 >> " [...] >> +static struct platform_driver imx8m_soc_driver = { >> + .probe = imx8m_soc_probe, >> + .driver = { >> + .name = "imx8m-soc", >> + }, >> +}; >> +module_platform_driver(imx8m_soc_driver); > > This just translates to a module_init() when compiled as a module. > >> >> + >> +static int __init imx8_soc_init(void) >> +{ >> + struct platform_device *pdev; >> + >> + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); >> + >> + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; >> +} >> device_initcall(imx8_soc_init); > > This also translates to a module_init() when compiled as a module. > > I've never seen a module with two module_init()s and I'm pretty sure > that doesn't work. I'm guessing this driver doesn't support tristate > in its current state. But with this patch, it should work as a module > too. Why not add support for that too? I am not entirely sure whether there are no dependencies on this soc driver, so let's start with builtin . > Why not just do both of these things in one initcall? > platform_create_bundle() doesn't work with deferred probing though. So > just do one initcall that adds the device and registers the platform > driver. Fixed in V3. [...]
Hi Marek, kernel test robot noticed the following build errors: [auto build test ERROR on shawnguo/for-next] [also build test ERROR on soc/for-next linus/master v6.11 next-20240927] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/soc-imx8m-Probe-the-SoC-driver-as-platform-driver/20240926-060831 base: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next patch link: https://lore.kernel.org/r/20240925220552.149551-1-marex%40denx.de patch subject: [PATCH v2] soc: imx8m: Probe the SoC driver as platform driver config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20240928/202409281619.4SHXVZe7-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240928/202409281619.4SHXVZe7-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409281619.4SHXVZe7-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/soc/imx/soc-imx8m.c:302:1: error: redefinition of '__inittest' 302 | device_initcall(imx8_soc_init); | ^ include/linux/module.h:122:30: note: expanded from macro 'device_initcall' 122 | #define device_initcall(fn) module_init(fn) | ^ include/linux/module.h:131:42: note: expanded from macro 'module_init' 131 | static inline initcall_t __maybe_unused __inittest(void) \ | ^ drivers/soc/imx/soc-imx8m.c:292:1: note: previous definition is here 292 | module_platform_driver(imx8m_soc_driver); | ^ include/linux/platform_device.h:302:2: note: expanded from macro 'module_platform_driver' 302 | module_driver(__platform_driver, platform_driver_register, \ | ^ include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver' 261 | } \ | ^ include/linux/module.h:131:42: note: expanded from macro '\ module_init' 131 | static inline initcall_t __maybe_unused __inittest(void) \ | ^ >> drivers/soc/imx/soc-imx8m.c:302:1: error: redefinition of 'init_module' 302 | device_initcall(imx8_soc_init); | ^ include/linux/module.h:122:30: note: expanded from macro 'device_initcall' 122 | #define device_initcall(fn) module_init(fn) | ^ include/linux/module.h:133:6: note: expanded from macro 'module_init' 133 | int init_module(void) __copy(initfn) \ | ^ drivers/soc/imx/soc-imx8m.c:292:1: note: previous definition is here 292 | module_platform_driver(imx8m_soc_driver); | ^ include/linux/platform_device.h:302:2: note: expanded from macro 'module_platform_driver' 302 | module_driver(__platform_driver, platform_driver_register, \ | ^ include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver' 261 | } \ | ^ include/linux/module.h:133:6: note: expanded from macro '\ module_init' 133 | int init_module(void) __copy(initfn) \ | ^ 2 errors generated. vim +/__inittest +302 drivers/soc/imx/soc-imx8m.c d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 293 d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 294 static int __init imx8_soc_init(void) d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 295 { d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 296 struct platform_device *pdev; d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 297 d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 298 pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 299 d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 300 return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; d817f7a9bde892 drivers/soc/imx/soc-imx8m.c Marek Vasut 2024-09-26 301 } a7e26f356ca129 drivers/soc/imx/soc-imx8.c Abel Vesa 2019-03-22 @302 device_initcall(imx8_soc_init);
diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c index fe111bae38c8e..170970d4955c6 100644 --- a/drivers/soc/imx/soc-imx8m.c +++ b/drivers/soc/imx/soc-imx8m.c @@ -30,7 +30,7 @@ struct imx8_soc_data { char *name; - u32 (*soc_revision)(void); + int (*soc_revision)(u32 *socrev); }; static u64 soc_uid; @@ -51,24 +51,29 @@ static u32 imx8mq_soc_revision_from_atf(void) static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; }; #endif -static u32 __init imx8mq_soc_revision(void) +static int imx8mq_soc_revision(u32 *socrev) { struct device_node *np; void __iomem *ocotp_base; u32 magic; u32 rev; struct clk *clk; + int ret; np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); if (!np) - return 0; + return -EINVAL; ocotp_base = of_iomap(np, 0); - WARN_ON(!ocotp_base); + if (!ocotp_base) { + ret = -EINVAL; + goto err_iomap; + } + clk = of_clk_get_by_name(np, NULL); if (IS_ERR(clk)) { - WARN_ON(IS_ERR(clk)); - return 0; + ret = PTR_ERR(clk); + goto err_clk; } clk_prepare_enable(clk); @@ -88,32 +93,45 @@ static u32 __init imx8mq_soc_revision(void) soc_uid <<= 32; soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); + *socrev = rev; + clk_disable_unprepare(clk); clk_put(clk); iounmap(ocotp_base); of_node_put(np); - return rev; + return 0; + +err_clk: + iounmap(ocotp_base); +err_iomap: + of_node_put(np); + return ret; } -static void __init imx8mm_soc_uid(void) +static int imx8mm_soc_uid(void) { void __iomem *ocotp_base; struct device_node *np; struct clk *clk; + int ret = 0; u32 offset = of_machine_is_compatible("fsl,imx8mp") ? IMX8MP_OCOTP_UID_OFFSET : 0; np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-ocotp"); if (!np) - return; + return -EINVAL; ocotp_base = of_iomap(np, 0); - WARN_ON(!ocotp_base); + if (!ocotp_base) { + ret = -EINVAL; + goto err_iomap; + } + clk = of_clk_get_by_name(np, NULL); if (IS_ERR(clk)) { - WARN_ON(IS_ERR(clk)); - return; + ret = PTR_ERR(clk); + goto err_clk; } clk_prepare_enable(clk); @@ -124,31 +142,41 @@ static void __init imx8mm_soc_uid(void) clk_disable_unprepare(clk); clk_put(clk); + +err_clk: iounmap(ocotp_base); +err_iomap: of_node_put(np); + + return ret; } -static u32 __init imx8mm_soc_revision(void) +static int imx8mm_soc_revision(u32 *socrev) { struct device_node *np; void __iomem *anatop_base; - u32 rev; + int ret; np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop"); if (!np) - return 0; + return -EINVAL; anatop_base = of_iomap(np, 0); - WARN_ON(!anatop_base); + if (!anatop_base) { + ret = -EINVAL; + goto err_iomap; + } - rev = readl_relaxed(anatop_base + ANADIG_DIGPROG_IMX8MM); + *socrev = readl_relaxed(anatop_base + ANADIG_DIGPROG_IMX8MM); iounmap(anatop_base); of_node_put(np); - imx8mm_soc_uid(); + return imx8mm_soc_uid(); - return rev; +err_iomap: + of_node_put(np); + return ret; } static const struct imx8_soc_data imx8mq_soc_data = { @@ -184,7 +212,7 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = { kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev & 0xf) : \ "unknown" -static int __init imx8_soc_init(void) +static int imx8m_soc_probe(struct platform_device *pdev) { struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; @@ -212,8 +240,11 @@ static int __init imx8_soc_init(void) data = id->data; if (data) { soc_dev_attr->soc_id = data->name; - if (data->soc_revision) - soc_rev = data->soc_revision(); + if (data->soc_revision) { + ret = data->soc_revision(&soc_rev); + if (ret) + goto free_soc; + } } soc_dev_attr->revision = imx8_revision(soc_rev); @@ -251,6 +282,23 @@ static int __init imx8_soc_init(void) kfree(soc_dev_attr); return ret; } + +static struct platform_driver imx8m_soc_driver = { + .probe = imx8m_soc_probe, + .driver = { + .name = "imx8m-soc", + }, +}; +module_platform_driver(imx8m_soc_driver); + +static int __init imx8_soc_init(void) +{ + struct platform_device *pdev; + + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); + + return IS_ERR(pdev) ? PTR_ERR(pdev) : 0; +} device_initcall(imx8_soc_init); MODULE_DESCRIPTION("NXP i.MX8M SoC driver"); MODULE_LICENSE("GPL");
With driver_async_probe=* on kernel command line, the following trace is produced because on i.MX8M Plus hardware because the soc-imx8m.c driver calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock driver is not yet probed. This was not detected during regular testing without driver_async_probe. Convert the SoC code to platform driver and instantiate a platform device in its current device_initcall() to probe the platform driver. Rework .soc_revision callback to always return valid error code and return SoC revision via parameter. This way, if anything in the .soc_revision callback return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get retried later. " ------------[ cut here ]------------ WARNING: CPU: 1 PID: 1 at drivers/soc/imx/soc-imx8m.c:115 imx8mm_soc_revision+0xdc/0x180 CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-next-20240924-00002-g2062bb554dea #603 Hardware name: DH electronics i.MX8M Plus DHCOM Premium Developer Kit (3) (DT) pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : imx8mm_soc_revision+0xdc/0x180 lr : imx8mm_soc_revision+0xd0/0x180 sp : ffff8000821fbcc0 x29: ffff8000821fbce0 x28: 0000000000000000 x27: ffff800081810120 x26: ffff8000818a9970 x25: 0000000000000006 x24: 0000000000824311 x23: ffff8000817f42c8 x22: ffff0000df8be210 x21: fffffffffffffdfb x20: ffff800082780000 x19: 0000000000000001 x18: ffffffffffffffff x17: ffff800081fff418 x16: ffff8000823e1000 x15: ffff0000c03b65e8 x14: ffff0000c00051b0 x13: ffff800082790000 x12: 0000000000000801 x11: ffff80008278ffff x10: ffff80008209d3a6 x9 : ffff80008062e95c x8 : ffff8000821fb9a0 x7 : 0000000000000000 x6 : 00000000000080e3 x5 : ffff0000df8c03d8 x4 : 0000000000000000 x3 : 0000000000000000 x2 : 0000000000000000 x1 : fffffffffffffdfb x0 : fffffffffffffdfb Call trace: imx8mm_soc_revision+0xdc/0x180 imx8_soc_init+0xb0/0x1e0 do_one_initcall+0x94/0x1a8 kernel_init_freeable+0x240/0x2a8 kernel_init+0x28/0x140 ret_from_fork+0x10/0x20 ---[ end trace 0000000000000000 ]--- SoC: i.MX8MP revision 1.1 " Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Fabio Estevam <festevam@gmail.com> Cc: Jeff Johnson <quic_jjohnson@quicinc.com> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Saravana Kannan <saravanak@google.com> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Shawn Guo <shawnguo@kernel.org> Cc: imx@lists.linux.dev Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- V2: Use the platform device approach instead of late_initcall --- drivers/soc/imx/soc-imx8m.c | 92 ++++++++++++++++++++++++++++--------- 1 file changed, 70 insertions(+), 22 deletions(-)