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

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

[Amit Daniel Kachhap]

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
have been tested on exynos5420 based board.

Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
---

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 8d64200..57cb3dc 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -198,10 +198,21 @@ 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)
+{
+	return exynos4_frc_read(&mct_frc);
+}
+
 static void __init exynos4_clocksource_init(void)
 {
 	exynos4_mct_frc_start(0, 0);
 
+	exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
+	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);
 
-- 
1.9.1

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

[Doug Anderson]

Amit,

Thanks for posting!

On Wed, Jun 18, 2014 at 4:31 AM, Amit Daniel Kachhap
<amit.daniel@samsung.com> 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
> have been tested on exynos5420 based board.
>
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> ---
>
> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 8d64200..57cb3dc 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -198,10 +198,21 @@ 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)
> +{
> +       return exynos4_frc_read(&mct_frc);

This is terribly inefficient to read all 64-bits and then cast back to
a 32-bit value.  Replace with:

return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);


> +}
> +
>  static void __init exynos4_clocksource_init(void)
>  {
>         exynos4_mct_frc_start(0, 0);

Please rebase atop (1d80415 clocksource: exynos_mct: Don't reset the
counter during boot and resume), which is in linuxnext among other
places.

>
> +       exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
> +       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);

It does seem to work for me though.  :)

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

[amit daniel kachhap]

On Thu, Jun 19, 2014 at 4:47 AM, Doug Anderson <dianders@chromium.org> wrote:
> Amit,
>
> Thanks for posting!
>
> On Wed, Jun 18, 2014 at 4:31 AM, Amit Daniel Kachhap
> <amit.daniel@samsung.com> 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
>> have been tested on exynos5420 based board.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> ---
>>
>> 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 | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>> index 8d64200..57cb3dc 100644
>> --- a/drivers/clocksource/exynos_mct.c
>> +++ b/drivers/clocksource/exynos_mct.c
>> @@ -198,10 +198,21 @@ 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)
>> +{
>> +       return exynos4_frc_read(&mct_frc);
>
> This is terribly inefficient to read all 64-bits and then cast back to
> a 32-bit value.  Replace with:
Yes agree.
This unsigned long strangely behaves as 32 bits for arm32 and 64 bits for arm64.
So I think it better is to do something like this,
+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
+}
+
I will post V2 version like above and see what the maintainer says.

>
> return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
>
>
>> +}
>> +
>>  static void __init exynos4_clocksource_init(void)
>>  {
>>         exynos4_mct_frc_start(0, 0);
>
> Please rebase atop (1d80415 clocksource: exynos_mct: Don't reset the
> counter during boot and resume), which is in linuxnext among other
> places.
OK will re base my patch on top of this.
>
>>
>> +       exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
>> +       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);
>
> It does seem to work for me though.  :)
thanks,

> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Lezcano]

On 06/19/2014 01:17 AM, Doug Anderson wrote:
> Amit,
>
> Thanks for posting!
>
> On Wed, Jun 18, 2014 at 4:31 AM, Amit Daniel Kachhap
> <amit.daniel@samsung.com> 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
>> have been tested on exynos5420 based board.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> ---
>>
>> 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 | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>> index 8d64200..57cb3dc 100644
>> --- a/drivers/clocksource/exynos_mct.c
>> +++ b/drivers/clocksource/exynos_mct.c
>> @@ -198,10 +198,21 @@ 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)
>> +{
>> +       return exynos4_frc_read(&mct_frc);
>
> This is terribly inefficient to read all 64-bits and then cast back to
> a 32-bit value.  Replace with:
>
> return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
>
>
>> +}
>> +
>>   static void __init exynos4_clocksource_init(void)
>>   {
>>          exynos4_mct_frc_start(0, 0);
>
> Please rebase atop (1d80415 clocksource: exynos_mct: Don't reset the
> counter during boot and resume), which is in linuxnext among other
> places.
>
>>
>> +       exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
>> +       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);
>
> It does seem to work for me though.  :)

Doug,

aren't you working on a 32 bits version ? So this patch could be 
simplified ?

Thanks
   -- Daniel


-- 
  <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

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

[Doug Anderson]

Daniel,

On Thu, Jun 19, 2014 at 2:07 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 06/19/2014 01:17 AM, Doug Anderson wrote:
>>
>> Amit,
>>
>> Thanks for posting!
>>
>> On Wed, Jun 18, 2014 at 4:31 AM, Amit Daniel Kachhap
>> <amit.daniel@samsung.com> 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
>>> have been tested on exynos5420 based board.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>> ---
>>>
>>> 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 | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/clocksource/exynos_mct.c
>>> b/drivers/clocksource/exynos_mct.c
>>> index 8d64200..57cb3dc 100644
>>> --- a/drivers/clocksource/exynos_mct.c
>>> +++ b/drivers/clocksource/exynos_mct.c
>>> @@ -198,10 +198,21 @@ 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)
>>> +{
>>> +       return exynos4_frc_read(&mct_frc);
>>
>>
>> This is terribly inefficient to read all 64-bits and then cast back to
>> a 32-bit value.  Replace with:
>>
>> return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
>>
>>
>>> +}
>>> +
>>>   static void __init exynos4_clocksource_init(void)
>>>   {
>>>          exynos4_mct_frc_start(0, 0);
>>
>>
>> Please rebase atop (1d80415 clocksource: exynos_mct: Don't reset the
>> counter during boot and resume), which is in linuxnext among other
>> places.
>>
>>>
>>> +       exynos4_delay_timer.read_current_timer =
>>> &exynos4_read_current_timer;
>>> +       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);
>>
>>
>> It does seem to work for me though.  :)
>
>
> Doug,
>
> aren't you working on a 32 bits version ? So this patch could be simplified

