[PATCH] soc: imx8m: Probe the SoC driver late

[PATCH] soc: imx8m: Probe the SoC driver late

[Marek Vasut]

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.

Attempt to fix it by probing the SoC driver late, but I don't think that
is the correct approach here.

"
------------[ 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
---
 drivers/soc/imx/soc-imx8m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
index fe111bae38c8e..04d6ab1073b25 100644
--- a/drivers/soc/imx/soc-imx8m.c
+++ b/drivers/soc/imx/soc-imx8m.c
@@ -251,6 +251,6 @@ static int __init imx8_soc_init(void)
 	kfree(soc_dev_attr);
 	return ret;
 }
-device_initcall(imx8_soc_init);
+late_initcall(imx8_soc_init);
 MODULE_DESCRIPTION("NXP i.MX8M SoC driver");
 MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Arnd Bergmann]

On Wed, Sep 25, 2024, at 16:00, 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.
>
> Attempt to fix it by probing the SoC driver late, but I don't think that
> is the correct approach here.

I think the correct fix would be to propagate the -EPROBE_DEFER
and return that from imx8_soc_init(), so it gets retried again
after the clock driver.

      Arnd

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Marek Vasut]

On 9/25/24 6:04 PM, Arnd Bergmann wrote:
> On Wed, Sep 25, 2024, at 16:00, 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.
>>
>> Attempt to fix it by probing the SoC driver late, but I don't think that
>> is the correct approach here.
> 
> I think the correct fix would be to propagate the -EPROBE_DEFER
> and return that from imx8_soc_init(), so it gets retried again
> after the clock driver.
I already tried that, but if I return -EPROBE_DEFER from 
device_initcall, it doesn't get retriggered . I suspect EPROBE_DEFER 
works only for proper drivers ?

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Arnd Bergmann]

On Wed, Sep 25, 2024, at 16:09, Marek Vasut wrote:
> On 9/25/24 6:04 PM, Arnd Bergmann wrote:
>> On Wed, Sep 25, 2024, at 16:00, 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.
>>>
>>> Attempt to fix it by probing the SoC driver late, but I don't think that
>>> is the correct approach here.
>> 
>> I think the correct fix would be to propagate the -EPROBE_DEFER
>> and return that from imx8_soc_init(), so it gets retried again
>> after the clock driver.
> I already tried that, but if I return -EPROBE_DEFER from 
> device_initcall, it doesn't get retriggered . I suspect EPROBE_DEFER 
> works only for proper drivers ?

Right, of course. And unfortunately it can't just register to
the fsl,imx8mm-anatop/fsl,imx8mm-ocotp/... nodes because they
all have a driver already.

On the other hand, making it a late_initcall() defeats the
purpose of the driver because then it can't be used by other
drivers with soc_device_match(), resulting in incorrect
behavior when another driver relies on this to enable
a chip revision specific workaround.

     Arnd

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Marek Vasut]

On 9/25/24 6:31 PM, Arnd Bergmann wrote:
> On Wed, Sep 25, 2024, at 16:09, Marek Vasut wrote:
>> On 9/25/24 6:04 PM, Arnd Bergmann wrote:
>>> On Wed, Sep 25, 2024, at 16:00, 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.
>>>>
>>>> Attempt to fix it by probing the SoC driver late, but I don't think that
>>>> is the correct approach here.
>>>
>>> I think the correct fix would be to propagate the -EPROBE_DEFER
>>> and return that from imx8_soc_init(), so it gets retried again
>>> after the clock driver.
>> I already tried that, but if I return -EPROBE_DEFER from
>> device_initcall, it doesn't get retriggered . I suspect EPROBE_DEFER
>> works only for proper drivers ?
> 
> Right, of course. And unfortunately it can't just register to
> the fsl,imx8mm-anatop/fsl,imx8mm-ocotp/... nodes because they
> all have a driver already.
> 
> On the other hand, making it a late_initcall() defeats the
> purpose of the driver because then it can't be used by other
> drivers with soc_device_match(), resulting in incorrect
> behavior when another driver relies on this to enable
> a chip revision specific workaround.
I know, I am unsure how to proceed with this. Convert this to 
platform_driver or some such and probe it early maybe ?

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Saravana Kannan]

