[PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Deepak Chitriki]

From: Fernando Guzman Lugo <x0095840@ti.com>

This patch fixes the BUG schedule/sleep while atomic which
was in the IO_DPC function, this function runs as a tasklet
and called a function that could be scheduled (OPP change).
we use in_interrupt to detect this and handle accordingly

Original Patch by Fernando:
http://dev.omapzoom.org/?p=tidspbridge/kernel-dspbridge.git;a=commitdiff;h=2dcd1964a138c85b5545f687f96c96b489fe4a00

Cc: Ameya Palande <ameya.palande@nokia.com>
Cc: Felipe Contreras <felipe.contreras@nokia.com>
Cc: Hiroshi Doyu <hiroshi.doyu@nokia.com>
Cc: Nishanth Menon <nm@ti.com>
Cc: Omar Ramirez <omap.ramirez@ti.com>

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
Signed-off-by: Deepak Chitriki <deepak.chitriki@ti.com>
---
 drivers/dsp/bridge/wmd/_tiomap_pwr.h    |    7 +++++++
 drivers/dsp/bridge/wmd/io_sm.c          |    2 +-
 drivers/dsp/bridge/wmd/tiomap3430_pwr.c |   31 +++++++++++++++++++++++++++++++
 drivers/dsp/bridge/wmd/tiomap_sm.c      |   13 ++-----------
 4 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/dsp/bridge/wmd/_tiomap_pwr.h b/drivers/dsp/bridge/wmd/_tiomap_pwr.h
index da2e7d9..54d7ca4 100644
--- a/drivers/dsp/bridge/wmd/_tiomap_pwr.h
+++ b/drivers/dsp/bridge/wmd/_tiomap_pwr.h
@@ -89,5 +89,12 @@ DSP_STATUS DSP_PeripheralClocks_Enable(struct WMD_DEV_CONTEXT *pDevContext,
  */
 void DSPClkWakeupEventCtrl(u32 ClkId, bool enable);
 
+/**
+ * tiomap3430_bump_dsp_opp_level() - bump up opp if required
+ *
+ * if the system is at a minimum opp, request for higher opp
+ */
+DSP_STATUS tiomap3430_bump_dsp_opp_level(void);
+
 #endif				/* _TIOMAP_PWR_ */
 
diff --git a/drivers/dsp/bridge/wmd/io_sm.c b/drivers/dsp/bridge/wmd/io_sm.c
index 79a714a..3481beb 100644
--- a/drivers/dsp/bridge/wmd/io_sm.c
+++ b/drivers/dsp/bridge/wmd/io_sm.c
@@ -1132,7 +1132,7 @@ void IO_Schedule(struct IO_MGR *pIOMgr)
 	spin_lock_irqsave(&pIOMgr->dpc_lock, flags);
 	pIOMgr->dpc_req++;
 	spin_unlock_irqrestore(&pIOMgr->dpc_lock, flags);
-
+	tiomap3430_bump_dsp_opp_level();
 	/* Schedule DPC */
 	tasklet_schedule(&pIOMgr->dpc_tasklet);
 }
diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
index 94b399f..54cba9f 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
@@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
 	break;
 	}
 }
