| Message ID | 1594283145-7981-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 7f6e8dffc30bd22b15ad810fb90ea741c15e6d54 |
| Headers | show |
| Series | soc: imx: check ls1021a | expand |
On 7/9/2020 11:29 AM, Peng Fan wrote: > From: Peng Fan <peng.fan@nxp.com> > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could > not use the soc driver which will break caam on ls1021a platform. > > So directly return if it is compatible with fsl,ls1021a. > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to drivers/soc/imx") > Signed-off-by: Peng Fan <peng.fan@nxp.com> Tested-by: Horia Geantă <horia.geanta@nxp.com> i.e. with this patch caam driver probes fine on LS1021A. Thanks, Horia
On Thu, Jul 9, 2020 at 5:30 AM <peng.fan@nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could > not use the soc driver which will break caam on ls1021a platform. > > So directly return if it is compatible with fsl,ls1021a. > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to drivers/soc/imx") > Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Fabio Estevam <festevam@gmail.com>
On Thu, Jul 09, 2020 at 04:25:45PM +0800, peng.fan@nxp.com wrote: > From: Peng Fan <peng.fan@nxp.com> > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could > not use the soc driver which will break caam on ls1021a platform. > > So directly return if it is compatible with fsl,ls1021a. > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to drivers/soc/imx") > Signed-off-by: Peng Fan <peng.fan@nxp.com> Applied, thanks.
On Thu, Jul 9, 2020 at 10:31 AM <peng.fan@nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could > not use the soc driver which will break caam on ls1021a platform. > > So directly return if it is compatible with fsl,ls1021a. > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to drivers/soc/imx") > Signed-off-by: Peng Fan <peng.fan@nxp.com> I just forwarded this patch to Linus as part of the v5.8 fixes. The patch goes in the right direction, but as far as I can tell, the code is still wrong and needs to be fixed. Can you create another patch on top? The problem is that loading this driver on *anything* other than an i.MX machine will cause unexpected results, and it first has to check that it is running on a compatible machine to start with! In a distro kernel, this driver is always built-in at the moment if CONFIG_ARCH_MXC is enabled, regardless of what other platforms are enabled in addition, and what machine we end up running on. Arnd
Hi Arnd, > Subject: Re: [PATCH] soc: imx: check ls1021a > > On Thu, Jul 9, 2020 at 10:31 AM <peng.fan@nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could not > > use the soc driver which will break caam on ls1021a platform. > > > > So directly return if it is compatible with fsl,ls1021a. > > > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to drivers/soc/imx") > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > I just forwarded this patch to Linus as part of the v5.8 fixes. > The patch goes in the right direction, but as far as I can tell, the code is still > wrong and needs to be fixed. Can you create another patch on top? > > The problem is that loading this driver on *anything* other than an i.MX > machine will cause unexpected results, and it first has to check that it is > running on a compatible machine to start with! > > In a distro kernel, this driver is always built-in at the moment if > CONFIG_ARCH_MXC is enabled, regardless of what other platforms are > enabled in addition, and what machine we end up running on. So I could use CONFIG_SOC_IMX[x] to build the file? Is this ok for you? Thanks, Peng. > > Arnd
On Mon, Jul 20, 2020 at 8:30 AM Peng Fan <peng.fan@nxp.com> wrote: > > Hi Arnd, > > > Subject: Re: [PATCH] soc: imx: check ls1021a > > > > On Thu, Jul 9, 2020 at 10:31 AM <peng.fan@nxp.com> wrote: > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could not > > > use the soc driver which will break caam on ls1021a platform. > > > > > > So directly return if it is compatible with fsl,ls1021a. > > > > > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to drivers/soc/imx") > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > I just forwarded this patch to Linus as part of the v5.8 fixes. > > The patch goes in the right direction, but as far as I can tell, the code is still > > wrong and needs to be fixed. Can you create another patch on top? > > > > The problem is that loading this driver on *anything* other than an i.MX > > machine will cause unexpected results, and it first has to check that it is > > running on a compatible machine to start with! > > > > In a distro kernel, this driver is always built-in at the moment if > > CONFIG_ARCH_MXC is enabled, regardless of what other platforms are > > enabled in addition, and what machine we end up running on. > > So I could use CONFIG_SOC_IMX[x] to build the file? Is this ok for you? Certainly not, that would not address the problem at all. You really have to replace the compile-time decision with a runtime decision. Running hardware specific code as the result of an initcall or module_init() is never correct. The usual way this is handled is to register a platform_driver instance that binds to a set of DT compatible strings, and then only do hardware specific actions in the probe function that is called if as compatible device is actually present. If you cannot do that here (e.g. because you need the soc_device to be present very early), then you have to manually check for some DT node and property to determine whether you run on an i.MX machine and do the silent 'return 0' if not. This is roughly what you did when you checked for one specific non-i.MX machine, but since you cannot provide an exhaustive list of all non-i.MX machines (a denylist) you have to provide some for of allowlist. Arnd
> Subject: Re: [PATCH] soc: imx: check ls1021a > > On Mon, Jul 20, 2020 at 8:30 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > Hi Arnd, > > > > > Subject: Re: [PATCH] soc: imx: check ls1021a > > > > > > On Thu, Jul 9, 2020 at 10:31 AM <peng.fan@nxp.com> wrote: > > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could > > > > not use the soc driver which will break caam on ls1021a platform. > > > > > > > > So directly return if it is compatible with fsl,ls1021a. > > > > > > > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to > > > > drivers/soc/imx") > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > > > I just forwarded this patch to Linus as part of the v5.8 fixes. > > > The patch goes in the right direction, but as far as I can tell, the > > > code is still wrong and needs to be fixed. Can you create another patch on > top? > > > > > > The problem is that loading this driver on *anything* other than an > > > i.MX machine will cause unexpected results, and it first has to > > > check that it is running on a compatible machine to start with! > > > > > > In a distro kernel, this driver is always built-in at the moment if > > > CONFIG_ARCH_MXC is enabled, regardless of what other platforms are > > > enabled in addition, and what machine we end up running on. > > > > So I could use CONFIG_SOC_IMX[x] to build the file? Is this ok for you? > > Certainly not, that would not address the problem at all. Ok, then we have same issue in soc-imx8m.c soc-imx-scu.c I think. soc-imx-scu.c use platform driver, but it not use DT. > > You really have to replace the compile-time decision with a runtime decision. > Running hardware specific code as the result of an initcall or module_init() is > never correct. Got it. > > The usual way this is handled is to register a platform_driver instance that > binds to a set of DT compatible strings, and then only do hardware specific > actions in the probe function that is called if as compatible device is actually > present. > > If you cannot do that here (e.g. because you need the soc_device to be > present very early), then you have to manually check for some DT node and > property to determine whether you run on an i.MX machine and do the silent > 'return 0' if not. > This is roughly what you did when you checked for one specific non-i.MX > machine, but since you cannot provide an exhaustive list of all non-i.MX > machines (a denylist) you have to provide some for of allowlist. Thanks for the tips, I'll check to see which could be used here. Thanks, Peng. > > Arnd
On Mon, Jul 20, 2020 at 9:05 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > The problem is that loading this driver on *anything* other than an > > > > i.MX machine will cause unexpected results, and it first has to > > > > check that it is running on a compatible machine to start with! > > > > > > > > In a distro kernel, this driver is always built-in at the moment if > > > > CONFIG_ARCH_MXC is enabled, regardless of what other platforms are > > > > enabled in addition, and what machine we end up running on. > > > > > > So I could use CONFIG_SOC_IMX[x] to build the file? Is this ok for you? > > > > Certainly not, that would not address the problem at all. > > Ok, then we have same issue in soc-imx8m.c soc-imx-scu.c I think. > soc-imx-scu.c use platform driver, but it not use DT. That driver checks for the presence of the "fsl,imx-scu" device node first: np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu"); if (!np) return -ENODEV; It might be better to return '0' here, but I think it is otherwise correct. Arnd
> Subject: Re: [PATCH] soc: imx: check ls1021a > > On Mon, Jul 20, 2020 at 8:30 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > Hi Arnd, > > > > > Subject: Re: [PATCH] soc: imx: check ls1021a > > > > > > On Thu, Jul 9, 2020 at 10:31 AM <peng.fan@nxp.com> wrote: > > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > fsl,ls1021a is a mach under arch/arm/mach-imx/, however it could > > > > not use the soc driver which will break caam on ls1021a platform. > > > > > > > > So directly return if it is compatible with fsl,ls1021a. > > > > > > > > Fixes: 52102a3ba6a61 ("soc: imx: move cpu code to > > > > drivers/soc/imx") > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > > > I just forwarded this patch to Linus as part of the v5.8 fixes. > > > The patch goes in the right direction, but as far as I can tell, the > > > code is still wrong and needs to be fixed. Can you create another patch on > top? > > > > > > The problem is that loading this driver on *anything* other than an > > > i.MX machine will cause unexpected results, and it first has to > > > check that it is running on a compatible machine to start with! > > > > > > In a distro kernel, this driver is always built-in at the moment if > > > CONFIG_ARCH_MXC is enabled, regardless of what other platforms are > > > enabled in addition, and what machine we end up running on. > > > > So I could use CONFIG_SOC_IMX[x] to build the file? Is this ok for you? > > Certainly not, that would not address the problem at all. > > You really have to replace the compile-time decision with a runtime decision. > Running hardware specific code as the result of an initcall or module_init() is > never correct. > > The usual way this is handled is to register a platform_driver instance that > binds to a set of DT compatible strings, and then only do hardware specific > actions in the probe function that is called if as compatible device is actually > present. > > If you cannot do that here (e.g. because you need the soc_device to be > present very early), then you have to manually check for some DT node and > property to determine whether you run on an i.MX machine and do the silent > 'return 0' if not. > This is roughly what you did when you checked for one specific non-i.MX > machine, but since you cannot provide an exhaustive list of all non-i.MX > machines (a denylist) you have to provide some for of allowlist. Is the following diff ok for you? Actually all i.MX SoCs will set __mxc_cpu_type. If it is not a valid one, just return. diff --git a/drivers/soc/imx/soc-imx.c b/drivers/soc/imx/soc-imx.c index 01bfea1cb64a..e55225a85592 100644 --- a/drivers/soc/imx/soc-imx.c +++ b/drivers/soc/imx/soc-imx.c @@ -33,9 +33,6 @@ static int __init imx_soc_device_init(void) u32 val; int ret; - if (of_machine_is_compatible("fsl,ls1021a")) - return 0; - soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); if (!soc_dev_attr) return -ENOMEM; @@ -131,6 +128,8 @@ static int __init imx_soc_device_init(void) break; default: soc_id = "Unknown"; + ret = 0; + goto free_soc; } soc_dev_attr->soc_id = soc_id; Thanks, Peng. > > Arnd
On Mon, Jul 20, 2020 at 11:01 AM Peng Fan <peng.fan@nxp.com> wrote: > > Subject: Re: [PATCH] soc: imx: check ls1021a > > On Mon, Jul 20, 2020 at 8:30 AM Peng Fan <peng.fan@nxp.com> wrote: > > > > Subject: Re: [PATCH] soc: imx: check ls1021a > > If you cannot do that here (e.g. because you need the soc_device to be > > present very early), then you have to manually check for some DT node and > > property to determine whether you run on an i.MX machine and do the silent > > 'return 0' if not. > > This is roughly what you did when you checked for one specific non-i.MX > > machine, but since you cannot provide an exhaustive list of all non-i.MX > > machines (a denylist) you have to provide some for of allowlist. > > Is the following diff ok for you? > Actually all i.MX SoCs will set __mxc_cpu_type. If it is not a valid > one, just return. > > diff --git a/drivers/soc/imx/soc-imx.c b/drivers/soc/imx/soc-imx.c > index 01bfea1cb64a..e55225a85592 100644 > --- a/drivers/soc/imx/soc-imx.c > +++ b/drivers/soc/imx/soc-imx.c > @@ -33,9 +33,6 @@ static int __init imx_soc_device_init(void) > u32 val; > int ret; > > - if (of_machine_is_compatible("fsl,ls1021a")) > - return 0; > - > soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); > if (!soc_dev_attr) > return -ENOMEM; This means the driver still has to allocate and then free the structure, as well as lookup some DT properties. > @@ -131,6 +128,8 @@ static int __init imx_soc_device_init(void) > break; > default: > soc_id = "Unknown"; > + ret = 0; > + goto free_soc; > } > soc_dev_attr->soc_id = soc_id; I took a closer look at where this information comes from. As the __mxc_cpu_type variable seems to just be initialized to zero until set explicitly, I guess the easiest way would be to change the of_machine_is_compatible() check to 'if (!__mxc_cpu_type)'. Arnd
diff --git a/drivers/soc/imx/soc-imx.c b/drivers/soc/imx/soc-imx.c index fec3d672b606..01bfea1cb64a 100644 --- a/drivers/soc/imx/soc-imx.c +++ b/drivers/soc/imx/soc-imx.c @@ -33,6 +33,9 @@ static int __init imx_soc_device_init(void) u32 val; int ret; + if (of_machine_is_compatible("fsl,ls1021a")) + return 0; + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); if (!soc_dev_attr) return -ENOMEM;