diff mbox

tidspbridge issue with omap_dm_timer_free

Message ID CAMP44s2Wou54d2UWkwebxO17=gaVoiXrB5Lk-q2g90x3ok8L7w@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Felipe Contreras Aug. 10, 2011, 11:27 p.m. UTC
On Wed, Aug 10, 2011 at 9:42 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> I am trying to use a more recent version of the tidspbridge code in
> the Nokia N9, but I'm stuck with this warning that is caused by using
> the dm timer framework.

Actually, the problem only happens on the 'dspbridge' branch, which
has some unmerged patches. These patches introduce some new mutexes
that trigger this.

My proposed solution is to request the dm timers at initialization
time, and just enable/disable them on wake/hibernate, which is a bit
similar to what was happening before, and probably more efficient and
proper.

I'm attaching the patch.

Comments

Felipe Contreras Aug. 14, 2011, 7:44 a.m. UTC | #1
On Sat, Aug 13, 2011 at 8:20 PM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote:
> On Wed, Aug 10, 2011 at 6:27 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 9:42 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> I am trying to use a more recent version of the tidspbridge code in
>>> the Nokia N9, but I'm stuck with this warning that is caused by using
>>> the dm timer framework.
>>
>> Actually, the problem only happens on the 'dspbridge' branch, which
>> has some unmerged patches. These patches introduce some new mutexes
>> that trigger this.
>
> The only lock introduced is a spin_lock_bh which makes sense to warn
> if the underlying functions have a mutex, but I couldn't find the
> clk_set_parent mutex which is causing this on the omap files.

Hmmm, that's true. It seems I also have this series:
http://thread.gmane.org/gmane.linux.ports.arm.omap/18509/focus=18508

That for some reason never made it anywhere. Perhaps it will?

>> My proposed solution is to request the dm timers at initialization
>> time, and just enable/disable them on wake/hibernate, which is a bit
>> similar to what was happening before, and probably more efficient and
>> proper.
>
> When I made these changes I wanted to avoid keeping an array with the
> clocks requested and nobody else being able to use them (even if that
> meant a conflict between two drivers trying to use the same clock, but
> other warnings could help to point to such cases). Also I wanted to
> maintain uniformity, not just requesting some clock, freeing them and
> then some others to be held and freed only on module removal.
>
> That said, I have no problem on changing to your approach if needed.

Yeah, but with the current approach it would be possible that
everything works fine, and then the DSP goes to hibernation, a module
is loaded that uses one dm timer, and then when the DSP wakes up, that
timer would fail, there isn't even a check for that. If I follow the
code correctly, when the DSP goes back to hibernation, there would be
a crash, as a NULL timer would be freed. I think that's too error
prone.
omar ramirez Aug. 23, 2011, 11:03 p.m. UTC | #2
Hi,

On Sun, Aug 14, 2011 at 2:44 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Yeah, but with the current approach it would be possible that
> everything works fine, and then the DSP goes to hibernation, a module
> is loaded that uses one dm timer, and then when the DSP wakes up, that
> timer would fail, there isn't even a check for that. If I follow the
> code correctly, when the DSP goes back to hibernation, there would be
> a crash, as a NULL timer would be freed. I think that's too error
> prone.

Yes, this scenario could trigger such error condition. So, FWIW, I'm
fine with just a check or your approach. If possible to apply the same
mechanism for the other clocks, so in the enable/disable functions all
the clocks are then enabled/disabled avoiding mixed behavior of
enable/request, disable/free between gpt and mcbsp clocks.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From cd7f245ba42eb0d18bdfc3f29e2856f09227528e Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@nokia.com>
Date: Thu, 11 Aug 2011 01:28:08 +0300
Subject: [PATCH] staging: tidspbridge: split gpt requests

Otherwise I get this:

 BUG: sleeping function called from invalid context at kernel/mutex.c:287
 in_atomic(): 1, irqs_disabled(): 0, pid: 305, name: mboxd/0
 3 locks held by mboxd/0/305:
  #0:  (mboxd){+.+...}, at: [<b0081778>] worker_thread+0x154/0x2bc
  #1:  (&mq->work){+.+...}, at: [<b0081778>] worker_thread+0x154/0x2bc
  #2:  (pwr_lock){+.....}, at: [<af12f870>] handle_hibernation_from_dsp+0x1c/0x158 [bridgedriver]
 [<b003f75c>] (unwind_backtrace+0x0/0xd4) from [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c)
 [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c) from [<b00568f8>] (clk_set_parent+0x34/0xf8)
 [<b00568f8>] (clk_set_parent+0x34/0xf8) from [<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58)
 [<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58) from [<b005e424>] (omap_dm_timer_reset+0x78/0xd0)
 [<b005e424>] (omap_dm_timer_reset+0x78/0xd0) from [<b005e490>] (omap_dm_timer_free+0x14/0x48)
 [<b005e490>] (omap_dm_timer_free+0x14/0x48) from [<af1309d4>] (dsp_clk_disable+0x98/0x15c [bridgedriver])
 [<af1309d4>] (dsp_clk_disable+0x98/0x15c [bridgedriver]) from [<af130abc>] (dsp_clock_disable_all+0x24/0x34 [bridgedriver])
 [<af130abc>] (dsp_clock_disable_all+0x24/0x34 [bridgedriver]) from [<af12f914>] (handle_hibernation_from_dsp+0xc0/0x158 [bridgedriver])
 [<af12f914>] (handle_hibernation_from_dsp+0xc0/0x158 [bridgedriver]) from [<af12c2bc>] (io_mbox_msg+0x8c/0x100 [bridgedriver])
 [<af12c2bc>] (io_mbox_msg+0x8c/0x100 [bridgedriver]) from [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox])
 [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox]) from [<b00817e4>] (worker_thread+0x1c0/0x2bc)
 [<b00817e4>] (worker_thread+0x1c0/0x2bc) from [<b008513c>] (kthread+0x7c/0x84)
 [<b008513c>] (kthread+0x7c/0x84) from [<b003b9d4>] (kernel_thread_exit+0x0/0x8)
 ------------[ cut here ]------------
 WARNING: at kernel/mutex.c:214 mutex_lock_nested+0xb0/0x32c()