+
+/**
+ * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
+ *
+ * if we need a higher opp index, request for the same
+ */
+DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
+{
+#ifndef CONFIG_BRIDGE_DVFS
+	u32 opplevel;
+
+	struct dspbridge_platform_data *pdata =
+			omap_dspbridge_dev->dev.platform_data;
+
+	if (pdata->dsp_get_opp) {
+		opplevel = (*pdata->dsp_get_opp)();
+
+		/*
+		 * If OPP is at minimum level, increase it before waking
+		 * up the DSP.
+		 */
+		if (opplevel == 1 && pdata->dsp_set_min_opp) {
+			(*pdata->dsp_set_min_opp)(opp_level + 1);
+			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
+				"the vdd1 constraint level to %d before "
+				"waking DSP \n", opp_level + 1);
+		}
+	}
+#endif
+	return DSP_SOK;
+}
diff --git a/drivers/dsp/bridge/wmd/tiomap_sm.c b/drivers/dsp/bridge/wmd/tiomap_sm.c
index b04ed6d..1d2e5d7 100644
--- a/drivers/dsp/bridge/wmd/tiomap_sm.c
+++ b/drivers/dsp/bridge/wmd/tiomap_sm.c
@@ -96,11 +96,6 @@ DSP_STATUS CHNLSM_DisableInterrupt(struct WMD_DEV_CONTEXT *pDevContext)
 DSP_STATUS CHNLSM_InterruptDSP2(struct WMD_DEV_CONTEXT *pDevContext,
 				u16 wMbVal)
 {
-#ifdef CONFIG_BRIDGE_DVFS
-	struct dspbridge_platform_data *pdata =
-		omap_dspbridge_dev->dev.platform_data;
-	u32 opplevel = 0;
-#endif
 	struct CFG_HOSTRES resources;
 	DSP_STATUS status = DSP_SOK;
 	unsigned long timeout;
@@ -114,12 +109,8 @@ DSP_STATUS CHNLSM_InterruptDSP2(struct WMD_DEV_CONTEXT *pDevContext,
 	if (pDevContext->dwBrdState == BRD_DSP_HIBERNATION ||
 	    pDevContext->dwBrdState == BRD_HIBERNATION) {
 #ifdef CONFIG_BRIDGE_DVFS
-		if (pdata->dsp_get_opp)
-			opplevel = (*pdata->dsp_get_opp)();
-		if (opplevel == VDD1_OPP1) {
-			if (pdata->dsp_set_min_opp)
-				(*pdata->dsp_set_min_opp)(VDD1_OPP2);
-		}
+		if (!in_interrupt())
+			tiomap3430_bump_dsp_opp_level();
 #endif
 		/* Restart the peripheral clocks */
 		DSP_PeripheralClocks_Enable(pDevContext, NULL);
-- 
1.6.0.4

RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Ramirez Luna, Omar]

>From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM
>
>From: Fernando Guzman Lugo <x0095840@ti.com>
>
>This patch fixes the BUG schedule/sleep while atomic which
>was in the IO_DPC function, this function runs as a tasklet
>and called a function that could be scheduled (OPP change).
>we use in_interrupt to detect this and handle accordingly
>
>Original Patch by Fernando:
>http://dev.omapzoom.org/?p=tidspbridge/kernel-
>dspbridge.git;a=commitdiff;h=2dcd1964a138c85b5545f687f96c96b489fe4a00

This patch was meant to be reworked before sending.

>
>Cc: Ameya Palande <ameya.palande@nokia.com>
>Cc: Felipe Contreras <felipe.contreras@nokia.com>
>Cc: Hiroshi Doyu <hiroshi.doyu@nokia.com>
>Cc: Nishanth Menon <nm@ti.com>
>Cc: Omar Ramirez <omap.ramirez@ti.com>

mail is wrong :) it would be a nice nickname though

>
>Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>Signed-off-by: Deepak Chitriki <deepak.chitriki@ti.com>
>---
> drivers/dsp/bridge/wmd/_tiomap_pwr.h    |    7 +++++++
> drivers/dsp/bridge/wmd/io_sm.c          |    2 +-
> drivers/dsp/bridge/wmd/tiomap3430_pwr.c |   31 +++++++++++++++++++++++++++++++
> drivers/dsp/bridge/wmd/tiomap_sm.c      |   13 ++-----------
> 4 files changed, 41 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/dsp/bridge/wmd/_tiomap_pwr.h b/drivers/dsp/bridge/wmd/_tiomap_pwr.h
>index da2e7d9..54d7ca4 100644
>--- a/drivers/dsp/bridge/wmd/_tiomap_pwr.h
>+++ b/drivers/dsp/bridge/wmd/_tiomap_pwr.h
>@@ -89,5 +89,12 @@ DSP_STATUS DSP_PeripheralClocks_Enable(struct WMD_DEV_CONTEXT *pDevContext,
>  */
> void DSPClkWakeupEventCtrl(u32 ClkId, bool enable);
>
>+/**
>+ * tiomap3430_bump_dsp_opp_level() - bump up opp if required
>+ *
>+ * if the system is at a minimum opp, request for higher opp
>+ */
>+DSP_STATUS tiomap3430_bump_dsp_opp_level(void);

Why DSP_STATUS if nobody checks?

>+
> #endif				/* _TIOMAP_PWR_ */
>
>diff --git a/drivers/dsp/bridge/wmd/io_sm.c b/drivers/dsp/bridge/wmd/io_sm.c
>index 79a714a..3481beb 100644
>--- a/drivers/dsp/bridge/wmd/io_sm.c
>+++ b/drivers/dsp/bridge/wmd/io_sm.c
>@@ -1132,7 +1132,7 @@ void IO_Schedule(struct IO_MGR *pIOMgr)
> 	spin_lock_irqsave(&pIOMgr->dpc_lock, flags);
> 	pIOMgr->dpc_req++;
> 	spin_unlock_irqrestore(&pIOMgr->dpc_lock, flags);
>-
>+	tiomap3430_bump_dsp_opp_level();
> 	/* Schedule DPC */
> 	tasklet_schedule(&pIOMgr->dpc_tasklet);
> }

This looks like:
	Increase DPC counts
	* Bump opp
	Schedule DPC

Should look better:
	* Bump opp
	Increase DPC counts
	Schedule DPC

>diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>index 94b399f..54cba9f 100644
>--- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>+++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>@@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
> 	break;
> 	}
> }
>+
>+/**
>+ * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
>+ *
>+ * if we need a higher opp index, request for the same
>+ */
>+DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
>+{
>+#ifndef CONFIG_BRIDGE_DVFS

Basically if DVFS is defined nothing is done, this was wrong in the original patch (like I mentioned offline).

>+	u32 opplevel;
>+
>+	struct dspbridge_platform_data *pdata =
>+			omap_dspbridge_dev->dev.platform_data;
>+
>+	if (pdata->dsp_get_opp) {
>+		opplevel = (*pdata->dsp_get_opp)();
>+
>+		/*
>+		 * If OPP is at minimum level, increase it before waking
>+		 * up the DSP.
>+		 */
>+		if (opplevel == 1 && pdata->dsp_set_min_opp) {
>+			(*pdata->dsp_set_min_opp)(opp_level + 1);
>+			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
>+				"the vdd1 constraint level to %d before "
>+				"waking DSP \n", opp_level + 1);
>+		}
>+	}
>+#endif
>+	return DSP_SOK;
>+}

