Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

Hello Daniel,

On Tue, Jan 21, 2014 at 10:57:37AM +0100, Daniel Lezcano wrote:

On 01/21/2014 10:12 AM, Ezequiel Garcia wrote:

quoted

Replace the driver-specific thread-safe shared register API
by the recently introduced atomic_io_clear_set().

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

in the future, put me in Cc when modifying files in clocksource that 
will make my life easier to track the changes.

Ouch, sorry about that.

[..]

quoted

  /*
   * Free-running clocksource handling.

@@ -68,7 +54,8 @@ static int orion_clkevt_next_event(unsigned long delta,
  {
  	/* setup and enable one-shot timer */
  	writel(delta, timer_base + TIMER1_VAL);
-	orion_timer_ctrl_clrset(TIMER1_RELOAD_EN, TIMER1_EN);
+	atomic_io_modify(timer_base + TIMER_CTRL,
+		TIMER1_RELOAD_EN | TIMER1_EN, TIMER1_EN);

I am not convinced this change is worth.

Could you elaborate the race the spinlock should prevent ?

Sure. Take a look to the patchset that adds the atomic I/O (sorry again for
not Ccing you on that one):

  http://www.spinics.net/lists/arm-kernel/msg292775.html

The TIMER_CTRL register provides clocksource and watchdog control, so we
need a safe way to access the register, since we're sharing it on two
completely unrelated drivers.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
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