Signed-off-by: Felipe Contreras <felipe.contreras@nokia.com>
---
 drivers/staging/tidspbridge/core/dsp-clock.c       |   49 ++++++++++++++++++-
 drivers/staging/tidspbridge/core/tiomap3430.c      |    3 +
 .../staging/tidspbridge/include/dspbridge/clk.h    |    4 ++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index aac06c0..b0aa831 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -115,6 +115,7 @@  static s8 get_clk_type(u8 id)
 void dsp_clk_exit(void)
 {
 	dsp_clock_disable_all(dsp_clocks);
+	dsp_clock_release_all();
 
 	clk_put(iva2_clk);
 	clk_put(ssi.sst_fck);
@@ -146,6 +147,45 @@  void dsp_clk_init(void)
 					ssi.sst_fck, ssi.ssr_fck, ssi.ick);
 }
 
+int dsp_clk_request(enum dsp_clk_id clk_id)
+{
+	struct omap_dm_timer *t;
+
+	if (get_clk_type(clk_id) != GPT_CLK)
+		return -EPERM;
+
+	t = omap_dm_timer_request_specific(DMT_ID(clk_id));
+	if (!t)
+		return -EPERM;
+	timer[clk_id - 1] = t;
+
+	return 0;
+}
+
+int dsp_clk_release(enum dsp_clk_id clk_id)
+{
+	if (get_clk_type(clk_id) != GPT_CLK)
+		return -EPERM;
+
+	omap_dm_timer_free(timer[clk_id - 1]);
+	timer[clk_id - 1] = NULL;
+
+	return 0;
+}
+
+void dsp_clock_release_all(void)
+{
+	int i;
+
+	/* release dm timer clocks */
+	for (i = 0; i < ARRAY_SIZE(timer); i++) {
+		if (!timer[i])
+			continue;
+		omap_dm_timer_free(timer[i]);
+		timer[i] = NULL;
+	}
+}
+
 #ifdef CONFIG_OMAP_MCBSP
 static void mcbsp_clk_prepare(bool flag, u8 id)
 {
@@ -252,8 +292,9 @@  int dsp_clk_enable(enum dsp_clk_id clk_id)
 		clk_enable(iva2_clk);
 		break;
 	case GPT_CLK:
-		timer[clk_id - 1] =
-				omap_dm_timer_request_specific(DMT_ID(clk_id));
+		if (!timer[clk_id - 1])
+			return -EINVAL;
+		omap_dm_timer_enable(timer[clk_id - 1]);
 		break;
 #ifdef CONFIG_OMAP_MCBSP
 	case MCBSP_CLK:
@@ -330,7 +371,9 @@  int dsp_clk_disable(enum dsp_clk_id clk_id)
 		clk_disable(iva2_clk);
 		break;
 	case GPT_CLK:
-		omap_dm_timer_free(timer[clk_id - 1]);
+		if (!timer[clk_id - 1])
+			return -EINVAL;
+		omap_dm_timer_disable(timer[clk_id - 1]);
 		break;
 #ifdef CONFIG_OMAP_MCBSP
 	case MCBSP_CLK:
diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index ecd865b..fbcea0b 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -512,6 +512,7 @@  static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
 		if (ul_load_monitor_timer != 0xFFFF) {
 			clk_cmd = (BPWR_ENABLE_CLOCK << MBX_PM_CLK_CMDSHIFT) |
 			    ul_load_monitor_timer;
+			dsp_clk_request(ul_load_monitor_timer - BPWR_GP_TIMER5 + DSP_CLK_GPT5);
 			dsp_peripheral_clk_ctrl(dev_context, &clk_cmd);
 		} else {
 			dev_dbg(bridge, "Not able to get the symbol for Load "
@@ -523,6 +524,7 @@  static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
 		if (ul_bios_gp_timer != 0xFFFF) {
 			clk_cmd = (BPWR_ENABLE_CLOCK << MBX_PM_CLK_CMDSHIFT) |
 			    ul_bios_gp_timer;
+			dsp_clk_request(ul_bios_gp_timer - BPWR_GP_TIMER5 + DSP_CLK_GPT5);
 			dsp_peripheral_clk_ctrl(dev_context, &clk_cmd);
 		} else {
 			dev_dbg(bridge,
@@ -691,6 +693,7 @@  static int bridge_brd_stop(struct bridge_dev_context *dev_ctxt)
 			OMAP3430_RST3_IVA2, OMAP3430_IVA2_MOD, RM_RSTCTRL);
 
 	dsp_clock_disable_all(dev_context->dsp_per_clks);
+	dsp_clock_release_all();
 	dsp_clk_disable(DSP_CLK_IVA2);
 
 	return status;
diff --git a/drivers/staging/tidspbridge/include/dspbridge/clk.h b/drivers/staging/tidspbridge/include/dspbridge/clk.h
index 685341c..14c5d7d 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/clk.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/clk.h
@@ -62,6 +62,10 @@  extern void dsp_clk_exit(void);
  */
 extern void dsp_clk_init(void);
 
+int dsp_clk_request(enum dsp_clk_id clk_id);
+int dsp_clk_release(enum dsp_clk_id clk_id);
+void dsp_clock_release_all(void);
+
 void dsp_gpt_wait_overflow(short int clk_id, unsigned int load);
 
 /*
-- 
1.7.6