From mboxrd@z Thu Jan 1 00:00:00 1970 From: Omar Ramirez Luna Subject: Re: [PATCH 1/2] DSPBRIDGE: Use enable_off_mode variable instead of dsp_test_sleepstate Date: Thu, 28 Jan 2010 10:58:15 -0600 Message-ID: <4B61C227.4040709@ti.com> References: <1264241986-29071-1-git-send-email-felipe.contreras@gmail.com> <1264241986-29071-2-git-send-email-felipe.contreras@gmail.com> <4B5F6B51.6050607@ti.com> <4B60DE5D.1090101@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from devils.ext.ti.com ([198.47.26.153]:40035 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752108Ab0A1Q6S (ORCPT ); Thu, 28 Jan 2010 11:58:18 -0500 In-Reply-To: <4B60DE5D.1090101@ti.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: "Menon, Nishanth" Cc: Felipe Contreras , linux-omap , Jouni Hogander , Hiroshi DOYU 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 >>> >>> Signed-off-by: Jouni Hogander >>> Signed-off-by: Hiroshi DOYU >> >> 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 >>> >>> #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