RE: [PATCH 6/7] [RFC] OMAP: hwmod: SYSCONFIG register modification for MCBSP
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Thursday, December 02, 2010 4:24 PM
To: Cousson, Benoit
Cc: Basak, Partha; ABRAHAM, KISHON VIJAY; Paul Walmsley; linux-
o...@vger.kernel.org; Kamat, Nishant; Varadarajan, Charulatha; Datta,
Shubhrajyoti; ext-eero.nurkk...@nokia.com; eduardo.valen...@nokia.com;
Raja, Govindraj
Subject: Re: [PATCH 6/7] [RFC] OMAP: hwmod: SYSCONFIG register
modification for MCBSP
Cousson, Benoit b-cous...@ti.com writes:
[...]
The issue is not really for the mcbsp but for the serial that need to
handle the 3 different states.
if (enable) {
/**
* Errata 2.15: [UART]:Cannot Acknowledge Idle Requests
* in Smartidle Mode When Configured for DMA Operations.
*/
if (uart-dma_enabled)
idlemode = HWMOD_IDLEMODE_FORCE;
else
idlemode = HWMOD_IDLEMODE_SMART;
} else {
idlemode = HWMOD_IDLEMODE_NO;
}
You do need to explicitly set the 3 modes, hence the following 3
APIs:
omap_device_force_idle
omap_device_no_idle
omap_device_smart_idle
I am OK with Benoit's 3 + 1 API approach, 3 individual set functions and
one common function to restore back to the value as dictated by the flag.
What do you say guys?
You can potentially add another API to restore the default idle mode:
omap_device_default_idle
That seems much simpler than 3 pairs of APIs.
My $0.02... based on the fact that we already have some IPs needing to
conrol all 3 modes, I think having the 3 functions above is better than
having 3 pairs of request/release functions.
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 v6 8/8] Input: omap4 - pm runtime
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Datta, Shubhrajyoti Sent: Monday, December 06, 2010 6:30 PM To: Kevin Hilman; Arce, Abraham Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org Subject: RE: [PATCH v6 8/8] Input: omap4 - pm runtime Kevin, -Original Message- From: linux-input-ow...@vger.kernel.org [mailto:linux-input- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Thursday, September 30, 2010 7:21 PM To: Arce, Abraham Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org Subject: Re: [PATCH v6 8/8] Input: omap4 - pm runtime Abraham Arce x0066...@ti.com writes: Enable pm runtime in driver So power is enabled on probe and cut on _remove(). Did you consider doing any more fine grained PM for this device? For example, cutting power after some inactivity timer and re-enabling on a keypress/interrupt? My idea is that the clock needs to be on to get interrupts (OMAP4 the control is at module level and ick/fclk level control is difficult). So disabling will prevent interrupts. The keypad is in wakeup domain which is always on. So the power impact may be minimal. What do you think. Kevin, I hope you are OK with this. Kevin Reviewed-by: Basak, Partha p-bas...@ti.com Signed-off-by: Abraham Arce x0066...@ti.com --- drivers/input/keyboard/omap4-keypad.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c index 45bd097..ed47e9a 100644 --- a/drivers/input/keyboard/omap4-keypad.c +++ b/drivers/input/keyboard/omap4-keypad.c @@ -29,6 +29,7 @@ #include linux/io.h #include linux/input.h #include linux/slab.h +#include linux/pm_runtime.h #include plat/omap4-keypad.h @@ -239,6 +240,9 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev) matrix_keypad_build_keymap(pdata-keymap_data, row_shift, input_dev-keycode, input_dev-keybit); + pm_runtime_enable(pdev-dev); + pm_runtime_get_sync(pdev-dev); + omap4_keypad_config(keypad_data); error = request_irq(keypad_data-irq, omap4_keypad_interrupt, @@ -277,6 +281,9 @@ static int __devexit omap4_keypad_remove(struct platform_device *pdev) struct omap4_keypad *keypad_data = platform_get_drvdata(pdev); struct resource *res; + pm_runtime_put_sync(pdev-dev); + pm_runtime_disable(pdev-dev); + free_irq(keypad_data-irq, keypad_data); input_unregister_device(keypad_data-input); -- To unsubscribe from this list: send the line unsubscribe linux- input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 6/7] [RFC] OMAP: hwmod: SYSCONFIG register modification for MCBSP
-Original Message-
From: Cousson, Benoit
Sent: Tuesday, November 30, 2010 9:33 PM
To: ABRAHAM, KISHON VIJAY; Paul Walmsley
Cc: Kevin Hilman; Basak, Partha; linux-omap@vger.kernel.org; Kamat,
Nishant; Varadarajan, Charulatha; Datta, Shubhrajyoti; ext-
eero.nurkk...@nokia.com; eduardo.valen...@nokia.com
Subject: Re: [PATCH 6/7] [RFC] OMAP: hwmod: SYSCONFIG register
modification for MCBSP
Hi Kishon,
Sorry, for the delay.
On 11/22/2010 4:59 PM, ABRAHAM, KISHON VIJAY wrote:
Resending the mail in plain text format..
On Mon, Nov 22, 2010 at 9:20 PM, ABRAHAM, KISHON VIJAYkis...@ti.com
wrote:
On Mon, Oct 11, 2010 at 11:48 AM, ABRAHAM, KISHON
VIJAYkis...@ti.com wrote:
On Friday 08 October 2010 01:12 PM, Cousson, Benoit wrote:
Hi Kishon,
On 10/5/2010 6:37 PM, ABRAHAM, KISHON VIJAY wrote:
MCBSP 2 and 3 in OMAP3 has sidetone feature which requires
autoidle to be disabled before starting the sidetone. Also
SYSCONFIG
register has to be set with smart idle or no idle depending on
the
dma op mode (threshold or element sync). For doing these
operations
dynamically at runtime, hwmod API'S are used to modify SYSCONFIG
register
directly.
OK, it looks like we don't have the choice... But for the
implementation, please discussed with Manju on that, He is doing
the
similar thing for the smartstandby issue on SDMA.
OK.
Looks like we have a problem when we write an API similar to
the one written
by Manju (omap_hwmod_set_master_standbymode()) [1]. In the
case of McBSP,
I have to modify omap_hwmod_set_slave_idlemode() (which is
already existing),
to include something like
+if (sf SYSC_HAS_SIDLEMODE) {
+if (idlemode)
+idlemode = HWMOD_IDLEMODE_NO;
+else
+idlemode = (oh-flags
HWMOD_SWSUP_SIDLE) ?
+HWMOD_IDLEMODE_NO :
HWMOD_IDLEMODE_SMART;
+}
How to you enable HWMOD_IDLEMODE_FORCE mode in that case?
Then an API like omap_device_require_no_idle() and
omap_device_release_no_idle()
would be written similar to omap_device_require_no_mstandby
and
omap_device_release_no_mstandby() (see [1]) that calls
omap_hwmod_set_slave_idlemode() with true/false. Passing true
to
omap_hwmod_set_slave_idlemode() will write SIDLE bits with
HWMOD_IDLEMODE_NO and false to it will write
HWMOD_IDLEMODE_NO/HWMOD_IDLEMODE_SMART depending on
HWMOD_SWSUP_SIDLE to SIDLE bits.
This works fine for McBSP since only two modes come to play
here
(HWMOD_IDLEMODE_NO/HWMOD_IDLEMODE_SMART).
But omap_hwmod_set_slave_idlemode() API is used by UART
(serial.c Please refer [2]) which writes SIDLE bits to
HWMOD_IDLEMODE_NO/HWMOD_IDLEMODE_SMART/ and
HWMOD_IDLEMODE_FORCE.
That code should not be there. It was a temporary hacks that should be
replaced eventually with a clean omap_device API like the one you are
currently implementing.
There are a bunch of direct access to omap_hwmod struct that need to be
removed as well.
Modifying omap_hwmod_set_slave_idlemode() will require
1) serial.c to be modified to call functions like
'omap_device_require_no_idle(),
omap_device_release_no_idle()' and
'omap_device_require_force_idle() and
omap_device_release_force_idle()' instead of
omap_hwmod_set_slave_idlemode() which is currently
present.
Yep, you're right.
There are 2 problems associated with it
1) The first problem is having a single API like
omap_hwmod_set_slave_idlemode() to
set two different values like HWMOD_IDLEMODE_NO or
HWMOD_IDLEMODE_FORCE (intended to be called by
omap_device_require_no_idle()
and omap_device_require_force_idle() respectively).
According to the new design
omap_hwmod_set_slave_idlemode() is intended to take only
true/false as
arguments.
2) The second problem is having the release API's
(omap_device_release_no_idle() and
omap_device_release_force_idle()) to restore the
SYSCONFIG to the previous state.
Remember, this was not problem for McBSP since only two
modes were needed.
And what about 3 APIs?
omap_device_force_idle
omap_device_no_idle
omap_device_smart_idle
Want to reiterate Paul Walmsley's comments on Manju's DMA email dated 11/11/2010
snip
2. Rather than just using a single omap_device_mstandby() function call,
I'd rather see something like omap_device_require_no_standby() and
omap_device_release_no_standby(). omap_device_require_no_standby() would
force the initiator port into no-standby
RE: RFC: hwmod, iclks, auto-idle and autodeps
-Original Message- From: Paul Walmsley [mailto:p...@pwsan.com] Sent: Tuesday, November 23, 2010 9:31 AM To: Kevin Hilman Cc: Cousson, Benoit; Basak, Partha; Varadarajan, Charulatha; linux- o...@vger.kernel.org Subject: Re: RFC: hwmod, iclks, auto-idle and autodeps Hi Kevin and I discussed this privately, this is just to summarize for everyone else. On Wed, 17 Nov 2010, Kevin Hilman wrote: There have been a few discussions over the few months about how to properly use omap_hwmod to manage certain IPs that have the interface clock as the functional clock (e.g. OMAP3 GPIOs.) The goal of this RFC is to come to a conclusion about what should be done, and how to go about implementing it. In the initial conversion of the GPIO core to omap_hwmod, main_clk was left NULL so that hwmod was not managing the clock on every hwmod enable/disable. Since GPIO has a register target, main_clk cannot be null. There's an erroneous comment in plat/omap_hwmod.h about this; I'll add it to my list of things to fix. This behavior matched current mainline GPIO code, which does not dynamically disable/enable GPIO iclks after init time. Instead, it relies on the module-level auto idle feature in HW. However, without dynamically managing the clock in hwmod, this meant that there were no autodeps tracked for GPIO and thus the PER powerdomain could transition independently of MPU/CORE. The current GPIO driver works only because it exploits some incidental properties of the OMAP core code. It implicitly relies on CM_AUTOIDLE = 1 on its iclk for power management to work, since the driver never disables its iclk. The driver also relies on the addition of MPU/IVA2 autodeps to avoid losing logic context once all devices in PER go idle. And by never explicitly disabling its iclk, the driver avoids dealing with the various GPIO wakeup/interrupt modes, causing its context save/restore to be implemented as a weird hack in pm34xx.c. That said, there definitely is one real limitation/bug in the OMAP PM core code that the GPIO driver has to work around. The current core code doesn't try to keep a powerdomain powered when an IP block inside the powerdomain is still considered to be in use but all its system-sourced clocks are cut. This can result in unexpected context loss and malfunction in some GPIO and McBSP cases, possibly some other modules also that can be externally clocked and contain FIFOs/buffers, or that can generate asynchronous wakeups. There's a patch in the works here that will require a powerdomain to stay on as long as a hwmod in that powerdomain is enabled. Once omap_hwmod_idle() is called for that hwmod, the lower-bound on the power state of the powerdomain is removed. So in the context of the PM runtime conversion of the GPIO driver, the thing to do is to only do a pm_runtime_put*() once the GPIO module is really ready to be powered down. After that call, the GPIO block may lose its logic context, and it may not be able to generate interrupts or wakeups. We may enforce the latter in the hwmod code for debugging purposes by forcing the module into idle and instructing the PRCM to ignore wakeups from the module in omap_hwmod_idle(). IO ring/chain wakeups may still occur, but these are independent of the GPIO block. The rest of the time, if the GPIO driver wants to use idle-mode wakeups (presumably higher wakeup latency, but lower power consumption), it should just clk_disable() its clocks, but not call pm_runtime_put*(). After This would require that clk_disable() be called directly by the driver outside of pm_runtime_putsync(). Isn't this not in line with runtime PM paradigm? the previously-mentioned improvement to the powerdomain/hwmod code is completed and applied, that should result in the powerdomain staying powered, but all clocks being disabled. Otherwise, if the GPIO driver wants to use active-mode interrupts (presumably lower wakeup latency, but higher power consumption), then the driver should just leave its clocks enabled and never call pm_runtime_put*(). So, if I understood correctly, it is up to the driver to take the call. It is perfectly OK if the driver decides to do a pm_runtime_get_sync() when a GPIO module is requested and not call pm_runtime_put_sync() until it is released. Now, in OMAP, because of the incidental availability of AutoIdle feature, this will not prevent the clocks getting auto-gated. Both of these latter modes will block some low-power states. At some point, the selection of the interrupt/wakeup mode -- GPIO active, GPIO idle, IO ring/chain -- should be made based on the required wakeup latency of the GPIO user. Multiple modes may need to used simultaneously, since individual modes are restricted to certain power states (e.g. IO ring is only available in CORE off, IO chain is only available in CORE/PER retention) The initial solution
RE: [PATCHv3 8/17] dmtimer: register mappings moved to hwmod database
-Original Message-
From: DebBarma, Tarun Kanti
Sent: Tuesday, October 12, 2010 11:19 AM
To: Cousson, Benoit
Cc: Basak, Partha; Kevin Hilman; G, Manjunath Kondaiah;
linux-omap@vger.kernel.org; Shilimkar, Santosh; Paul
Walmsley; Tony Lindgren
Subject: RE: [PATCHv3 8/17] dmtimer: register mappings moved
to hwmod database
Benoit,
-Original Message-
From: Cousson, Benoit
Sent: Tuesday, October 12, 2010 4:42 AM
To: DebBarma, Tarun Kanti
Cc: Basak, Partha; Kevin Hilman; G, Manjunath Kondaiah; linux-
o...@vger.kernel.org; Shilimkar, Santosh; Paul Walmsley;
Tony Lindgren
Subject: Re: [PATCHv3 8/17] dmtimer: register mappings
moved to hwmod
database
Hi Tarun,
On 10/9/2010 5:40 PM, DebBarma, Tarun Kanti wrote:
Benoit,
From: Cousson, Benoit
Sent: Thursday, September 23, 2010 10:15 PM
To: Basak, Partha
Cc: Kevin Hilman; G, Manjunath Kondaiah; DebBarma, Tarun
Kanti; linux-
o...@vger.kernel.org; Shilimkar, Santosh; Paul Walmsley;
Tony Lindgren
Subject: Re: [PATCHv3 8/17] dmtimer: register mappings
moved to hwmod
database
On 9/23/2010 11:34 AM, Basak, Partha wrote:
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Thursday, September 23, 2010 3:10 AM
G, Manjunath Kondaiahmanj...@ti.com writes:
Hi Tarun,
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
DebBarma, Tarun Kanti
...
+static u32 omap_timer_reg_map_v1[] = {
+ [OMAP_TIMER_ID_REG] = (0x00 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_OCP_CFG_REG]= (0x10 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_SYS_STAT_REG] = (0x14 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_STAT_REG] = (0x18 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_INT_EN_REG] = (0x1c | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_WAKEUP_EN_REG] = (0x20 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_CTRL_REG] = (0x24 | (WP_TCLR
WPSHIFT)),
+ [OMAP_TIMER_COUNTER_REG]= (0x28 | (WP_TCRR
WPSHIFT)),
+ [OMAP_TIMER_LOAD_REG] = (0x2c | (WP_TLDR
WPSHIFT)),
+ [OMAP_TIMER_TRIGGER_REG]= (0x30 | (WP_TTGR
WPSHIFT)),
+ [OMAP_TIMER_WRITE_PEND_REG] = (0x34 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_MATCH_REG] = (0x38 | (WP_TMAR
WPSHIFT)),
+ [OMAP_TIMER_CAPTURE_REG]= (0x3c | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_IF_CTRL_REG]= (0x40 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_CAPTURE2_REG] = (0x44 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_TICK_POS_REG] = (0x48 | (WP_TPIR
WPSHIFT)),
+ [OMAP_TIMER_TICK_NEG_REG] = (0x4c | (WP_TNIR
WPSHIFT)),
+ [OMAP_TIMER_TICK_COUNT_REG] = (0x50 | (WP_TCVR
WPSHIFT)),
+ [OMAP_TIMER_TICK_INT_MASK_SET_REG] = (0x54 |
(WP_TOCR WPSHIFT)),
+ [OMAP_TIMER_TICK_INT_MASK_COUNT_REG]= (0x58 |
(WP_TOWR WPSHIFT)),
+};
+
+/* OMAP4 timers register map */
+static u32 omap_timer_reg_map_v2[] = {
+ [OMAP_TIMER_ID_REG] = (0x00 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_OCP_CFG_REG]= (0x10 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_SYS_STAT_REG] = (0x14 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_STAT_REG] = (0x28 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_INT_EN_REG] = (0x2c | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_WAKEUP_EN_REG] = (0x34 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_CTRL_REG] = (0x38 | (WP_TCLR
WPSHIFT)),
+ [OMAP_TIMER_COUNTER_REG]= (0x3c | (WP_TCRR
WPSHIFT)),
+ [OMAP_TIMER_LOAD_REG] = (0x40 | (WP_TLDR
WPSHIFT)),
+ [OMAP_TIMER_TRIGGER_REG]= (0x44 | (WP_TTGR
WPSHIFT)),
+ [OMAP_TIMER_WRITE_PEND_REG] = (0x48 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_MATCH_REG] = (0x4c | (WP_TMAR
WPSHIFT)),
+ [OMAP_TIMER_CAPTURE_REG]= (0x50 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_IF_CTRL_REG]= (0x54 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_CAPTURE2_REG] = (0x58 | (WP_NONE
WPSHIFT)),
+ [OMAP_TIMER_INT_CLR_REG]= (0x30 | (WP_NONE
WPSHIFT)),
+};
+
Why not these defines in mach-omap2/dmtimer.c? since
register offsets are
same for omap2plus except omap4 which has additional
register offsets. Same
register offsets are getting repeated in all the omap2plus
hwmod database.
I agree with Manjunath.
Manju, the number of tables needed to manage the
information are same
really.
Its only where they are located changes from the mach
layer to the
hwmod
database. So, thats not the point. dev_attrs are
pointing to the reg-
map
tables, they are not duplicating them.
The offset differences are stemming from the IP differences.
To my understanding, only IP-integration specific
idiosyncrasies into
a
given
SOC qualifiy as dev_attrs.
IP specific details like reg-map information should
RE: [PATCHv3 8/17] dmtimer: register mappings moved to hwmod database
-Original Message- From: Cousson, Benoit Sent: Tuesday, October 12, 2010 1:32 PM To: Basak, Partha Cc: DebBarma, Tarun Kanti; Kevin Hilman; G, Manjunath Kondaiah; linux-omap@vger.kernel.org; Shilimkar, Santosh; Paul Walmsley; Tony Lindgren Subject: Re: [PATCHv3 8/17] dmtimer: register mappings moved to hwmod database On 10/12/2010 9:22 AM, Basak, Partha wrote: From: DebBarma, Tarun Kanti Sent: Tuesday, October 12, 2010 11:19 AM To: Cousson, Benoit Benoit, From: Cousson, Benoit Sent: Tuesday, October 12, 2010 4:42 AM To: DebBarma, Tarun Kanti Hi Tarun, On 10/9/2010 5:40 PM, DebBarma, Tarun Kanti wrote: Benoit, From: Cousson, Benoit Sent: Thursday, September 23, 2010 10:15 PM To: Basak, Partha On 9/23/2010 11:34 AM, Basak, Partha wrote: From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, September 23, 2010 3:10 AM G, Manjunath Kondaiahmanj...@ti.comwrites: Hi Tarun, From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of DebBarma, Tarun Kanti ... Also, I'm not sure that you have to handle each register separately considering that you have one offset to handle for the functional register. The change in sysconfig and interrupt register are all standard mapping that stick to TI highlander standard. Meaning, as soon as an IP is a v2 (highlander version) all these registers will be at the same place... at least in theory :-) Here are the deltas between the legacy and the new one: [OMAP_TIMER_ID_REG] 0x00, [OMAP_TIMER_OCP_CFG_REG]0x10, same [OMAP_TIMER_SYS_STAT_REG] 0x14, (not used anymore) You should not care about these ones, because hwmod will handle them. [OMAP_TIMER_STAT_REG] 0x28, +10 [OMAP_TIMER_INT_EN_REG] 0x2c, +10 [OMAP_TIMER_INT_CLR_REG]0x30, (new) These ones are standard registers [OMAP_TIMER_WAKEUP_EN_REG] 0x34, +14 [OMAP_TIMER_CTRL_REG] 0x38, +14 [OMAP_TIMER_COUNTER_REG]0x3c, +14 [OMAP_TIMER_LOAD_REG] 0x40, +14 [OMAP_TIMER_TRIGGER_REG]0x44, +14 [OMAP_TIMER_WRITE_PEND_REG] 0x48, +14 [OMAP_TIMER_MATCH_REG] 0x4c, +14 [OMAP_TIMER_CAPTURE_REG]0x50, +14 [OMAP_TIMER_IF_CTRL_REG]0x54, +14 [OMAP_TIMER_CAPTURE2_REG] 0x58, +14 You can probably handle that with only 2 offsets instead of having one address per registers. To keep aligned with other driver implementations, I would like to follow this: 1) move the table in mach-omap1/2 since 2) remove entries which are handled by hwmod. However, I am not sure if there is any issue in keeping them in the table. I don't think you need a table for that. All the functional registers are using a constant offset. IRQ is then the only exception. Other than the offsets 0x14 for the Timer functional registers and 0x10 for the IRQ registers, following differences exist between the two versions: 1. OMAP_TIMER_SYS_STAT_REG is used only for v1, missing for v2. This is handled by hwmod anyway. 2. OMAP_TIMER_INT_CLR_REG is new for v2, not used for v1. Follow regular IRQ management for highlander IP. Should be some generic code in order to allow reuse. 3. Following registers are applicable only for v1. [OMAP_TIMER_TICK_POS_REG] 0x48 [OMAP_TIMER_TICK_NEG_REG] 0x4c [OMAP_TIMER_TICK_COUNT_REG] 0x50 [OMAP_TIMER_TICK_INT_MASK_SET_REG] 0x54 [OMAP_TIMER_TICK_INT_MASK_COUNT_REG]0x58 These are only there in the 1ms version of this IP. In the end, having separate tables is neater and consistent with other drivers. I don't see why? And what other driver are you referring too? I2c driver uses reg_map tables. Initially McSPI and MMC were using similar reg_map tables which were later removed based on review comments. However, the delta were minimal in this case. e.g. MCSPI_HL_REV, MCSPI_HL_HWINFO, MCSPI_HL_SYSCONFIG were the only difference for McSPI, all the other functional registers were moved forward by 0x100. Maybe, it is relevant for I2C as the amount of delta is more. I also observed your comments on MMC. Now that both McSPI MMC are not having table based approach, my argument of consistency across drivers does not hold good. It depends upon the IPs really. What the point of adding a table and an extra level of indirection, when a simple addition will do it? But instead of duplicating for each mach, keeping them in plat should be OK. Additionally, the implementation should take care to prevent access of non-existing registers for a particular version. You just need the IP version to do that. We have 3 timer variants to handle today: Legacy timer, legacy 1ms timer and highlander timer. You can handle that with 2 flags and 2 offsets. If you look
RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Friday, September 24, 2010 5:23 AM To: Basak, Partha Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo; Cousson, Benoit Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path Kevin Hilman khil...@deeprootsystems.com writes: [...] We cannot do a get_sync() from ISR context, right? Right, but we *should* be able to. ;) I'm still trying to craft a good description of this problem so I can argue better for it on linux-pm. Until then... A bit of a hack, but you could do a _get_noresume() (which is safe from interrupt context) and directly call the drivers -runtime_resume() method, which would be the equivalent of a _get_sync(). Followed of course by a _put() (async version, also interrupt safe) at the end of the ISR to keep the usecount correct. You probably figured this out already, but I just realized that this won't currently work either as omap_hwmod is using mutexes, and is safe in ISR context either. :( What about for now just directly enabling (and re-disabling) the hwmod clocks in the ISR using omap_hwmod_[enable|disable]_clocks() Since this is a core driver in arch/arm/*omap*, you can directly call the omap_hwmod API. omap_hwmod_[enable/disable]_clocks() use mutex inside therefore cannot be used in the ISR context We cannot readily use the _enable_clocks/_disble_clocks directly as they are static APIs. But we can use the non-mutex versions, of omap_hwmod_enable/idle. (_omap_hwmod_enable/_omap_hwmod_disable) Do you agree? 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: [PATCHv3 8/17] dmtimer: register mappings moved to hwmod database
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Thursday, September 23, 2010 3:10 AM
To: G, Manjunath Kondaiah
Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; Basak,
Partha; Shilimkar, Santosh; Cousson, Benoit; Paul Walmsley;
Tony Lindgren
Subject: Re: [PATCHv3 8/17] dmtimer: register mappings moved
to hwmod database
G, Manjunath Kondaiah manj...@ti.com writes:
Hi Tarun,
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
DebBarma, Tarun Kanti
Sent: Tuesday, September 21, 2010 2:24 PM
To: linux-omap@vger.kernel.org
Cc: DebBarma, Tarun Kanti; Basak, Partha; Shilimkar, Santosh;
Cousson, Benoit; Paul Walmsley; Kevin Hilman; Tony Lindgren
Subject: [PATCHv3 8/17] dmtimer: register mappings moved to
hwmod database
This patch adds dmtimer register mappings to hwmod database.
This is to avoid maintaining different several mappings
within the omap-plat. The mapping is made available to
platform through dev_attr during device build. The pointer to
register map is preserved in the platform data.
Please note that the cleanup of register map from plat-omap
will be removed in later patch after conversion to platform driver.
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
Signed-off-by: Partha Basak p-bas...@ti.com
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Cousson, Benoit b-cous...@ti.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Tony Lindgren t...@atomide.com
---
arch/arm/mach-omap2/omap_hwmod_2420_data.c | 25 ++
arch/arm/mach-omap2/omap_hwmod_2430_data.c | 25 ++
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 25 ++
arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 67
---
arch/arm/plat-omap/dmtimer.c | 13 -
arch/arm/plat-omap/include/plat/dmtimer.h | 38
6 files changed, 172 insertions(+), 21 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
index 6003c2e..b3dd8ac 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
@@ -131,8 +131,33 @@ static char *timer_clk_src_names[] = {
NULL,
};
+/* OMAP2/3 timers register map */
+static u32 omap_timer_reg_map_v1[] = {
+ [OMAP_TIMER_ID_REG] = (0x00 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_OCP_CFG_REG]= (0x10 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_SYS_STAT_REG] = (0x14 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_STAT_REG] = (0x18 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_INT_EN_REG] = (0x1c | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_WAKEUP_EN_REG] = (0x20 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_CTRL_REG] = (0x24 | (WP_TCLR WPSHIFT)),
+ [OMAP_TIMER_COUNTER_REG]= (0x28 | (WP_TCRR WPSHIFT)),
+ [OMAP_TIMER_LOAD_REG] = (0x2c | (WP_TLDR WPSHIFT)),
+ [OMAP_TIMER_TRIGGER_REG]= (0x30 | (WP_TTGR WPSHIFT)),
+ [OMAP_TIMER_WRITE_PEND_REG] = (0x34 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_MATCH_REG] = (0x38 | (WP_TMAR WPSHIFT)),
+ [OMAP_TIMER_CAPTURE_REG]= (0x3c | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_IF_CTRL_REG]= (0x40 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_CAPTURE2_REG] = (0x44 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_TICK_POS_REG] = (0x48 | (WP_TPIR WPSHIFT)),
+ [OMAP_TIMER_TICK_NEG_REG] = (0x4c | (WP_TNIR WPSHIFT)),
+ [OMAP_TIMER_TICK_COUNT_REG] = (0x50 | (WP_TCVR WPSHIFT)),
+ [OMAP_TIMER_TICK_INT_MASK_SET_REG] = (0x54 |
(WP_TOCR WPSHIFT)),
+ [OMAP_TIMER_TICK_INT_MASK_COUNT_REG]= (0x58 |
(WP_TOWR WPSHIFT)),
+};
+
static struct omap_timer_dev_attr timer_dev_attr = {
.clk_names = timer_clk_src_names,
+ .reg_map= omap_timer_reg_map_v1,
};
static struct omap_hwmod_class_sysconfig omap2420_timer_sysc
= { diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
index 4b43fb9..787d3ce 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
@@ -137,8 +137,33 @@ static char *timer_clk_src_names[] = {
NULL
};
+/* OMAP2/3 timers register map */
+static u32 omap_timer_reg_map_v1[] = {
+ [OMAP_TIMER_ID_REG] = (0x00 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_OCP_CFG_REG]= (0x10 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_SYS_STAT_REG] = (0x14 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_STAT_REG] = (0x18 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_INT_EN_REG] = (0x1c | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_WAKEUP_EN_REG] = (0x20 | (WP_NONE WPSHIFT)),
+ [OMAP_TIMER_CTRL_REG] = (0x24 | (WP_TCLR
RE: [PATCH 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, September 14, 2010 10:28 PM
To: Basak, Partha
Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
interrupts-disabled idle path
Basak, Partha p-bas...@ti.com writes:
From: Kevin Hilman khil...@ti.com
Currently, we wait until late in the idle path where interrupts are
disabled to do runtime-PM-like management for certain special-case
devices like GPIO.
As a prerequiste to moving GPIO to the new runtime PM
framework, move
this runtime-PM-like code out of the late idle path into new device
idle and resume functions that can be called before interrupts are
disabled by CPUidle and/or suspend.
In addition, move all the GPIO-specific logic into the GPIO core
instead of keeping GPIO-specific knowledge of power-states, context
saving etc. in the PM core.
Also, call the new device-idle and -resume methods from CPUidle and
static suspend path.
Signed-off-by: Kevin Hilman khil...@ti.com
---
arch/arm/mach-omap2/cpuidle34xx.c |4 ++
arch/arm/mach-omap2/pm.h |2 +
arch/arm/mach-omap2/pm24xx.c |2 +-
arch/arm/mach-omap2/pm34xx.c | 38
+
arch/arm/plat-omap/gpio.c | 57
arch/arm/plat-omap/include/plat/gpio.h |4 +--
6 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 0923b82..681d823 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
pwrdm_set_next_pwrst(per_pd, per_next_state);
select_state:
+ omap3_device_idle();
+
dev-last_state = new_state;
ret = omap3_enter_idle(dev, new_state);
+ omap3_device_resume();
+
In the generic cpu_idle() in process.c, interrupts are
already disabled
before control comes to cpuidle_idle_call() via pm_idle()
local_irq_disable();
if (hlt_counter) {
local_irq_enable();
cpu_relax();
} else {
stop_critical_timings();
pm_idle();
start_critical_timings();
/*
* This will eventually be
removed - pm_idle
* functions should always
return with IRQs
* enabled.
*/
WARN_ON(irqs_disabled());
local_irq_enable();
}
omap3_enter_idle_bm() will be called from inside
cpuidle_idle_call()
via target_state-enter(dev, target_state).
So, interrupts are already disabled here.
Am I missing something?
You're right.
I knew this was the case for !CPUIDLE setup, but had thought (without
testing) that the CPUidle core had re-enabled interrupts during the
governor selection process etc.
While I investigate ways to manage this in CPUidle, the
following should
be fine for now to include with $SUBJECT patch.
Kevin
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 681d823..c5cb9d0 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
goto select_state;
}
+ /*
+ * Enable IRQs during the device activity checking and
idle management.
+ * IRQs are later (re)disabled when entering the actual
idle function.
+ * Device idle management that is using runtime PM needs to have
+ * interrupts enabled when calling into the runtime PM core.
+ */
+ local_irq_enable();
After put_sync() retuns, there will be a time window where interrupts
are enabled but clocks are disabled before the interrupts are disabled again.
Accessing any register to service a device interrupt coming during this window
will lead to a crash for cases where iclk and fclks are same and we have the
iclk defined as the main_clk as well.
Same argument holds while returning from Idle. We are facing this issue for
OMAP3
GPIO while trying to define the main_clk = interface clock based on your other
commment.
+
cx = cpuidle_get_statedata(state);
core_next_state = cx-core_state;
k
--
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 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Thursday, September 23, 2010 9:08 PM
To: Basak, Partha
Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
interrupts-disabled idle path
Basak, Partha p-bas...@ti.com writes:
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, September 14, 2010 10:28 PM
To: Basak, Partha
Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha;
Tero Kristo
Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
interrupts-disabled idle path
Basak, Partha p-bas...@ti.com writes:
From: Kevin Hilman khil...@ti.com
Currently, we wait until late in the idle path where
interrupts are
disabled to do runtime-PM-like management for certain
special-case
devices like GPIO.
As a prerequiste to moving GPIO to the new runtime PM
framework, move
this runtime-PM-like code out of the late idle path
into new device
idle and resume functions that can be called before
interrupts are
disabled by CPUidle and/or suspend.
In addition, move all the GPIO-specific logic into the GPIO core
instead of keeping GPIO-specific knowledge of
power-states, context
saving etc. in the PM core.
Also, call the new device-idle and -resume methods from
CPUidle and
static suspend path.
Signed-off-by: Kevin Hilman khil...@ti.com
---
arch/arm/mach-omap2/cpuidle34xx.c |4 ++
arch/arm/mach-omap2/pm.h |2 +
arch/arm/mach-omap2/pm24xx.c |2 +-
arch/arm/mach-omap2/pm34xx.c | 38
+
arch/arm/plat-omap/gpio.c | 57
arch/arm/plat-omap/include/plat/gpio.h |4 +--
6 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 0923b82..681d823 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
pwrdm_set_next_pwrst(per_pd, per_next_state);
select_state:
+ omap3_device_idle();
+
dev-last_state = new_state;
ret = omap3_enter_idle(dev, new_state);
+ omap3_device_resume();
+
In the generic cpu_idle() in process.c, interrupts are
already disabled
before control comes to cpuidle_idle_call() via pm_idle()
local_irq_disable();
if (hlt_counter) {
local_irq_enable();
cpu_relax();
} else {
stop_critical_timings();
pm_idle();
start_critical_timings();
/*
* This will eventually be
removed - pm_idle
* functions should always
return with IRQs
* enabled.
*/
WARN_ON(irqs_disabled());
local_irq_enable();
}
omap3_enter_idle_bm() will be called from inside
cpuidle_idle_call()
via target_state-enter(dev, target_state).
So, interrupts are already disabled here.
Am I missing something?
You're right.
I knew this was the case for !CPUIDLE setup, but had
thought (without
testing) that the CPUidle core had re-enabled interrupts during the
governor selection process etc.
While I investigate ways to manage this in CPUidle, the
following should
be fine for now to include with $SUBJECT patch.
Kevin
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 681d823..c5cb9d0 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
goto select_state;
}
+ /*
+ * Enable IRQs during the device activity checking and
idle management.
+ * IRQs are later (re)disabled when entering the actual
idle function.
+ * Device idle management that is using runtime PM needs to have
+ * interrupts enabled when calling into the runtime PM core.
+ */
+ local_irq_enable();
After put_sync() retuns, there will be a time window where
interrupts
are enabled but clocks are disabled before the interrupts
are disabled again.
Accessing any register to service a device interrupt coming
during this window
will lead to a crash for cases where iclk and fclks are
same and we have the
iclk defined as the main_clk as well.
Same argument holds while
RE: [PATCH 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of sys_dev_class
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 10, 2010 5:51 AM
To: Varadarajan, Charulatha
Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson,
Benoit; Nayak, Rajendra; Basak, Partha
Subject: Re: [PATCH 12/13 v5] OMAP: GPIO: Use dev_pm_ops
instead of sys_dev_class
Charulatha V ch...@ti.com writes:
This patch makes GPIO driver to use dev_pm_ops instead of
sysdev_class. With this approach, gpio_bank_suspend
gpio_bank_resume
are not part of sys_dev_class.
snip
/*
* OMAP1510 GPIO registers
@@ -179,7 +179,6 @@ struct gpio_bank {
* related to all instances of the device
*/
static struct gpio_bank *gpio_bank;
-
static int bank_width;
/* TODO: Analyze removing gpio_bank_count usage from driver code */
@@ -1045,6 +1044,9 @@ static int omap_gpio_request(struct
gpio_chip *chip, unsigned offset)
struct gpio_bank *bank = container_of(chip, struct
gpio_bank, chip);
unsigned long flags;
+ if (!bank-mod_usage)
+ pm_runtime_get_sync(bank-dev);
+
Would be fine to skip the 'if' here and let runtime PM continue the
usecounting. Since we'll have trace tools that instrument runtime PM,
it will be nice to be able to trace all the users instead of just the
first one to request a GPIO in a given bank.
We are continuing to use mod_usage checks for the following reasons:
1. In the absence of mod_usage,
pm_runtime_getsync() would be called in the omap_gpio_request()once per
pin for each bank. However, a matching pm_runtime_putsync() would be
called in the CPU_Idle path only once for a given bank. This would lead to
atomic_dec_and_test(dev-power.usage_count) to return false and
the put_sync will not be effective.
2. Consider a case that a bank is not requested at all but in the CPU_Idle path
we
go-ahead and call pm_runtime_putsync() for that bank. Since usage_count is
already zero, this call makes it negative. Now, a subsequent call to
get_sync() will increment it to 0 and enable the clocks.
This leads to an error-scenario where clocks are enabled with usage_cnt = 0.
3. Ideally we should not be even attempting to fiddle with the
un-requested GPIO banks in the CPU_Idle path.
spin_lock_irqsave(bank-lock, flags);
/* Set trigger to none. You need to enable the desired
trigger with
@@ -1061,22 +1063,19 @@ static int omap_gpio_request(struct
gpio_chip *chip, unsigned offset)
__raw_writel(__raw_readl(reg) | (1 offset), reg);
}
#endif
- if (!cpu_class_is_omap1()) {
- if (!bank-mod_usage) {
- void __iomem *reg = bank-base;
- u32 ctrl;
-
- if (cpu_is_omap24xx() || cpu_is_omap34xx())
- reg += OMAP24XX_GPIO_CTRL;
- else if (cpu_is_omap44xx())
- reg += OMAP4_GPIO_CTRL;
- ctrl = __raw_readl(reg);
- /* Module is enabled, clocks are not gated */
- ctrl = 0xFFFE;
- __raw_writel(ctrl, reg);
- }
- bank-mod_usage |= 1 offset;
+ if ((!bank-mod_usage) (!cpu_class_is_omap1())) {
+ void __iomem *reg = bank-base;
+ u32 ctrl;
+ if (bank-method == METHOD_GPIO_24XX)
+ reg += OMAP24XX_GPIO_CTRL;
+ else if (bank-method == METHOD_GPIO_44XX)
+ reg += OMAP4_GPIO_CTRL;
+ ctrl = __raw_readl(reg);
+ /* Module is enabled, clocks are not gated */
+ ctrl = 0xFFFE;
+ __raw_writel(ctrl, reg);
If you get rid of the 'if (!mod_usage)' check above for the
pm_runtime_get, you could just get rid of the mod_usage flag all
together and do this section in the runtime_resume hook.
--
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 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, September 14, 2010 10:28 PM
To: Basak, Partha
Cc: linux-omap@vger.kernel.org; Varadarajan, Charulatha; Tero Kristo
Subject: Re: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
interrupts-disabled idle path
Basak, Partha p-bas...@ti.com writes:
From: Kevin Hilman khil...@ti.com
Currently, we wait until late in the idle path where interrupts are
disabled to do runtime-PM-like management for certain special-case
devices like GPIO.
As a prerequiste to moving GPIO to the new runtime PM
framework, move
this runtime-PM-like code out of the late idle path into new device
idle and resume functions that can be called before interrupts are
disabled by CPUidle and/or suspend.
In addition, move all the GPIO-specific logic into the GPIO core
instead of keeping GPIO-specific knowledge of power-states, context
saving etc. in the PM core.
Also, call the new device-idle and -resume methods from CPUidle and
static suspend path.
Signed-off-by: Kevin Hilman khil...@ti.com
---
arch/arm/mach-omap2/cpuidle34xx.c |4 ++
arch/arm/mach-omap2/pm.h |2 +
arch/arm/mach-omap2/pm24xx.c |2 +-
arch/arm/mach-omap2/pm34xx.c | 38
+
arch/arm/plat-omap/gpio.c | 57
arch/arm/plat-omap/include/plat/gpio.h |4 +--
6 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 0923b82..681d823 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
pwrdm_set_next_pwrst(per_pd, per_next_state);
select_state:
+ omap3_device_idle();
+
dev-last_state = new_state;
ret = omap3_enter_idle(dev, new_state);
+ omap3_device_resume();
+
In the generic cpu_idle() in process.c, interrupts are
already disabled
before control comes to cpuidle_idle_call() via pm_idle()
local_irq_disable();
if (hlt_counter) {
local_irq_enable();
cpu_relax();
} else {
stop_critical_timings();
pm_idle();
start_critical_timings();
/*
* This will eventually be
removed - pm_idle
* functions should always
return with IRQs
* enabled.
*/
WARN_ON(irqs_disabled());
local_irq_enable();
}
omap3_enter_idle_bm() will be called from inside
cpuidle_idle_call()
via target_state-enter(dev, target_state).
So, interrupts are already disabled here.
Am I missing something?
You're right.
I knew this was the case for !CPUIDLE setup, but had thought (without
testing) that the CPUidle core had re-enabled interrupts during the
governor selection process etc.
While I investigate ways to manage this in CPUidle, the
following should
be fine for now to include with $SUBJECT patch.
Kevin
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 681d823..c5cb9d0 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -245,6 +245,14 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
goto select_state;
}
+ /*
+ * Enable IRQs during the device activity checking and
idle management.
+ * IRQs are later (re)disabled when entering the actual
idle function.
+ * Device idle management that is using runtime PM needs to have
+ * interrupts enabled when calling into the runtime PM core.
+ */
+ local_irq_enable();
+
cx = cpuidle_get_statedata(state);
core_next_state = cx-core_state;
OK. We will base on top of this patch.
k
--
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 2/2] OMAP2+: GPIO: move late PM out of interrupts-disabled idle path
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, September 14, 2010 4:33 AM
To: linux-omap@vger.kernel.org
Cc: Varadarajan, Charulatha; Basak, Partha; Tero Kristo
Subject: [PATCH 2/2] OMAP2+: GPIO: move late PM out of
interrupts-disabled idle path
From: Kevin Hilman khil...@ti.com
Currently, we wait until late in the idle path where interrupts are
disabled to do runtime-PM-like management for certain special-case
devices like GPIO.
As a prerequiste to moving GPIO to the new runtime PM framework, move
this runtime-PM-like code out of the late idle path into new device
idle and resume functions that can be called before interrupts are
disabled by CPUidle and/or suspend.
In addition, move all the GPIO-specific logic into the GPIO core
instead of keeping GPIO-specific knowledge of power-states, context
saving etc. in the PM core.
Also, call the new device-idle and -resume methods from CPUidle and
static suspend path.
Signed-off-by: Kevin Hilman khil...@ti.com
---
arch/arm/mach-omap2/cpuidle34xx.c |4 ++
arch/arm/mach-omap2/pm.h |2 +
arch/arm/mach-omap2/pm24xx.c |2 +-
arch/arm/mach-omap2/pm34xx.c | 38 +
arch/arm/plat-omap/gpio.c | 57
arch/arm/plat-omap/include/plat/gpio.h |4 +--
6 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 0923b82..681d823 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -274,9 +274,13 @@ static int omap3_enter_idle_bm(struct
cpuidle_device *dev,
pwrdm_set_next_pwrst(per_pd, per_next_state);
select_state:
+ omap3_device_idle();
+
dev-last_state = new_state;
ret = omap3_enter_idle(dev, new_state);
+ omap3_device_resume();
+
In the generic cpu_idle() in process.c, interrupts are already disabled
before control comes to cpuidle_idle_call() via pm_idle()
local_irq_disable();
if (hlt_counter) {
local_irq_enable();
cpu_relax();
} else {
stop_critical_timings();
pm_idle();
start_critical_timings();
/*
* This will eventually be removed - pm_idle
* functions should always return with IRQs
* enabled.
*/
WARN_ON(irqs_disabled());
local_irq_enable();
}
omap3_enter_idle_bm() will be called from inside cpuidle_idle_call()
via target_state-enter(dev, target_state).
So, interrupts are already disabled here.
Am I missing something?
/* Restore original PER state if it was modified */
if (per_next_state != per_saved_state)
pwrdm_set_next_pwrst(per_pd, per_saved_state);
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 3de6ece..33cae4b 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -22,6 +22,8 @@ extern void omap_sram_idle(void);
extern int omap3_can_sleep(void);
extern int set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
extern int omap3_idle_init(void);
+extern void omap3_device_idle(void);
+extern void omap3_device_resume(void);
struct cpuidle_params {
u8 valid;
diff --git a/arch/arm/mach-omap2/pm24xx.c
b/arch/arm/mach-omap2/pm24xx.c
index 6aeedea..7bbf0db 100644
--- a/arch/arm/mach-omap2/pm24xx.c
+++ b/arch/arm/mach-omap2/pm24xx.c
@@ -106,7 +106,7 @@ static void omap2_enter_full_retention(void)
l = omap_ctrl_readl(OMAP2_CONTROL_DEVCONF0) |
OMAP24XX_USBSTANDBYCTRL;
omap_ctrl_writel(l, OMAP2_CONTROL_DEVCONF0);
- omap2_gpio_prepare_for_idle(PWRDM_POWER_RET);
+ omap2_gpio_prepare_for_idle();
if (omap2_pm_debug) {
omap2_pm_dump(0, 0, 0);
diff --git a/arch/arm/mach-omap2/pm34xx.c
b/arch/arm/mach-omap2/pm34xx.c
index bb2ba1e..9e590d9 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -74,16 +74,6 @@ static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
static struct powerdomain *core_pwrdm, *per_pwrdm;
static struct powerdomain *cam_pwrdm;
-static inline void omap3_per_save_context(void)
-{
- omap_gpio_save_context();
-}
-
-static inline void omap3_per_restore_context(void)
-{
- omap_gpio_restore_context();
-}
-
static void omap3_enable_io_chain(void)
{
int timeout = 0;
@@ -332,6 +322,16 @@ static void restore_table_entry(void)
restore_control_register(control_reg_value
RE: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform device
-Original Message-
From: Basak, Partha
Sent: Thursday, August 12, 2010 5:35 PM
To: 'Paul Walmsley'
Cc: Kevin Hilman; Varadarajan, Charulatha; linux-omap@vger.kernel.org;
Cousson, Benoit; Nayak, Rajendra
Subject: RE: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform
device
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Wednesday, August 11, 2010 11:17 AM
To: Basak, Partha
Cc: Kevin Hilman; Varadarajan, Charulatha; linux-omap@vger.kernel.org;
Cousson, Benoit; Nayak, Rajendra
Subject: RE: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform
device
On Tue, 10 Aug 2010, Basak, Partha wrote:
As per our discussion with Paul you during workshop, I believe,
optional clock control should be done using clock APIs. So, I would go
by your suggestion 1 of exposing an API to expose the optional clocks
in
the hwmod, something like:
struct omap_hwmod_opt_clk * omap_hwmod_get_opt_clks(struct omap_hwmod
*oh);
If agreed, Charu will send updated patch.
This should be done by modifying the hwmod code to call clk_add_alias()
for the clock names for the optional clocks. I don't think any extra
API
is needed.
Lets see, if I got it right:
Refer to the OMAP3 hwmod data-base (omap_hwmod_3xxx.c):
static struct omap_hwmod_opt_clk gpio1_opt_clks[] = {
{ .role = dbclk, .clk = gpio1_dbck, },
};
Clock database(Clock3xxx_data.c):
CLK(NULL, gpio1_dbck, gpio1_dbck,CK_3XXX),
Refer to the OMAP4 hwmod database(omap_hwmod_44xx.c):
static struct omap_hwmod_opt_clk gpio1_opt_clks[] = {
{ .role = dbclk, .clk = sys_32k_ck },
Clock database(Clock44xx_data.c):
CLK(NULL, sys_32k_ck, sys_32k_ck,CK_443X),
/*int clk_add_alias(const char *alias, const char *alias_dev_name, char
*id,
struct device *dev);*/
I believe, you are suggesting to do the following in the hwmod framework,
say _setup(?):
clk_add_alias ( gpio1_opt_clks.role,
oh-od.pdev.name,
gpio1_opt_clks.clk,
NULL);
Then, from the driver, we can simply do a
clk_get(dev_ptr,
role--dbclk); /*hard-coded in the driver to be same
as in the hwmod database*/
Sent patch [PATCH] OMAP: HWMOD: Handle opt clocks using clk_add_alias.
- Paul
--
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: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki Sent: Tuesday, August 17, 2010 2:48 AM To: Kevin Hilman Cc: linux...@lists.linux-foundation.org; linux-omap@vger.kernel.org; Alan Stern; Ming Lei Subject: Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled On Tuesday, August 10, 2010, Kevin Hilman wrote: When using runtime PM in combination with CPUidle, the runtime PM transtions of some devices may be triggered during the idle path. Late in the idle sequence, interrupts will likely be disabled when runtime PM for these devices is initiated. Currently, the runtime PM core assumes methods are called with interrupts enabled. However, if it is called with interrupts disabled, the internal locking unconditionally enables interrupts, for example: pm_runtime_put_sync() Please don't use that from interrupt context. There's pm_runtime_put() exactly for this purpose that puts the _idle() call into a workqueue. Rafael, we execute this little before executing idle instruction with interrupts locked. So, we cannot call pm_runtime_put() as we want it to take effect immediately. Kevin?? __pm_runtime_put() pm_runtime_idle() spin_lock_irq() __pm_runtime_idle() spin_unlock_irq() --- interrupts unconditionally enabled That's because __pm_runtime_idle() has to be called from process context. dev-bus-pm-runtime_idle() spin_lock_irq() spin_unlock_irq() Thanks, Rafael -- 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 -- 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 3/3] OMAP: hwmod: Force a softreset during _setup
-Original Message-
From: Cousson, Benoit
Sent: Wednesday, August 18, 2010 8:31 PM
To: Basak, Partha
Cc: Nayak, Rajendra; Shilimkar, Santosh; linux-omap@vger.kernel.org;
khil...@deeprootsystems.com; p...@pwsan.com
Subject: Re: [PATCH 3/3] OMAP: hwmod: Force a softreset during _setup
Hi Partha,
On 8/17/2010 2:46 PM, Basak, Partha wrote:
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Cousson, Benoit
Sent: Thursday, August 05, 2010 3:43 AM
Force the softreset of every IPs during the _setup phase.
IPs that cannot support softreset or that should not
be reset must set the HWMOD_INIT_NO_RESET flag in the
hwmod struct.
Signed-off-by: Benoit Coussonb-cous...@ti.com
Cc: Paul Walmsleyp...@pwsan.com
Cc: Kevin Hilmankhil...@deeprootsystems.com
---
arch/arm/mach-omap2/omap_hwmod.c | 17 -
1 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
omap2/omap_hwmod.c
index 53b08e3..91ad9c6 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -856,8 +856,8 @@ static int _reset(struct omap_hwmod *oh)
/* clocks must be on for this operation */
if (oh-_state != _HWMOD_STATE_ENABLED) {
- WARN(1, omap_hwmod: %s: reset can only be entered from
- enabled state\n, oh-name);
+ pr_warning(omap_hwmod: %s: reset can only be entered from
+ enabled state\n, oh-name);
return -EINVAL;
}
@@ -874,8 +874,8 @@ static int _reset(struct omap_hwmod *oh)
MAX_MODULE_RESET_WAIT, c);
if (c == MAX_MODULE_RESET_WAIT)
- WARN(1, omap_hwmod: %s: failed to reset in %d usec\n,
- oh-name, MAX_MODULE_RESET_WAIT);
+ pr_warning(omap_hwmod: %s: failed to reset in %d usec\n,
+ oh-name, MAX_MODULE_RESET_WAIT);
http://focus.ti.com/pdfs/wtbu/SWPU177B_FinalEPDF_12_04_2009.pdf
This is leading to the above warning for DSS on OMAP3430/3630. Referring
to the reference manual (7.5.1 Display Subsystem Reset), successful reset
of DSS would need PRCM.CM_FCLKEN_DSS[2] EN_TV bit set to 1. For DSS, even
though TV clock is an optional clock, this is mandatory for successful DSS
reset. We could ignore this warning, or possibly WA it.
One way could be:
1. In the database, have HWMOD_INIT_NO_RESET flag set so that _setup()
skips reset.
2. After omap device build of DSS is done, lookup the opt-clock and
enable it (using clock framework).
4. Then do DSS reset again calling omap_device_reset().
All IPs that potentially have any special soft-reset sequence will need
customized handling. Should we keep the framework light and handle such
special cases in the drivers? In that case, the framework need not throw
up a warning. Please comment.
If the softreset is not mandatory for an IP like DSS, you just have to
set this HWMOD_INIT_NO_RESET flag.
There is no need to use the softreset for every IP, the PRCM already
resets every IPs each time the power domain is powered on.
softreset is useful if you need to recover from an HW error.
I agree, however without doing soft-reset, we have observed DSI malfunction.
Senthil can provide more details. As DSS needs TV_clk for successful reset, I
doubt whether PRCM can reset DSS once we merely turn on the DSS power domain.
So, it really depends on the IP in question. If it is necessary, we will do a
omap_device_reset()in the driver. Correct?
Regards,
Benoit
--
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 3/3] OMAP: hwmod: Force a softreset during _setup
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Cousson, Benoit
Sent: Thursday, August 05, 2010 3:43 AM
To: linux-omap@vger.kernel.org; khil...@deeprootsystems.com;
p...@pwsan.com
Cc: Nayak, Rajendra; Shilimkar, Santosh; Cousson, Benoit
Subject: [PATCH 3/3] OMAP: hwmod: Force a softreset during _setup
Force the softreset of every IPs during the _setup phase.
IPs that cannot support softreset or that should not
be reset must set the HWMOD_INIT_NO_RESET flag in the
hwmod struct.
Signed-off-by: Benoit Cousson b-cous...@ti.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Kevin Hilman khil...@deeprootsystems.com
---
arch/arm/mach-omap2/omap_hwmod.c | 17 -
1 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
omap2/omap_hwmod.c
index 53b08e3..91ad9c6 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -856,8 +856,8 @@ static int _reset(struct omap_hwmod *oh)
/* clocks must be on for this operation */
if (oh-_state != _HWMOD_STATE_ENABLED) {
- WARN(1, omap_hwmod: %s: reset can only be entered from
- enabled state\n, oh-name);
+ pr_warning(omap_hwmod: %s: reset can only be entered from
+enabled state\n, oh-name);
return -EINVAL;
}
@@ -874,8 +874,8 @@ static int _reset(struct omap_hwmod *oh)
MAX_MODULE_RESET_WAIT, c);
if (c == MAX_MODULE_RESET_WAIT)
- WARN(1, omap_hwmod: %s: failed to reset in %d usec\n,
- oh-name, MAX_MODULE_RESET_WAIT);
+ pr_warning(omap_hwmod: %s: failed to reset in %d usec\n,
+oh-name, MAX_MODULE_RESET_WAIT);
http://focus.ti.com/pdfs/wtbu/SWPU177B_FinalEPDF_12_04_2009.pdf
This is leading to the above warning for DSS on OMAP3430/3630. Referring to the
reference manual (7.5.1 Display Subsystem Reset), successful reset of DSS would
need PRCM.CM_FCLKEN_DSS[2] EN_TV bit set to 1. For DSS, even though TV clock is
an optional clock, this is mandatory for successful DSS reset. We could ignore
this warning, or possibly WA it.
One way could be:
1. In the database, have HWMOD_INIT_NO_RESET flag set so that _setup() skips
reset.
2. After omap device build of DSS is done, lookup the opt-clock and enable it
(using clock framework).
3. Then do DSS reset again calling omap_device_reset().
All IPs that potentially have any special soft-reset sequence will need
customized handling. Should we keep the framework light and handle such special
cases in the drivers? In that case, the framework need not throw up a warning.
Please comment.
else
pr_debug(omap_hwmod: %s: reset in %d usec\n, oh-name, c);
@@ -1074,12 +1074,11 @@ static int _setup(struct omap_hwmod *oh, void
*data)
}
if (!(oh-flags HWMOD_INIT_NO_RESET)) {
+ _reset(oh);
/*
- * XXX Do the OCP_SYSCONFIG bits need to be
- * reprogrammed after a reset? If not, then this can
- * be removed. If they do, then probably the
- * _omap_hwmod_enable() function should be split to avoid the
- * rewrite of the OCP_SYSCONFIG register.
+ * OCP_SYSCONFIG bits need to be reprogrammed after a
softreset.
+ * The _omap_hwmod_enable() function should be split to
+ * avoid the rewrite of the OCP_SYSCONFIG register.
*/
if (oh-class-sysc) {
_update_sysc_cache(oh);
--
1.6.1.3
--
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
--
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 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of sys_dev_class
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 10, 2010 11:40 PM
To: Basak, Partha
Cc: Varadarajan, Charulatha; linux-omap@vger.kernel.org; p...@pwsan.com;
Cousson, Benoit; Nayak, Rajendra
Subject: Re: [PATCH 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of
sys_dev_class
Basak, Partha p-bas...@ti.com writes:
[...]
In the idle path (interrupt disabled context), PM runtime APIs cannot
be used as they are not mutex-free functions. Hence omap_device APIs
are used in the idle and resume after idle path.
This needs much more fleshing out.
Exactly what mutexes are causing the problems here. As pointed out in
previous discussions, the ones in the PM runtime core should not be a
problem in this path. Therefore, I'll assume the problems are coming
from the mutexes when the device code (mach-omap2/gpio.c) calls into
the
hwmod layer. More on this in comments on the next patch.
Sorry, this has not been documented correctly. The issue has more to
do unconditional enabling of interrupts. We have received a patch from
you on using pm_runtime functions in Idle path. We will try on GPIO
and revert back.
OK
To summarize,
1. pm_runtime_get_sync() for any gpio bank is called when one of the
gpios
is requested on the bank, in which, no other gpio is being used
(when
mod_usage becomes non-zero)
2. omap_device_enable() is called during gpio resume after idle, only
if the particular bank is being used (if mod_usage is non-zero)
context is saved/restored in the idle path, but...
3. pm_runtime_put_sync() is called when the last used gpio in that
gpio bank is freed (when mod_usage becomes zero)
in this path, the bank is now runtime suspended, but context has not
been saved for it. That should be fine, since this bank is no longer
used, but now let's assume there was an off-mode transition and context
is lost. Then, gpio_request() is called which will trigger
a pm_runtime_get_sync() and gpio_bank_runtime_resume() will be called.
In this case, it's not terribly clear that runtime_resume is doing sane
things if context has just been lost. Seems like runtime_resume should
be a nop in this case since any re-init will be be done in
gpio_request().
Runtime_suspend/resume for GPIO is not doing any save/restore
context. In that sense, they are NOP. Context save/restore is taken
care of only in the Idle path based on target power state and last
power state respectively.
OK, I didn't explain the problem I'm suspecting very well. Imagine this
sequence of events:
- mod_usage becomes zero
- pm_runtime_put_sync()
- gpio_bank_runtime_suspend() [ no context is saved ]
[ off-mode transition, context is lost]
- gpio_request()
- pm_runtime_get_sync()
- gpio_bank_runtime_resume()
In this path, no context is saved, and no context is restored, which is
what I would expect, since there's no need to save context if nobody is
using that gpio bank anymore. However, gpio_bank_runtime_resume() is
doing lots of reads/writes and read-modify-writes on GPIO bank registers
that may have undefined contents after a context loss.
The point is that the GPIO register twiddling in
gpio_bank_runtime_resume() does not seem to be needed if there are no
users of that GPIO bank.
[...]
static void omap3_enable_io_chain(void)
{
int timeout = 0;
@@ -395,15 +385,17 @@ void omap_sram_idle(void)
/* PER */
if (per_next_state PWRDM_POWER_ON) {
omap_uart_prepare_idle(2);
-omap2_gpio_prepare_for_idle(per_next_state);
if (per_next_state == PWRDM_POWER_OFF) {
if (core_next_state == PWRDM_POWER_ON) {
per_next_state = PWRDM_POWER_RET;
pwrdm_set_next_pwrst(per_pwrdm,
per_next_state);
per_state_modified = 1;
-} else
-omap3_per_save_context();
+}
}
+if (per_next_state == PWRDM_POWER_OFF)
+omap2_gpio_prepare_for_idle(true);
+else
+omap2_gpio_prepare_for_idle(false);
Why is this better than passing the next power state?
This would keep the GPIO function omap2_gpio_prepare_for_idle agnostic
of Power state definition dependencies.
And why is this better?
Personally, I think the GPIO code should be told about the powerdomain
state so it can make it's own decision about whether or not to save
context.
I see your point.
But, in the approach we have followed so far, we are trying to localize all
Power domain related logic and information in pm_34xxx.c. If we refer to all
other such functions like
RE: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform device
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Wednesday, August 11, 2010 11:17 AM
To: Basak, Partha
Cc: Kevin Hilman; Varadarajan, Charulatha; linux-omap@vger.kernel.org;
Cousson, Benoit; Nayak, Rajendra
Subject: RE: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform
device
On Tue, 10 Aug 2010, Basak, Partha wrote:
As per our discussion with Paul you during workshop, I believe,
optional clock control should be done using clock APIs. So, I would go
by your suggestion 1 of exposing an API to expose the optional clocks in
the hwmod, something like:
struct omap_hwmod_opt_clk * omap_hwmod_get_opt_clks(struct omap_hwmod
*oh);
If agreed, Charu will send updated patch.
This should be done by modifying the hwmod code to call clk_add_alias()
for the clock names for the optional clocks. I don't think any extra API
is needed.
Lets see, if I got it right:
Refer to the OMAP3 hwmod data-base (omap_hwmod_3xxx.c):
static struct omap_hwmod_opt_clk gpio1_opt_clks[] = {
{ .role = dbclk, .clk = gpio1_dbck, },
};
Clock database(Clock3xxx_data.c):
CLK(NULL, gpio1_dbck, gpio1_dbck,CK_3XXX),
Refer to the OMAP4 hwmod database(omap_hwmod_44xx.c):
static struct omap_hwmod_opt_clk gpio1_opt_clks[] = {
{ .role = dbclk, .clk = sys_32k_ck },
Clock database(Clock44xx_data.c):
CLK(NULL, sys_32k_ck, sys_32k_ck,CK_443X),
/*int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
struct device *dev);*/
I believe, you are suggesting to do the following in the hwmod framework, say
_setup(?):
clk_add_alias ( gpio1_opt_clks.role,
oh-od.pdev.name,
gpio1_opt_clks.clk,
NULL);
Then, from the driver, we can simply do a
clk_get(dev_ptr,
role--dbclk); /*hard-coded in the driver to be
sameas in the hwmod database*/
- Paul
--
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 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of sys_dev_class
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Tuesday, August 10, 2010 5:51 AM To: Varadarajan, Charulatha Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Nayak, Rajendra; Basak, Partha Subject: Re: [PATCH 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of sys_dev_class Charulatha V ch...@ti.com writes: This patch makes GPIO driver to use dev_pm_ops instead of sysdev_class. With this approach, gpio_bank_suspend gpio_bank_resume are not part of sys_dev_class. According to this patch, a GPIO bank relinquishes the clock using PM runtime APIs when all the gpios in that bank are freed. This does not match the code. The only clock associated with a GPIO hwmod is the optional clock for the debounce clock. This clock is managed by the driver itself, not by using the PM runtime API. It also relinquishes the clocks in the idle-path too, as it is possible to have a GPIO bank requested and still allow PER domain to go to OFF state. This doesn't make sense to me. What clocks are you referring to? The main clock is there for OMAP24xx, but not relevant for OMAP3 4. Are you aligned? In the idle path (interrupt disabled context), PM runtime APIs cannot be used as they are not mutex-free functions. Hence omap_device APIs are used in the idle and resume after idle path. This needs much more fleshing out. Exactly what mutexes are causing the problems here. As pointed out in previous discussions, the ones in the PM runtime core should not be a problem in this path. Therefore, I'll assume the problems are coming from the mutexes when the device code (mach-omap2/gpio.c) calls into the hwmod layer. More on this in comments on the next patch. Sorry, this has not been documented correctly. The issue has more to do unconditional enabling of interrupts. We have received a patch from you on using pm_runtime functions in Idle path. We will try on GPIO and revert back. To summarize, 1. pm_runtime_get_sync() for any gpio bank is called when one of the gpios is requested on the bank, in which, no other gpio is being used (when mod_usage becomes non-zero) 2. omap_device_enable() is called during gpio resume after idle, only if the particular bank is being used (if mod_usage is non-zero) context is saved/restored in the idle path, but... 3. pm_runtime_put_sync() is called when the last used gpio in that gpio bank is freed (when mod_usage becomes zero) in this path, the bank is now runtime suspended, but context has not been saved for it. That should be fine, since this bank is no longer used, but now let's assume there was an off-mode transition and context is lost. Then, gpio_request() is called which will trigger a pm_runtime_get_sync() and gpio_bank_runtime_resume() will be called. In this case, it's not terribly clear that runtime_resume is doing sane things if context has just been lost. Seems like runtime_resume should be a nop in this case since any re-init will be be done in gpio_request(). Runtime_suspend/resume for GPIO is not doing any save/restore context. In that sense, they are NOP. Context save/restore is taken care of only in the Idle path based on target power state and last power state respectively. OK, I didn't explain the problem I'm suspecting very well. Imagine this sequence of events: - mod_usage becomes zero - pm_runtime_put_sync() - gpio_bank_runtime_suspend() [ no context is saved ] [ off-mode transition, context is lost] - gpio_request() - pm_runtime_get_sync() - gpio_bank_runtime_resume() In this path, no context is saved, and no context is restored, which is what I would expect, since there's no need to save context if nobody is using that gpio bank anymore. However, gpio_bank_runtime_resume() is doing lots of reads/writes and read-modify-writes on GPIO bank registers that may have undefined contents after a context loss. Agreed. This can be resolved by saving the Init configurations when a GPIO bank is released restoring the same during GPIO bank request. The point is that the GPIO register twiddling in gpio_bank_runtime_resume() does not seem to be needed if there are no users of that GPIO bank. Can you elaborate more? -- 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 01/13 v5] OMAP: GPIO: Modify init() in preparation for platform device implementation
-Original Message-
From: Varadarajan, Charulatha
Sent: Tuesday, August 10, 2010 10:49 AM
To: Kevin Hilman
Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Nayak,
Rajendra; Basak, Partha
Subject: RE: [PATCH 01/13 v5] OMAP: GPIO: Modify init() in preparation for
platform device implementation
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 10, 2010 3:51 AM
To: Varadarajan, Charulatha
Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Nayak,
Rajendra; Basak, Partha
Subject: Re: [PATCH 01/13 v5] OMAP: GPIO: Modify init() in preparation
for
platform device implementation
Charulatha V ch...@ti.com writes:
This is in prepartion for implementing GPIO as a platform device.
gpio bank's base addresses are moved from gpio.c to plat/gpio.h.
This patch also modifies omap_gpio_init() to make use of
omap_gpio_chip_init() and omap_gpio_mod_init(). omap_gpio_mod_init()
does
the module init by clearing the status register and initializing the
GPIO control register. omap_gpio_chip_init() initializes the chip
request,
free, get, set and other function pointers and sets the gpio irq
handler.
Signed-off-by: Charulatha V ch...@ti.com
Signed-off-by: Basak, Partha p-bas...@ti.com
[...]
+static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
+{
+ if (cpu_class_is_omap2()) {
+ if (cpu_is_omap44xx()) {
+ __raw_writel(0x, bank-base +
+ OMAP4_GPIO_IRQSTATUSCLR0);
+ __raw_writel(0x, bank-base +
+ OMAP4_GPIO_DEBOUNCENABLE);
+ /* Initialize interface clk ungated, module enabled */
+ __raw_writel(0, bank-base + OMAP4_GPIO_CTRL);
+ } else if (cpu_is_omap34xx()) {
+ __raw_writel(0x, bank-base +
+ OMAP24XX_GPIO_IRQENABLE1);
+ __raw_writel(0x, bank-base +
+ OMAP24XX_GPIO_IRQSTATUS1);
+ __raw_writel(0x, bank-base +
+ OMAP24XX_GPIO_DEBOUNCE_EN);
+
+ /* Initialize interface clk ungated, module enabled */
+ __raw_writel(0, bank-base + OMAP24XX_GPIO_CTRL);
+ /* Enable autoidle for the OCP interface */
+ omap_writel(1 0, 0x48306814);
This autoidle stuff should be removed in this series as setting this is
handled by the hwmod layer.
Okay.
This code is incorrectly setting the PRCM_SYSCONFIG(0x48306814) register inside
GPIO module which is incorrect. Ideally it should be moved to generic code like
prcm_setup_regs() inside PM44xx.c/PM34xx.c. Having said that, the reset value
of PRCM_SYSCONFIG is 0x01, so it would be safe just to remove this.
Now, coming to setting of AutoIdle (in CM_AUTOIDLE_XXX registers), even though
prcm_reg_ids are being populated, hwmod framework is not setting these
anywhere, all CM_AutoIdle settings are being done one-time in side
prcm_setup_regs().
Kevin, as you pointed out this needs to be done in the framework. Can we do it
as part of enabling the slave clocks? How does the following look?
static int _enable_clocks(struct omap_hwmod *oh)
{
int i;
pr_debug(omap_hwmod: %s: enabling clocks\n, oh-name);
if (oh-_clk)
clk_enable(oh-_clk);
if (oh-slaves_cnt 0) {
for (i = 0; i oh-slaves_cnt; i++) {
struct omap_hwmod_ocp_if *os = oh-slaves[i];
struct clk *c = os-_clk;
if (c (os-flags OCPIF_SWSUP_IDLE))
clk_enable(c);
else
/*TODO: Set CM_AutoIdle here*/
}
}
/* The opt clocks are controlled by the device driver. */
return 0;
}
+ } else if (cpu_is_omap24xx()) {
+ static const u32 non_wakeup_gpios[] = {
+ 0xe203ffc0, 0x08700040
+ };
+ if (id ARRAY_SIZE(non_wakeup_gpios))
+ bank-non_wakeup_gpios = non_wakeup_gpios[id];
+
+ /* Enable autoidle for the OCP interface */
+ omap_writel(1 0, 0x48019010);
ditto
Okay.
+ }
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 01/13 v5] OMAP: GPIO: Modify init() in preparation for platform device implementation
-Original Message-
From: Cousson, Benoit
Sent: Tuesday, August 10, 2010 4:15 PM
To: Basak, Partha
Cc: Varadarajan, Charulatha; Kevin Hilman; linux-omap@vger.kernel.org;
p...@pwsan.com; Nayak, Rajendra; Syed Mohammed, Khasim
Subject: Re: [PATCH 01/13 v5] OMAP: GPIO: Modify init() in preparation for
platform device implementation
On 8/10/2010 9:20 AM, Basak, Partha wrote:
From: Varadarajan, Charulatha
Sent: Tuesday, August 10, 2010 10:49 AM
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 10, 2010 3:51 AM
Charulatha Vch...@ti.com writes:
This is in prepartion for implementing GPIO as a platform device.
gpio bank's base addresses are moved from gpio.c to plat/gpio.h.
This patch also modifies omap_gpio_init() to make use of
omap_gpio_chip_init() and omap_gpio_mod_init(). omap_gpio_mod_init()
does
the module init by clearing the status register and initializing the
GPIO control register. omap_gpio_chip_init() initializes the chip
request,
free, get, set and other function pointers and sets the gpio irq
handler.
Signed-off-by: Charulatha Vch...@ti.com
Signed-off-by: Basak, Parthap-bas...@ti.com
[...]
+static void omap_gpio_mod_init(struct gpio_bank *bank, int id)
+{
+if (cpu_class_is_omap2()) {
+if (cpu_is_omap44xx()) {
+__raw_writel(0x, bank-base +
+OMAP4_GPIO_IRQSTATUSCLR0);
+__raw_writel(0x, bank-base +
+ OMAP4_GPIO_DEBOUNCENABLE);
+/* Initialize interface clk ungated, module
enabled */
+__raw_writel(0, bank-base + OMAP4_GPIO_CTRL);
+} else if (cpu_is_omap34xx()) {
+__raw_writel(0x, bank-base +
+OMAP24XX_GPIO_IRQENABLE1);
+__raw_writel(0x, bank-base +
+OMAP24XX_GPIO_IRQSTATUS1);
+__raw_writel(0x, bank-base +
+OMAP24XX_GPIO_DEBOUNCE_EN);
+
+/* Initialize interface clk ungated, module
enabled */
+__raw_writel(0, bank-base +
OMAP24XX_GPIO_CTRL);
+/* Enable autoidle for the OCP interface */
+omap_writel(1 0, 0x48306814);
This autoidle stuff should be removed in this series as setting this
is
handled by the hwmod layer.
Okay.
This code is incorrectly setting the PRCM_SYSCONFIG(0x48306814) register
inside GPIO module which is incorrect. Ideally it should be moved to
generic code like prcm_setup_regs() inside PM44xx.c/PM34xx.c. Having said
that, the reset value of PRCM_SYSCONFIG is 0x01, so it would be safe just
to remove this.
That's weird, do you know where it come from? Maybe it is re-enable
because someone disable it at some point?
It is indeed a dirty hack, but it will be good to understand the
rational, if any?
The code is from that commit: 5492fb1a ARM: OMAP: Add 3430 gpio support
from Khasim Syed Mohammed (Added in Cc).
It seems to be a bad copy paste of the 2420 code (omap_writel(1 0,
0x48019010)). That one is indeed changing the GPIO SYSCONFIG.
Now, coming to setting of AutoIdle (in CM_AUTOIDLE_XXX registers), even
though prcm_reg_ids are being populated, hwmod framework is not setting
these anywhere, all CM_AutoIdle settings are being done one-time in side
prcm_setup_regs().
In this case, Kevin was referring to the SYSCONFIG autoidle setting not
the PRCM one. But the following point is still valid.
Kevin, as you pointed out this needs to be done in the framework.
Yep, good point, it was indeed already suggested by the comment:
/*
* Enable interface clock autoidle for all modules.
* Note that in the long run this should be done by clockfw
*/
Except that doing that in hwmod make more sense now. hwmod probably
didn't exist at that time.
Everything is in place in the hwmod prcm struct to set this setting from
the hwmod core code.
Can we do it as part of enabling the slave clocks? How does the
following look?
static int _enable_clocks(struct omap_hwmod *oh)
{
int i;
pr_debug(omap_hwmod: %s: enabling clocks\n, oh-name);
if (oh-_clk)
clk_enable(oh-_clk);
if (oh-slaves_cnt 0) {
for (i = 0; i oh-slaves_cnt; i++) {
struct omap_hwmod_ocp_if *os = oh-slaves[i];
struct clk *c = os-_clk;
if (c (os-flags OCPIF_SWSUP_IDLE))
clk_enable(c);
else
/*TODO: Set CM_AutoIdle here*/
}
}
/* The opt clocks
RE: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform device
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 10, 2010 4:36 AM
To: Varadarajan, Charulatha
Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Nayak,
Rajendra; Basak, Partha
Subject: Re: [PATCH 10/13 v5] OMAP: GPIO: Implement GPIO as a platform
device
Charulatha V ch...@ti.com writes:
@@ -650,21 +511,28 @@ static void _set_gpio_debounce(struct gpio_bank
*bank, unsigned gpio,
__raw_writel(debounce, reg);
reg = bank-base;
- if (cpu_is_omap44xx())
+ if (bank-method == METHOD_GPIO_44XX)
reg += OMAP4_GPIO_DEBOUNCENABLE;
else
reg += OMAP24XX_GPIO_DEBOUNCE_EN;
val = __raw_readl(reg);
+ if (!bank-dbck) {
+ struct platform_device *pdev = to_platform_device(bank-dev);
+ struct omap_device *odev = to_omap_device(pdev);
insert empty line
+ if (odev-hwmods[0]-opt_clks-_clk)
+ bank-dbck = odev-hwmods[0]-opt_clks-_clk;
This is not correct always as opt_clks points to an array of optional clocks
which can have more than one element.
The correct approach would be to scan through the list of optional clocks and
pick up the one with role === dbclk.
As was discussed in Bangalore, drivers should have no knowledge of hwmod
internals.
Instead, please propose an API to hwmod for getting the optional
clock(s), or possibly cleaner, an API to directly enable/disable
optional clocks for an omap_device.
As per our discussion with Paul you during workshop, I believe, optional
clock control should be done using clock APIs. So, I would go by your
suggestion 1 of exposing an API to expose the optional clocks in the hwmod,
something like:
struct omap_hwmod_opt_clk * omap_hwmod_get_opt_clks(struct omap_hwmod
*oh);
If agreed, Charu will send updated patch.
Kevin
+ if (IS_ERR(bank-dbck))
+ dev_err(bank-dev, Could not get gpio dbck\n);
+ }
+
--
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 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of sys_dev_class
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Tuesday, August 10, 2010 5:51 AM To: Varadarajan, Charulatha Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Nayak, Rajendra; Basak, Partha Subject: Re: [PATCH 12/13 v5] OMAP: GPIO: Use dev_pm_ops instead of sys_dev_class Charulatha V ch...@ti.com writes: This patch makes GPIO driver to use dev_pm_ops instead of sysdev_class. With this approach, gpio_bank_suspend gpio_bank_resume are not part of sys_dev_class. According to this patch, a GPIO bank relinquishes the clock using PM runtime APIs when all the gpios in that bank are freed. This does not match the code. The only clock associated with a GPIO hwmod is the optional clock for the debounce clock. This clock is managed by the driver itself, not by using the PM runtime API. It also relinquishes the clocks in the idle-path too, as it is possible to have a GPIO bank requested and still allow PER domain to go to OFF state. This doesn't make sense to me. What clocks are you referring to? The main clock is there for OMAP24xx, but not relevant for OMAP3 4. In the idle path (interrupt disabled context), PM runtime APIs cannot be used as they are not mutex-free functions. Hence omap_device APIs are used in the idle and resume after idle path. This needs much more fleshing out. Exactly what mutexes are causing the problems here. As pointed out in previous discussions, the ones in the PM runtime core should not be a problem in this path. Therefore, I'll assume the problems are coming from the mutexes when the device code (mach-omap2/gpio.c) calls into the hwmod layer. More on this in comments on the next patch. Sorry, this has not been documented correctly. The issue has more to do unconditional enabling of interrupts. We have received a patch from you on using pm_runtime functions in Idle path. We will try on GPIO and revert back. To summarize, 1. pm_runtime_get_sync() for any gpio bank is called when one of the gpios is requested on the bank, in which, no other gpio is being used (when mod_usage becomes non-zero) 2. omap_device_enable() is called during gpio resume after idle, only if the particular bank is being used (if mod_usage is non-zero) context is saved/restored in the idle path, but... 3. pm_runtime_put_sync() is called when the last used gpio in that gpio bank is freed (when mod_usage becomes zero) in this path, the bank is now runtime suspended, but context has not been saved for it. That should be fine, since this bank is no longer used, but now let's assume there was an off-mode transition and context is lost. Then, gpio_request() is called which will trigger a pm_runtime_get_sync() and gpio_bank_runtime_resume() will be called. In this case, it's not terribly clear that runtime_resume is doing sane things if context has just been lost. Seems like runtime_resume should be a nop in this case since any re-init will be be done in gpio_request(). Runtime_suspend/resume for GPIO is not doing any save/restore context. In that sense, they are NOP. Context save/restore is taken care of only in the Idle path based on target power state and last power state respectively. 4. omap_device_idle() is called during idle, if the particular bank is being used (if mod_usage is non-zero) This mixture of pm_runtime_* and omap_device_* APIs is confusing at best. There should be a single path into the drivers runtime_suspend hooks. Namely, when pm_runtime_put_* is called and the usecount goes to zero. If you can't use the runtime PM APIs, then we need to understand *exactly* why and work on a solution for that particular problem. On my omap34xx/omap3evm, I had to disable the omap_device_* calls in the idle path since as they were causing strange crashes, and as stated above, I'm not sure what the purpose is. With this patch, GPIO's prepare_for_idle and resume_after_idle APIs makes use of the parameter save_context and restore_context respectively inorder to identify if save context/restore context needs to be done. Why? Links to related discussion: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg32833.html For suspend/resume path to work, this patch has dependency of 1. reverting the following patch: OMAP: bus-level PM: enable use of runtime PM API for suspend/resume http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=commitdiff; h=8041359e18e49bf8a3d41f15894db9e476f3a8fc (or) 2. Remove the locking in the omap_hwmod_for_each* function Did you mean 'and' instead of 'or'? If you meant 'or', then clearly (20 is preferred over (1), and I have a patch to fix that in the current pm-wip/runtime branch. If you meant 'and', then you need to describe the root cause for (1). Signed-off-by: Charulatha V ch...@ti.com Signed-off-by: Basak, Partha p-bas...@ti.com
RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Monday, August 09, 2010 9:01 PM
To: Basak, Partha
Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
Govindraj; Varadarajan, Charulatha
Subject: Re: Issues with calling pm_runtime functions in
platform_pm_suspend_noirq/IRQ disabled context.
Basak, Partha p-bas...@ti.com writes:
Finally, we have omap_sram_idle():
In particular, for USB, while executing SRAM_Idle, is we use
pm_runtime
functions, we see spurious IO-Pad interrupts.
Your message doesn't specify what PM runtime functions you are
executing.
But if those functions are calling mutex_lock(), then they obviously
must
not be called from omap_sram_idle() in interrupt context.
It's not clear from your message why you need to call PM runtime
functions
from the idle loop. I'd suggest that you post the problematic code in
question to the list.
USB issue:
Please refer to the USB patch attached (will be sent to the list as well
in a few minutes)
As I mentioned previously, few drivers like GPIO, USB UART have clock-
disable/enable + context save/restore in Idle path(omap_sram_idle()).
In particular, I am referring to calling of the functions
pm_runtime_put_sync() pm_runtime_get_sync() for USB around wfi.
Now, the following call sequence from omap_sram_idle()will enable IRQs:
pm_runtime_put_sync--
__pm_runtime_put--
pm_runtime_idle--
spin_unlock_irq(),
__pm_runtime_idle(),--
spin_unlock_irq,
spin_unlock_irq
The functions pm_runtime_idle() __ pm_runtime_idle() do not use
spin_lock_irqsave spin_unlock_irqrestore. This leads to enabling of
the interrupts in omap_sram_idle unconditionally, which for USB in
particular is leading to IO-pad interrupt being triggered which does
not come otherwise.
You seem to have correctly identified the problem. Can you try the
patch below (untested) which attempts to address the root cause you've
identified?
Kevin
To work around this problem, instead of using Runtime APIs, we are using
omap_device_enable/disable, which in-turn call to __omap_hwmod_enable/idle
Simply put, I am not talking about grabbing a mutex when interrupts are
disabled, rather of a scenario where interrupts are getting enabled as a
side-effect of calling these functions in omap_sram_idle().
From be3aeea5f8d29c8ce2fa278f48aef849eb682282 Mon Sep 17 00:00:00 2001
From: Kevin Hilman khil...@deeprootsystems.com
Date: Mon, 9 Aug 2010 08:12:39 -0700
Subject: [PATCH] PM: allow runtime PM get/put from interrupts-disabled
context
When using runtime PM in combination with CPUidle, the runtime PM
transtions of some devices may be triggered during the idle path.
Late in the idle sequence, interrupts may already be disabled when
runtime PM for these devices is initiated (using the _sync versions of
the runtime PM API.)
Currently, the runtime PM core assumes methods are called with
interrupts enabled. However, if it is called with interrupts
disabled, the internal locking unconditionally enables interrupts, for
example:
pm_runtime_put_sync()
__pm_runtime_put()
pm_runtime_idle()
spin_lock_irq()
__pm_runtime_idle()
spin_unlock_irq() --- interrupts unconditionally
enabled
dev-bus-pm-runtime_idle()
spin_lock_irq()
spin_unlock_irq()
To fix, use the save/restore versions of the spinlock API.
Reported-by: Partha Basak p-bas...@ti.com
Signed-off-by: Kevin Hilman khil...@deeprootsystems.com
---
drivers/base/power/runtime.c | 68 ++---
include/linux/pm.h |1 +
2 files changed, 37 insertions(+), 32 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b0ec0e9..b02ef35 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -80,24 +80,24 @@ static int __pm_runtime_idle(struct device *dev)
dev-power.idle_notification = true;
if (dev-bus dev-bus-pm dev-bus-pm-runtime_idle) {
- spin_unlock_irq(dev-power.lock);
+ spin_unlock_irqrestore(dev-power.lock, dev-
power.lock_flags);
dev-bus-pm-runtime_idle(dev);
- spin_lock_irq(dev-power.lock);
+ spin_lock_irqsave(dev-power.lock, dev-power.lock_flags);
} else if (dev-type dev-type-pm dev-type-pm-
runtime_idle) {
- spin_unlock_irq(dev-power.lock);
+ spin_unlock_irqrestore(dev-power.lock, dev-
power.lock_flags);
dev-type-pm-runtime_idle(dev);
- spin_lock_irq(dev-power.lock);
+ spin_lock_irqsave(dev-power.lock, dev-power.lock_flags);
} else if (dev-class dev-class-pm
dev
RE: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Que, Simon Sent: Wednesday, July 07, 2010 1:55 AM To: linux-omap@vger.kernel.org Cc: Kanigeri, Hari; Ohad Ben-Cohen; Shilimkar, Santosh Subject: [RFC v.4] omap: hwspinlock: Added hwspinlock driver Hello, This is the fourth and final RFC for the hardware spinlock driver. I have incorporated some changes and fixes based on Santosh's comments. Please give your comments. Thanks, Simon = From d4794eff60e5e509581fedaf2660b0d2d92463ab Mon Sep 17 00:00:00 2001 From: Simon Que s...@ti.com Date: Wed, 23 Jun 2010 18:40:30 -0500 Subject: [PATCH] omap: hwspinlock: Added hwspinlock driver Created driver for OMAP hardware spinlock. This driver supports: - Reserved spinlocks for internal use - Dynamic allocation of unreserved locks - Lock, unlock, and trylock functions, with or without disabling irqs/preempt - Registered as a platform device driver The device initialization uses hwmod to configure the devices. One device will be created for each hardware spinlock. It will pass spinlock register addresses to the driver. The device initialization file is: arch/arm/mach-omap2/hwspinlocks.c The driver takes in data passed in device initialization. The function hwspinlock_probe() initializes the array of spinlock structures, each containing a spinlock register address provided by the device initialization. The device driver file is: arch/arm/plat-omap/hwspinlock.c Here's an API summary: int hwspinlock_lock(struct hwspinlock *); Attempt to lock a hardware spinlock. If it is busy, the function will keep trying until it succeeds. This is a blocking function. int hwspinlock_trylock(struct hwspinlock *); Attempt to lock a hardware spinlock. If it is busy, the function will return BUSY. If it succeeds in locking, the function will return ACQUIRED. This is a non-blocking function int hwspinlock_unlock(struct hwspinlock *); Unlock a hardware spinlock. struct hwspinlock *hwspinlock_request(void); Provides for dynamic allocation of a hardware spinlock. It returns the handle to the next available (unallocated) spinlock. If no more locks are available, it returns NULL. struct hwspinlock *hwspinlock_request_specific(unsigned int); Provides for static allocation of a specific hardware spinlock. This allows the system to use a specific spinlock, identified by an ID. If the ID is invalid or if the desired lock is already allocated, this will return NULL. Otherwise it returns a spinlock handle. int hwspinlock_free(struct hwspinlock *); Frees an allocated hardware spinlock (either reserved or unreserved). Signed-off-by: Simon Que s...@ti.com --- arch/arm/mach-omap2/Makefile |2 + arch/arm/mach-omap2/hwspinlocks.c| 71 ++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c |2 +- arch/arm/plat-omap/Makefile |3 +- arch/arm/plat-omap/hwspinlock.c | 295 ++ arch/arm/plat-omap/include/plat/hwspinlock.h | 29 +++ arch/arm/plat-omap/include/plat/omap44xx.h |2 + 7 files changed, 402 insertions(+), 2 deletions(-) create mode 100644 arch/arm/mach-omap2/hwspinlocks.c create mode 100644 arch/arm/plat-omap/hwspinlock.c create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile index 6725b3a..5f5c87b 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -170,3 +170,5 @@ obj-y += $(nand- m) $(nand-y) smc91x-$(CONFIG_SMC91X):= gpmc-smc91x.o obj-y += $(smc91x-m) $(smc91x-y) + +obj-$(CONFIG_ARCH_OMAP4) += hwspinlocks.o \ No newline at end of file diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach- omap2/hwspinlocks.c new file mode 100644 index 000..58a6483 --- /dev/null +++ b/arch/arm/mach-omap2/hwspinlocks.c @@ -0,0 +1,71 @@ +/* + * OMAP hardware spinlock device initialization + * + * Copyright (C) 2010 Texas Instruments. All rights reserved. + * + * Contact: Simon Que s...@ti.com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of
RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Thursday, August 05, 2010 5:05 AM
To: Basak, Partha
Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
Subject: Re: Issues with calling pm_runtime functions in
platform_pm_suspend_noirq/IRQ disabled context.
Basak, Partha p-bas...@ti.com writes:
Tested GPIO for Suspend with these changes obtained dump of Circular
Locking dependencies:
It would *really* help to have these GPIO conversion patches posted
against a public tree/branch one could see exactly what is going on and
be able to reproduce this problem for easier analysis. I have some
hunches as to what is going wrong here, but cannot be sure. I'd much
rather be able to see and try the code.
Fortunately, lockdep is verbose enough to try and understand the
symptoms here. Lockdep seems to have detected that these two locks have
been acquired in different order, resulting in *potential* deadlock.
Indeed, during init (#0 below) you have the hwmod_mutex acquired
(hwmod_for_each_by_class()) then the dpm_list_mtx acquired
(device_pm_add()). Later, during suspend the dpm_list_mtx is aquired
first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired
(omap_hwmod_idle()).
Taking the same locks in different order at different times is the
circular dependency and a recipe for potential deadlock.
In our case, there is no real potential for deadlock here as the locks
are only taken the hwmod-dpm order once during init, all other times
(when the omap_device/hwmod are ready) it will happen in the dpm-hwmod
order.
The simple fix here is probably to remove the locking in the
omap_hwmod_for_each* functions. Could you try that?
We tried this, it works. But the GPIO patch series sent out(posted today, Aug
06 2010) are tested based on reverting the pm_bus.c suspend_no_irq patch.
I'm a bit confused why I don't see the same problem with the UART layer
which currently also handles things in the idle path as well.
Kevin
# echo mem /sys/power/state
[ 809.981658] PM: Syncing filesystems ... done.
[ 810.037963] Freezing user space processes ... (elapsed 0.02 seconds)
done.
[ 810.067901] Freezing remaining freezable tasks ... (elapsed 0.02
seconds) done.
[ 810.105224] Suspending console(s) (use no_console_suspend to debug)
[ 810.166748] PM: suspend of devices complete after 46.142 msecs
[ 810.170562]
[ 810.170593] ===
[ 810.170593] [ INFO: possible circular locking dependency detected ]
[ 810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
[ 810.170654] ---
[ 810.170654] sh/670 is trying to acquire lock:
[ 810.170684] (omap_hwmod_mutex){+.+.+.}, at: [c004fe84]
omap_hwmod_idle+0x1c/0x38
[ 810.170745]
[ 810.170745] but task is already holding lock:
[ 810.170776] (dpm_list_mtx){+.+...}, at: [c023baf8]
dpm_suspend_noirq+0x28/0x188
[ 810.170806]
[ 810.170837] which lock already depends on the new lock.
[ 810.170837]
[ 810.170837]
[ 810.170837] the existing dependency chain (in reverse order) is:
[ 810.170867]
[ 810.170867] - #1 (dpm_list_mtx){+.+...}:
[ 810.170898][c009bc3c] lock_acquire+0x60/0x74
[ 810.170959][c0437a9c] mutex_lock_nested+0x58/0x2e4
[ 810.170989][c023bcc0] device_pm_add+0x14/0xcc
[ 810.171020][c0235304] device_add+0x3b8/0x564
[ 810.171051][c0238834] platform_device_add+0x104/0x160
[ 810.171112][c005f2a8] omap_device_build_ss+0x14c/0x1c8
[ 810.171142][c005f36c] omap_device_build+0x48/0x50
[ 810.171173][c004d34c] omap2_init_gpio+0xf0/0x15c
[ 810.171203][c004f254]
omap_hwmod_for_each_by_class+0x60/0xa4
[ 810.171264][c0040340] do_one_initcall+0x58/0x1b4
[ 810.171295][c0008574] kernel_init+0x98/0x150
[ 810.171325][c0041968] kernel_thread_exit+0x0/0x8
[ 810.171356]
[ 810.171356] - #0 (omap_hwmod_mutex){+.+.+.}:
[ 810.171386][c009b4e4] __lock_acquire+0x108c/0x1784
[ 810.171447][c009bc3c] lock_acquire+0x60/0x74
[ 810.171478][c0437a9c] mutex_lock_nested+0x58/0x2e4
[ 810.171508][c004fe84] omap_hwmod_idle+0x1c/0x38
[ 810.171539][c005eb9c] omap_device_idle_hwmods+0x20/0x3c
[ 810.171600][c005ec88] _omap_device_deactivate+0x58/0x14c
[ 810.171630][c005ef50] omap_device_idle+0x4c/0x6c
[ 810.171661][c0053e7c] platform_pm_runtime_suspend+0x4c/0x74
[ 810.171691][c023c9f8] __pm_runtime_suspend+0x204/0x34c
[ 810.171722][c023cbe0] pm_runtime_suspend+0x20/0x34
[ 810.171752][c0053dbc] platform_pm_runtime_idle+0x8/0x10
[ 810.171783][c023c344] __pm_runtime_idle+0x15c/0x198
[ 810.171813
RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, August 05, 2010 4:51 AM To: Basak, Partha Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja, Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit Subject: Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context. Kevin Hilman khil...@deeprootsystems.com writes: Kevin Hilman khil...@deeprootsystems.com writes: Basak, Partha p-bas...@ti.com writes: -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Tuesday, August 03, 2010 11:06 PM To: Basak, Partha Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja, Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit Subject: Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context. Basak, Partha p-bas...@ti.com writes: Resending with the corrected mailing list Hello Kevin, I want to draw your attention to an issue the following patch introduces. http://dev.omapzoom.org/?p=swarch/linux-omap- adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc [...] I believe, it is not correct to call the pm_runtime APIs from interrupt locked context since the underlying functions __pm_runtime_suspend __pm_runtime_resume enable interrupts and call schedule(). Further, from a s/w layering standpoint, platform-bus is a deeper layer than the Runtime layer, which the runtime layer calls into. So it may not be correct to have this layer calling into pm_runtime(). It should directly call omap_device APIs. The original version of this patch did directly call the omap_device APIs. However, Paul didn't like that approach and there were use-counting problems to handle as well (e.g. if a driver was not (yet) in use, it would already be disabled, and a suspend would trigger an omap_device_disable() which would fail since device was already disabled.) Paul and I agreed that this current approach would solve the use-counting problems, but you're correct in that it introduces new problems as these functions can _possibly_ sleep/schedule. That being said, have you actually seen problems here? I would like to see how they are triggered? Scenario 1: Consider the case where a driver has already called pm_runtime_put_sync as part of aggressive clock cutting scheme. Subsequently, when system suspend is called, pm_runtime_put_sync is called again. Due to atomic_dec_and_test check on usage_count, the second call goes through w/o error. I don't think you're fully understanding the point of the put/get in the suspend/resume path. The reason for the 'put' in the suspend path is because the PM core has done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that runtime PM transitions are prevented during suspend/resume. On OMAP, we want to allow those transitions to happen so we can use the same runtime PM calls in the idle and suspend paths. At System resume, pm_runtime_get_sync is called twice, once by resume, once by the driver itself. Yes, but there is a 'put_sync' in between those done by the PM core during resume (c.f. dpm_complete() in drivers/base/power/main.c] Because of the extra reference count, the aggressive clock control by the driver is broken, as call to put_sync by a driver reduces to usage_count to 1. As a result clock transition in Idle-path is broken. A closer look here, and there is indeed a bug in the _resume_noirq() method. The get here needs to be a _noresume version to match what is done in the PM core. This doesn't change the use count, but it does change whether the device is actually awoken by this 'get' or not. This get should never actually awake the device. It is only there to compensate for what the PM core does. Can you try this patch? Will post a version to the list with changelog shortly. After a little more thinking, I'm not sure yet if this is a real problem or not. One other question. At least for GPIO testing, does it work if you simply remove the _noirq hooks from pm_bus.c? If so, please feel to post a version with the dependency that the patch adding suspend/resume hooks in pm_bus.c needs to be reverted first. We have reverted this patch and tested before submitting the modified GPIO series. Kevin Kevin diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c index bab0186..01bbe65 100644 --- a/arch/arm/mach-omap2/pm_bus.c +++ b/arch/arm/mach-omap2/pm_bus.c @@ -90,7 +90,7 @@ int platform_pm_resume_noirq(struct device *dev) * method so that the runtime PM usage counting is in the same * state it was when suspend was called. */ - pm_runtime_get_sync(dev
RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, August 05, 2010 3:17 AM To: Basak, Partha Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja, Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit Subject: Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context. Basak, Partha p-bas...@ti.com writes: -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Tuesday, August 03, 2010 11:06 PM To: Basak, Partha Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja, Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit Subject: Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context. Basak, Partha p-bas...@ti.com writes: Resending with the corrected mailing list Hello Kevin, I want to draw your attention to an issue the following patch introduces. http://dev.omapzoom.org/?p=swarch/linux-omap- adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc [...] I believe, it is not correct to call the pm_runtime APIs from interrupt locked context since the underlying functions __pm_runtime_suspend __pm_runtime_resume enable interrupts and call schedule(). Further, from a s/w layering standpoint, platform-bus is a deeper layer than the Runtime layer, which the runtime layer calls into. So it may not be correct to have this layer calling into pm_runtime(). It should directly call omap_device APIs. The original version of this patch did directly call the omap_device APIs. However, Paul didn't like that approach and there were use-counting problems to handle as well (e.g. if a driver was not (yet) in use, it would already be disabled, and a suspend would trigger an omap_device_disable() which would fail since device was already disabled.) Paul and I agreed that this current approach would solve the use-counting problems, but you're correct in that it introduces new problems as these functions can _possibly_ sleep/schedule. That being said, have you actually seen problems here? I would like to see how they are triggered? Scenario 1: Consider the case where a driver has already called pm_runtime_put_sync as part of aggressive clock cutting scheme. Subsequently, when system suspend is called, pm_runtime_put_sync is called again. Due to atomic_dec_and_test check on usage_count, the second call goes through w/o error. I don't think you're fully understanding the point of the put/get in the suspend/resume path. The reason for the 'put' in the suspend path is because the PM core has done a 'get' in dpm_prepare() [c.f. drivers/base/power/main.c] so that runtime PM transitions are prevented during suspend/resume. On OMAP, we want to allow those transitions to happen so we can use the same runtime PM calls in the idle and suspend paths. At System resume, pm_runtime_get_sync is called twice, once by resume, once by the driver itself. Yes, but there is a 'put_sync' in between those done by the PM core during resume (c.f. dpm_complete() in drivers/base/power/main.c] Because of the extra reference count, the aggressive clock control by the driver is broken, as call to put_sync by a driver reduces to usage_count to 1. As a result clock transition in Idle-path is broken. I understand the rationale better, but having these changes is making the next Idle call after a suspend-resume cycle to fail. I will continue debugging on this and come back with more details. Scenario2: Tested GPIO for Suspend with these changes obtained dump of Circular Locking dependencies: I don't think these to problems are related, AFAICT, they are separate issues. I'll respond to this scenario in a different reply. [...] The UART driver is a special case as it is managed from deep inside the idle path. Can you feedback on my observations and comment on the approach taken for these drivers? I cannot comment until I understand why these drivers need to enable/disable with interrupts off. In general, any use of the interrupts-off versions of the omap_device APIs need to be thorougly justified. Even the UART one will go away when the omap-serial driver is converted to runtime PM. For GPIO, it is a must to relinquish clocks in Idle-path as it is possible to have a GPIO bank requested and still allow PER domain to go to OFF. These the kind of things that I would expect to be discussed/described in the changelogs to patches posted to the list. For USB, this is required because of a h/w issue. Again, patches with descriptive changelogs describing the h/w issue please. My point is, just by exposing a _omap_hwmod_idle()(mutex-free version) will not let us call pm_runtime APIs in Idle path
RE: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.
-Original Message-
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Tuesday, August 03, 2010 11:06 PM
To: Basak, Partha
Cc: Paul Walmsley; linux-omap@vger.kernel.org; Kalliguddi, Hema; Raja,
Govindraj; Varadarajan, Charulatha; Nayak, Rajendra; Cousson, Benoit
Subject: Re: Issues with calling pm_runtime functions in
platform_pm_suspend_noirq/IRQ disabled context.
Basak, Partha p-bas...@ti.com writes:
Resending with the corrected mailing list
Hello Kevin,
I want to draw your attention to an issue the following patch
introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-
adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
[...]
I believe, it is not correct to call the pm_runtime APIs from
interrupt locked context since the underlying functions
__pm_runtime_suspend __pm_runtime_resume enable interrupts and call
schedule().
Further, from a s/w layering standpoint, platform-bus is a deeper
layer than the Runtime layer, which the runtime layer calls into. So
it may not be correct to have this layer calling into pm_runtime(). It
should directly call omap_device APIs.
The original version of this patch did directly call the omap_device
APIs. However, Paul didn't like that approach and there were
use-counting problems to handle as well (e.g. if a driver was not (yet)
in use, it would already be disabled, and a suspend would trigger an
omap_device_disable() which would fail since device was already
disabled.)
Paul and I agreed that this current approach would solve the
use-counting problems, but you're correct in that it introduces
new problems as these functions can _possibly_ sleep/schedule.
That being said, have you actually seen problems here? I would like
to see how they are triggered?
Scenario 1:
Consider the case where a driver has already called pm_runtime_put_sync as part
of aggressive clock cutting scheme. Subsequently, when system suspend is
called, pm_runtime_put_sync is called again. Due to atomic_dec_and_test check
on usage_count, the second call goes through w/o error.
At System resume, pm_runtime_get_sync is called twice, once by resume, once by
the driver itself. Because of the extra reference count, the aggressive clock
control by the driver is broken, as call to put_sync by a driver reduces to
usage_count to 1. As a result clock transition in Idle-path is broken.
Scenario2:
Tested GPIO for Suspend with these changes obtained dump of Circular Locking
dependencies:
# echo mem /sys/power/state
[ 809.981658] PM: Syncing filesystems ... done.
[ 810.037963] Freezing user space processes ... (elapsed 0.02 seconds) done.
[ 810.067901] Freezing remaining freezable tasks ... (elapsed 0.02 seconds)
done.
[ 810.105224] Suspending console(s) (use no_console_suspend to debug)
[ 810.166748] PM: suspend of devices complete after 46.142 msecs
[ 810.170562]
[ 810.170593] ===
[ 810.170593] [ INFO: possible circular locking dependency detected ]
[ 810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
[ 810.170654] ---
[ 810.170654] sh/670 is trying to acquire lock:
[ 810.170684] (omap_hwmod_mutex){+.+.+.}, at: [c004fe84]
omap_hwmod_idle+0x1c/0x38
[ 810.170745]
[ 810.170745] but task is already holding lock:
[ 810.170776] (dpm_list_mtx){+.+...}, at: [c023baf8]
dpm_suspend_noirq+0x28/0x188
[ 810.170806]
[ 810.170837] which lock already depends on the new lock.
[ 810.170837]
[ 810.170837]
[ 810.170837] the existing dependency chain (in reverse order) is:
[ 810.170867]
[ 810.170867] - #1 (dpm_list_mtx){+.+...}:
[ 810.170898][c009bc3c] lock_acquire+0x60/0x74
[ 810.170959][c0437a9c] mutex_lock_nested+0x58/0x2e4
[ 810.170989][c023bcc0] device_pm_add+0x14/0xcc
[ 810.171020][c0235304] device_add+0x3b8/0x564
[ 810.171051][c0238834] platform_device_add+0x104/0x160
[ 810.171112][c005f2a8] omap_device_build_ss+0x14c/0x1c8
[ 810.171142][c005f36c] omap_device_build+0x48/0x50
[ 810.171173][c004d34c] omap2_init_gpio+0xf0/0x15c
[ 810.171203][c004f254] omap_hwmod_for_each_by_class+0x60/0xa4
[ 810.171264][c0040340] do_one_initcall+0x58/0x1b4
[ 810.171295][c0008574] kernel_init+0x98/0x150
[ 810.171325][c0041968] kernel_thread_exit+0x0/0x8
[ 810.171356]
[ 810.171356] - #0 (omap_hwmod_mutex){+.+.+.}:
[ 810.171386][c009b4e4] __lock_acquire+0x108c/0x1784
[ 810.171447][c009bc3c] lock_acquire+0x60/0x74
[ 810.171478][c0437a9c] mutex_lock_nested+0x58/0x2e4
[ 810.171508][c004fe84] omap_hwmod_idle+0x1c/0x38
[ 810.171539][c005eb9c] omap_device_idle_hwmods+0x20/0x3c
[ 810.171600][c005ec88] _omap_device_deactivate+0x58/0x14c
[ 810.171630][c005ef50] omap_device_idle+0x4c/0x6c
[ 810.171661][c0053e7c] platform_pm_runtime_suspend
Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.
Resending with the corrected mailing list
Hello Kevin,
I want to draw your attention to an issue the following patch introduces.
http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc
arch/arm/mach-omap2/pm_bus.c patch | blob | history
diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
index 9719a9f..5e453dc 100644 (file)
--- a/arch/arm/mach-omap2/pm_bus.c
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -68,3 +68,51 @@ int platform_pm_runtime_idle(struct device *dev)
};
#endif /* CONFIG_PM_RUNTIME */
+#ifdef CONFIG_SUSPEND
+int platform_pm_suspend_noirq(struct device *dev)
+{
+ struct device_driver *drv = dev-driver;
+ int ret = 0;
+
+ if (!drv)
+ return 0;
+
+ if (drv-pm) {
+ if (drv-pm-suspend_noirq)
+ ret = drv-pm-suspend_noirq(dev);
+ }
+
+ /*
+* The DPM core has done a 'get' to prevent runtime PM
+* transitions during system PM. This put is to balance
+* out that get so that this device can now be runtime
+* suspended.
+*/
+ pm_runtime_put_sync(dev);
+
+ return ret;
+}
+
+int platform_pm_resume_noirq(struct device *dev)
+{
+ struct device_driver *drv = dev-driver;
+ int ret = 0;
+
+ /*
+* This 'get' is to balance the 'put' in the above suspend_noirq
+* method so that the runtime PM usage counting is in the same
+* state it was when suspend was called.
+*/
+ pm_runtime_get_sync(dev);
+
+ if (!drv)
+ return 0;
+
+ if (drv-pm) {
+ if (drv-pm-resume_noirq)
+ ret = drv-pm-resume_noirq(dev);
+ }
+
+ return ret;
+}
+#endif /* CONFIG_SUSPEND */
I believe, it is not correct to call the pm_runtime APIs from interrupt locked
context since the underlying functions __pm_runtime_suspend
__pm_runtime_resume enable interrupts and call schedule().
Further, from a s/w layering standpoint, platform-bus is a deeper layer than
the Runtime layer, which the runtime layer calls into. So it may not be correct
to have this layer calling into pm_runtime(). It should directly call
omap_device APIs.
We are facing a similar issue with a few drivers USB, UART GPIO where, we
need to enable/disable clocks restore/save context in the CPU-Idle path with
interrupts locked.
As we are unable to use Runtime APIs, we are using omap_device APIs directly
with the following modification in the .activate_funcion/ deactivate_funcion
(example UART driver)
static int uart_idle_hwmod(struct omap_device *od)
{
if (!irqs_disabled())
omap_hwmod_idle(od-hwmods[0]);
else
_omap_hwmod_idle(od-hwmods[0]);
return 0;
}
static int uart_enable_hwmod(struct omap_device *od)
{
if (!irqs_disabled())
omap_hwmod_enable(od-hwmods[0]);
else
_omap_hwmod_enable(od-hwmods[0]);
return 0;
}
Can you feedback on my observations and comment on the approach taken for these
drivers? (We will be posting out the refreshed patches for GPIO, WDT, Timers
shortly).
Thanks
Partha
--
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 12/20] OMAP: PM: create omap_devices for MPU, DSP, L3
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Paul Walmsley
Sent: Friday, July 02, 2010 9:00 PM
To: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Cc: Kevin Hilman
Subject: [PATCH 12/20] OMAP: PM: create omap_devices for MPU, DSP, L3
From: Kevin Hilman khil...@deeprootsystems.com
Create simple omap_devices for the main processors and busses.
This is required to support the forth-coming device-based OPP
approach, where OPPs are managed and tracked at the device level.
Also, move these common PM init functions into a common_pm_init call
that is called as a device_initcall(). The PM init is done at this level
to ensure that the driver core is initialized before initialized.
Signed-off-by: Kevin Hilman khil...@deeprootsystems.com
[p...@pwsan.com: sparse warnings cleaned up; newly-created functions moved
from mach-omap2/io.c to mach-omap2/pm.c; newly-created functions renamed
to start with omap2 rather than omap]
Signed-off-by: Paul Walmsley p...@pwsan.com
---
arch/arm/mach-omap2/Makefile |2 -
arch/arm/mach-omap2/io.c |2 -
arch/arm/mach-omap2/pm.c | 84
++
arch/arm/plat-omap/include/plat/common.h |4 +
4 files changed, 90 insertions(+), 2 deletions(-)
create mode 100644 arch/arm/mach-omap2/pm.c
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 2fa3418..213f1df 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -3,7 +3,7 @@
#
# Common support
-obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o
+obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer-gp.o
pm.o
omap-2-3-common = irq.o sdrc.o
hwmod-common = omap_hwmod.o \
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 05c9cdb..2b983ac 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -44,6 +44,7 @@
#include plat/clockdomain.h
#include clockdomains.h
+
#include plat/omap_hwmod.h
/*
@@ -346,7 +347,6 @@ void __init omap2_init_common_hw(struct
omap_sdrc_params *sdrc_cs0,
if (cpu_is_omap24xx() || cpu_is_omap34xx()) /* FIXME: OMAP4 */
omap_hwmod_late_init(skip_setup_idle);
- omap_pm_if_init();
if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
omap2_sdrc_init(sdrc_cs0, sdrc_cs1);
_omap2_init_reprogram_sdrc();
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
new file mode 100644
index 000..68f9f2e
--- /dev/null
+++ b/arch/arm/mach-omap2/pm.c
@@ -0,0 +1,84 @@
+/*
+ * pm.c - Common OMAP2+ power management-related code
+ *
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ * Copyright (C) 2010 Nokia Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/kernel.h
+#include linux/init.h
+#include linux/io.h
+#include linux/err.h
+
+#include plat/omap-pm.h
+#include plat/omap_device.h
+#include plat/common.h
+
+static struct omap_device_pm_latency *pm_lats;
+
+static struct device *mpu_dev;
+static struct device *dsp_dev;
+static struct device *l3_dev;
+
+struct device *omap2_get_mpuss_device(void)
+{
+ WARN_ON_ONCE(!mpu_dev);
+ return mpu_dev;
+}
+
+struct device *omap2_get_dsp_device(void)
+{
+ WARN_ON_ONCE(!dsp_dev);
+ return dsp_dev;
+}
+
+struct device *omap2_get_l3_device(void)
+{
+ WARN_ON_ONCE(!l3_dev);
+ return l3_dev;
+}
+
+/* static int _init_omap_device(struct omap_hwmod *oh, void *user) */
+static int _init_omap_device(char *name, struct device **new_dev)
+{
+ struct omap_hwmod *oh;
+ struct omap_device *od;
+
+ oh = omap_hwmod_lookup(name);
+ if (WARN(!oh, %s: could not find omap_hwmod for %s\n,
+ __func__, name))
+ return -ENODEV;
+
+ od = omap_device_build(oh-name, 0, oh, NULL, 0, pm_lats, 0, false);
+ if (WARN(IS_ERR(od), %s: could not build omap_device for %s\n,
+ __func__, name))
+ return -ENODEV;
+
+ *new_dev = od-pdev.dev;
+
+ return 0;
+}
+
+/*
+ * Build omap_devices for processors and bus.
+ */
+static void omap2_init_processor_devices(void)
+{
+ _init_omap_device(mpu, mpu_dev);
+ _init_omap_device(iva, dsp_dev);
+ _init_omap_device(l3_main, l3_dev);
This breaks OMAP4 as l3_main is not applicable for OMAP4.
+}
+
+static int __init omap2_common_pm_init(void)
+{
+ omap2_init_processor_devices();
+ omap_pm_if_init();
+
+ return 0;
+}
+device_initcall(omap2_common_pm_init);
+
diff --git a/arch/arm/plat-omap/include/plat/common.h
RE: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable and idle functions
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Paul Walmsley
Sent: Thursday, June 24, 2010 10:39 AM
To: Kevin Hilman
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable
and idle functions
Hi Kevin
On Wed, 23 Jun 2010, Kevin Hilman wrote:
Some hwmods may need to be idled/enabled in atomic context, so
non-locking versions of these functions are required.
Most users should not need these and usage of theses should be
controlled to understand why access is being done in atomic context.
For this reason, the non-locking functions are only exposed at the
hwmod level and not at the omap-device level.
The use-case that led to the need for the non-locking versions is
hwmods that are enabled/idled from within the core idle/suspend path.
Since interrupts are already disabled here, the mutex-based locking in
hwmod can sleep and will cause potential deadlocks.
I accept the use-case. But maybe it would be preferable to rename
_enable(), _idle(), _shutdown() to _omap_hwmod_{enable,idle,shutdown}() ?
That would avoid the need to add new functions that just call the existing
ones.
- Paul
Cc: Paul Walmsley p...@pwsan.com
Signed-off-by: Kevin Hilman khil...@deeprootsystems.com
---
arch/arm/mach-omap2/omap_hwmod.c | 32
++---
arch/arm/plat-omap/include/plat/omap_hwmod.h |2 +
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
omap2/omap_hwmod.c
index 3d11523..8b2b44a 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1287,6 +1287,18 @@ int omap_hwmod_unregister(struct omap_hwmod *oh)
}
/**
+ * __omap_hwmod_enable - enable an omap_hwmod (non-locking version)
+ * @oh: struct omap_hwmod *
+ *
+ * Enable an omap_hwomd @oh. Intended to be called in rare cases
+ * where usage is required in atomic context.
+ */
+int __omap_hwmod_enable(struct omap_hwmod *oh)
+{
+ return _enable(oh);
+}
+
+/**
* omap_hwmod_enable - enable an omap_hwmod
* @oh: struct omap_hwmod *
*
@@ -1301,12 +1313,26 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
return -EINVAL;
mutex_lock(omap_hwmod_mutex);
- r = _enable(oh);
+ r = __omap_hwmod_enable(oh);
mutex_unlock(omap_hwmod_mutex);
return r;
}
+
+/**
+ * __omap_hwmod_idle - idle an omap_hwmod (non-locking)
+ * @oh: struct omap_hwmod *
+ *
+ * Idle an omap_hwomd @oh. Intended to be called in rare instances
where
+ * used in atomic context.
+ */
+int __omap_hwmod_idle(struct omap_hwmod *oh)
+{
+ _idle(oh);
+ return 0;
+}
+
/**
* omap_hwmod_idle - idle an omap_hwmod
* @oh: struct omap_hwmod *
@@ -1319,9 +1345,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
if (!oh)
return -EINVAL;
- mutex_lock(omap_hwmod_mutex);
- _idle(oh);
- mutex_unlock(omap_hwmod_mutex);
+ __omap_hwmod_idle(oh);
BTW: The mutex locks are missing. Typo?
return 0;
}
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 0eccc09..9a3f4dc 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -486,7 +486,9 @@ int omap_hwmod_for_each(int (*fn)(struct omap_hwmod
*oh));
int omap_hwmod_late_init(void);
int omap_hwmod_enable(struct omap_hwmod *oh);
+int __omap_hwmod_enable(struct omap_hwmod *oh);
int omap_hwmod_idle(struct omap_hwmod *oh);
+int __omap_hwmod_idle(struct omap_hwmod *oh);
int omap_hwmod_shutdown(struct omap_hwmod *oh);
int omap_hwmod_enable_clocks(struct omap_hwmod *oh);
--
1.7.0.2
- Paul
--
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
--
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: On the APIs for Enabling and Disabling Wakeup capability.
Kevin, Thanks Partha -Original Message- From: Kalliguddi, Hema Sent: Thursday, June 24, 2010 8:25 PM To: Kevin Hilman; Basak, Partha Cc: Cousson, Benoit; p...@pwsan.com; Nayak, Rajendra; linux- o...@vger.kernel.org; Gadiyar, Anand; Kamat, Nishant Subject: RE: On the APIs for Enabling and Disabling Wakeup capability. Kevin, Replying on top of latest chin. Thanks, Hema -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, June 24, 2010 2:01 AM To: Basak, Partha Cc: Kalliguddi, Hema; Cousson, Benoit; p...@pwsan.com; Nayak, Rajendra; linux-omap@vger.kernel.org Subject: Re: On the APIs for Enabling and Disabling Wakeup capability. Basak, Partha p-bas...@ti.com writes: Benoit/Kevin, -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Friday, June 18, 2010 8:29 PM To: Kalliguddi, Hema Cc: Cousson, Benoit; Basak, Partha; p...@pwsan.com; Nayak, Rajendra; linux-omap@vger.kernel.org Subject: Re: On the APIs for Enabling and Disabling Wakeup capability. Kalliguddi, Hema hem...@ti.com writes: Hi Benoit, I have 2 cases in usb for the need of separate API for setting the auto idle bit. 1. Below the link for omap3430TRM. Please refer 24.1.5.4.2 and 24.1.5.4.3 there is note not to set smart idle and autoidle bit simultaneously. Suggestion is to set the auto idle 0 before setting smart idle. Then set to 1. Maybe this sequence should be enforced by the hwmod code itself, rather than the knowledge living in the driver. However, based on the errata below, auto-idle will always be zero so the there should be no conflict in setting smart-idle and auto-idle simultaneously today. This applicable for omap4 as well. I don't have the omap4430 pblic TRM to share. http://focus.ti.com/pdfs/wtbu/SWPU223D_Final_EPDF_06_07_2010.pdf [HK] The errata is applicable to OMAP3430 only. But this sequence applicable for omap4 as well. So for submitting the changes we need to have the HWMOD api to be changed to enaforce the Autodile set only after the smart idle. 2. There is a Errata in OMAP3 errata #1.59 that If auto idle is set then the USB can't wakeup the system by usb external events. The suggested workaround is to disable the autoIdle for omap3. This one could be handled at init time in usb-musb.c by setting HWMOD_NO_OCP_AUTOIDLE for the hwmod with a comment summarizing this errata. Note also that Errata 1.166 is another one related to MUSB auto-idle and we have a workaround in the PM branch to ensure that MUSB auto-idle is disabled in the idle path since it may be re-enabled after an off-mode transition. Few questions: 1. Are you mentioning about the following patch: http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm .git;a=commit;h=865f0e0b1dd25899fe30eee5c8f1dba420eea177? No, this is a totally unrelated problem related to a specific init sequene. [Partha] Please send the commit ID Though this patch allows clearing of AutoIdle bit(signified by HWMOD_INIT_NO_IDLE) while remaining in Idle state, it does not allow the reverse, i.e. setting back the AutoIdle bit, while still remaining in Idle state. The use-case for setting the auto-idle bit while leaving the device in idle have not been described. [Partha] My comment was more from a consistency of implementation standpoint. Once you clear the AutoIdle, there is no way to set it back before enabling the module. Anyhow, not clear what use-case you are trying to cater to here. 2. Changing the hwmod flags (oh-flags) is acceptable in run-time. Correct? For errata workarounds, in device/SoC init code yes. In drivers, no. Drivers should not have any knowledge of hwmod internals. 3. I believe, SysConfig settings have been a tricky area because of different h/w-errata. Instead of looking into particular errata, as and when they come in time to time and explore how to fit them in the framework, would it not be more useful to provide a more generic framework to operate on the SysConfig settings? Of course, as you suggested, the preferred approach would be to absorb the changes in the omap_device/hw-mod layer as much as possible. But unfortunately, that may not be sufficient in all situations. For this kind of thing, I strongly prefer to better understand the specific errata that require the special settings. History suggests that having an API to modify sysconfig means it would get used not just for errata workarounds but also because it doesn't work unless I do this. We *really* need to better understand the reasons behind these special cases. [Partha] Refer to the details provided by Hema for the supporting information. [HK] I agree that this can be handled using hw-flags for omap3. Can you please point me the commit for the above errata fix? I am not successful in getting
RE: On the APIs for Enabling and Disabling Wakeup capability.
Benoit/Kevin, -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Friday, June 18, 2010 8:29 PM To: Kalliguddi, Hema Cc: Cousson, Benoit; Basak, Partha; p...@pwsan.com; Nayak, Rajendra; linux-omap@vger.kernel.org Subject: Re: On the APIs for Enabling and Disabling Wakeup capability. Kalliguddi, Hema hem...@ti.com writes: Hi Benoit, I have 2 cases in usb for the need of separate API for setting the auto idle bit. 1. Below the link for omap3430TRM. Please refer 24.1.5.4.2 and 24.1.5.4.3 there is note not to set smart idle and autoidle bit simultaneously. Suggestion is to set the auto idle 0 before setting smart idle. Then set to 1. Maybe this sequence should be enforced by the hwmod code itself, rather than the knowledge living in the driver. However, based on the errata below, auto-idle will always be zero so the there should be no conflict in setting smart-idle and auto-idle simultaneously today. This applicable for omap4 as well. I don't have the omap4430 pblic TRM to share. http://focus.ti.com/pdfs/wtbu/SWPU223D_Final_EPDF_06_07_2010.pdf 2. There is a Errata in OMAP3 errata #1.59 that If auto idle is set then the USB can't wakeup the system by usb external events. The suggested workaround is to disable the autoIdle for omap3. This one could be handled at init time in usb-musb.c by setting HWMOD_NO_OCP_AUTOIDLE for the hwmod with a comment summarizing this errata. Note also that Errata 1.166 is another one related to MUSB auto-idle and we have a workaround in the PM branch to ensure that MUSB auto-idle is disabled in the idle path since it may be re-enabled after an off-mode transition. Few questions: 1. Are you mentioning about the following patch: http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commit;h=865f0e0b1dd25899fe30eee5c8f1dba420eea177? Though this patch allows clearing of AutoIdle bit(signified by HWMOD_INIT_NO_IDLE) while remaining in Idle state, it does not allow the reverse, i.e. setting back the AutoIdle bit, while still remaining in Idle state. 2. Changing the hwmod flags (oh-flags) is acceptable in run-time. Correct? 3. I believe, SysConfig settings have been a tricky area because of different h/w-errata. Instead of looking into particular errata, as and when they come in time to time and explore how to fit them in the framework, would it not be more useful to provide a more generic framework to operate on the SysConfig settings? Of course, as you suggested, the preferred approach would be to absorb the changes in the omap_device/hw-mod layer as much as possible. But unfortunately, that may not be sufficient in all situations. Partha Kevin -Original Message- From: Cousson, Benoit Sent: Thursday, June 17, 2010 3:04 PM To: Kalliguddi, Hema; Kevin Hilman; Basak, Partha Cc: p...@pwsan.com; Nayak, Rajendra; linux-omap@vger.kernel.org Subject: RE: On the APIs for Enabling and Disabling Wakeup capability. Hi Hema, From: linux-omap-ow...@vger.kernel.org Kevin, There is no errata in the USB which needs the Enable/Disable wakeup to be done Seperately. If it can be handled with omap_devie_enable/idle Apis it is sufficient. But there is a need of setting the Auto idle bit seperately as for the USBOTG there is a need to set the Autoidle only after configuring the smart idle. It is recommended in the TRM not set the auto idle and smart idle together. This I discussed with Partha he sent a mail to you. For setting autoidle there is an api at hwmod layer but not yet omap device layer. Is there a plan to add API at omap device layer for enabling/disabling the autoidle? The whole issue with exposing all the low level stuff at driver level is that the hwmod abstraction become less and less useful. Drivers will start hacking with that for no good reason. And then we will start adding again some omap version specific hacks in the driver. Could you provide the full errata description of that USB_OTG issue? Or at least that TRM part you are talking about. Thanks, Benoit Regards, Hema Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920 -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, June 17, 2010 5:56 AM To: Basak, Partha Cc: p...@pwsan.com; Kalliguddi, Hema; Nayak, Rajendra; linux-omap@vger.kernel.org Subject: Re: On the APIs for Enabling and Disabling Wakeup capability. Basak, Partha p-bas...@ti.com writes: I wanted to close on the introduction of two new OMAP device APIs omap_device_enable_wakeup () omap_device_disable_wakeup() in omap_device layer. These APIs are potentially needed by the USB driver (via function pointers) to work around some USB erratum. Alternatively, can we call omap_hwmod_enable_wakeup() via function pointer
On the APIs for Enabling and Disabling Wakeup capability.
Hello Paul, I wanted to close on the introduction of two new OMAP device APIs omap_device_enable_wakeup () omap_device_disable_wakeup() in omap_device layer. These APIs are potentially needed by the USB driver (via function pointers) to work around some USB erratum. Alternatively, can we call omap_hwmod_enable_wakeup() via function pointer? Is it agreeable to call these from driver code (via function pointers)in some special cases such as to handle some errata? Thanks to clarify. Partha -- 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: On the APIs for Enabling and Disabling Wakeup capability.
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Basak, Partha Sent: Wednesday, June 16, 2010 8:10 PM To: p...@pwsan.com Cc: khil...@deeprootsystems.com; Kalliguddi, Hema; Nayak, Rajendra; linux- o...@vger.kernel.org Subject: On the APIs for Enabling and Disabling Wakeup capability. Hello Paul, I wanted to close on the introduction of two new OMAP device APIs omap_device_enable_wakeup () omap_device_disable_wakeup() in omap_device layer. We also need the API omap_hwmod_set_slave_idlemode to be exposed as an omap_device API. Net-net, the following APIs which are exported in hwmod layer needs to be wrapped under omap_device_layer as well so that drivers can use them: omap_hwmod_enable_wakeup omap_hwmod_disable_wakeup omap_hwmod_set_slave_idlemode These APIs are potentially needed by the USB driver (via function pointers) to work around some USB erratum. Alternatively, can we call omap_hwmod_enable_wakeup() via function pointer? Is it agreeable to call these from driver code (via function pointers)in some special cases such as to handle some errata? Thanks to clarify. Partha -- 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 -- 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
