[PATCH] ARM: dts: Update arch timer node with clock frequency

[PATCH] ARM: dts: Update arch timer node with clock frequency

[Yuvaraj Kumar C D]

Without the "clock-frequency" property in arch timer node, could able
to see the below crash dump.

[<c0014e28>] (unwind_backtrace+0x0/0xf4) from [<c0011808>] (show_stack+0x10/0x14)
[<c0011808>] (show_stack+0x10/0x14) from [<c036ac1c>] (dump_stack+0x7c/0xb0)
[<c036ac1c>] (dump_stack+0x7c/0xb0) from [<c01ab760>] (Ldiv0_64+0x8/0x18)
[<c01ab760>] (Ldiv0_64+0x8/0x18) from [<c0062f60>] (clockevents_config.part.2+0x1c/0x74)
[<c0062f60>] (clockevents_config.part.2+0x1c/0x74) from [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c)
[<c0062fd8>] (clockevents_config_and_register+0x20/0x2c) from [<c02b8e8c>] (arch_timer_setup+0xa8/0x134)
[<c02b8e8c>] (arch_timer_setup+0xa8/0x134) from [<c04b47b4>] (arch_timer_init+0x1f4/0x24c)
[<c04b47b4>] (arch_timer_init+0x1f4/0x24c) from [<c04b40d8>] (clocksource_of_init+0x34/0x58)
[<c04b40d8>] (clocksource_of_init+0x34/0x58) from [<c049ed8c>] (time_init+0x20/0x2c)
[<c049ed8c>] (time_init+0x20/0x2c) from [<c049b95c>] (start_kernel+0x1e0/0x39c)

Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5250.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 7d7cc77..668ce5d 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -96,6 +96,7 @@
 			     <1 14 0xf08>,
 			     <1 11 0xf08>,
 			     <1 10 0xf08>;
+		clock-frequency = <24000000>;
 	};
 
 	mct@101C0000 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Mark Rutland]

[adding lakml]

On Wed, Sep 18, 2013 at 11:11:53AM +0100, Yuvaraj Kumar C D wrote:
> Without the "clock-frequency" property in arch timer node, could able
> to see the below crash dump.

Why does this cause the below crash specifically? What is CNTFRQ reading
as?

Your firmware or bootloader should set CNTFRQ -- setting the
clock-frequency is a work-around for buggy firmware/bootloaders that
should be avoided as far as possible.

Is it not possible to fix your firmware or bootlaoder to set CNTFRQ?

Thanks,
Mark.

> 
> [<c0014e28>] (unwind_backtrace+0x0/0xf4) from [<c0011808>] (show_stack+0x10/0x14)
> [<c0011808>] (show_stack+0x10/0x14) from [<c036ac1c>] (dump_stack+0x7c/0xb0)
> [<c036ac1c>] (dump_stack+0x7c/0xb0) from [<c01ab760>] (Ldiv0_64+0x8/0x18)
> [<c01ab760>] (Ldiv0_64+0x8/0x18) from [<c0062f60>] (clockevents_config.part.2+0x1c/0x74)
> [<c0062f60>] (clockevents_config.part.2+0x1c/0x74) from [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c)
> [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c) from [<c02b8e8c>] (arch_timer_setup+0xa8/0x134)
> [<c02b8e8c>] (arch_timer_setup+0xa8/0x134) from [<c04b47b4>] (arch_timer_init+0x1f4/0x24c)
> [<c04b47b4>] (arch_timer_init+0x1f4/0x24c) from [<c04b40d8>] (clocksource_of_init+0x34/0x58)
> [<c04b40d8>] (clocksource_of_init+0x34/0x58) from [<c049ed8c>] (time_init+0x20/0x2c)
> [<c049ed8c>] (time_init+0x20/0x2c) from [<c049b95c>] (start_kernel+0x1e0/0x39c)
> 
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm/boot/dts/exynos5250.dtsi |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 7d7cc77..668ce5d 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -96,6 +96,7 @@
>  			     <1 14 0xf08>,
>  			     <1 11 0xf08>,
>  			     <1 10 0xf08>;
> +		clock-frequency = <24000000>;
>  	};
>  
>  	mct@101C0000 {
> -- 
> 1.7.9.5
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Yuvaraj Kumar]

