[PATCH] soc: imx: check ls1021a

[PATCH] soc: imx: check ls1021a

[peng.fan]

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>
---
 drivers/soc/imx/soc-imx.c | 3 +++
 1 file changed, 3 insertions(+)

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;
-- 
2.16.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Horia Geantă]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Fabio Estevam]

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>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Shawn Guo]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] soc: imx: check ls1021a

[Peng Fan]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] soc: imx: check ls1021a

[Peng Fan]

> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] soc: imx: check ls1021a

[Peng Fan]

> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: imx: check ls1021a

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel