Re: [2/4] powerpc/rcpm: add RCPM driver
From: Scott Wood <hidden>
Date: 2015-03-31 01:31:10
Also in:
linux-devicetree, lkml
On Thu, Mar 26, 2015 at 06:18:13PM +0800, chenhui zhao wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.cdiff --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.
quoted hunk ↗ jump to hunk
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