Resending it as it bounced from kernel mailing group

On Wed, Sep 18, 2013 at 3:53 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> [adding lakml]
>
> On Wed, Sep 18, 2013 at 11:11:53AM +0100, Yuvaraj Kumar C D wrote:
>> Without the "clock-frequency" property in arch timer node, could able
>> to see the below crash dump.
>
> Why does this cause the below crash specifically? What is CNTFRQ reading
> as?
Return value of arch_timer_get_cntfrq() is 0
>
> Your firmware or bootloader should set CNTFRQ -- setting the
> clock-frequency is a work-around for buggy firmware/bootloaders that
> should be avoided as far as possible.
Why kernel should depend on bootloader/firmware  to set CNTFRQ? Any
specific reasons?
Should'nt be indepenedent each other(kernel and bootloader/firmware)?
>
> Is it not possible to fix your firmware or bootlaoder to set CNTFRQ?
>
> Thanks,
> Mark.
>
>>
>> [<c0014e28>] (unwind_backtrace+0x0/0xf4) from [<c0011808>] (show_stack+0x10/0x14)
>> [<c0011808>] (show_stack+0x10/0x14) from [<c036ac1c>] (dump_stack+0x7c/0xb0)
>> [<c036ac1c>] (dump_stack+0x7c/0xb0) from [<c01ab760>] (Ldiv0_64+0x8/0x18)
>> [<c01ab760>] (Ldiv0_64+0x8/0x18) from [<c0062f60>] (clockevents_config.part.2+0x1c/0x74)
>> [<c0062f60>] (clockevents_config.part.2+0x1c/0x74) from [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c)
>> [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c) from [<c02b8e8c>] (arch_timer_setup+0xa8/0x134)
>> [<c02b8e8c>] (arch_timer_setup+0xa8/0x134) from [<c04b47b4>] (arch_timer_init+0x1f4/0x24c)
>> [<c04b47b4>] (arch_timer_init+0x1f4/0x24c) from [<c04b40d8>] (clocksource_of_init+0x34/0x58)
>> [<c04b40d8>] (clocksource_of_init+0x34/0x58) from [<c049ed8c>] (time_init+0x20/0x2c)
>> [<c049ed8c>] (time_init+0x20/0x2c) from [<c049b95c>] (start_kernel+0x1e0/0x39c)
>>
>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> ---
>>  arch/arm/boot/dts/exynos5250.dtsi |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
>> index 7d7cc77..668ce5d 100644
>> --- a/arch/arm/boot/dts/exynos5250.dtsi
>> +++ b/arch/arm/boot/dts/exynos5250.dtsi
>> @@ -96,6 +96,7 @@
>>                            <1 14 0xf08>,
>>                            <1 11 0xf08>,
>>                            <1 10 0xf08>;
>> +             clock-frequency = <24000000>;
>>       };
>>
>>       mct@101C0000 {
>> --
>> 1.7.9.5
>>
>>

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Marc Zyngier]

On 20/09/13 05:57, Yuvaraj Kumar wrote:
> Resending it as it bounced from kernel mailing group
> 
> On Wed, Sep 18, 2013 at 3:53 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> [adding lakml]
>>
>> On Wed, Sep 18, 2013 at 11:11:53AM +0100, Yuvaraj Kumar C D wrote:
>>> Without the "clock-frequency" property in arch timer node, could able
>>> to see the below crash dump.
>>
>> Why does this cause the below crash specifically? What is CNTFRQ reading
>> as?
> Return value of arch_timer_get_cntfrq() is 0
>>
>> Your firmware or bootloader should set CNTFRQ -- setting the
>> clock-frequency is a work-around for buggy firmware/bootloaders that
>> should be avoided as far as possible.
> Why kernel should depend on bootloader/firmware  to set CNTFRQ? Any
> specific reasons?

Because the kernel can't set it if running non-secure. Only secure mode
can do this (see the ARM ARM for details).

