RE: [PATCH v2] AM3517 : support for suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



hi Kevin

Thanks for reviewing the patch.

On Fri, Sep 23, 2011 at 3:25 AM, Hilman, Kevin wrote:
>
> Abhilash K V <abhilash.kv@xxxxxx> writes:
>
>> This patch-set adds support for suspension to RAM and
>> resumption on the AM3517. This includes:
>> 1. Patch to disable dynamic sleep (as it is not supported
>>    on AM35xx).
>
> This should still be a separate patch, with justification.  More on this
> below.
[Abhilash K V] OK
>
>> 2. Imported the unique suspend/resume sequence for AM3517,
>>    contained in the new file arch/arm/mach-omap2/sleep3517.S.
>>
>> Caveat: If "no_console_suspend" is enabled (via boot-args),the
>> device doesnot resume but simply hangs.
>> Kevin's fix below should fix this:
>>  http://marc.info/?l=linux-omap&m=131593828001388&w=2#1
>
> should fix?  I assumed you tested it along with this.
[Abhilash K V] I've tested that it does; pardon my choice of words.
>
>> Signed-off-by: Ranjith Lohithakshan <ranjithl@xxxxxx>
>> Reviewed-by: Vaibhav Hiremath <hvaibhav@xxxxxx>
>> Signed-off-by: Abhilash K V <abhilash.kv@xxxxxx>
>> ---
>> This patch is dependent on the following patch-sets:
>> * [PATCH v3 0/2] AM3517: Booting up
>>   at http://marc.info/?l=linux-omap&m=131548349725176&w=2
>> * [PATCH v2 0/3] AM35x: Adding PM init
>>   at http://marc.info/?l=linux-kernel&m=131548606728209&w=2
>>
>> The patches are tested on master of tmlind/linux-omap-2.6.git.
>> Kernel version is 3.1-rc3 and last commit on top of which these patches
>> were added is:
>>       b148d763841161894ed6629794064065a834aa2b: Linux-omap rebuilt: Updated to
>>       use omap_sdrc_init
>>
>> with the folowing commit reverted:
>>       f3637a5f2e2eb391ff5757bc83fb5de8f9726464: irq: Always set IRQF_ONESHOT
>>       if no primary handler is specified
>>
>> Changes in v2:
>>  * Synchronised with the cleaned-up suspend-resume code for OMAP3
>>  * Removed unused *_get_restore_pointer code
>>  * Added SECURE_SRAM feature to disallow saving and restoring
>>    secure ram context for AM35x
>
> Adding a new feature flag should be a separate patch.
>
>>  * Compacted the number of patches by squashing three closely coupled
>>    ones and eliminating one that was no longer needed.
>>
>>  arch/arm/mach-omap2/Makefile          |    3 +-
>>  arch/arm/mach-omap2/id.c              |    4 +-
>>  arch/arm/mach-omap2/pm.h              |    3 +
>>  arch/arm/mach-omap2/pm34xx.c          |   21 ++++-
>>  arch/arm/mach-omap2/sleep3517.S       |  156 +++++++++++++++++++++++++++++++++
>>  arch/arm/plat-omap/include/plat/cpu.h |    2 +
>>  6 files changed, 183 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/arm/mach-omap2/sleep3517.S
>>
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 590e797..37f62ae 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>> @@ -61,7 +61,7 @@ endif
>>  ifeq ($(CONFIG_PM),y)
>>  obj-$(CONFIG_ARCH_OMAP2)             += pm24xx.o
>>  obj-$(CONFIG_ARCH_OMAP2)             += sleep24xx.o
>> -obj-$(CONFIG_ARCH_OMAP3)             += pm34xx.o sleep34xx.o \
>> +obj-$(CONFIG_ARCH_OMAP3)             += pm34xx.o sleep34xx.o sleep3517.o \
>>                                          cpuidle34xx.o
>>  obj-$(CONFIG_ARCH_OMAP4)             += pm44xx.o
>>  obj-$(CONFIG_PM_DEBUG)                       += pm-debug.o
>> @@ -70,6 +70,7 @@ obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3)       += smartreflex-class3.o
>>
>>  AFLAGS_sleep24xx.o                   :=-Wa,-march=armv6
>>  AFLAGS_sleep34xx.o                   :=-Wa,-march=armv7-a$(plus_sec)
>> +AFLAGS_sleep3517.o                   :=-Wa,-march=armv7-a$(plus_sec)
>>
>>  ifeq ($(CONFIG_PM_VERBOSE),y)
>>  CFLAGS_pm_bus.o                              += -DDEBUG
>> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
>> index da71098..3e40c02 100644
>> --- a/arch/arm/mach-omap2/id.c
>> +++ b/arch/arm/mach-omap2/id.c
>> @@ -202,7 +202,9 @@ static void __init omap3_check_features(void)
>>       if (cpu_is_omap3630())
>>               omap_features |= OMAP3_HAS_192MHZ_CLK;
>>       if (!cpu_is_omap3505() && !cpu_is_omap3517())
>> -             omap_features |= (OMAP3_HAS_IO_WAKEUP | OMAP3_HAS_SR);
>> +             omap_features |= (OMAP3_HAS_IO_WAKEUP
>> +                                     | OMAP3_HAS_SR
>> +                                     | OMAP3_HAS_SECURE_SRAM);
>
> This is not related to suspend either, and should probably be part of
> the bootup series.
>
> The HAS_SECURE_SRAM part should be added to the separate patch that adds
> this new feature flag.
>
>>       omap_features |= OMAP3_HAS_SDRC;
>>
>> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
>> index ce028f6..952eb2b 100644
>> --- a/arch/arm/mach-omap2/pm.h
>> +++ b/arch/arm/mach-omap2/pm.h
>> @@ -82,10 +82,13 @@ extern unsigned int omap24xx_cpu_suspend_sz;
>>
>>  /* 3xxx */
>>  extern void omap34xx_cpu_suspend(int save_state);
>> +extern void omap3517_cpu_suspend(int save_state);
>>
>>  /* omap3_do_wfi function pointer and size, for copy to SRAM */
>>  extern void omap3_do_wfi(void);
>> +extern void omap3517_do_wfi(void);
>>  extern unsigned int omap3_do_wfi_sz;
>> +extern unsigned int omap3517_do_wfi_sz;
>>  /* ... and its pointer from SRAM after copy */
>>  extern void (*omap3_do_wfi_sram)(void);
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index 7255d9b..44f7bac 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -80,6 +80,7 @@ static LIST_HEAD(pwrst_list);
>>
>>  static int (*_omap_save_secure_sram)(u32 *addr);
>>  void (*omap3_do_wfi_sram)(void);
>> +void (*omap3517_do_wfi_sram)(void);
>>
>>  static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
>>  static struct powerdomain *core_pwrdm, *per_pwrdm;
>> @@ -323,7 +324,10 @@ static void omap34xx_save_context(u32 *save)
>>
>>  static int omap34xx_do_sram_idle(unsigned long save_state)
>>  {
>> -     omap34xx_cpu_suspend(save_state);
>> +     if (cpu_is_omap3505() || cpu_is_omap3517())
>> +             omap3517_cpu_suspend(save_state);
>> +     else
>> +             omap34xx_cpu_suspend(save_state);
>
> We don't want cpu_is_* checks at runtime.  Make this a function pointer
> that is initialized at init time based on SoC.
>
>>       return 0;
>>  }
>>
>> @@ -485,6 +489,8 @@ console_still_active:
>>
>>  int omap3_can_sleep(void)
>>  {
>> +     if (cpu_is_omap3505() || cpu_is_omap3517())
>> +             return 0;
>
> This needs to be a separate patch with a descriptive changelog and
> justification as to why you can't do WFI in idle.
[Abhilash K V]OK
>
> Adding something like this means the device will *never* attempt a WFI
> during idle.
[Abhilash K V] This patch was put in as dynamic sleep feature is not supported
by the device, there are no C states etc. The only PM supported is forced suspend
/resume. There is just one power-domain and it can be in ON or RET states.
>
> I suspect that avoiding WFI in idle is masking a bug that you don't see
> in the suspend path.
[Abhilash K V] I need to recap a bit to find out if there is a better way to
indicate the lack of "idle" feature.
>
>>       if (!omap_uart_can_sleep())
>>               return 0;
>>       return 1;
>> @@ -843,11 +849,18 @@ static int __init clkdms_setup(struct clockdomain *clkdm, void *unused)
>>   */
>>  void omap_push_sram_idle(void)
>>  {
>> -     omap3_do_wfi_sram = omap_sram_push(omap3_do_wfi, omap3_do_wfi_sz);
>> +     if (cpu_is_omap3505() || cpu_is_omap3517())
>> +             omap3517_do_wfi_sram = omap_sram_push(omap3517_do_wfi,
>> +                                              omap3517_do_wfi_sz);
>> +     else
>> +             omap3_do_wfi_sram = omap_sram_push(omap3_do_wfi,
>> +                                              omap3_do_wfi_sz);
>>
>>       if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>> -             _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
>> -                             save_secure_ram_context_sz);
>> +             if (omap3_has_secure_sram())
>> +                     _omap_save_secure_sram = omap_sram_push(
>> +                                             save_secure_ram_context,
>> +                                             save_secure_ram_context_sz);
>
> This should be part of the separate patch that add the secure SRAM
> feature flag.
[Abhilash K V] OK
>
> Kevin
>

--
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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux