From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754177AbbCaBbK (ORCPT ); Mon, 30 Mar 2015 21:31:10 -0400 Received: from mail-bn1on0119.outbound.protection.outlook.com ([157.56.110.119]:22896 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754035AbbCaBbH (ORCPT ); Mon, 30 Mar 2015 21:31:07 -0400 Date: Mon, 30 Mar 2015 20:30:57 -0500 From: Scott Wood To: chenhui zhao CC: , , , Subject: Re: [2/4] powerpc/rcpm: add RCPM driver Message-ID: <20150331013057.GB5667@home.buserror.net> References: <1427365095-26396-2-git-send-email-chenhui.zhao@freescale.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <1427365095-26396-2-git-send-email-chenhui.zhao@freescale.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Originating-IP: [2601:2:5800:3f7:12bf:48ff:fe84:c9a0] X-ClientProxiedBy: BY2PR04CA0084.namprd04.prod.outlook.com (10.255.247.52) To BN3PR03MB1480.namprd03.prod.outlook.com (25.163.35.143) Authentication-Results: freescale.com; dkim=none (message not signed) header.d=none; X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN3PR03MB1480; X-Forefront-Antispam-Report: BMV:1;SFV:NSPM;SFS:(10019020)(6009001)(51704005)(24454002)(47776003)(87976001)(46102003)(33656002)(62966003)(86362001)(92566002)(40100003)(122386002)(77156002)(110136001)(54356999)(46406003)(50986999)(23726002)(19580405001)(53416004)(50466002)(19580395003)(97756001)(42186005)(2950100001)(76176999)(3826002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN3PR03MB1480;H:home.buserror.net;FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5002010)(5005006);SRVR:BN3PR03MB1480;BCL:0;PCL:0;RULEID:;SRVR:BN3PR03MB1480; X-Forefront-PRVS: 0532BF6DC2 X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2015 01:31:03.4832 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR03MB1480 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 26, 2015 at 06:18:13PM +0800, chenhui zhao wrote: > There is a RCPM (Run Control/Power Management) in Freescale QorIQ > series processors. The device performs tasks associated with device > run control and power management. > > The driver implements some features: mask/unmask irq, enter/exit low > power states, freeze time base, etc. > > Signed-off-by: Chenhui Zhao > --- > Documentation/devicetree/bindings/soc/fsl/rcpm.txt | 23 ++ > arch/powerpc/include/asm/fsl_guts.h | 105 ++++++ > arch/powerpc/include/asm/fsl_pm.h | 49 +++ > arch/powerpc/platforms/85xx/Kconfig | 1 + > arch/powerpc/sysdev/Kconfig | 5 + > arch/powerpc/sysdev/Makefile | 1 + > arch/powerpc/sysdev/fsl_rcpm.c | 353 +++++++++++++++++++++ > 7 files changed, 537 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/fsl/rcpm.txt > create mode 100644 arch/powerpc/include/asm/fsl_pm.h > create mode 100644 arch/powerpc/sysdev/fsl_rcpm.c > > diff --git a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt > new file mode 100644 > index 0000000..8c21b6c > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt > @@ -0,0 +1,23 @@ > +* Run Control and Power Management > + > +The RCPM performs all device-level tasks associated with device run control > +and power management. > + > +Required properites: > + - reg : Offset and length of the register set of RCPM block. > + - compatible : Specifies the compatibility list for the RCPM. The type > + should be string, such as "fsl,qoriq-rcpm-1.0", "fsl,qoriq-rcpm-2.0". > + > +Example: > +The RCPM node for T4240: > + rcpm: global-utilities@e2000 { > + compatible = "fsl,t4240-rcpm", "fsl,qoriq-rcpm-2.0"; > + reg = <0xe2000 0x1000>; > + }; > + > +The RCPM node for P4080: > + rcpm: global-utilities@e2000 { > + compatible = "fsl,qoriq-rcpm-1.0"; > + reg = <0xe2000 0x1000>; > + #sleep-cells = <1>; > + }; Where is #sleep-cells documented? It's copy-and-paste from something that was never finished from many years ago. > diff --git a/arch/powerpc/include/asm/fsl_pm.h b/arch/powerpc/include/asm/fsl_pm.h > new file mode 100644 > index 0000000..bbe6089 > --- /dev/null > +++ b/arch/powerpc/include/asm/fsl_pm.h > @@ -0,0 +1,49 @@ > +/* > + * Support Power Management > + * > + * Copyright 2014-2015 Freescale Semiconductor Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > +#ifndef __PPC_FSL_PM_H > +#define __PPC_FSL_PM_H > +#ifdef __KERNEL__ Put a space after #ifdef, not a tab. > +#define E500_PM_PH10 1 > +#define E500_PM_PH15 2 > +#define E500_PM_PH20 3 > +#define E500_PM_PH30 4 > +#define E500_PM_DOZE E500_PM_PH10 > +#define E500_PM_NAP E500_PM_PH15 > + > +#define PLAT_PM_SLEEP 20 > +#define PLAT_PM_LPM20 30 > + > +#define FSL_PM_SLEEP (1 << 0) > +#define FSL_PM_DEEP_SLEEP (1 << 1) > + > +struct fsl_pm_ops { > + /* mask pending interrupts to the RCPM from MPIC */ > + void (*irq_mask)(int cpu); > + /* unmask pending interrupts to the RCPM from MPIC */ > + void (*irq_unmask)(int cpu); > + /* place the CPU in the specified state */ > + void (*cpu_enter_state)(int cpu, int state); > + /* exit the CPU from the specified state */ > + void (*cpu_exit_state)(int cpu, int state); > + /* place the platform in the sleep state */ > + int (*plat_enter_sleep)(void); > + /* freeze the time base */ > + void (*freeze_time_base)(int freeze); > + /* keep the power of IP blocks during sleep/deep sleep */ > + void (*set_ip_power)(int enable, u32 *mask); > + /* get platform supported power management modes */ > + unsigned int (*get_pm_modes)(void); > +}; Drop the comments that are basically just a restatement of the function name. Where there are comments, it'd be easier to read with a blank line between a function and the next comment. s/int enable/bool enable/ s/int freeze/bool freeze/ > +#endif /* __KERNEL__ */ > +#endif /* __PPC_FSL_PM_H */ Please be consistent with whitespace. > + default: > + pr_err("%s: Unknown cpu PM state (%d)\n", __func__, state); WARN? > +static int rcpm_v2_plat_enter_state(int state) > +{ > + u32 *pmcsr_reg = &rcpm_v2_regs->powmgtcsr; > + int ret = 0; > + int result; > + > + switch (state) { > + case PLAT_PM_LPM20: > + /* clear previous LPM20 status */ > + setbits32(pmcsr_reg, RCPM_POWMGTCSR_P_LPM20_ST); How would the bit be set when you enter here, given that you wait for it to clear when leaving? > + /* enter LPM20 status */ > + setbits32(pmcsr_reg, RCPM_POWMGTCSR_LPM20_RQ); > + > + /* At this point, the device is in LPM20 status. */ > + > + /* resume ... */ > + result = spin_event_timeout( > + !(in_be32(pmcsr_reg) & RCPM_POWMGTCSR_LPM20_ST), 10000, 10); > + if (!result) { > + pr_err("%s: timeout waiting for LPM20 bit to be cleared\n", > + __func__); > + ret = -ETIMEDOUT; > + } > + break; "At this point" is a bit misleading. I think it's clear enough if you just drop that comment. > + default: > + pr_err("%s: Unknown platform PM state (%d)\n", > + __func__, state); > + ret = -EINVAL; > + } WARN? > +static const struct of_device_id rcpm_matches[] = { > + { > + .compatible = "fsl,qoriq-rcpm-1.0", > + .data = (void *)RCPM_V1, > + }, > + { > + .compatible = "fsl,qoriq-rcpm-2.0", > + .data = (void *)RCPM_V2, > + }, Why not point .data directly at the ops? > + switch ((unsigned long)match->data) { > + case RCPM_V1: > + rcpm_v1_regs = base; > + qoriq_pm_ops = &qoriq_rcpm_v1_ops; > + break; > + > + case RCPM_V2: > + rcpm_v2_regs = base; > + qoriq_pm_ops = &qoriq_rcpm_v2_ops; > + break; > + > + default: > + break; > + } default: break; is unnecessary (and impossible to hit -- if you really want default: it should probably WARN). -Scott