Add basic suspend-resume support for AM33XX
Subsystems (9)
cover Add basic suspend-resume support for AM33XX
This patch series adds support for suspend-resume on AM33XX.
AM33XX family has a Cortex-M3 (WKUP_M3) which assists the MPU in DeepSleep0 entry and exit. WKUP_M3 takes care of the clockdomain and powerdomain transitions based on targeted intended low power state. MPU needs to load the appropriate WKUP_M3 binary onto the WKUP_M3 memory space before it can leverage any of the PM features.
The various low power modes and the overall s/w sequence is
documented in section 8.1.4.3 of the AM335x TRM available
on the TI website[1].
DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a complete reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate resume process.
The IPC mechanism between MPU and WKUP_M3 uses a mailbox sub-module and 8 IPC registers in the Control module. MPU populates the IPC registers for the payload and uses the assigned Mailbox for issuing an interrupt to WKUP_M3 which then goes and checks the IPC registers for the payload. WKUP_M3 has the ability to trigger on interrupt to MPU by executing the "sev" instruction.
In the current implementation when the suspend process is initiated MPU interrupts the WKUP_M3 to let about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received, MPU continues with its suspend process to suspend all the drivers and then jumps to assembly code in OCMC RAM to put the PLLs in bypass, put the external RAM in self-refresh mode and then finally execute the WFI instruction. The WFI instruction triggers another interrupt to the WKUP_M3 which then continues wiht the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt lines for the wakeup sources. When WKUP_M3 executes WFI, the hardware disables the main oscillator.
When a wakeup event occurs, WKUP_M3 starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence, in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence.
The first patch adds an API in the OMAP mailbox code to enable the MPU to clear the FIFO since WKUP_M3 does not have any access to the mailbox module. Minimal support for AM33XX family is added in the 2nd patch and the 3rd patch changes the initcall level of the mailbox code since the PM init happens very early in the boot process.
The fourth patch updates the reset API for AM33XX to reflect the fact that there is a reset status bit to be checked.
The next four patches are update the AM33XX hwmod data to register OCMCRAM, WKUP_M3 and fix up a couple of issues related to TPTC and CPGMAC data.
The next patch gets rid of some header files included in the AM33XX code and also adds the appropriate macro to ensure that the files can be included in assembly code.
Some control module registers which are required in the assembly code are added in the next patch.
The next two patches are related to the timer code. First the clocksource and clockevent timers are interchanged to ensure that have a free-running counter in suspend and then suspend-resume support for the clockevent timers added.
The device tree data for AM33XX is updated to register the OCMC and WKUP_M3 modules and then OMAP2 defconfig updated to include mailbox support which is required by the PM code.
The last patch builds upon all the work done in the other patches to implement suspend-resume functionality.
These patches apply on top the current linux-omap master branch cc2b5d5 + the HWMOD patch for CPSW [3] and has been tested on the AM335x EVM and the BeagleBone. Some of the patches will need minor rework to address the changes to the header files being done by Tony and the timer changes done by Jon Hunter.
With these dependencies met, the PM code uses the firmware interface and expects the userspace to load the WKUP_M3 binary before the suspend-resume functionality is made available. The binary file (and the source-code for WKUP_M3) can be obtained from the HEAD of the master branch at [2].
A sample usage of the suspend-resume process is shown below... (the binary from [2] is renamed to binary_blob and is included in the ramdisk used for booting)
[ 0.000000] Booting Linux on physical CPU 0 ... [ 1.873005] Power Management for AM33XX family [ 1.878365] Trying to load am335x-pm-firmware.bin [ 1.883264] ThumbEE CPU extension supported. [ 1.900231] clock: disabling unused clocks to save power [ 1.913804] drivers/rtc/hctosys.c: unable to open rtc device (rtc0) [ 1.924564] RAMDISK: gzip image found at block 0 [ 2.397241] VFS: Mounted root (ext2 filesystem) on device 1:0. [ 2.404341] Freeing init memory: 324K
mount: mounting none on /var/shm failed: No such file or directory
::
:: Enabling hot-plug : [SUCCESS]
::
::
: Populating /dev : [SUCCESS]
[SUCCESS]
::
::
:: Setting PATH
::
: syslogd : [SUCCESS]
: telnetd : [SUCCESS]
Please press Enter to activate this console.
::
:: Setting shell environment ...
::
: Path
: Aliases
: Touch Screen
::
:: Done
::
[root at arago /]# echo 1 > ./sys/devices/ocp.2/wkup_m3.4/firmware/am335x-pm-firmware.bin/loading [root at arago /]# cat binary_blob > ./sys/devices/ocp.2/wkup_m3.4/firmware/am335x-pm-firmware.bin/data [root at arago /]# echo 0 > ./sys/devices/ocp.2/wkup_m3.4/firmware/am335x-pm-firmware.bin/loading
[ 28.770356] Copied the M3 firmware to UMEM [ 28.778178] omap_hwmod: wkup_m3: _wait_target_disable failed [root at arago /]# echo mem > /sys/power/state [ 32.959753] PM: Syncing filesystems ... done. [ 32.988434] Freezing user space processes ... (elapsed 0.02 seconds) done. [ 33.016120] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done. [ 33.047607] Suspending console(s) (use no_console_suspend to debug) [ 33.070420] PM: suspend of devices complete after 11.538 msecs [ 33.074095] PM: late suspend of devices complete after 3.651 msecs [ 33.079135] PM: noirq suspend of devices complete after 5.011 msecs [ 33.079196] Disabling non-boot CPUs ... [ 33.083923] GFX domain entered low power state [ 33.083973] Successfully transitioned to low power state [ 33.087132] PM: noirq resume of devices complete after 2.663 msecs [ 33.090629] PM: early resume of devices complete after 2.346 msecs [ 33.097607] PM: resume of devices complete after 6.951 msecs [ 33.155869] Restarting tasks ... done.
[root at arago /]#
All the patches have been run through checkpatch and i have also tried a few OMAP build configs that Paul uses to ensure that these patches do not introduce any new build warnings/errors for the OMAP family.
A few TODOs
0. Test all the patches on OMAP platforms like BeagleBoard
1. Add support for DDR3 in the assembly code
2. Update the DTB documentation for OCMCRAM and WKUP_M3 nodes.
Regards,
Vaibhav
[1] http://www.ti.com/litv/pdf/spruh73f
[2] http://arago-project.org/git/projects/?p=am33x-cm3.git;a=summary
[3] https://patchwork.kernel.org/patch/1661771/
Vaibhav Bedia (15):
ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
ARM: OMAP2+: mailbox: Add support for AM33XX
ARM: OMAP: mailbox: Convert to device_initcall
ARM: OMAP2+: hwmod: Update the reset API for AM33XX
ARM: OMAP2+: AM33XX: Update WKUP_M3 hwmod entry for reset status
ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
ARM: OMAP2+: hwmod: Update the hwmod data for TPTCs in AM33XX
ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
ARM: OMAP: AM33XX: Remove unnecessary include and use __ASSEMBLER__
macros
ARM: OMAP2+: control: Add some AM33XX Control module registers
ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
ARM: OMAP2+: AM33XX: Basic suspend resume support
arch/arm/boot/dts/am33xx.dtsi | 11 + arch/arm/configs/omap2plus_defconfig | 2 + arch/arm/mach-omap2/Makefile | 2 + arch/arm/mach-omap2/board-generic.c | 1 + arch/arm/mach-omap2/clock33xx_data.c | 1 + arch/arm/mach-omap2/cm33xx.h | 10 +- arch/arm/mach-omap2/common.h | 10 + arch/arm/mach-omap2/control.h | 29 ++ arch/arm/mach-omap2/io.c | 7 + arch/arm/mach-omap2/mailbox.c | 79 +++- arch/arm/mach-omap2/omap_hwmod.c | 5 +- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 15 +- arch/arm/mach-omap2/pm.h | 7 + arch/arm/mach-omap2/pm33xx.c | 429 +++++++++++++++++++++ arch/arm/mach-omap2/pm33xx.h | 100 +++++ arch/arm/mach-omap2/prm33xx.c | 15 +- arch/arm/mach-omap2/prm33xx.h | 4 +- arch/arm/mach-omap2/sleep33xx.S | 571 ++++++++++++++++++++++++++++ arch/arm/mach-omap2/timer.c | 33 ++- arch/arm/plat-omap/include/plat/mailbox.h | 3 + arch/arm/plat-omap/mailbox.c | 35 ++ arch/arm/plat-omap/sram.c | 10 +- arch/arm/plat-omap/sram.h | 2 + 23 files changed, 1343 insertions(+), 38 deletions(-) create mode 100644 arch/arm/mach-omap2/pm33xx.c create mode 100644 arch/arm/mach-omap2/pm33xx.h create mode 100644 arch/arm/mach-omap2/sleep33xx.S
PATCH 1/15 ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
On AM33XX, the mailbox module between the MPU and the WKUP-M3 co-processor facilitates a one-way communication. MPU uses the assigned mailbox sub-module to issue the interrupt to the WKUP-M3 co-processor which then goes and reads the the IPC data from registers in the control module.
WKUP-M3 is in the L4_WKUP and does not have any access to the Mailbox module. Due to this limitation, the MPU is completely responsible for FIFO maintenance and interrupt generation. MPU needs to ensure that the FIFO does not overflow by reading by the assigned mailbox sub-module.
This patch adds an API in the mailbox code which the MPU can use to empty the FIFO by issuing a readback command.
--- arch/arm/mach-omap2/mailbox.c | 42 ++++++++++++++++++++--------- arch/arm/plat-omap/include/plat/mailbox.h | 3 ++ arch/arm/plat-omap/mailbox.c | 35 ++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 13 deletions(-)
arch/arm/mach-omap2/mailbox.c +29 -13
@@ -123,6 +123,20 @@ static int omap2_mbox_fifo_full(struct omap_mbox *mbox)return mbox_read_reg (fifo->fifo_stat); } +static int omap2_mbox_fifo_needs_flush (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; +return mbox_read_reg (fifo->msg_stat); +} + +static mbox_msg_t omap2_mbox_fifo_readback (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; +return (mbox_msg_t )mbox_read_reg (fifo->msg); +} +/* Mailbox IRQ handle functions */ static void omap2_mbox_enable_irq (struct omap_mbox *mbox,omap_mbox_type_t irq)
@@ -205,19 +219,21 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) }static struct omap_mbox_ops omap2_mbox_ops = { - .type =OMAP_MBOX_TYPE2 , - .startup = omap2_mbox_startup, - .shutdown = omap2_mbox_shutdown, - .fifo_read = omap2_mbox_fifo_read, - .fifo_write = omap2_mbox_fifo_write, - .fifo_empty = omap2_mbox_fifo_empty, - .fifo_full = omap2_mbox_fifo_full, - .enable_irq = omap2_mbox_enable_irq, - .disable_irq = omap2_mbox_disable_irq, - .ack_irq = omap2_mbox_ack_irq, - .is_irq = omap2_mbox_is_irq, - .save_ctx = omap2_mbox_save_ctx, - .restore_ctx = omap2_mbox_restore_ctx, + .type =OMAP_MBOX_TYPE2 , + .startup = omap2_mbox_startup, + .shutdown = omap2_mbox_shutdown, + .fifo_read = omap2_mbox_fifo_read, + .fifo_write = omap2_mbox_fifo_write, + .fifo_empty = omap2_mbox_fifo_empty, + .fifo_full = omap2_mbox_fifo_full, + .fifo_needs_flush = omap2_mbox_fifo_needs_flush, + .fifo_readback = omap2_mbox_fifo_readback, + .enable_irq = omap2_mbox_enable_irq, + .disable_irq = omap2_mbox_disable_irq, + .ack_irq = omap2_mbox_ack_irq, + .is_irq = omap2_mbox_is_irq, + .save_ctx = omap2_mbox_save_ctx, + .restore_ctx = omap2_mbox_restore_ctx, };/*
arch/arm/plat-omap/include/plat/mailbox.h +3 -0
@@ -29,6 +29,8 @@ struct omap_mbox_ops {void (*fifo_write)(struct omap_mbox *mbox,mbox_msg_t msg);int (*fifo_empty)(struct omap_mbox *mbox);int (*fifo_full)(struct omap_mbox *mbox); +int (*fifo_needs_flush)(struct omap_mbox *mbox); +mbox_msg_t (*fifo_readback)(struct omap_mbox *mbox);/* irq */ void (*enable_irq)(struct omap_mbox *mbox,omap_mbox_irq_t irq);
@@ -61,6 +63,7 @@ struct omap_mbox {struct blocking_notifier_head notifier; }; +int omap_mbox_msg_rx_flush (struct omap_mbox *mbox);int omap_mbox_msg_send (struct omap_mbox *,mbox_msg_t msg);void omap_mbox_init_seq (struct omap_mbox *);
arch/arm/plat-omap/mailbox.c +35 -0
@@ -59,6 +59,14 @@ static inline int mbox_fifo_full(struct omap_mbox *mbox) {return mbox->ops->fifo_full (mbox); } +static inline int mbox_fifo_needs_flush (struct omap_mbox *mbox) +{ +return mbox->ops->fifo_needs_flush (mbox); +} +static inline mbox_msg_t mbox_fifo_readback (struct omap_mbox *mbox) +{ +return mbox->ops->fifo_readback (mbox); +}/* Mailbox IRQ handle functions */ static inline void ack_mbox_irq (struct omap_mbox *mbox,omap_mbox_irq_t irq)
@@ -116,6 +124,33 @@ out: }EXPORT_SYMBOL (omap_mbox_msg_send); +/* +* Flush the Rx FIFO by reading back the messages +* Since the normal expectation is that the Rx will do the +* reading, add a debug message to indicate if we really flush +* +* Returns the no. of messages read back +*/ +int omap_mbox_msg_rx_flush (struct omap_mbox *mbox) +{ +int ret =0 ; +mbox_msg_t msg; + +while (mbox_fifo_needs_flush (mbox)) { + msg =mbox_fifo_readback (mbox); + ret++; + } + +if (ret) { +/* no more messages in the fifo. clear IRQ source. */ +ack_mbox_irq (mbox,IRQ_RX ); +pr_debug ("Flushed %s Rx FIFO by reading back\n" , mbox->name); + } + +return ret; +} +EXPORT_SYMBOL (omap_mbox_msg_rx_flush); +static void mbox_tx_tasklet (unsigned long tx_data) {struct omap_mbox *mbox = (struct omap_mbox *)tx_data;
Signature · 1 line
1.7.0.4
Tony Lindgren
maintainerPATCH 1/15 ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
Quoted 13 lines · Vaibhav Bedia
On AM33XX, the mailbox module between the MPU and the WKUP-M3 co-processor facilitates a one-way communication. MPU uses the assigned mailbox sub-module to issue the interrupt to the WKUP-M3 co-processor which then goes and reads the the IPC data from registers in the control module.
WKUP-M3 is in the
L4_WKUPand does not have any access to the Mailbox module. Due to this limitation, the MPU is completely responsible for FIFO maintenance and interrupt generation. MPU needs to ensure that the FIFO does not overflow by reading by the assigned mailbox sub-module.
Quoted 8 lines
This patch adds an API in the mailbox code which the MPU can use to empty the FIFO by issuing a readback command.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/mailbox.c | 42 ++++++++++++++++++++--------- arch/arm/plat-omap/include/plat/mailbox.h | 3 ++ arch/arm/plat-omap/mailbox.c | 35 ++++++++++++++++++++++++
Patches have been posted to move the mailbox related files out of arch/arm, so you'll have to update those for that. Please see thread "[PATCH 0/2] ARM: OMAP:
mailbox out of plat code" for more information.
Regards,
Tony
PATCH 1/15 ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
Hi Tony,
On Sat, Nov 03, 2012 at 00:30:04, Tony Lindgren wrote:
[...]
Quoted 6 lines
Patches have been posted to move the mailbox related files out of
arch/arm, so you'll have to update those for that. Please see thread "[PATCH 0/2] ARM: OMAP:
mailbox out of plat code" for more information.
Thanks for pointing this out. I'll update the patches for these.
I had used your master branch as the baseline. Some of the patches also need rework to account the header file and timer code changes. Is there some other branch of yours that I could use or should I just mention the dependencies in the next version of patches?
Regards,
Vaibhav
PATCH 1/15 ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
In reply to Vaibhav BediaQuoted 28 lines · Vaibhav Bedia
On AM33XX, the mailbox module between the MPU and the WKUP-M3 co-processor facilitates a one-way communication. MPU uses the assigned mailbox sub-module to issue the interrupt to the WKUP-M3 co-processor which then goes and reads the the IPC data from registers in the control module.
WKUP-M3 is in the
L4_WKUPand does not have any access to the Mailbox module. Due to this limitation, the MPU is completely responsible for FIFO maintenance and interrupt generation. MPU needs to ensure that the FIFO does not overflow by reading by the assigned mailbox sub-module.This patch adds an API in the mailbox code which the MPU can use to empty the FIFO by issuing a readback command.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/mailbox.c | 42 ++++++++++++++++++++--------- arch/arm/plat-omap/include/plat/mailbox.h | 3 ++ arch/arm/plat-omap/mailbox.c | 35 ++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 13 deletions(-)arch/arm/mach-omap2/mailbox.c +0
-0
arch/arm/mach-omap2/mailbox.c static int omap2_mbox_fifo_full(struct omap_mbox *mbox) · line 129
Quoted 7 lines
arch/arm/mach-omap2/mailbox.c +4
-0@@ -123,6 +123,20 @@ static int omap2_mbox_fifo_full(struct omap_mbox *mbox)return mbox_read_reg (fifo->fifo_stat); } +static int omap2_mbox_fifo_needs_flush (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo;
type casting is generally avoided in linux code.
arch/arm/mach-omap2/mailbox.c static int omap2_mbox_fifo_full(struct omap_mbox *mbox) · line 137
Quoted 8 lines
arch/arm/mach-omap2/mailbox.c +8
-0+return mbox_read_reg (fifo->msg_stat); +} + +static mbox_msg_t omap2_mbox_fifo_readback (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; +return (mbox_msg_t )mbox_read_reg (fifo->msg);
same here.
Quoted 27 lines
arch/arm/mach-omap2/mailbox.c +7
-13+} +/* Mailbox IRQ handle functions */ static void omap2_mbox_enable_irq (struct omap_mbox *mbox,omap_mbox_type_t irq)@@ -205,19 +219,21 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) }static struct omap_mbox_ops omap2_mbox_ops = { - .type =OMAP_MBOX_TYPE2 , - .startup = omap2_mbox_startup, - .shutdown = omap2_mbox_shutdown, - .fifo_read = omap2_mbox_fifo_read, - .fifo_write = omap2_mbox_fifo_write, - .fifo_empty = omap2_mbox_fifo_empty, - .fifo_full = omap2_mbox_fifo_full, - .enable_irq = omap2_mbox_enable_irq, - .disable_irq = omap2_mbox_disable_irq, - .ack_irq = omap2_mbox_ack_irq, - .is_irq = omap2_mbox_is_irq, - .save_ctx = omap2_mbox_save_ctx, - .restore_ctx = omap2_mbox_restore_ctx, + .type =OMAP_MBOX_TYPE2 , + .startup = omap2_mbox_startup, + .shutdown = omap2_mbox_shutdown, + .fifo_read = omap2_mbox_fifo_read, + .fifo_write = omap2_mbox_fifo_write,
arch/arm/mach-omap2/mailbox.c static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) · line 236
Quoted 10 lines
arch/arm/mach-omap2/mailbox.c +10
-0+ .fifo_empty = omap2_mbox_fifo_empty, + .fifo_full = omap2_mbox_fifo_full, + .fifo_needs_flush = omap2_mbox_fifo_needs_flush, + .fifo_readback = omap2_mbox_fifo_readback, + .enable_irq = omap2_mbox_enable_irq, + .disable_irq = omap2_mbox_disable_irq, + .ack_irq = omap2_mbox_ack_irq, + .is_irq = omap2_mbox_is_irq, + .save_ctx = omap2_mbox_save_ctx, + .restore_ctx = omap2_mbox_restore_ctx,
You should do the indentation fix in another patch.
arch/arm/plat-omap/include/plat/mailbox.h struct omap_mbox_ops { · line 33
Quoted 13 lines
arch/arm/mach-omap2/mailbox.c +0
-0};/* arch/arm/plat-omap/include/plat/mailbox.h +2
-0@@ -29,6 +29,8 @@ struct omap_mbox_ops {void (*fifo_write)(struct omap_mbox *mbox,mbox_msg_t msg);int (*fifo_empty)(struct omap_mbox *mbox);int (*fifo_full)(struct omap_mbox *mbox); +int (*fifo_needs_flush)(struct omap_mbox *mbox); +mbox_msg_t (*fifo_readback)(struct omap_mbox *mbox);
Do you think passing the msg structure as an argument and letting the function populate it will be better instead of returning the msg structure ? No strong opinion since from read_foo() point of view what you have done might be right thing. In either case, please get rid of typecasting.
Regards
Santosh
PATCH 1/15 ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
On Sat, Nov 03, 2012 at 21:33:47, Shilimkar, Santosh wrote:
[...]
Quoted 5 lines
arch/arm/mach-omap2/mailbox.c static int omap2_mbox_fifo_full(struct omap_mbox *mbox) · line 129
Quoted 4 lines
arch/arm/mach-omap2/mailbox.c +4
-0+static int omap2_mbox_fifo_needs_flush (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo;type casting is generally avoided in linux code.
I was just trying to be consistent with the rest of the mailbox FIFO related code :)
Will see how to get rid of these globally in the next version.
Quoted 9 lines
arch/arm/mach-omap2/mailbox.c static int omap2_mbox_fifo_full(struct omap_mbox *mbox) · line 137
Quoted 8 lines
arch/arm/mach-omap2/mailbox.c +8
-0+return mbox_read_reg (fifo->msg_stat); +} + +static mbox_msg_t omap2_mbox_fifo_readback (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; +return (mbox_msg_t )mbox_read_reg (fifo->msg);same here.
Ok.
Quoted 30 lines
Quoted 30 lines
arch/arm/mach-omap2/mailbox.c +10
-13+} +/* Mailbox IRQ handle functions */ static void omap2_mbox_enable_irq (struct omap_mbox *mbox,omap_mbox_type_t irq)@@ -205,19 +219,21 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) }static struct omap_mbox_ops omap2_mbox_ops = { - .type =OMAP_MBOX_TYPE2 , - .startup = omap2_mbox_startup, - .shutdown = omap2_mbox_shutdown, - .fifo_read = omap2_mbox_fifo_read, - .fifo_write = omap2_mbox_fifo_write, - .fifo_empty = omap2_mbox_fifo_empty, - .fifo_full = omap2_mbox_fifo_full, - .enable_irq = omap2_mbox_enable_irq, - .disable_irq = omap2_mbox_disable_irq, - .ack_irq = omap2_mbox_ack_irq, - .is_irq = omap2_mbox_is_irq, - .save_ctx = omap2_mbox_save_ctx, - .restore_ctx = omap2_mbox_restore_ctx, + .type =OMAP_MBOX_TYPE2 , + .startup = omap2_mbox_startup, + .shutdown = omap2_mbox_shutdown, + .fifo_read = omap2_mbox_fifo_read, + .fifo_write = omap2_mbox_fifo_write, + .fifo_empty = omap2_mbox_fifo_empty, + .fifo_full = omap2_mbox_fifo_full, + .fifo_needs_flush = omap2_mbox_fifo_needs_flush,
Quoted 9 lines
arch/arm/mach-omap2/mailbox.c static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) · line 236
Quoted 7 lines
arch/arm/mach-omap2/mailbox.c +7
-0+ .fifo_readback = omap2_mbox_fifo_readback, + .enable_irq = omap2_mbox_enable_irq, + .disable_irq = omap2_mbox_disable_irq, + .ack_irq = omap2_mbox_ack_irq, + .is_irq = omap2_mbox_is_irq, + .save_ctx = omap2_mbox_save_ctx, + .restore_ctx = omap2_mbox_restore_ctx,You should do the indentation fix in another patch.
Ok will split it up.
Quoted 19 lines
arch/arm/plat-omap/include/plat/mailbox.h struct omap_mbox_ops { · line 33
Quoted 13 lines
(quoted diff) +0
-0}; /*arch/arm/plat-omap/include/plat/mailbox.h +2
-0@@ -29,6 +29,8 @@ struct omap_mbox_ops {void (*fifo_write)(struct omap_mbox *mbox,mbox_msg_t msg);int (*fifo_empty)(struct omap_mbox *mbox);int (*fifo_full)(struct omap_mbox *mbox); +int (*fifo_needs_flush)(struct omap_mbox *mbox); +mbox_msg_t (*fifo_readback)(struct omap_mbox *mbox);Do you think passing the msg structure as an argument and letting the function populate it will be better instead of returning the msg structure ? No strong opinion since from
read_foo()point of view what you have done might be right thing. In either case, please get rid of typecasting.
Passing the msg structure looks fine. Will do that in the next version.
Regards,
Vaibhav
PATCH 1/15 ARM: OMAP2+: mailbox: Add an API for flushing the FIFO
Quoted 88 lines · Vaibhav
On Sat, Nov 03, 2012 at 21:33:47, Shilimkar, Santosh wrote:
[...]Quoted 5 lines
arch/arm/mach-omap2/mailbox.c static int omap2_mbox_fifo_full(struct omap_mbox *mbox) · line 129
Quoted 4 lines
arch/arm/mach-omap2/mailbox.c +4
-0+static int omap2_mbox_fifo_needs_flush (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo;type casting is generally avoided in linux code.
I was just trying to be consistent with the rest of the mailbox FIFO related code :)
Will see how to get rid of these globally in the next version.
Quoted 9 lines
arch/arm/mach-omap2/mailbox.c static int omap2_mbox_fifo_full(struct omap_mbox *mbox) · line 137
Quoted 8 lines
arch/arm/mach-omap2/mailbox.c +8
-0+return mbox_read_reg (fifo->msg_stat); +} + +static mbox_msg_t omap2_mbox_fifo_readback (struct omap_mbox *mbox) +{ +struct omap_mbox2_fifo *fifo = + &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; +return (mbox_msg_t )mbox_read_reg (fifo->msg);same here.
Ok.
Quoted 30 lines
Quoted 30 lines
arch/arm/mach-omap2/mailbox.c +10
-13+} +/* Mailbox IRQ handle functions */ static void omap2_mbox_enable_irq (struct omap_mbox *mbox,omap_mbox_type_t irq)@@ -205,19 +219,21 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) }static struct omap_mbox_ops omap2_mbox_ops = { - .type =OMAP_MBOX_TYPE2 , - .startup = omap2_mbox_startup, - .shutdown = omap2_mbox_shutdown, - .fifo_read = omap2_mbox_fifo_read, - .fifo_write = omap2_mbox_fifo_write, - .fifo_empty = omap2_mbox_fifo_empty, - .fifo_full = omap2_mbox_fifo_full, - .enable_irq = omap2_mbox_enable_irq, - .disable_irq = omap2_mbox_disable_irq, - .ack_irq = omap2_mbox_ack_irq, - .is_irq = omap2_mbox_is_irq, - .save_ctx = omap2_mbox_save_ctx, - .restore_ctx = omap2_mbox_restore_ctx, + .type =OMAP_MBOX_TYPE2 , + .startup = omap2_mbox_startup, + .shutdown = omap2_mbox_shutdown, + .fifo_read = omap2_mbox_fifo_read, + .fifo_write = omap2_mbox_fifo_write, + .fifo_empty = omap2_mbox_fifo_empty, + .fifo_full = omap2_mbox_fifo_full, + .fifo_needs_flush = omap2_mbox_fifo_needs_flush,Quoted 9 lines
arch/arm/mach-omap2/mailbox.c static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) · line 236
Quoted 7 lines
arch/arm/mach-omap2/mailbox.c +7
-0+ .fifo_readback = omap2_mbox_fifo_readback, + .enable_irq = omap2_mbox_enable_irq, + .disable_irq = omap2_mbox_disable_irq, + .ack_irq = omap2_mbox_ack_irq, + .is_irq = omap2_mbox_is_irq, + .save_ctx = omap2_mbox_save_ctx, + .restore_ctx = omap2_mbox_restore_ctx,You should do the indentation fix in another patch.
Ok will split it up.
Quoted 19 lines
arch/arm/plat-omap/include/plat/mailbox.h struct omap_mbox_ops { · line 33
Quoted 13 lines
(quoted diff) +0
-0}; /*arch/arm/plat-omap/include/plat/mailbox.h +2
-0@@ -29,6 +29,8 @@ struct omap_mbox_ops {void (*fifo_write)(struct omap_mbox *mbox,mbox_msg_t msg);int (*fifo_empty)(struct omap_mbox *mbox);int (*fifo_full)(struct omap_mbox *mbox); +int (*fifo_needs_flush)(struct omap_mbox *mbox); +mbox_msg_t (*fifo_readback)(struct omap_mbox *mbox);Do you think passing the msg structure as an argument and letting the function populate it will be better instead of returning the msg structure ? No strong opinion since from
read_foo()point of view what you have done might be right thing. In either case, please get rid of typecasting.
Quoted 2 lines
Passing the msg structure looks fine. Will do that in the next version.
OK
Regards
Santosh
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
In reply to Vaibhav BediaMailbox IP on AM33XX, is the same as that present in OMAP4. The single instance of Mailbox module contains 8 sub-modules and facilitates communication between MPU, PRUs and WKUP_M3.
The first mailbox sub-module is assigned for communication between MPU and WKUP-M3.
--- arch/arm/mach-omap2/mailbox.c | 35 ++++++++++++++++++++++++++++++++++- 1 files changed, 34 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/mailbox.c +34 -1
@@ -155,7 +155,7 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,struct omap_mbox2_priv *p = mbox->priv;u32 bit = (irq ==IRQ_TX ) ? p->notfull_bit : p->newmsg_bit; -if (!cpu_is_omap44xx ()) +if (!cpu_is_omap44xx () || !soc_is_am33xx ()) bit =mbox_read_reg (p->irqdisable) & ~bit;mbox_write_reg (bit, p->irqdisable);
@@ -358,6 +358,32 @@ struct omap_mbox mbox_2_info = {struct omap_mbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info,NULL };#endif +#if defined(CONFIG_SOC_AM33XX ) +static struct omap_mbox2_priv omap2_mbox_wkup_m3_priv = { + .tx_fifo = { + .msg =MAILBOX_MESSAGE (0 ), + .fifo_stat =MAILBOX_FIFOSTATUS (0 ), + .msg_stat =MAILBOX_MSGSTATUS (0 ), + }, + .rx_fifo = { + .msg =MAILBOX_MESSAGE (1 ), + .msg_stat =MAILBOX_MSGSTATUS (1 ), + }, + .irqenable =OMAP4_MAILBOX_IRQENABLE (3 ), + .irqstatus =OMAP4_MAILBOX_IRQSTATUS (3 ), + .irqdisable =OMAP4_MAILBOX_IRQENABLE_CLR (3 ), + .notfull_bit =MAILBOX_IRQ_NOTFULL (0 ), + .newmsg_bit =MAILBOX_IRQ_NEWMSG (0 ), +}; + +struct omap_mbox mbox_wkup_m3_info = { + .name ="wkup_m3" , + .ops = &omap2_mbox_ops, + .priv = &omap2_mbox_wkup_m3_priv, +}; +struct omap_mbox *am33xx_mboxes[] = { &mbox_wkup_m3_info,NULL }; +#endif +static int __devinit omap2_mbox_probe (struct platform_device *pdev) {struct resource *mem;
@@ -392,6 +418,13 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev) list[0 ]->irq = list[1 ]->irq =platform_get_irq (pdev,0 ); }#endif +#if defined(CONFIG_SOC_AM33XX ) + else if (soc_is_am33xx()) { + list = am33xx_mboxes; + + list[0 ]->irq =platform_get_irq (pdev,0 ); + } +#endif else {pr_err ("%s: platform not supported\n" , __func__);return -ENODEV ;
Signature · 1 line
1.7.0.4
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
Quoted 18 lines · Vaibhav Bedia
Mailbox IP on AM33XX, is the same as that present in OMAP4. The single instance of Mailbox module contains 8 sub-modules and facilitates communication between MPU, PRUs and
WKUP_M3.The first mailbox sub-module is assigned for
communication between MPU and WKUP-M3.Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/mailbox.c | 35 ++++++++++++++++++++++++++++++++++- 1 files changed, 34 insertions(+), 1 deletions(-)arch/arm/mach-omap2/mailbox.c +0
-0
arch/arm/mach-omap2/mailbox.c static void omap2_mbox_disable_irq(struct omap_mbox *mbox, · line 159
Quoted 6 lines
arch/arm/mach-omap2/mailbox.c +1
-1@@ -155,7 +155,7 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,struct omap_mbox2_priv *p = mbox->priv;u32 bit = (irq ==IRQ_TX ) ? p->notfull_bit : p->newmsg_bit; -if (!cpu_is_omap44xx ()) +if (!cpu_is_omap44xx () || !soc_is_am33xx ()) bit =mbox_read_reg (p->irqdisable) & ~bit;
Do you mean &&?
Quoted 55 lines
arch/arm/mach-omap2/mailbox.c +33
-1mbox_write_reg (bit, p->irqdisable);@@ -358,6 +358,32 @@ struct omap_mbox mbox_2_info = {struct omap_mbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info,NULL };#endif +#if defined(CONFIG_SOC_AM33XX ) +static struct omap_mbox2_priv omap2_mbox_wkup_m3_priv = { + .tx_fifo = { + .msg =MAILBOX_MESSAGE (0 ), + .fifo_stat =MAILBOX_FIFOSTATUS (0 ), + .msg_stat =MAILBOX_MSGSTATUS (0 ), + }, + .rx_fifo = { + .msg =MAILBOX_MESSAGE (1 ), + .msg_stat =MAILBOX_MSGSTATUS (1 ), + }, + .irqenable =OMAP4_MAILBOX_IRQENABLE (3 ), + .irqstatus =OMAP4_MAILBOX_IRQSTATUS (3 ), + .irqdisable =OMAP4_MAILBOX_IRQENABLE_CLR (3 ), + .notfull_bit =MAILBOX_IRQ_NOTFULL (0 ), + .newmsg_bit =MAILBOX_IRQ_NEWMSG (0 ), +}; + +struct omap_mbox mbox_wkup_m3_info = { + .name ="wkup_m3" , + .ops = &omap2_mbox_ops, + .priv = &omap2_mbox_wkup_m3_priv, +}; +struct omap_mbox *am33xx_mboxes[] = { &mbox_wkup_m3_info,NULL }; +#endif +static int __devinit omap2_mbox_probe (struct platform_device *pdev) {struct resource *mem;@@ -392,6 +418,13 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev) list[0 ]->irq = list[1 ]->irq =platform_get_irq (pdev,0 ); }#endif +#if defined(CONFIG_SOC_AM33XX ) + else if (soc_is_am33xx()) { + list = am33xx_mboxes; + + list[0 ]->irq =platform_get_irq (pdev,0 ); + } +#endif else {pr_err ("%s: platform not supported\n" , __func__);return -ENODEV ; --1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
Hi Russ,
On Sat, Nov 03, 2012 at 05:44:21, Russ Dill wrote:
[...]
Quoted 6 lines
arch/arm/mach-omap2/mailbox.c static void omap2_mbox_disable_irq(struct omap_mbox *mbox, · line 159
Quoted 3 lines
arch/arm/mach-omap2/mailbox.c +1
-1-if (!cpu_is_omap44xx ()) +if (!cpu_is_omap44xx () || !soc_is_am33xx ()) bit =mbox_read_reg (p->irqdisable) & ~bit;Do you mean &&?
I didn't change that line but it looks ok to me :)
Regards,
Vaibhav
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
In reply to Russ DillQuoted 11 lines · Vaibhav
Hi Russ,
On Sat, Nov 03, 2012 at 05:44:21, Russ Dill wrote:
[...]Quoted 6 lines
arch/arm/mach-omap2/mailbox.c static void omap2_mbox_disable_irq(struct omap_mbox *mbox, · line 159
Quoted 3 lines
arch/arm/mach-omap2/mailbox.c +1
-1-if (!cpu_is_omap44xx ()) +if (!cpu_is_omap44xx () || !soc_is_am33xx ()) bit =mbox_read_reg (p->irqdisable) & ~bit;Do you mean &&?
Ok I understood what you meant. Will fix it in next rev.
Regards,
Vaibhav
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
In reply to Vaibhav BediaQuoted 60 lines · Vaibhav Bedia
Mailbox IP on AM33XX, is the same as that present in OMAP4. The single instance of Mailbox module contains 8 sub-modules and facilitates communication between MPU, PRUs and
WKUP_M3.The first mailbox sub-module is assigned for communication between MPU and WKUP-M3.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/mailbox.c | 35 ++++++++++++++++++++++++++++++++++- 1 files changed, 34 insertions(+), 1 deletions(-)arch/arm/mach-omap2/mailbox.c +27
-1@@ -155,7 +155,7 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,struct omap_mbox2_priv *p = mbox->priv;u32 bit = (irq ==IRQ_TX ) ? p->notfull_bit : p->newmsg_bit; -if (!cpu_is_omap44xx ()) +if (!cpu_is_omap44xx () || !soc_is_am33xx ()) bit =mbox_read_reg (p->irqdisable) & ~bit;mbox_write_reg (bit, p->irqdisable);@@ -358,6 +358,32 @@ struct omap_mbox mbox_2_info = {struct omap_mbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info,NULL };#endif +#if defined(CONFIG_SOC_AM33XX ) +static struct omap_mbox2_priv omap2_mbox_wkup_m3_priv = { + .tx_fifo = { + .msg =MAILBOX_MESSAGE (0 ), + .fifo_stat =MAILBOX_FIFOSTATUS (0 ), + .msg_stat =MAILBOX_MSGSTATUS (0 ), + }, + .rx_fifo = { + .msg =MAILBOX_MESSAGE (1 ), + .msg_stat =MAILBOX_MSGSTATUS (1 ), + }, + .irqenable =OMAP4_MAILBOX_IRQENABLE (3 ), + .irqstatus =OMAP4_MAILBOX_IRQSTATUS (3 ), + .irqdisable =OMAP4_MAILBOX_IRQENABLE_CLR (3 ), + .notfull_bit =MAILBOX_IRQ_NOTFULL (0 ), + .newmsg_bit =MAILBOX_IRQ_NEWMSG (0 ), +}; + +struct omap_mbox mbox_wkup_m3_info = { + .name ="wkup_m3" , + .ops = &omap2_mbox_ops, + .priv = &omap2_mbox_wkup_m3_priv, +}; +struct omap_mbox *am33xx_mboxes[] = { &mbox_wkup_m3_info,NULL }; +#endif +static int __devinit omap2_mbox_probe (struct platform_device *pdev) {struct resource *mem;
arch/arm/mach-omap2/mailbox.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 427
Quoted 10 lines
arch/arm/mach-omap2/mailbox.c +7
-0@@ -392,6 +418,13 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev) list[0]->irq = list[1]->irq = platform_get_irq(pdev, 0); }#endif +#if defined(CONFIG_SOC_AM33XX ) + else if (soc_is_am33xx()) { + list = am33xx_mboxes; + + list[0 ]->irq =platform_get_irq (pdev,0 ); + } +#endif
#ifdef in middle of the function looks really ugly. But I can't complain just for your patch because looks like rest of the mailbox code is flooded with #ifdeffery.
Mailbox needs cleanup. and probably can be moved out of arch/arm/*omap*/ to some driver directory.
Regards
santosh
inside arch/arm/mach-omap2/* directory.
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
On Sat, Nov 03, 2012 at 21:40:37, Shilimkar, Santosh wrote:
[...]
Quoted 11 lines
arch/arm/mach-omap2/mailbox.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 427
Quoted 7 lines
arch/arm/mach-omap2/mailbox.c +7
-0+#if defined(CONFIG_SOC_AM33XX ) + else if (soc_is_am33xx()) { + list = am33xx_mboxes; + + list[0 ]->irq =platform_get_irq (pdev,0 ); + } +#endif #ifdef in middle of the function looks really ugly. But I can't complain just for your patch because looks like rest of the mailbox code is flooded with #ifdeffery.
Quoted 3 lines
Mailbox needs cleanup. and probably can be moved out of
arch/arm/*omap*/to some driver directory.
Tony pointed out that the movement to drivers directory is underway [1]. Getting rid of the #ifdeffery could be next.
Regards,
Vaibhav
PATCH 2/15 ARM: OMAP2+: mailbox: Add support for AM33XX
Quoted 14 lines · Vaibhav
On Sat, Nov 03, 2012 at 21:40:37, Shilimkar, Santosh wrote:
[...]Quoted 11 lines
arch/arm/mach-omap2/mailbox.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 427
Quoted 7 lines
arch/arm/mach-omap2/mailbox.c +7
-0+#if defined(CONFIG_SOC_AM33XX ) + else if (soc_is_am33xx()) { + list = am33xx_mboxes; + + list[0 ]->irq =platform_get_irq (pdev,0 ); + } +#endif #ifdef in middle of the function looks really ugly. But I can't complain just for your patch because looks like rest of the mailbox code is flooded with #ifdeffery.
Quoted 7 lines
Quoted 3 lines
Mailbox needs cleanup. and probably can be moved out of
arch/arm/*omap*/to some driver directory.Tony pointed out that the movement to drivers directory is underway [1]. Getting rid of the #ifdeffery could be next.
Thanks for the pointer. Nice to see mailbox is getting moved to drivers directory.
Regards
Santosh
PATCH 3/15 ARM: OMAP: mailbox: Convert to device_initcall
In reply to Vaibhav BediaThe power management code for AM33XX is a late_initcall and the PM features depend on the mailbox for IPC. In preparation for this, convert the mailbox init to a device_initcall.
--- arch/arm/mach-omap2/mailbox.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/mailbox.c +1 -1
@@ -469,7 +469,7 @@ static void __exit omap2_mbox_exit(void)platform_driver_unregister (&omap2_mbox_driver); } -module_init (omap2_mbox_init); +device_initcall (omap2_mbox_init);module_exit (omap2_mbox_exit);MODULE_LICENSE ("GPL v2" );
Signature · 1 line
1.7.0.4
PATCH 3/15 ARM: OMAP: mailbox: Convert to device_initcall
Quoted 7 lines · Vaibhav Bedia
The power management code for AM33XX is a
late_initcalland the PM features depend on the mailbox for IPC. In preparation for this, convert the mailbox init to adevice_initcall.Signed-off-by Vaibhav Bedia <[email protected]>---
Looks fine
PATCH 4/15 ARM: OMAP2+: hwmod: Update the reset API for AM33XX
In reply to Vaibhav BediaWKUP-M3 has a reset status bit (RM_WKUP_STST.WKUP_M3_LRST) Update the hardreset API to take care of the same to ensure that the reset line properly deasserted.
--- arch/arm/mach-omap2/omap_hwmod.c | 5 +---- arch/arm/mach-omap2/prm33xx.c | 15 +++++++-------- arch/arm/mach-omap2/prm33xx.h | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +1 -4
@@ -2944,11 +2944,8 @@ static int _am33xx_assert_hardreset(struct omap_hwmod *oh,static int _am33xx_deassert_hardreset (struct omap_hwmod *oh,struct omap_hwmod_rst_info *ohri) { -if (ohri->st_shift) -pr_err ("omap_hwmod: %s: %s: hwmod data error: OMAP4 does not support st_shift\n" , - oh->name, ohri->name); -return am33xx_prm_deassert_hardreset (ohri->rst_shift, + ohri->st_shift, oh->clkdm->pwrdm.ptr->prcm_offs, oh->prcm.omap4.rstctrl_offs, oh->prcm.omap4.rstst_offs);
arch/arm/mach-omap2/prm33xx.c +7 -8
@@ -112,23 +112,22 @@ int am33xx_prm_assert_hardreset(u8 shift, s16 inst, u16 rstctrl_offs)* -EINVAL upon an argument error, -EEXIST if the submodule was already out * of reset, or -EBUSY if the submodule did not exit reset promptly. */ -int am33xx_prm_deassert_hardreset (u8 shift,s16 inst, +int am33xx_prm_deassert_hardreset (u8 shift,u8 st_shift,s16 inst,u16 rstctrl_offs,u16 rstst_offs) {int c; -u32 mask =1 << shift; - -/* Check the current status to avoid de-asserting the line twice */ -if (am33xx_prm_is_hardreset_asserted (shift, inst, rstctrl_offs) ==0 ) -return -EEXIST ; +u32 mask =1 << st_shift;/* Clear the reset status by writing 1 to the status bit */ am33xx_prm_rmw_reg_bits (0xffffffff , mask, inst, rstst_offs); +/* de-assert the reset control line */ + mask =1 << shift; +am33xx_prm_rmw_reg_bits (mask,0 , inst, rstctrl_offs); -/* wait the status to be set */ -omap_test_timeout (am33xx_prm_is_hardreset_asserted (shift, inst, +/* wait the status to be set */ +omap_test_timeout (am33xx_prm_is_hardreset_asserted (st_shift, inst, rstst_offs),MAX_MODULE_HARDRESET_WAIT , c);
arch/arm/mach-omap2/prm33xx.h +1 -1
@@ -124,6 +124,6 @@ extern void am33xx_prm_global_warm_sw_reset(void);extern int am33xx_prm_is_hardreset_asserted (u8 shift,s16 inst,u16 rstctrl_offs);extern int am33xx_prm_assert_hardreset (u8 shift,s16 inst,u16 rstctrl_offs); -extern int am33xx_prm_deassert_hardreset (u8 shift,s16 inst, +extern int am33xx_prm_deassert_hardreset (u8 shift,u8 st_shift,s16 inst,u16 rstctrl_offs,u16 rstst_offs);#endif
Signature · 1 line
1.7.0.4
PATCH 4/15 ARM: OMAP2+: hwmod: Update the reset API for AM33XX
Quoted 4 lines · Vaibhav Bedia
WKUP-M3 has a reset status bit (RM_WKUP_STST.WKUP_M3_LRST) Update the hardreset API to take care of the same to ensure that the reset line properly deasserted.
Thanks for the patch, comments below -
Quoted 32 lines
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/omap_hwmod.c | 5 +---- arch/arm/mach-omap2/prm33xx.c | 15 +++++++-------- arch/arm/mach-omap2/prm33xx.h | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-)arch/arm/mach-omap2/omap_hwmod.c +1
-4@@ -2944,11 +2944,8 @@ static int _am33xx_assert_hardreset(struct omap_hwmod *oh,static int _am33xx_deassert_hardreset (struct omap_hwmod *oh,struct omap_hwmod_rst_info *ohri) { -if (ohri->st_shift) -pr_err ("omap_hwmod: %s: %s: hwmod data error: OMAP4 does not support st_shift\n" , - oh->name, ohri->name); -return am33xx_prm_deassert_hardreset (ohri->rst_shift, + ohri->st_shift, oh->clkdm->pwrdm.ptr->prcm_offs, oh->prcm.omap4.rstctrl_offs, oh->prcm.omap4.rstst_offs);arch/arm/mach-omap2/prm33xx.c +0
-0@@ -112,23 +112,22 @@ int am33xx_prm_assert_hardreset(u8 shift, s16 inst, u16 rstctrl_offs)* -EINVAL upon an argument error, -EEXIST if the submodule was already out * of reset, or -EBUSY if the submodule did not exit reset promptly. */
arch/arm/mach-omap2/prm33xx.c int am33xx_prm_assert_hardreset(u8 shift, s16 inst, u16 rstctrl_offs) · line 123
Quoted 10 lines
arch/arm/mach-omap2/prm33xx.c +1
-6-int am33xx_prm_deassert_hardreset (u8 shift,s16 inst, +int am33xx_prm_deassert_hardreset (u8 shift,u8 st_shift,s16 inst,u16 rstctrl_offs,u16 rstst_offs) {int c; -u32 mask =1 << shift; - -/* Check the current status to avoid de-asserting the line twice */ -if (am33xx_prm_is_hardreset_asserted (shift, inst, rstctrl_offs) ==0 ) -return -EEXIST ;
Any specific reason why you have removed this check?
Thanks,
Vaibhav
Quoted 30 lines
arch/arm/mach-omap2/prm33xx.c +6
-2+u32 mask =1 << st_shift;/* Clear the reset status by writing 1 to the status bit */ am33xx_prm_rmw_reg_bits (0xffffffff , mask, inst, rstst_offs); +/* de-assert the reset control line */ + mask =1 << shift; +am33xx_prm_rmw_reg_bits (mask,0 , inst, rstctrl_offs); -/* wait the status to be set */ -omap_test_timeout (am33xx_prm_is_hardreset_asserted (shift, inst, +/* wait the status to be set */ +omap_test_timeout (am33xx_prm_is_hardreset_asserted (st_shift, inst, rstst_offs),MAX_MODULE_HARDRESET_WAIT , c);arch/arm/mach-omap2/prm33xx.h +1
-1@@ -124,6 +124,6 @@ extern void am33xx_prm_global_warm_sw_reset(void);extern int am33xx_prm_is_hardreset_asserted (u8 shift,s16 inst,u16 rstctrl_offs);extern int am33xx_prm_assert_hardreset (u8 shift,s16 inst,u16 rstctrl_offs); -extern int am33xx_prm_deassert_hardreset (u8 shift,s16 inst, +extern int am33xx_prm_deassert_hardreset (u8 shift,u8 st_shift,s16 inst,u16 rstctrl_offs,u16 rstst_offs);#endif
PATCH 4/15 ARM: OMAP2+: hwmod: Update the reset API for AM33XX
On Mon, Nov 05, 2012 at 12:28:36, Hiremath, Vaibhav wrote:
[...]
Quoted 7 lines
arch/arm/mach-omap2/prm33xx.c int am33xx_prm_assert_hardreset(u8 shift, s16 inst, u16 rstctrl_offs) · line 123
Quoted 5 lines
arch/arm/mach-omap2/prm33xx.c +0
-5-u32 mask =1 << shift; - -/* Check the current status to avoid de-asserting the line twice */ -if (am33xx_prm_is_hardreset_asserted (shift, inst, rstctrl_offs) ==0 ) -return -EEXIST ;Any specific reason why you have removed this check?
During bootup the hardreset line is asserted, so wouldn't that check lead to the function always returning without doing anything?
Regards,
Vaibhav
PATCH 4/15 ARM: OMAP2+: hwmod: Update the reset API for AM33XX
Quoted 10 lines · Vaibhav
On Mon, Nov 05, 2012 at 12:28:36, Hiremath, Vaibhav wrote:
[...]Quoted 7 lines
arch/arm/mach-omap2/prm33xx.c int am33xx_prm_assert_hardreset(u8 shift, s16 inst, u16 rstctrl_offs) · line 123
Quoted 5 lines
arch/arm/mach-omap2/prm33xx.c +0
-5-u32 mask =1 << shift; - -/* Check the current status to avoid de-asserting the line twice */ -if (am33xx_prm_is_hardreset_asserted (shift, inst, rstctrl_offs) ==0 ) -return -EEXIST ;Any specific reason why you have removed this check?
Quoted 3 lines
During bootup the hardreset line is asserted, so wouldn't that check lead to the function always returning without doing anything?
The check is,
/* Check the current status to avoid de-asserting the line twice */ if (am33xx_prm_is_hardreset_asserted(shift, inst, rstctrl_offs) == 0)
return -EEXIST;
The code is checking whether the line is already de-asserted (== 0), so I am not sure how this will change if hardreset line is asserted during bootup.
Thanks,
Vaibhav
PATCH 4/15 ARM: OMAP2+: hwmod: Update the reset API for AM33XX
On Tue, Nov 06, 2012 at 11:36:20, Hiremath, Vaibhav wrote:
[...]
Quoted 3 lines
The code is checking whether the line is already de-asserted (== 0), so I am not sure how this will change if hardreset line is asserted during bootup.
You are right. I just checked the behavior since I recall seeing something odd earlier. Looks like this is needed to avoid issues with subsequent hardreset deassertions so I'll put it back in.
Regards.
Vaibhav
PATCH 5/15 ARM: OMAP2+: AM33XX: Update WKUP_M3 hwmod entry for reset status
In reply to Vaibhav BediaAdd the reset status offset for WKUP_M3 in the hwmod data
--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1 -0
@@ -269,6 +269,7 @@ static struct omap_hwmod am33xx_wkup_m3_hwmod = { .omap4 = { .clkctrl_offs = AM33XX_CM_WKUP_WKUP_M3_CLKCTRL_OFFSET, .rstctrl_offs = AM33XX_RM_WKUP_RSTCTRL_OFFSET, + .rstst_offs = AM33XX_RM_WKUP_RSTST_OFFSET, .modulemode = MODULEMODE_SWCTRL, }, },
Signature · 1 line
1.7.0.4
PATCH 5/15 ARM: OMAP2+: AM33XX: Update WKUP_M3 hwmod entry for reset status
Quoted 13 lines · Vaibhav Bedia
Add the reset status offset for
WKUP_M3in the hwmod dataSigned-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)arch/arm/mach-omap2/omap_hwmod_33xx_data.c +0
-0
arch/arm/mach-omap2/omap_hwmod_33xx_data.c static struct omap_hwmod am33xx_wkup_m3_hwmod = { · line 275
Quoted 7 lines
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1
-0@@ -269,6 +269,7 @@ static struct omap_hwmod am33xx_wkup_m3_hwmod = { .omap4 = { .clkctrl_offs = AM33XX_CM_WKUP_WKUP_M3_CLKCTRL_OFFSET, .rstctrl_offs = AM33XX_RM_WKUP_RSTCTRL_OFFSET, + .rstst_offs = AM33XX_RM_WKUP_RSTST_OFFSET, .modulemode = MODULEMODE_SWCTRL, }, },
You are adding reset bit in this patch but using it in 4/15. Probably you can re-order it to keep git bisect happy.
Regards
Santosh
PATCH 5/15 ARM: OMAP2+: AM33XX: Update WKUP_M3 hwmod entry for reset status
On Sat, Nov 03, 2012 at 21:45:29, Shilimkar, Santosh wrote:
[...]
Quoted 4 lines
Quoted 1 line
You are adding reset bit in this patch but using it in 4/15. Probably you can re-order it to keep git bisect happy.
Ok. Will reorder.
Regards,
Vaibhav
PATCH 5/15 ARM: OMAP2+: AM33XX: Update WKUP_M3 hwmod entry for reset status
In reply to Vaibhav BediaQuoted 7 lines · Vaibhav Bedia
Add the reset status offset for
WKUP_M3in the hwmod dataSigned-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Thanks for the patch, Probably you can resubmit this patch separately which can be merged independently from this patch-series.
Thanks,
Vaibhav
Quoted 13 lines
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1
-0@@ -269,6 +269,7 @@ static struct omap_hwmod am33xx_wkup_m3_hwmod = { .omap4 = { .clkctrl_offs = AM33XX_CM_WKUP_WKUP_M3_CLKCTRL_OFFSET, .rstctrl_offs = AM33XX_RM_WKUP_RSTCTRL_OFFSET, + .rstst_offs = AM33XX_RM_WKUP_RSTST_OFFSET, .modulemode = MODULEMODE_SWCTRL, }, },
PATCH 6/15 ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
In reply to Vaibhav BediaThe hwmod data for OCMCRAM in AM33XX was commented out. This data is needed by the power management code, hence uncomment the same and register the OCP interface for it.
--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +10 -1
@@ -415,7 +415,6 @@ static struct omap_hwmod am33xx_adc_tsc_hwmod = {* - cEFUSE (doesn't fall under any ocp_if) * - clkdiv32k * - debugss -* - ocmc ram * - ocp watch point * - aes0 * - sha0
@@ -481,6 +480,7 @@ static struct omap_hwmod am33xx_debugss_hwmod = { }, }, }; +#endif /* ocmcram */ static struct omap_hwmod_class am33xx_ocmcram_hwmod_class = {
@@ -501,6 +501,7 @@ static struct omap_hwmod am33xx_ocmcram_hwmod = { }, }; +#if 0 /* ocpwp */ static struct omap_hwmod_class am33xx_ocpwp_hwmod_class = { .name = "ocpwp",
@@ -3331,6 +3332,13 @@ static struct omap_hwmod_ocp_if am33xx_l3_s__usbss = { .flags = OCPIF_SWSUP_IDLE, }; +/* l3 main -> ocmc */ +static struct omap_hwmod_ocp_if am33xx_l3_main__ocmc = { + .master = &am33xx_l3_main_hwmod, + .slave = &am33xx_ocmcram_hwmod, + .user =OCP_USER_MPU |OCP_USER_SDMA , +}; +static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[]__initdata = { &am33xx_l4_fw__emif_fw, &am33xx_l3_main__emif,
@@ -3401,6 +3409,7 @@ static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[] __initdata = { &am33xx_l3_main__tptc0, &am33xx_l3_main__tptc1, &am33xx_l3_main__tptc2, + &am33xx_l3_main__ocmc, &am33xx_l3_s__usbss, &am33xx_l4_hs__cpgmac0, &am33xx_cpgmac0__mdio,
Signature · 1 line
1.7.0.4
PATCH 6/15 ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
Quoted 4 lines · Vaibhav Bedia
The hwmod data for OCMCRAM in AM33XX was commented out. This data is needed by the power management code, hence uncomment the same and register the OCP interface for it.
Why this data is needed by PM code ?
Regards
Santosh
PATCH 6/15 ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
Quoted 7 lines · Santosh
Quoted 4 lines · Vaibhav Bedia
The hwmod data for OCMCRAM in AM33XX was commented out. This data is needed by the power management code, hence uncomment the same and register the OCP interface for it.
Why this data is needed by PM code ?
It's needed to ensure that the clock to ocmcram is running during boot and doesn't get disabled as part of the suspend sequence. Will reword the changelog.
Regards,
Vaibhav
PATCH 6/15 ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
In reply to Vaibhav BediaQuoted 30 lines · Vaibhav Bedia
The hwmod data for OCMCRAM in AM33XX was commented out. This data is needed by the power management code, hence uncomment the same and register the OCP interface for it.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1
-1@@ -415,7 +415,6 @@ static struct omap_hwmod am33xx_adc_tsc_hwmod = {* - cEFUSE (doesn't fall under any ocp_if) * - clkdiv32k * - debugss -* - ocmc ram * - ocp watch point * - aes0 * - sha0 @@ -481,6 +480,7 @@ static struct omap_hwmod am33xx_debugss_hwmod = { }, }, }; +#endif /* ocmcram */ static struct omap_hwmod_class am33xx_ocmcram_hwmod_class = {
arch/arm/mach-omap2/omap_hwmod_33xx_data.c static struct omap_hwmod am33xx_ocmcram_hwmod = { · line 504
Quoted 4 lines
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1
-0@@ -501,6 +501,7 @@ static struct omap_hwmod am33xx_ocmcram_hwmod = { }, }; +#if 0
Can you cut-n-paste the ocmcram hwmod entry outside of #if and resubmit it again?
Thanks,
Vaibhav
Quoted 26 lines
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +8
-0/* ocpwp */ static struct omap_hwmod_class am33xx_ocpwp_hwmod_class = { .name ="ocpwp" ,@@ -3331,6 +3332,13 @@ static struct omap_hwmod_ocp_if am33xx_l3_s__usbss = { .flags = OCPIF_SWSUP_IDLE, }; +/* l3 main -> ocmc */ +static struct omap_hwmod_ocp_if am33xx_l3_main__ocmc = { + .master = &am33xx_l3_main_hwmod, + .slave = &am33xx_ocmcram_hwmod, + .user =OCP_USER_MPU |OCP_USER_SDMA , +}; +static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[]__initdata = { &am33xx_l4_fw__emif_fw, &am33xx_l3_main__emif,@@ -3401,6 +3409,7 @@ static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[] __initdata = { &am33xx_l3_main__tptc0, &am33xx_l3_main__tptc1, &am33xx_l3_main__tptc2, + &am33xx_l3_main__ocmc, &am33xx_l3_s__usbss, &am33xx_l4_hs__cpgmac0, &am33xx_cpgmac0__mdio,
PATCH 6/15 ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
Quoted 4 lines · Vaibhav
Can you cut-n-paste the ocmcram hwmod entry outside of #if and resubmit it again?
Ok. Will do that in the next version.
Regards,
Vaibhav
PATCH 6/15 ARM: OMAP2+: hwmod: Enable OCMCRAM registration in AM33XX
Quoted 8 lines · Vaibhav
Quoted 4 lines · Vaibhav
Can you cut-n-paste the ocmcram hwmod entry outside of #if and resubmit it again?
Ok. Will do that in the next version.
Another suggestion, all hwmod patches looks independent and trivial to me, so can you submit all hwmod patches separately so that it can go separately?
Thanks,
Vaibhav
Quoted 4 lines
Regards,
Vaibhav
PATCH 7/15 ARM: OMAP2+: hwmod: Update the hwmod data for TPTCs in AM33XX
In reply to Vaibhav BediaUpdate the TPTC hwmod entry to reflect the fact that the idle and standby transitions are s/w controlled.
--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1 -0
@@ -1827,6 +1827,7 @@ static struct omap_hwmod am33xx_tptc0_hwmod = { .class = &am33xx_tptc_hwmod_class, .clkdm_name = "l3_clkdm", .mpu_irqs = am33xx_tptc0_irqs, + .flags = (HWMOD_SWSUP_SIDLE | HWMOD_SWSUP_MSTANDBY), .main_clk = "l3_gclk", .prcm = { .omap4 = {
Signature · 1 line
1.7.0.4
PATCH 7/15 ARM: OMAP2+: hwmod: Update the hwmod data for TPTCs in AM33XX
Quoted 3 lines · Vaibhav Bedia
Update the TPTC hwmod entry to reflect the fact that the idle and standby transitions are s/w controlled.
Thanks,
Vaibhav
Quoted 18 lines
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1
-0@@ -1827,6 +1827,7 @@ static struct omap_hwmod am33xx_tptc0_hwmod = { .class = &am33xx_tptc_hwmod_class, .clkdm_name = "l3_clkdm", .mpu_irqs = am33xx_tptc0_irqs, + .flags = (HWMOD_SWSUP_SIDLE | HWMOD_SWSUP_MSTANDBY), .main_clk = "l3_gclk", .prcm = { .omap4 = {
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
In reply to Vaibhav BediaThe first entry for CPGMAC0 should be ADDR_MAP_ON_INIT instead of ADDR_TYPE_RT to ensure the omap hwmod code maps the memory space at init and writes to the SYSCONFIG registers.
--- arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/omap_hwmod_33xx_data.c +1 -1
@@ -2500,7 +2500,7 @@ static struct omap_hwmod_addr_space am33xx_cpgmac0_addr_space[] = { { .pa_start = 0x4a100000, .pa_end = 0x4a100000 + SZ_2K - 1, - .flags = ADDR_TYPE_RT, + .flags = ADDR_MAP_ON_INIT, },/* cpsw wr */ {
Signature · 1 line
1.7.0.4
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Quoted 7 lines · Vaibhav Bedia
The first entry for CPGMAC0 should be
ADDR_MAP_ON_INITinstead ofADDR_TYPE_RTto ensure the omap hwmod code maps the memory space at init and writes to the SYSCONFIG registers.Signed-off-by Vaibhav Bedia <[email protected]>---
Sorry again similar question.
Why CPGMAC0 should be mapped and sysconfig updated early ?
Regards
Santosh
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Quoted 6 lines · Santosh
Quoted 5 lines · Vaibhav Bedia
The first entry for CPGMAC0 should be
ADDR_MAP_ON_INITinstead ofADDR_TYPE_RTto ensure the omap hwmod code maps the memory space at init and writes to the SYSCONFIG registers.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Vaibhav Bedia <[email protected]>---
Sorry again similar question.
Why CPGMAC0 should be mapped and sysconfig updated early ?
Hmm I need to revisit this one. CPGMAC0 was not going to standby without this. Maybe something else is wrong in the hwmod data and needs fixing.
Regards,
Vaibhav
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
In reply to Santosh ShilimkarQuoted 9 lines · Vaibhav
Quoted 8 lines · Santosh
Quoted 7 lines · Vaibhav Bedia
The first entry for CPGMAC0 should be
ADDR_MAP_ON_INITinstead ofADDR_TYPE_RTto ensure the omap hwmod code maps the memory space at init and writes to the SYSCONFIG registers.Signed-off-by Vaibhav Bedia <[email protected]>---
Quoted 9 lines
Quoted 4 lines
Sorry again similar question.
Why CPGMAC0 should be mapped and sysconfig updated early ?
Hmm I need to revisit this one. CPGMAC0 was not going to standby without this. Maybe something else is wrong in the hwmod data and needs fixing.
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function _find_mpu_rt_addr_space in omap_hwmod.c looks for the first entry with the flag ADDR_TYPE_RT flag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with the ADDR_TYPE_RT flag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.
I'll make the changes in the next version.
Regards,
Vaibhav
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Quoted 20 lines · Vaibhav
Quoted 9 lines · Vaibhav
Quoted 8 lines · Santosh
Quoted 7 lines · Vaibhav Bedia
The first entry for CPGMAC0 should be
ADDR_MAP_ON_INITinstead ofADDR_TYPE_RTto ensure the omap hwmod code maps the memory space at init and writes to the SYSCONFIG registers.Signed-off-by Vaibhav Bedia <[email protected]>---
Quoted 9 lines
Quoted 4 lines
Sorry again similar question.
Why CPGMAC0 should be mapped and sysconfig updated early ?
Hmm I need to revisit this one. CPGMAC0 was not going to standby without this. Maybe something else is wrong in the hwmod data and needs fixing.
Quoted 11 lines
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function
_find_mpu_rt_addr_spaceinomap_hwmod.clooks for the first entry with the flagADDR_TYPE_RTflag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with theADDR_TYPE_RTflag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.
Good catch.
Just a side note on this, driver expects the addresses in this order only, first SS and then WR.
Thanks,
Vaibhav
Quoted 9 lines
I'll make the changes in the next version.
Regards,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
On Tue, Nov 06, 2012 at 14:59:45, Hiremath, Vaibhav wrote:
[...]
Quoted 13 lines
Quoted 12 lines
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function
_find_mpu_rt_addr_spaceinomap_hwmod.clooks for the first entry with the flagADDR_TYPE_RTflag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with theADDR_TYPE_RTflag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.
Quoted 5 lines
Good catch.
Just a side note on this, driver expects the addresses in this order only, first SS and then WR.
Sorry I didn't understand your comment. For HWMOD code to work as expected, we need to change the order. Are you saying that I should not be doing this because the driver will get affected?
Regards,
Vaibhav
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Quoted 15 lines · Vaibhav
On Tue, Nov 06, 2012 at 14:59:45, Hiremath, Vaibhav wrote:
[...]Quoted 13 lines
Quoted 12 lines
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function
_find_mpu_rt_addr_spaceinomap_hwmod.clooks for the first entry with the flagADDR_TYPE_RTflag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with theADDR_TYPE_RTflag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.
Quoted 8 lines
Quoted 5 lines
Good catch.
Just a side note on this, driver expects the addresses in this order only, first SS and then WR.
Sorry I didn't understand your comment. For HWMOD code to work as expected, we need to change the order.
Why do you want to change the order? Just specify "ADDR_TYPE_RT", that should be it.
Thanks,
Vaibhav
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Quoted 22 lines · Vaibhav
Quoted 21 lines · Vaibhav
On Tue, Nov 06, 2012 at 14:59:45, Hiremath, Vaibhav wrote:
[...]Quoted 18 lines
Quoted 12 lines
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function
_find_mpu_rt_addr_spaceinomap_hwmod.clooks for the first entry with the flagADDR_TYPE_RTflag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with theADDR_TYPE_RTflag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.Good catch.
Just a side note on this, driver expects the addresses in this order only, first SS and then WR.
Quoted 6 lines
Quoted 2 lines
Sorry I didn't understand your comment. For HWMOD code to work as expected, we need to change the order.
Why do you want to change the order? Just specify "
ADDR_TYPE_RT", that should be it.
The problem is that the memory space without the SYSC comes first and is labeled as ADDR_TYPE_RT. I think this is not correct and hence either we change the order or remove the flag from the first entry. If we do the latter then taking the logic of putting in the flag only for memory spaces with SYSC further we need to fixup the entries for ephrpwm0/1/2 and ecap0/1/2.
Regards,
Vaibhav
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Hi Vaibhav & Vaibhav,
Quoted 30 lines · Vaibhav
Quoted 22 lines · Vaibhav
Quoted 21 lines · Vaibhav
On Tue, Nov 06, 2012 at 14:59:45, Hiremath, Vaibhav wrote:
[...]Quoted 18 lines
Quoted 12 lines
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function
_find_mpu_rt_addr_spaceinomap_hwmod.clooks for the first entry with the flagADDR_TYPE_RTflag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with theADDR_TYPE_RTflag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.Good catch.
Just a side note on this, driver expects the addresses in this order only, first SS and then WR.
Quoted 6 lines
Quoted 2 lines
Sorry I didn't understand your comment. For HWMOD code to work as expected, we need to change the order.
Why do you want to change the order? Just specify "
ADDR_TYPE_RT", that should be it.
Quoted 5 lines
The problem is that the memory space without the SYSC comes first and is labeled as
ADDR_TYPE_RT. I think this is not correct and hence either we change the order or remove the flag from the first entry. If we do the latter then taking the logic of putting in the flag only for memory spaces with SYSC further we need to fixup the entries for ephrpwm0/1/2 and ecap0/1/2.
The order should not matter, just use ADDR_TYPE_RT for the relevant entry. I have a patch ongoing to remove this flag for the non-SYSC entry to avoid this kind of confusion.
The name should probably be changed as well to reflect that at some point. Since these entries will be removed anyway with pure DT boot, that should be a temporary issue.
Regards,
Benoit
PATCH 8/15 ARM: OMAP2+: hwmod: Fix the omap_hwmod_addr_space for CPGMAC0
Hi Benoit,
Quoted 39 lines · Benoit
Hi Vaibhav & Vaibhav,
Quoted 30 lines · Vaibhav
Quoted 22 lines · Vaibhav
Quoted 21 lines · Vaibhav
On Tue, Nov 06, 2012 at 14:59:45, Hiremath, Vaibhav wrote:
[...]Quoted 18 lines
Quoted 12 lines
Ok I checked this one. The change I made was indirectly fixing another issue with the AM33xx hwmod data. am33xx_cpgmac0_addr_space[] has two entries and the SYSC register is part of the second entry. The function
_find_mpu_rt_addr_spaceinomap_hwmod.clooks for the first entry with the flagADDR_TYPE_RTflag. The change I made indirectly made the second entry in am33xx_cpgmac0_addr_space[] become the first memory space with theADDR_TYPE_RTflag. Due to this the hwmod code wrote to the correct SYSC address of CPGMAC0 and the IP went to standby during bootup. After changing the order of the entries in am33xx_cpgmac0_addr_space[] things work fine.Good catch.
Just a side note on this, driver expects the addresses in this order only, first SS and then WR.
Quoted 6 lines
Quoted 2 lines
Sorry I didn't understand your comment. For HWMOD code to work as expected, we need to change the order.
Why do you want to change the order? Just specify "
ADDR_TYPE_RT", that should be it.Quoted 5 lines
The problem is that the memory space without the SYSC comes first and is labeled as
ADDR_TYPE_RT. I think this is not correct and hence either we change the order or remove the flag from the first entry. If we do the latter then taking the logic of putting in the flag only for memory spaces with SYSC further we need to fixup the entries for ephrpwm0/1/2 and ecap0/1/2.
Quoted 7 lines
The order should not matter, just use
ADDR_TYPE_RTfor the relevant entry. I have a patch ongoing to remove this flag for the non-SYSC entry to avoid this kind of confusion.
The name should probably be changed as well to reflect that at some point. Since these entries will be removed anyway with pure DT boot, that should be a temporary issue.
Thanks for the clarification. I'll make the change accordingly.
Regards,
Vaibhav
PATCH 9/15 ARM: OMAP: AM33XX: Remove unnecessary include and use __ASSEMBLER__ macros
In reply to Vaibhav BediaGet rid of some unnecessary header file inclusions and also use __ASSEMBLER__ macros to allow the various register offsets from PM assembly code which be added in a subsequent patch.
--- arch/arm/mach-omap2/clock33xx_data.c | 1 + arch/arm/mach-omap2/cm33xx.h | 10 ++++------ arch/arm/mach-omap2/prm33xx.h | 2 ++ 3 files changed, 7 insertions(+), 6 deletions(-)
arch/arm/mach-omap2/clock33xx_data.c +1 -0
@@ -17,6 +17,7 @@#include <linux/kernel.h> #include <linux/list.h> #include <linux/clk.h> +#include <linux/io.h> #include "soc.h" #include "iomap.h"
arch/arm/mach-omap2/cm33xx.h +4 -6
@@ -17,16 +17,11 @@#ifndef __ARCH_ARM_MACH_OMAP2_CM_33XX_H#define __ARCH_ARM_MACH_OMAP2_CM_33XX_H -#include <linux/delay.h> -#include <linux/errno.h> -#include <linux/err.h> -#include <linux/io.h> -#include "common.h" #include "cm.h" #include "cm-regbits-33xx.h" -#include "cm33xx.h" +#include "iomap.h" /* CM base address */ #define AM33XX_CM_BASE 0x44e00000
@@ -381,6 +376,7 @@#define AM33XX_CM_CEFUSE_CEFUSE_CLKCTRL AM33XX_CM_REGADDR(AM33XX_CM_CEFUSE_MOD, 0x0020) +#ifndef __ASSEMBLER__extern bool am33xx_cm_is_clkdm_in_hwsup (s16 inst,u16 cdoffs);extern void am33xx_cm_clkdm_enable_hwsup (s16 inst,u16 cdoffs);extern void am33xx_cm_clkdm_disable_hwsup (s16 inst,u16 cdoffs);
@@ -417,4 +413,6 @@ static inline int am33xx_cm_wait_module_ready(u16 inst, s16 cdoffs, }#endif +#endif /* ASSEMBLER */ +#endif
arch/arm/mach-omap2/prm33xx.h +2 -0
@@ -117,6 +117,7 @@#define AM33XX_PM_CEFUSE_PWRSTST_OFFSET 0x0004#define AM33XX_PM_CEFUSE_PWRSTST AM33XX_PRM_REGADDR(AM33XX_PRM_CEFUSE_MOD, 0x0004) +#ifndef __ASSEMBLER__extern u32 am33xx_prm_read_reg (s16 inst,u16 idx);extern void am33xx_prm_write_reg (u32 val,s16 inst,u16 idx);extern u32 am33xx_prm_rmw_reg_bits (u32 mask,u32 bits,s16 inst,s16 idx);
@@ -126,4 +127,5 @@ extern int am33xx_prm_is_hardreset_asserted(u8 shift, s16 inst,extern int am33xx_prm_assert_hardreset (u8 shift,s16 inst,u16 rstctrl_offs);extern int am33xx_prm_deassert_hardreset (u8 shift,u8 st_shift,s16 inst,u16 rstctrl_offs,u16 rstst_offs); +#endif /* __ASSEMBLER__ */ #endif
Signature · 1 line
1.7.0.4
PATCH 9/15 ARM: OMAP: AM33XX: Remove unnecessary include and use __ASSEMBLER__ macros
Quoted 6 lines · Vaibhav Bedia
Get rid of some unnecessary header file inclusions and also use
__ASSEMBLER__macros to allow the various register offsets from PM assembly code which be added in a subsequent patch.Signed-off-by Vaibhav Bedia <[email protected]>
Ideally you should split header clean-up and assembler fix in seperate patches.
Regards
Santosh
PATCH 9/15 ARM: OMAP: AM33XX: Remove unnecessary include and use __ASSEMBLER__ macros
Quoted 11 lines · Santosh
Quoted 6 lines · Vaibhav Bedia
Get rid of some unnecessary header file inclusions and also use
__ASSEMBLER__macros to allow the various register offsets from PM assembly code which be added in a subsequent patch.Signed-off-by Vaibhav Bedia <[email protected]>Ideally you should split header clean-up and assembler fix in seperate patches.
Ok. Will do.
Regards,
Vaibhav
PATCH 10/15 ARM: OMAP2+: control: Add some AM33XX Control module registers
In reply to Vaibhav BediaThese registers will be required in a subsequent
patch which adds basic suspend-resume support for
AM33XX.
--- arch/arm/mach-omap2/control.h | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/control.h +29 -0
@@ -357,6 +357,35 @@#define AM33XX_CONTROL_STATUS_SYSBOOT1_WIDTH 0x2#define AM33XX_CONTROL_STATUS_SYSBOOT1_MASK (0x3 << 22) +#define AM33XX_DDR_IO_CTRL 0x0E04 +#define AM33XX_VTP0_CTRL_REG 0x0E0C + +/* AM33XX VTP0_CTRL_REG bits */ +#define AM33XX_VTP_CTRL_START_EN BIT(0) +#define AM33XX_VTP_CTRL_LOCK_EN BIT(4) +#define AM33XX_VTP_CTRL_READY BIT(5) +#define AM33XX_VTP_CTRL_ENABLE BIT(6) + + +/* AM33XX M3_TXEV_EOI registers */ +#define AM33XX_CONTROL_M3_TXEV_EOI 0x1324 + +/* AM33XX IPC message registers */ +#define AM33XX_CONTROL_IPC_MSG_REG0 0x1328 +#define AM33XX_CONTROL_IPC_MSG_REG1 0x132C +#define AM33XX_CONTROL_IPC_MSG_REG2 0x1330 +#define AM33XX_CONTROL_IPC_MSG_REG3 0x1334 +#define AM33XX_CONTROL_IPC_MSG_REG4 0x1338 +#define AM33XX_CONTROL_IPC_MSG_REG5 0x133C +#define AM33XX_CONTROL_IPC_MSG_REG6 0x1340 +#define AM33XX_CONTROL_IPC_MSG_REG7 0x1344 + +#define AM33XX_DDR_CMD0_IOCTRL 0x1404 +#define AM33XX_DDR_CMD1_IOCTRL 0x1408 +#define AM33XX_DDR_CMD2_IOCTRL 0x140C +#define AM33XX_DDR_DATA0_IOCTRL 0x1440 +#define AM33XX_DDR_DATA1_IOCTRL 0x1444 +/* CONTROL OMAP STATUS register to identify OMAP3 features */ #define OMAP3_CONTROL_OMAP_STATUS 0x044c
Signature · 1 line
1.7.0.4
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
In reply to Vaibhav BediaAM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX. A subsequent patch will add suspend-resume support for the clockevent to ensure that there are no issues with timekeeping.
--- arch/arm/mach-omap2/timer.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/timer.c +1 -1
@@ -460,7 +460,7 @@ OMAP_SYS_TIMER(3_secure)#endif #ifdef CONFIG_SOC_AM33XX -OMAP_SYS_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, 2, OMAP4_MPU_SOURCE) +OMAP_SYS_TIMER_INIT(3_am33xx, 2, OMAP4_MPU_SOURCE, 1, OMAP4_MPU_SOURCE) OMAP_SYS_TIMER(3_am33xx)#endif
Signature · 1 line
1.7.0.4
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Quoted 8 lines · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
Kevin
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Hi Kevin,
Quoted 10 lines · Kevin Hilman
Quoted 8 lines · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.
Quoted 4 lines
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
The lowest idle state that we are targeting will have MPU powered off with external memory in self-refresh mode. Peripheral domain with the clockevent will be kept on.
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
T
Quoted 13 lines · Vaibhav
Hi Kevin,
Quoted 10 lines · Kevin Hilman
Quoted 8 lines · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.
Quoted 8 lines
Quoted 4 lines
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
The lowest idle state that we are targeting will have MPU powered off with external memory in self-refresh mode. Peripheral domain with the clockevent will be kept on.
Is this a limitation of the hardware? or the software?
Kevin
P.S. I haven't yet made my way through the TRM, so I'll probably figure this out when I read it, but having this summarized in the changelog would be helpful since even after I read the TRM, I'll forget. I'm saving the TRM
reading for entertainment on upcoming flight home from Linaro Connect.
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
On Sat, Nov 03, 2012 at 18:34:30, Kevin Hilman wrote:
[...]
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
Quoted 6 lines
Quoted 3 lines
The lowest idle state that we are targeting will have MPU powered off with external memory in self-refresh mode. Peripheral domain with the clockevent will be kept on.
Is this a limitation of the hardware? or the software?
Well, making the lowest idle state same as the suspend state will require us to involve WKUP_M3 in the idle path and wakeup sources get limited to the IPs in the WKUP domain alone. There's no IO daisy chaining in AM33XX so that's one big difference compared to OMAP.
The other potential problem is that the IPC mechanism that we have uses interrupts. Assuming that the lowest idle state, say Cx, is the same as the suspend state, we'll need to communicate with the WKUP_M3 using interrupts once we decide to enter Cx. I am not sure if we can do something in the cpuidle implementation to work around the "interrupt for idle" problem. We could probably not wait for an ACK when we want to enter Cx, but the problem of limited wakeup sources remains. If we let the various drivers block the entry to Cx, since almost all the IPs are in the peripheral domain a system which uses anything other than UART and Timer in WKUP domain will probably never be able enter Cx.
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Quoted 15 lines · Vaibhav
On Sat, Nov 03, 2012 at 18:34:30, Kevin Hilman wrote:
[...]Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
Quoted 6 lines
Quoted 3 lines
The lowest idle state that we are targeting will have MPU powered off with external memory in self-refresh mode. Peripheral domain with the clockevent will be kept on.
Is this a limitation of the hardware? or the software?
Quoted 6 lines
Well, making the lowest idle state same as the suspend state will require us to involve
WKUP_M3in the idle path and wakeup sources get limited to the IPs in the WKUP domain alone. There's no IO daisy chaining in AM33XX so that's one big difference compared to OMAP. The other potential problem is that the IPC mechanism that we have uses interrupts.
It can still interrupt the M3, it's only the interrupt back to the MPU that is the issue, right? That being said, there's no reason it couldn't use polling in the idle path, right?
Quoted 7 lines
Assuming that the lowest idle state, say Cx, is the same as the suspend state, we'll need to communicate with the
WKUP_M3using interrupts once we decide to enter Cx. I am not sure if we can do something in the cpuidle implementation to work around the "interrupt for idle" problem.We could probably not wait for an ACK when we want to enter Cx,
why not?
Are the response times from the M3 really up to 500ms (guessing based on the timeout you used in the suspend path.) That seems rather unlikely.
Hmm, but as I think about it. Why does the MPU need to wait for an ACK at all? Why not just send the cmd and WFI?
Quoted 4 lines
but the problem of limited wakeup sources remains. If we let the various drivers block the entry to Cx, since almost all the IPs are in the peripheral domain a system which uses anything other than UART and Timer in WKUP domain will probably never be able enter Cx.
Even so, I think the system needs to be designed to hit the same power states in idle and suspend. Then, the states can be restricted based wakeup capabilities as you described. This would be easy to do in the runtime PM implementation for this device.
IMO, assuming that idle will not be useful from the begining is leading down the path to poor design choices that will be much more difficult to fixup down the road in order to add idle support later. We need to design both idle and suspend at the same time.
Also, don't forget about GPIO0. Systems could easily be built such that peripherals which want to wakeup but don't have native wakeup capabilities could use a GPIO in bank 0 to wake the system.
Similarily, I2C0 is in WKUP, and brought out to capes, so some simple designs with with I2C devices on a cape might be perfectly capable of hitting deep power states in idle.
Kevin
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Quoted 54 lines · Kevin Hilman
Quoted 15 lines · Vaibhav
On Sat, Nov 03, 2012 at 18:34:30, Kevin Hilman wrote:
[...]Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
Quoted 6 lines
Quoted 3 lines
The lowest idle state that we are targeting will have MPU powered off with external memory in self-refresh mode. Peripheral domain with the clockevent will be kept on.
Is this a limitation of the hardware? or the software?
Quoted 6 lines
Well, making the lowest idle state same as the suspend state will require us to involve
WKUP_M3in the idle path and wakeup sources get limited to the IPs in the WKUP domain alone. There's no IO daisy chaining in AM33XX so that's one big difference compared to OMAP. The other potential problem is that the IPC mechanism that we have uses interrupts.It can still interrupt the M3, it's only the interrupt back to the MPU that is the issue, right? That being said, there's no reason it couldn't use polling in the idle path, right?
Quoted 7 lines
Assuming that the lowest idle state, say Cx, is the same as the suspend state, we'll need to communicate with the
WKUP_M3using interrupts once we decide to enter Cx. I am not sure if we can do something in the cpuidle implementation to work around the "interrupt for idle" problem.We could probably not wait for an ACK when we want to enter Cx,
why not?
Are the response times from the M3 really up to 500ms (guessing based on the timeout you used in the suspend path.) That seems rather unlikely.
Hmm, but as I think about it. Why does the MPU need to wait for an ACK at all? Why not just send the cmd and WFI?
Quoted 4 lines
but the problem of limited wakeup sources remains. If we let the various drivers block the entry to Cx, since almost all the IPs are in the peripheral domain a system which uses anything other than UART and Timer in WKUP domain will probably never be able enter Cx.
Even so, I think the system needs to be designed to hit the same power states in idle and suspend. Then, the states can be restricted based wakeup capabilities as you described. This would be easy to do in the runtime PM implementation for this device.
Quoted 5 lines
IMO, assuming that idle will not be useful from the begining is leading down the path to poor design choices that will be much more difficult to fixup down the road in order to add idle support later. We need to design both idle and suspend at the same time.
I agree with Kevin. Not supporting CPUIDLE deep states can hit the active power numbers dearly. I just don't know why the SOCs don't share the standard and must have design choices. But thats another discussion.
How about leaving the timer choices as is. PER timer for clock source and wakeuptimer for clock event. Anyway in suspend the clock-source can be suspended and that is evident from recent discussion. The only downside is you won't count time in suspend which is any way the case.
Vaibhav,
Do you guys see any implementation bottleneck for above ?
Regards
Santosh
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Hi Santosh,
On Tue, Nov 06, 2012 at 03:29:22, Shilimkar, Santosh wrote:
[...]
Quoted 10 lines
Quoted 6 lines
IMO, assuming that idle will not be useful from the begining is leading down the path to poor design choices that will be much more difficult to fixup down the road in order to add idle support later. We need to design both idle and suspend at the same time.
I agree with Kevin. Not supporting CPUIDLE deep states can hit the active power numbers dearly. I just don't know why the SOCs don't share the standard and must have design choices. But thats another discussion.
Yes, active power numbers are not comparable to OMAP :(
Quoted 8 lines
How about leaving the timer choices as is. PER timer for clock source and wakeuptimer for clock event. Anyway in suspend the clock-source can be suspended and that is evident from recent discussion. The only downside is you won't count time in suspend which is any way the case.
Vaibhav,
Do you guys see any implementation bottleneck for above ?
Looking at the timekeeping code I see one more potential reason for making this change. OMAP registers the 32k sync timer as the persistent clock and since there's no 32k sync timer in AM33xx it doesn't register a persistent clock right now. Based on what I understood, we need to have to register one and DMTimer1 is the only clock that can serve as the persistent clock in suspend state. When we do so we might as well use it as the clocksource.
A related question that I had was, is there a mechanism to handle the 32k counter (DMTimer or sync timer) wraparound condition in suspend?
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
In reply to Santosh ShilimkarHi Santosh,
Quoted 35 lines · Vaibhav
Hi Santosh,
On Tue, Nov 06, 2012 at 03:29:22, Shilimkar, Santosh wrote:
[...]
Quoted 10 lines
Quoted 6 lines
IMO, assuming that idle will not be useful from the begining is leading down the path to poor design choices that will be much more difficult to fixup down the road in order to add idle support later. We need to design both idle and suspend at the same time.
I agree with Kevin. Not supporting CPUIDLE deep states can hit the active power numbers dearly. I just don't know why the SOCs don't share the standard and must have design choices. But thats another discussion.
Yes, active power numbers are not comparable to OMAP :(
Quoted 8 lines
How about leaving the timer choices as is. PER timer for clock source and wakeuptimer for clock event. Anyway in suspend the clock-source can be suspended and that is evident from recent discussion. The only downside is you won't count time in suspend which is any way the case.
Vaibhav,
Do you guys see any implementation bottleneck for above ?Looking at the timekeeping code I see one more potential reason for making this change. OMAP registers the 32k sync timer as the persistent clock and since there's no 32k sync timer in AM33xx it doesn't register a persistent clock right now. Based on what I understood, we need to have to register one and DMTimer1 is the only clock that can serve as the persistent clock in suspend state. When we do so we might as well use it as the clocksource.
Quoted 3 lines
A related question that I had was, is there a mechanism to handle the 32k counter (DMTimer or sync timer) wraparound condition in suspend?
Does interchanging the clksrc and clkevt look fine to you?
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
In reply to Kevin HilmanHi Kevin,
Quoted 24 lines · Kevin Hilman
Quoted 15 lines · Vaibhav
On Sat, Nov 03, 2012 at 18:34:30, Kevin Hilman wrote:
[...]Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Doesn't this also mean that you won't get timer wakeups in idle? Or are you keeping the domain where the clockevent is on during idle?
Quoted 6 lines
Quoted 3 lines
The lowest idle state that we are targeting will have MPU powered off with external memory in self-refresh mode. Peripheral domain with the clockevent will be kept on.
Is this a limitation of the hardware? or the software?
Quoted 6 lines
Well, making the lowest idle state same as the suspend state will require us to involve
WKUP_M3in the idle path and wakeup sources get limited to the IPs in the WKUP domain alone. There's no IO daisy chaining in AM33XX so that's one big difference compared to OMAP. The other potential problem is that the IPC mechanism that we have uses interrupts.
Quoted 4 lines
It can still interrupt the M3, it's only the interrupt back to the MPU that is the issue, right? That being said, there's no reason it couldn't use polling in the idle path, right?
Yes we could use polling but I think we have a bigger problem in the chip architecture.
Quoted 6 lines
Quoted 6 lines
Assuming that the lowest idle state, say Cx, is the same as the suspend state, we'll need to communicate with the
WKUP_M3using interrupts once we decide to enter Cx. I am not sure if we can do something in the cpuidle implementation to work around the "interrupt for idle" problem.
Quoted 7 lines
Quoted 1 line
We could probably not wait for an ACK when we want to enter Cx,
why not?
Are the response times from the M3 really up to 500ms (guessing based on the timeout you used in the suspend path.) That seems rather unlikely.
No 500ms is too high. Actual delays would be much lower, I need to check with the design team on the expected number.
Quoted 3 lines
Hmm, but as I think about it. Why does the MPU need to wait for an ACK at all? Why not just send the cmd and WFI?
I have myself being going back and forth on this. There are lot of things that we do in software, DDR being one of them. We can't do some of the DDR related stuff unless memory enter self-refresh AND EMIF gets disabled. Doing so essentially means that the drivers have entered sort of suspend state. Given this h/w limitation I don't see how we could handle without impacting a running system.
Quoted 10 lines
Quoted 4 lines
but the problem of limited wakeup sources remains. If we let the various drivers block the entry to Cx, since almost all the IPs are in the peripheral domain a system which uses anything other than UART and Timer in WKUP domain will probably never be able enter Cx.
Even so, I think the system needs to be designed to hit the same power states in idle and suspend. Then, the states can be restricted based wakeup capabilities as you described. This would be easy to do in the runtime PM implementation for this device.
Quoted 5 lines
IMO, assuming that idle will not be useful from the begining is leading down the path to poor design choices that will be much more difficult to fixup down the road in order to add idle support later. We need to design both idle and suspend at the same time.
Getting PER to transition on a running system is something I can't figure out. Maybe MPU OFF is the lowest we can go.
Quoted 8 lines
Also, don't forget about GPIO0. Systems could easily be built such that peripherals which want to wakeup but don't have native wakeup capabilities could use a GPIO in bank 0 to wake the system.
Similarily, I2C0 is in WKUP, and brought out to capes, so some simple designs with with I2C devices on a cape might be perfectly capable of hitting deep power states in idle.
Ok this is interesting. AFAIK I2C wakeup requires the device to be operating in slave mode. If so, is this something that's already supported on OMAP?
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
In reply to Vaibhav BediaQuoted 1 line · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain.
After reading the TRM, it seems there are two: DMTIMER0 and DMTIMER1.
Looking at the hwmod data though, I don't see a hwmod for DMTIMER0. Can you explain a little about why DMTIMER0 is missing/broken?
Kevin
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Hi Kevin,
Quoted 8 lines · Kevin Hilman
Quoted 1 line · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain.
After reading the TRM, it seems there are two: DMTIMER0 and DMTIMER1.
Looking at the hwmod data though, I don't see a hwmod for DMTIMER0. Can you explain a little about why DMTIMER0 is missing/broken?
DMTimer0 is usable only in secure devices only. This timer by default runs from an inaccurate RC oscillator the frequency of which can be anywhere from 16-60KHz based on process variations. There is a mux to change the clock source but the register for the mux can't be modified in GP devices. (will add this in the changelog)
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
In reply to Vaibhav BediaQuoted 21 lines · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX. A subsequent patch will add suspend-resume support for the clockevent to ensure that there are no issues with timekeeping.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/mach-omap2/timer.c +0
-0
arch/arm/mach-omap2/timer.c OMAP_SYS_TIMER(3_secure) · line 466
Quoted 8 lines
arch/arm/mach-omap2/timer.c +1
-1@@ -460,7 +460,7 @@ OMAP_SYS_TIMER(3_secure)#endif #ifdef CONFIG_SOC_AM33XX -OMAP_SYS_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, 2, OMAP4_MPU_SOURCE) +OMAP_SYS_TIMER_INIT(3_am33xx, 2, OMAP4_MPU_SOURCE, 1, OMAP4_MPU_SOURCE) OMAP_SYS_TIMER(3_am33xx)#endif
As mentioned on other patch comment, I think this might break your SOC idle.
Regards
Santosh
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Quoted 4 lines · Santosh
Quoted 1 line
As mentioned on other patch comment, I think this might break your SOC idle.
Unfortunately we don't have SOC idle.
Regards,
Vaibhav
PATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
In reply to Vaibhav BediaQuoted 21 lines · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX. A subsequent patch will add suspend-resume support for the clockevent to ensure that there are no issues with timekeeping.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/mach-omap2/timer.c +0
-0
arch/arm/mach-omap2/timer.c OMAP_SYS_TIMER(3_secure) · line 465
Quoted 7 lines
arch/arm/mach-omap2/timer.c +1
-1@@ -460,7 +460,7 @@ OMAP_SYS_TIMER(3_secure)#endif #ifdef CONFIG_SOC_AM33XX -OMAP_SYS_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, 2, OMAP4_MPU_SOURCE) +OMAP_SYS_TIMER_INIT(3_am33xx, 2, OMAP4_MPU_SOURCE, 1, OMAP4_MPU_SOURCE) OMAP_SYS_TIMER(3_am33xx)#endif
By the way, for v3.8 (assuming that timer DT patches go in, currently queued in Tony's master), when booting with DT the clock-source and clock-events will be selected by timer feature and not ID. So you may wish to rebase on top of Tony's master.
Cheers
Jon
Tony Lindgren
maintainerPATCH 11/15 ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
Quoted 25 lines · Jon Hunter
Quoted 23 lines · Vaibhav Bedia
AM33XX has only one usable timer in the WKUP domain. Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. The timer in WKUP domain can keep running in suspend from a 32K clock and hence serve as the persistent clock. To enable this, interchange the timers used as clocksource and clockevent for AM33XX. A subsequent patch will add suspend-resume support for the clockevent to ensure that there are no issues with timekeeping.
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/mach-omap2/timer.c +0
-0@@ -460,7 +460,7 @@ OMAP_SYS_TIMER(3_secure)#endif
Quoted 10 lines
arch/arm/mach-omap2/timer.c OMAP_SYS_TIMER(3_secure) · line 465
Quoted 5 lines
arch/arm/mach-omap2/timer.c +1
-1#ifdef CONFIG_SOC_AM33XX -OMAP_SYS_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, 2, OMAP4_MPU_SOURCE) +OMAP_SYS_TIMER_INIT(3_am33xx, 2, OMAP4_MPU_SOURCE, 1, OMAP4_MPU_SOURCE) OMAP_SYS_TIMER(3_am33xx)#endif By the way, for v3.8 (assuming that timer DT patches go in, currently queued in Tony's master), when booting with DT the clock-source and clock-events will be selected by timer feature and not ID. So you may wish to rebase on top of Tony's master.
Please don't base anything going upstream on the master branch except for testing. The master branch is just a merge of the upstream heading branches, and cannot be used as a base for pulling into anything.
In this case the right branch to use as a base is omap-for-v3.8/dt.
Regards,
Tony
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
In reply to Vaibhav BediaFrom: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/timer.c +31 -0
@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } } +static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW ); +} +static struct clock_event_device clockevent_gpt = { .name ="gp_timer" , .features =CLOCK_EVT_FEAT_PERIODIC |CLOCK_EVT_FEAT_ONESHOT ,
@@ -142,6 +171,8 @@ static struct clock_event_device clockevent_gpt = { .rating = 300, .set_next_event = omap2_gp_timer_set_next_event, .set_mode = omap2_gp_timer_set_mode, + .suspend = omap_clkevt_suspend, + .resume = omap_clkevt_resume, }; static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
Signature · 1 line
1.7.0.4
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Quoted 7 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
The changelog describes "what", but doesn't answer "why?"
Quoted 6 lines
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
You say it behaves properly without describing what improper behavior is happening.
Quoted 11 lines
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)arch/arm/mach-omap2/timer.c +0
-0
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 143
Quoted 9 lines
arch/arm/mach-omap2/timer.c +6
-0@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } } +static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 );
hard-coded timer ID? the rest of the code is using timer_id
Quoted 12 lines
arch/arm/mach-omap2/timer.c +12
-0+ oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; +
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 165
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW ); +}
I don't like the sprintf/hwmod_lookup for every suspend/resume. Just add a new file-global static 'struct omap_hwmod clockevt_oh' along side clockevent_gpt,
and assign it at init time in dmtimer_init_one. Then you don't have to do this
sprintf/lookup on every suspend/resume.
Kevin
Quoted 13 lines
arch/arm/mach-omap2/timer.c +2
-0static struct clock_event_device clockevent_gpt = { .name ="gp_timer" , .features =CLOCK_EVT_FEAT_PERIODIC |CLOCK_EVT_FEAT_ONESHOT ,@@ -142,6 +171,8 @@ static struct clock_event_device clockevent_gpt = { .rating = 300, .set_next_event = omap2_gp_timer_set_next_event, .set_mode = omap2_gp_timer_set_mode, + .suspend = omap_clkevt_suspend, + .resume = omap_clkevt_resume, }; static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Quoted 11 lines · Kevin Hilman
Quoted 7 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
The changelog describes "what", but doesn't answer "why?"
Sorry I'll try to take of this in the future.
Quoted 10 lines
Quoted 6 lines
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
You say it behaves properly without describing what improper behavior is happening.
There are two issues. One is that the clockevent timer doesn't get idled which blocks PER domain transition. The next one is that the clockevent doesn't generate any further interrupts once the system resumes. We need to restore the pre-suspend configuration. I haven't tried but I guess we could have used the save and restore of timer registers here.
Quoted 14 lines
Quoted 14 lines
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)arch/arm/mach-omap2/timer.c +0
-0@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } }
Quoted 8 lines
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 143
Quoted 6 lines
arch/arm/mach-omap2/timer.c +6
-0+static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 );hard-coded timer ID? the rest of the code is using
timer_id
Will fix.
Quoted 22 lines
Quoted 22 lines
arch/arm/mach-omap2/timer.c +21
-0+ oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW );
Quoted 9 lines
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 165
Quoted 1 line
arch/arm/mach-omap2/timer.c +1
-0+}I don't like the sprintf/hwmod_lookup for every suspend/resume. Just add a new file-global static 'struct
omap_hwmodclockevt_oh' along sideclockevent_gpt,
and assign it at init time indmtimer_init_one. Then you don't have to do this
sprintf/lookup on every suspend/resume.
Ok. Will make the changes accordingly.
Regards,
Vaibhav
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Quoted 10 lines · Vaibhav
Quoted 9 lines · Kevin Hilman
Quoted 7 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
Quoted 4 lines
Quoted 2 lines
The changelog describes "what", but doesn't answer "why?"
Sorry I'll try to take of this in the future.
Thanks. Here's a general rule. Assume you (or I) will be reading this a year from now and will have forgotten the details. The changelog then serves as our long-term memory. :)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
Quoted 8 lines
Quoted 3 lines
You say it behaves properly without describing what improper behavior is happening.
There are two issues. One is that the clockevent timer doesn't get idled which blocks PER domain transition. The next one is that the clockevent doesn't generate any further interrupts once the system resumes.
Please include both in the next rev of the changelog.
Quoted 3 lines
We need to restore the pre-suspend configuration.
I haven't tried but I guess we could have used the save and restore c timer registers here.
Yes, please try with that. Won't that be necessary anyways for situations where the powerdomain goes off?
Kevin
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
On Sat, Nov 03, 2012 at 19:11:54, Kevin Hilman wrote:
[...]
Quoted 4 lines
Yes, please try with that. Won't that be necessary anyways for situations where the powerdomain goes off?
Yes, we probably got lucky with the minimal resume routine.
Regards,
Vaibhav
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
In reply to Bedia, VaibhavQuoted 22 lines · Vaibhav
Quoted 11 lines · Kevin Hilman
Quoted 7 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
The changelog describes "what", but doesn't answer "why?"
Sorry I'll try to take of this in the future.
Quoted 7 lines
Quoted 6 lines
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
Quoted 6 lines
Quoted 3 lines
You say it behaves properly without describing what improper behavior is happening.
There are two issues. One is that the clockevent timer doesn't get idled which blocks PER domain transition.
Why is this? How is the dmtimer TIOCP_CFG register configured on AM33xx? Is it using smart-idle?
Quoted 5 lines
The next one is that
the clockevent doesn't generate any further interrupts once the system resumes. We need to restore the pre-suspend configuration. I haven't tried but I guess we could have used the save and restore of timer registers here.
It would be interesting to try using an non-wakeup domain timer on OMAP3/4 for clock events and seeing if suspend/resume works.
Do you know what the exact problem here is? I understand that the timer context could get lost, but exactly what is not getting restarted by the kernel? For example, the only place we set the interrupt enable is during the clock event init and so if the context is lost, then I could see no more interrupts occurring. So is it enough to just restore the interrupt enable register, do you really need to program the timer again?
Cheers
Jon
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Hi Jon,
On Tue, Nov 06, 2012 at 02:50:50, Hunter, Jon wrote:
[...]
Quoted 4 lines
Why is this? How is the dmtimer
TIOCP_CFGregister configured on AM33xx? Is it using smart-idle?
Yes, it is set to smart-idle with wakeup capable mode. (this needs a fixup since this timer is not wakeup capable) but unfortunately this is not sufficient. On AM33xx there's no HW_AUTO mode magic so unless the IPs in PER domain are disabled by s/w, PER domain can't transition.
Quoted 9 lines
Quoted 5 lines
The next one is that
the clockevent doesn't generate any further interrupts once the system resumes. We need to restore the pre-suspend configuration. I haven't tried but I guess we could have used the save and restore of timer registers here.It would be interesting to try using an non-wakeup domain timer on OMAP3/4 for clock events and seeing if suspend/resume works.
Quoted 7 lines
Do you know what the exact problem here is? I understand that the timer context could get lost, but exactly what is not getting restarted by the kernel? For example, the only place we set the interrupt enable is during the clock event init and so if the context is lost, then I could see no more interrupts occurring. So is it enough to just restore the interrupt enable register, do you really need to program the timer again?
Just restoring the interrupt enable register works. But since there's no logic retention I think a context save-restore would be better.
Regards,
Vaibhav
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Quoted 31 lines · Vaibhav
Hi Jon,
On Tue, Nov 06, 2012 at 02:50:50, Hunter, Jon wrote:
[...]Quoted 4 lines
Why is this? How is the dmtimer
TIOCP_CFGregister configured on AM33xx? Is it using smart-idle?Yes, it is set to smart-idle with wakeup capable mode. (this needs a fixup since this timer is not wakeup capable) but unfortunately this is not sufficient. On AM33xx there's no
HW_AUTOmode magic so unless the IPs in PER domain are disabled by s/w, PER domain can't transition.Quoted 9 lines
Quoted 5 lines
The next one is that
the clockevent doesn't generate any further interrupts once the system resumes. We need to restore the pre-suspend configuration. I haven't tried but I guess we could have used the save and restore of timer registers here.It would be interesting to try using an non-wakeup domain timer on OMAP3/4 for clock events and seeing if suspend/resume works.
Quoted 7 lines
Do you know what the exact problem here is? I understand that the timer context could get lost, but exactly what is not getting restarted by the kernel? For example, the only place we set the interrupt enable is during the clock event init and so if the context is lost, then I could see no more interrupts occurring. So is it enough to just restore the interrupt enable register, do you really need to program the timer again?
Quoted 2 lines
Just restoring the interrupt enable register works. But since there's no logic retention I think a context save-restore would be better.
Ok, we may need to check the order in which events occur following resume. The kernel will restart the clock-events and we just need to make sure we do not restore the context after the clock-events has been restarted.
Cheers
Jon
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
In reply to Vaibhav BediaQuoted 4 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent.
Actually OMAP also uses only one timer. The clocksource is taken care by 32K syntimer till OMAP4 and by realtime counter on OMAP5. There is a clocksource registration of timer is available but that is not being used in systems.
Quoted 11 lines
AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
So you use WKUP domain timer for clocksource and PER domain one for clock-event ?
Quoted 13 lines
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)arch/arm/mach-omap2/timer.c +0
-0@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } }
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 146
Quoted 10 lines
arch/arm/mach-omap2/timer.c +9
-0+static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ;
You can move all the look up stuff in init code and then suspend resume hooks will be cleaner.
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+ +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 );
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 166
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+ oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW ); +} +
OK. So since your clk_event stops when PER idles, how do you plan to support the SOC idle. For CPUIDLE path, you need your clock-event to wakeup the system based on next timer expiry. So you need your clock event to be active. Indirectly, you can't let PER idle which leads npo CORE idle->SOC idle.
How do you plan to address this ? Os is SOC idle is not suppose to be added for AMXXX ?
Regards
Santosh
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Hi Santosh,
Quoted 10 lines · Santosh
Quoted 4 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent.
Actually OMAP also uses only one timer. The clocksource is taken care by 32K syntimer till OMAP4 and by realtime counter on OMAP5. There is a clocksource registration of timer is available but that is not being used in systems.
Yes, I guess the changelog should mention that AM33xx does not have the 32k synctimer. I'll also add in the OMAP details that you pointed out so that all the details get captured.
Quoted 11 lines
Quoted 11 lines
AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
Quoted 2 lines
So you use WKUP domain timer for clocksource and PER domain one for clock-event ?
Yes, that's correct.
Quoted 16 lines
Quoted 16 lines
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)arch/arm/mach-omap2/timer.c +0
-0@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } }
Quoted 12 lines
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 146
Quoted 9 lines
arch/arm/mach-omap2/timer.c +9
-0+static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ;You can move all the look up stuff in init code and then suspend resume hooks will be cleaner.
Will do. Kevin also pointed this out.
Quoted 26 lines
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+ +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 );arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 166
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+ oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW ); +} +OK. So since your
clk_eventstops when PER idles, how do you plan to support the SOC idle. For CPUIDLE path, you need your clock-event to wakeup the system based on next timer expiry. So you need your clock event to be active. Indirectly, you can't let PER idle which leads npo COREidle->SOCidle.
Quoted 3 lines
How do you plan to address this ? Os is SOC idle is not suppose to be added for AMXXX ?
We can't really have SOC idle on AM33xx or at least that's what I think. The deepest that we should be able to support is MPU off with external memory in self-refresh mode. I mentioned the reasons for that in the reply to Kevin [1]. If there's any another approach that we could take that would be great to know.
Regards,
Vaibhav
[1] http://marc.info/?l=linux-arm-kernel&m=135195053104053&w=2
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Quoted 95 lines · Vaibhav
Hi Santosh,
Quoted 10 lines · Santosh
Quoted 4 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent.
Actually OMAP also uses only one timer. The clocksource is taken care by 32K syntimer till OMAP4 and by realtime counter on OMAP5. There is a clocksource registration of timer is available but that is not being used in systems.
Yes, I guess the changelog should mention that AM33xx does not have the 32k synctimer. I'll also add in the OMAP details that you pointed out so that all the details get captured.
Quoted 11 lines
Quoted 11 lines
AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
Quoted 2 lines
So you use WKUP domain timer for clocksource and PER domain one for clock-event ?
Yes, that's correct.
Quoted 16 lines
Quoted 16 lines
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)arch/arm/mach-omap2/timer.c +0
-0@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } }Quoted 12 lines
arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 146
Quoted 9 lines
arch/arm/mach-omap2/timer.c +9
-0+static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ;You can move all the look up stuff in init code and then suspend resume hooks will be cleaner.
Will do. Kevin also pointed this out.
Quoted 26 lines
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+ +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 );arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 166
Quoted 10 lines
arch/arm/mach-omap2/timer.c +10
-0+ oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW ); +} +OK. So since your
clk_eventstops when PER idles, how do you plan to support the SOC idle. For CPUIDLE path, you need your clock-event to wakeup the system based on next timer expiry. So you need your clock event to be active. Indirectly, you can't let PER idle which leads npo COREidle->SOCidle.Quoted 3 lines
How do you plan to address this ? Os is SOC idle is not suppose to be added for AMXXX ?
Quoted 6 lines
We can't really have SOC idle on AM33xx or at least that's what I think. The deepest that we should be able to support is MPU off with external memory in self-refresh mode. I mentioned the reasons for that in the reply to Kevin [1]. If there's any another approach that we could take that would be great to know.
Thanks for information.
Regards
Santosh
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
In reply to Vaibhav BediaQuoted 62 lines · Vaibhav Bedia
From: Vaibhav Hiremath <[email protected]>
The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.
commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-)arch/arm/mach-omap2/timer.c +29
-0@@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, } } +static void omap_clkevt_suspend (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_idle (oh); +} + +static void omap_clkevt_resume (struct clock_event_device *unused) +{ +char name[10 ]; +struct omap_hwmod *oh; + +sprintf (name,"timer%d" ,2 ); + oh =omap_hwmod_lookup (name); +if (!oh) +return ; + +omap_hwmod_enable (oh); +__omap_dm_timer_load_start (&clkev, +OMAP_TIMER_CTRL_ST |OMAP_TIMER_CTRL_AR ,0 ,1 ); +__omap_dm_timer_int_enable (&clkev,OMAP_TIMER_INT_OVERFLOW ); +} +static struct clock_event_device clockevent_gpt = { .name ="gp_timer" , .features =CLOCK_EVT_FEAT_PERIODIC |CLOCK_EVT_FEAT_ONESHOT ,
arch/arm/mach-omap2/timer.c static struct clock_event_device clockevent_gpt = { · line 175
Quoted 5 lines
arch/arm/mach-omap2/timer.c +2
-0@@ -142,6 +171,8 @@ static struct clock_event_device clockevent_gpt = { .rating = 300, .set_next_event = omap2_gp_timer_set_next_event, .set_mode = omap2_gp_timer_set_mode, + .suspend = omap_clkevt_suspend, + .resume = omap_clkevt_resume,
So these suspend/resume callbacks are going to be called for all OMAP2+ and AMxxxx devices? I don't think we want that. AFAIK OMAP timers will idle on their own when stopped and don't require this.
Cheers
Jon
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Hi Jon,
On Tue, Nov 06, 2012 at 02:34:05, Hunter, Jon wrote:
[...]
Quoted 5 lines
Quoted 5 lines
static struct
clock_event_deviceclockevent_gpt= {arch/arm/mach-omap2/timer.c +0
-0.name = "gp_timer", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,@@ -142,6 +171,8 @@ static struct clock_event_device clockevent_gpt = { .rating = 300,
Quoted 9 lines
arch/arm/mach-omap2/timer.c static struct clock_event_device clockevent_gpt = { · line 175
Quoted 4 lines
arch/arm/mach-omap2/timer.c +2
-0.set_next_event = omap2_gp_timer_set_next_event, .set_mode = omap2_gp_timer_set_mode, + .suspend = omap_clkevt_suspend, + .resume = omap_clkevt_resume,So these suspend/resume callbacks are going to be called for all OMAP2+ and AMxxxx devices? I don't think we want that. AFAIK OMAP timers will idle on their own when stopped and don't require this.
IMO instead of skipping the callback registration we could have checks in the suspend/resume callbacks to decide what to do.
I'll check if the idling part is AM33xx specific. If not, based on the recent timer changes that you did, perhaps checking if the clockevent selected doesn't have the "ti,timer-alwon" capability will be good enough. What do you think?
Regards.
Vaibhav
PATCH 12/15 ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
Quoted 19 lines · Vaibhav
Hi Jon,
On Tue, Nov 06, 2012 at 02:34:05, Hunter, Jon wrote:
[...]Quoted 5 lines
Quoted 5 lines
static struct
clock_event_deviceclockevent_gpt= {arch/arm/mach-omap2/timer.c +0
-0.name = "gp_timer", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,@@ -142,6 +171,8 @@ static struct clock_event_device clockevent_gpt = { .rating = 300,Quoted 9 lines
arch/arm/mach-omap2/timer.c static struct clock_event_device clockevent_gpt = { · line 175
Quoted 4 lines
arch/arm/mach-omap2/timer.c +2
-0.set_next_event = omap2_gp_timer_set_next_event, .set_mode = omap2_gp_timer_set_mode, + .suspend = omap_clkevt_suspend, + .resume = omap_clkevt_resume,So these suspend/resume callbacks are going to be called for all OMAP2+ and AMxxxx devices? I don't think we want that. AFAIK OMAP timers will idle on their own when stopped and don't require this.
Quoted 6 lines
IMO instead of skipping the callback registration we could have checks in the suspend/resume callbacks to decide what to do.
I'll check if the idling part is AM33xx specific. If not, based on the recent timer changes that you did, perhaps checking if the clockevent selected doesn't have the "ti,timer-alwon" capability will be good enough. What do you think?
Yes, I was thinking along the same lines. If I get chance I will try and test your scenario on an OMAP3 too.
Cheers
Jon
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
In reply to Vaibhav Bedia--- arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
arch/arm/boot/dts/am33xx.dtsi +11 -0
@@ -210,5 +210,16 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + ocmcram: ocmcram at 40300000 { + compatible = "ti,ocmcram"; + ti,hwmods = "ocmcram"; + ti,no_idle_on_suspend; + }; + + wkup_m3: wkup_m3 at 44d00000 { + compatible = "ti,wkup_m3"; + ti,hwmods = "wkup_m3"; + }; }; };
Signature · 1 line
1.7.0.4
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 1 line · Vaibhav Bedia
Signed-off-by Vaibhav Bedia <[email protected]>
Even simple patches need a simple changelog.
Kevin
Quoted 25 lines
arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)arch/arm/boot/dts/am33xx.dtsi +11
-0@@ -210,5 +210,16 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + ocmcram: ocmcram at 40300000 { + compatible = "ti,ocmcram"; + ti,hwmods = "ocmcram"; + ti,no_idle_on_suspend; + }; + + wkup_m3: wkup_m3 at 44d00000 { + compatible = "ti,wkup_m3"; + ti,hwmods = "wkup_m3"; + }; }; };
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 4 lines · Kevin Hilman
Quoted 1 line · Vaibhav Bedia
Signed-off-by Vaibhav Bedia <[email protected]>Even simple patches need a simple changelog.
Again, sorry about this. Will take care in the future.
Regards,
Vaibhav
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
In reply to Vaibhav BediaQuoted 10 lines · Vaibhav Bedia
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)arch/arm/boot/dts/am33xx.dtsi +0
-0
arch/arm/boot/dts/am33xx.dtsi line 218
Quoted 9 lines
arch/arm/boot/dts/am33xx.dtsi +6
-0@@ -210,5 +210,16 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + ocmcram: ocmcram at 40300000 { + compatible = "ti,ocmcram"; + ti,hwmods = "ocmcram"; + ti,no_idle_on_suspend; + };
Whats the intention behind adding OCMRAM ? Sorry if I missed any comments from the cover letter ?
Regards
Santosh
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 6 lines · Santosh
Quoted 5 lines · Vaibhav Bedia
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
Quoted 17 lines
Quoted 5 lines
arch/arm/boot/dts/am33xx.dtsi +0
-0arch/arm/boot/dts/am33xx.dtsi line 218
Quoted 9 lines
arch/arm/boot/dts/am33xx.dtsi +6
-0@@ -210,5 +210,16 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + ocmcram: ocmcram at 40300000 { + compatible = "ti,ocmcram"; + ti,hwmods = "ocmcram"; + ti,no_idle_on_suspend; + };Whats the intention behind adding OCMRAM ? Sorry if I missed any comments from the cover letter ?
We need a mechanism to ensure that the clock to OCMC is kept running during boot and that it doesn't get disabled as part of the suspend sequence. Since the hwmod data for OCMC is already present and we have the no_idle_on_suspend flag for hwmod entries we get the desired behavior.
This could also have been done via the clock tree but looks like we want to avoid adding leaf nodes in the clock data, hence the hwmod + DT approach.
Regards,
Vaibhav
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 25 lines · Vaibhav
Quoted 6 lines · Santosh
Quoted 5 lines · Vaibhav Bedia
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)Quoted 17 lines
Quoted 5 lines
arch/arm/boot/dts/am33xx.dtsi +0
-0arch/arm/boot/dts/am33xx.dtsi line 218
Quoted 9 lines
arch/arm/boot/dts/am33xx.dtsi +6
-0@@ -210,5 +210,16 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + ocmcram: ocmcram at 40300000 { + compatible = "ti,ocmcram"; + ti,hwmods = "ocmcram"; + ti,no_idle_on_suspend; + };Whats the intention behind adding OCMRAM ? Sorry if I missed any comments from the cover letter ?
Quoted 5 lines
We need a mechanism to ensure that the clock to OCMC is kept running during boot and that it doesn't get disabled as part of the suspend sequence. Since the hwmod data for OCMC is already present and we have the
no_idle_on_suspendflag for hwmod entries we get the desired behavior.
On OMAP the OCMC RAM is always clocked and doesn't need any special clock enable. CM_L3_2_OCMC_RAM_CLKCTRL module mode field is read only. Isn't it same on AMXX ?
Quoted 4 lines
This could also have been done via the clock tree but looks like we want to avoid adding leaf nodes in the clock data, hence the hwmod + DT approach.
Sure. I was just trying to see why AMXX is different with OMAP here.
Regards
Santosh
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
On Mon, Nov 05, 2012 at 20:23:11, Shilimkar, Santosh wrote:
[...]
Quoted 5 lines
Quoted 1 line
On OMAP the OCMC RAM is always clocked and doesn't need any special clock enable.
CM_L3_2_OCMC_RAM_CLKCTRLmodule mode field is read only. Isn't it same on AMXX ?
On AM33xx, OCMC RAM is in PER domain and the corresponding CLKCLTR module mode fields are r/w. OCMC RAM needs to be disabled as part of the DeepSleep0 entry to let PER domain transition.
Regards,
Vaibhav
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 11 lines · Vaibhav
On Mon, Nov 05, 2012 at 20:23:11, Shilimkar, Santosh wrote:
[...]Quoted 5 lines
Quoted 1 line
On OMAP the OCMC RAM is always clocked and doesn't need any special clock enable.
CM_L3_2_OCMC_RAM_CLKCTRLmodule mode field is read only. Isn't it same on AMXX ?On AM33xx, OCMC RAM is in PER domain and the corresponding CLKCLTR module mode fields are r/w. OCMC RAM needs to be disabled as part of the DeepSleep0 entry to let PER domain transition.
After DeepSleep0, the ROM code is being given an address in OCMC RAM to jump to. If OCMC RAM is disabled as part of suspend, this means that OCMC RAM contents are maintained even though PER domain transitions?
If so, that needs to be more clearly documented.
Kevin
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 14 lines · Kevin Hilman
Quoted 11 lines · Vaibhav
On Mon, Nov 05, 2012 at 20:23:11, Shilimkar, Santosh wrote:
[...]Quoted 5 lines
Quoted 1 line
On OMAP the OCMC RAM is always clocked and doesn't need any special clock enable.
CM_L3_2_OCMC_RAM_CLKCTRLmodule mode field is read only. Isn't it same on AMXX ?On AM33xx, OCMC RAM is in PER domain and the corresponding CLKCLTR module mode fields are r/w. OCMC RAM needs to be disabled as part of the DeepSleep0 entry to let PER domain transition.
Quoted 6 lines
After DeepSleep0, the ROM code is being given an address in OCMC RAM to jump to. If OCMC RAM is disabled as part of suspend, this means that OCMC RAM contents are maintained even though PER domain transitions?
If so, that needs to be more clearly documented.
Thats very good point. How does OCMC RAM retains the contents without clock ?
Regards
Santosh
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 17 lines · Santosh Shilimkar
Quoted 16 lines · Kevin Hilman
Quoted 13 lines · Vaibhav
On Mon, Nov 05, 2012 at 20:23:11, Shilimkar, Santosh wrote:
[...]Quoted 5 lines
Quoted 1 line
On OMAP the OCMC RAM is always clocked and doesn't need any special clock enable.
CM_L3_2_OCMC_RAM_CLKCTRLmodule mode field is read only. Isn't it same on AMXX ?On AM33xx, OCMC RAM is in PER domain and the corresponding CLKCLTR module
mode fields are r/w. OCMC RAM needs to be disabled as part of the DeepSleep0
entry to let PER domain transition.
Quoted 9 lines
Quoted 6 lines
After DeepSleep0, the ROM code is being given an address in OCMC RAM to jump to. If OCMC RAM is disabled as part of suspend, this means that OCMC RAM contents are maintained even though PER domain transitions?
If so, that needs to be more clearly documented.
Thats very good point. How does OCMC RAM retains the contents without clock ?
Ignore the question. I figured out from other patch changelog the OCMC RAM supports retention. Please have that clearly captured in change log.
Regards
Santosh
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
Quoted 22 lines · Santosh
Quoted 21 lines · Santosh Shilimkar
Quoted 20 lines · Kevin Hilman
Quoted 8 lines · Vaibhav
On Mon, Nov 05, 2012 at 20:23:11, Shilimkar, Santosh wrote:
[...]Quoted 5 lines
Quoted 1 line
On OMAP the OCMC RAM is always clocked and doesn't need any special clock enable.
CM_L3_2_OCMC_RAM_CLKCTRLmodule mode field is read only. Isn't it same on AMXX ?Quoted 5 lines
On AM33xx, OCMC RAM is in PER domain and the corresponding CLKCLTR module
mode fields are r/w. OCMC RAM needs to be disabled as part of the DeepSleep0
entry to let PER domain transition.After DeepSleep0, the ROM code is being given an address in OCMC RAM to jump to. If OCMC RAM is disabled as part of suspend, this means that OCMC RAM contents are maintained even though PER domain transitions?
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
If so, that needs to be more clearly documented.
Thats very good point. How does OCMC RAM retains the contents without clock ?
Ignore the question. I figured out from other patch changelog the OCMC RAM supports retention. Please have that clearly captured in change log.
Yes, OCMC RAM support retention. Will document that here also.
Regards,
Vaibhav
PATCH 13/15 ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
In reply to Bedia, VaibhavQuoted 25 lines · Vaibhav
Quoted 6 lines · Santosh
Quoted 5 lines · Vaibhav Bedia
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)Quoted 17 lines
Quoted 5 lines
arch/arm/boot/dts/am33xx.dtsi +0
-0arch/arm/boot/dts/am33xx.dtsi line 218
Quoted 9 lines
arch/arm/boot/dts/am33xx.dtsi +6
-0@@ -210,5 +210,16 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + ocmcram: ocmcram at 40300000 { + compatible = "ti,ocmcram"; + ti,hwmods = "ocmcram"; + ti,no_idle_on_suspend; + };Whats the intention behind adding OCMRAM ? Sorry if I missed any comments from the cover letter ?
Quoted 9 lines
We need a mechanism to ensure that the clock to OCMC is kept running during boot and that it doesn't get disabled as part of the suspend sequence. Since the hwmod data for OCMC is already present and we have the
no_idle_on_suspendflag for hwmod entries we get the desired behavior.This could also have been done via the clock tree but looks like we want to avoid adding leaf nodes in the clock data, hence the hwmod + DT approach.
OK. I was just comparing the OCMC RAM on OMAP which is always clocked and hence didn't need any of the above.
Regards
Santosh
PATCH 14/15 ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
In reply to Vaibhav BediaAM33XX PM code depends on Mailbox module for IPC
between MPU and WKUP_M3.
--- arch/arm/configs/omap2plus_defconfig | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/configs/omap2plus_defconfig +2 -0
@@ -23,6 +23,8 @@ CONFIG_MODULE_SRCVERSION_ALL=y CONFIG_ARCH_OMAP=y CONFIG_OMAP_RESET_CLOCKS=y CONFIG_OMAP_MUX_DEBUG=y +CONFIG_OMAP_MBOX_FWK=y +CONFIG_OMAP_MBOX_KFIFO_SIZE=256 CONFIG_ARM_THUMBEE=y CONFIG_ARM_ERRATA_411920=y CONFIG_NO_HZ=y
Signature · 1 line
1.7.0.4
PATCH 14/15 ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
Quoted 2 lines · Vaibhav Bedia
AM33XX PM code depends on Mailbox module for IPC
between MPU andWKUP_M3.
OK, but what if I try to build for AM33xx without starting from omap2plus_defconfig?
IOW, instead of changing the default defconfig, AM33xx should select this when PM
is enabled. Something like the (untested) change below.
Kevin
arch/arm/mach-omap2/Kconfig +1 -0
@@ -109,6 +109,7 @@ config SOC_AM33XX bool "AM33XX support" default y select ARM_CPU_SUSPEND if PM + select OMAP_MBOX_FWK if PM select CPU_V7 select MULTI_IRQ_HANDLER
PATCH 14/15 ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
Quoted 6 lines · Kevin Hilman
Quoted 2 lines · Vaibhav Bedia
AM33XX PM code depends on Mailbox module for IPC
between MPU andWKUP_M3.OK, but what if I try to build for AM33xx without starting from
omap2plus_defconfig?
I honestly didn't think about this scenario.
Quoted 5 lines
IOW, instead of changing the default defconfig, AM33xx should select this when PM
is enabled. Something like the (untested) change below.
Will try it out. Thanks for the suggestion.
Regards,
Vaibhav
PATCH 14/15 ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
Quoted 11 lines · Vaibhav
Quoted 6 lines · Kevin Hilman
Quoted 2 lines · Vaibhav Bedia
AM33XX PM code depends on Mailbox module for IPC
between MPU andWKUP_M3.OK, but what if I try to build for AM33xx without starting from
omap2plus_defconfig?I honestly didn't think about this scenario.
Quoted 1 line
Quoted 6 lines
Quoted 4 lines
IOW, instead of changing the default defconfig, AM33xx should select this when PM
is enabled. Something like the (untested) change below.Will try it out. Thanks for the suggestion.
You're welcome.
See... sometimes maintainers can be useful for something other than complaining. ;)
Kevin
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
In reply to Vaibhav BediaAM335x supports various low power modes as documented in section 8.1.4.3 of the AM335x TRM which is available @ http://www.ti.com/litv/pdf/spruh73f
DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a system reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate resume process. As part of the boot process, the assembly code is copied over to OCMCRAM using the OMAP SRAM code.
AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU in DeepSleep0 entry and exit. WKUP_M3 takes care of the clockdomain and powerdomain transitions based on the intended low power state. MPU needs to load the appropriate WKUP_M3 binary onto the WKUP_M3 memory space before it can leverage any of the PM features like DeepSleep.
The IPC mechanism between MPU and WKUP_M3 uses a mailbox sub-module and 8 IPC registers in the Control module. MPU uses the assigned Mailbox for issuing an interrupt to WKUP_M3 which then goes and checks the IPC registers for the payload. WKUP_M3 has the ability to trigger on interrupt to MPU by executing the "sev" instruction.
In the current implementation when the suspend process is initiated MPU interrupts the WKUP_M3 to let about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received, MPU continues with its suspend process to suspend all the drivers and then jumps to assembly in OCMC RAM to put the PLLs in bypass, put the external RAM in self-refresh mode and then finally execute the WFI instruction. The WFI instruction triggers another interrupt to the WKUP_M3 which then continues wiht the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt lines for the wakeup sources. When WKUP_M3 executes WFI, the hardware disables the main oscillator.
When a wakeup event occurs, WKUP_M3 starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence, in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence.
The low level code in OCMC relocks the PLLs, enables access to external RAM and then jumps to the cpu_resume code of the kernel to finish the resume process.
--- arch/arm/mach-omap2/Makefile | 2 + arch/arm/mach-omap2/board-generic.c | 1 + arch/arm/mach-omap2/common.h | 10 + arch/arm/mach-omap2/io.c | 7 + arch/arm/mach-omap2/pm.h | 7 + arch/arm/mach-omap2/pm33xx.c | 429 ++++++++++++++++++++++++++ arch/arm/mach-omap2/pm33xx.h | 100 ++++++ arch/arm/mach-omap2/sleep33xx.S | 571 +++++++++++++++++++++++++++++++++++ arch/arm/plat-omap/sram.c | 10 +- arch/arm/plat-omap/sram.h | 2 + 10 files changed, 1138 insertions(+), 1 deletions(-) create mode 100644 arch/arm/mach-omap2/pm33xx.c create mode 100644 arch/arm/mach-omap2/pm33xx.h create mode 100644 arch/arm/mach-omap2/sleep33xx.S
arch/arm/mach-omap2/Makefile +2 -0
@@ -71,6 +71,7 @@ endif ifeq ($(CONFIG_PM),y) obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o +obj-$(CONFIG_SOC_AM33XX) += pm33xx.o sleep33xx.o obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o omap-mpuss-lowpower.o obj-$(CONFIG_SOC_OMAP5) += omap-mpuss-lowpower.o
@@ -80,6 +81,7 @@ obj-$(CONFIG_POWER_AVS_OMAP) += sr_device.o obj-$(CONFIG_POWER_AVS_OMAP_CLASS3) += smartreflex-class3.o AFLAGS_sleep24xx.o :=-Wa,-march=armv6 +AFLAGS_sleep33xx.o :=-Wa,-march=armv7-a$(plus_sec) AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a$(plus_sec) ifeq ($(CONFIG_PM_VERBOSE),y)
arch/arm/mach-omap2/board-generic.c +1 -0
@@ -109,6 +109,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)") .reserve = omap_reserve, .map_io = am33xx_map_io, .init_early = am33xx_init_early, + .init_late = am33xx_init_late, .init_irq = omap_intc_of_init, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_generic_init,
arch/arm/mach-omap2/common.h +10 -0
@@ -109,6 +109,15 @@ static inline int omap3_pm_init(void) }#endif +#if defined(CONFIG_PM ) && defined(CONFIG_SOC_AM33XX ) +int am33xx_pm_init (void ); +#else +static inline int am33xx_pm_init (void ) +{ +return 0 ; +} +#endif +#if defined(CONFIG_PM ) && defined(CONFIG_ARCH_OMAP4 )int omap4_pm_init (void );#else
@@ -157,6 +166,7 @@ void am33xx_init_early(void);void omap4430_init_early (void );void omap5_init_early (void );void omap3_init_late (void );/* Do not use this one */ +void am33xx_init_late (void );void omap4430_init_late (void );void omap2420_init_late (void );void omap2430_init_late (void );
arch/arm/mach-omap2/io.c +7 -0
@@ -528,6 +528,13 @@ void __init am33xx_init_early(void)omap_hwmod_init_postsetup (); am33xx_clk_init(); } + +void __init am33xx_init_late (void ) +{ +omap_mux_late_init (); +omap2_common_pm_late_init (); +am33xx_pm_init (); +}#endif #ifdef CONFIG_ARCH_OMAP4
arch/arm/mach-omap2/pm.h +7 -0
@@ -83,6 +83,13 @@ extern unsigned int omap3_do_wfi_sz;/* ... and its pointer from SRAM after copy */ extern void (*omap3_do_wfi_sram)(void ); +/* am33xx_do_wfi function pointer and size, for copy to SRAM */ +extern void am33xx_do_wfi (void ); +extern unsigned int am33xx_do_wfi_sz; +extern unsigned int am33xx_resume_offset; +/* ... and its pointer from SRAM after copy */ +extern void (*am33xx_do_wfi_sram)(void ); +/* save_secure_ram_context function pointer and size, for copy to SRAM */ extern int save_secure_ram_context (u32 *addr);extern unsigned int save_secure_ram_context_sz;
arch/arm/mach-omap2/pm33xx.c new file +429 -0
429 lines not on this page · raw message
arch/arm/mach-omap2/pm33xx.h new file +100 -0
100 lines not on this page · raw message
arch/arm/mach-omap2/sleep33xx.S new file +571 -0
571 lines not on this page · raw message
arch/arm/plat-omap/sram.c +9 -1
@@ -182,7 +182,7 @@ static void __init omap_map_sram(void) omap_sram_size -= SZ_16K; }#endif -if (cpu_is_omap34xx ()) { +if (cpu_is_omap34xx () ||soc_is_am33xx ()) {/* * SRAM must be marked as non-cached on OMAP3 since the * CORE DPLL M2 divider change code (in SRAM) runs with the
@@ -381,10 +381,18 @@ static inline int omap34xx_sram_init(void) }#endif /* CONFIG_ARCH_OMAP3 */ +#ifdef CONFIG_SOC_AM33XX static inline int am33xx_sram_init (void ) { +am33xx_push_sram_idle ();return 0 ; } +#else +static inline int am33xx_sram_init (void ) +{ +return 0 ; +} +#endif int __init omap_sram_init (void ) {
arch/arm/plat-omap/sram.h +2 -0
@@ -85,8 +85,10 @@ extern unsigned long omap3_sram_configure_core_dpll_sz;#ifdef CONFIG_PM extern void omap_push_sram_idle (void ); +extern void am33xx_push_sram_idle (void );#else static inline void omap_push_sram_idle (void ) {} +static inline void am33xx_push_sram_idle (void ) {}#endif /* CONFIG_PM */ #endif /* __ASSEMBLY__ */
Signature · 1 line
1.7.0.4
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
On Fri, Nov 02, 2012 at 18:02:46, Bedia, Vaibhav wrote:
[...]
arch/arm/mach-omap2/pm33xx.c line 257
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c new file +4
-0+int wkup_mbox_msg (struct notifier_block *self,unsigned long len,void *msg) +{ +return 0 ; +}
This should have been static. Will change in the next version.
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
In reply to Vaibhav BediaQuoted 49 lines · Vaibhav Bedia
AM335x supports various low power modes as documented in section 8.1.4.3 of the AM335x TRM which is available @ http://www.ti.com/litv/pdf/spruh73f
DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a system reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate resume process. As part of the boot process, the assembly code is copied over to OCMCRAM using the OMAP SRAM code.
AM335x has a Cortex-M3 (
WKUP_M3) which assists the MPU in DeepSleep0 entry and exit.WKUP_M3takes care of the clockdomain and powerdomain transitions based on the intended low power state. MPU needs to load the appropriateWKUP_M3binary onto theWKUP_M3memory space before it can leverage any of the PM features like DeepSleep.The IPC mechanism between MPU and
WKUP_M3uses a mailbox sub-module and 8 IPC registers in the Control module. MPU uses the assigned Mailbox for issuing an interrupt toWKUP_M3which then goes and checks the IPC registers for the payload.WKUP_M3has the ability to trigger on interrupt to MPU by executing the "sev" instruction.In the current implementation when the suspend process is initiated MPU interrupts the
WKUP_M3to let about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received, MPU continues with its suspend process to suspend all the drivers and then jumps to assembly in OCMC RAM to put the PLLs in bypass, put the external RAM in self-refresh mode and then finally execute the WFI instruction. The WFI instruction triggers another interrupt to theWKUP_M3which then continues wiht the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence,WKUP_M3unmasks the interrupt lines for the wakeup sources. WhenWKUP_M3executes WFI, the hardware disables the main oscillator.When a wakeup event occurs,
WKUP_M3starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence, in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence.
Quoted 4 lines
The low level code in OCMC relocks the PLLs, enables access to external RAM and then jumps to the
cpu_resumecode of the kernel to finish the resume process.
Nice descriptive change log Vaibhav.
Quoted 155 lines
Signed-off-by Vaibhav Bedia <[email protected]>--- arch/arm/mach-omap2/Makefile | 2 + arch/arm/mach-omap2/board-generic.c | 1 + arch/arm/mach-omap2/common.h | 10 + arch/arm/mach-omap2/io.c | 7 + arch/arm/mach-omap2/pm.h | 7 + arch/arm/mach-omap2/pm33xx.c | 429 ++++++++++++++++++++++++++ arch/arm/mach-omap2/pm33xx.h | 100 ++++++ arch/arm/mach-omap2/sleep33xx.S | 571 +++++++++++++++++++++++++++++++++++ arch/arm/plat-omap/sram.c | 10 +- arch/arm/plat-omap/sram.h | 2 + 10 files changed, 1138 insertions(+), 1 deletions(-)create mode 100644
arch/arm/mach-omap2/pm33xx.ccreate mode 100644arch/arm/mach-omap2/pm33xx.hcreate mode 100644arch/arm/mach-omap2/sleep33xx.Sarch/arm/mach-omap2/Makefile +2
-0@@ -71,6 +71,7 @@ endif ifeq ($(CONFIG_PM),y) obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o +obj-$(CONFIG_SOC_AM33XX) += pm33xx.o sleep33xx.o obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o omap-mpuss-lowpower.o obj-$(CONFIG_SOC_OMAP5) += omap-mpuss-lowpower.o@@ -80,6 +81,7 @@ obj-$(CONFIG_POWER_AVS_OMAP) += sr_device.o obj-$(CONFIG_POWER_AVS_OMAP_CLASS3) += smartreflex-class3.o AFLAGS_sleep24xx.o :=-Wa,-march=armv6 +AFLAGS_sleep33xx.o :=-Wa,-march=armv7-a$(plus_sec) AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a$(plus_sec) ifeq ($(CONFIG_PM_VERBOSE),y)arch/arm/mach-omap2/board-generic.c +1
-0@@ -109,6 +109,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)") .reserve = omap_reserve, .map_io = am33xx_map_io, .init_early = am33xx_init_early, + .init_late = am33xx_init_late, .init_irq = omap_intc_of_init, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_generic_init,arch/arm/mach-omap2/common.h +10
-0@@ -109,6 +109,15 @@ static inline int omap3_pm_init(void) }#endif +#if defined(CONFIG_PM ) && defined(CONFIG_SOC_AM33XX ) +int am33xx_pm_init (void ); +#else +static inline int am33xx_pm_init (void ) +{ +return 0 ; +} +#endif +#if defined(CONFIG_PM ) && defined(CONFIG_ARCH_OMAP4 )int omap4_pm_init (void );#else @@ -157,6 +166,7 @@ void am33xx_init_early(void);void omap4430_init_early (void );void omap5_init_early (void );void omap3_init_late (void );/* Do not use this one */ +void am33xx_init_late (void );void omap4430_init_late (void );void omap2420_init_late (void );void omap2430_init_late (void );arch/arm/mach-omap2/io.c +7
-0@@ -528,6 +528,13 @@ void __init am33xx_init_early(void)omap_hwmod_init_postsetup (); am33xx_clk_init(); } + +void __init am33xx_init_late (void ) +{ +omap_mux_late_init (); +omap2_common_pm_late_init (); +am33xx_pm_init (); +}#endif #ifdef CONFIG_ARCH_OMAP4 arch/arm/mach-omap2/pm.h +7
-0@@ -83,6 +83,13 @@ extern unsigned int omap3_do_wfi_sz;/* ... and its pointer from SRAM after copy */ extern void (*omap3_do_wfi_sram)(void ); +/* am33xx_do_wfi function pointer and size, for copy to SRAM */ +extern void am33xx_do_wfi (void ); +extern unsigned int am33xx_do_wfi_sz; +extern unsigned int am33xx_resume_offset; +/* ... and its pointer from SRAM after copy */ +extern void (*am33xx_do_wfi_sram)(void ); +/* save_secure_ram_context function pointer and size, for copy to SRAM */ extern int save_secure_ram_context (u32 *addr);extern unsigned int save_secure_ram_context_sz;arch/arm/mach-omap2/pm33xx.c new file +36
-0@@ -0,0 +1,429 @@ +/* +* AM33XX Power Management Routines +* +* Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ +* Vaibhav Bedia <[email protected]> +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation version 2. +* +* This program is distributed "as is" WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/firmware.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/sched.h> +#include <linux/suspend.h> +#include <linux/completion.h> +#include <linux/module.h> + +#include <plat/prcm.h> +#include <plat/mailbox.h> +#include "../plat-omap/sram.h" + +#include <asm/suspend.h> +#include <asm/proc-fns.h> +#include <asm/sizes.h> +#include <asm/system_misc.h> +
arch/arm/mach-omap2/pm33xx.c line 46
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+#include "pm.h" +#include "cm33xx.h" +#include "pm33xx.h" +#include "control.h" +#include "clockdomain.h" +#include "powerdomain.h" +#include "omap_hwmod.h" +#include "omap_device.h" +#include "soc.h" +
In case not checked yet, see if you need
all above headers.
Quoted 25 lines
arch/arm/mach-omap2/pm33xx.c new file +25
-0+void (*am33xx_do_wfi_sram)(void ); + +static void __iomem *am33xx_emif_base; +static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm; +static struct clockdomain *gfx_l3_clkdm, *gfx_l4ls_clkdm; +static struct wkup_m3_context *wkup_m3; + +static DECLARE_COMPLETION (wkup_m3_sync); + +#ifdef CONFIG_SUSPEND +static int am33xx_do_sram_idle (long unsigned int unused) +{ +am33xx_do_wfi_sram (); +return 0 ; +} + +static int am33xx_pm_suspend (void ) +{ +int status, ret =0 ; + +struct omap_hwmod *gpmc_oh, *usb_oh; +struct omap_hwmod *tptc0_oh, *tptc1_oh, *tptc2_oh; + +/* +* By default the following IPs do not have MSTANDBY asserted
arch/arm/mach-omap2/pm33xx.c line 81
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+* which is necessary for PER domain transition. If the drivers +* are not compiled into the kernel HWMOD code will not change the +* state of the IPs if the IP was not never enabled +*/ + usb_oh =omap_hwmod_lookup ("usb_otg_hs" ); + gpmc_oh =omap_hwmod_lookup ("gpmc" ); + tptc0_oh =omap_hwmod_lookup ("tptc0" ); + tptc1_oh =omap_hwmod_lookup ("tptc1" ); + tptc2_oh =omap_hwmod_lookup ("tptc2" ); +
This look you don't need every suspend.
arch/arm/mach-omap2/pm33xx.c line 93
Quoted 12 lines
arch/arm/mach-omap2/pm33xx.c new file +12
-0+omap_hwmod_enable (usb_oh); +omap_hwmod_enable (gpmc_oh); +omap_hwmod_enable (tptc0_oh); +omap_hwmod_enable (tptc1_oh); +omap_hwmod_enable (tptc2_oh); + +omap_hwmod_idle (usb_oh); +omap_hwmod_idle (gpmc_oh); +omap_hwmod_idle (tptc0_oh); +omap_hwmod_idle (tptc1_oh); +omap_hwmod_idle (tptc2_oh); +
Calling omap_hwmod_idle() directly tells me something is not right. Can these module not idle themself with respective device drivers ?
arch/arm/mach-omap2/pm33xx.c line 96
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+/* Put the GFX clockdomains to sleep */ +clkdm_sleep (gfx_l3_clkdm); +clkdm_sleep (gfx_l4ls_clkdm);
Can GFX driver suspend code not take care of above ? Also are these clock domains are not supporting HW supervised mode ?
arch/arm/mach-omap2/pm33xx.c line 100
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+/* Try to put GFX to sleep */ +pwrdm_set_next_pwrst (gfx_pwrdm,PWRDM_POWER_OFF ); +
Above as well can be taken care by constraint QOS API by GFX driver.
Quoted 23 lines
arch/arm/mach-omap2/pm33xx.c new file +23
-0+ ret =cpu_suspend (0 , am33xx_do_sram_idle); + + status =pwrdm_read_pwrst (gfx_pwrdm); +if (status !=PWRDM_POWER_OFF ) +pr_err ("GFX domain did not transition\n" ); +else +pr_info ("GFX domain entered low power state\n" ); + +/* Needed to ensure L4LS clockdomain transitions properly */ +clkdm_wakeup (gfx_l3_clkdm); +clkdm_wakeup (gfx_l4ls_clkdm); + +if (ret) { +pr_err ("Kernel suspend failure\n" ); + }else { + status =omap_ctrl_readl (AM33XX_CONTROL_IPC_MSG_REG1 ); + status &=IPC_RESP_MASK ; + status >>=__ffs (IPC_RESP_MASK ); + +switch (status) { +case 0 : +pr_info ("Successfully transitioned to low power state\n" ); +if (wkup_m3->sleep_mode ==IPC_CMD_DS0 )
arch/arm/mach-omap2/pm33xx.c line 133
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+/* XXX: Use SOC specific ops for this? */ + per_pwrdm->ret_logic_off_counter++; +break ; +case 1 : +pr_err ("Could not enter low power state\n" ); + ret =-1 ; +break ; +default : +pr_err ("Something is terribly wrong :(\nStatus = %d\n" , + status);
Sounds terrible :-)
Quoted 44 lines
arch/arm/mach-omap2/pm33xx.c new file +44
-0+ ret =-1 ; + } + } + +return ret; +} + +static int am33xx_pm_enter (suspend_state_t suspend_state) +{ +int ret =0 ; + +switch (suspend_state) { +case PM_SUSPEND_STANDBY : +case PM_SUSPEND_MEM : + ret =am33xx_pm_suspend (); +break ; +default : + ret = -EINVAL ; + } + +return ret; +} + +static int am33xx_pm_begin (suspend_state_t state) +{ +int ret =0 ; + +disable_hlt (); + +/* +* Physical resume address to be used by ROM code +*/ + wkup_m3->resume_addr = (AM33XX_OCMC_END - am33xx_do_wfi_sz + + am33xx_resume_offset +0x4 ); + + wkup_m3->sleep_mode =IPC_CMD_DS0 ; + wkup_m3->ipc_data1 =DS_IPC_DEFAULT ; + wkup_m3->ipc_data2 =DS_IPC_DEFAULT ; + +am33xx_ipc_cmd (); + + wkup_m3->state =M3_STATE_MSG_FOR_LP ; + +omap_mbox_enable_irq (wkup_m3->mbox,IRQ_RX );
arch/arm/mach-omap2/pm33xx.c line 187
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+ + ret =omap_mbox_msg_send (wkup_m3->mbox,0xABCDABCD ); +if (ret) { +pr_err ("A8<->CM3 MSG for LP failed\n" ); +am33xx_m3_state_machine_reset (); + ret =-1 ; + } + +if (!wait_for_completion_timeout (&wkup_m3_sync, +msecs_to_jiffies (500 ))) {
500 is from spec or arbitrary timeout ?
Quoted 31 lines
arch/arm/mach-omap2/pm33xx.c new file +31
-0+/* +* Push the minimal suspend-resume code to SRAM +*/ +void am33xx_push_sram_idle (void ) +{ + am33xx_do_wfi_sram = (void *)omap_sram_push + (am33xx_do_wfi, am33xx_do_wfi_sz); +} + +static int __init am33xx_map_emif (void ) +{ + am33xx_emif_base =ioremap (AM33XX_EMIF_BASE ,SZ_32K ); + +if (!am33xx_emif_base) +return -ENOMEM ; + +return 0 ; +} + +void __iomem *am33xx_get_emif_base (void ) +{ +return am33xx_emif_base; +} + +int __init am33xx_pm_init (void ) +{ +int ret; + +if (!soc_is_am33xx ()) +return -ENODEV ; +
arch/arm/mach-omap2/pm33xx.c line 406
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+pr_info ("Power Management for AM33XX family\n" ); + + wkup_m3 =kzalloc (sizeof (struct wkup_m3_context ),GFP_KERNEL ); +if (!wkup_m3) { +pr_err ("Memory allocation failed\n" ); +return -ENOMEM ; + } + + ret =am33xx_map_emif (); +
No EMIF driver to handle EMIF MAP, registers etc ?
Quoted 118 lines
arch/arm/mach-omap2/pm33xx.c new file +23
-0+ (void )clkdm_for_each (omap_pm_clkdms_setup,NULL ); + +/* CEFUSE domain should be turned off post bootup */ + cefuse_pwrdm =pwrdm_lookup ("cefuse_pwrdm" ); +if (cefuse_pwrdm) +pwrdm_set_next_pwrst (cefuse_pwrdm,PWRDM_POWER_OFF ); +else +pr_err ("Failed to get cefuse_pwrdm\n" ); + + gfx_pwrdm =pwrdm_lookup ("gfx_pwrdm" ); + per_pwrdm =pwrdm_lookup ("per_pwrdm" ); + + gfx_l3_clkdm =clkdm_lookup ("gfx_l3_clkdm" ); + gfx_l4ls_clkdm =clkdm_lookup ("gfx_l4ls_gfx_clkdm" ); + + wkup_m3->dev =omap_device_get_by_hwmod_name ("wkup_m3" ); + + ret =wkup_m3_init (); +if (ret) +pr_err ("Could not initialise firmware loading\n" ); + +return ret; +}arch/arm/mach-omap2/pm33xx.h new file +89
-0@@ -0,0 +1,100 @@ +/* +* AM33XX Power Management Routines +* +* Copyright (C) 2012 Texas Instruments Inc. +* Vaibhav Bedia <[email protected]> +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation version 2. +* +* This program is distributed "as is" WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __ARCH_ARM_MACH_OMAP2_PM33XX_H +#define __ARCH_ARM_MACH_OMAP2_PM33XX_H + +#ifndef __ASSEMBLER__ +struct wkup_m3_context { +struct device *dev; +struct firmware *firmware; +struct omap_mbox *mbox; +void __iomem *code; +int resume_addr; +int ipc_data1; +int ipc_data2; +int sleep_mode; +u8 state; +}; + +#ifdef CONFIG_SUSPEND +static void am33xx_ipc_cmd (void ); +static void am33xx_m3_state_machine_reset (void ); +#else +static inline void am33xx_ipc_cmd (void ) {} +static inline void am33xx_m3_state_machine_reset (void ) {} +#endif /* CONFIG_SUSPEND */ + +extern void __iomem *am33xx_get_emif_base (void ); +int wkup_mbox_msg (struct notifier_block *self,unsigned long len,void *msg); +#endif + +#define IPC_CMD_DS0 0x3 +#define IPC_CMD_RESET 0xe +#define DS_IPC_DEFAULT 0xffffffff + +#define IPC_RESP_SHIFT 16 +#define IPC_RESP_MASK (0xffff << 16) + +#define M3_STATE_UNKNOWN 0 +#define M3_STATE_RESET 1 +#define M3_STATE_INITED 2 +#define M3_STATE_MSG_FOR_LP 3 +#define M3_STATE_MSG_FOR_RESET 4 + +#define AM33XX_OCMC_END 0x40310000 +#define AM33XX_EMIF_BASE 0x4C000000 + +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/ +#define EMIF_MODULE_ID_AND_REVISION 0x0000 +#define EMIF_STATUS 0x0004 +#define EMIF_SDRAM_CONFIG 0x0008 +#define EMIF_SDRAM_CONFIG_2 0x000c +#define EMIF_SDRAM_REFRESH_CONTROL 0x0010 +#define EMIF_SDRAM_REFRESH_CTRL_SHDW 0x0014 +#define EMIF_SDRAM_TIMING_1 0x0018 +#define EMIF_SDRAM_TIMING_1_SHDW 0x001c +#define EMIF_SDRAM_TIMING_2 0x0020 +#define EMIF_SDRAM_TIMING_2_SHDW 0x0024 +#define EMIF_SDRAM_TIMING_3 0x0028 +#define EMIF_SDRAM_TIMING_3_SHDW 0x002c +#define EMIF_LPDDR2_NVM_TIMING 0x0030 +#define EMIF_LPDDR2_NVM_TIMING_SHDW 0x0034 +#define EMIF_POWER_MANAGEMENT_CONTROL 0x0038 +#define EMIF_POWER_MANAGEMENT_CTRL_SHDW 0x003c +#define EMIF_PERFORMANCE_COUNTER_1 0x0080 +#define EMIF_PERFORMANCE_COUNTER_2 0x0084 +#define EMIF_PERFORMANCE_COUNTER_CONFIG 0x0088 +#define EMIF_PERFORMANCE_COUNTER_MASTER_REGION_SELECT 0x008c +#define EMIF_PERFORMANCE_COUNTER_TIME 0x0090 +#define EMIF_MISC_REG 0x0094 +#define EMIF_DLL_CALIB_CTRL 0x0098 +#define EMIF_DLL_CALIB_CTRL_SHDW 0x009c +#define EMIF_SYSTEM_OCP_INTERRUPT_RAW_STATUS 0x00a4
arch/arm/mach-omap2/pm33xx.h line 99
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.h new file +10
-0+#define EMIF_SYSTEM_OCP_INTERRUPT_STATUS 0x00ac +#define EMIF_SYSTEM_OCP_INTERRUPT_ENABLE_SET 0x00b4 +#define EMIF_SYSTEM_OCP_INTERRUPT_ENABLE_CLEAR 0x00bc +#define EMIF_SDRAM_OUTPUT_IMPEDANCE_CALIBRATION_CONFIG 0x00c8 +#define EMIF_READ_WRITE_LEVELING_RAMP_WINDOW 0x00d4 +#define EMIF_READ_WRITE_LEVELING_RAMP_CONTROL 0x00d8 +#define EMIF_READ_WRITE_LEVELING_CONTROL 0x00dc +#define EMIF_DDR_PHY_CTRL_1 0x00e4 +#define EMIF_DDR_PHY_CTRL_1_SHDW 0x00e8 +
Above should be part of the EMIF driver, no ?
Quoted 28 lines
arch/arm/mach-omap2/pm33xx.h new file +1
-0+#endif arch/arm/mach-omap2/sleep33xx.S new file +21
-0@@ -0,0 +1,571 @@ +/* + * Low level suspend code for AM33XX SoCs + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * Vaibhav Bedia <[email protected]> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/linkage.h> +#include <asm/memory.h> +#include <asm/assembler.h> + +#include "cm33xx.h"
arch/arm/mach-omap2/sleep33xx.S line 31
Quoted 10 lines
arch/arm/mach-omap2/sleep33xx.S new file +10
-0+#include "pm33xx.h" +#include "prm33xx.h" +#include "control.h" + +/* replicated define because linux/bitops.h cannot be included in assembly */ +#define BIT(nr) (1 << (nr)) + + .text + .align 3 +
Sleep code looks pretty big so I will have a closer look at it bit later. At least from the code it seems, the EMIF registers and hence memory controller needs to be maneged by SW which is really bad.
Regards
Santosh
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
On Sat, Nov 03, 2012 at 22:27:19, Shilimkar, Santosh wrote:
[...]
Quoted 2 lines
Quoted 1 line
Nice descriptive change log Vaibhav.
Thanks :)
Quoted 1 line
[...]
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#include "soc.h" +In case not checked yet, see if you need
all above headers.
Will double-check, I know it's a long list of includes.
Quoted 28 lines
Quoted 28 lines
arch/arm/mach-omap2/pm33xx.c new file +28
-0+void (*am33xx_do_wfi_sram)(void ); + +static void __iomem *am33xx_emif_base; +static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm; +static struct clockdomain *gfx_l3_clkdm, *gfx_l4ls_clkdm; +static struct wkup_m3_context *wkup_m3; + +static DECLARE_COMPLETION (wkup_m3_sync); + +#ifdef CONFIG_SUSPEND +static int am33xx_do_sram_idle (long unsigned int unused) +{ +am33xx_do_wfi_sram (); +return 0 ; +} + +static int am33xx_pm_suspend (void ) +{ +int status, ret =0 ; + +struct omap_hwmod *gpmc_oh, *usb_oh; +struct omap_hwmod *tptc0_oh, *tptc1_oh, *tptc2_oh; + +/* +* By default the following IPs do not have MSTANDBY asserted +* which is necessary for PER domain transition. If the drivers +* are not compiled into the kernel HWMOD code will not change the +* state of the IPs if the IP was not never enabled
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.c line 81
Quoted 7 lines
arch/arm/mach-omap2/pm33xx.c new file +7
-0+*/ + usb_oh =omap_hwmod_lookup ("usb_otg_hs" ); + gpmc_oh =omap_hwmod_lookup ("gpmc" ); + tptc0_oh =omap_hwmod_lookup ("tptc0" ); + tptc1_oh =omap_hwmod_lookup ("tptc1" ); + tptc2_oh =omap_hwmod_lookup ("tptc2" ); +This look you don't need every suspend.
Sorry I don't follow you here.
Quoted 7 lines
Quoted 7 lines
arch/arm/mach-omap2/pm33xx.c new file +7
-0+omap_hwmod_enable (usb_oh); +omap_hwmod_enable (gpmc_oh); +omap_hwmod_enable (tptc0_oh); +omap_hwmod_enable (tptc1_oh); +omap_hwmod_enable (tptc2_oh); + +omap_hwmod_idle (usb_oh);
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.c line 93
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.c new file +5
-0+omap_hwmod_idle (gpmc_oh); +omap_hwmod_idle (tptc0_oh); +omap_hwmod_idle (tptc1_oh); +omap_hwmod_idle (tptc2_oh); +Calling
omap_hwmod_idle()directly tells me something is not right. Can these module not idle themself with respective device drivers ?
With device drivers, yes. The problem comes if the drivers are not compiled in. MSTANDBY needs to be forced for each suspend cycle. During resume, these IPs come out of standby and sysconfig changes.
If it makes sense I could add a new HWMOD flag and some sort of suspend-resume routine, perhaps syscore_ops, in there to do this?
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c line 96
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+/* Put the GFX clockdomains to sleep */ +clkdm_sleep (gfx_l3_clkdm); +clkdm_sleep (gfx_l4ls_clkdm);Can GFX driver suspend code not take care of above ?
Will check if the GFX driver does this. I needed this to ensure that even without the GFX driver the PER domain transition doesn't get blocked.
Quoted 2 lines
Also are these clock domains are not supporting HW supervised mode ?
All clock domains in AM33xx are SW-supervised.
Quoted 7 lines
arch/arm/mach-omap2/pm33xx.c line 100
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+/* Try to put GFX to sleep */ +pwrdm_set_next_pwrst (gfx_pwrdm,PWRDM_POWER_OFF ); +Above as well can be taken care by constraint QOS API by GFX driver.
Will check if I can get rid of this.
Quoted 26 lines
Quoted 26 lines
arch/arm/mach-omap2/pm33xx.c new file +26
-0+ ret =cpu_suspend (0 , am33xx_do_sram_idle); + + status =pwrdm_read_pwrst (gfx_pwrdm); +if (status !=PWRDM_POWER_OFF ) +pr_err ("GFX domain did not transition\n" ); +else +pr_info ("GFX domain entered low power state\n" ); + +/* Needed to ensure L4LS clockdomain transitions properly */ +clkdm_wakeup (gfx_l3_clkdm); +clkdm_wakeup (gfx_l4ls_clkdm); + +if (ret) { +pr_err ("Kernel suspend failure\n" ); + }else { + status =omap_ctrl_readl (AM33XX_CONTROL_IPC_MSG_REG1 ); + status &=IPC_RESP_MASK ; + status >>=__ffs (IPC_RESP_MASK ); + +switch (status) { +case 0 : +pr_info ("Successfully transitioned to low power state\n" ); +if (wkup_m3->sleep_mode ==IPC_CMD_DS0 ) +/* XXX: Use SOC specific ops for this? */ + per_pwrdm->ret_logic_off_counter++; +break ;
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.c line 133
Quoted 7 lines
arch/arm/mach-omap2/pm33xx.c new file +7
-0+case 1 : +pr_err ("Could not enter low power state\n" ); + ret =-1 ; +break ; +default : +pr_err ("Something is terribly wrong :(\nStatus = %d\n" , + status);Sounds terrible :-)
Well this is not the expected state. But I guess better to leave in a message instead of ignoring the unexpected :)
[...]
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.c line 187
Quoted 2 lines
arch/arm/mach-omap2/pm33xx.c new file +2
-0+if (!wait_for_completion_timeout (&wkup_m3_sync, +msecs_to_jiffies (500 ))) {500 is from spec or arbitrary timeout ?
We just need enough delay to let the M3 respond. I didn't have the hw delays so put in a timeout which is not too big.
[...]
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.c line 406
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+ + ret =am33xx_map_emif (); +No EMIF driver to handle EMIF MAP, registers etc ?
We just need to ioremap it here. EMIF registers are updated from assembly only.
[...]
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.h line 99
Quoted 2 lines
arch/arm/mach-omap2/pm33xx.h new file +2
-0+#define EMIF_DDR_PHY_CTRL_1_SHDW 0x00e8 +Above should be part of the EMIF driver, no ?
The driver would end up being a dummy driver which just does an ioremap so I added the register offsets here.
[...]
Quoted 5 lines
Quoted 1 line
+
Sleep code looks pretty big so I will have a closer look at it bit later. At least from the code it seems, the EMIF registers and hence memory controller needs to be maneged by SW which is really bad.
It will get a slightly bigger once DDR3 specific handling gets added ;) I agree the s/w managed memory controller is not good. It even places limitations on Core DVFS.
Regards,
Vaibhav
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
In reply to Vaibhav Bedia+Santosh (to help with EMIF questions/comments)
Quoted 49 lines · Vaibhav Bedia
AM335x supports various low power modes as documented in section 8.1.4.3 of the AM335x TRM which is available @ http://www.ti.com/litv/pdf/spruh73f
DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a system reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate resume process. As part of the boot process, the assembly code is copied over to OCMCRAM using the OMAP SRAM code.
AM335x has a Cortex-M3 (
WKUP_M3) which assists the MPU in DeepSleep0 entry and exit.WKUP_M3takes care of the clockdomain and powerdomain transitions based on the intended low power state. MPU needs to load the appropriateWKUP_M3binary onto theWKUP_M3memory space before it can leverage any of the PM features like DeepSleep.The IPC mechanism between MPU and
WKUP_M3uses a mailbox sub-module and 8 IPC registers in the Control module. MPU uses the assigned Mailbox for issuing an interrupt toWKUP_M3which then goes and checks the IPC registers for the payload.WKUP_M3has the ability to trigger on interrupt to MPU by executing the "sev" instruction.In the current implementation when the suspend process is initiated MPU interrupts the
WKUP_M3to let about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received, MPU continues with its suspend process to suspend all the drivers and then jumps to assembly in OCMC RAM to put the PLLs in bypass, put the external RAM in self-refresh mode and then finally execute the WFI instruction. The WFI instruction triggers another interrupt to theWKUP_M3which then continues wiht the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence,WKUP_M3unmasks the interrupt lines for the wakeup sources. WhenWKUP_M3executes WFI, the hardware disables the main oscillator.When a wakeup event occurs,
WKUP_M3starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence, in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence.
Quoted 5 lines
The low level code in OCMC relocks the PLLs, enables access to external RAM and then jumps to the
cpu_resumecode of the kernel to finish the resume process.Signed-off-by Vaibhav Bedia <[email protected]>
Very well summarized. Thanks for the thorough changelog.
First, some general comments. This is a big patch and probably should be broken up a bit. I suspect it could be broken up a bit, maybe into at least:
- EMIF interface
- SCM interface, new APIs
- assembly/OCM code
- pm33xx.[ch]
- lastly, the late_init stuff that actually initizlizes
I have a handful of comments below. I wrote this up on the plane over the weekend, and I see that Santosh has already made some similar comments, but I'll send mine anyways.
[...]
Quoted 20 lines
arch/arm/mach-omap2/pm33xx.c new file +20
-0+static int am33xx_pm_suspend (void ) +{ +int status, ret =0 ; + +struct omap_hwmod *gpmc_oh, *usb_oh; +struct omap_hwmod *tptc0_oh, *tptc1_oh, *tptc2_oh; + +/* +* By default the following IPs do not have MSTANDBY asserted +* which is necessary for PER domain transition. If the drivers +* are not compiled into the kernel HWMOD code will not change the +* state of the IPs if the IP was not never enabled +*/ + usb_oh =omap_hwmod_lookup ("usb_otg_hs" ); + gpmc_oh =omap_hwmod_lookup ("gpmc" ); + tptc0_oh =omap_hwmod_lookup ("tptc0" ); + tptc1_oh =omap_hwmod_lookup ("tptc1" ); + tptc2_oh =omap_hwmod_lookup ("tptc2" ); + +omap_hwmod_enable (usb_oh);
arch/arm/mach-omap2/pm33xx.c line 92
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+omap_hwmod_enable (gpmc_oh); +omap_hwmod_enable (tptc0_oh); +omap_hwmod_enable (tptc1_oh); +omap_hwmod_enable (tptc2_oh); + +omap_hwmod_idle (usb_oh); +omap_hwmod_idle (gpmc_oh); +omap_hwmod_idle (tptc0_oh); +omap_hwmod_idle (tptc1_oh); +omap_hwmod_idle (tptc2_oh);
Doing this on every suspend looks a bit strange. Why not just have some init function handle these devices once at boot. If this is really needed on every suspend, it needs some more description, and probably some basic stub drivers need to be created.
Also, if there are drivers for these devices, won't this interfere?
arch/arm/mach-omap2/pm33xx.c line 99
Quoted 6 lines
arch/arm/mach-omap2/pm33xx.c new file +6
-0+/* Put the GFX clockdomains to sleep */ +clkdm_sleep (gfx_l3_clkdm); +clkdm_sleep (gfx_l4ls_clkdm); + +/* Try to put GFX to sleep */ +pwrdm_set_next_pwrst (gfx_pwrdm,PWRDM_POWER_OFF );
ditto.
[...]
arch/arm/mach-omap2/pm33xx.c line 167
Quoted 11 lines
arch/arm/mach-omap2/pm33xx.c new file +11
-0+static int am33xx_pm_begin (suspend_state_t state) +{ +int ret =0 ; + +disable_hlt (); + +/* +* Physical resume address to be used by ROM code +*/ + wkup_m3->resume_addr = (AM33XX_OCMC_END - am33xx_do_wfi_sz + + am33xx_resume_offset +0x4 );
Why does this need to be calculated every suspend/resume?
arch/arm/mach-omap2/pm33xx.c line 173
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.c new file +5
-0+ wkup_m3->sleep_mode =IPC_CMD_DS0 ; + wkup_m3->ipc_data1 =DS_IPC_DEFAULT ; + wkup_m3->ipc_data2 =DS_IPC_DEFAULT ; + +am33xx_ipc_cmd ();
This IPC needs a cleaner interface/API. Also, since it involves register writes to the SCM, it should be defined in control.c. (NOTE:
we're in the process of creating a real driver out of the SCM, so all SCM register accesses need to be contained in control.c)
For example, you probably want an am33xx_m3_* API in control.c, with some pre-baked commands for the M3.
arch/arm/mach-omap2/pm33xx.c line 187
Quoted 13 lines
arch/arm/mach-omap2/pm33xx.c new file +12
-0+ wkup_m3->state =M3_STATE_MSG_FOR_LP ; +omap_mbox_enable_irq (wkup_m3->mbox,IRQ_RX ); + + ret =omap_mbox_msg_send (wkup_m3->mbox,0xABCDABCD ); +if (ret) { +pr_err ("A8<->CM3 MSG for LP failed\n" ); +am33xx_m3_state_machine_reset (); + ret =-1 ; + } + +if (!wait_for_completion_timeout (&wkup_m3_sync, +msecs_to_jiffies (500 ))) {
hmm, interesting. I know you're not implementing idle here, but I'm rather curious how this sync w/M3 is going to work for idle.
Quoted 11 lines
arch/arm/mach-omap2/pm33xx.c new file +11
-0+pr_err ("A8<->CM3 sync failure\n" ); +am33xx_m3_state_machine_reset (); + ret =-1 ; + }else { +pr_debug ("Message sent for entering DeepSleep mode\n" ); +omap_mbox_disable_irq (wkup_m3->mbox,IRQ_RX ); + } + +return ret; +} +
[...]
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c new file +4
-0+static void am33xx_m3_state_machine_reset (void ) +{ +int ret =0 ; +
arch/arm/mach-omap2/pm33xx.c line 238
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+ wkup_m3->resume_addr =0x0 ; + wkup_m3->sleep_mode =IPC_CMD_RESET ; + wkup_m3->ipc_data1 =DS_IPC_DEFAULT ; + wkup_m3->ipc_data2 =DS_IPC_DEFAULT ; + +am33xx_ipc_cmd (); + + wkup_m3->state =M3_STATE_MSG_FOR_RESET ; + + ret =omap_mbox_msg_send (wkup_m3->mbox,0xABCDABCD );
magic constant needs a #define
Quoted 13 lines
arch/arm/mach-omap2/pm33xx.c new file +13
-0+if (!ret) { +pr_debug ("Message sent for resetting M3 state machine\n" ); +if (!wait_for_completion_timeout (&wkup_m3_sync, +msecs_to_jiffies (500 ))) +pr_err ("A8<->CM3 sync failure\n" ); + }else { +pr_err ("Could not reset M3 state machine!!!\n" ); + wkup_m3->state =M3_STATE_UNKNOWN ; + } +} +#endif /* CONFIG_SUSPEND */ + +/*
arch/arm/mach-omap2/pm33xx.c line 261
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+* Dummy notifier for the mailbox +*/ +int wkup_mbox_msg (struct notifier_block *self,unsigned long len,void *msg) +{ +return 0 ; +} + +static struct notifier_block wkup_mbox_notifier = { + .notifier_call = wkup_mbox_msg, +};
Why do you need a dummy notifier? Looks like maybe plat-omap/mailbox.c needs a few minor fixes to support not being passed a notifier instead?
arch/arm/mach-omap2/pm33xx.c line 265
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+static irqreturn_t wkup_m3_txev_handler (int irq,void *unused) +{ +omap_ctrl_writel (0x1 ,AM33XX_CONTROL_M3_TXEV_EOI );
undocumented magic write (but presumably an IRQ ack?)
Note this also needs to be moved to control.c and given a useful API.
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+switch (wkup_m3->state) { +case M3_STATE_RESET : + wkup_m3->state =M3_STATE_INITED ; +break ; +case M3_STATE_MSG_FOR_RESET : + wkup_m3->state =M3_STATE_INITED ; +omap_mbox_msg_rx_flush (wkup_m3->mbox); +complete (&wkup_m3_sync); +break ; +case M3_STATE_MSG_FOR_LP :
arch/arm/mach-omap2/pm33xx.c line 286
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+omap_mbox_msg_rx_flush (wkup_m3->mbox); +complete (&wkup_m3_sync); +break ; +case M3_STATE_UNKNOWN : +pr_err ("IRQ %d with WKUP_M3 in unknown state\n" , irq); +omap_mbox_msg_rx_flush (wkup_m3->mbox); +return IRQ_NONE ; + } + +omap_ctrl_writel (0x0 ,AM33XX_CONTROL_M3_TXEV_EOI );
undoumented magic write?
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+return IRQ_HANDLED ; +} +
[...]
Quoted 11 lines
arch/arm/mach-omap2/pm33xx.c new file +11
-0+static int wkup_m3_init (void ) +{ +int irq, ret =0 ; +struct resource *mem; +struct platform_device *pdev =to_platform_device (wkup_m3->dev); + +omap_device_enable_hwmods (to_omap_device (pdev)); + +/* Reserve the MBOX for sending messages to M3 */ + wkup_m3->mbox =omap_mbox_get ("wkup_m3" , &wkup_mbox_notifier); +if (IS_ERR (wkup_m3->mbox)) {
arch/arm/mach-omap2/pm33xx.c line 342
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+pr_err ("Could not reserve mailbox for A8->M3 IPC\n" ); + ret = -ENODEV ; + goto exit; + } + + irq =platform_get_irq (pdev,0 ); + + mem =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!mem) +dev_err (wkup_m3->dev,"no memory resource\n" );
if !mem, this continues and still tries to ioremap NULL.
Quoted 22 lines
arch/arm/mach-omap2/pm33xx.c new file +22
-0+ wkup_m3->code =devm_request_and_ioremap (wkup_m3->dev, mem); + ret =devm_request_irq (wkup_m3->dev, irq, wkup_m3_txev_handler, +IRQF_DISABLED ,"wkup_m3_txev" ,NULL ); +if (ret) { +dev_err (wkup_m3->dev,"request_irq failed\n" ); + goto err; + } + +pr_info ("Trying to load am335x-pm-firmware.bin" ); + +/* We don't want to delay boot */ +request_firmware_nowait (THIS_MODULE ,0 ,"am335x-pm-firmware.bin" , + wkup_m3->dev,GFP_KERNEL , wkup_m3, + am33xx_pm_firmware_cb); +return 0 ; + +err: +omap_mbox_put (wkup_m3->mbox, &wkup_mbox_notifier); +exit: +return ret; +} +
[...]
Quoted 14 lines
arch/arm/mach-omap2/pm33xx.h new file +14
-0+extern void __iomem *am33xx_get_emif_base (void ); +int wkup_mbox_msg (struct notifier_block *self,unsigned long len,void *msg); +#endif + +#define IPC_CMD_DS0 0x3 +#define IPC_CMD_RESET 0xe +#define DS_IPC_DEFAULT 0xffffffff + +#define IPC_RESP_SHIFT 16 +#define IPC_RESP_MASK (0xffff << 16) + +#define M3_STATE_UNKNOWN 0 +#define M3_STATE_RESET 1 +#define M3_STATE_INITED 2
arch/arm/mach-omap2/pm33xx.h line 64
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.h new file +10
-0+#define M3_STATE_MSG_FOR_LP 3 +#define M3_STATE_MSG_FOR_RESET 4 + +#define AM33XX_OCMC_END 0x40310000 +#define AM33XX_EMIF_BASE 0x4C000000 + +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/
I'd probably suggest just moving the register definitions you need into <plat/emif_plat.h> so they can be shared with the driver.
Also, the EMIF stuff would benefit greatly from using symbolic defines for the values being written. Probably having those in <plat/emif_plat.h> would also help out here.
Or, maybe the EMIF driver can provide some self-contained stubs that can be copied to OCP RAM for the functionality needed here?
Santosh, what do you think of that?
Another user of the EMIF virtual addr is emif_self_refresh_dis, but I don't see that code actually used anywhere.
Looking closer, now I see the ddr_self_refresh macro is using emif_virt_addr (that macro should be fixed to take the base address, for readability.)
On a related note, just a quick glance at all the code running out of OCM RAM makes me wonder if any that could be done before jumping to OCM RAM. Ideally, we only want the absolute minimum running out of OCM RAM.
[...]
Quoted 23 lines
arch/arm/mach-omap2/sleep33xx.S new file +17
-0@@ -0,0 +1,571 @@ +/* + * Low level suspend code for AM33XX SoCs + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * Vaibhav Bedia <[email protected]> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/linkage.h>
arch/arm/mach-omap2/sleep33xx.S line 27
Quoted 10 lines
arch/arm/mach-omap2/sleep33xx.S new file +10
-0+#include <asm/memory.h> +#include <asm/assembler.h> + +#include "cm33xx.h" +#include "pm33xx.h" +#include "prm33xx.h" +#include "control.h" + +/* replicated define because linux/bitops.h cannot be included in assembly */ +#define BIT(nr) (1 << (nr))
never used, just remove it. Using shifts as below is fine (but using symbolic constants would be even better.)
In fact, there are lots of magic constants in this code that I'd like to see #defined.
[...]
Quoted 4 lines
arch/arm/mach-omap2/sleep33xx.S new file +4
-0+ wfi + nop + nop + nop
arch/arm/mach-omap2/sleep33xx.S line 209
Quoted 10 lines
arch/arm/mach-omap2/sleep33xx.S new file +10
-0+ nop + nop + nop + nop + nop + nop + nop + nop + nop + nop
Why all the nops? Some comments would be helpful there.
[...]
arch/arm/mach-omap2/sleep33xx.S line 521
Quoted 1 line
arch/arm/mach-omap2/sleep33xx.S new file +1
-0+/* Values recommended by the HW team */
A little documentation of these values would be helpful here.
arch/arm/mach-omap2/sleep33xx.S line 533
Quoted 12 lines
arch/arm/mach-omap2/sleep33xx.S new file +12
-0+susp_io_pull_data:. + .word 0x3FF00003 +susp_io_pull_cmd1: + .word 0xFFE0018B +susp_io_pull_cmd2: + .word 0xFFA0098B +resume_io_pull_data: + .word 0x18B +resume_io_pull_cmd: + .word 0x18B +susp_vtp_ctrl_val: + .word 0x10117
Kevin
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Quoted 74 lines · Kevin Hilman
+Santosh (to help with EMIF questions/comments)
Quoted 49 lines · Vaibhav Bedia
AM335x supports various low power modes as documented in section 8.1.4.3 of the AM335x TRM which is available @ http://www.ti.com/litv/pdf/spruh73f
DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a system reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate resume process. As part of the boot process, the assembly code is copied over to OCMCRAM using the OMAP SRAM code.
AM335x has a Cortex-M3 (
WKUP_M3) which assists the MPU in DeepSleep0 entry and exit.WKUP_M3takes care of the clockdomain and powerdomain transitions based on the intended low power state. MPU needs to load the appropriateWKUP_M3binary onto theWKUP_M3memory space before it can leverage any of the PM features like DeepSleep.The IPC mechanism between MPU and
WKUP_M3uses a mailbox sub-module and 8 IPC registers in the Control module. MPU uses the assigned Mailbox for issuing an interrupt toWKUP_M3which then goes and checks the IPC registers for the payload.WKUP_M3has the ability to trigger on interrupt to MPU by executing the "sev" instruction.In the current implementation when the suspend process is initiated MPU interrupts the
WKUP_M3to let about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received, MPU continues with its suspend process to suspend all the drivers and then jumps to assembly in OCMC RAM to put the PLLs in bypass, put the external RAM in self-refresh mode and then finally execute the WFI instruction. The WFI instruction triggers another interrupt to theWKUP_M3which then continues wiht the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence,WKUP_M3unmasks the interrupt lines for the wakeup sources. WhenWKUP_M3executes WFI, the hardware disables the main oscillator.When a wakeup event occurs,
WKUP_M3starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence, in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence.Quoted 5 lines
The low level code in OCMC relocks the PLLs, enables access to external RAM and then jumps to the
cpu_resumecode of the kernel to finish the resume process.Signed-off-by Vaibhav Bedia <[email protected]>Very well summarized. Thanks for the thorough changelog.
First, some general comments. This is a big patch and probably should be broken up a bit. I suspect it could be broken up a bit, maybe into at least:
- EMIF interface
- SCM interface, new APIs
- assembly/OCM code
- pm33xx.[ch]
- lastly, thelate_initstuff that actually initizlizesI have a handful of comments below. I wrote this up on the plane over the weekend, and I see that Santosh has already made some similar comments, but I'll send mine anyways.
[...]
Quoted 29 lines
Quoted 15 lines
arch/arm/mach-omap2/pm33xx.h new file +14
-0+extern void __iomem *am33xx_get_emif_base (void ); +int wkup_mbox_msg (struct notifier_block *self,unsigned long len,void *msg); +#endif + +#define IPC_CMD_DS0 0x3 +#define IPC_CMD_RESET 0xe +#define DS_IPC_DEFAULT 0xffffffff + +#define IPC_RESP_SHIFT 16 +#define IPC_RESP_MASK (0xffff << 16) + +#define M3_STATE_UNKNOWN 0 +#define M3_STATE_RESET 1 +#define M3_STATE_INITED 2arch/arm/mach-omap2/pm33xx.h line 64
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.h new file +10
-0+#define M3_STATE_MSG_FOR_LP 3 +#define M3_STATE_MSG_FOR_RESET 4 + +#define AM33XX_OCMC_END 0x40310000 +#define AM33XX_EMIF_BASE 0x4C000000 + +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/ I'd probably suggest just moving the register definitions you need into <
plat/emif_plat.h> so they can be shared with the driver.
Quoted 9 lines
Also, the EMIF stuff would benefit greatly from using symbolic defines for the values being written. Probably having those in <
plat/emif_plat.h> would also help out here.Or, maybe the EMIF driver can provide some self-contained stubs that can be copied to OCP RAM for the functionality needed here?
Santosh, what do you think of that?
Thats what I mentioned in my comment. I also don't know why such a bad hardware choice was made when we have perfectly working EMIF IP across low power states. Even the self refresh control is managed inside hardware upon idle. My guess is DDR self-refresh might be the reason to use OCMC RAM.
In either case, Keeping EMIF changes separate as part of EMIF driver/platform code is right way to go about it. May be the disable_selfrefresh() might need to kept in assembly files since it needs to be running from SRAM but rest need not be part of PM code.
Regards
Santosh
For
Regards
Santosh
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Santosh, Kevin
On Tue, Nov 06, 2012 at 03:22:16, Shilimkar, Santosh wrote:
[...]
Quoted 24 lines
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.h line 64
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.h new file +5
-0+ +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/ I'd probably suggest just moving the register definitions you need into <
plat/emif_plat.h> so they can be shared with the driver.Quoted 9 lines
Also, the EMIF stuff would benefit greatly from using symbolic defines for the values being written. Probably having those in <
plat/emif_plat.h> would also help out here.Or, maybe the EMIF driver can provide some self-contained stubs that can be copied to OCP RAM for the functionality needed here?
Santosh, what do you think of that?
Thats what I mentioned in my comment. I also don't know why such a bad hardware choice was made when we have perfectly working EMIF IP across low power states. Even the self refresh control is managed inside hardware upon idle. My guess is DDR self-refresh might be the reason to use OCMC RAM.
Quoted 6 lines
In either case, Keeping EMIF changes separate as part of EMIF driver/platform code is right way to go about it. May be the
disable_selfrefresh()might need to kept in assembly files since it needs to be running from SRAM but rest need not be part of PM code.
In the suspend path we do lot of I/O manipulations to lower final power number which must be done after the external memory has gone into self-refresh. So, these steps will have to be done from code in OCMC RAM.
Other than saving the EMIF configuration the other thing that we do during suspend from assembly is to put the PLLs in bypass. We could possibly move the DISP PLL bypass to C code. MPU PLL is another candidate but this might add in more delays in the suspend and abort sequence (I don't have any delay numbers to justify this right now)
The resume path undoes the I/O manipulations and then restores the EMIF configuration all of which I think is necessary before we can jump back to external memory.
So, I think we'll have just the EMIF context save and possibly the PLL bypass for DISP PLL during suspend that we can move out of assembly.
Coming to point of sharing the EMIF register definitions, with the recent changes to move around the header files out of plat-omap, is it ok to add in the required offsets and related bit-field definitions from the EMIF driver to plat-omap?
Regards,
Vaibhav
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Quoted 37 lines · Vaibhav
Hi Santosh, Kevin
On Tue, Nov 06, 2012 at 03:22:16, Shilimkar, Santosh wrote:
[...]Quoted 24 lines
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.h line 64
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.h new file +5
-0+ +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/ I'd probably suggest just moving the register definitions you need into <
plat/emif_plat.h> so they can be shared with the driver.Quoted 9 lines
Also, the EMIF stuff would benefit greatly from using symbolic defines for the values being written. Probably having those in <
plat/emif_plat.h> would also help out here.Or, maybe the EMIF driver can provide some self-contained stubs that can be copied to OCP RAM for the functionality needed here?
Santosh, what do you think of that?
Thats what I mentioned in my comment. I also don't know why such a bad hardware choice was made when we have perfectly working EMIF IP across low power states. Even the self refresh control is managed inside hardware upon idle. My guess is DDR self-refresh might be the reason to use OCMC RAM.
Quoted 6 lines
In either case, Keeping EMIF changes separate as part of EMIF driver/platform code is right way to go about it. May be the
disable_selfrefresh()might need to kept in assembly files since it needs to be running from SRAM but rest need not be part of PM code.
Quoted 4 lines
In the suspend path we do lot of I/O manipulations to lower final power number which must be done after the external memory has gone into self-refresh. So, these steps will have to be done from code in OCMC RAM.
Only the DDR IO needs to be done after memory enters into self refresh. Rest of the IOs can be handled and moved to low power modes before DDR being in self refresh, No ?
Quoted 6 lines
Other than saving the EMIF configuration the other thing that we do during suspend from assembly is to put the PLLs in bypass. We could possibly move the DISP PLL bypass to C code. MPU PLL is another candidate but this might add in more delays in the suspend and abort sequence (I don't have any delay numbers to justify this right now)
So DPLLS doesn't support low power bypass mode which should take care of itself on clock gating ? Are the DPLL IPs different than what they are on OMAP ?
Quoted 4 lines
The resume path undoes the I/O manipulations and then restores the EMIF configuration all of which I think is necessary before we can jump back to external memory.
As I memtioned above, you should limit these IOs to only DDR IOs.
Quoted 3 lines
So, I think we'll have just the EMIF context save and possibly the PLL bypass for DISP PLL during suspend that we can move out of assembly.
EMIF context save also can be done in advance. Restore is what you need to take care in early resume before bringing out DDR out of self refresh.
Quoted 5 lines
Coming to point of sharing the EMIF register definitions, with the recent changes to move around the header files out of plat-omap, is it ok to add in the required offsets and related bit-field definitions from the EMIF driver to plat-omap?
We can have that in linux/include/* as well if the register defines can be shared.
Regards
Santosh
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Quoted 38 lines · Santosh
Quoted 37 lines · Vaibhav
Hi Santosh, Kevin
On Tue, Nov 06, 2012 at 03:22:16, Shilimkar, Santosh wrote:
[...]Quoted 30 lines
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.h line 64
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.h new file +5
-0+ +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/ I'd probably suggest just moving the register definitions you need into <
plat/emif_plat.h> so they can be shared with the driver.Quoted 9 lines
Also, the EMIF stuff would benefit greatly from using symbolic defines for the values being written. Probably having those in <
plat/emif_plat.h> would also help out here.Or, maybe the EMIF driver can provide some self-contained stubs that can be copied to OCP RAM for the functionality needed here?
Santosh, what do you think of that?
Thats what I mentioned in my comment. I also don't know why such a bad hardware choice was made when we have perfectly working EMIF IP across low power states. Even the self refresh control is managed inside hardware upon idle. My guess is DDR self-refresh might be the reason to use OCMC RAM.
In either case, Keeping EMIF changes separate as part of EMIF driver/platform code is right way to go about it. May be the
disable_selfrefresh()might need to kept in assembly files since it needs to be running from SRAM but rest need not be part of PM code.
Quoted 8 lines
Quoted 4 lines
In the suspend path we do lot of I/O manipulations to lower final power number which must be done after the external memory has gone into self-refresh. So, these steps will have to be done from code in OCMC RAM.
Only the DDR IO needs to be done after memory enters into self refresh. Rest of the IOs can be handled and moved to low power modes before DDR being in self refresh, No ?
All the code is related to DDR IOs only. We have some code for changing the pull configuration of the DATA and CMD macros of the PHY and then some code for DDR VTP control. We also switch over the DDR IOs to mDDR mode to lower the leakage.
Quoted 9 lines
Quoted 6 lines
Other than saving the EMIF configuration the other thing that we do during suspend from assembly is to put the PLLs in bypass. We could possibly move the DISP PLL bypass to C code. MPU PLL is another candidate but this might add in more delays in the suspend and abort sequence (I don't have any delay numbers to justify this right now)
So DPLLS doesn't support low power bypass mode which should take care of itself on clock gating ? Are the DPLL IPs different than what they are on OMAP ?
Same IP but no auto-mode :(
Quoted 5 lines
Quoted 5 lines
The resume path undoes the I/O manipulations and then restores the EMIF configuration all of which I think is necessary before we can jump back to external memory.
Quoted 9 lines
As I memtioned above, you should limit these IOs to only DDR IOs.
Quoted 3 lines
So, I think we'll have just the EMIF context save and possibly the PLL bypass for DISP PLL during suspend that we can move out of assembly.
EMIF context save also can be done in advance. Restore is what you need to take care in early resume before bringing out DDR out of self refresh.
Yes I can move the context save earlier. Will try that out and put in the next version.
Quoted 8 lines
Quoted 5 lines
Coming to point of sharing the EMIF register definitions, with the recent changes to move around the header files out of plat-omap, is it ok to add in the required offsets and related bit-field definitions from the EMIF driver to plat-omap?
We can have that in linux/include/* as well if the register defines can be shared.
Ok.
Regards,
Vaibhav
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
In reply to Kevin HilmanHi Kevin,
On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote:
[...]
Quoted 10 lines
First, some general comments. This is a big patch and probably should be broken up a bit. I suspect it could be broken up a bit, maybe into at least:
- EMIF interface
- SCM interface, new APIs
- assembly/OCM code
- pm33xx.[ch]
- lastly, thelate_initstuff that actually initizlizes
Ok, I'll try splitting the patches in the next version.
Quoted 4 lines
I have a handful of comments below. I wrote this up on the plane over the weekend, and I see that Santosh has already made some similar comments, but I'll send mine anyways.
[...]
Quoted 5 lines
Doing this on every suspend looks a bit strange. Why not just have some init function handle these devices once at boot. If this is really needed on every suspend, it needs some more description, and probably some basic stub drivers need to be created.
We do require it for every suspend (but more below). I'll add in more description in the comment that's already there.
Quoted 3 lines
Also, if there are drivers for these devices, won't this interfere?
Hmm, I can think of the following scenarios
1. Runtime PM adapted drivers are compiled in - We'll have to ensure that in their suspend callbacks they end up doing omap_hwmod_idle() via the runtime PM APIs. In this case the omap_hwmod_enable() followed by omap_hwmod_idle() is not necessary in the PM code.
2. The drivers are not compiled in - In this case, the hwmod code puts the IPs in standby during bootup so the first suspend-resume iteration will pass. During resuming, the SYSC configuration for forced standby will be lost, so in the subsequent iterations these IPs won't go standby and the hwmod code does not touch them since they never got enabled. In this case we do need the sequence that is there currently.
3. For some reason the respective drivers don't idle the IPs during suspend - (no_idle_on_suspend flag for the hwmod in DT?). In this scenario I think we should abort the suspend process since we know that the suspend is not going to work.
Is there some other scenario that needs to be covered?
How about adding some flag in hwmod code for handling this? Something similar to what was done for MMC [1]. I think the problem that we have is in some ways similar to the one addressed in [1].
Quoted 8 lines
arch/arm/mach-omap2/pm33xx.c line 99
Quoted 6 lines
arch/arm/mach-omap2/pm33xx.c new file +6
-0+/* Put the GFX clockdomains to sleep */ +clkdm_sleep (gfx_l3_clkdm); +clkdm_sleep (gfx_l4ls_clkdm); + +/* Try to put GFX to sleep */ +pwrdm_set_next_pwrst (gfx_pwrdm,PWRDM_POWER_OFF );ditto.
I'll check if this can be removed.
Quoted 17 lines
[...]
arch/arm/mach-omap2/pm33xx.c line 167
Quoted 11 lines
arch/arm/mach-omap2/pm33xx.c new file +11
-0+static int am33xx_pm_begin (suspend_state_t state) +{ +int ret =0 ; + +disable_hlt (); + +/* +* Physical resume address to be used by ROM code +*/ + wkup_m3->resume_addr = (AM33XX_OCMC_END - am33xx_do_wfi_sz + + am33xx_resume_offset +0x4 );Why does this need to be calculated every suspend/resume?
Will move this to init phase.
Quoted 11 lines
arch/arm/mach-omap2/pm33xx.c line 173
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.c new file +5
-0+ wkup_m3->sleep_mode =IPC_CMD_DS0 ; + wkup_m3->ipc_data1 =DS_IPC_DEFAULT ; + wkup_m3->ipc_data2 =DS_IPC_DEFAULT ; + +am33xx_ipc_cmd ();This IPC needs a cleaner interface/API. Also, since it involves register writes to the SCM, it should be defined in
control.c. (NOTE:
we're in the process of creating a real driver out of the SCM, so all SCM register accesses need to be contained incontrol.c)
Quoted 2 lines
For example, you probably want an am33xx_m3_* API in
control.c, with some pre-baked commands for the M3.
Ok. I'll work on this for the next version.
Quoted 18 lines
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c new file +2
-0+ wkup_m3->state =M3_STATE_MSG_FOR_LP ; +omap_mbox_enable_irq (wkup_m3->mbox,IRQ_RX );arch/arm/mach-omap2/pm33xx.c line 187
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+ + ret =omap_mbox_msg_send (wkup_m3->mbox,0xABCDABCD ); +if (ret) { +pr_err ("A8<->CM3 MSG for LP failed\n" ); +am33xx_m3_state_machine_reset (); + ret =-1 ; + } + +if (!wait_for_completion_timeout (&wkup_m3_sync, +msecs_to_jiffies (500 ))) {hmm, interesting. I know you're not implementing idle here, but I'm rather curious how this sync w/M3 is going to work for idle.
Like you mentioned in another thread we could probably not have a sync in the idle path. (More in the other thread).
Quoted 14 lines
Quoted 11 lines
arch/arm/mach-omap2/pm33xx.c new file +11
-0+pr_err ("A8<->CM3 sync failure\n" ); +am33xx_m3_state_machine_reset (); + ret =-1 ; + }else { +pr_debug ("Message sent for entering DeepSleep mode\n" ); +omap_mbox_disable_irq (wkup_m3->mbox,IRQ_RX ); + } + +return ret; +} +[...]
Quoted 16 lines
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c new file +4
-0+static void am33xx_m3_state_machine_reset (void ) +{ +int ret =0 ; +arch/arm/mach-omap2/pm33xx.c line 238
Quoted 10 lines
arch/arm/mach-omap2/pm33xx.c new file +10
-0+ wkup_m3->resume_addr =0x0 ; + wkup_m3->sleep_mode =IPC_CMD_RESET ; + wkup_m3->ipc_data1 =DS_IPC_DEFAULT ; + wkup_m3->ipc_data2 =DS_IPC_DEFAULT ; + +am33xx_ipc_cmd (); + + wkup_m3->state =M3_STATE_MSG_FOR_RESET ; + + ret =omap_mbox_msg_send (wkup_m3->mbox,0xABCDABCD );magic constant needs a #define
Ok.
Quoted 19 lines
Quoted 19 lines
arch/arm/mach-omap2/pm33xx.c new file +18
-0+if (!ret) { +pr_debug ("Message sent for resetting M3 state machine\n" ); +if (!wait_for_completion_timeout (&wkup_m3_sync, +msecs_to_jiffies (500 ))) +pr_err ("A8<->CM3 sync failure\n" ); + }else { +pr_err ("Could not reset M3 state machine!!!\n" ); + wkup_m3->state =M3_STATE_UNKNOWN ; + } +} +#endif /* CONFIG_SUSPEND */ + +/* +* Dummy notifier for the mailbox +*/ +int wkup_mbox_msg (struct notifier_block *self,unsigned long len,void *msg) +{ +return 0 ;
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.c line 261
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.c new file +5
-0+} + +static struct notifier_block wkup_mbox_notifier = { + .notifier_call = wkup_mbox_msg, +};Why do you need a dummy notifier? Looks like maybe
plat-omap/mailbox.cneeds a few minor fixes to support not being passed a notifier instead?
Ok. Will add checks in the mailbox code instead. (I wasn't too sure about the mailbox users so I tried to take the path of least resistance :)
Quoted 6 lines
arch/arm/mach-omap2/pm33xx.c line 265
Quoted 3 lines
arch/arm/mach-omap2/pm33xx.c new file +3
-0+static irqreturn_t wkup_m3_txev_handler (int irq,void *unused) +{ +omap_ctrl_writel (0x1 ,AM33XX_CONTROL_M3_TXEV_EOI );undocumented magic write (but presumably an IRQ ack?)
Yes, it's for clearing the interrupt. Will add a comment put a #def for this and other places that you mentioned.
Quoted 16 lines
Note this also needs to be moved to
control.cand given a useful API.Quoted 14 lines
arch/arm/mach-omap2/pm33xx.c new file +14
-0+switch (wkup_m3->state) { +case M3_STATE_RESET : + wkup_m3->state =M3_STATE_INITED ; +break ; +case M3_STATE_MSG_FOR_RESET : + wkup_m3->state =M3_STATE_INITED ; +omap_mbox_msg_rx_flush (wkup_m3->mbox); +complete (&wkup_m3_sync); +break ; +case M3_STATE_MSG_FOR_LP : +omap_mbox_msg_rx_flush (wkup_m3->mbox); +complete (&wkup_m3_sync); +break ; +case M3_STATE_UNKNOWN :
Quoted 9 lines
arch/arm/mach-omap2/pm33xx.c line 286
Quoted 6 lines
arch/arm/mach-omap2/pm33xx.c new file +6
-0+pr_err ("IRQ %d with WKUP_M3 in unknown state\n" , irq); +omap_mbox_msg_rx_flush (wkup_m3->mbox); +return IRQ_NONE ; + } + +omap_ctrl_writel (0x0 ,AM33XX_CONTROL_M3_TXEV_EOI );undoumented magic write?
Will fix. This is for re-arming the interrupt line.
[...]
Quoted 7 lines
arch/arm/mach-omap2/pm33xx.c line 342
Quoted 4 lines
arch/arm/mach-omap2/pm33xx.c new file +4
-0+ + mem =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!mem) +dev_err (wkup_m3->dev,"no memory resource\n" );if !mem, this continues and still tries to ioremap NULL.
Will fix the error path.
[...]
Quoted 18 lines
arch/arm/mach-omap2/pm33xx.h line 64
Quoted 5 lines
arch/arm/mach-omap2/pm33xx.h new file +5
-0+ +/* +* This a subset of registers defined in drivers/memory/emif.h +* Move that to include/linux/? +*/ I'd probably suggest just moving the register definitions you need into <
plat/emif_plat.h> so they can be shared with the driver.Also, the EMIF stuff would benefit greatly from using symbolic defines for the values being written. Probably having those in <
plat/emif_plat.h> would also help out here.Or, maybe the EMIF driver can provide some self-contained stubs that can be copied to OCP RAM for the functionality needed here?
Santosh, what do you think of that?
[...]
Quoted 2 lines
Another user of the EMIF virtual addr is
emif_self_refresh_dis, but I don't see that code actually used anywhere.
Will check and remove it.
Quoted 5 lines
Looking closer, now I see the
ddr_self_refreshmacro is usingemif_virt_addr(that macro should be fixed to take the base address, for readability.)
Ok.
Quoted 4 lines
On a related note, just a quick glance at all the code running out of OCM RAM makes me wonder if any that could be done before jumping to OCM RAM. Ideally, we only want the absolute minimum running out of OCM RAM.
Some of the EMIF register saves could perhaps be moved in C but I kept it in assembly to be consistent with the resume sequence for the case where WFI ends up as NOP and the normal resume path.
[...]
Quoted 6 lines
arch/arm/mach-omap2/sleep33xx.S line 27
Quoted 3 lines
arch/arm/mach-omap2/sleep33xx.S new file +3
-0+ +/* replicated define because linux/bitops.h cannot be included in assembly */ +#define BIT(nr) (1 << (nr))never used, just remove it. Using shifts as below is fine (but using symbolic constants would be even better.)
I recall using it one place. Looks like I got rid of it after all.
Quoted 4 lines
In fact, there are lots of magic constants in this code that I'd like to see #defined.
Ok.
Quoted 19 lines
[...]
Quoted 4 lines
arch/arm/mach-omap2/sleep33xx.S new file +4
-0+ wfi + nop + nop + noparch/arm/mach-omap2/sleep33xx.S line 209
Quoted 10 lines
arch/arm/mach-omap2/sleep33xx.S new file +10
-0+ nop + nop + nop + nop + nop + nop + nop + nop + nop + nopWhy all the nops? Some comments would be helpful there.
That was for the A8 pipeline. Will add a comment.
Quoted 6 lines
[...]
Quoted 1 line
+/* Values recommended by the HW team */
A little documentation of these values would be helpful here.
Ok.
Thanks,
Vaibhav
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Quoted 1 line · Vaibhav
On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote:
[...]
Quoted 9 lines
Quoted 3 lines
Also, if there are drivers for these devices, won't this interfere?
Hmm, I can think of the following scenarios
1. Runtime PM adapted drivers are compiled in - We'll have to ensure that in their suspend callbacks they end up doing
omap_hwmod_idle()via the runtime PM APIs.
That already happens for all omap_devices.
Quoted 2 lines
In this case the
omap_hwmod_enable()followed byomap_hwmod_idle()is not necessary in the PM code.
Correct.
Quoted 4 lines
2. The drivers are not compiled in - In this case, the hwmod code puts the IPs in standby during bootup so the first suspend-resume iteration will pass. During resuming, the SYSC configuration for forced standby will be lost,
Please clarify how the SYSC is lost in this case.
Quoted 8 lines
so in the subsequent iterations these IPs won't go standby and the hwmod code does not touch them since they never got enabled. In this case we do need the sequence that is there currently.
3. For some reason the respective drivers don't idle the IPs during suspend - (
no_idle_on_suspendflag for the hwmod in DT?). In this scenario I think we should abort the suspend process since we know that the suspend is not going to work.
Agreed.
Quoted 1 line
Is there some other scenario that needs to be covered?
You covered the ones I was thinking of.
Quoted 3 lines
How about adding some flag in hwmod code for handling this? Something similar to what was done for MMC [1]. I think the problem that we have is in some ways similar to the one addressed in [1].
Except that in the absence of drivers, no hwmod code is executed on the suspend/resume path.
Kevin
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Kevin,
Quoted 21 lines · Kevin Hilman
Quoted 1 line · Vaibhav
On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote:
[...]
Quoted 9 lines
Quoted 3 lines
Also, if there are drivers for these devices, won't this interfere?
Hmm, I can think of the following scenarios
1. Runtime PM adapted drivers are compiled in - We'll have to ensure that in their suspend callbacks they end up doing
omap_hwmod_idle()via the runtime PM APIs.That already happens for all
omap_devices.Quoted 2 lines
In this case the
omap_hwmod_enable()followed byomap_hwmod_idle()is not necessary in the PM code.
Quoted 8 lines
Correct.
Quoted 4 lines
2. The drivers are not compiled in - In this case, the hwmod code puts the IPs in standby during bootup so the first suspend-resume iteration will pass. During resuming, the SYSC configuration for forced standby will be lost,
Please clarify how the SYSC is lost in this case.
The register configuration of IPs in the PER domain is lost when we enter the suspend state. So the forced standby configuration from SYSC is lost and we need to do this for every successful suspend-resume cycle.
Quoted 14 lines
Quoted 9 lines
so in the subsequent iterations these IPs won't go standby and the hwmod code does not touch them since they never got enabled. In this case we do need the sequence that is there currently.
3. For some reason the respective drivers don't idle the IPs during suspend - (
no_idle_on_suspendflag for the hwmod in DT?). In this scenario I think we should abort the suspend process since we know that the suspend is not going to work.Agreed.
Quoted 1 line
Is there some other scenario that needs to be covered?
Quoted 9 lines
You covered the ones I was thinking of.
Quoted 3 lines
How about adding some flag in hwmod code for handling this? Something similar to what was done for MMC [1]. I think the problem that we have is in some ways similar to the one addressed in [1].
Except that in the absence of drivers, no hwmod code is executed on the suspend/resume path.
We could perhaps add a couple of APIs to check the SYSC values when coming out of suspend and take appropriate action if the sysc cache does not match?
Regards,
Vaibhav
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
Quoted 57 lines · Vaibhav
Hi Kevin,
Quoted 21 lines · Kevin Hilman
Quoted 1 line · Vaibhav
On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote:
[...]
Quoted 9 lines
Quoted 3 lines
Also, if there are drivers for these devices, won't this interfere?
Hmm, I can think of the following scenarios
1. Runtime PM adapted drivers are compiled in - We'll have to ensure that in their suspend callbacks they end up doing
omap_hwmod_idle()via the runtime PM APIs.That already happens for all
omap_devices.Quoted 2 lines
In this case the
omap_hwmod_enable()followed byomap_hwmod_idle()is not necessary in the PM code.Quoted 8 lines
Correct.
Quoted 4 lines
2. The drivers are not compiled in - In this case, the hwmod code puts the IPs in standby during bootup so the first suspend-resume iteration will pass. During resuming, the SYSC configuration for forced standby will be lost,
Please clarify how the SYSC is lost in this case.
The register configuration of IPs in the PER domain is lost when we enter the suspend state. So the forced standby configuration from SYSC is lost and we need to do this for every successful suspend-resume cycle.
Quoted 20 lines
Quoted 9 lines
so in the subsequent iterations these IPs won't go standby and the hwmod code does not touch them since they never got enabled. In this case we do need the sequence that is there currently.
3. For some reason the respective drivers don't idle the IPs during suspend - (
no_idle_on_suspendflag for the hwmod in DT?). In this scenario I think we should abort the suspend process since we know that the suspend is not going to work.Agreed.
Quoted 1 line
Is there some other scenario that needs to be covered?
You covered the ones I was thinking of.
Quoted 3 lines
How about adding some flag in hwmod code for handling this? Something similar to what was done for MMC [1]. I think the problem that we have is in some ways similar to the one addressed in [1].
Quoted 6 lines
Quoted 3 lines
Except that in the absence of drivers, no hwmod code is executed on the suspend/resume path.
We could perhaps add a couple of APIs to check the SYSC values when coming out of suspend and take appropriate action if the sysc cache does not match?
Yes, for IPs with only SW support and no drivers, we may need something like this. But again, it needs to be suspend and idle aware, not just suspend.
Kevin
PATCH 15/15 ARM: OMAP2+: AM33XX: Basic suspend resume support
On Wed, Nov 07, 2012 at 22:45:20, Kevin Hilman wrote:
[...]
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
We could perhaps add a couple of APIs to check the SYSC values when coming out of suspend and take appropriate action if the sysc cache does not match?
Yes, for IPs with only SW support and no drivers, we may need something like this. But again, it needs to be suspend and idle aware, not just suspend.
Ok, if the pwrmdm pre and post transition callbacks do this that should take care of both suspend and idle.
Regards,
Vaibhav
cover Add basic suspend-resume support for AM33XX
In reply to Vaibhav BediaQuoted 1 line · Vaibhav Bedia
This patch series adds support for suspend-resume on AM33XX.
Thanks a lot for working on this!
[...]
Quoted 22 lines
[root at arago /]# echo 1 > ./sys/devices/ocp.2/wkup_m3.4/firmware/am335x-pm-firmware.bin/loading [root at arago /]# cat
binary_blob> ./sys/devices/ocp.2/wkup_m3.4/firmware/am335x-pm-firmware.bin/data [root at arago /]# echo 0 > ./sys/devices/ocp.2/wkup_m3.4/firmware/am335x-pm-firmware.bin/loading[ 28.770356] Copied the M3 firmware to UMEM [ 28.778178] omap_hwmod: wkup_m3: _wait_target_disable failed [root at arago /]# echo mem > /sys/power/state [ 32.959753] PM: Syncing filesystems ... done. [ 32.988434] Freezing user space processes ... (elapsed 0.02 seconds) done. [ 33.016120] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done. [ 33.047607] Suspending console(s) (use no_console_suspend to debug) [ 33.070420] PM: suspend of devices complete after 11.538 msecs [ 33.074095] PM: late suspend of devices complete after 3.651 msecs [ 33.079135] PM: noirq suspend of devices complete after 5.011 msecs [ 33.079196] Disabling non-boot CPUs ... [ 33.083923] GFX domain entered low power state [ 33.083973] Successfully transitioned to low power state [ 33.087132] PM: noirq resume of devices complete after 2.663 msecs [ 33.090629] PM: early resume of devices complete after 2.346 msecs [ 33.097607] PM: resume of devices complete after 6.951 msecs [ 33.155869] Restarting tasks ... done.[root at arago /]#
Quoted 7 lines
All the patches have been run through checkpatch and i have also tried a few OMAP build configs that Paul uses to ensure that these patches do not introduce any new build warnings/errors for the OMAP family.
A few TODOs
0. Test all the patches on OMAP platforms like BeagleBoard
I'm testing this on a custom AM33xx board, and with "no_console_suspend", I get the following:
# echo mem >/sys/power/state [ 27.306144] PM: Syncing filesystems ... done. [ 27.335524] Freezing user space processes ... (elapsed 0.01 seconds) done. [ 27.361603] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done. [ 27.408269] ------------[ cut here ]------------ [ 27.413147] WARNING: at drivers/net/ethernet/ti/cpsw.c:298 cpsw_rx_handler+0xbd/0xdc() [ 27.421438] Modules linked in: ipv6 [ 27.425158] [<c0010d85>] (unwind_backtrace+0x1/0x9c) from [<c0319f77>] (dump_stack+0xb/0xc) [ 27.433932] [<c0319f77>] (dump_stack+0xb/0xc) from [<c00266a3>] (warn_slowpath_common+0x33/0x48) [ 27.443149] [<c00266a3>] (warn_slowpath_common+0x33/0x48) from [<c00266c9>] (warn_slowpath_null+0x11/0x14) [ 27.453273] [<c00266c9>] (warn_slowpath_null+0x11/0x14) from [<c020f4b9>] (cpsw_rx_handler+0xbd/0xdc) [ 27.462943] [<c020f4b9>] (cpsw_rx_handler+0xbd/0xdc) from [<c020d773>] (__cpdma_chan_free+0x83/0x90) [ 27.472519] [<c020d773>] (__cpdma_chan_free+0x83/0x90) from [<c020d815>] (__cpdma_chan_process+0x95/0xb8) [ 27.482550] [<c020d815>] (__cpdma_chan_process+0x95/0xb8) from [<c020d8ff>] (cpdma_chan_stop+0x93/0x110) [ 27.492490] [<c020d8ff>] (cpdma_chan_stop+0x93/0x110) from [<c020d9ab>] (cpdma_ctlr_stop+0x2f/0x6c) [ 27.501975] [<c020d9ab>] (cpdma_ctlr_stop+0x2f/0x6c) from [<c020f033>] (cpsw_ndo_stop+0x2b/0x128) [ 27.511278] [<c020f033>] (cpsw_ndo_stop+0x2b/0x128) from [<c020f145>] (cpsw_suspend+0x15/0x20) [ 27.520311] [<c020f145>] (cpsw_suspend+0x15/0x20) from [<c01cce83>] (platform_pm_suspend+0x13/0x28) [ 27.529809] [<c01cce83>] (platform_pm_suspend+0x13/0x28) from [<c01cfb8b>] (dpm_run_callback.clone.6+0x1f/0x78) [ 27.540390] [<c01cfb8b>] (dpm_run_callback.clone.6+0x1f/0x78) from [<c01d0337>] (__device_suspend+0xb3/0x168) [ 27.550789] [<c01d0337>] (__device_suspend+0xb3/0x168) from [<c01d0a3d>] (dpm_suspend+0x45/0x178) [ 27.560095] [<c01d0a3d>] (dpm_suspend+0x45/0x178) from [<c01d0d15>] (dpm_suspend_start+0x39/0x44) [ 27.569404] [<c01d0d15>] (dpm_suspend_start+0x39/0x44) from [<c0046fdd>] (suspend_devices_and_enter+0x31/0x148) [ 27.579981] [<c0046fdd>] (suspend_devices_and_enter+0x31/0x148) from [<c0047203>] (pm_suspend+0x10f/0x170) [ 27.590103] [<c0047203>] (pm_suspend+0x10f/0x170) from [<c004693f>] (state_store+0x43/0x7c) [ 27.598870] [<c004693f>] (state_store+0x43/0x7c) from [<c019500d>] (kobj_attr_store+0x9/0x10) [ 27.607828] [<c019500d>] (kobj_attr_store+0x9/0x10) from [<c00cdb2d>] (sysfs_write_file+0xb5/0x104) [ 27.617319] [<c00cdb2d>] (sysfs_write_file+0xb5/0x104) from [<c008cb5b>] (vfs_write+0x77/0x110) [ 27.626442] [<c008cb5b>] (vfs_write+0x77/0x110) from [<c008cd75>] (sys_write+0x2d/0x48) [ 27.634844] [<c008cd75>] (sys_write+0x2d/0x48) from [<c000cfc1>] (ret_fast_syscall+0x1/0x50) [ 27.643683] ---[ end trace f2a0c58ea7742606 ]--- [ 27.650834] PM: suspend of devices complete after 253.597 msecs [ 27.660392] PM: late suspend of devices complete after 3.253 msecs [ 27.674504] omap_hwmod: cpgmac0: _wait_target_disable failed
And the power consumption does not drop noticable at this point.
What event did you use to bring the system back to life? I tried a GPIO button which has "linux,wakeup" set and is connected to GPIO bank 0, but without success.
Thanks,
Daniel
cover Add basic suspend-resume support for AM33XX
Hi Daniel,
On Sat, Nov 03, 2012 at 03:46:53, Daniel Mack wrote:
[...]
Quoted 4 lines
What event did you use to bring the system back to life? I tried a GPIO button which has "linux,wakeup" set and is connected to GPIO bank 0, but without success.
I used uart wakeup in my testing. I see that you have CPSW driver also in your kernel. Can you try with a minimal set of drivers, similar to what BeagleBone has on Tony's master branch right now.
Mugunthan mentioned he has a fix of the WARN_ON from CPSW driver that you see in the logs and that will be posted shortly. Can you mention what other patches you have pulled in?
Regards,
Vaibhav