RE: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Wed, Nov 07, 2012 at 22:45:20, Kevin Hilman wrote: [...] 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Kevin, On Wed, Nov 07, 2012 at 06:36:06, Kevin Hilman wrote: Bedia, Vaibhav vaibhav.be...@ti.com writes: On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote: [...] 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. In this case the omap_hwmod_enable() followed by omap_hwmod_idle() is not necessary in the PM code. Correct. 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. 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. Agreed. Is there some other scenario that needs to be covered? You covered the ones I was thinking of. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
Bedia, Vaibhav vaibhav.be...@ti.com writes: Hi Kevin, On Wed, Nov 07, 2012 at 06:36:06, Kevin Hilman wrote: Bedia, Vaibhav vaibhav.be...@ti.com writes: On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote: [...] 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. In this case the omap_hwmod_enable() followed by omap_hwmod_idle() is not necessary in the PM code. Correct. 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. 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. Agreed. Is there some other scenario that needs to be covered? You covered the ones I was thinking of. 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? 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Kevin,
On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote:
[...]
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
Ok, I'll try splitting the patches in the next version.
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.
[...]
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.
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].
+ /* 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.
[...]
+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.
+ 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.
Ok. I'll work on this for the next version.
+ 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.
Like you mentioned in another thread we could probably not have
a sync in the idle path. (More in the other thread).
+ 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;
+}
+
[...]
+static void am33xx_m3_state_machine_reset(void)
+{
+ int ret = 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.
+ if (!ret) {
+
RE: [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: [...] + +/* + * 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? 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Tuesday 06 November 2012 06:29 AM, Bedia, Vaibhav wrote: Hi Santosh, Kevin On Tue, Nov 06, 2012 at 03:22:16, Shilimkar, Santosh wrote: [...] + +/* + * 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? 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. 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 ? 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 ? 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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Tue, Nov 06, 2012 at 18:08:36, Shilimkar, Santosh wrote: On Tuesday 06 November 2012 06:29 AM, Bedia, Vaibhav wrote: Hi Santosh, Kevin On Tue, Nov 06, 2012 at 03:22:16, Shilimkar, Santosh wrote: [...] + +/* + * 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? 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. 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. 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 :( 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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
Bedia, Vaibhav vaibhav.be...@ti.com writes: On Mon, Nov 05, 2012 at 23:10:27, Kevin Hilman wrote: [...] 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. In this case the omap_hwmod_enable() followed by omap_hwmod_idle() is not necessary in the PM code. Correct. 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. 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. Agreed. Is there some other scenario that needs to be covered? You covered the ones I was thinking of. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
+Santosh (to help with EMIF questions/comments)
On 11/02/2012 12:32 PM, Vaibhav Bedia wrote:
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_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.
Signed-off-by: Vaibhav Bedia vaibhav.be...@ti.com
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.
[...]
+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);
+ 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?
+ /* 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.
[...]
+static int am33xx_pm_begin(suspend_state_t state)
+{
+ int ret = 0;
+
+ disable_hlt();
+
+ /*
+ * Physical
Re: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Monday 05 November 2012 11:10 PM, Kevin Hilman wrote: +Santosh (to help with EMIF questions/comments) On 11/02/2012 12:32 PM, Vaibhav Bedia wrote: 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_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. Signed-off-by: Vaibhav Bedia vaibhav.be...@ti.com 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. [...] +extern void __iomem *am33xx_get_emif_base(void); +int wkup_mbox_msg(struct notifier_block *self, unsigned long len, void *msg); +#endif + +#defineIPC_CMD_DS0 0x3 +#define IPC_CMD_RESET 0xe +#define DS_IPC_DEFAULT 0x + +#define IPC_RESP_SHIFT 16 +#define IPC_RESP_MASK (0x 16) + +#define M3_STATE_UNKNOWN 0 +#define M3_STATE_RESET 1 +#define M3_STATE_INITED2 +#define M3_STATE_MSG_FOR_LP3 +#define M3_STATE_MSG_FOR_RESET 4 + +#define AM33XX_OCMC_END0x4031 +#define AM33XX_EMIF_BASE 0x4C00 + +/* + * 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? 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
RE: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Sat, Nov 03, 2012 at 22:27:19, Shilimkar, Santosh wrote:
[...]
Nice descriptive change log Vaibhav.
Thanks :)
[...]
+#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.
+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
+*/
+ 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.
+ 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 ?
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?
+ /* 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.
Also are these clock domains are not supporting HW supervised
mode ?
All clock domains in AM33xx are SW-supervised.
+ /* 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.
+ 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;
+ 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 :)
[...]
+ 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.
[...]
+
+ 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.
[...]
+#define EMIF_DDR_PHY_CTRL_1_SHDW 0x00e8
+
Above should be part of the EMIF driver,
Re: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Friday 02 November 2012 06:02 PM, Vaibhav Bedia wrote: 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_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. Nice descriptive change log Vaibhav. Signed-off-by: Vaibhav Bedia vaibhav.be...@ti.com --- 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 diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile index ae87a3e..80736aa 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -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) diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c index 601ecdf..23894df 100644 --- a/arch/arm/mach-omap2/board-generic.c +++ b/arch/arm/mach-omap2/board-generic.c @@ -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, diff --git
RE: [PATCH 15/15] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Fri, Nov 02, 2012 at 18:02:46, Bedia, Vaibhav wrote:
[...]
+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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