Since we are reworking all of this can be changed (u32, opplevel == MAGIC_NUM), besides this was specific to 3430.

>diff --git a/drivers/dsp/bridge/wmd/tiomap_sm.c b/drivers/dsp/bridge/wmd/tiomap_sm.c
>index b04ed6d..1d2e5d7 100644
>--- a/drivers/dsp/bridge/wmd/tiomap_sm.c
>+++ b/drivers/dsp/bridge/wmd/tiomap_sm.c
>@@ -96,11 +96,6 @@ DSP_STATUS CHNLSM_DisableInterrupt(struct WMD_DEV_CONTEXT *pDevContext)
> DSP_STATUS CHNLSM_InterruptDSP2(struct WMD_DEV_CONTEXT *pDevContext,
> 				u16 wMbVal)
> {
>-#ifdef CONFIG_BRIDGE_DVFS
>-	struct dspbridge_platform_data *pdata =
>-		omap_dspbridge_dev->dev.platform_data;
>-	u32 opplevel = 0;
>-#endif
> 	struct CFG_HOSTRES resources;
> 	DSP_STATUS status = DSP_SOK;
> 	unsigned long timeout;
>@@ -114,12 +109,8 @@ DSP_STATUS CHNLSM_InterruptDSP2(struct WMD_DEV_CONTEXT *pDevContext,
> 	if (pDevContext->dwBrdState == BRD_DSP_HIBERNATION ||
> 	    pDevContext->dwBrdState == BRD_HIBERNATION) {
> #ifdef CONFIG_BRIDGE_DVFS
>-		if (pdata->dsp_get_opp)
>-			opplevel = (*pdata->dsp_get_opp)();
>-		if (opplevel == VDD1_OPP1) {
>-			if (pdata->dsp_set_min_opp)
>-				(*pdata->dsp_set_min_opp)(VDD1_OPP2);
>-		}
>+		if (!in_interrupt())
>+			tiomap3430_bump_dsp_opp_level();
> #endif
> 		/* Restart the peripheral clocks */
> 		DSP_PeripheralClocks_Enable(pDevContext, NULL);
>--
>1.6.0.4
>
>--
>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

Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Nishanth Menon]

Ramirez Luna, Omar had written, on 01/21/2010 11:43 AM, the following:
>> From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM
>>
[...]

>> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>> index 94b399f..54cba9f 100644
>> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>> @@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
>> 	break;
>> 	}
>> }
>> +
>> +/**
>> + * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
>> + *
>> + * if we need a higher opp index, request for the same
>> + */
>> +DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
>> +{
>> +#ifndef CONFIG_BRIDGE_DVFS
> 
> Basically if DVFS is defined nothing is done, this was wrong in the original patch (like I mentioned offline).
> 
>> +	u32 opplevel;
>> +
>> +	struct dspbridge_platform_data *pdata =
>> +			omap_dspbridge_dev->dev.platform_data;
>> +
>> +	if (pdata->dsp_get_opp) {
>> +		opplevel = (*pdata->dsp_get_opp)();
>> +
>> +		/*
>> +		 * If OPP is at minimum level, increase it before waking
>> +		 * up the DSP.
>> +		 */
>> +		if (opplevel == 1 && pdata->dsp_set_min_opp) {
>> +			(*pdata->dsp_set_min_opp)(opp_level + 1);
>> +			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
>> +				"the vdd1 constraint level to %d before "
>> +				"waking DSP \n", opp_level + 1);
>> +		}
>> +	}
>> +#endif
>> +	return DSP_SOK;
>> +}
> 
> Since we are reworking all of this can be changed (u32, opplevel == MAGIC_NUM), besides this was specific to 3430.
							^^^^^^^^^^^^^^^
