drivers/soc/imx/soc-imx8m.c | 92 ++++++++++++++++++++++++++++--------- 1 file changed, 70 insertions(+), 22 deletions(-)
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(-)
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");
--
2.45.2Hi 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);
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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 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 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.
[...]
© 2016 - 2024 Red Hat, Inc.