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

[Scott Wood]

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@xxxxxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/