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

[Mark Rutland <mark.rutland@arm.com>]

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.