Re: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete
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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete
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
>
>
> 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.
> 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?
> 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.
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete
/0x38)
>>> [] (regmap_write+0x24/0x38) from []
>>> (tps65023_dcdc_set_volta
>>> ge+0xc0/0xe8)
>>> [] (tps65023_dcdc_set_voltage+0xc0/0xe8) from []
>>> (_regulator
>>> _do_set_voltage+0x3c/0x1d0)
>>> [] (_regulator_do_set_voltage+0x3c/0x1d0) from []
>>> (regulator
>>> _set_voltage+0xb8/0xcc)
>>> [] (regulator_set_voltage+0xb8/0xcc) from []
>>> (davinci_target
>>> +0xcc/0x14c)
>>> [] (davinci_target+0xcc/0x14c) from []
>>> (__cpufreq_driver_tar
>>> get+0x2c/0x3c)
>>> [] (__cpufreq_driver_target+0x2c/0x3c) from []
>>> (cpufreq_set+ 0x54/0x70)
>>> [] (cpufreq_set+0x54/0x70) from []
>>> (store_scaling_setspeed+0
>>> x58/0x6c)
>>> [] (store_scaling_setspeed+0x58/0x6c) from []
>>> (store+0x58/0x 70)
>>> [] (store+0x58/0x70) from []
>>> (sysfs_write_file+0x108/0x140)
>>> [] (sysfs_write_file+0x108/0x140) from []
>>> (vfs_write+0xb0/0x 138)
>>> [] (vfs_write+0xb0/0x138) from [] (sys_write+0x3c/0x68)
>>> [] (sys_write+0x3c/0x68) from []
>>> (ret_fast_syscall+0x0/0x2c)
>>>
>>> davinci_target was notifying CPUFREQ_PRECHANGE before calling
>>> set_voltage which relied tried talking to the pmic over i2c causing a
>>> hang.
>>>
>>> @@ -101,17 +101,17 @@ static int davinci_target(struct cpufreq_policy
>>> *policy,
>>>
>>> cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>>>
>>> /* if moving to higher frequency, up the voltage beforehand */
>>> if (pdata->set_voltage && freqs.new > freqs.old) {
>>> ret = pdata->set_voltage(idx);
>>> if (ret)
>>> goto out;
>>> }
>>>
>>> I fixed this by moving the cpufreq_notifiy. This doesn't apply to the
>>> later kernels as the davinci cpufreq driver has changed quite a bit.
>>> Unfortunately I don't have the setup to test a patch against a newer
>>> kernel right now.
>>>
>>> Author: Cormier, Jonathan
>>> Date: Tue Jul 29 11:22:50 2014 -0400
>>>
>>> ARM: DAVINCI: Reorder cpufreq_nofity_transistion so that
>>> set_voltage happens first.
>>>
>>> set_voltage may make use of the i2c bus to communicate with the
>>> PMIC. In this case we dont want the i2c to reset until after we set
>>> the voltage.
>>>
>>> Signed-off-by: Cormier, Jonathan
>>>
>>> diff --git a/arch/arm/mach-davinci/cpufreq.c
>>> b/arch/arm/mach-davinci/cpufreq.c
>>> index 5bba707..cbaee6c 100644
>>> --- a/arch/arm/mach-davinci/cpufreq.c
>>> +++ b/arch/arm/mach-davinci/cpufreq.c
>>> @@ -102,8 +102,6 @@ static int davinci_target(struct cpufreq_policy *policy,
>>> if (ret)
>>> return -EINVAL;
>>>
>>> -cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>>> -
>>> /* if moving to higher frequency, up the voltage beforehand */
>>> if (pdata->set_voltage && freqs.new > freqs.old) {
>>> ret = pdata->set_voltage(idx);
>>> @@ -111,6 +109,8 @@ static int davinci_target(struct cpufreq_policy *policy,
>>> goto out;
>>> }
>>>
>>> +cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>>> +
>>> ret = clk_set_rate(armclk, idx);
>>> if (ret)
>>> goto out;
>>>
>>> On Tue, Jul 29, 2014 at 10:38 AM, Brian Niebuhr wrote:
>>>>
>>>>
>>>>> -Original Message-
>>>>> From: davinci-linux-open-source-boun...@linux.davincidsp.com
>>>>> [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com] On
>>>>> Behalf Of Jon Cormier
>>>>> Sent: Tuesday, July 29, 2014 9:20 AM
>>>>> To: davinci-linux-open-sou...@linux.davincidsp.com
>>>>> Cc: Tim Iskander
>>>>> Subject: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete
>>>>>
>>>>> Reported issue:
>>>>>
>>>>> 1. Enable I2C1, flash the new kernel and reboot
>>>>> 2. Immediately after reboot, attempt to change the processor clock: "echo
>>>>> 456000 &g
Re: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete
+0xb0/0x138) from [] (sys_write+0x3c/0x68)
>> [] (sys_write+0x3c/0x68) from []
>> (ret_fast_syscall+0x0/0x2c)
>>
>> davinci_target was notifying CPUFREQ_PRECHANGE before calling
>> set_voltage which relied tried talking to the pmic over i2c causing a
>> hang.
>>
>> @@ -101,17 +101,17 @@ static int davinci_target(struct cpufreq_policy
>> *policy,
>>
>> cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>>
>> /* if moving to higher frequency, up the voltage beforehand */
>> if (pdata->set_voltage && freqs.new > freqs.old) {
>> ret = pdata->set_voltage(idx);
>> if (ret)
>> goto out;
>> }
>>
>> I fixed this by moving the cpufreq_notifiy. This doesn't apply to the
>> later kernels as the davinci cpufreq driver has changed quite a bit.
>> Unfortunately I don't have the setup to test a patch against a newer
>> kernel right now.
>>
>> Author: Cormier, Jonathan
>> Date: Tue Jul 29 11:22:50 2014 -0400
>>
>> ARM: DAVINCI: Reorder cpufreq_nofity_transistion so that
>> set_voltage happens first.
>>
>> set_voltage may make use of the i2c bus to communicate with the
>> PMIC. In this case we dont want the i2c to reset until after we set
>> the voltage.
>>
>> Signed-off-by: Cormier, Jonathan
>>
>> diff --git a/arch/arm/mach-davinci/cpufreq.c
>> b/arch/arm/mach-davinci/cpufreq.c
>> index 5bba707..cbaee6c 100644
>> --- a/arch/arm/mach-davinci/cpufreq.c
>> +++ b/arch/arm/mach-davinci/cpufreq.c
>> @@ -102,8 +102,6 @@ static int davinci_target(struct cpufreq_policy *policy,
>> if (ret)
>> return -EINVAL;
>>
>> -cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>> -
>> /* if moving to higher frequency, up the voltage beforehand */
>> if (pdata->set_voltage && freqs.new > freqs.old) {
>> ret = pdata->set_voltage(idx);
>> @@ -111,6 +109,8 @@ static int davinci_target(struct cpufreq_policy *policy,
>> goto out;
>> }
>>
>> +cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>> +
>> ret = clk_set_rate(armclk, idx);
>> if (ret)
>> goto out;
>>
>> On Tue, Jul 29, 2014 at 10:38 AM, Brian Niebuhr wrote:
>>>
>>>
>>>> -Original Message-
>>>> From: davinci-linux-open-source-boun...@linux.davincidsp.com
>>>> [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com] On
>>>> Behalf Of Jon Cormier
>>>> Sent: Tuesday, July 29, 2014 9:20 AM
>>>> To: davinci-linux-open-sou...@linux.davincidsp.com
>>>> Cc: Tim Iskander
>>>> Subject: i2c-davinci.c: CPU FREQ causes lock up due to xfr_complete
>>>>
>>>> Reported issue:
>>>>
>>>> 1. Enable I2C1, flash the new kernel and reboot
>>>> 2. Immediately after reboot, attempt to change the processor clock: "echo
>>>> 456000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed"
>>>> 3. Process blocks
>>>>
>>>> However, if we do the following:
>>>> 1. Enable I2C1, flash the new kernel and reboot
>>>> 2. Immediately after reboot, run: "i2cdetect -y 2 0x08 0x08" or just
>>>> "i2cdetect
>>>> -y 2"
>>>> 3. Then run: "echo 456000 >
>>>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed"
>>>> 4. Command succeeds
>>>
>>> I found this problem as well, and I haven't had time to put together a
>>> proper patch. Right now we're just working around it. There are several
>>> problems with the use of a completion for this task:
>>>
>>> 1. If no I2C transfer has occurred, a cpufreq transition will block forever
>>> (which is the bug you found)
>>> 2. Once an I2C transfer has occurred the cpufreq transition will never
>>> block since the completion is never reinitialized.
>>> 3. Even if you do reinitialize the completion for every I2C transfer, (1)
>>> is not solved and there is still a race condition where the cpufreq
>>> transition could start just before an I2C transfer starts and the I2C
>>> transfer occurs during the cpufreq transition.
>>>
>>> Personally I think the correct solution is to use a mutex instead of a
>>> completion. The cpufreq code would take the mu
