[PATCH v3 0/3] Introduce atomic MMIO register modify

[PATCH v3 0/3] Introduce atomic MMIO register modify

[Ezequiel Garcia]

After some discussion about the API semantic and implementation,
here's a new version, re-working both:

The semantic has been changed, and the new one is considered cleaner
(although slighlty less intuitive). In addition, it matches the
regmap_update_bits() prototype, which should appear nicely consistent
to our beloved users.

Following Will Deacon's suggestions, I reworked the implementation
mainly to reduce the performance penalty. See the previous discussion,
or the changelog (below) for details.

Changes from v2:
* As suggested by Will Deacon, dropped the iowmb() barrier
  and use relaxed variants instead. See Will's explanation for
  details: http://www.spinics.net/lists/arm-kernel/msg268775.html

* Use spin_{}_irqsave/restore to allow irq-context usage
  also suggested by Will Deacon.

* Re-worked the API semantics as proposed by Russell King.

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

Russell: If nobody has any objections, I'll add patch 1/3 to the
tracking system.

Thanks!

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

 arch/arm/include/asm/io.h        |  6 ++++++
 arch/arm/kernel/io.c             | 29 +++++++++++++++++++++++++++++
 drivers/clocksource/time-orion.c | 28 ++++++++++------------------
 drivers/watchdog/orion_wdt.c     |  8 ++------
 4 files changed, 47 insertions(+), 24 deletions(-)

-- 
1.8.1.5

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Ezequiel Garcia]

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.

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.

We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
respectively. The rationale for this is that some users may not require
register write completion but only thread-safe access to a register.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Based on a similar approach suggested by Russell King:
http://archive.arm.linux.org.uk/lurker/message/20130618.113606.d7d4fe4b.en.html

Changes from v2:
* As suggested by Will Deacon, dropped the iowmb() barrier
  and use relaxed variants instead. See Will's explanation for
  details:
  http://www.spinics.net/lists/arm-kernel/msg268775.html

* Use spin_{}_irqsave/restore to allow irq-context usage
  also suggested by Will Deacon.

* Re-work the API semantics as proposed by Russell King.

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

 arch/arm/include/asm/io.h |  6 ++++++
 arch/arm/kernel/io.c      | 29 +++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index d070741..27521e1 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -36,6 +36,12 @@
 #define isa_bus_to_virt phys_to_virt
 
 /*
+ * Atomic MMIO-wide IO modify
+ */
+extern void atomic_io_modify(void __iomem *reg, u32 mask, u32 set);
+extern void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set);
+
+/*
  * Generic IO read/write.  These perform native-endian accesses.  Note
  * that some architectures will want to re-define __raw_{read,write}w.
  */
diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
index dcd5b4d..651f1ad 100644
--- a/arch/arm/kernel/io.c
+++ b/arch/arm/kernel/io.c
@@ -1,6 +1,35 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
+
+static DEFINE_SPINLOCK(__io_lock);
+
+void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel_relaxed(value, reg);
+	spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify);
+
+void atomic_io_modify(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel(value, reg);
+	spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify);
 
 /*
  * Copy data from IO memory space to "real" memory space.
-- 
1.8.1.5

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Baruch Siach]

HI Ezequiel,

On Fri, Aug 23, 2013 at 07:24:03AM -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.
> 
> 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.
> 
> We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> respectively. The rationale for this is that some users may not require
> register write completion but only thread-safe access to a register.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Is there a reason why this should be limited to ARM? I haven't found anything 
ARM specific in the code.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Ezequiel Garcia]

On Fri, Aug 23, 2013 at 01:38:02PM +0300, Baruch Siach wrote:
> HI Ezequiel,
> 
> On Fri, Aug 23, 2013 at 07:24:03AM -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.
> > 
> > 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.
> > 
> > We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> > respectively. The rationale for this is that some users may not require
> > register write completion but only thread-safe access to a register.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> Is there a reason why this should be limited to ARM? I haven't found anything 
> ARM specific in the code.
> 

I guess not.

Any suggestions on where this could be located?
I can't find a suitable file in kernel/, maybe in a new file kernel/io.c?

Does this make any sense?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Ezequiel Garcia]

On Fri, Aug 23, 2013 at 08:07:50AM -0300, Ezequiel Garcia wrote:
> On Fri, Aug 23, 2013 at 01:38:02PM +0300, Baruch Siach wrote:
> > HI Ezequiel,
> > 
> > On Fri, Aug 23, 2013 at 07:24:03AM -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.
> > > 
> > > 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.
> > > 
> > > We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> > > respectively. The rationale for this is that some users may not require
> > > register write completion but only thread-safe access to a register.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > 
> > Is there a reason why this should be limited to ARM? I haven't found anything 
> > ARM specific in the code.
> > 
> 
> I guess not.
> 

... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
to exist in every architecture. So, indeed, this seems to be ARM-dependent.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Catalin Marinas]

On Fri, Aug 23, 2013 at 12:32:26PM +0100, Ezequiel Garcia wrote:
> On Fri, Aug 23, 2013 at 08:07:50AM -0300, Ezequiel Garcia wrote:
> > On Fri, Aug 23, 2013 at 01:38:02PM +0300, Baruch Siach wrote:
> > > On Fri, Aug 23, 2013 at 07:24:03AM -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.
> > > > 
> > > > 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.
> > > > 
> > > > We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> > > > respectively. The rationale for this is that some users may not require
> > > > register write completion but only thread-safe access to a register.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > 
> > > Is there a reason why this should be limited to ARM? I haven't found anything 
> > > ARM specific in the code.
> > 
> > I guess not.
> 
> ... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
> to exist in every architecture. So, indeed, this seems to be ARM-dependent.

There was a discussion couple of years ago to make these part of the IO
specification since many architectures define them:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626

(and some older threads on linux-arch which I haven't searched)

We could have some default implementation pointing to readl/writel while
letting the arch code to define more optimised variants.

-- 
Catalin

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Will Deacon]

On Fri, Aug 23, 2013 at 12:48:05PM +0100, Catalin Marinas wrote:
> On Fri, Aug 23, 2013 at 12:32:26PM +0100, Ezequiel Garcia wrote:
> > On Fri, Aug 23, 2013 at 08:07:50AM -0300, Ezequiel Garcia wrote:
> > > On Fri, Aug 23, 2013 at 01:38:02PM +0300, Baruch Siach wrote:
> > > > On Fri, Aug 23, 2013 at 07:24:03AM -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.
> > > > > 
> > > > > 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.
> > > > > 
> > > > > We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> > > > > respectively. The rationale for this is that some users may not require
> > > > > register write completion but only thread-safe access to a register.
> > > > > 
> > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > 
> > > > Is there a reason why this should be limited to ARM? I haven't found anything 
> > > > ARM specific in the code.
> > > 
> > > I guess not.
> > 
> > ... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
> > to exist in every architecture. So, indeed, this seems to be ARM-dependent.
> 
> There was a discussion couple of years ago to make these part of the IO
> specification since many architectures define them:
> 
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
> 
> (and some older threads on linux-arch which I haven't searched)
> 
> We could have some default implementation pointing to readl/writel while
> letting the arch code to define more optimised variants.

The main thing I dislike about that is the back-to-back dsbs that you will
get from the read-(modify)-write. It really makes the non-optimised version
needlessly expensive.

Will

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Catalin Marinas]

On Fri, Aug 30, 2013 at 10:08:07AM +0100, Will Deacon wrote:
> On Fri, Aug 23, 2013 at 12:48:05PM +0100, Catalin Marinas wrote:
> > On Fri, Aug 23, 2013 at 12:32:26PM +0100, Ezequiel Garcia wrote:
> > > On Fri, Aug 23, 2013 at 08:07:50AM -0300, Ezequiel Garcia wrote:
> > > > On Fri, Aug 23, 2013 at 01:38:02PM +0300, Baruch Siach wrote:
> > > > > On Fri, Aug 23, 2013 at 07:24:03AM -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.
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> > > > > > respectively. The rationale for this is that some users may not require
> > > > > > register write completion but only thread-safe access to a register.
> > > > > > 
> > > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > > 
> > > > > Is there a reason why this should be limited to ARM? I haven't found anything 
> > > > > ARM specific in the code.
> > > > 
> > > > I guess not.
> > > 
> > > ... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
> > > to exist in every architecture. So, indeed, this seems to be ARM-dependent.
> > 
> > There was a discussion couple of years ago to make these part of the IO
> > specification since many architectures define them:
> > 
> > http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
> > 
> > (and some older threads on linux-arch which I haven't searched)
> > 
> > We could have some default implementation pointing to readl/writel while
> > letting the arch code to define more optimised variants.
> 
> The main thing I dislike about that is the back-to-back dsbs that you will
> get from the read-(modify)-write. It really makes the non-optimised version
> needlessly expensive.

Yes, it's pretty bad. But we don't have relaxed (write) accessors on
other architectures and I'm not sure about their semantics either. I
guess here it's a data dependency so you cannot write the value before
reading it, especially since sane architectures should speculate reads
or writes to device memory.

What about making it always use *_relaxed() accessors if the
architecture provides them? No need for atomic_io_modify_relaxed().

-- 
Catalin

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Will Deacon]

On Fri, Aug 30, 2013 at 10:15:36AM +0100, Catalin Marinas wrote:
> On Fri, Aug 30, 2013 at 10:08:07AM +0100, Will Deacon wrote:
> > On Fri, Aug 23, 2013 at 12:48:05PM +0100, Catalin Marinas wrote:
> > > On Fri, Aug 23, 2013 at 12:32:26PM +0100, Ezequiel Garcia wrote:
> > > > ... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
> > > > to exist in every architecture. So, indeed, this seems to be ARM-dependent.
> > > 
> > > There was a discussion couple of years ago to make these part of the IO
> > > specification since many architectures define them:
> > > 
> > > http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
> > > 
> > > (and some older threads on linux-arch which I haven't searched)
> > > 
> > > We could have some default implementation pointing to readl/writel while
> > > letting the arch code to define more optimised variants.
> > 
> > The main thing I dislike about that is the back-to-back dsbs that you will
> > get from the read-(modify)-write. It really makes the non-optimised version
> > needlessly expensive.
> 
> Yes, it's pretty bad. But we don't have relaxed (write) accessors on
> other architectures and I'm not sure about their semantics either. I
> guess here it's a data dependency so you cannot write the value before
> reading it, especially since sane architectures should speculate reads
> or writes to device memory.
> 
> What about making it always use *_relaxed() accessors if the
> architecture provides them? No need for atomic_io_modify_relaxed().

The only potential problem there is if somebody uses this function to kick
off a DMA. That would require explicit barriers to enforce ordering against
population of normal, cacheable buffers, which isn't usually the case in
driver code (since we have the dsb/outer_sync in the accessor).

Perhaps we should just bit the bullet and define relaxed accessors for all
architectures? It's not difficult to default them to the non-relaxed
variants if the architecture doesn't provide an optimised implementation.

Will

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Catalin Marinas]

On Fri, Aug 30, 2013 at 10:20:33AM +0100, Will Deacon wrote:
> On Fri, Aug 30, 2013 at 10:15:36AM +0100, Catalin Marinas wrote:
> > On Fri, Aug 30, 2013 at 10:08:07AM +0100, Will Deacon wrote:
> > > On Fri, Aug 23, 2013 at 12:48:05PM +0100, Catalin Marinas wrote:
> > > > On Fri, Aug 23, 2013 at 12:32:26PM +0100, Ezequiel Garcia wrote:
> > > > > ... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
> > > > > to exist in every architecture. So, indeed, this seems to be ARM-dependent.
> > > > 
> > > > There was a discussion couple of years ago to make these part of the IO
> > > > specification since many architectures define them:
> > > > 
> > > > http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
> > > > 
> > > > (and some older threads on linux-arch which I haven't searched)
> > > > 
> > > > We could have some default implementation pointing to readl/writel while
> > > > letting the arch code to define more optimised variants.
> > > 
> > > The main thing I dislike about that is the back-to-back dsbs that you will
> > > get from the read-(modify)-write. It really makes the non-optimised version
> > > needlessly expensive.
> > 
> > Yes, it's pretty bad. But we don't have relaxed (write) accessors on
> > other architectures and I'm not sure about their semantics either. I
> > guess here it's a data dependency so you cannot write the value before
> > reading it, especially since sane architectures should speculate reads
> > or writes to device memory.
> > 
> > What about making it always use *_relaxed() accessors if the
> > architecture provides them? No need for atomic_io_modify_relaxed().
> 
> The only potential problem there is if somebody uses this function to kick
> off a DMA. That would require explicit barriers to enforce ordering against
> population of normal, cacheable buffers, which isn't usually the case in
> driver code (since we have the dsb/outer_sync in the accessor).
> 
> Perhaps we should just bit the bullet and define relaxed accessors for all
> architectures? It's not difficult to default them to the non-relaxed
> variants if the architecture doesn't provide an optimised implementation.

Yes, an asm-generic default relaxed would be good (that's what I
suggested earlier in this thread and it was discussed in the past). But
no-one volunteered ;).

-- 
Catalin

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Jason Gunthorpe]

On Fri, Aug 30, 2013 at 11:03:42AM +0100, Catalin Marinas wrote:

> > Perhaps we should just bit the bullet and define relaxed accessors for all
> > architectures? It's not difficult to default them to the non-relaxed
> > variants if the architecture doesn't provide an optimised implementation.
> 
> Yes, an asm-generic default relaxed would be good (that's what I
> suggested earlier in this thread and it was discussed in the past). But
> no-one volunteered ;).

Something I've always been confused about..

Do these _relaxed operators on ARM differ from the PCI-X definition of
relaxed ordering, and are they expected to generate a PCI TLP with
the relaxed ordering bit set?

If so, what does writel_relaxed do? RO has no effect on transactions
travelling away from the PCI host bridge, so it is useless for the
CPU to generate RO TLPs.

AFAIK, on x86 read_relaxed is expected to cause the PCI behavior.
Documentation/DocBook/deviceiobook.tmpl seems to confirm this.

It seems important to reconcile the meaning before standardizing these
things :)

Jason

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Russell King - ARM Linux]

On Fri, Aug 30, 2013 at 02:08:30PM -0600, Jason Gunthorpe wrote:
> On Fri, Aug 30, 2013 at 11:03:42AM +0100, Catalin Marinas wrote:
> 
> > > Perhaps we should just bit the bullet and define relaxed accessors for all
> > > architectures? It's not difficult to default them to the non-relaxed
> > > variants if the architecture doesn't provide an optimised implementation.
> > 
> > Yes, an asm-generic default relaxed would be good (that's what I
> > suggested earlier in this thread and it was discussed in the past). But
> > no-one volunteered ;).
> 
> Something I've always been confused about..
> 
> Do these _relaxed operators on ARM differ from the PCI-X definition of
> relaxed ordering, and are they expected to generate a PCI TLP with
> the relaxed ordering bit set?

It's more to do with the memory model.  The _relaxed IO accessors just
read/write to the desired IO location without any ordering with respect
to memory - so if you have for instance DMA coherent memory, and you've
written a set of descriptors to that memory, there is no implicit
guarantee that those writes will be visible to the DMA engine if you
enable it using a relaxed operator unless you use an appropriate barrier
between the two.

Conversely, reading a register and then accessing memory, there is no
guarantee that you will have read the register before the memory access
has been performed with the relaxed IO.

The standard IO operations (readl, writel) contain barriers which ensure
correct ordering - the write*() have a barrier before their write, and
the read*() have a barrier after their read.  This ensures correct and
expected ordering with DMA coherent memory.

However, those barriers can be very expensive - they end up having to
themselves issue a write to the L2 cache controller (which obviously
can't use the standard IO operations).

> AFAIK, on x86 read_relaxed is expected to cause the PCI behavior.
> Documentation/DocBook/deviceiobook.tmpl seems to confirm this.

I guess our read* is compatible with that (even though it has nothing
to do with PCI) - the issues in Docbook appear to suggest similar
behaviours from the CPU point of view - IOW, the visibility of DMA vs
the read transaction.  The write is just an extension of that.

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Gregory CLEMENT]

Hi all,

On 30/08/2013 12:03, Catalin Marinas wrote:
> On Fri, Aug 30, 2013 at 10:20:33AM +0100, Will Deacon wrote:
>> On Fri, Aug 30, 2013 at 10:15:36AM +0100, Catalin Marinas wrote:
>>> On Fri, Aug 30, 2013 at 10:08:07AM +0100, Will Deacon wrote:
>>>> On Fri, Aug 23, 2013 at 12:48:05PM +0100, Catalin Marinas wrote:
>>>>> On Fri, Aug 23, 2013 at 12:32:26PM +0100, Ezequiel Garcia wrote:
>>>>>> ... or maybe yes. I'm not seeing {readl,writel}_relaxed as guaranteed
>>>>>> to exist in every architecture. So, indeed, this seems to be ARM-dependent.
>>>>>
>>>>> There was a discussion couple of years ago to make these part of the IO
>>>>> specification since many architectures define them:
>>>>>
>>>>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
>>>>>
>>>>> (and some older threads on linux-arch which I haven't searched)
>>>>>
>>>>> We could have some default implementation pointing to readl/writel while
>>>>> letting the arch code to define more optimised variants.
>>>>
>>>> The main thing I dislike about that is the back-to-back dsbs that you will
>>>> get from the read-(modify)-write. It really makes the non-optimised version
>>>> needlessly expensive.
>>>
>>> Yes, it's pretty bad. But we don't have relaxed (write) accessors on
>>> other architectures and I'm not sure about their semantics either. I
>>> guess here it's a data dependency so you cannot write the value before
>>> reading it, especially since sane architectures should speculate reads
>>> or writes to device memory.
>>>
>>> What about making it always use *_relaxed() accessors if the
>>> architecture provides them? No need for atomic_io_modify_relaxed().
>>
>> The only potential problem there is if somebody uses this function to kick
>> off a DMA. That would require explicit barriers to enforce ordering against
>> population of normal, cacheable buffers, which isn't usually the case in
>> driver code (since we have the dsb/outer_sync in the accessor).
>>
>> Perhaps we should just bit the bullet and define relaxed accessors for all
>> architectures? It's not difficult to default them to the non-relaxed
>> variants if the architecture doesn't provide an optimised implementation.
> 
> Yes, an asm-generic default relaxed would be good (that's what I
> suggested earlier in this thread and it was discussed in the past). But
> no-one volunteered ;).
> 

I would like to make the things move on about this subject. Should it
be possible to merge this version of the patch set? Currently the
only users of this new API are drivers for ARM SoCs.

In the meantime, I am willing to introduce an asm-generic default
relaxed variant of read and write, but as Catalin had already pointed
in the past
http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
it may take time to get an agreement from all the other architectures.

That's why I propose that this patch set do not depend on the
introduction of a asm-generic default relaxed variant of read and
write. Later when it will be accepted then this new API will be moved
in the asm-generic part.

Thanks,

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Will Deacon]

On Thu, Sep 05, 2013 at 09:59:40AM +0100, Gregory CLEMENT wrote:
> Hi all,

Hi Gregory,

> On 30/08/2013 12:03, Catalin Marinas wrote:
> > On Fri, Aug 30, 2013 at 10:20:33AM +0100, Will Deacon wrote:
> >> Perhaps we should just bit the bullet and define relaxed accessors for all
> >> architectures? It's not difficult to default them to the non-relaxed
> >> variants if the architecture doesn't provide an optimised implementation.
> > 
> > Yes, an asm-generic default relaxed would be good (that's what I
> > suggested earlier in this thread and it was discussed in the past). But
> > no-one volunteered ;).
> > 
> 
> I would like to make the things move on about this subject. Should it
> be possible to merge this version of the patch set? Currently the
> only users of this new API are drivers for ARM SoCs.

In the short term then, I'd keep this restricted to ARM. Without relaxed
accessors available across all architectures, I don't think we can make this
usefully generic (a readX/writeX version would be horribly slow on ARM).

> In the meantime, I am willing to introduce an asm-generic default
> relaxed variant of read and write, but as Catalin had already pointed
> in the past
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
> it may take time to get an agreement from all the other architectures.

I actually wrote those patches yesterday but, in doing so, I realised that
the kernel doesn't have well-defined semantics for relaxed I/O accessors
(i.e. different architectures use them to mean different things). That makes
portability is an issue, even if the functions are defined everywhere.

> That's why I propose that this patch set do not depend on the
> introduction of a asm-generic default relaxed variant of read and
> write. Later when it will be accepted then this new API will be moved
> in the asm-generic part.

Indeed, we can't make this generic until we've resolved the issues I mention
above. I'll start a new thread around that once I've put my thoughts
together, but that needn't block this patch imo.

That said, I'd still suggest resending a final version of this patch, since
there were some minor issues with this iteration.

Cheers,

Will

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Gregory CLEMENT]

On 05/09/2013 11:08, Will Deacon wrote:
> On Thu, Sep 05, 2013 at 09:59:40AM +0100, Gregory CLEMENT wrote:
>> Hi all,
> 
> Hi Gregory,
> 
>> On 30/08/2013 12:03, Catalin Marinas wrote:
>>> On Fri, Aug 30, 2013 at 10:20:33AM +0100, Will Deacon wrote:
>>>> Perhaps we should just bit the bullet and define relaxed accessors for all
>>>> architectures? It's not difficult to default them to the non-relaxed
>>>> variants if the architecture doesn't provide an optimised implementation.
>>>
>>> Yes, an asm-generic default relaxed would be good (that's what I
>>> suggested earlier in this thread and it was discussed in the past). But
>>> no-one volunteered ;).
>>>
>>
>> I would like to make the things move on about this subject. Should it
>> be possible to merge this version of the patch set? Currently the
>> only users of this new API are drivers for ARM SoCs.
> 
> In the short term then, I'd keep this restricted to ARM. Without relaxed
> accessors available across all architectures, I don't think we can make this
> usefully generic (a readX/writeX version would be horribly slow on ARM).

OK

> 
>> In the meantime, I am willing to introduce an asm-generic default
>> relaxed variant of read and write, but as Catalin had already pointed
>> in the past
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
>> it may take time to get an agreement from all the other architectures.
> 
> I actually wrote those patches yesterday but, in doing so, I realised that

Great!

> the kernel doesn't have well-defined semantics for relaxed I/O accessors
> (i.e. different architectures use them to mean different things). That makes
> portability is an issue, even if the functions are defined everywhere.

It confirms that Catalin pointed in this old thread.

> 
>> That's why I propose that this patch set do not depend on the
>> introduction of a asm-generic default relaxed variant of read and
>> write. Later when it will be accepted then this new API will be moved
>> in the asm-generic part.
> 
> Indeed, we can't make this generic until we've resolved the issues I mention
> above. I'll start a new thread around that once I've put my thoughts
> together, but that needn't block this patch imo.

I will definitely have a look on this thread!

> 
> That said, I'd still suggest resending a final version of this patch, since
> there were some minor issues with this iteration.

Yes sure, I suspect that Ezequiel already fixed  all the issues and is ready
to send a new iteration.

> 
> Cheers,
> 
> Will
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Jason Gunthorpe]

On Thu, Sep 05, 2013 at 10:08:42AM +0100, Will Deacon wrote:

> > I would like to make the things move on about this subject. Should it
> > be possible to merge this version of the patch set? Currently the
> > only users of this new API are drivers for ARM SoCs.
> 
> In the short term then, I'd keep this restricted to ARM. Without relaxed
> accessors available across all architectures, I don't think we can make this
> usefully generic (a readX/writeX version would be horribly slow on ARM).

Realistically though, the current use cases for this function have it
called only a few times during startup, so it 'horribly slow' isn't
really important at all.

So why not start with this in generic headers:

extern void atomic_io_modify(void __iomem *reg, u32 mask, u32 set);
#define atomic_io_modify_relaxed atomic_io_modify

And the relaxed optimization can come along in due course?

Jason

[PATCH v3 1/3] ARM: Introduce atomic MMIO modify

[Ezequiel Garcia]

On Fri, Aug 23, 2013 at 07:24:03AM -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.
> 
> 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.
> 
> We add relaxed and non-relaxed variants, by using writel_relaxed and writel,
> respectively. The rationale for this is that some users may not require
> register write completion but only thread-safe access to a register.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> Based on a similar approach suggested by Russell King:
> http://archive.arm.linux.org.uk/lurker/message/20130618.113606.d7d4fe4b.en.html
> 
> Changes from v2:
> * As suggested by Will Deacon, dropped the iowmb() barrier
>   and use relaxed variants instead. See Will's explanation for
>   details:
>   http://www.spinics.net/lists/arm-kernel/msg268775.html
> 
> * Use spin_{}_irqsave/restore to allow irq-context usage
>   also suggested by Will Deacon.
> 
> * Re-work the API semantics as proposed by Russell King.
> 
> Changes from v1:
> * Added an io barrier iowmb() as suggested by Will Deacon,
>   to ensure the writel gets completed before the spin_unlock().
> 
>  arch/arm/include/asm/io.h |  6 ++++++
>  arch/arm/kernel/io.c      | 29 +++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index d070741..27521e1 100644
> --- a/arch/arm/include/asm/io.h
> +++ b/arch/arm/include/asm/io.h
> @@ -36,6 +36,12 @@
>  #define isa_bus_to_virt phys_to_virt
>  
>  /*
> + * Atomic MMIO-wide IO modify
> + */
> +extern void atomic_io_modify(void __iomem *reg, u32 mask, u32 set);
> +extern void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set);
> +
> +/*
>   * Generic IO read/write.  These perform native-endian accesses.  Note
>   * that some architectures will want to re-define __raw_{read,write}w.
>   */
> diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
> index dcd5b4d..651f1ad 100644
> --- a/arch/arm/kernel/io.c
> +++ b/arch/arm/kernel/io.c
> @@ -1,6 +1,35 @@
>  #include <linux/export.h>
>  #include <linux/types.h>
>  #include <linux/io.h>
> +#include <linux/spinlock.h>
> +
> +static DEFINE_SPINLOCK(__io_lock);
> +
> +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> +{
> +	unsigned long flags;
> +	u32 value;
> +
> +	spin_lock_irqsave(&__io_lock, flags);
> +	value = readl_relaxed(reg) & ~mask;
> +	value |= (set & mask);
> +	writel_relaxed(value, reg);
> +	spin_unlock_irqrestore(&__io_lock, flags);
> +}

