[PATCHv2] omap3: beagle: Use GPTIMER1 for clockevents

Commit Message

Sanjeev Premi June 27, 2011, 10:36 a.m. UTC

From: Hrishikesh Bhandiwad <[e-mail]>

Present current selection of the GPTIMER on Beagleboard
was result of a hardware issue in early versions of the
Beagleboards (Ax and B1 thru B4). [1][2]

Its been long since the hardware issue has been fixed.
This patch uses GPTIMER 1 for all newer board revisions
incl. Beagleboard XM.

Also, the clock source for GPTIMER12 is much less frequency
stable than clock sources for GPTIMER1. Using GPTIMER12 can
result in major time skew over a fairly short interval.

 [1] http://thread.gmane.org/gmane.comp.hardware.beagleboard.general/91
 [2] Errata #7 at http://elinux.org/BeagleBoard#Errata

Signed-off-by: Sanjeev Premi <[e-mail]>
Reviewed-by: Paul Walmsley <[e-mail]>
Cc: Hrishikesh Bhandiwad <[e-mail]>
Cc: Thomas Weber <[e-mail]>
Cc: Jason Lam <[e-mail]>
Cc: Gregoire Gentil <[e-mail]>
---

 Changes since v1:
   * Updated description with details from Paul Walmsley.
   * Changed author to Hrishikesh.
   * Expanded cc: to include original authors for other
     boards that are still using GPTIMER12.

 arch/arm/mach-omap2/board-omap3beagle.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Comments

Tony Lindgren June 27, 2011, 11:15 a.m. UTC | #1

* Sanjeev Premi <[e-mail]> [110627 03:33]:
> From: Hrishikesh Bhandiwad <[e-mail]>
> 
> Present current selection of the GPTIMER on Beagleboard
> was result of a hardware issue in early versions of the
> Beagleboards (Ax and B1 thru B4). [1][2]
> 
> Its been long since the hardware issue has been fixed.
> This patch uses GPTIMER 1 for all newer board revisions
> incl. Beagleboard XM.
> 
> Also, the clock source for GPTIMER12 is much less frequency
> stable than clock sources for GPTIMER1. Using GPTIMER12 can
> result in major time skew over a fairly short interval.

I don't think omap3_beagle_init_rev is even called when
the timer is set?

But even if it was, this is not a good fix because of the
dependency issues it causes to mux and gpio framework in
omap3_beagle_rev_init.

The best way to fix this is to set a separate machine ID
for the working beagle boards like I commented earlier.
It allows just setting the .timer based on that, the rest
of the code can be shared.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sanjeev Premi June 27, 2011, 12:12 p.m. UTC | #2

> -----Original Message-----
> From: Tony Lindgren [mailto:[e-mail]] 
> Sent: Monday, June 27, 2011 4:46 PM
> To: Premi, Sanjeev
> Cc: [e-mail]; 
> [e-mail]; Gregoire Gentil; 
> Bhandiwad, Hrishikesh; Jason Lam; Thomas Weber
> Subject: Re: [PATCHv2] omap3: beagle: Use GPTIMER1 for clockevents
> 
> * Sanjeev Premi <[e-mail]> [110627 03:33]:
> > From: Hrishikesh Bhandiwad <[e-mail]>
> > 
> > Present current selection of the GPTIMER on Beagleboard
> > was result of a hardware issue in early versions of the
> > Beagleboards (Ax and B1 thru B4). [1][2]
> > 
> > Its been long since the hardware issue has been fixed.
> > This patch uses GPTIMER 1 for all newer board revisions
> > incl. Beagleboard XM.
> > 
> > Also, the clock source for GPTIMER12 is much less frequency
> > stable than clock sources for GPTIMER1. Using GPTIMER12 can
> > result in major time skew over a fairly short interval.
> 
> I don't think omap3_beagle_init_rev is even called when
> the timer is set?

[sp] I verified the patch based on the print indicating that
     GPTIMER1 being used as clockevent source.
     http://marc.info/?l=linux-omap&m=130893319726456&w=2

> 
> But even if it was, this is not a good fix because of the
> dependency issues it causes to mux and gpio framework in
> omap3_beagle_rev_init.
> 
> The best way to fix this is to set a separate machine ID
> for the working beagle boards like I commented earlier.
> It allows just setting the .timer based on that, the rest
> of the code can be shared.

[sp] Sorry missed reading your comment. I wasn't checking
     mails while sending the updated patch.

> 
> Regards,
> 
> Tony
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren June 27, 2011, 12:31 p.m. UTC | #3

* Premi, Sanjeev <[e-mail]> [110627 05:08]:
> > From: Tony Lindgren [mailto:[e-mail]] 
> > 
> > I don't think omap3_beagle_init_rev is even called when
> > the timer is set?
> 
> [sp] I verified the patch based on the print indicating that
>      GPTIMER1 being used as clockevent source.
>      http://marc.info/?l=linux-omap&m=130893319726456&w=2

