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

On Tuesday 21 January 2014 10:58:17 Sebastian Hesselbarth wrote:

On 01/21/14 10:46, Arnd Bergmann wrote:

quoted

On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:

quoted

-/*
- * Thread-safe access to TIMER_CTRL register
- * (shared with watchdog timer)
- */
-void orion_timer_ctrl_clrset(u32 clr, u32 set)
-{
-       spin_lock(&timer_ctrl_lock);
-       writel((readl(timer_base + TIMER_CTRL) & ~clr) | set,
-               timer_base + TIMER_CTRL);
-       spin_unlock(&timer_ctrl_lock);
-}
-EXPORT_SYMBOL(orion_timer_ctrl_clrset);

I don't understand what's wrong with this function, it seems like
a cleaner approach than touching the register directly from two
different drivers. Is this something that would only work on
orion but not on armadaxp?

The real problem with this is that it resides in orion-time.c which
is fine for Orion SoCs. Armada 370/XP use a different timer and
therefore the _common_ watchdog driver cannot call this function.

Right, I had missed this part.

Moreover, Dove (out of Orion) and Armada 370/XP will happily live in
one V7 kernel with both time-orion and time-armada-370-xp compiled in.

It still wouldn't be too hard to do: both clock drivers could export
functions with different identifiers, and the wdt driver can use
conditional compilation (#ifdef or if(IS_ENABLED())) to only call
those functions when the respective clocksource drivers are enabled.

I think the part where it went wrong is the fact that we have two
device nodes in DT that claim the same register range, which should
not have happened, but seems too hard to fix now without breaking
one part or another.

The idea of the atomic readl/writel was to have something lightweight
and _early_ to allow such locking even for timers.

IIRC, there was some kind of consensus that it is okay to have atomic
readl/writel here.

Ok, Ezequiel pointed me to that discussion now, seems ok then.

	Arnd