> Should'nt be indepenedent each other(kernel and bootloader/firmware)?

In my book, the firmware is responsible for setting up the platform in a
sane state. Leaving CNTFRQ in its UNKNOWN reset state is a bug, and your
firmware should program it on each CPU. Same goes for CNTVOFF, which
should be set to a common value (preferably zero).

	M.
-- 
Jazz is not dead. It just smells funny...

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Christopher Covington]

On 09/20/2013 04:17 AM, Marc Zyngier wrote:
> On 20/09/13 05:57, Yuvaraj Kumar wrote:
>> Resending it as it bounced from kernel mailing group
>>
>> On Wed, Sep 18, 2013 at 3:53 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>>> [adding lakml]
>>>
>>> On Wed, Sep 18, 2013 at 11:11:53AM +0100, Yuvaraj Kumar C D wrote:
>>>> Without the "clock-frequency" property in arch timer node, could able
>>>> to see the below crash dump.
>>>
>>> Why does this cause the below crash specifically? What is CNTFRQ reading
>>> as?
>> Return value of arch_timer_get_cntfrq() is 0
>>>
>>> Your firmware or bootloader should set CNTFRQ -- setting the
>>> clock-frequency is a work-around for buggy firmware/bootloaders that
>>> should be avoided as far as possible.
>> Why kernel should depend on bootloader/firmware  to set CNTFRQ? Any
>> specific reasons?
> 
> Because the kernel can't set it if running non-secure. Only secure mode
> can do this (see the ARM ARM for details).

What software outside the kernel actually reads the CNTFRQ and why?

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Marc Zyngier]

On 20/09/13 15:55, Christopher Covington wrote:
> On 09/20/2013 04:17 AM, Marc Zyngier wrote:
>> On 20/09/13 05:57, Yuvaraj Kumar wrote:
>>> Resending it as it bounced from kernel mailing group
>>>
>>> On Wed, Sep 18, 2013 at 3:53 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>>>> [adding lakml]
>>>>
>>>> On Wed, Sep 18, 2013 at 11:11:53AM +0100, Yuvaraj Kumar C D wrote:
>>>>> Without the "clock-frequency" property in arch timer node, could able
>>>>> to see the below crash dump.
>>>>
>>>> Why does this cause the below crash specifically? What is CNTFRQ reading
>>>> as?
>>> Return value of arch_timer_get_cntfrq() is 0
>>>>
>>>> Your firmware or bootloader should set CNTFRQ -- setting the
>>>> clock-frequency is a work-around for buggy firmware/bootloaders that
>>>> should be avoided as far as possible.
>>> Why kernel should depend on bootloader/firmware  to set CNTFRQ? Any
>>> specific reasons?
>>
>> Because the kernel can't set it if running non-secure. Only secure mode
>> can do this (see the ARM ARM for details).
> 
> What software outside the kernel actually reads the CNTFRQ and why?

Your favourite virtual machine does, for the same reason the host kernel
does. And as you can't guess the frequency from userspace, you cannot
specify it in the guest's DT, getting whatever random number sits in CNTFRQ.

As you would expect, things don't go smoothly when that happens.

	M.
-- 
Jazz is not dead. It just smells funny...

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Tomasz Figa]

Hi Yuvaraj,

On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
> Without the "clock-frequency" property in arch timer node, could able
> to see the below crash dump.
[snip]
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi
> b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -96,6 +96,7 @@
>  			     <1 14 0xf08>,
>  			     <1 11 0xf08>,
>  			     <1 10 0xf08>;
> +		clock-frequency = <24000000>;

Shouldn't it rather come from some clock provided by some clock controller 
instead?

The frequency would be then retrieved using clk_get_rate() on a clock 
received by clk_get(), specified in device tree using generic clock 
bindings.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Mark Rutland]

