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

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

[Ezequiel Garcia]

After this patchset stalled for several months, I'm picking where
we left. The last conclusion [1] was that instead of doing the arch-generic
from scratch, we'd start by implementing the API in ARM only.

In other words, this is basically a resend of v3, with some of the typos fixed
and using raw spinlocks.

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

The patch that adds the atomic-modify API is PATCH 1. The other two are sent
as usage examples.

Changes from v4:
* Returned to the ARM-specific implementation for this API. The arch-generic
  can be added later.

Changes from v3:
* Implemented an arch-generic atomic_io_modify(), as suggested by Baruch
  and Catalin.

* Add an ARM-specific variant, using relaxed R/W as Will suggested.

* Replaced spin_locks by raw_spin_locks, to protect the registers
  even on RT.

* Fixed two stupid typos.

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().

If nobody has objections, I'll add patch 1/3 to ARM's tracking system
as usual.

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             | 35 +++++++++++++++++++++++++++++++++++
 drivers/clocksource/time-orion.c | 28 ++++++++++------------------
 drivers/watchdog/orion_wdt.c     |  8 ++------
 4 files changed, 53 insertions(+), 24 deletions(-)

-- 
1.8.1.5

[PATCH v5 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>
---
 arch/arm/include/asm/io.h |  6 ++++++
 arch/arm/kernel/io.c      | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 3c597c2..05722ac 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -38,6 +38,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..9203cf8 100644
--- a/arch/arm/kernel/io.c
+++ b/arch/arm/kernel/io.c
@@ -1,6 +1,41 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
+
+static DEFINE_RAW_SPINLOCK(__io_lock);
+
+/*
+ * Generic atomic MMIO modify.
+ *
+ * Allows thread-safe access to registers shared by unrelated subsystems.
+ * The access is protected by a single MMIO-wide lock.
+ */
+void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel_relaxed(value, reg);
+	raw_spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify_relaxed);
+
+void atomic_io_modify(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel(value, reg);
+	raw_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 v5 1/3] ARM: Introduce atomic MMIO modify

[Mark Brown]

On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:

> +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> +{
> +	unsigned long flags;
> +	u32 value;
> +
> +	raw_spin_lock_irqsave(&__io_lock, flags);
> +	value = readl_relaxed(reg) & ~mask;
> +	value |= (set & mask);
> +	writel_relaxed(value, reg);
> +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> +}
> +EXPORT_SYMBOL(atomic_io_modify_relaxed);

This looks quite generic - why is it in architecture specific code?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131210/a9b3a487/attachment.sig>

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

[Russell King - ARM Linux]

On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> 
> > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > +{
> > +	unsigned long flags;
> > +	u32 value;
> > +
> > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > +	value = readl_relaxed(reg) & ~mask;
> > +	value |= (set & mask);
> > +	writel_relaxed(value, reg);
> > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > +}
> > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> 
> This looks quite generic - why is it in architecture specific code?

because the _relaxed IO operators don't exist on other architectures, and
there's been some discussion around whether they should with no conclusions
being reached.

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

[Mark Brown]

On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:

> > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);

> > This looks quite generic - why is it in architecture specific code?

> because the _relaxed IO operators don't exist on other architectures, and
> there's been some discussion around whether they should with no conclusions
> being reached.

Oh, that's depressing.  I guess the non-relaxed version could be made
generic but meh.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131210/97105f31/attachment.sig>

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

[Ezequiel Garcia]

On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> > On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> > 
> > > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > > +{
> > > +	unsigned long flags;
> > > +	u32 value;
> > > +
> > > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > > +	value = readl_relaxed(reg) & ~mask;
> > > +	value |= (set & mask);
> > > +	writel_relaxed(value, reg);
> > > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > > +}
> > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> > 
> > This looks quite generic - why is it in architecture specific code?
> 
> because the _relaxed IO operators don't exist on other architectures, and
> there's been some discussion around whether they should with no conclusions
> being reached.

Exactly.

Will? Catalin? Any comments on this?

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

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

[Will Deacon]

On Wed, Dec 11, 2013 at 08:49:43PM +0000, Ezequiel Garcia wrote:
> On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> > > On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> > > 
> > > > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > > > +{
> > > > +	unsigned long flags;
> > > > +	u32 value;
> > > > +
> > > > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > > > +	value = readl_relaxed(reg) & ~mask;
> > > > +	value |= (set & mask);
> > > > +	writel_relaxed(value, reg);
> > > > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > > > +}
> > > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> > > 
> > > This looks quite generic - why is it in architecture specific code?
> > 
> > because the _relaxed IO operators don't exist on other architectures, and
> > there's been some discussion around whether they should with no conclusions
> > being reached.
> 
> Exactly.
> 
> Will? Catalin? Any comments on this?

Yes: BenH and I managed to come up with an agreement on the relaxed I/O
accessors during kernel summit which I need to write up and send out again.
This would allow for a generic definition of the accessors, then this could
potentially be done in core code.

That said, I think the code you have will work correctly for ARM.

Will

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

[Jason Cooper]

On Thu, Dec 12, 2013 at 01:58:19PM +0000, Will Deacon wrote:
> On Wed, Dec 11, 2013 at 08:49:43PM +0000, Ezequiel Garcia wrote:
> > On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> > > > On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> > > > 
> > > > > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > > > > +{
> > > > > +	unsigned long flags;
> > > > > +	u32 value;
> > > > > +
> > > > > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > > > > +	value = readl_relaxed(reg) & ~mask;
> > > > > +	value |= (set & mask);
> > > > > +	writel_relaxed(value, reg);
> > > > > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > > > > +}
> > > > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> > > > 
> > > > This looks quite generic - why is it in architecture specific code?
> > > 
> > > because the _relaxed IO operators don't exist on other architectures, and
> > > there's been some discussion around whether they should with no conclusions
> > > being reached.
> > 
> > Exactly.
> > 
> > Will? Catalin? Any comments on this?
> 
> Yes: BenH and I managed to come up with an agreement on the relaxed I/O
> accessors during kernel summit which I need to write up and send out again.
> This would allow for a generic definition of the accessors, then this could
> potentially be done in core code.

Do you prefer this series to wait then?

thx,

Jason.

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

[Will Deacon]