On Wed, Sep 25, 2024 at 10:07 AM Marek Vasut <marex@denx.de> wrote:
>
> On 9/25/24 6:31 PM, Arnd Bergmann wrote:
> > On Wed, Sep 25, 2024, at 16:09, Marek Vasut wrote:
> >> On 9/25/24 6:04 PM, Arnd Bergmann wrote:
> >>> On Wed, Sep 25, 2024, at 16:00, 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.
> >>>>
> >>>> Attempt to fix it by probing the SoC driver late, but I don't think that
> >>>> is the correct approach here.
> >>>
> >>> I think the correct fix would be to propagate the -EPROBE_DEFER
> >>> and return that from imx8_soc_init(), so it gets retried again
> >>> after the clock driver.
> >> I already tried that, but if I return -EPROBE_DEFER from
> >> device_initcall, it doesn't get retriggered . I suspect EPROBE_DEFER
> >> works only for proper drivers ?
> >
> > Right, of course. And unfortunately it can't just register to
> > the fsl,imx8mm-anatop/fsl,imx8mm-ocotp/... nodes because they
> > all have a driver already.

Arnd,

Can't we change this to add a platform device and a platform driver in
the initcall? And then the driver can return -EPROBE_DEFER if it can't
get the clock yet?

> >
> > On the other hand, making it a late_initcall() defeats the
> > purpose of the driver because then it can't be used by other
> > drivers with soc_device_match(), resulting in incorrect
> > behavior when another driver relies on this to enable
> > a chip revision specific workaround.

We could have soc_device_match() return -EPROBE_DEFER if no soc device
has been registered yet.

For cases where it's already working without any changes, we shouldn't
see any new -EPROBE_DEFER return values. But for cases like what Marek
is trying to do, it should work properly. He might have to fix bad
driver code where they remap the error instead of returning it as is.

On a tangential note, the soc framework seems to be yet another
framework violating the bus vs class thing. If it's a bus, then you
need to have a probe. Otherwise, just make it a class. Might be too
much to fix at this point, but might be good to keep this in mind if
you plan to write more frameworks or redo soc framework at some point
:)

See Slide 20:
https://lpc.events/event/18/contributions/1734/

> I know, I am unsure how to proceed with this. Convert this to
> platform_driver or some such and probe it early maybe ?

Marek,

Thanks for trying out drive_async_probe=* and trying to fix boards/drivers.

The issue you are facing and the proper way to handle it was covered
in my talk at LPC 2024 in Slide 18:
https://lpc.events/event/18/contributions/1734/

All the benefits of fw_devlink are only present for drivers that use a
device-driver model. And yes, in this case we should convert this
driver into a platform device/driver if it's possible.

Thanks,
Saravana

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Arnd Bergmann]

On Wed, Sep 25, 2024, at 18:48, Saravana Kannan wrote:
> On Wed, Sep 25, 2024 at 10:07 AM Marek Vasut <marex@denx.de> wrote:
>> >
>> > Right, of course. And unfortunately it can't just register to
>> > the fsl,imx8mm-anatop/fsl,imx8mm-ocotp/... nodes because they
>> > all have a driver already.
>
> Can't we change this to add a platform device and a platform driver in
> the initcall? And then the driver can return -EPROBE_DEFER if it can't
> get the clock yet?

Yes, good idea. So the initcall would still use of_match_node()
to see if wants to be loaded and then either bail early or
call platform_create_bundle() to register the driver and the
device.

>> > On the other hand, making it a late_initcall() defeats the
>> > purpose of the driver because then it can't be used by other
>> > drivers with soc_device_match(), resulting in incorrect
>> > behavior when another driver relies on this to enable
>> > a chip revision specific workaround.
>
> We could have soc_device_match() return -EPROBE_DEFER if no soc device
> has been registered yet.
>
> For cases where it's already working without any changes, we shouldn't
> see any new -EPROBE_DEFER return values. But for cases like what Marek
> is trying to do, it should work properly. He might have to fix bad
> driver code where they remap the error instead of returning it as is.

Right.

> On a tangential note, the soc framework seems to be yet another
> framework violating the bus vs class thing. If it's a bus, then you
> need to have a probe. Otherwise, just make it a class. Might be too
> much to fix at this point, but might be good to keep this in mind if
> you plan to write more frameworks or redo soc framework at some point
> :)
>
> See Slide 20:
> https://lpc.events/event/18/contributions/1734/

