From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753297AbcHZL2r (ORCPT ); Fri, 26 Aug 2016 07:28:47 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:33348 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753219AbcHZL2p (ORCPT ); Fri, 26 Aug 2016 07:28:45 -0400 Date: Fri, 26 Aug 2016 12:17:58 +0100 From: Russell King - ARM Linux To: Anson Huang Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, shawnguo@kernel.org, kernel@pengutronix.de, fabio.estevam@nxp.com, robh+dt@kernel.org, mark.rutland@arm.com Subject: Re: [PATCH 2/3] ARM: imx: add gpcv2 support Message-ID: <20160826111758.GN1041@n2100.armlinux.org.uk> References: <1472209971-32469-1-git-send-email-Anson.Huang@nxp.com> <1472209971-32469-3-git-send-email-Anson.Huang@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1472209971-32469-3-git-send-email-Anson.Huang@nxp.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > +#include > +#include > +#include > +#include > + > +#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. > + > + 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. > +} > -- > 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.