On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
> Hi Yuvaraj,
> 
> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
> > Without the "clock-frequency" property in arch timer node, could able
> > to see the below crash dump.
> [snip]
> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
> > --- a/arch/arm/boot/dts/exynos5250.dtsi
> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
> > @@ -96,6 +96,7 @@
> >  			     <1 14 0xf08>,
> >  			     <1 11 0xf08>,
> >  			     <1 10 0xf08>;
> > +		clock-frequency = <24000000>;
> 
> Shouldn't it rather come from some clock provided by some clock controller 
> instead?
> 
> The frequency would be then retrieved using clk_get_rate() on a clock 
> received by clk_get(), specified in device tree using generic clock 
> bindings.

If the bootloader has initialised the generic timer correctly, the
CNTFRQ register should contain the clock frequency, independent of any
external clock.

Having the bootloader set CNTFRQ is by far the preferable solution, it
is architected for this purpose.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Olof Johansson]

[Adding Tony, who reported a mainline booting issue, and Sean who
helped me track this down]

On Mon, Sep 23, 2013 at 7:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
>> Hi Yuvaraj,
>>
>> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
>> > Without the "clock-frequency" property in arch timer node, could able
>> > to see the below crash dump.
>> [snip]
>> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
>> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
>> > --- a/arch/arm/boot/dts/exynos5250.dtsi
>> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
>> > @@ -96,6 +96,7 @@
>> >                          <1 14 0xf08>,
>> >                          <1 11 0xf08>,
>> >                          <1 10 0xf08>;
>> > +           clock-frequency = <24000000>;
>>
>> Shouldn't it rather come from some clock provided by some clock controller
>> instead?
>>
>> The frequency would be then retrieved using clk_get_rate() on a clock
>> received by clk_get(), specified in device tree using generic clock
>> bindings.
>
> If the bootloader has initialised the generic timer correctly, the
> CNTFRQ register should contain the clock frequency, independent of any
> external clock.

So, we just sat here to bisect a problem on the Samsung Chromebook
where we hit exactly this problem. The read-only firmware on the
device does not set CNTFRQ at boot, so this fails.

Apparantly the u-boot that comes with Arndale sets it, so I haven't
seen this error on that platform.

> Having the bootloader set CNTFRQ is by far the preferable solution, it
> is architected for this purpose.

Unfortunately there is now real hardware out there that needs this due
to firmware bugs / missing features, so there's little other choice.
:(

I'll pick this patch up in the fixes branch for 3.12, unless someone
complains loudly.


-Olof

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Mark Rutland]

On Tue, Oct 08, 2013 at 11:15:47PM +0100, Olof Johansson wrote:
> [Adding Tony, who reported a mainline booting issue, and Sean who
> helped me track this down]
> 
> On Mon, Sep 23, 2013 at 7:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
> >> Hi Yuvaraj,
> >>
> >> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
> >> > Without the "clock-frequency" property in arch timer node, could able
> >> > to see the below crash dump.
> >> [snip]
> >> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
> >> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
> >> > --- a/arch/arm/boot/dts/exynos5250.dtsi
> >> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
> >> > @@ -96,6 +96,7 @@
> >> >                          <1 14 0xf08>,
> >> >                          <1 11 0xf08>,
> >> >                          <1 10 0xf08>;
> >> > +           clock-frequency = <24000000>;
> >>
> >> Shouldn't it rather come from some clock provided by some clock controller
> >> instead?
> >>
> >> The frequency would be then retrieved using clk_get_rate() on a clock
> >> received by clk_get(), specified in device tree using generic clock
> >> bindings.
> >
> > If the bootloader has initialised the generic timer correctly, the
> > CNTFRQ register should contain the clock frequency, independent of any
> > external clock.
> 
> So, we just sat here to bisect a problem on the Samsung Chromebook
> where we hit exactly this problem. The read-only firmware on the
> device does not set CNTFRQ at boot, so this fails.

Ouch. That's a shame.

A chained bootloader (like the KVM guys are using) should be able to set
CNTFRQ, so as long as any chained loader actually does that this won't
cause that to blow up...

