Re: [PATCH 18/25] OMAP4: suspend: Add MPUSS power domain RETENTION support

[Kevin Hilman <khilman@ti.com>]

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

> This patch adds MPUSS(MPU Sub System) power domain
> CSWR(Close Switch Retention) support to system wide suspend.
> For both MPUSS RET support, CPUs are programmed to OFF state.

is 'both' in the wrong place in this sentence?   I think you meant:

For MPUSS retention support, both CPUs are programmed to OFF state.

> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c |   16 +++++++
>  arch/arm/mach-omap2/pm44xx.c              |   71 +++++++++++++++++++++++++++--
>  2 files changed, 82 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> index 9d68abf..9f632fe 100644
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -66,6 +66,7 @@ struct omap4_cpu_pm_info {
>  };
>  
>  static DEFINE_PER_CPU(struct omap4_cpu_pm_info, omap4_pm_info);
> +static struct powerdomain *mpuss_pd;
>  
>  /*
>   * Program the wakeup routine address for the CPU0 and CPU1
> @@ -140,6 +141,13 @@ static void scu_pwrst_prepare(unsigned int cpu_id, unsigned int cpu_state)
>   * of OMAP4 MPUSS subsystem
>   * @cpu : CPU ID
>   * @power_state: Low power state.
> + *
> + * MPUSS states for the context save:
> + * save_state =
> + *	0 - Nothing lost and no need to save: MPUSS INACTIVE
> + *	1 - CPUx L1 and logic lost: MPUSS CSWR
> + *	2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR
> + *	3 - CPUx L1 and logic lost + GIC + L2 lost: DEVICE OFF
>   */
>  int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>  {
> @@ -169,6 +177,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>  		return -ENXIO;
>  	}
>  
> +	pwrdm_clear_all_prev_pwrst(mpuss_pd);
>  	clear_cpu_prev_pwrst(cpu);
>  	set_cpu_next_pwrst(cpu, power_state);
>  	set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume));
> @@ -270,6 +279,13 @@ int __init omap4_mpuss_init(void)
>  	/* Initialise CPU1 power domain state to ON */
>  	pwrdm_set_next_pwrst(pm_info->pwrdm, PWRDM_POWER_ON);
>  
> +	mpuss_pd = pwrdm_lookup("mpu_pwrdm");
> +	if (!mpuss_pd) {
> +		pr_err("Failed to lookup MPUSS power domain\n");
> +		return -ENODEV;
> +	}
> +	pwrdm_clear_all_prev_pwrst(mpuss_pd);
> +
>  	/* Save device type on scratchpad for low level code to use */
>  	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>  		__raw_writel(1, sar_base + OMAP_TYPE_OFFSET);
> diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
> index 4f39de5..63e8f9b 100644
> --- a/arch/arm/mach-omap2/pm44xx.c
> +++ b/arch/arm/mach-omap2/pm44xx.c
> @@ -1,8 +1,9 @@
>  /*
>   * OMAP4 Power Management Routines
>   *
> - * Copyright (C) 2010 Texas Instruments, Inc.
> + * Copyright (C) 2010-2011 Texas Instruments, Inc.
>   * Rajendra Nayak <rnayak@ti.com>
> + * Santosh Shilimkar <santosh.shilimkar@ti.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -16,9 +17,11 @@
>  #include <linux/err.h>
>  #include <linux/slab.h>
>  
> +#include <mach/omap4-common.h>
> +
>  #include "powerdomain.h"
>  #include "clockdomain.h"
> -#include <mach/omap4-common.h>
> +#include "pm.h"
>  
>  struct power_state {
>  	struct powerdomain *pwrdm;
> @@ -34,7 +37,48 @@ static LIST_HEAD(pwrst_list);
>  #ifdef CONFIG_SUSPEND
>  static int omap4_pm_suspend(void)
>  {
> -	omap_do_wfi();
> +	struct power_state *pwrst;
> +	int state, ret = 0;
> +	u32 cpu_id = smp_processor_id();
> +
> +	/* Save current powerdomain state */
> +	list_for_each_entry(pwrst, &pwrst_list, node) {
> +		pwrst->saved_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
> +	}
> +
> +	/* Set targeted power domain states by suspend */
> +	list_for_each_entry(pwrst, &pwrst_list, node) {
> +		omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> +	}
> +
> +	/*
> +	 * For MPUSS to hit power domain retention(CSWR or OSWR),
> +	 * CPU0 and CPU1 power domain needs to be in OFF or DORMANT

s/domain needs/domains need/

> +	 * state. For MPUSS to reach off-mode. CPU0 and CPU1 power domain
> +	 * should be in off state.

nit: please be consistent with naming of power states (e.g. OFF vs. off)

> +	 * Only master CPU followes suspend path. All other CPUs follow
> +	 * cpu-hotplug path in system wide suspend. On OMAP4, CPU power
> +	 * domain CSWR is not supported by hardware.

I think this sentence belongs a little earlier.  E.g. something like
...CPU0 and CPU1 power domains need to be in OFF or DORMANT state, since
CPU power domain CSWR is not supported by hardware.

> +	 * More details can be found in OMAP4430 TRM section 4.3.4.2.
> +	 */
> +	omap4_enter_lowpower(cpu_id, PWRDM_POWER_OFF);
> +
> +	/* Restore next powerdomain state */
> +	list_for_each_entry(pwrst, &pwrst_list, node) {
> +		state = pwrdm_read_prev_pwrst(pwrst->pwrdm);
> +		if (state > pwrst->next_state) {
> +			pr_info("Powerdomain (%s) didn't enter "
> +			       "target state %d\n",
> +			       pwrst->pwrdm->name, pwrst->next_state);
> +			ret = -1;
> +		}
> +		omap_set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state);
> +	}
> +	if (ret)
> +		pr_err("Could not enter target state in pm_suspend\n");

Without more details, this isn't terribly useful.  I'd suggest just
making the per-state one above pr_err().

> +	else
> +		pr_err("Successfully put all powerdomains to target state\n");

and this one pr_info.

>  	return 0;
>  }
>  
> @@ -97,14 +141,31 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
>  	if (!pwrdm->pwrsts)
>  		return 0;
>  
> +	/*
> +	 * Skip CPU0 and CPU1 power domains. CPU1 is programmed
> +	 * through hotplug path and CPU0 explicitly programmed
> +	 * further down in the code path
> +	 */
> +	if ((!strcmp(pwrdm->name, "cpu0_pwrdm")) ||
> +		(!strcmp(pwrdm->name, "cpu1_pwrdm")))

or just one compare using strncmp(pwrdm->name, "cpu", 3)

> +		return 0;
> +
> +	/*
> +	 * FIXME: Remove this check when core retention is supported
> +	 * Only MPUSS power domain is added in the list.
> +	 */
> +	if (strcmp(pwrdm->name, "mpu_pwrdm"))
> +		return 0;
> +
>  	pwrst = kmalloc(sizeof(struct power_state), GFP_ATOMIC);
>  	if (!pwrst)
>  		return -ENOMEM;
> +
>  	pwrst->pwrdm = pwrdm;
> -	pwrst->next_state = PWRDM_POWER_ON;
> +	pwrst->next_state = PWRDM_POWER_RET;
>  	list_add(&pwrst->node, &pwrst_list);
>  
> -	return pwrdm_set_next_pwrst(pwrst->pwrdm, pwrst->next_state);
> +	return omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
>  }
>  
>  /**

Kevin