opplevel==1 is independent of 3430.. index 1 has to be the lowest right?
[...]

-- 
Regards,
Nishanth Menon

RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Ramirez Luna, Omar]

>From: Menon, Nishanth on Thursday, January 21, 2010 11:47 AM
>
>Ramirez Luna, Omar had written, on 01/21/2010 11:43 AM, the following:
>>> From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM
>>>
>[...]
>
>>> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>> index 94b399f..54cba9f 100644
>>> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>> @@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
>>> 	break;
>>> 	}
>>> }
>>> +
>>> +/**
>>> + * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
>>> + *
>>> + * if we need a higher opp index, request for the same
>>> + */
>>> +DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
>>> +{
>>> +#ifndef CONFIG_BRIDGE_DVFS
>>
>> Basically if DVFS is defined nothing is done, this was wrong in the original patch (like I
>mentioned offline).
>>
>>> +	u32 opplevel;
>>> +
>>> +	struct dspbridge_platform_data *pdata =
>>> +			omap_dspbridge_dev->dev.platform_data;
>>> +
>>> +	if (pdata->dsp_get_opp) {
>>> +		opplevel = (*pdata->dsp_get_opp)();
>>> +
>>> +		/*
>>> +		 * If OPP is at minimum level, increase it before waking
>>> +		 * up the DSP.
>>> +		 */
>>> +		if (opplevel == 1 && pdata->dsp_set_min_opp) {
>>> +			(*pdata->dsp_set_min_opp)(opp_level + 1);
>>> +			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
>>> +				"the vdd1 constraint level to %d before "
>>> +				"waking DSP \n", opp_level + 1);
>>> +		}
>>> +	}
>>> +#endif
>>> +	return DSP_SOK;
>>> +}
>>
>> Since we are reworking all of this can be changed (u32, opplevel == MAGIC_NUM), besides this was
>specific to 3430.
>							^^^^^^^^^^^^^^^
>opplevel==1 is independent of 3430.. index 1 has to be the lowest right?