And before anyone else screams at me, here's another typo:

> +EXPORT_SYMBOL(atomic_io_modify);
> +


-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

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

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/clocksource/time-orion.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/clocksource/time-orion.c b/drivers/clocksource/time-orion.c
index ecbeb68..da9379e 100644
--- a/drivers/clocksource/time-orion.c
+++ b/drivers/clocksource/time-orion.c
@@ -35,20 +35,6 @@
 #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)
-{
-	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);
 
 /*
  * 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);
 
 	return 0;
 }
@@ -80,10 +67,13 @@ static void orion_clkevt_mode(enum clock_event_mode mode,
 		/* setup and enable periodic timer at 1/HZ intervals */
 		writel(ticks_per_jiffy - 1, timer_base + TIMER1_RELOAD);
 		writel(ticks_per_jiffy - 1, timer_base + TIMER1_VAL);
-		orion_timer_ctrl_clrset(0, TIMER1_RELOAD_EN | TIMER1_EN);
+		atomic_io_modify(timer_base + TIMER_CTRL,
+			TIMER1_RELOAD_EN | TIMER1_EN,
+			TIMER1_RELOAD_EN | TIMER1_EN);
 	} else {
 		/* disable timer */
-		orion_timer_ctrl_clrset(TIMER1_RELOAD_EN | TIMER1_EN, 0);
+		atomic_io_modify(timer_base + TIMER_CTRL,
+			TIMER1_RELOAD_EN | TIMER1_EN, 0);
 	}
 }
 
@@ -131,7 +121,9 @@ static void __init orion_timer_init(struct device_node *np)
 	/* setup timer0 as free-running clocksource */
 	writel(~0, timer_base + TIMER0_VAL);
 	writel(~0, timer_base + TIMER0_RELOAD);
-	orion_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | TIMER0_EN);
+	oatomic_io_modify(timer_base + TIMER_CTRL,
+		TIMER0_RELOAD_EN | TIMER0_EN,
+		TIMER0_RELOAD_EN | TIMER0_EN);
 	clocksource_mmio_init(timer_base + TIMER0_VAL, "orion_clocksource",
 			      clk_get_rate(clk), 300, 32,
 			      clocksource_mmio_readl_down);
-- 
1.8.1.5

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

[Baruch Siach]

Hi Ezequiel,

On Fri, Aug 23, 2013 at 07:24:04AM -0300, Ezequiel Garcia wrote:
> Replace the driver-specific thread-safe shared register API
> by the recently introduced atomic_io_clear_set().
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

[...]

> @@ -131,7 +121,9 @@ static void __init orion_timer_init(struct device_node 
>   *np)
>  	/* setup timer0 as free-running clocksource */
>  	writel(~0, timer_base + TIMER0_VAL);
>  	writel(~0, timer_base + TIMER0_RELOAD);
> -	orion_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | TIMER0_EN);
> +	oatomic_io_modify(timer_base + TIMER_CTRL,

oatomic? Are you sure this code builds?

baruch

> +		TIMER0_RELOAD_EN | TIMER0_EN,
> +		TIMER0_RELOAD_EN | TIMER0_EN);
>  	clocksource_mmio_init(timer_base + TIMER0_VAL, "orion_clocksource",
>  			      clk_get_rate(clk), 300, 32,
>  			      clocksource_mmio_readl_down);

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

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

