Ezequiel Garcia | 20 Aug 18:48 2013

[PATCH v2 0/3] Introduce atomic MMIO register clear-set

Here's the v2 for the introduction of a generic thread-safe clear-set
register access.

Following the v1, I've added a proper barrier to ensure proper ordering,
between the writel and the spin_unlock.

It's worth noticing a different approach has been proposed (several times)
for getting thread-safe register access, through the use of regmap_update_bits
(just as it's done in mfd/syscon).

However, as noted in [1], such solution has been rejected (several times)
for two reasons:

  * It's way more heavy-weight than necessary
  * It cannot be used in very early scenarios,
    such as a clocksource initialization.

Using this API it's possible -for instance- to add support for Armada 370/XP
in the orion_wdt driver. That work is ready, and it's been hold until we
decide a proper solution for shared-register access.

[1] http://www.spinics.net/lists/arm-kernel/msg266494.html

Ezequiel Garcia (3):
  ARM: Introduce atomic MMIO clear/set
  clocksource: orion: Use atomic access for shared registers
  watchdog: orion: Use atomic access for shared registers

 arch/arm/include/asm/io.h        |  5 +++++
 arch/arm/kernel/io.c             | 13 +++++++++++++
(Continue reading)

Ezequiel Garcia | 20 Aug 18:48 2013

[PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

Some SoC have MMIO regions that are shared across orthogonal
subsystems. This commit implements a possible solution for the
thread-safe access of such regions through a spinlock-protected API
with clear-set semantics.

Concurrent access is protected with a single spinlock for the
entire MMIO address space. While this protects shared-registers,
it also serializes access to unrelated/unshared registers.

---
Based on a similar approach suggested by Russel King:
http://archive.arm.linux.org.uk/lurker/message/20130618.113606.d7d4fe4b.en.html

Changes from v1:

* Added an io barrier iowmb() as suggested by Will Deacon,
  to ensure the writel gets completed before the spin_unlock().

Signed-off-by: Ezequiel Garcia <ezequiel.garcia <at> free-electrons.com>
---
 arch/arm/include/asm/io.h |  5 +++++
 arch/arm/kernel/io.c      | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index d070741..3cea1f0 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
 <at>  <at>  -36,6 +36,11  <at>  <at> 
 #define isa_bus_to_virt phys_to_virt
(Continue reading)

Ezequiel Garcia | 20 Aug 18:55 2013

Re: [PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

On Tue, Aug 20, 2013 at 01:48:25PM -0300, Ezequiel Garcia wrote:
> Some SoC have MMIO regions that are shared across orthogonal
> subsystems. This commit implements a possible solution for the
> thread-safe access of such regions through a spinlock-protected API
> with clear-set semantics.
> 
> Concurrent access is protected with a single spinlock for the
> entire MMIO address space. While this protects shared-registers,
> it also serializes access to unrelated/unshared registers.
> 
> ---
> Based on a similar approach suggested by Russel King:
> http://archive.arm.linux.org.uk/lurker/message/20130618.113606.d7d4fe4b.en.html
> 
> Changes from v1:
> 
> * Added an io barrier iowmb() as suggested by Will Deacon,
>   to ensure the writel gets completed before the spin_unlock().

Argh! of course the above goes below the SOB.

> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia <at> free-electrons.com>
> ---
>  arch/arm/include/asm/io.h |  5 +++++
>  arch/arm/kernel/io.c      | 13 +++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index d070741..3cea1f0 100644
(Continue reading)

Russell King - ARM Linux | 20 Aug 23:08 2013
Picon

Re: [PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

On Tue, Aug 20, 2013 at 01:48:25PM -0300, Ezequiel Garcia wrote:
> Based on a similar approach suggested by Russel King:

Russell please.

We Russells get upset when our names are incorrectly spelt, just like
others get upset if they end up with extra letters in their names, or
you confuse Steven vs Stephen.  Or even dare call a Deborah "Debs"
(I did that once and the result was not particularly nice!)

> +void atomic_io_clear_set(u32 clear, u32 set, void __iomem *reg)
> +{
> +	spin_lock(&__io_lock);
> +	writel((readl(reg) & ~clear) | set, reg);
> +	/* ensure the write get done before unlocking */
> +	__iowmb();
> +	spin_unlock(&__io_lock);
> +}
> +EXPORT_SYMBOL(atomic_io_clear_set);

Some comments - neither of them you _have_ to act on:

1. writel((readl(reg) & ~mask) | (set & mask), reg) could be deemed
   to give better semantics - consider that if you don't look at the
   implementation, how do you know what the result of setting a bit
   in both the set & clear masks would be?

2. A historical note, that back in the 1980s with things like the BBC
   micro, this kind of operation was defined:

(Continue reading)

Ezequiel Garcia | 21 Aug 16:36 2013

Re: [PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

Russell,

On Tue, Aug 20, 2013 at 10:08:39PM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 20, 2013 at 01:48:25PM -0300, Ezequiel Garcia wrote:
> > Based on a similar approach suggested by Russel King:
> 
> Russell please.
> 
> We Russells get upset when our names are incorrectly spelt, just like
> others get upset if they end up with extra letters in their names, or
> you confuse Steven vs Stephen.  Or even dare call a Deborah "Debs"
> (I did that once and the result was not particularly nice!)
> 

Ouch... sorry about that!

> > +void atomic_io_clear_set(u32 clear, u32 set, void __iomem *reg)
> > +{
> > +	spin_lock(&__io_lock);
> > +	writel((readl(reg) & ~clear) | set, reg);
> > +	/* ensure the write get done before unlocking */
> > +	__iowmb();
> > +	spin_unlock(&__io_lock);
> > +}
> > +EXPORT_SYMBOL(atomic_io_clear_set);
> 
> Some comments - neither of them you _have_ to act on:
> 
> 1. writel((readl(reg) & ~mask) | (set & mask), reg) could be deemed
>    to give better semantics - consider that if you don't look at the
(Continue reading)

Will Deacon | 21 Aug 14:24 2013

Re: [PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

Hi Ezequiel,

Cheers for the v2. I've been thinking about how to improve the performance
of this operation and I ended up completely changing my mind about how it
should be implemented :)

Comments and questions below...

On Tue, Aug 20, 2013 at 05:48:25PM +0100, Ezequiel Garcia wrote:
> diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
> index dcd5b4d..b2a53a3 100644
> --- a/arch/arm/kernel/io.c
> +++ b/arch/arm/kernel/io.c
>  <at>  <at>  -1,6 +1,19  <at>  <at> 
>  #include <linux/export.h>
>  #include <linux/types.h>
>  #include <linux/io.h>
> +#include <linux/spinlock.h>
> +
> +static DEFINE_SPINLOCK(__io_lock);
> +
> +void atomic_io_clear_set(u32 clear, u32 set, void __iomem *reg)

First off, what exactly is this function supposed to guarantee? Do you simply
require thread-safe device access, or do you also require completion of the
the device writes?

Since the latter is device-specific (probably requiring some read-backs in
the driver), I assume you actually just need to ensure that multiple drivers
don't trip over each other. In which case, this can be *much* lighter
(Continue reading)

Ezequiel Garcia | 21 Aug 16:22 2013

Re: [PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

Will,

On Wed, Aug 21, 2013 at 01:24:24PM +0100, Will Deacon wrote:
> 
> Cheers for the v2. I've been thinking about how to improve the performance
> of this operation and I ended up completely changing my mind about how it
> should be implemented :)
> 
> Comments and questions below...
> 

Sure! Much appreciated...

> On Tue, Aug 20, 2013 at 05:48:25PM +0100, Ezequiel Garcia wrote:
> > diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
> > index dcd5b4d..b2a53a3 100644
> > --- a/arch/arm/kernel/io.c
> > +++ b/arch/arm/kernel/io.c
> >  <at>  <at>  -1,6 +1,19  <at>  <at> 
> >  #include <linux/export.h>
> >  #include <linux/types.h>
> >  #include <linux/io.h>
> > +#include <linux/spinlock.h>
> > +
> > +static DEFINE_SPINLOCK(__io_lock);
> > +
> > +void atomic_io_clear_set(u32 clear, u32 set, void __iomem *reg)
> 
> First off, what exactly is this function supposed to guarantee? Do you simply
> require thread-safe device access, or do you also require completion of the
(Continue reading)

Will Deacon | 21 Aug 18:28 2013

Re: [PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

On Wed, Aug 21, 2013 at 03:22:04PM +0100, Ezequiel Garcia wrote:
> On Wed, Aug 21, 2013 at 01:24:24PM +0100, Will Deacon wrote:
> > On Tue, Aug 20, 2013 at 05:48:25PM +0100, Ezequiel Garcia wrote:
> > > diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
> > > index dcd5b4d..b2a53a3 100644
> > > --- a/arch/arm/kernel/io.c
> > > +++ b/arch/arm/kernel/io.c
> > >  <at>  <at>  -1,6 +1,19  <at>  <at> 
> > >  #include <linux/export.h>
> > >  #include <linux/types.h>
> > >  #include <linux/io.h>
> > > +#include <linux/spinlock.h>
> > > +
> > > +static DEFINE_SPINLOCK(__io_lock);
> > > +
> > > +void atomic_io_clear_set(u32 clear, u32 set, void __iomem *reg)
> > 
> > First off, what exactly is this function supposed to guarantee? Do you simply
> > require thread-safe device access, or do you also require completion of the
> > the device writes?
> > 
> 
> Indeed, thread-safe device access would be enough, unless I'm missing
> something.

Great!

> > > +	spin_lock(&__io_lock);
> > 
> > Is this function likely to be used in irq context? If so, better disable
(Continue reading)

Ezequiel Garcia | 20 Aug 18:48 2013

[PATCH v2 2/3] clocksource: orion: Use atomic access for shared registers

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

Signed-off-by: Ezequiel Garcia <ezequiel.garcia <at> free-electrons.com>
---
 drivers/clocksource/time-orion.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/time-orion.c b/drivers/clocksource/time-orion.c
index ecbeb68..4334ea7 100644
--- a/drivers/clocksource/time-orion.c
+++ b/drivers/clocksource/time-orion.c
 <at>  <at>  -35,20 +35,15  <at>  <at> 
 #define ORION_ONESHOT_MAX	0xfffffffe

 static void __iomem *timer_base;
-static DEFINE_SPINLOCK(timer_ctrl_lock);

 /*
  * Thread-safe access to TIMER_CTRL register
  * (shared with watchdog timer)
  */
-void orion_timer_ctrl_clrset(u32 clr, u32 set)
+static 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);
+	atomic_io_clear_set(timer_base + TIMER_CTRL, clr, set);
(Continue reading)

Ezequiel Garcia | 20 Aug 18:48 2013

[PATCH v2 3/3] watchdog: orion: Use atomic access for shared registers

Since the timer control register is shared with the clocksource driver,
use the recently introduced atomic_io_clear_set() to access such register.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia <at> free-electrons.com>
---
 drivers/watchdog/orion_wdt.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 4ea5fcc..de35ae9 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
 <at>  <at>  -73,9 +73,7  <at>  <at>  static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	writel(~WDT_INT_REQ, BRIDGE_CAUSE);

 	/* Enable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg |= WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
+	atomic_io_clear_set(wdt_reg + TIMER_CTRL, 0, WDT_EN);

 	/* Enable reset on watchdog */
 	reg = readl(RSTOUTn_MASK);
 <at>  <at>  -98,9 +96,7  <at>  <at>  static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	writel(reg, RSTOUTn_MASK);

 	/* Disable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg &= ~WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
(Continue reading)


Gmane