Re: [PATCH 1/2] DSPBRIDGE: Use enable_off_mode variable instead of dsp_test_sleepstate

Nishanth Menon
2010-01-28T00:46:25Z

Omar Ramirez Luna had written, on 01/26/2010 04:23 PM, the following:
> Hi,
>
> On 1/23/2010 4:19 AM, Felipe Contreras wrote:
>> From: Jouni Hogander<jouni.hogander@nokia.com>
>>
>> Signed-off-by: Jouni Hogander<jouni.hogander@nokia.com>
>> Signed-off-by: Hiroshi DOYU<Hiroshi.DOYU@nokia.com>
>
> This patch is missing to export enable_off_mode otherwise it will fail
> while linking.
>
> FTR, I don't like to be exporting symbols of other subsystems, guess the
> same applies to:
>
> http://marc.info/?l=linux-omap&m=126450677527042&w=2
looking at the patch itself few questions arise:

>
>> ---
>> drivers/dsp/bridge/rmgr/drv_interface.c | 5 -----
>> drivers/dsp/bridge/wmd/tiomap3430_pwr.c | 9 ++++-----
>> 2 files changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
>> index a02a32a..b20f77e 100644
>> --- a/drivers/dsp/bridge/rmgr/drv_interface.c
>> +++ b/drivers/dsp/bridge/rmgr/drv_interface.c
>> @@ -78,8 +78,6 @@ s32 dsp_debug;
>>
>> struct platform_device *omap_dspbridge_dev;
>>
>> -/* This is a test variable used by Bridge to test different sleep states */
>> -s32 dsp_test_sleepstate;
>> struct bridge_dev {
>> struct cdev cdev;
>> };
>> @@ -129,9 +127,6 @@ module_param(dsp_debug, int, 0);
>> MODULE_PARM_DESC(dsp_debug, "Wait after loading DSP image. default = false");
>> #endif
>>
>> -module_param(dsp_test_sleepstate, int, 0);
>> -MODULE_PARM_DESC(dsp_test_sleepstate, "DSP Sleep state = 0");
>> -
>> module_param(base_img, charp, 0);
>> MODULE_PARM_DESC(base_img, "DSP base image, default = NULL");
>>
>> diff --git a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>> index 084f406..c0e0994 100644
>> --- a/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>> +++ b/drivers/dsp/bridge/wmd/tiomap3430_pwr.c
>> @@ -54,10 +54,9 @@
>> #include<mach-omap2/cm-regbits-34xx.h>
>>
>> #ifdef CONFIG_PM
>> -extern s32 dsp_test_sleepstate;
>> +extern unsigned short enable_off_mode;
>> #endif
>> extern struct MAILBOX_CONTEXT mboxsetting;
>> -
>> /*
>> * ======== handle_constraints_set ========
>> * Sets new DSP constraint
>> @@ -196,7 +195,7 @@ DSP_STATUS SleepDSP(struct WMD_DEV_CONTEXT *pDevContext, IN u32 dwCmd,
>> switch (pDevContext->dwBrdState) {
>> case BRD_RUNNING:
>> status = HW_MBOX_saveSettings(resources.dwMboxBase);
>> - if (dsp_test_sleepstate == HW_PWR_STATE_OFF) {
>> + if (enable_off_mode) {
>> CHNLSM_InterruptDSP2(pDevContext,
>> MBX_PM_DSPHIBERNATE);
>> DBG_Trace(DBG_LEVEL7,
>> @@ -211,7 +210,7 @@ DSP_STATUS SleepDSP(struct WMD_DEV_CONTEXT *pDevContext, IN u32 dwCmd,
>> break;
>> case BRD_RETENTION:
>> status = HW_MBOX_saveSettings(resources.dwMboxBase);
>> - if (dsp_test_sleepstate == HW_PWR_STATE_OFF) {
>> + if (enable_off_mode) {
>> CHNLSM_InterruptDSP2(pDevContext,
>> MBX_PM_DSPHIBERNATE);
>> targetPwrState = HW_PWR_STATE_OFF;
>> @@ -261,7 +260,7 @@ DSP_STATUS SleepDSP(struct WMD_DEV_CONTEXT *pDevContext, IN u32 dwCmd,
>> pwrState);
>>
>> /* Update the Bridger Driver state */
>> - if (dsp_test_sleepstate == HW_PWR_STATE_OFF)
>> + if (enable_off_mode)
>> pDevContext->dwBrdState = BRD_HIBERNATION;
>> else
>> pDevContext->dwBrdState = BRD_RETENTION;
IMHO it changes behavior:
before:
IF dsp_test_sleepstate was set to HW_PWR_STATE_OFF DSP was told to go to
hibernate every time SleepDSP is called else the mbox event
default behavior is to ask BIOS for retention.

with the patch:
If sysfs entry(meant for MPU) has enabled off mode, THEN DSP will be
send to off mode every time we get message to SleepDSP. In other words
MPU and DSP retention/OFF are tied together.

The questions then are:
a) why was this done so?
b) is the intention of test_sleepstate param meant only as a test?
c) was it intended that SleepDSP should put IVA to sleep no matter what
MPU decisions are?

--
Regards,
Nishanth Menon