Best Regards! Anson Huang
> -----Original Message----- > From: Russell King - ARM Linux [mailto:li...@armlinux.org.uk] > Sent: 2016-08-26 7:18 PM > To: Yongcai Huang <anson.hu...@nxp.com> > Cc: linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; linux- > ker...@vger.kernel.org; shawn...@kernel.org; ker...@pengutronix.de; > Fabio Estevam <fabio.este...@nxp.com>; robh...@kernel.org; > mark.rutl...@arm.com > Subject: Re: [PATCH 2/3] ARM: imx: add gpcv2 support > > On Fri, Aug 26, 2016 at 07:12:50PM +0800, Anson Huang wrote: > > diff --git a/arch/arm/mach-imx/gpcv2.c b/arch/arm/mach-imx/gpcv2.c new > > file mode 100644 index 0000000..98577c4 > > --- /dev/null > > +++ b/arch/arm/mach-imx/gpcv2.c > > @@ -0,0 +1,66 @@ > > +/* > > + * Copyright 2016 Freescale Semiconductor, Inc. > > + * > > + * The code contained herein is licensed under the GNU General Public > > + * License. You may obtain a copy of the GNU General Public License > > + * Version 2 or later at the following locations: > > + * > > + * http://www.opensource.org/licenses/gpl-license.html > > + * http://www.gnu.org/copyleft/gpl.html > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/io.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > + > > +#include "common.h" > > + > > +#define GPC_CPU_PGC_SW_PUP_REQ 0xf0 > > +#define GPC_CPU_PGC_SW_PDN_REQ 0xfc > > +#define GPC_PGC_C1 0x840 > > + > > +#define BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7 0x2 > > +#define BM_GPC_PGC_PCG 0x1 > > + > > +static void __iomem *gpcv2_base; > > + > > +static void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset) { > > + u32 val = readl_relaxed(gpcv2_base + offset) & (~BM_GPC_PGC_PCG); > > Unnecessary parens, and the code doesn't flow with the bit clearance here... > > > + > > + if (enable) > > + val |= BM_GPC_PGC_PCG; > > My first read of this lead me to think "okay, so what's the point of > enable=false". It would be clearer with: > > u32 val = readl_relaxed(gpcv2_base + offset); > > if (enable) > val |= BM_GPC_PGC_PCG; > else > val &= ~BM_GPC_PGC_PCG; > > here. Agree, will improve it in V2 patch. > > > + > > + writel_relaxed(val, gpcv2_base + offset); } > > + > > +void imx_gpcv2_set_core1_pdn_pup_by_software(bool pdn) { > > + u32 val = readl_relaxed(gpcv2_base + (pdn ? > > + GPC_CPU_PGC_SW_PDN_REQ : > GPC_CPU_PGC_SW_PUP_REQ)); > > + > > + imx_gpcv2_set_m_core_pgc(true, GPC_PGC_C1); > > + val |= BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7; > > + writel_relaxed(val, gpcv2_base + (pdn ? > > + GPC_CPU_PGC_SW_PDN_REQ : > GPC_CPU_PGC_SW_PUP_REQ)); > > + > > + while ((readl_relaxed(gpcv2_base + (pdn ? > > + GPC_CPU_PGC_SW_PDN_REQ : > GPC_CPU_PGC_SW_PUP_REQ)) & > > + BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7) != 0) > > + ; > > + imx_gpcv2_set_m_core_pgc(false, GPC_PGC_C1); } > > + > > +void __init imx_gpcv2_check_dt(void) > > +{ > > + struct device_node *np; > > + > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-gpc"); > > + if (WARN_ON(!np)) > > + return; > > + > > + gpcv2_base = of_iomap(np, 0); > > + WARN_ON(!gpcv2_base); > > What happens if gpcv2_base is NULL (apart from the obvious warning from the > above line)? I guess a bit later in the boot, > imx_gpcv2_set_core1_pdn_pup_by_software() oopses the kernel, probably > before the console has been initialised. Probably not nice behaviour. > Yes, normal console is NOT ready at this stage, unless early console is enabled. Could you please advise how to handle such case if gpcv2_base is NULL and console is NOT ready? Checking gpcv2_base everywhere before using it? Normally gpcv2_base should NOT be NULL. Thanks. Anson > > +} > > -- > > 1.9.1 > > > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
