On Saturday 02 March 2013 05:55 AM, Nishanth Menon wrote:
> On 17:41-20130301, Santosh Shilimkar wrote:
>> The OMAP5 idle driver can re-use OMAP4 CPUidle driver implementation thanks
>> to compatible MPUSS design.
>>
>> Though unlike OMAP4, on OMAP5 devices, MPUSS CSWR (Close Switch Retention)
>> power states can be achieved with respective power states on CPU0 and CPU1
>> power domain. This mode was broken on OMAP4 devices because of hardware
>> limitation. Also there is no CPU low power entry order requirement like
>> master CPU etc for CSWR C-state, which is icing on the cake. Code makes
>> use of voting scheme for cluster low power state to support MPUSS CSWR
>> C-state.
>>
>> Supported OMAP5 CPUidle C-states:
>> C1 - CPU0 ON(WFI) + CPU1 ON(WFI) + MPUSS ON
>> C2 - CPU0 CSWR + CPU1 CSWR + MPUSS CSWR
>> C3 - CPU0 OFF + CPU1 Force OFF + MPUSS OSWR
> think you meant CPU1 OFF?
Yes.
>>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
>> ---
>> arch/arm/mach-omap2/Kconfig | 1 +
>> arch/arm/mach-omap2/Makefile | 4 +-
>> .../{cpuidle44xx.c => cpuidle_omap4plus.c} | 103 +++++++++++++++++++-
>> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 7 +-
>> arch/arm/mach-omap2/pm_omap4plus.c | 2 +-
>> 5 files changed, 110 insertions(+), 7 deletions(-)
>> rename arch/arm/mach-omap2/{cpuidle44xx.c => cpuidle_omap4plus.c} (70%)
>>
>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>> index 41b581f..2df91dc 100644
>> --- a/arch/arm/mach-omap2/Kconfig
>> +++ b/arch/arm/mach-omap2/Kconfig
>> @@ -82,6 +82,7 @@ config SOC_OMAP5
>> select CPU_V7
>> select HAVE_SMP
>> select COMMON_CLK
>> + select ARCH_NEEDS_CPU_IDLE_COUPLED
>>
>> comment "OMAP Core Type"
>> depends on ARCH_OMAP2
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 7c3c6b6..f6ff88f 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>> @@ -97,8 +97,10 @@ endif
>> endif
>>
>> ifeq ($(CONFIG_CPU_IDLE),y)
>> +omap4plus-common-idle = cpuidle_omap4plus.o
>> obj-$(CONFIG_ARCH_OMAP3) += cpuidle34xx.o
>> -obj-$(CONFIG_ARCH_OMAP4) += cpuidle44xx.o
>> +obj-$(CONFIG_ARCH_OMAP4) += $(omap4plus-common-idle)
>> +obj-$(CONFIG_SOC_OMAP5) += $(omap4plus-common-idle)
> nit pick: simpler to use cpuidle_omap4plus.o or do we expect more objs -
> like moving the idle_data to DTS or so?
No I haven't that on that path. Easy from maintenance point of view. common
object and a label. Thats all.
>> endif
>>
>> # PRCM
>> diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle_omap4plus.c
>> similarity index 70%
>> rename from arch/arm/mach-omap2/cpuidle44xx.c
>> rename to arch/arm/mach-omap2/cpuidle_omap4plus.c
>> index 23286c1..8681fa6 100644
>> --- a/arch/arm/mach-omap2/cpuidle44xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle_omap4plus.c
>> @@ -29,6 +29,7 @@ struct idle_statedata {
>> u32 cpu_state;
>> u32 mpu_logic_state;
>> u32 mpu_state;
>> + u32 mpu_state_vote;
>> };
>>
>> static struct idle_statedata omap4_idle_data[] = {
>> @@ -49,17 +50,36 @@ static struct idle_statedata omap4_idle_data[] = {
>> },
>> };
>>
>> +static struct idle_statedata omap5_idle_data[] = {
>> + {
>> + .cpu_state = PWRDM_POWER_ON,
>> + .mpu_state = PWRDM_POWER_ON,
>> + .mpu_logic_state = PWRDM_POWER_RET,
>> + },
>> + {
>> + .cpu_state = PWRDM_POWER_RET,
> CSWR I assume -> Documenting it helps? or we should introdce
> cpu_logic_state to be explicit?
Its is documented just below the table(desc). No need to add useless
cpu_logic_state variable. This is part of the big C-state table.
>
[..]
>> /*
>> * For each cpu, setup the broadcast timer because local timers
>> * stops for the states above C1.
>> @@ -209,6 +262,43 @@ static struct cpuidle_driver omap4_idle_driver = {
>> .safe_state_index = 0,
>> };
>>
>> +static struct cpuidle_driver omap5_idle_driver = {
>> + .name = "omap5_idle",
>> + .owner = THIS_MODULE,
>> + .en_core_tk_irqen = 1,
>> + .states = {
>> + {
>> + /* C1 - CPU0 ON + CPU1 ON + MPU ON */
> we could move these to .desc
That will be too much ... also above just for refence and understanding
otherwise, CPU state is per CPU so we can't say CPU0 =x, and CPU1 = y in
CPU0 C-state. I like the way .desc is today and would want to keep it that
way.
>> + .exit_latency = 2 + 2,
>> + .target_residency = 5,
> the obvious question on how these latencies were arrived at..
Based on the internal measurements done only considering MPUSS
C-state latencies.
>> @@ -243,8 +333,13 @@ int __init omap4_idle_init(void)
>> #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
>> dev->coupled_cpus = *cpu_online_mask;
>> #endif
>> - state_ptr = &omap4_idle_data[0];
>> - cpuidle_register_driver(&omap4_idle_driver);
>> + if (cpu_is_omap44xx()) {
>> + state_ptr = &omap4_idle_data[0];
>> + cpuidle_register_driver(&omap4_idle_driver);
>> + } else if (soc_is_omap54xx()) {
>> + state_ptr = &omap5_idle_data[0];
>> + cpuidle_register_driver(&omap5_idle_driver);
>> + }
> we'd be doing cpuidle_register_driver twice since it is within the
> for_each_cpu loop - we dont want to do that.
Yep. I will move that out of the loop though second registration is just
a nop.
>>
>> if (cpuidle_register_device(dev)) {
>> pr_err("%s: CPUidle register failed\n", __func__);
>> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
>> index 5d32444..0ccb76e 100644
>> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
>> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
>> @@ -87,7 +87,7 @@ extern void omap5_cpu_resume(void);
>> static DEFINE_PER_CPU(struct omap4_cpu_pm_info, omap4_pm_info);
>> static struct powerdomain *mpuss_pd;
>> static void __iomem *sar_base;
>> -static u32 cpu_context_offset;
>> +static u32 cpu_context_offset, cpu_cswr_supported;
>>
>> static int default_finish_suspend(unsigned long cpu_state)
>> {
>> @@ -265,6 +265,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>> save_state = 1;
>> break;
>> case PWRDM_POWER_RET:
>> + if (cpu_cswr_supported) {
>> + save_state = 0;
>> + break;
>> + }
> ok, I guess this is the best we can do under the current circumstance
> where PWRDM_POWER_RET means either CSWR or OSWR!
The switch is for CPUx power states and hence there is no question of
OSWR. CPUx OSWR = CPUx OFF hence CPUx OSWR isn't supported. This has been
known from OMAP4 days. CPU RET isn't supported on OMAP4 and hence
that variable is added.
Regards,
Santosh
--
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
