RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic
From: Chitriki Rudramuni, Deepak on Wednesday, January 20, 2010 10:01 PM From: Fernando Guzman Lugo x0095...@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.pala...@nokia.com Cc: Felipe Contreras felipe.contre...@nokia.com Cc: Hiroshi Doyu hiroshi.d...@nokia.com Cc: Nishanth Menon n...@ti.com Cc: Omar Ramirez omap.rami...@ti.com mail is wrong :) it would be a nice nickname though Signed-off-by: Fernando Guzman Lugo x0095...@ti.com Signed-off-by: Deepak Chitriki deepak.chitr...@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) -
Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic
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... NOoo 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic
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... NOoo 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic
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... NOoo 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html