On Thu, Dec 12, 2013 at 02:02:53PM +0000, Jason Cooper wrote:
> On Thu, Dec 12, 2013 at 01:58:19PM +0000, Will Deacon wrote:
> > On Wed, Dec 11, 2013 at 08:49:43PM +0000, Ezequiel Garcia wrote:
> > > On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> > > > On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> > > > > On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> > > > > 
> > > > > > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > > > > > +{
> > > > > > +	unsigned long flags;
> > > > > > +	u32 value;
> > > > > > +
> > > > > > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > > > > > +	value = readl_relaxed(reg) & ~mask;
> > > > > > +	value |= (set & mask);
> > > > > > +	writel_relaxed(value, reg);
> > > > > > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> > > > > 
> > > > > This looks quite generic - why is it in architecture specific code?
> > > > 
> > > > because the _relaxed IO operators don't exist on other architectures, and
> > > > there's been some discussion around whether they should with no conclusions
> > > > being reached.
> > > 
> > > Exactly.
> > > 
> > > Will? Catalin? Any comments on this?
> > 
> > Yes: BenH and I managed to come up with an agreement on the relaxed I/O
> > accessors during kernel summit which I need to write up and send out again.
> > This would allow for a generic definition of the accessors, then this could
> > potentially be done in core code.
> 
> Do you prefer this series to wait then?

It'd be pretty unfair of me to insist that you wait; particularly when
there's bound to be further discussion once I get a proposal out. I guess
all I can ask is that you guys try to move this into core code once we have
standard definitions for the relaxed accessors.

Is that reasonable?

Will

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

[Jason Cooper]

On Thu, Dec 12, 2013 at 02:07:35PM +0000, Will Deacon wrote:
> On Thu, Dec 12, 2013 at 02:02:53PM +0000, Jason Cooper wrote:
> > On Thu, Dec 12, 2013 at 01:58:19PM +0000, Will Deacon wrote:
> > > On Wed, Dec 11, 2013 at 08:49:43PM +0000, Ezequiel Garcia wrote:
> > > > On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> > > > > On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> > > > > > On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> > > > > > 
> > > > > > > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > > > > > > +{
> > > > > > > +	unsigned long flags;
> > > > > > > +	u32 value;
> > > > > > > +
> > > > > > > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > > > > > > +	value = readl_relaxed(reg) & ~mask;
> > > > > > > +	value |= (set & mask);
> > > > > > > +	writel_relaxed(value, reg);
> > > > > > > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> > > > > > 
> > > > > > This looks quite generic - why is it in architecture specific code?
> > > > > 
> > > > > because the _relaxed IO operators don't exist on other architectures, and
> > > > > there's been some discussion around whether they should with no conclusions
> > > > > being reached.
> > > > 
> > > > Exactly.
> > > > 
> > > > Will? Catalin? Any comments on this?
> > > 
> > > Yes: BenH and I managed to come up with an agreement on the relaxed I/O
> > > accessors during kernel summit which I need to write up and send out again.
> > > This would allow for a generic definition of the accessors, then this could
> > > potentially be done in core code.
> > 
> > Do you prefer this series to wait then?
> 
> It'd be pretty unfair of me to insist that you wait; particularly when
> there's bound to be further discussion once I get a proposal out. I guess
> all I can ask is that you guys try to move this into core code once we have
> standard definitions for the relaxed accessors.
> 
> Is that reasonable?

Fair enough, I'll add this email to my onice mail folder and we'll
re-attack it once the core code is in place.  No guarantees on timeline,
though.

thx,

Jason.

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

[Ezequiel Garcia]

On Thu, Dec 12, 2013 at 02:07:35PM +0000, Will Deacon wrote:
> On Thu, Dec 12, 2013 at 02:02:53PM +0000, Jason Cooper wrote:
> > On Thu, Dec 12, 2013 at 01:58:19PM +0000, Will Deacon wrote:
> > > On Wed, Dec 11, 2013 at 08:49:43PM +0000, Ezequiel Garcia wrote:
> > > > On Tue, Dec 10, 2013 at 05:00:25PM +0000, Russell King - ARM Linux wrote:
> > > > > On Tue, Dec 10, 2013 at 04:49:07PM +0000, Mark Brown wrote:
> > > > > > On Tue, Dec 10, 2013 at 11:41:35AM -0300, Ezequiel Garcia wrote:
> > > > > > 
> > > > > > > +void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
> > > > > > > +{
> > > > > > > +	unsigned long flags;
> > > > > > > +	u32 value;
> > > > > > > +
> > > > > > > +	raw_spin_lock_irqsave(&__io_lock, flags);
> > > > > > > +	value = readl_relaxed(reg) & ~mask;
> > > > > > > +	value |= (set & mask);
> > > > > > > +	writel_relaxed(value, reg);
> > > > > > > +	raw_spin_unlock_irqrestore(&__io_lock, flags);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(atomic_io_modify_relaxed);
> > > > > > 
> > > > > > This looks quite generic - why is it in architecture specific code?
> > > > > 
> > > > > because the _relaxed IO operators don't exist on other architectures, and
> > > > > there's been some discussion around whether they should with no conclusions
> > > > > being reached.
> > > > 
> > > > Exactly.
> > > > 
> > > > Will? Catalin? Any comments on this?
> > > 
> > > Yes: BenH and I managed to come up with an agreement on the relaxed I/O
> > > accessors during kernel summit which I need to write up and send out again.
> > > This would allow for a generic definition of the accessors, then this could
> > > potentially be done in core code.
> > 
> > Do you prefer this series to wait then?
> 
> It'd be pretty unfair of me to insist that you wait; particularly when
> there's bound to be further discussion once I get a proposal out. I guess
> all I can ask is that you guys try to move this into core code once we have
> standard definitions for the relaxed accessors.
> 
> Is that reasonable?
> 

Of course. We'll take care of doing the proper work when the relaxed I/O moves
forward.

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

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

[Russell King - ARM Linux]

On Tue, Dec 10, 2013 at 11:41:35AM -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>

Okay, so this patch has been submitted to the patch system, but it
contains no other tags other than Ezequiel's sign-off.  Clearly
other people *have* reviewed it.

Can we please have some acks etc for it please?

--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Jason Cooper]

On Thu, Jan 02, 2014 at 11:30:57AM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 10, 2013 at 11:41:35AM -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>
> 
> Okay, so this patch has been submitted to the patch system, but it
> contains no other tags other than Ezequiel's sign-off.  Clearly
> other people *have* reviewed it.
> 
> Can we please have some acks etc for it please?

Acked-by: Jason Cooper <jason@lakedaemon.net>

Happy New Year!

Jason.

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

[Ezequiel Garcia]

On Thu, Jan 02, 2014 at 09:47:24AM -0500, Jason Cooper wrote:
> On Thu, Jan 02, 2014 at 11:30:57AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Dec 10, 2013 at 11:41:35AM -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>
> > 
> > Okay, so this patch has been submitted to the patch system, but it
> > contains no other tags other than Ezequiel's sign-off.  Clearly
> > other people *have* reviewed it.
> > 
> > Can we please have some acks etc for it please?
> 
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> 

Thanks!

Will: Can you ack this patch as well?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On Thu, Jan 02, 2014 at 11:58:35AM -0300, Ezequiel Garcia wrote:
> On Thu, Jan 02, 2014 at 09:47:24AM -0500, Jason Cooper wrote:
> > On Thu, Jan 02, 2014 at 11:30:57AM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Dec 10, 2013 at 11:41:35AM -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>
> > > 
> > > Okay, so this patch has been submitted to the patch system, but it
> > > contains no other tags other than Ezequiel's sign-off.  Clearly
> > > other people *have* reviewed it.
> > > 
> > > Can we please have some acks etc for it please?
> > 
> > Acked-by: Jason Cooper <jason@lakedaemon.net>
> > 

Catalin, Will: Can you ack as well, so Russell can take this?

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

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

[Catalin Marinas]

On Sun, Jan 12, 2014 at 02:52:07PM +0000, Ezequiel Garcia wrote:
> On Thu, Jan 02, 2014 at 11:58:35AM -0300, Ezequiel Garcia wrote:
> > On Thu, Jan 02, 2014 at 09:47:24AM -0500, Jason Cooper wrote:
> > > On Thu, Jan 02, 2014 at 11:30:57AM +0000, Russell King - ARM Linux wrote:
> > > > On Tue, Dec 10, 2013 at 11:41:35AM -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>
> > > > 
> > > > Okay, so this patch has been submitted to the patch system, but it
> > > > contains no other tags other than Ezequiel's sign-off.  Clearly
> > > > other people *have* reviewed it.
> > > > 
> > > > Can we please have some acks etc for it please?
> > > 
> > > Acked-by: Jason Cooper <jason@lakedaemon.net>
> 
> Catalin, Will: Can you ack as well, so Russell can take this?

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

(with the condition that Will promises to sort out the generic relaxed
IO accessors and move this to generic code ;))

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

[Russell King - ARM Linux]

On Mon, Jan 13, 2014 at 01:58:58PM +0000, Catalin Marinas wrote:
> On Sun, Jan 12, 2014 at 02:52:07PM +0000, Ezequiel Garcia wrote:
> > Catalin, Will: Can you ack as well, so Russell can take this?
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> (with the condition that Will promises to sort out the generic relaxed
> IO accessors and move this to generic code ;))

