Re: [PATCH v16 09/12] OMAP: dmtimer: low-power mode support
On Thu, Nov 17, 2011 at 6:23 AM, Omar Ramirez Luna or.r...@gmail.com wrote: On Wed, Nov 16, 2011 at 12:18 AM, DebBarma, Tarun Kanti tarun.ka...@ti.com wrote: ... My use case is as follows: DSP sends a mailbox message to ARM, this triggers a tasklet in mailbox for processing the message, when it reaches tidspbridge it turns out that the DSP wants to enable a gpt timer; however, locks involved clocks_mutex and dm_timer_lock now could cause a deadlock as they have been called from unsafe contexts in the past (http://pastebin.com/7Dtz8t0f). Is the intention to restrict enabling the dmtimer clocks from hard|soft irqs? The aim is to prevent client drivers perform clock enable/disable independently. Instead just use the request/start/stop APIs. In that way we can make clock enable/disable functions static in the future. Those are the APIs I'm using, omap_dm_timer_request_specific from a softirq triggers this warning, this was not hinted by the patch or cover letter of the series, hence the question: Was this change intentional? Does omap_dm_timer_request_specific should be allowed on other contexts (soft|hard irq)? omap_dm_timer_request_specific() uses spin_lock_irqsave and spin_unlock_irqrestore to protect the timer list. There is nothing specific to prevent this function from being called from interrupt context. But you probably have to be careful to avoid deadlock in the interrupt context by disabling interrupts appropriately so that we avoid trying to re-acquire the lock. If you send me the code snippet I can have a look. -- Tarun ... You are right. This change some how got missed in mainline. I have posted a patch for this already. Ok. Regards, Omar -- 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 v16 09/12] OMAP: dmtimer: low-power mode support
On Thu, Nov 17, 2011 at 3:42 AM, DebBarma, Tarun Kanti tarun.ka...@ti.com wrote: ... Is the intention to restrict enabling the dmtimer clocks from hard|soft irqs? The aim is to prevent client drivers perform clock enable/disable independently. Instead just use the request/start/stop APIs. In that way we can make clock enable/disable functions static in the future. Those are the APIs I'm using, omap_dm_timer_request_specific from a softirq triggers this warning, this was not hinted by the patch or cover letter of the series, hence the question: Was this change intentional? Does omap_dm_timer_request_specific should be allowed on other contexts (soft|hard irq)? omap_dm_timer_request_specific() uses spin_lock_irqsave and spin_unlock_irqrestore to protect the timer list. There is nothing specific to prevent this function from being called from interrupt context. But you probably have to be careful to avoid deadlock in the interrupt context by disabling interrupts appropriately so that we avoid trying to re-acquire the lock. ? ...interrupt context should have interrupts disabled. If you send me the code snippet I can have a look. Here it is a module that triggers this: http://pastebin.com/bfcWtmQp Please apply this patch too, otherwise OOT modules disable lockdep: http://comments.gmane.org/gmane.linux.kernel/1216036 Also enable Schedule-while-atomic checks as this triggers errors even on process context, I have prepared a patch for those, will submit soon. And BTW, these errors are seen since patch ARM: OMAP: dmtimer: switch-over to platform device driver, where code to do clk_get was placed under spin_lock_irqsave, clk_get is the one holding a mutex_lock which can't be done under a spin_lock. Regards, Omar -- 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 v16 09/12] OMAP: dmtimer: low-power mode support
On Wed, Nov 16, 2011 at 12:18 AM, DebBarma, Tarun Kanti tarun.ka...@ti.com wrote: ... My use case is as follows: DSP sends a mailbox message to ARM, this triggers a tasklet in mailbox for processing the message, when it reaches tidspbridge it turns out that the DSP wants to enable a gpt timer; however, locks involved clocks_mutex and dm_timer_lock now could cause a deadlock as they have been called from unsafe contexts in the past (http://pastebin.com/7Dtz8t0f). Is the intention to restrict enabling the dmtimer clocks from hard|soft irqs? The aim is to prevent client drivers perform clock enable/disable independently. Instead just use the request/start/stop APIs. In that way we can make clock enable/disable functions static in the future. Those are the APIs I'm using, omap_dm_timer_request_specific from a softirq triggers this warning, this was not hinted by the patch or cover letter of the series, hence the question: Was this change intentional? Does omap_dm_timer_request_specific should be allowed on other contexts (soft|hard irq)? ... You are right. This change some how got missed in mainline. I have posted a patch for this already. Ok. Regards, Omar -- 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 v16 09/12] OMAP: dmtimer: low-power mode support
Hi Tarun,
On Tue, Sep 20, 2011 at 6:30 AM, Tarun Kanti DebBarma
tarun.ka...@ti.com wrote:
Clock is enabled only when timer is started and disabled when the the timer
is stopped. Therefore before accessing registers in functions clock is enabled
and then disabled back at the end of access. Context save is done dynamically
whenever the registers are modified. Context restore is called when context is
lost.
Now that handling of the clock has been decoupled from
omap_dm_timer_request_specific/free and placed in start/stop
functions, there is a possible irq lock inversion dependency
detected for any attempt to enable the clock from soft|hard irq.
My use case is as follows:
DSP sends a mailbox message to ARM, this triggers a tasklet in mailbox
for processing the message, when it reaches tidspbridge it turns out
that the DSP wants to enable a gpt timer; however, locks involved
clocks_mutex and dm_timer_lock now could cause a deadlock as they
have been called from unsafe contexts in the past
(http://pastebin.com/7Dtz8t0f).
Is the intention to restrict enabling the dmtimer clocks from hard|soft irqs?
...
@@ -303,6 +342,22 @@ void omap_dm_timer_stop(struct omap_dm_timer *timer)
rate = clk_get_rate(timer-fclk);
__omap_dm_timer_stop(timer, timer-posted, rate, is_omap2);
+
+ if (timer-loses_context) {
+ if (timer-get_context_loss_count)
+ timer-ctx_loss_count =
+ timer-get_context_loss_count(timer-pdev-dev);
+ }
+
+ /*
+ * Since the register values are computed and written within
+ * __omap_dm_timer_stop, we need to use read to retrieve the
+ * context.
+ */
+ timer-context.tclr =
+ omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
+ timer-context.tisr = __raw_readl(timer-irq_stat);
+ omap_dm_timer_disable(timer);
}
EXPORT_SYMBOL_GPL(omap_dm_timer_stop);
I didn't review the whole patch, but at least this part is not in
mainline, it seems like the patch modified by Tony and the v16 were
not the same.
I could diff and include the missing parts, if someone can confirm
that we need v16, which I believe so, since current stop function
doesn't call omap_dm_timer_disable.
Regards,
Omar
--
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 v16 09/12] OMAP: dmtimer: low-power mode support
On Tue, Nov 15, 2011 at 11:27 PM, Omar Ramirez Luna or.r...@gmail.com wrote:
Hi Tarun,
On Tue, Sep 20, 2011 at 6:30 AM, Tarun Kanti DebBarma
tarun.ka...@ti.com wrote:
Clock is enabled only when timer is started and disabled when the the timer
is stopped. Therefore before accessing registers in functions clock is
enabled
and then disabled back at the end of access. Context save is done dynamically
whenever the registers are modified. Context restore is called when context
is
lost.
Now that handling of the clock has been decoupled from
omap_dm_timer_request_specific/free and placed in start/stop
functions, there is a possible irq lock inversion dependency
detected for any attempt to enable the clock from soft|hard irq.
My use case is as follows:
DSP sends a mailbox message to ARM, this triggers a tasklet in mailbox
for processing the message, when it reaches tidspbridge it turns out
that the DSP wants to enable a gpt timer; however, locks involved
clocks_mutex and dm_timer_lock now could cause a deadlock as they
have been called from unsafe contexts in the past
(http://pastebin.com/7Dtz8t0f).
Is the intention to restrict enabling the dmtimer clocks from hard|soft irqs?
The aim is to prevent client drivers perform clock enable/disable independently.
Instead just use the request/start/stop APIs. In that way we can make clock
enable/disable functions static in the future.
...
@@ -303,6 +342,22 @@ void omap_dm_timer_stop(struct omap_dm_timer *timer)
rate = clk_get_rate(timer-fclk);
__omap_dm_timer_stop(timer, timer-posted, rate, is_omap2);
+
+ if (timer-loses_context) {
+ if (timer-get_context_loss_count)
+ timer-ctx_loss_count =
+ timer-get_context_loss_count(timer-pdev-dev);
+ }
+
+ /*
+ * Since the register values are computed and written within
+ * __omap_dm_timer_stop, we need to use read to retrieve the
+ * context.
+ */
+ timer-context.tclr =
+ omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
+ timer-context.tisr = __raw_readl(timer-irq_stat);
+ omap_dm_timer_disable(timer);
}
EXPORT_SYMBOL_GPL(omap_dm_timer_stop);
I didn't review the whole patch, but at least this part is not in
mainline, it seems like the patch modified by Tony and the v16 were
not the same.
I could diff and include the missing parts, if someone can confirm
that we need v16, which I believe so, since current stop function
doesn't call omap_dm_timer_disable.
You are right. This change some how got missed in mainline.
I have posted a patch for this already.
--
Tarun
Regards,
Omar
--
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 v16 09/12] OMAP: dmtimer: low-power mode support
On Thu, Sep 22, 2011 at 6:30 AM, Tony Lindgren t...@atomide.com wrote:
* Tarun Kanti DebBarma tarun.ka...@ti.com [110920 03:57]:
Clock is enabled only when timer is started and disabled when the the timer
is stopped. Therefore before accessing registers in functions clock is
enabled
and then disabled back at the end of access. Context save is done dynamically
whenever the registers are modified. Context restore is called when context
is
lost.
I've updated this to use revision instead of tidr. Updated patch below.
Ok.
--
Tarun
Regards,
Tony
From: Tarun Kanti DebBarma tarun.ka...@ti.com
Date: Tue, 20 Sep 2011 17:00:24 +0530
Subject: [PATCH] ARM: OMAP: dmtimer: low-power mode support
Clock is enabled only when timer is started and disabled when the the timer
is stopped. Therefore before accessing registers in functions clock is enabled
and then disabled back at the end of access. Context save is done dynamically
whenever the registers are modified. Context restore is called when context is
lost.
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
Reviewed-by: Santosh Shilimkar santosh.shilim...@ti.com
[t...@atomide.com: updated to use revision instead of tidr]
Signed-off-by: Tony Lindgren t...@atomide.com
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index f1e3ec1..1140e98 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -44,6 +44,9 @@
#include plat/common.h
#include plat/omap_hwmod.h
#include plat/omap_device.h
+#include plat/omap-pm.h
+
+#include powerdomain.h
/* Parent clocks, eventually these will come from the clock framework */
@@ -433,6 +436,7 @@ static int __init omap_timer_init(struct omap_hwmod *oh,
void *unused)
struct dmtimer_platform_data *pdata;
struct omap_device *od;
struct omap_timer_capability_dev_attr *timer_dev_attr;
+ struct powerdomain *pwrdm;
pr_debug(%s: %s\n, __func__, oh-name);
@@ -467,6 +471,11 @@ static int __init omap_timer_init(struct omap_hwmod *oh,
void *unused)
if ((sys_timer_reserved (id - 1)) 0x1)
pdata-reserved = 1;
+ pwrdm = omap_hwmod_get_pwrdm(oh);
+ pdata-loses_context = pwrdm_can_ever_lose_context(pwrdm);
+#ifdef CONFIG_PM
+ pdata-get_context_loss_count = omap_pm_get_dev_context_loss_count;
+#endif
od = omap_device_build(name, id, oh, pdata, sizeof(*pdata),
omap2_dmtimer_latency,
ARRAY_SIZE(omap2_dmtimer_latency),
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index c8df3c3..43eb750 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -77,6 +77,29 @@ static void omap_dm_timer_write_reg(struct omap_dm_timer
*timer, u32 reg,
__omap_dm_timer_write(timer, reg, value, timer-posted);
}
+static void omap_timer_restore_context(struct omap_dm_timer *timer)
+{
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_OCP_CFG_OFFSET,
+ timer-context.tiocp_cfg);
+ if (timer-revision 1)
+ __raw_writel(timer-context.tistat, timer-sys_stat);
+
+ __raw_writel(timer-context.tisr, timer-irq_stat);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_WAKEUP_EN_REG,
+ timer-context.twer);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_COUNTER_REG,
+ timer-context.tcrr);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG,
+ timer-context.tldr);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_MATCH_REG,
+ timer-context.tmar);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG,
+ timer-context.tsicr);
+ __raw_writel(timer-context.tier, timer-irq_ena);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG,
+ timer-context.tclr);
+}
+
static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
{
int c;
@@ -96,12 +119,14 @@ static void omap_dm_timer_wait_for_reset(struct
omap_dm_timer *timer)
static void omap_dm_timer_reset(struct omap_dm_timer *timer)
{
+ omap_dm_timer_enable(timer);
if (timer-pdev-id != 1) {
omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
omap_dm_timer_wait_for_reset(timer);
}
__omap_dm_timer_reset(timer, 0, 0);
+ omap_dm_timer_disable(timer);
timer-posted = 1;
}
@@ -117,8 +142,6 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
return -EINVAL;
}
- omap_dm_timer_enable(timer);
-
if (pdata-needs_manual_reset)
omap_dm_timer_reset(timer);
@@ -193,7 +216,6 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_request_specific);
void omap_dm_timer_free(struct omap_dm_timer *timer)
{
-
Re: [PATCH v16 09/12] OMAP: dmtimer: low-power mode support
* Tarun Kanti DebBarma tarun.ka...@ti.com [110920 03:57]:
Clock is enabled only when timer is started and disabled when the the timer
is stopped. Therefore before accessing registers in functions clock is enabled
and then disabled back at the end of access. Context save is done dynamically
whenever the registers are modified. Context restore is called when context is
lost.
I've updated this to use revision instead of tidr. Updated patch below.
Regards,
Tony
From: Tarun Kanti DebBarma tarun.ka...@ti.com
Date: Tue, 20 Sep 2011 17:00:24 +0530
Subject: [PATCH] ARM: OMAP: dmtimer: low-power mode support
Clock is enabled only when timer is started and disabled when the the timer
is stopped. Therefore before accessing registers in functions clock is enabled
and then disabled back at the end of access. Context save is done dynamically
whenever the registers are modified. Context restore is called when context is
lost.
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
Reviewed-by: Santosh Shilimkar santosh.shilim...@ti.com
[t...@atomide.com: updated to use revision instead of tidr]
Signed-off-by: Tony Lindgren t...@atomide.com
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index f1e3ec1..1140e98 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -44,6 +44,9 @@
#include plat/common.h
#include plat/omap_hwmod.h
#include plat/omap_device.h
+#include plat/omap-pm.h
+
+#include powerdomain.h
/* Parent clocks, eventually these will come from the clock framework */
@@ -433,6 +436,7 @@ static int __init omap_timer_init(struct omap_hwmod *oh,
void *unused)
struct dmtimer_platform_data *pdata;
struct omap_device *od;
struct omap_timer_capability_dev_attr *timer_dev_attr;
+ struct powerdomain *pwrdm;
pr_debug(%s: %s\n, __func__, oh-name);
@@ -467,6 +471,11 @@ static int __init omap_timer_init(struct omap_hwmod *oh,
void *unused)
if ((sys_timer_reserved (id - 1)) 0x1)
pdata-reserved = 1;
+ pwrdm = omap_hwmod_get_pwrdm(oh);
+ pdata-loses_context = pwrdm_can_ever_lose_context(pwrdm);
+#ifdef CONFIG_PM
+ pdata-get_context_loss_count = omap_pm_get_dev_context_loss_count;
+#endif
od = omap_device_build(name, id, oh, pdata, sizeof(*pdata),
omap2_dmtimer_latency,
ARRAY_SIZE(omap2_dmtimer_latency),
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index c8df3c3..43eb750 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -77,6 +77,29 @@ static void omap_dm_timer_write_reg(struct omap_dm_timer
*timer, u32 reg,
__omap_dm_timer_write(timer, reg, value, timer-posted);
}
+static void omap_timer_restore_context(struct omap_dm_timer *timer)
+{
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_OCP_CFG_OFFSET,
+ timer-context.tiocp_cfg);
+ if (timer-revision 1)
+ __raw_writel(timer-context.tistat, timer-sys_stat);
+
+ __raw_writel(timer-context.tisr, timer-irq_stat);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_WAKEUP_EN_REG,
+ timer-context.twer);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_COUNTER_REG,
+ timer-context.tcrr);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG,
+ timer-context.tldr);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_MATCH_REG,
+ timer-context.tmar);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG,
+ timer-context.tsicr);
+ __raw_writel(timer-context.tier, timer-irq_ena);
+ omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG,
+ timer-context.tclr);
+}
+
static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
{
int c;
@@ -96,12 +119,14 @@ static void omap_dm_timer_wait_for_reset(struct
omap_dm_timer *timer)
static void omap_dm_timer_reset(struct omap_dm_timer *timer)
{
+ omap_dm_timer_enable(timer);
if (timer-pdev-id != 1) {
omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
omap_dm_timer_wait_for_reset(timer);
}
__omap_dm_timer_reset(timer, 0, 0);
+ omap_dm_timer_disable(timer);
timer-posted = 1;
}
@@ -117,8 +142,6 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
return -EINVAL;
}
- omap_dm_timer_enable(timer);
-
if (pdata-needs_manual_reset)
omap_dm_timer_reset(timer);
@@ -193,7 +216,6 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_request_specific);
void omap_dm_timer_free(struct omap_dm_timer *timer)
{
- omap_dm_timer_disable(timer);
clk_put(timer-fclk);
WARN_ON(!timer-reserved);
@@ -275,6 +297,11 @@
