| Message ID | 20240926213729.2882045-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] soc: imx8m: Probe the SoC driver as platform driver | expand |
On Thu, Sep 26, 2024 at 2:37 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. > > " > ------------[ 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 > V3: Do not register imx8m_soc_driver on non-iMX8M hardware > --- > drivers/soc/imx/soc-imx8m.c | 107 ++++++++++++++++++++++++++++-------- > 1 file changed, 85 insertions(+), 22 deletions(-) > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c > index fe111bae38c8e..5ea8887828c06 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); Using devm_of_iomap() and scoped "whatever it's called" might help simplify the error handling. So something like this for np: struct device_node *np __free(device_node) = np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); And this for ocotp_base: ocotp_base = devm_of_iomap(dev, np, 0); Would mean you can delete all the error handling parts > - 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); If you use devm_of_iomap() then you should use devm_iounmap() here and not just delete this part. devm_* is mainly to skip freeing when probe fails. > 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; > + } > } I'm glad it's working for you, but I think there might still be a race that you are just lucky enough to not hit. I think you still need to fix up drivers/base/soc.c to return -EPROBE_DEFER when soc_device_match() is called but soc_bus_type has no devices registered. That way any drivers that try to use that API will defer probe until this device gets to probe. And then you'll have to look at all the callers of that API for the boards this driver is meant for and make sure they don't ignore the error return value. Just add a WARN() on the API to figure out all the callers in your board. Also, you might want to check that your list of probed devices doesn't change without any async probing or this patch vs with async probing and this patch. Quick way to get list of successfully probed devices is: # find /sys/devices -name driver Arnd, Why is soc_device_match() doing a bus_for_each_dev(&soc_bus_type,...)? Are the real use cases where more than one soc device can be registered with soc_device_register()? -Saravana > > soc_dev_attr->revision = imx8_revision(soc_rev); > @@ -251,6 +282,38 @@ 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", > + }, > +}; > + > +static int __init imx8_soc_init(void) > +{ > + struct platform_device *pdev; > + int ret; > + > + /* No match means this is non-i.MX8M hardware, do nothing. */ > + if (!of_match_node(imx8_soc_match, of_root)) > + return 0; > + > + ret = platform_driver_register(&imx8m_soc_driver); > + if (ret) { > + pr_err("Failed to register imx8m-soc platform driver: %d\n", ret); > + return ret; > + } > + > + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); > + if (IS_ERR(pdev)) { > + pr_err("Failed to register imx8m-soc platform device: %ld\n", PTR_ERR(pdev)); > + platform_driver_unregister(&imx8m_soc_driver); > + return PTR_ERR(pdev); > + } > + > + return 0; > +} > device_initcall(imx8_soc_init); > MODULE_DESCRIPTION("NXP i.MX8M SoC driver"); > MODULE_LICENSE("GPL"); > -- > 2.45.2 >
On Thu, Sep 26, 2024, at 23:39, Saravana Kannan wrote: > > Also, you might want to check that your list of probed devices doesn't > change without any async probing or this patch vs with async probing > and this patch. Quick way to get list of successfully probed devices > is: > # find /sys/devices -name driver > > Arnd, > > Why is soc_device_match() doing a bus_for_each_dev(&soc_bus_type,...)? > Are the real use cases where more than one soc device can be > registered with soc_device_register()? Anything can register a soc_device, and I think there is a case where both the actual SoC and a firmware driver each register one, see drivers/firmware/smccc/soc_id.c and drivers/firmware/imx/imx-scu-soc.c. Not sure how common this is, but this was something that people asked for when we created the interface. Arnd
On 9/27/24 1:39 AM, Saravana Kannan wrote: [...] >> +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); > > Using devm_of_iomap() and scoped "whatever it's called" might help > simplify the error handling. > > So something like this for np: > struct device_node *np __free(device_node) = np = > of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); > > And this for ocotp_base: > ocotp_base = devm_of_iomap(dev, np, 0); This would fail if OCOTP driver probes first and claims the memory area with request_mem_region() (or devm_request_mem_region(), used in __devm_ioremap_resource() which is called from devm_of_iomap()). I ran into this with ANATOP, which is the other iomap()d device here. The of_iomap() does not use request_mem_region(), so it can map the area. > Would mean you can delete all the error handling parts All right, let's do this in separate 3/3 patch , because the amount of changes in this one patch are growing to be too much and difficult to review. [...] >> @@ -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; >> + } >> } > > I'm glad it's working for you, but I think there might still be a race > that you are just lucky enough to not hit. I think you still need to > fix up drivers/base/soc.c to return -EPROBE_DEFER when > soc_device_match() is called but soc_bus_type has no devices > registered. That way any drivers that try to use that API will defer > probe until this device gets to probe. soc_device_match() returns a pointer to soc_device_attribute or NULL, do you have some other function in mind ? > And then you'll have to look at all the callers of that API for the > boards this driver is meant for and make sure they don't ignore the > error return value. Just add a WARN() on the API to figure out all the > callers in your board. > > Also, you might want to check that your list of probed devices doesn't > change without any async probing or this patch vs with async probing > and this patch. Quick way to get list of successfully probed devices > is: > # find /sys/devices -name driver It seems OK. [...]
On Fri, Sep 27, 2024 at 2:55 PM Marek Vasut <marex@denx.de> wrote: > > On 9/27/24 1:39 AM, Saravana Kannan wrote: > > [...] > > >> +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); > > > > Using devm_of_iomap() and scoped "whatever it's called" might help > > simplify the error handling. > > > > So something like this for np: > > struct device_node *np __free(device_node) = np = > > of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); > > > > And this for ocotp_base: > > ocotp_base = devm_of_iomap(dev, np, 0); > > This would fail if OCOTP driver probes first and claims the memory area > with request_mem_region() (or devm_request_mem_region(), used in > __devm_ioremap_resource() which is called from devm_of_iomap()). I ran > into this with ANATOP, which is the other iomap()d device here. The > of_iomap() does not use request_mem_region(), so it can map the area. I'll take your word for it. > > > Would mean you can delete all the error handling parts > > All right, let's do this in separate 3/3 patch , because the amount of > changes in this one patch are growing to be too much and difficult to > review. Sure. If you can't use devm, I don't care too much about just cleaning up "of_put()" error handling. Your call on whether you do a 3/3. > > [...] > > >> @@ -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; > >> + } > >> } > > > > I'm glad it's working for you, but I think there might still be a race > > that you are just lucky enough to not hit. I think you still need to > > fix up drivers/base/soc.c to return -EPROBE_DEFER when > > soc_device_match() is called but soc_bus_type has no devices > > registered. That way any drivers that try to use that API will defer > > probe until this device gets to probe. > > soc_device_match() returns a pointer to soc_device_attribute or NULL, do > you have some other function in mind ? No, I'm talking about the same function. I'm asking to change it to return ERR_PTR(-EPROBE_DEFER) instead of NULL if no soc device has been registered yet. And you'll also go change all the drivers that use that API and are on the IMX boards supported by this soc driver, to handle the -EPROBE_DEFER correctly. And this error will only get returned for boards that do async probing and using a platform device to register the soc device. So it's not going to break everyone if you do this change. > > > And then you'll have to look at all the callers of that API for the > > boards this driver is meant for and make sure they don't ignore the > > error return value. Just add a WARN() on the API to figure out all the > > callers in your board. > > > > Also, you might want to check that your list of probed devices doesn't > > change without any async probing or this patch vs with async probing > > and this patch. Quick way to get list of successfully probed devices > > is: > > # find /sys/devices -name driver > > It seems OK. Good to know. -Saravana > > [...]
On Fri, Sep 27, 2024, at 22:27, Saravana Kannan wrote: > On Fri, Sep 27, 2024 at 2:55 PM Marek Vasut <marex@denx.de> wrote: >> > >> > I'm glad it's working for you, but I think there might still be a race >> > that you are just lucky enough to not hit. I think you still need to >> > fix up drivers/base/soc.c to return -EPROBE_DEFER when >> > soc_device_match() is called but soc_bus_type has no devices >> > registered. That way any drivers that try to use that API will defer >> > probe until this device gets to probe. >> >> soc_device_match() returns a pointer to soc_device_attribute or NULL, do >> you have some other function in mind ? > > No, I'm talking about the same function. I'm asking to change it to > return ERR_PTR(-EPROBE_DEFER) instead > of NULL if no soc device has been registered yet. > > And you'll also go change all the drivers that use that API and are on > the IMX boards supported by this soc driver, to handle the > -EPROBE_DEFER correctly. > > And this error will only get returned for boards that do async probing > and using a platform device to register the soc device. So it's > not going to break everyone if you do this change. I don't like it when an interface can return both NULL an ERR_PTR(), that just makes it too easy to be used incorrectly. Changing it to always return ERR_PTR() on an error is still risky, e.g. when a new driver gets merged that did get the same treewide API change, so if we do that, this should probably also change either the function name or the prototype. Arnd
On 9/28/24 12:27 AM, Saravana Kannan wrote: [...] >>> I'm glad it's working for you, but I think there might still be a race >>> that you are just lucky enough to not hit. I think you still need to >>> fix up drivers/base/soc.c to return -EPROBE_DEFER when >>> soc_device_match() is called but soc_bus_type has no devices >>> registered. That way any drivers that try to use that API will defer >>> probe until this device gets to probe. >> >> soc_device_match() returns a pointer to soc_device_attribute or NULL, do >> you have some other function in mind ? > > No, I'm talking about the same function. I'm asking to change it to > return ERR_PTR(-EPROBE_DEFER) instead > of NULL if no soc device has been registered yet. > > And you'll also go change all the drivers that use that API and are on > the IMX boards supported by this soc driver, to handle the > -EPROBE_DEFER correctly. > > And this error will only get returned for boards that do async probing > and using a platform device to register the soc device. So it's > not going to break everyone if you do this change. It seems the imx8m has no users of this, so I created a local patch, but I'll send a V4 of this series first.
diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c index fe111bae38c8e..5ea8887828c06 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,38 @@ 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", + }, +}; + +static int __init imx8_soc_init(void) +{ + struct platform_device *pdev; + int ret; + + /* No match means this is non-i.MX8M hardware, do nothing. */ + if (!of_match_node(imx8_soc_match, of_root)) + return 0; + + ret = platform_driver_register(&imx8m_soc_driver); + if (ret) { + pr_err("Failed to register imx8m-soc platform driver: %d\n", ret); + return ret; + } + + pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0); + if (IS_ERR(pdev)) { + pr_err("Failed to register imx8m-soc platform device: %ld\n", PTR_ERR(pdev)); + platform_driver_unregister(&imx8m_soc_driver); + return PTR_ERR(pdev); + } + + return 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 V3: Do not register imx8m_soc_driver on non-iMX8M hardware --- drivers/soc/imx/soc-imx8m.c | 107 ++++++++++++++++++++++++++++-------- 1 file changed, 85 insertions(+), 22 deletions(-)