Hi Tero,
I have a few remarks regarding the genericity of this code. I think it
is better if the code in powerdomain.c stays generic and that the
platform specific checks and operations be moved in the specific
functions (via the pwrdm_ops struct).
On Tue, Jun 12, 2012 at 5:31 PM, Tero Kristo <t-kristo@xxxxxx> wrote:
> On OMAP4+, device wide off-mode has its own enable mechanism in addition
> to powerdomain target states. This patch adds support for this on top
> of functional power states by overloading the OFF state for core pwrdm.
> On pwrdm level, the deepest power state supported by core pwrdm is OSWR.
> When user (e.g. suspend) programs core pwrdm target as OFF, the functional
> power state for the domain will be OSWR with the additional device off
> enabled. Previous power state information will reflect this also.
>
> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> ---
> arch/arm/mach-omap2/powerdomain.c | 17 +++++++++
> arch/arm/mach-omap2/powerdomain.h | 13 +++++++-
> arch/arm/mach-omap2/powerdomain44xx.c | 48 +++++++++++++++++++++++++++
> arch/arm/mach-omap2/powerdomains44xx_data.c | 3 +-
> 4 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index ac63f86..78a9308 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -677,6 +677,8 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 func_pwrst)
> int sleep_switch = -1, ret = 0, hwsup = 0;
> int new_func_pwrst, next_func_pwrst, pwrst, logic;
> u8 curr_pwrst;
> + bool extra_off_enable = false;
> + bool has_extra_off = false;
>
> if (!pwrdm || IS_ERR(pwrdm)) {
> pr_debug("%s: invalid params: pwrdm=%p\n", __func__, pwrdm);
> @@ -687,6 +689,13 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 func_pwrst)
>
> mutex_lock(&pwrdm->lock);
>
> + /* Check if powerdomain has extra off mode handling */
> + if (pwrdm->flags & PWRDM_HAS_EXTRA_OFF_ENABLE) {
> + has_extra_off = true;
> + if (func_pwrst == PWRDM_FUNC_PWRST_OFF)
> + extra_off_enable = true;
> + }
Could those checks be moved in the OMAP4 specific functions, so that
the power domain code stays generic?
> +
> new_func_pwrst = pwrdm_get_achievable_func_pwrst(pwrdm, func_pwrst);
> pwrst = pwrdm_func_to_pwrst(pwrdm, new_func_pwrst);
> logic = pwrdm_func_to_logic_pwrst(pwrdm, new_func_pwrst);
> @@ -741,6 +750,9 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 func_pwrst)
> break;
> }
>
> + if (has_extra_off && arch_pwrdm->pwrdm_enable_off)
> + arch_pwrdm->pwrdm_enable_off(pwrdm, extra_off_enable);
> +
> out:
> mutex_unlock(&pwrdm->lock);
> return ret;
> @@ -810,6 +822,11 @@ int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm)
> int next_pwrst = pwrdm_read_next_pwrst(pwrdm);
> int next_logic = pwrdm_read_logic_retst(pwrdm);
>
> + if (pwrdm->flags & PWRDM_HAS_EXTRA_OFF_ENABLE &&
> + arch_pwrdm->pwrdm_read_next_off &&
> + arch_pwrdm->pwrdm_read_next_off(pwrdm))
> + return PWRDM_FUNC_PWRST_OFF;
> +
Same comment here. The OMAP4 specific function for
pwrdm_read_next_pwrst could return PWRDM_FUNC_PWRST_OFF.
> return pwrdm_pwrst_to_func(pwrdm, next_pwrst, next_logic);
> }
>
> diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
> index 0729d91..cc30b94 100644
> --- a/arch/arm/mach-omap2/powerdomain.h
> +++ b/arch/arm/mach-omap2/powerdomain.h
> @@ -75,6 +75,13 @@
> * state without waking up the
> * powerdomain
> */
> +/*
> + * OMAP4+ has device off feature, which must be enabled separately in
> + * addition to power domain next state setup. This feature is overloaded
> + * as EXTRA_OFF_ENABLE for core_pwrdm, and is implemented on top of
> + * functional power state
> + */
> +#define PWRDM_HAS_EXTRA_OFF_ENABLE (1 << 3)
>
> /*
> * Number of memory banks that are power-controllable. On OMAP4430, the
> @@ -173,7 +180,9 @@ struct powerdomain {
> * @pwrdm_disable_hdwr_sar: Disable Hardware Save-Restore feature for a pd
> * @pwrdm_set_lowpwrstchange: Enable pd transitions from a shallow to deep sleep
> * @pwrdm_wait_transition: Wait for a pd state transition to complete
> - * @pwrdm_lost_context_rff: Check if pd has lost RFF context (entered off)
> + * @pwrdm_lost_context_rff: Check if pd has lost RFF context (omap4+ device off)
> + * @pwrdm_enable_off: Extra off mode enable for pd (omap4+ device off)
> + * @pwrdm_read_next_off: Check if pd next state is off (omap4+ device off)
> */
> struct pwrdm_ops {
> int (*pwrdm_func_to_pwrst)(struct powerdomain *pwrdm, u8 func_pwrst);
> @@ -199,6 +208,8 @@ struct pwrdm_ops {
> int (*pwrdm_set_lowpwrstchange)(struct powerdomain *pwrdm);
> int (*pwrdm_wait_transition)(struct powerdomain *pwrdm);
> bool (*pwrdm_lost_context_rff)(struct powerdomain *pwrdm);
> + void (*pwrdm_enable_off)(struct powerdomain *pwrdm, bool enable);
> + bool (*pwrdm_read_next_off)(struct powerdomain *pwrdm);
> };
>
> int pwrdm_register_platform_funcs(struct pwrdm_ops *custom_funcs);
> diff --git a/arch/arm/mach-omap2/powerdomain44xx.c b/arch/arm/mach-omap2/powerdomain44xx.c
> index 562f78a..b2bdc0f 100644
> --- a/arch/arm/mach-omap2/powerdomain44xx.c
> +++ b/arch/arm/mach-omap2/powerdomain44xx.c
> @@ -358,6 +358,52 @@ bool omap4_pwrdm_lost_context_rff(struct powerdomain *pwrdm)
> return false;
> }
>
> +/**
> + * omap4_device_set_next_state_off - setup device off state
> + * @pwrdm: struct powerdomain * to target powerdomain
> + * @enable: true if off-mode should be enabled
> + *
> + * When Device OFF is enabled, Device is allowed to perform
> + * transition to off mode as soon as all power domains in MPU, IVA
> + * and CORE voltage are in OFF or OSWR state (open switch retention)
> + */
> +void omap4_device_set_next_state_off(struct powerdomain *pwrdm, bool enable)
> +{
> + u8 val = enable ? 0x1 : 0x0;
> +
> + if (!(pwrdm->flags & PWRDM_HAS_EXTRA_OFF_ENABLE))
> + return;
> +
> + omap4_prminst_write_inst_reg(val << OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> + OMAP4430_PRM_PARTITION,
> + OMAP4430_PRM_DEVICE_INST,
> + OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> +}
> +
> +
> +/**
> + * omap4_device_read_next_state_off - read device off state
> + * @pwrdm: struct powerdomain * to target powerdomain
> + *
> + * Checks if device off is enabled or not.
> + * Returns true if enabled, false otherwise.
> + */
> +bool omap4_device_read_next_state_off(struct powerdomain *pwrdm)
> +{
> + u32 val;
> +
> + if (!(pwrdm->flags & PWRDM_HAS_EXTRA_OFF_ENABLE))
> + return false;
> +
> + val = omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
> + OMAP4430_PRM_DEVICE_INST,
> + OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> +
> + val &= OMAP4430_DEVICE_OFF_ENABLE_MASK;
> +
> + return val ? true : false;
> +}
> +
> struct pwrdm_ops omap4_pwrdm_operations = {
> .pwrdm_func_to_pwrst = omap2_pwrdm_func_to_pwrst,
> .pwrdm_func_to_logic_pwrst = omap2_pwrdm_func_to_logic_pwrst,
> @@ -381,4 +427,6 @@ struct pwrdm_ops omap4_pwrdm_operations = {
> .pwrdm_enable_hdwr_sar = omap4_pwrdm_enable_hdwr_sar,
> .pwrdm_disable_hdwr_sar = omap4_pwrdm_disable_hdwr_sar,
> .pwrdm_lost_context_rff = omap4_pwrdm_lost_context_rff,
> + .pwrdm_enable_off = omap4_device_set_next_state_off,
> + .pwrdm_read_next_off = omap4_device_read_next_state_off,
> };
> diff --git a/arch/arm/mach-omap2/powerdomains44xx_data.c b/arch/arm/mach-omap2/powerdomains44xx_data.c
> index c4de02f..13db876 100644
> --- a/arch/arm/mach-omap2/powerdomains44xx_data.c
> +++ b/arch/arm/mach-omap2/powerdomains44xx_data.c
> @@ -54,7 +54,8 @@ static struct powerdomain core_44xx_pwrdm = {
> [3] = PWRSTS_ON, /* ducati_l2ram */
> [4] = PWRSTS_ON, /* ducati_unicache */
> },
> - .flags = PWRDM_HAS_LOWPOWERSTATECHANGE,
> + .flags = PWRDM_HAS_LOWPOWERSTATECHANGE |
> + PWRDM_HAS_EXTRA_OFF_ENABLE,
> };
>
> /* gfx_44xx_pwrdm: 3D accelerator power domain */
Regards,
Jean
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