I think that's an impossibility, as there's no such thing as a "generic
relaxed IO accessor" and agreement on what that should be is very hard
to come by.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Will Deacon]

On Mon, Jan 13, 2014 at 02:02:15PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 13, 2014 at 01:58:58PM +0000, Catalin Marinas wrote:
> > On Sun, Jan 12, 2014 at 02:52:07PM +0000, Ezequiel Garcia wrote:
> > > Catalin, Will: Can you ack as well, so Russell can take this?
> > 
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > 
> > (with the condition that Will promises to sort out the generic relaxed
> > IO accessors and move this to generic code ;))
> 
> I think that's an impossibility, as there's no such thing as a "generic
> relaxed IO accessor" and agreement on what that should be is very hard
> to come by.

We'll see... I managed to get agreement with powerpc, and I think most other
people (esp. x86) don't really care. I'll post an RFC when I've written
something up. It will likely involve use of things like mmiowb for truly
portable code, so how many drivers will actually be portable even after the
generic definition of the relaxed accessors remains to be seen.

Will

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

[Jason Cooper]

Hey Russell,

On Mon, Jan 13, 2014 at 02:02:15PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 13, 2014 at 01:58:58PM +0000, Catalin Marinas wrote:
> > On Sun, Jan 12, 2014 at 02:52:07PM +0000, Ezequiel Garcia wrote:
> > > Catalin, Will: Can you ack as well, so Russell can take this?
> > 
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > 
> > (with the condition that Will promises to sort out the generic relaxed
> > IO accessors and move this to generic code ;))
> 
> I think that's an impossibility, as there's no such thing as a "generic
> relaxed IO accessor" and agreement on what that should be is very hard
> to come by.

