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

Omar Ramirez Luna
2010-01-28T16:58:18Z

On 1/27/2010 6:46 PM, Menon, Nishanth wrote:
> 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.

dsp_test_sleepstate is initialized always to 0

>
> 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.

(-->) means mbx bridge to dsp
(...) means do not disturb dsp while sleeping

Suspend coming for this scenarios:

if DSP = ON, enable_off_mode = 1 --> OFF (HIB)
if DSP = ON, enable_off_mode = 0 --> RET
if DSP = OFF, enable_off_mode = 1 ...
if DSP = OFF, enable_off_mode = 0 ...

>
> The questions then are:
> a) why was this done so?

always target the next lower state (according to the specified), but DO
NOT disturb the dsp if it already went to sleep

> b) is the intention of test_sleepstate param meant only as a test?

should be clear enough:
/* This is a test variable used by Bridge to test different sleep states */

> c) was it intended that SleepDSP should put IVA to sleep no matter what
> MPU decisions are?

if the DSP is already OFF and MPU wants to be in RET why MPU can be
smart enough to tell ok this guy went down, print a message like "this
might be wrong" but it is acceptable.

the other tendency claims that the dsp should be woken and then told to
go to next-power-state (if OFF not enabled).

bottom line is that we listen to what mpu wants if the dsp is awake but
ignore if the dsp has gone to off.

- omar