Re: [PATCH 3/9] powerpc/rcpm: add RCPM driver

[Scott Wood <scottwood@freescale.com>]

On Fri, 2014-03-07 at 12:57 +0800, Chenhui Zhao wrote:
> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
> index b756f3d..3fdf9f3 100644
> --- a/arch/powerpc/platforms/85xx/corenet_generic.c
> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
> @@ -56,6 +56,8 @@ void __init corenet_gen_setup_arch(void)
>  
>  	swiotlb_detect_4g();
>  
> +	fsl_rcpm_init();
> +
>  	pr_info("%s board from Freescale Semiconductor\n", ppc_md.name);

RCPM is not board-specific.  Why is this in board code?

> +static void rcpm_v1_cpu_enter_state(int cpu, int state)
> +{
> +	unsigned int hw_cpu = get_hard_smp_processor_id(cpu);
> +	unsigned int mask = 1 << hw_cpu;
> +
> +	switch (state) {
> +	case E500_PM_PH10:
> +		setbits32(&rcpm_v1_regs->cdozcr, mask);
> +		break;
> +	case E500_PM_PH15:
> +		setbits32(&rcpm_v1_regs->cnapcr, mask);
> +		break;
> +	default:
> +		pr_err("Unknown cpu PM state\n");
> +		break;
> +	}
> +}

Put __func__ in error messages -- and for "unknown value" type messages,
print the value.


> +static int rcpm_v1_plat_enter_state(int state)
> +{
> +	u32 *pmcsr_reg = &rcpm_v1_regs->powmgtcsr;
> +	int ret = 0;
> +	int result;
> +
> +	switch (state) {
> +	case PLAT_PM_SLEEP:
> +		setbits32(pmcsr_reg, RCPM_POWMGTCSR_SLP);
> +
> +		/* At this point, the device is in sleep mode. */
> +
> +		/* Upon resume, wait for RCPM_POWMGTCSR_SLP bit to be clear. */
> +		result = spin_event_timeout(
> +		  !(in_be32(pmcsr_reg) & RCPM_POWMGTCSR_SLP), 10000, 10);
> +		if (!result) {
> +			pr_err("%s: timeout waiting for SLP bit to be cleared\n",
> +			  __func__);

Why are you indenting continuation lines with only two spaces (and yet
still not aligning with anything)?

> +			ret = -ETIMEDOUT;
> +		}
> +		break;
> +	default:
> +		pr_err("Unsupported platform PM state\n");
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static void rcpm_v1_freeze_time_base(int freeze)
> +{
> +	u32 *tben_reg = &rcpm_v1_regs->ctbenr;
> +	static u32 mask;
> +
> +	if (freeze) {
> +		mask = in_be32(tben_reg);
> +		clrbits32(tben_reg, mask);
> +	} else {
> +		setbits32(tben_reg, mask);
> +	}
> +
> +	/* read back to push the previous write */
> +	in_be32(tben_reg);
> +}
> +
> +static void rcpm_v2_freeze_time_base(int freeze)
> +{
> +	u32 *tben_reg = &rcpm_v2_regs->pctbenr;
> +	static u32 mask;
> +
> +	if (freeze) {
> +		mask = in_be32(tben_reg);
> +		clrbits32(tben_reg, mask);
> +	} else {
> +		setbits32(tben_reg, mask);
> +	}
> +
> +	/* read back to push the previous write */
> +	in_be32(tben_reg);
> +}

It looks like the only difference between these two functions is how you
calculate tben_reg -- factor the rest out into a single function.

> +int fsl_rcpm_init(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-rcpm-2.0");
> +	if (np) {
> +		rcpm_v2_regs = of_iomap(np, 0);
> +		of_node_put(np);
> +		if (!rcpm_v2_regs)
> +			return -ENOMEM;
> +
> +		qoriq_pm_ops = &qoriq_rcpm_v2_ops;
> +
> +	} else {
> +		np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-rcpm-1.0");
> +		if (np) {
> +			rcpm_v1_regs = of_iomap(np, 0);
> +			of_node_put(np);
> +			if (!rcpm_v1_regs)
> +				return -ENOMEM;
> +
> +			qoriq_pm_ops = &qoriq_rcpm_v1_ops;
> +
> +		} else {
> +			pr_err("%s: can't find the rcpm node.\n", __func__);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}

Why isn't this a proper platform driver?

-Scott