Re: [PATCH v2] clocksource: exynos-mct: Register the timer for stable udelay

Daniel Lezcano
2014-06-19T10:30:43Z

On 06/19/2014 12:21 PM, amit daniel kachhap wrote:
> On Thu, Jun 19, 2014 at 2:41 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 06/19/2014 10:39 AM, Amit Daniel Kachhap wrote:
>>>
>>> This patch register the exynos mct clocksource as the current timer
>>> as it has constant clock rate. This will generate correct udelay for the
>>> exynos platform and avoid using unnecessary calibrated jiffies. This
>>> change
>>> has been tested on exynos5420 based board and udelay is very close to
>>> expected.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>> ---
>>> Changes in V2:
>>> * Added #defines for ARM and ARM64 as pointed by Doug Anderson.
>>>
>>> Patches from David Riley confirmed that udelay is broken in exynos5420.
>>> Link to those patches are,
>>> 1) https://patchwork.kernel.org/patch/4344911/
>>> 2) https://patchwork.kernel.org/patch/4344881/
>>>
>>> drivers/clocksource/exynos_mct.c | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/clocksource/exynos_mct.c
>>> b/drivers/clocksource/exynos_mct.c
>>> index f71d55f..02927e2 100644
>>> --- a/drivers/clocksource/exynos_mct.c
>>> +++ b/drivers/clocksource/exynos_mct.c
>>> @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void)
>>> return exynos4_frc_read(&mct_frc);
>>> }
>>>
>>> +static struct delay_timer exynos4_delay_timer;
>>> +
>>> +static unsigned long exynos4_read_current_timer(void)
>>> +{
>>> +#ifdef ARM
>>> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
>>> +#else /* ARM64, etc */
>>> + return exynos4_frc_read(&mct_frc);
>>> +#endif
>>> +}
>>> +
>>
>>
>> There isn't another solution than that ? macros definitions in C file are
>> avoided as much as possible.
> I also didn't want to use macros but used as a last option. you want
> me to put more comments here?
> Or something like below is also possible for checking the size of
> (unsigned long) in runtime.
>
> unsigned long x;
> unsigned int size = (char *)(&x + 1) - (char *)(&x);
> if (size == 4)
> return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
> else
> return exynos4_frc_read(&mct_frc);
>
> But this involves extra computation which should not be used for time
> critical functions.
> Any suggestion?

AFAIU, Doug may change the exynos4_frc_read to be 32 bits. So may not
need those macros as 'exynos4_frc_read' could be used instead.

Doug may confirm this.

>>> static void __init exynos4_clocksource_init(void)
>>> {
>>> exynos4_mct_frc_start();
>>>
>>> + exynos4_delay_timer.read_current_timer =
>>> &exynos4_read_current_timer;
>>
>>
>> &exynos4_read_current_timer ?
> Any issue in the naming?

No problem with the naming. As Tomasz pointed also, you are passing '&'
for the function pointer.

>>> + exynos4_delay_timer.freq = clk_rate;
>>> + register_current_timer_delay(&exynos4_delay_timer);
>>> +
>>> if (clocksource_register_hz(&mct_frc, clk_rate))
>>> panic("%s: can't register clocksource\n", mct_frc.name);
>>>
>>>
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |