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

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 01/21/14 10:46, Arnd Bergmann wrote:
> On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:
>> -/*
>> - * 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.

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.

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.

Sebastian