| Message ID | 1386686497-20335-2-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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.
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.
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?
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
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.
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
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.
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.
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".
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.
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?
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!
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 ;))
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.
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
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.
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.
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(+)