You are right, I meant opplevel == VDD1_OPP or similar.

But the entire bumping thing is specific to 3430 IMHO.

- omar

Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Nishanth Menon]

Ramirez Luna, Omar had written, on 01/21/2010 11:57 AM, the following:
>> From: Menon, Nishanth on Thursday, January 21, 2010 11:47 AM
>>
>> Ramirez Luna, Omar had written, on 01/21/2010 11:43 AM, the following:
>>>> From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM
>>>>
>> [...]
>>
>>>> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>> index 94b399f..54cba9f 100644
>>>> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>> @@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
>>>> 	break;
>>>> 	}
>>>> }
>>>> +
>>>> +/**
>>>> + * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
>>>> + *
>>>> + * if we need a higher opp index, request for the same
>>>> + */
>>>> +DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
>>>> +{
>>>> +#ifndef CONFIG_BRIDGE_DVFS
>>> Basically if DVFS is defined nothing is done, this was wrong in the original patch (like I
>> mentioned offline).
>>>> +	u32 opplevel;
>>>> +
>>>> +	struct dspbridge_platform_data *pdata =
>>>> +			omap_dspbridge_dev->dev.platform_data;
>>>> +
>>>> +	if (pdata->dsp_get_opp) {
>>>> +		opplevel = (*pdata->dsp_get_opp)();
>>>> +
>>>> +		/*
>>>> +		 * If OPP is at minimum level, increase it before waking
>>>> +		 * up the DSP.
>>>> +		 */
>>>> +		if (opplevel == 1 && pdata->dsp_set_min_opp) {
>>>> +			(*pdata->dsp_set_min_opp)(opp_level + 1);
>>>> +			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
>>>> +				"the vdd1 constraint level to %d before "
>>>> +				"waking DSP \n", opp_level + 1);
>>>> +		}
>>>> +	}
>>>> +#endif
>>>> +	return DSP_SOK;
>>>> +}
>>> Since we are reworking all of this can be changed (u32, opplevel == MAGIC_NUM), besides this was
>> specific to 3430.
>> 							^^^^^^^^^^^^^^^
>> opplevel==1 is independent of 3430.. index 1 has to be the lowest right?
> 
> You are right, I meant opplevel == VDD1_OPP or similar.
arrggh... NOoooooo more #ifdefs, I would rather see my patch for min,max 
opps pushed in(after a rework and add a nominal_opp, min_opp params to 
pdata and use it :D) - that way all silicon variances are handled in 
arch/arm/mach-omap2 rather than drivers/dsp/bridge.

> 
> But the entire bumping thing is specific to 3430 IMHO.
> 
if I get your point, you would rather see that this bump up disappear? 
OR is there a reason for saying this 3430 specific? on 3630, there are 
upto 4 OPPs(on the 1Ghz device), OR is this a errata workaround that we 
have here? the original commit is lacking in info about this.


-- 
Regards,
Nishanth Menon

RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Ramirez Luna, Omar]

