Re: [2/4] powerpc/rcpm: add RCPM driver

["chenhui.zhao@freescale.com" <chenhui.zhao@freescale.com>]

________________________________________
From: Wood Scott-B07421
Sent: Tuesday, March 31, 2015 9:30
To: Zhao Chenhui-B35336
Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [2/4] powerpc/rcpm: add RCPM driver

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 <chenhui.zhao@freescale.com>
> ---
>  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.

[chenhui] Will get rid of them.

> 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.

[Chenhui] Will change it.

> +#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/

[chenhui] Yes, you are right.

> +#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?

[chenhui] Actually, the bit is not used by software. Just follow the instruction in RM.

> +             /* 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?

[chenhui] I agree.

> +     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

[chenhui] Will get rid of them.