I suspect the test always fails though, so it probably never
gets set to gptimer12 on any board :)

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sanjeev Premi June 27, 2011, 1:28 p.m. UTC | #4

> -----Original Message-----
> From: Tony Lindgren [mailto:[e-mail]] 
> Sent: Monday, June 27, 2011 6:02 PM
> To: Premi, Sanjeev
> Cc: [e-mail]; 
> [e-mail]; Gregoire Gentil; 
> Bhandiwad, Hrishikesh; Jason Lam; Thomas Weber
> Subject: Re: [PATCHv2] omap3: beagle: Use GPTIMER1 for clockevents
> 
> * Premi, Sanjeev <[e-mail]> [110627 05:08]:
> > > From: Tony Lindgren [mailto:[e-mail]] 
> > > 
> > > I don't think omap3_beagle_init_rev is even called when
> > > the timer is set?
> > 
> > [sp] I verified the patch based on the print indicating that
> >      GPTIMER1 being used as clockevent source.
> >      http://marc.info/?l=linux-omap&m=130893319726456&w=2
> 
> I suspect the test always fails though, so it probably never
> gets set to gptimer12 on any board :)
>

[sp] While I take my time understanding things on devel-timer;
     I had a quick question - at risk of being flamed.

     Adding a new machine ID would trickle to u-boot and same
     uImage (default) may not work across board revisions.

     How does this scheme look like:
     - GPTIMER1 is used as default - as it works for most boards.
     - GPTIMER12 is used based on a static config option OR a
       board specific bootarg

     I know both these options aren't general practice. Still
     wanted to know your views in the current context.

~sanjeev

> Tony
>--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren June 27, 2011, 6 p.m. UTC | #5

* Premi, Sanjeev <[e-mail]> [110627 06:24]:
> 
> [sp] While I take my time understanding things on devel-timer;
>      I had a quick question - at risk of being flamed.
> 
>      Adding a new machine ID would trickle to u-boot and same
>      uImage (default) may not work across board revisions.

Yes that's a hassle :( Until we have device tree, maybe see if
u-boot passes ATAG_REVISION that's available as system_rev in
kernel?
 
>      How does this scheme look like:
>      - GPTIMER1 is used as default - as it works for most boards.
>      - GPTIMER12 is used based on a static config option OR a
>        board specific bootarg
> 
>      I know both these options aren't general practice. Still
>      wanted to know your views in the current context.

Let's not do a Kconfig option, but a cmdline option should work
if done in a generic way. If you decide to go that way, then
please add that to timer.c against devel-timer branch.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman June 27, 2011, 7:04 p.m. UTC | #6

"Premi, Sanjeev" <[e-mail]> writes:

>> -----Original Message-----
>> From: Tony Lindgren [mailto:[e-mail]] 
>> Sent: Monday, June 27, 2011 6:02 PM
>> To: Premi, Sanjeev
>> Cc: [e-mail]; 
>> [e-mail]; Gregoire Gentil; 
>> Bhandiwad, Hrishikesh; Jason Lam; Thomas Weber
>> Subject: Re: [PATCHv2] omap3: beagle: Use GPTIMER1 for clockevents
>> 
>> * Premi, Sanjeev <[e-mail]> [110627 05:08]:
>> > > From: Tony Lindgren [mailto:[e-mail]] 
>> > > 
>> > > I don't think omap3_beagle_init_rev is even called when
>> > > the timer is set?
>> > 
>> > [sp] I verified the patch based on the print indicating that
>> >      GPTIMER1 being used as clockevent source.
>> >      http://marc.info/?l=linux-omap&m=130893319726456&w=2
>> 
>> I suspect the test always fails though, so it probably never
>> gets set to gptimer12 on any board :)
>>
>
> [sp] While I take my time understanding things on devel-timer;
>      I had a quick question - at risk of being flamed.
>
>      Adding a new machine ID would trickle to u-boot and same
>      uImage (default) may not work across board revisions.
>
>      How does this scheme look like:
>      - GPTIMER1 is used as default - as it works for most boards.
>      - GPTIMER12 is used based on a static config option OR a
>        board specific bootarg
>
>      I know both these options aren't general practice. Still
>      wanted to know your views in the current context.
>

Another option may be to always use GPTIMER1 on early boot for all
boards.  Later in the boot, when the old rev boards are detected, a new
GPT12 clockevent could be registered (with a higher rating) such that
the clockevent core would switch to the new source.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 4b113b2..edc1596 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -509,7 +509,10 @@  static void __init omap3_beagle_init_irq(void)
 {
 	omap_init_irq();
 #ifdef CONFIG_OMAP_32K_TIMER
-	omap2_gp_clockevent_set_gptimer(12);
+	if (omap3_beagle_version == OMAP3BEAGLE_BOARD_AXBX)
+		omap2_gp_clockevent_set_gptimer(12);
+	else
+		omap2_gp_clockevent_set_gptimer(1);
 #endif
 }