>From: Menon, Nishanth on Thursday, January 21, 2010 12:03 PM
>
>Ramirez Luna, Omar had written, on 01/21/2010 11:57 AM, the following:
>>> From: Menon, Nishanth on Thursday, January 21, 2010 11:47 AM
>>>
>>> Ramirez Luna, Omar had written, on 01/21/2010 11:43 AM, the following:
>>>>> From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM
>>>>>
>>> [...]
>>>
>>>>> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>>> index 94b399f..54cba9f 100644
>>>>> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>>> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>>> @@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
>>>>> 	break;
>>>>> 	}
>>>>> }
>>>>> +
>>>>> +/**
>>>>> + * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
>>>>> + *
>>>>> + * if we need a higher opp index, request for the same
>>>>> + */
>>>>> +DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
>>>>> +{
>>>>> +#ifndef CONFIG_BRIDGE_DVFS
>>>> Basically if DVFS is defined nothing is done, this was wrong in the original patch (like I
>>> mentioned offline).
>>>>> +	u32 opplevel;
>>>>> +
>>>>> +	struct dspbridge_platform_data *pdata =
>>>>> +			omap_dspbridge_dev->dev.platform_data;
>>>>> +
>>>>> +	if (pdata->dsp_get_opp) {
>>>>> +		opplevel = (*pdata->dsp_get_opp)();
>>>>> +
>>>>> +		/*
>>>>> +		 * If OPP is at minimum level, increase it before waking
>>>>> +		 * up the DSP.
>>>>> +		 */
>>>>> +		if (opplevel == 1 && pdata->dsp_set_min_opp) {
>>>>> +			(*pdata->dsp_set_min_opp)(opp_level + 1);
>>>>> +			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
>>>>> +				"the vdd1 constraint level to %d before "
>>>>> +				"waking DSP \n", opp_level + 1);
>>>>> +		}
>>>>> +	}
>>>>> +#endif
>>>>> +	return DSP_SOK;
>>>>> +}
>>>> Since we are reworking all of this can be changed (u32, opplevel == MAGIC_NUM), besides this was
>>> specific to 3430.
>>> 							^^^^^^^^^^^^^^^
>>> opplevel==1 is independent of 3430.. index 1 has to be the lowest right?
>>
>> You are right, I meant opplevel == VDD1_OPP or similar.
>arrggh... NOoooooo more #ifdefs, I would rather see my patch for min,max
>opps pushed in(after a rework and add a nominal_opp, min_opp params to
>pdata and use it :D) - that way all silicon variances are handled in
>arch/arm/mach-omap2 rather than drivers/dsp/bridge.

Need to see the patch before commenting; agree to no #def's, thought
definition was there already.

>
>>
>> But the entire bumping thing is specific to 3430 IMHO.
>>
>if I get your point, you would rather see that this bump up disappear?

Only if it is not needed.

>OR is there a reason for saying this 3430 specific? on 3630, there are
>upto 4 OPPs(on the 1Ghz device)

Doesn't matter if you have a 100 opps, no point if you have to bump from
1 to 2 and don't need it. Apparently it is needed for 3430.

> OR is this a errata workaround that we have here? the original commit
> is lacking in info about this.

This patch moves the code that bumps the opp outside an interrupt or
atomic context only (would be nice to add a good commit message so it can
be clear).

Code to bump the opp was there already, fixing a crash when waking up
the dsp at OPP1; haven't found the errata, if there is, there might be
one that says this also applies for 3630.

- omar

Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Nishanth Menon]