Very useful, I don't think I've seen this explained like this
in the past. It's probably not easy to change now since I'm
sure there is existing userspace looking at /sys/bus/soc, but
I can at least make sure I'll follow these when reviewing new
bus_type or class submissions.

     Arnd

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Saravana Kannan]

On Wed, Sep 25, 2024 at 1:00 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Sep 25, 2024, at 18:48, Saravana Kannan wrote:
> > On Wed, Sep 25, 2024 at 10:07 AM Marek Vasut <marex@denx.de> wrote:
> >> >
> >> > Right, of course. And unfortunately it can't just register to
> >> > the fsl,imx8mm-anatop/fsl,imx8mm-ocotp/... nodes because they
> >> > all have a driver already.
> >
> > Can't we change this to add a platform device and a platform driver in
> > the initcall? And then the driver can return -EPROBE_DEFER if it can't
> > get the clock yet?
>
> Yes, good idea. So the initcall would still use of_match_node()
> to see if wants to be loaded and then either bail early or
> call platform_create_bundle() to register the driver and the
> device.
>
> >> > On the other hand, making it a late_initcall() defeats the
> >> > purpose of the driver because then it can't be used by other
> >> > drivers with soc_device_match(), resulting in incorrect
> >> > behavior when another driver relies on this to enable
> >> > a chip revision specific workaround.
> >
> > We could have soc_device_match() return -EPROBE_DEFER if no soc device
> > has been registered yet.
> >
> > For cases where it's already working without any changes, we shouldn't
> > see any new -EPROBE_DEFER return values. But for cases like what Marek
> > is trying to do, it should work properly. He might have to fix bad
> > driver code where they remap the error instead of returning it as is.
>
> Right.

Sweet!

Marek, now you know what to do :)

>
> > On a tangential note, the soc framework seems to be yet another
> > framework violating the bus vs class thing. If it's a bus, then you
> > need to have a probe. Otherwise, just make it a class. Might be too
> > much to fix at this point, but might be good to keep this in mind if
> > you plan to write more frameworks or redo soc framework at some point
> > :)
> >
> > See Slide 20:
> > https://lpc.events/event/18/contributions/1734/
>
> Very useful, I don't think I've seen this explained like this
> in the past. It's probably not easy to change now since I'm
> sure there is existing userspace looking at /sys/bus/soc, but
> I can at least make sure I'll follow these when reviewing new
> bus_type or class submissions.

Great to hear it was useful!

-Saravana

Re: [PATCH] soc: imx8m: Probe the SoC driver late

[Marek Vasut]

On 9/25/24 8:48 PM, Saravana Kannan wrote:

Hi,

>>> On the other hand, making it a late_initcall() defeats the
>>> purpose of the driver because then it can't be used by other
>>> drivers with soc_device_match(), resulting in incorrect
>>> behavior when another driver relies on this to enable
>>> a chip revision specific workaround.
> 
> We could have soc_device_match() return -EPROBE_DEFER if no soc device
> has been registered yet.
> 
> For cases where it's already working without any changes, we shouldn't
> see any new -EPROBE_DEFER return values. But for cases like what Marek
> is trying to do, it should work properly. He might have to fix bad
> driver code where they remap the error instead of returning it as is.

I sent out V2.

> On a tangential note, the soc framework seems to be yet another
> framework violating the bus vs class thing. If it's a bus, then you
> need to have a probe. Otherwise, just make it a class. Might be too
> much to fix at this point, but might be good to keep this in mind if
> you plan to write more frameworks or redo soc framework at some point
> :)
> 
> See Slide 20:
> https://lpc.events/event/18/contributions/1734/
> 
>> I know, I am unsure how to proceed with this. Convert this to
>> platform_driver or some such and probe it early maybe ?
> 
> Marek,
> 
> Thanks for trying out drive_async_probe=* and trying to fix boards/drivers.

That was one of the most useful things I picked at LPC this year, thanks.

> The issue you are facing and the proper way to handle it was covered
> in my talk at LPC 2024 in Slide 18:
> https://lpc.events/event/18/contributions/1734/
> 
> All the benefits of fw_devlink are only present for drivers that use a
> device-driver model. And yes, in this case we should convert this
> driver into a platform device/driver if it's possible.
Done in V2, thanks.