> 
> Apparantly the u-boot that comes with Arndale sets it, so I haven't
> seen this error on that platform.
> 
> > Having the bootloader set CNTFRQ is by far the preferable solution, it
> > is architected for this purpose.
> 
> Unfortunately there is now real hardware out there that needs this due
> to firmware bugs / missing features, so there's little other choice.
> :(

Indeed :(

> 
> I'll pick this patch up in the fixes branch for 3.12, unless someone
> complains loudly.

Could you please add a note to the dts regarding why we actually need
this? It would be nice to maintain the impression that this is not the
preferred way of doing things...

Cheers,
Mark.

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Olof Johansson]

On Wed, Oct 9, 2013 at 1:25 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Oct 08, 2013 at 11:15:47PM +0100, Olof Johansson wrote:
>> [Adding Tony, who reported a mainline booting issue, and Sean who
>> helped me track this down]
>>
>> On Mon, Sep 23, 2013 at 7:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
>> >> Hi Yuvaraj,
>> >>
>> >> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
>> >> > Without the "clock-frequency" property in arch timer node, could able
>> >> > to see the below crash dump.
>> >> [snip]
>> >> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
>> >> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
>> >> > --- a/arch/arm/boot/dts/exynos5250.dtsi
>> >> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
>> >> > @@ -96,6 +96,7 @@
>> >> >                          <1 14 0xf08>,
>> >> >                          <1 11 0xf08>,
>> >> >                          <1 10 0xf08>;
>> >> > +           clock-frequency = <24000000>;
>> >>
>> >> Shouldn't it rather come from some clock provided by some clock controller
>> >> instead?
>> >>
>> >> The frequency would be then retrieved using clk_get_rate() on a clock
>> >> received by clk_get(), specified in device tree using generic clock
>> >> bindings.
>> >
>> > If the bootloader has initialised the generic timer correctly, the
>> > CNTFRQ register should contain the clock frequency, independent of any
>> > external clock.
>>
>> So, we just sat here to bisect a problem on the Samsung Chromebook
>> where we hit exactly this problem. The read-only firmware on the
>> device does not set CNTFRQ at boot, so this fails.
>
> Ouch. That's a shame.
>
> A chained bootloader (like the KVM guys are using) should be able to set
> CNTFRQ, so as long as any chained loader actually does that this won't
> cause that to blow up...

Yes, but we have cases where we want to be able to boot without a
chained u-boot as well.

>> Apparantly the u-boot that comes with Arndale sets it, so I haven't
>> seen this error on that platform.
>>
>> > Having the bootloader set CNTFRQ is by far the preferable solution, it
>> > is architected for this purpose.
>>
>> Unfortunately there is now real hardware out there that needs this due
>> to firmware bugs / missing features, so there's little other choice.
>> :(
>
> Indeed :(
>
>>
>> I'll pick this patch up in the fixes branch for 3.12, unless someone
>> complains loudly.
>
> Could you please add a note to the dts regarding why we actually need
> this? It would be nice to maintain the impression that this is not the
> preferred way of doing things...

s/dts/dtsi/, yes I can do that. Hopefully with that we won't get too
much automated copy and paste to other platforms.

It looks like current upstream u-boot got the timer enablement patches
merged together with HYP support, which explains why I didn't see this
on Arndale since it has a forked version of u-boot from Linaro
somewhere.


-Olof

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Rob Herring]

On Tue, Oct 8, 2013 at 5:15 PM, Olof Johansson <olof@lixom.net> wrote:
> [Adding Tony, who reported a mainline booting issue, and Sean who
> helped me track this down]
>
> On Mon, Sep 23, 2013 at 7:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
>>> Hi Yuvaraj,
>>>
>>> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
>>> > Without the "clock-frequency" property in arch timer node, could able
>>> > to see the below crash dump.
>>> [snip]
>>> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
>>> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
>>> > --- a/arch/arm/boot/dts/exynos5250.dtsi
>>> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
>>> > @@ -96,6 +96,7 @@
>>> >                          <1 14 0xf08>,
>>> >                          <1 11 0xf08>,
>>> >                          <1 10 0xf08>;
>>> > +           clock-frequency = <24000000>;
>>>
>>> Shouldn't it rather come from some clock provided by some clock controller
>>> instead?
>>>
>>> The frequency would be then retrieved using clk_get_rate() on a clock
>>> received by clk_get(), specified in device tree using generic clock
>>> bindings.
>>
>> If the bootloader has initialised the generic timer correctly, the
>> CNTFRQ register should contain the clock frequency, independent of any
>> external clock.
>
> So, we just sat here to bisect a problem on the Samsung Chromebook
> where we hit exactly this problem. The read-only firmware on the
> device does not set CNTFRQ at boot, so this fails.
>
> Apparantly the u-boot that comes with Arndale sets it, so I haven't
> seen this error on that platform.
>
>> Having the bootloader set CNTFRQ is by far the preferable solution, it
>> is architected for this purpose.
>
> Unfortunately there is now real hardware out there that needs this due
> to firmware bugs / missing features, so there's little other choice.
> :(
>
> I'll pick this patch up in the fixes branch for 3.12, unless someone
> complains loudly.

Perhaps the subject should say something about this only applying to exynos.

Rob

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Mark Rutland]

On Wed, Oct 09, 2013 at 08:46:06PM +0100, Olof Johansson wrote:
> On Wed, Oct 9, 2013 at 1:25 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Oct 08, 2013 at 11:15:47PM +0100, Olof Johansson wrote:
> >> [Adding Tony, who reported a mainline booting issue, and Sean who
> >> helped me track this down]
> >>
> >> On Mon, Sep 23, 2013 at 7:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
> >> >> Hi Yuvaraj,
> >> >>
> >> >> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
> >> >> > Without the "clock-frequency" property in arch timer node, could able
> >> >> > to see the below crash dump.
> >> >> [snip]
> >> >> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
> >> >> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
> >> >> > --- a/arch/arm/boot/dts/exynos5250.dtsi
> >> >> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
> >> >> > @@ -96,6 +96,7 @@
> >> >> >                          <1 14 0xf08>,
> >> >> >                          <1 11 0xf08>,
> >> >> >                          <1 10 0xf08>;
> >> >> > +           clock-frequency = <24000000>;
> >> >>
> >> >> Shouldn't it rather come from some clock provided by some clock controller
> >> >> instead?
> >> >>
> >> >> The frequency would be then retrieved using clk_get_rate() on a clock
> >> >> received by clk_get(), specified in device tree using generic clock
> >> >> bindings.
> >> >
> >> > If the bootloader has initialised the generic timer correctly, the
> >> > CNTFRQ register should contain the clock frequency, independent of any
> >> > external clock.
> >>
> >> So, we just sat here to bisect a problem on the Samsung Chromebook
> >> where we hit exactly this problem. The read-only firmware on the
> >> device does not set CNTFRQ at boot, so this fails.
> >
> > Ouch. That's a shame.
> >
> > A chained bootloader (like the KVM guys are using) should be able to set
> > CNTFRQ, so as long as any chained loader actually does that this won't
> > cause that to blow up...
> 
> Yes, but we have cases where we want to be able to boot without a
> chained u-boot as well.

Sorry, that was poorly worded on my behalf. I meant that in the main case I'm
aware of where the having the clock-frequency property wasn't good enough
(because it doesn't get propagated to guests), we have another workaround.

> 
> >> Apparantly the u-boot that comes with Arndale sets it, so I haven't
> >> seen this error on that platform.
> >>
> >> > Having the bootloader set CNTFRQ is by far the preferable solution, it
> >> > is architected for this purpose.
> >>
> >> Unfortunately there is now real hardware out there that needs this due
> >> to firmware bugs / missing features, so there's little other choice.
> >> :(
> >
> > Indeed :(
> >
> >>
> >> I'll pick this patch up in the fixes branch for 3.12, unless someone
> >> complains loudly.
> >
> > Could you please add a note to the dts regarding why we actually need
> > this? It would be nice to maintain the impression that this is not the
> > preferred way of doing things...
> 
> s/dts/dtsi/, yes I can do that. Hopefully with that we won't get too
> much automated copy and paste to other platforms.

Cheers!

Thanks,
Mark.

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Olof Johansson]

On Wed, Oct 09, 2013 at 04:48:32PM -0500, Rob Herring wrote:
> On Tue, Oct 8, 2013 at 5:15 PM, Olof Johansson <olof@lixom.net> wrote:
> > [Adding Tony, who reported a mainline booting issue, and Sean who
> > helped me track this down]
> >
> > On Mon, Sep 23, 2013 at 7:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> On Sat, Sep 21, 2013 at 04:24:59PM +0100, Tomasz Figa wrote:
> >>> Hi Yuvaraj,
> >>>
> >>> On Wednesday 18 of September 2013 15:41:53 Yuvaraj Kumar C D wrote:
> >>> > Without the "clock-frequency" property in arch timer node, could able
> >>> > to see the below crash dump.
> >>> [snip]
> >>> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi
> >>> > b/arch/arm/boot/dts/exynos5250.dtsi index 7d7cc77..668ce5d 100644
> >>> > --- a/arch/arm/boot/dts/exynos5250.dtsi
> >>> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
> >>> > @@ -96,6 +96,7 @@
> >>> >                          <1 14 0xf08>,
> >>> >                          <1 11 0xf08>,
> >>> >                          <1 10 0xf08>;
> >>> > +           clock-frequency = <24000000>;
> >>>
> >>> Shouldn't it rather come from some clock provided by some clock controller
> >>> instead?
> >>>
> >>> The frequency would be then retrieved using clk_get_rate() on a clock
> >>> received by clk_get(), specified in device tree using generic clock
> >>> bindings.
> >>
> >> If the bootloader has initialised the generic timer correctly, the
> >> CNTFRQ register should contain the clock frequency, independent of any
> >> external clock.
> >
> > So, we just sat here to bisect a problem on the Samsung Chromebook
> > where we hit exactly this problem. The read-only firmware on the
> > device does not set CNTFRQ at boot, so this fails.
> >
> > Apparantly the u-boot that comes with Arndale sets it, so I haven't
> > seen this error on that platform.
> >
> >> Having the bootloader set CNTFRQ is by far the preferable solution, it
> >> is architected for this purpose.
> >
> > Unfortunately there is now real hardware out there that needs this due
> > to firmware bugs / missing features, so there's little other choice.
> > :(
> >
> > I'll pick this patch up in the fixes branch for 3.12, unless someone
> > complains loudly.
> 
> Perhaps the subject should say something about this only applying to exynos.

Definitely, and I'll add a comment as Mark requested.


-Olof

Re: [PATCH] ARM: dts: Update arch timer node with clock frequency

[Olof Johansson]

On Wed, Sep 18, 2013 at 03:41:53PM +0530, Yuvaraj Kumar C D wrote:
> Without the "clock-frequency" property in arch timer node, could able
> to see the below crash dump.
> 
> [<c0014e28>] (unwind_backtrace+0x0/0xf4) from [<c0011808>] (show_stack+0x10/0x14)
> [<c0011808>] (show_stack+0x10/0x14) from [<c036ac1c>] (dump_stack+0x7c/0xb0)
> [<c036ac1c>] (dump_stack+0x7c/0xb0) from [<c01ab760>] (Ldiv0_64+0x8/0x18)
> [<c01ab760>] (Ldiv0_64+0x8/0x18) from [<c0062f60>] (clockevents_config.part.2+0x1c/0x74)
> [<c0062f60>] (clockevents_config.part.2+0x1c/0x74) from [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c)
> [<c0062fd8>] (clockevents_config_and_register+0x20/0x2c) from [<c02b8e8c>] (arch_timer_setup+0xa8/0x134)
> [<c02b8e8c>] (arch_timer_setup+0xa8/0x134) from [<c04b47b4>] (arch_timer_init+0x1f4/0x24c)
> [<c04b47b4>] (arch_timer_init+0x1f4/0x24c) from [<c04b40d8>] (clocksource_of_init+0x34/0x58)
> [<c04b40d8>] (clocksource_of_init+0x34/0x58) from [<c049ed8c>] (time_init+0x20/0x2c)
> [<c049ed8c>] (time_init+0x20/0x2c) from [<c049b95c>] (start_kernel+0x1e0/0x39c)
> 
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>

Applied with subject:

ARM: exynos: dts: Update 5250 arch timer node with clock frequency


-Olof