* [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic @ 2010-01-21 4:01 Deepak Chitriki 2010-01-21 17:43 ` Ramirez Luna, Omar 0 siblings, 1 reply; 7+ messages in thread From: Deepak Chitriki @ 2010-01-21 4:01 UTC (permalink / raw) To: linux-omap Cc: Fernando Guzman Lugo, Ameya Palande, Felipe Contreras, Hiroshi Doyu, Nishanth Menon, Omar Ramirez, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic 2010-01-21 4:01 [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic Deepak Chitriki @ 2010-01-21 17:43 ` Ramirez Luna, Omar 2010-01-21 17:47 ` Nishanth Menon 0 siblings, 1 reply; 7+ messages in thread From: Ramirez Luna, Omar @ 2010-01-21 17:43 UTC (permalink / raw) To: Chitriki Rudramuni, Deepak, linux-omap Cc: Guzman Lugo, Fernando, Ameya Palande, Felipe Contreras, Hiroshi Doyu, Menon, Nishanth >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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic 2010-01-21 17:43 ` Ramirez Luna, Omar @ 2010-01-21 17:47 ` Nishanth Menon 2010-01-21 17:57 ` Ramirez Luna, Omar 0 siblings, 1 reply; 7+ messages in thread From: Nishanth Menon @ 2010-01-21 17:47 UTC (permalink / raw) To: Ramirez Luna, Omar Cc: Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando, Ameya Palande, Felipe Contreras, Hiroshi Doyu 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic 2010-01-21 17:47 ` Nishanth Menon @ 2010-01-21 17:57 ` Ramirez Luna, Omar 2010-01-21 18:03 ` Nishanth Menon 0 siblings, 1 reply; 7+ messages in thread From: Ramirez Luna, Omar @ 2010-01-21 17:57 UTC (permalink / raw) To: Menon, Nishanth Cc: Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando, Ameya Palande, Felipe Contreras, Hiroshi Doyu >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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic 2010-01-21 17:57 ` Ramirez Luna, Omar @ 2010-01-21 18:03 ` Nishanth Menon 2010-01-21 19:46 ` Ramirez Luna, Omar 0 siblings, 1 reply; 7+ messages in thread From: Nishanth Menon @ 2010-01-21 18:03 UTC (permalink / raw) To: Ramirez Luna, Omar Cc: Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando, Ameya Palande, Felipe Contreras, Hiroshi Doyu 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic 2010-01-21 18:03 ` Nishanth Menon @ 2010-01-21 19:46 ` Ramirez Luna, Omar 2010-01-21 19:53 ` Nishanth Menon 0 siblings, 1 reply; 7+ messages in thread From: Ramirez Luna, Omar @ 2010-01-21 19:46 UTC (permalink / raw) To: Menon, Nishanth Cc: Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando, Ameya Palande, Felipe Contreras, Hiroshi Doyu >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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic 2010-01-21 19:46 ` Ramirez Luna, Omar @ 2010-01-21 19:53 ` Nishanth Menon 0 siblings, 0 replies; 7+ messages in thread From: Nishanth Menon @ 2010-01-21 19:53 UTC (permalink / raw) To: Ramirez Luna, Omar Cc: Chitriki Rudramuni, Deepak, linux-omap, Guzman Lugo, Fernando, Ameya Palande, Felipe Contreras, Hiroshi Doyu 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-01-21 19:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-21 4:01 [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic Deepak Chitriki 2010-01-21 17:43 ` Ramirez Luna, Omar 2010-01-21 17:47 ` Nishanth Menon 2010-01-21 17:57 ` Ramirez Luna, Omar 2010-01-21 18:03 ` Nishanth Menon 2010-01-21 19:46 ` Ramirez Luna, Omar 2010-01-21 19:53 ` Nishanth Menon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox