All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Hiremath, Vaibhav" <hvaibhav@ti.com>
To: "Hunter, Jon" <jon-hunter@ti.com>
Cc: Tony Lindgren <tony@atomide.com>,
	"Hilman, Kevin" <khilman@ti.com>, Paul Walmsley <paul@pwsan.com>,
	linux-omap <linux-omap@vger.kernel.org>,
	linux-arm <linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
Date: Thu, 13 Sep 2012 10:24:53 +0000	[thread overview]
Message-ID: <79CD15C6BA57404B839C016229A409A83EAB7BCE@DBDE01.ent.ti.com> (raw)
In-Reply-To: <5048ADD0.4050804@ti.com>

On Thu, Sep 06, 2012 at 19:36:08, Hunter, Jon wrote:
> 
> On 09/06/2012 12:07 AM, Vaibhav Hiremath wrote:
> > 
> > 
> > On 9/6/2012 12:34 AM, Jon Hunter wrote:
> >> Errata Titles:
> >> i103: Delay needed to read some GP timer, WD timer and sync timer registers
> >>       after wakeup (OMAP3/4)
> >> i767: Delay needed to read some GP timer registers after wakeup (OMAP5)
> >>
> >> Description (i103/i767):
> >> If a General Purpose Timer (GPTimer) is in posted mode (TSICR [2].POSTED=1),
> >> due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
> >> registers right after the timer interface clock (L4) goes from stopped to
> >> active may not return the expected values. The most common event leading to
> >> this situation occurs upon wake up from idle.
> >>
> >> GPTimer non-posted synchronization mode is not impacted by this limitation.
> >>
> >> Workarounds:
> >> 1). Disable posted mode
> >> 2). Use static dependency between timer clock domain and MPUSS clock domain
> >> 3). Use no-idle mode when the timer is active
> >>
> >> Workarounds #2 and #3 are not pratical from a power standpoint and so
> >> workaround #1 has been implemented. Disabling posted mode adds some CPU overhead
> >> for configuring the timers as the CPU has to wait for the write to complete.
> >> However, disabling posted mode guarantees correct operation.
> >>
> >> Please note that it is safe to use posted mode for timers if the counter (TCRR)
> >> and capture (TCARx) registers will never be read. An example of this is the
> >> clock-event system timer. This is used by the kernel to schedule events however,
> >> the timers counter is never read and capture registers are not used. Given that
> >> the kernel configures this timer often yet never reads the counter register it
> >> is safe to enable posted mode in this case. Hence, for the timer used for kernel
> >> clock-events, posted mode is enabled by overriding the errata for devices that
> >> are impacted by this defect.
> >>
> >> Although both dmtimers and watchdogs are impacted by this defect this patch only
> >> implements the workaround for the dmtimer. Currently the watchdog driver does
> >> not read the counter register and so no workaround is necessary.
> >>
> >> Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.
> >>
> > 
> > Thanks for pinging me on this and getting it confirmed.
> > 
> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> >> ---
> >>  arch/arm/mach-omap2/timer.c               |    9 +++++++
> >>  arch/arm/plat-omap/dmtimer.c              |    2 ++
> >>  arch/arm/plat-omap/include/plat/dmtimer.h |   39 +++++++++++++++++++++++++++++
> >>  3 files changed, 50 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> >> index 2ff6d41..5471706 100644
> >> --- a/arch/arm/mach-omap2/timer.c
> >> +++ b/arch/arm/mach-omap2/timer.c
> >> @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int gptimer_id,
> >>  {
> >>  	int res;
> >>  
> >> +	/*
> >> +	 * For clock-event timers we never read the timer counter and
> >> +	 * so we are not impacted by errata i103 and i767. Therefore,
> >> +	 * we can safely ignore this errata for clock-event timers.
> >> +	 */
> >> +	__omap_dm_timer_populate_errata(&clkev, OMAP_TIMER_ERRATA_I103_I767);
> >> +
> > 
> > Couple of points,
> > 
> > 1. It is confusing to me, as you are passing the errata flag so i expect
> > api should set it. Why can't we do reverse way, you pass 0 here, since
> > you don't want to set and pass this flag every other places where you
> > want to enable this errata.
> 
> Per the design of the __omap_dm_timer_populate_errata function, the 2nd
> argument is called override to allow us to override an errata. I am not
> a huge fan of this, but I wanted to be explicit in the code that we are
> intentionally allowing posted mode for the clock-events timer.
> 
> I did not wish to pass the flags we want to set because if there more
> flags added in the future then we will have to keep changing the calls
> to the populate_errata function to add these.
> 

Isn't that would self-explain himself which flag is going to set without 
looking at the implementation?
I do not have any reservations here, it just doesn't seem easily readable to 
me.
You can make a call here.


> > 2. Why can't we enable for all timers? Even though clock-event is anyway
> > not reading it, but still is is applicable to it, right?
> 
> Yes it is still applicable but we never read it so it is ok to override.
> If you see Richard W's original patch for enabling posted mode it is to
> reduce overhead of programming timers, specifically the clock-events
> timer which is program very often.
> 
> For other timers, we do not know how they will be used and so by default
> we disable posted mode as this is safe.
> 
> > 3. Why can't we just simply Add this flag to hwmod_data file and read it
> > back in omap_timer_init() and omap_dm_timer_init_one(). Wouldn't that be
> > a good approach to handle it?
> 
> It could be done in this case, but typically I have not seen errata
> flags added to HWMOD. 

We are already using it, look at I2C, MMC. And in some cases, we have done 
indirect implementation of errata. So I still feel, we should leverage hwmod 
info for this, and anyway going forward DT will replace it.

Thanks,
Vaibhav

> One limitation you would have with HWMOD is if an
> erratum is only specific to a certain revision of the device. In this
> case it is not and so it could work.
> 
> In general I think that having errata described by HWMOD would be a good
> thing, but if think that should be a longer term goal with agreement
> from Benoit and add some general errata helpers to HWMOD.
> 
> Cheers
> Jon
> 


WARNING: multiple messages have this Message-ID (diff)
From: hvaibhav@ti.com (Hiremath, Vaibhav)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
Date: Thu, 13 Sep 2012 10:24:53 +0000	[thread overview]
Message-ID: <79CD15C6BA57404B839C016229A409A83EAB7BCE@DBDE01.ent.ti.com> (raw)
In-Reply-To: <5048ADD0.4050804@ti.com>

On Thu, Sep 06, 2012 at 19:36:08, Hunter, Jon wrote:
> 
> On 09/06/2012 12:07 AM, Vaibhav Hiremath wrote:
> > 
> > 
> > On 9/6/2012 12:34 AM, Jon Hunter wrote:
> >> Errata Titles:
> >> i103: Delay needed to read some GP timer, WD timer and sync timer registers
> >>       after wakeup (OMAP3/4)
> >> i767: Delay needed to read some GP timer registers after wakeup (OMAP5)
> >>
> >> Description (i103/i767):
> >> If a General Purpose Timer (GPTimer) is in posted mode (TSICR [2].POSTED=1),
> >> due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
> >> registers right after the timer interface clock (L4) goes from stopped to
> >> active may not return the expected values. The most common event leading to
> >> this situation occurs upon wake up from idle.
> >>
> >> GPTimer non-posted synchronization mode is not impacted by this limitation.
> >>
> >> Workarounds:
> >> 1). Disable posted mode
> >> 2). Use static dependency between timer clock domain and MPUSS clock domain
> >> 3). Use no-idle mode when the timer is active
> >>
> >> Workarounds #2 and #3 are not pratical from a power standpoint and so
> >> workaround #1 has been implemented. Disabling posted mode adds some CPU overhead
> >> for configuring the timers as the CPU has to wait for the write to complete.
> >> However, disabling posted mode guarantees correct operation.
> >>
> >> Please note that it is safe to use posted mode for timers if the counter (TCRR)
> >> and capture (TCARx) registers will never be read. An example of this is the
> >> clock-event system timer. This is used by the kernel to schedule events however,
> >> the timers counter is never read and capture registers are not used. Given that
> >> the kernel configures this timer often yet never reads the counter register it
> >> is safe to enable posted mode in this case. Hence, for the timer used for kernel
> >> clock-events, posted mode is enabled by overriding the errata for devices that
> >> are impacted by this defect.
> >>
> >> Although both dmtimers and watchdogs are impacted by this defect this patch only
> >> implements the workaround for the dmtimer. Currently the watchdog driver does
> >> not read the counter register and so no workaround is necessary.
> >>
> >> Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.
> >>
> > 
> > Thanks for pinging me on this and getting it confirmed.
> > 
> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> >> ---
> >>  arch/arm/mach-omap2/timer.c               |    9 +++++++
> >>  arch/arm/plat-omap/dmtimer.c              |    2 ++
> >>  arch/arm/plat-omap/include/plat/dmtimer.h |   39 +++++++++++++++++++++++++++++
> >>  3 files changed, 50 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> >> index 2ff6d41..5471706 100644
> >> --- a/arch/arm/mach-omap2/timer.c
> >> +++ b/arch/arm/mach-omap2/timer.c
> >> @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int gptimer_id,
> >>  {
> >>  	int res;
> >>  
> >> +	/*
> >> +	 * For clock-event timers we never read the timer counter and
> >> +	 * so we are not impacted by errata i103 and i767. Therefore,
> >> +	 * we can safely ignore this errata for clock-event timers.
> >> +	 */
> >> +	__omap_dm_timer_populate_errata(&clkev, OMAP_TIMER_ERRATA_I103_I767);
> >> +
> > 
> > Couple of points,
> > 
> > 1. It is confusing to me, as you are passing the errata flag so i expect
> > api should set it. Why can't we do reverse way, you pass 0 here, since
> > you don't want to set and pass this flag every other places where you
> > want to enable this errata.
> 
> Per the design of the __omap_dm_timer_populate_errata function, the 2nd
> argument is called override to allow us to override an errata. I am not
> a huge fan of this, but I wanted to be explicit in the code that we are
> intentionally allowing posted mode for the clock-events timer.
> 
> I did not wish to pass the flags we want to set because if there more
> flags added in the future then we will have to keep changing the calls
> to the populate_errata function to add these.
> 

Isn't that would self-explain himself which flag is going to set without 
looking at the implementation?
I do not have any reservations here, it just doesn't seem easily readable to 
me.
You can make a call here.


> > 2. Why can't we enable for all timers? Even though clock-event is anyway
> > not reading it, but still is is applicable to it, right?
> 
> Yes it is still applicable but we never read it so it is ok to override.
> If you see Richard W's original patch for enabling posted mode it is to
> reduce overhead of programming timers, specifically the clock-events
> timer which is program very often.
> 
> For other timers, we do not know how they will be used and so by default
> we disable posted mode as this is safe.
> 
> > 3. Why can't we just simply Add this flag to hwmod_data file and read it
> > back in omap_timer_init() and omap_dm_timer_init_one(). Wouldn't that be
> > a good approach to handle it?
> 
> It could be done in this case, but typically I have not seen errata
> flags added to HWMOD. 

We are already using it, look at I2C, MMC. And in some cases, we have done 
indirect implementation of errata. So I still feel, we should leverage hwmod 
info for this, and anyway going forward DT will replace it.

Thanks,
Vaibhav

> One limitation you would have with HWMOD is if an
> erratum is only specific to a certain revision of the device. In this
> case it is not and so it could work.
> 
> In general I think that having errata described by HWMOD would be a good
> thing, but if think that should be a longer term goal with agreement
> from Benoit and add some general errata helpers to HWMOD.
> 
> Cheers
> Jon
> 

  parent reply	other threads:[~2012-09-13 10:25 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-05 19:04 [PATCH 00/10] ARM: OMAP: DMTIMER fixes and clean-up Jon Hunter
2012-09-05 19:04 ` Jon Hunter
2012-09-05 19:04 ` [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767 Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-06  5:07   ` Vaibhav Hiremath
2012-09-06  5:07     ` Vaibhav Hiremath
2012-09-06 14:06     ` Jon Hunter
2012-09-06 14:06       ` Jon Hunter
2012-09-06 14:42       ` Jon Hunter
2012-09-06 14:42         ` Jon Hunter
2012-09-06 15:20         ` Jon Hunter
2012-09-06 15:20           ` Jon Hunter
2012-09-13 10:26           ` Hiremath, Vaibhav
2012-09-13 10:26             ` Hiremath, Vaibhav
2012-09-13 10:24       ` Hiremath, Vaibhav [this message]
2012-09-13 10:24         ` Hiremath, Vaibhav
2012-09-05 19:04 ` [PATCH 02/10] ARM: OMAP: Fix timer posted mode support Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-06 12:57   ` Vaibhav Hiremath
2012-09-06 12:57     ` Vaibhav Hiremath
2012-09-06 14:20     ` Jon Hunter
2012-09-06 14:20       ` Jon Hunter
2012-09-06 16:01       ` Jon Hunter
2012-09-06 16:01         ` Jon Hunter
2012-09-13 10:24         ` Hiremath, Vaibhav
2012-09-13 10:24           ` Hiremath, Vaibhav
2012-09-05 19:04 ` [PATCH 03/10] ARM: OMAP3: Correct HWMOD DMTIMER SYSC register declarations Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-05 19:04 ` [PATCH 04/10] ARM: OMAP2/3: Define HWMOD software reset status for DMTIMERs Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-05 19:04 ` [PATCH 05/10] ARM: OMAP2+: Don't use __omap_dm_timer_reset() Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-05 19:04 ` [PATCH 06/10] ARM: OMAP: Fix dmtimer reset for timer1 Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-05 19:04 ` [PATCH 07/10] ARM: OMAP: Clean-up dmtimer reset code Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-05 19:04 ` [PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-07 22:22   ` Tony Lindgren
2012-09-07 22:22     ` Tony Lindgren
2012-09-10 21:59     ` Jon Hunter
2012-09-10 21:59       ` Jon Hunter
2012-09-11  0:58       ` Tony Lindgren
2012-09-11  0:58         ` Tony Lindgren
2012-09-11 16:26         ` Jon Hunter
2012-09-11 16:26           ` Jon Hunter
2012-09-11 16:34           ` Tony Lindgren
2012-09-11 16:34             ` Tony Lindgren
2012-09-13  3:26             ` Jon Hunter
2012-09-13  3:26               ` Jon Hunter
2012-09-05 19:04 ` [PATCH 09/10] ARM: OMAP: Add dmtimer interrupt disable function Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-06 12:58   ` Vaibhav Hiremath
2012-09-06 12:58     ` Vaibhav Hiremath
2012-09-06 14:26     ` Jon Hunter
2012-09-06 14:26       ` Jon Hunter
2012-09-05 19:04 ` [PATCH 10/10] ARM: OMAP: Remove unnecessary call to clk_get() Jon Hunter
2012-09-05 19:04   ` Jon Hunter
2012-09-06 12:58 ` [PATCH 00/10] ARM: OMAP: DMTIMER fixes and clean-up Vaibhav Hiremath
2012-09-06 12:58   ` Vaibhav Hiremath
2012-09-06 14:30   ` Jon Hunter
2012-09-06 14:30     ` Jon Hunter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=79CD15C6BA57404B839C016229A409A83EAB7BCE@DBDE01.ent.ti.com \
    --to=hvaibhav@ti.com \
    --cc=jon-hunter@ti.com \
    --cc=khilman@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=paul@pwsan.com \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.