I could do that if someone told me that they'll land it.

My understanding of the current status is:
* I posed the 64-bit version that's almost as fast as the 32-bit version.
* I asked if people want the 32-bit version: no answer
* I asked if anyone is opposed to the 64-bit version: no answer

I know that you wanted me to clean up the description of the 64-bit
version so I was going to do that and repost.  If there's someone
willing to review / ack the 32-bit version I'd be happy to do that
instead.  Possibly I'll do both and a maintainer can choose which to
apply?


In the case here I was suggesting using the 32-bit version just
because on ARM32 there's totally no reason to read 64-bits.  I hadn't
given a thought to ARM64.  More on that in response to the other
messages.

-Doug

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

[Daniel Lezcano]

On 06/19/2014 05:49 PM, Doug Anderson wrote:
> Daniel,
>
> On Thu, Jun 19, 2014 at 2:07 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 06/19/2014 01:17 AM, Doug Anderson wrote:
>>>
>>> Amit,
>>>
>>> Thanks for posting!
>>>
>>> On Wed, Jun 18, 2014 at 4:31 AM, Amit Daniel Kachhap
>>> <amit.daniel@samsung.com> 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
>>>> have been tested on exynos5420 based board.
>>>>
>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>> ---
>>>>
>>>> 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 | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/clocksource/exynos_mct.c
>>>> b/drivers/clocksource/exynos_mct.c
>>>> index 8d64200..57cb3dc 100644
>>>> --- a/drivers/clocksource/exynos_mct.c
>>>> +++ b/drivers/clocksource/exynos_mct.c
>>>> @@ -198,10 +198,21 @@ 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)
>>>> +{
>>>> +       return exynos4_frc_read(&mct_frc);
>>>
>>>
>>> This is terribly inefficient to read all 64-bits and then cast back to
>>> a 32-bit value.  Replace with:
>>>
>>> return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L);
>>>
>>>
>>>> +}
>>>> +
>>>>    static void __init exynos4_clocksource_init(void)
>>>>    {
>>>>           exynos4_mct_frc_start(0, 0);
>>>
>>>
>>> Please rebase atop (1d80415 clocksource: exynos_mct: Don't reset the
>>> counter during boot and resume), which is in linuxnext among other
>>> places.
>>>
>>>>
>>>> +       exynos4_delay_timer.read_current_timer =
>>>> &exynos4_read_current_timer;
>>>> +       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);
>>>
>>>
>>> It does seem to work for me though.  :)
>>
>>
>> Doug,
>>
>> aren't you working on a 32 bits version ? So this patch could be simplified
>
> I could do that if someone told me that they'll land it.
>
> My understanding of the current status is:
> * I posed the 64-bit version that's almost as fast as the 32-bit version.
> * I asked if people want the 32-bit version: no answer
> * I asked if anyone is opposed to the 64-bit version: no answer

Yeah, that happens. I thought you were working on the 32 bits.

> I know that you wanted me to clean up the description of the 64-bit
> version so I was going to do that and repost.  If there's someone
> willing to review / ack the 32-bit version I'd be happy to do that
> instead.  Possibly I'll do both and a maintainer can choose which to
> apply?

Please, resend me the patch 1/3 as it is urgent with the changelog changed.

> In the case here I was suggesting using the 32-bit version just
> because on ARM32 there's totally no reason to read 64-bits.  I hadn't
> given a thought to ARM64.  More on that in response to the other
> messages.

Is there a 64bits platform using exynos_mct ?


-- 
  <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

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

[Doug Anderson]

Daniel,

On Thu, Jun 19, 2014 at 9:02 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>> My understanding of the current status is:
>> * I posed the 64-bit version that's almost as fast as the 32-bit version.
>> * I asked if people want the 32-bit version: no answer
>> * I asked if anyone is opposed to the 64-bit version: no answer
>
>
> Yeah, that happens. I thought you were working on the 32 bits.

It was on my list of things to do today to touch base and make sure we
were on the same page.  Apparently we weren't.  ;)


>> I know that you wanted me to clean up the description of the 64-bit
>> version so I was going to do that and repost.  If there's someone
>> willing to review / ack the 32-bit version I'd be happy to do that
>> instead.  Possibly I'll do both and a maintainer can choose which to
>> apply?
>
>
> Please, resend me the patch 1/3 as it is urgent with the changelog changed.

OK, I'll do that right now, but just send it as a single patch (so I
can get it out right away).  Then I'll post up the 32-bit versions
with an implicit dependency on that patch.

-Doug