Re: [PATCH V2] ARM: imx: add smp support for imx7d - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Shawn Guo <shawnguo@kernel.org>
Cc: Marek Vasut <marex@denx.de>,
	Arulpandiyan Vadivel <arulpandiyan_vadivel@mentor.com>,
	Anson Huang <b20788@freescale.com>,
	Leonard Crestez <leonard.crestez@nxp.com>,
	Fabio Estevam <festevam@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	NXP Linux Team <linux-imx@nxp.com>
Subject: Re: [PATCH V2] ARM: imx: add smp support for imx7d
Date: Sun, 17 Jan 2021 12:31:59 +0000	[thread overview]
Message-ID: <20210117123159.GC1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210117114617.GT28365@dragon>

On Sun, Jan 17, 2021 at 07:46:20PM +0800, Shawn Guo wrote:
> On Thu, Jan 07, 2021 at 01:31:57PM +0100, Marek Vasut wrote:
> > From: Anson Huang <b20788@freescale.com>
> > 
> > Add SMP support for i.MX7D, including CPU hotplug support, for
> > systems where TFA is not present.
> > 
> > The arm,cpu-registers-not-fw-configured is required, otherwise the
> > timer does not work correctly.
> 
> As DT change becomes another patch, this should be dropped.
> 
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > Signed-off-by: Arulpandiyan Vadivel <arulpandiyan_vadivel@mentor.com> # Fix merge conflicts
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Signed-off-by: Marek Vasut <marex@denx.de> # heavy cleanup
> 
> Checkpatch warnings:
> 
> WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> #151: FILE: arch/arm/mach-imx/platsmp.c:108:
> +	for (i = ncores; i < NR_CPUS; i++)

False. This is initialising the cpu possible map, so can't use
cpu_possible().

> > +void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset)
> 
> static inline?

Only "static" not "static inline" in a .c file.

> > +void __init imx7_src_init(void)
> > +{
> > +	struct device_node *np;
> > +	gpr_v2 = true;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-src");
> > +	if (!np)
> > +		return;
> > +	src_base = of_iomap(np, 0);
> > +	WARN_ON(!src_base);
> 
> Don't we need of_node_put()?

I always worry about this. The resources of the node remains in use
after the initialisation function has completed, so why _shouldn't_
the node also have a reference to it - in the same way that any
bound driver effectively maintains a reference on its DT node for
its lifetime. The only difference is that system devices such as this
have a lifetime of the system.

It's not like you could drop the "fsl,imx7d-src" node at runtime and
the system will do the right thing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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