Ramirez Luna, Omar had written, on 01/21/2010 01:46 PM, the following:
>> From: Menon, Nishanth on Thursday, January 21, 2010 12:03 PM
>>
>> Ramirez Luna, Omar had written, on 01/21/2010 11:57 AM, the following:
>>>> From: Menon, Nishanth on Thursday, January 21, 2010 11:47 AM
>>>>
>>>> Ramirez Luna, Omar had written, on 01/21/2010 11:43 AM, the following:
>>>>>> From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM
>>>>>>
>>>> [...]
>>>>
>>>>>> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>>>> index 94b399f..54cba9f 100644
>>>>>> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>>>> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>>>>>> @@ -806,3 +806,34 @@ void DSPClkWakeupEventCtrl(u32 ClkId, bool enable)
>>>>>> 	break;
>>>>>> 	}
>>>>>> }
>>>>>> +
>>>>>> +/**
>>>>>> + * tiomap3430_bump_dsp_opp_level() - bump up the opp if at minimum
>>>>>> + *
>>>>>> + * if we need a higher opp index, request for the same
>>>>>> + */
>>>>>> +DSP_STATUS tiomap3430_bump_dsp_opp_level(void)
>>>>>> +{
>>>>>> +#ifndef CONFIG_BRIDGE_DVFS
>>>>> Basically if DVFS is defined nothing is done, this was wrong in the original patch (like I
>>>> mentioned offline).
>>>>>> +	u32 opplevel;
>>>>>> +
>>>>>> +	struct dspbridge_platform_data *pdata =
>>>>>> +			omap_dspbridge_dev->dev.platform_data;
>>>>>> +
>>>>>> +	if (pdata->dsp_get_opp) {
>>>>>> +		opplevel = (*pdata->dsp_get_opp)();
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * If OPP is at minimum level, increase it before waking
>>>>>> +		 * up the DSP.
>>>>>> +		 */
>>>>>> +		if (opplevel == 1 && pdata->dsp_set_min_opp) {
>>>>>> +			(*pdata->dsp_set_min_opp)(opp_level + 1);
>>>>>> +			DBG_Trace(DBG_LEVEL7, "CHNLSM_InterruptDSP: Setting "
>>>>>> +				"the vdd1 constraint level to %d before "
>>>>>> +				"waking DSP \n", opp_level + 1);
>>>>>> +		}
>>>>>> +	}
>>>>>> +#endif
>>>>>> +	return DSP_SOK;
>>>>>> +}
>>>>> Since we are reworking all of this can be changed (u32, opplevel == MAGIC_NUM), besides this was
>>>> specific to 3430.
>>>> 							^^^^^^^^^^^^^^^
>>>> opplevel==1 is independent of 3430.. index 1 has to be the lowest right?
>>> You are right, I meant opplevel == VDD1_OPP or similar.
>> arrggh... NOoooooo more #ifdefs, I would rather see my patch for min,max
>> opps pushed in(after a rework and add a nominal_opp, min_opp params to
>> pdata and use it :D) - that way all silicon variances are handled in
>> arch/arm/mach-omap2 rather than drivers/dsp/bridge.
> 
> Need to see the patch before commenting; agree to no #def's, thought
> definition was there already.
Hoping to isolate direct index accesses to pdata. that would be cleaner. 
VDD1_OPP1, OPP2 definitions are there in pm branch today to keep SRF 
happy, but in future, it will go away.. and it is a simple thing anyways..

> 
>>> But the entire bumping thing is specific to 3430 IMHO.
>>>
>> if I get your point, you would rather see that this bump up disappear?
> 
> Only if it is not needed.
> 
>> OR is there a reason for saying this 3430 specific? on 3630, there are
>> upto 4 OPPs(on the 1Ghz device)
> 
> Doesn't matter if you have a 100 opps, no point if you have to bump from
> 1 to 2 and don't need it. Apparently it is needed for 3430.
> 
>> OR is this a errata workaround that we have here? the original commit
>> is lacking in info about this.
> 
> This patch moves the code that bumps the opp outside an interrupt or
> atomic context only (would be nice to add a good commit message so it can
> be clear).
> 
> Code to bump the opp was there already, fixing a crash when waking up
> the dsp at OPP1; haven't found the errata, if there is, there might be
> one that says this also applies for 3630.
> 
Now you have me really curious - maybe we need to understand the need to 
bump the OPP up for loading to DSP.. we could create a patch to remove 
the entire logic out and see?

-- 
Regards,
Nishanth Menon