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

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

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.