Introduce atomic MMIO register modify
v3 cover Introduce atomic MMIO register modify
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(-)
Signature · 1 line
1.8.1.5
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
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.
---
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(+)
arch/arm/include/asm/io.h +6 -0
@@ -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. */
arch/arm/kernel/io.c +29 -0
@@ -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.
Signature · 1 line
1.8.1.5
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
HI Ezequiel,
Quoted 8 lines · 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.
Quoted 5 lines
We add relaxed and non-relaxed variants, by using
writel_relaxedand 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 <[email protected]>
Is there a reason why this should be limited to ARM? I haven't found anything ARM specific in the code.
baruch
Signature · 3 lines
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
Quoted 11 lines · Baruch Siach
HI Ezequiel,
Quoted 8 lines · 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.
Quoted 9 lines
Quoted 5 lines
We add relaxed and non-relaxed variants, by using
writel_relaxedand 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 <[email protected]>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?
Signature · 3 lines
Ezequiel Garc?a, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
Quoted 16 lines · Ezequiel Garcia
Quoted 15 lines · Baruch Siach
HI Ezequiel,
Quoted 12 lines · 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_relaxedand writel, respectively. The rationale for this is that some users may not require register write completion but only thread-safe access to a register.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Ezequiel Garcia <[email protected]>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.
Signature · 3 lines
Ezequiel Garc?a, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
Quoted 17 lines · Ezequiel Garcia
Quoted 16 lines · Ezequiel Garcia
Quoted 15 lines · Baruch Siach
Quoted 13 lines · 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_relaxedand 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 <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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.
Signature · 1 line
Catalin
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
Quoted 29 lines · Catalin Marinas
Quoted 17 lines · Ezequiel Garcia
Quoted 16 lines · Ezequiel Garcia
Quoted 15 lines · Baruch Siach
Quoted 13 lines · 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_relaxedand 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 <[email protected]>Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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:
Quoted 6 lines
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
Quoted 32 lines · Will Deacon
Quoted 31 lines · Catalin Marinas
Quoted 17 lines · Ezequiel Garcia
Quoted 16 lines · Ezequiel Garcia
Quoted 15 lines · Baruch Siach
Quoted 13 lines · 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_relaxedand 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 <[email protected]>Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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:
Quoted 8 lines
Quoted 4 lines
(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().
Signature · 1 line
Catalin
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
Quoted 20 lines · Catalin Marinas
Quoted 10 lines · Will Deacon
Quoted 9 lines · Catalin Marinas
Quoted 2 lines · Ezequiel Garcia
... 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:
Quoted 8 lines
Quoted 4 lines
(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.
Quoted 8 lines
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 foratomic_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
Quoted 30 lines · Will Deacon
Quoted 20 lines · Catalin Marinas
Quoted 10 lines · Will Deacon
Quoted 9 lines · Catalin Marinas
Quoted 2 lines · Ezequiel Garcia
... 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:
Quoted 8 lines
Quoted 4 lines
(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.
Quoted 8 lines
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 foratomic_io_modify_relaxed().
Quoted 8 lines
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 ;).
Signature · 1 line
Catalin
PATCH v3 1/3 ARM: Introduce atomic MMIO modify
Quoted 7 lines · Catalin Marinas
Quoted 3 lines
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
Quoted 10 lines · Jason Gunthorpe
Quoted 7 lines · Catalin Marinas
Quoted 3 lines
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 ;).
Quoted 5 lines
Something I've always been confused about..
Do these
_relaxedoperators 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).
Quoted 2 lines
AFAIK, on x86
read_relaxedis expected to cause the PCI behavior.Documentation/DocBook/deviceiobook.tmplseems 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
In reply to Catalin MarinasHi all,
Quoted 36 lines · Catalin Marinas
Quoted 35 lines · Will Deacon
Quoted 20 lines · Catalin Marinas
Quoted 10 lines · Will Deacon
Quoted 9 lines · Catalin Marinas
Quoted 2 lines · Ezequiel Garcia
... 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:
Quoted 8 lines
Quoted 4 lines
(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.
Quoted 8 lines
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 foratomic_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).
Quoted 8 lines
Quoted 3 lines
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,
Signature · 4 lines
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
Quoted 1 line · Gregory CLEMENT
Hi all,
Hi Gregory,
Quoted 6 lines · Catalin Marinas
Quoted 5 lines · Will Deacon
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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).
Quoted 5 lines
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.
Quoted 4 lines
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
Quoted 16 lines · Will Deacon
Quoted 1 line · Gregory CLEMENT
Hi all,
Hi Gregory,
Quoted 11 lines · Catalin Marinas
Quoted 9 lines · Will Deacon
Quoted 3 lines
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 ;).
Quoted 7 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 6 lines
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!
Quoted 3 lines
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.
Quoted 9 lines
Quoted 5 lines
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!
Quoted 3 lines
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.
Quoted 5 lines
Cheers,
Will
Signature · 4 lines
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
In reply to Will DeaconQuoted 7 lines · Will Deacon
Quoted 3 lines
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
In reply to Ezequiel GarciaQuoted 66 lines · 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_relaxedand 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 <[email protected]>---
Based on a similar approach suggested by Russell King:
http://archive.arm.linux.org.uk/lurker/message/20130618.113606.d7d4fe4b.en.htmlChanges from v2:
* As suggested by Will Deacon, dropped theiowmb()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 barrieriowmb()as suggested by Will Deacon, to ensure the writel gets completed before thespin_unlock().arch/arm/include/asm/io.h | 6 ++++++ arch/arm/kernel/io.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)arch/arm/include/asm/io.h +6
-0@@ -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. */ arch/arm/kernel/io.c +5
-0@@ -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)
arch/arm/kernel/io.c line 18
Quoted 10 lines
arch/arm/kernel/io.c +10
-0+{ +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:
Quoted 2 lines
arch/arm/kernel/io.c +2
-0+EXPORT_SYMBOL (atomic_io_modify); +
Signature · 3 lines
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
In reply to Ezequiel GarciaReplace the driver-specific thread-safe shared register API by the recently introduced atomic_io_clear_set().
--- drivers/clocksource/time-orion.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)
drivers/clocksource/time-orion.c +10 -18
@@ -35,20 +35,6 @@#define ORION_ONESHOT_MAX 0xfffffffestatic 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);
Signature · 1 line
1.8.1.5
PATCH v3 2/3 clocksource: orion: Use atomic access for shared registers
Hi Ezequiel,
Quoted 4 lines · Ezequiel Garcia
Replace the driver-specific thread-safe shared register API by the recently introduced
atomic_io_clear_set().Signed-off-by Ezequiel Garcia <[email protected]>
[...]
drivers/clocksource/time-orion.c static void __init orion_timer_init(struct device_node · line 125
Quoted 7 lines
drivers/clocksource/time-orion.c +1
-1@@ -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
Quoted 5 lines
drivers/clocksource/time-orion.c +2
-0+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);
Signature · 3 lines
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
Quoted 10 lines · Baruch Siach
Hi Ezequiel,
Quoted 4 lines · Ezequiel Garcia
Replace the driver-specific thread-safe shared register API by the recently introduced
atomic_io_clear_set().Signed-off-by Ezequiel Garcia <[email protected]>[...]
Quoted 10 lines
drivers/clocksource/time-orion.c static void __init orion_timer_init(struct device_node · line 125
Quoted 7 lines
drivers/clocksource/time-orion.c +1
-1@@ -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!
Signature · 3 lines
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
In reply to Ezequiel GarciaSince the timer control register is shared with the clocksource driver, use the recently introduced atomic_io_clear_set() to access such register.
--- drivers/watchdog/orion_wdt.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/watchdog/orion_wdt.c +2 -6
@@ -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 ;
Signature · 1 line
1.8.1.5