Re: [PATCH] DSPBRIDGE: Fix BUG scheduling while atomic

[Nishanth Menon <nm@ti.com>]

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