[Ezequiel Garcia]

On Fri, Aug 23, 2013 at 01:38:57PM +0300, Baruch Siach wrote:
> Hi Ezequiel,
> 
> On Fri, Aug 23, 2013 at 07:24:04AM -0300, Ezequiel Garcia wrote:
> > Replace the driver-specific thread-safe shared register API
> > by the recently introduced atomic_io_clear_set().
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> [...]
> 
> > @@ -131,7 +121,9 @@ static void __init orion_timer_init(struct device_node 
> >   *np)
> >  	/* setup timer0 as free-running clocksource */
> >  	writel(~0, timer_base + TIMER0_VAL);
> >  	writel(~0, timer_base + TIMER0_RELOAD);
> > -	orion_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | TIMER0_EN);
> > +	oatomic_io_modify(timer_base + TIMER_CTRL,
> 
> oatomic? Are you sure this code builds?
> 

Argh, no. I wasn't building the proper configuration/tree
and thus this clocksource driver wasn't being used.

Thanks for the catch!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

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@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..cfc037d 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -73,9 +73,7 @@ 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_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
 	/* Enable reset on watchdog */
 	reg = readl(RSTOUTn_MASK);
@@ -98,9 +96,7 @@ 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);
+	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
 
 	spin_unlock(&wdt_lock);
 	return 0;
-- 
1.8.1.5