Re: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete

[Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>]

On 07/30/2014 09:18 AM, Sekhar Nori wrote:
> On Tuesday 29 July 2014 11:00 PM, Grygorii Strashko wrote:
>> Hi Jon,
>>
>> On 07/29/2014 06:53 PM, Jon Cormier wrote:
>>> A slimmer patch suggested by Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>>
>>
>> Ok. The problem seems to be deeper than at first look.
>> You have sequence (in Mainline kernel):
>> cpufreq:
>>   |- notify CPUFREQ_PRECHANGE
>>      |- i2c-davinci will lock & put I2C in reset
>>   |- cpufreq_driver->target_index
>>      |- davinci_target()
>>         |- pdata->set_voltage() - It will try to use I2C to set new voltage,
>>         while I2C is in reset or locked! Bug!
>>   |- notify CPUFREQ_POSTCHANGE
>>      |- i2c-davinci will re-enable I2C and adjust I2C clock
>
> Good find. I really wonder how this escaped so far. I can swear cpufreq
> transitions were tested multiple times. From the description it looks
> like this should hit every single time there is a voltage adjustment.
>
> On DA850 which is the only DaVinci implementing cpufreq, I2C0 input
> frequency will remain constant across cpufreq transitions since it
> derives from PLL0 AUXCLK which is used pre-multipler/divider. It remains
> fixed.
>
> The code seems to have been added for I2C1 which does have a fixed ratio
> with cpu clock.
>
> PMIC should usually be put on I2C0 to help prevent these kind of issues.
>
>> I see few possible ways to solve it:
>> 1) use CLK notifier instead of CPUfreq notifiers
>
> This will require common clock framework, right? We dont have that on
> mach-davinci.

:( I've forgotten about that.

>
>> 2) do smth similar to "61c7cff8 i2c: S3C24XX I2C frequency scaling support"
>>    + "9bcd04bf i2c: s3c2410: grab adapter lock while changing i2c clock"
>
> This looks promising. Although I wonder if delta_f will always remain
> zero in s3c24xx_i2c_cpufreq_transition() when the CPUFREQ_PRECHANGE call
> is made - because clock tree is not updated yet?

That's funny - seems PRE/POST notifiers are called twice for s3c24xx :)
First one from cpufreq core.
Second time from s3c_cpufreq_target() and, looks like, clock
freq will be updated at that time.


>
>> 3) update I2C clock in CPUFREQ_POSTCHANGE - may be unsafe
>
> Well, even now the I2C clock is only getting updated in POSTCHANGE,
> right? Also, resetting I2C in PRECHANGE seems excessive. It is only
> required when changing the prescalar. So you can do:
>
> 	} else if (val == CPUFREQ_POSTCHANGE) {
> 		davinci_i2c_reset_ctrl(dev, 0);
> 		i2c_davinci_calc_clk_dividers(dev);
> 		davinci_i2c_reset_ctrl(dev, 1);
> 	}
>
> So this along with the i2c_lock_adapter() a la
> s3c24xx_i2c_cpufreq_transition() should be the right fix, I guess.
>

Regards,
-grygorii