This has been Acked by myself and Catalin.  Do think it will make it in
to v3.14?

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7930/1

thx,

Jason.

[PATCH v5 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 9c7f018..3f14e56 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);
+	atomic_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 v5 2/3] clocksource: orion: Use atomic access for shared registers

[Jason Cooper]

On Tue, Dec 10, 2013 at 11:41:36AM -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>
> ---
>  drivers/clocksource/time-orion.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

[PATCH v5 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 44edca6..adeb17f 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

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

[Jason Cooper]

On Tue, Dec 10, 2013 at 11:41:37AM -0300, Ezequiel Garcia wrote:
> 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(-)

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

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

[Jason Cooper]

On Tue, Dec 10, 2013 at 11:41:34AM -0300, Ezequiel Garcia wrote:
> After this patchset stalled for several months, I'm picking where
> we left. The last conclusion [1] was that instead of doing the arch-generic
> from scratch, we'd start by implementing the API in ARM only.
> 
> In other words, this is basically a resend of v3, with some of the typos fixed
> and using raw spinlocks.
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg271773.html
...
> If nobody has objections, I'll add patch 1/3 to ARM's tracking system
> as usual.

It looks like everyone's ok with that.  We can submit the drivers for
v3.15 unless Russell's willing to set up a topic branch for patches 2
and 3 to depend upon.

thx,

Jason.

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

[Ezequiel Garcia]

On Thu, Dec 12, 2013 at 08:52:21AM -0500, Jason Cooper wrote:
> On Tue, Dec 10, 2013 at 11:41:34AM -0300, Ezequiel Garcia wrote:
> > After this patchset stalled for several months, I'm picking where
> > we left. The last conclusion [1] was that instead of doing the arch-generic
> > from scratch, we'd start by implementing the API in ARM only.
> > 
> > In other words, this is basically a resend of v3, with some of the typos fixed
> > and using raw spinlocks.
> > 
> > [1] http://www.spinics.net/lists/arm-kernel/msg271773.html
> ...
> > If nobody has objections, I'll add patch 1/3 to ARM's tracking system
> > as usual.
> 
> It looks like everyone's ok with that.  We can submit the drivers for
> v3.15 unless Russell's willing to set up a topic branch for patches 2
> and 3 to depend upon.
> 

IMO, v3.15 is perfectly fine, there's no rush on the drivers.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com