| Message ID | 20170726231917.6073-4-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On Thu, Jul 27, 2017 at 2:19 AM, Logan Gunthorpe <logang@deltatee.com> wrote: > In order to provide non-atomic functions for io{read|write}64 that will > use readq and writeq when appropriate. We define a number of variants > of these functions in the generic iomap that will do non-atomic > operations on pio but atomic operations on mmio. > > These functions are only defined if readq and writeq are defined. If > they are not, then the wrappers that always use non-atomic operations > from include/linux/io-64-nonatomic*.h will be used. Don't you see here a slight problem? In some cases we want to substitute atomic in favour of non-atomic when both are defined. So, please don't do this "smartly". > +u64 ioread64_lo_hi(void __iomem *addr) > +{ > + IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr)); > + return 0xffffffffffffffffLL; > +} U missed u.
On 30/07/17 10:03 AM, Andy Shevchenko wrote: > On Thu, Jul 27, 2017 at 2:19 AM, Logan Gunthorpe <logang@deltatee.com> wrote: >> In order to provide non-atomic functions for io{read|write}64 that will >> use readq and writeq when appropriate. We define a number of variants >> of these functions in the generic iomap that will do non-atomic >> operations on pio but atomic operations on mmio. >> >> These functions are only defined if readq and writeq are defined. If >> they are not, then the wrappers that always use non-atomic operations >> from include/linux/io-64-nonatomic*.h will be used. > > Don't you see here a slight problem? > > In some cases we want to substitute atomic in favour of non-atomic > when both are defined. > So, please don't do this "smartly". I'm not sure what you mean here. The driver should use ioread64 and include an io-64-nonatomic header. Then there are three cases: 1) The arch has no atomic 64 bit io operations defined. In this case it uses the non-atomic inline function in the io-64-nonatomic header. 2) The arch uses CONFIG_GENERIC_IOMAP and has readq defined, but not ioread64 defined (likely because pio can't do atomic 64 bit operations but mmio can). In this case we need to use the ioread64_xx functions defined in iomap.c which do atomic mmio and non-atomic pio. 3) The arch has ioread64 defined so the atomic operation is used. >> +u64 ioread64_lo_hi(void __iomem *addr) >> +{ >> + IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr)); >> + return 0xffffffffffffffffLL; >> +} > > U missed u. I'll fix this in the next revision. Thanks, Logan
On Mon, Jul 31, 2017 at 6:55 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > On 30/07/17 10:03 AM, Andy Shevchenko wrote: >> On Thu, Jul 27, 2017 at 2:19 AM, Logan Gunthorpe <logang@deltatee.com> wrote: >>> In order to provide non-atomic functions for io{read|write}64 that will >>> use readq and writeq when appropriate. We define a number of variants >>> of these functions in the generic iomap that will do non-atomic >>> operations on pio but atomic operations on mmio. >>> >>> These functions are only defined if readq and writeq are defined. If >>> they are not, then the wrappers that always use non-atomic operations >>> from include/linux/io-64-nonatomic*.h will be used. >> >> Don't you see here a slight problem? >> >> In some cases we want to substitute atomic in favour of non-atomic >> when both are defined. >> So, please don't do this "smartly". > > I'm not sure what you mean here. The driver should use ioread64 and > include an io-64-nonatomic header. Then there are three cases: > > 1) The arch has no atomic 64 bit io operations defined. In this case it > uses the non-atomic inline function in the io-64-nonatomic header. Okay > 2) The arch uses CONFIG_GENERIC_IOMAP and has readq defined, but not > ioread64 defined (likely because pio can't do atomic 64 bit operations > but mmio can). In this case we need to use the ioread64_xx functions > defined in iomap.c which do atomic mmio and non-atomic pio. Not okay. Some drivers (hardware) would like to have non-atomic MMIO accesses when readq() defined > 3) The arch has ioread64 defined so the atomic operation is used. Not okay. Same reason as above. In case of readq() / writeq() it's defined by the order of inclusion: 1) include <...non-atomic...> include <linux/io.h> Always non-atomic will be used. 2) include <linux/io.h> include <...non-atomic...> Auto switch like you described. I don't like above solution, since it's fragile, but few drivers depend on that. If you wish to do it always like 2) perhaps we need to split accessors to ones for fixed bus width and ones for atomic/non-atomic cases. OTOH, it would be done by introducing memcpyXX_fromio() memcpyXX_toio() memsetXX_io() Where XX = 64, 32, 16, 8. Note, that ioreadXX_rep() is not the same as above. P.S. I have done a table of comparison between IO accessors in Linux kernel and it looks hell out of being consistent.
On 31/07/17 10:10 AM, Andy Shevchenko wrote: > Some drivers (hardware) would like to have non-atomic MMIO accesses > when readq() defined Huh? But that's the whole point of the io64-nonatomic header. If a driver wants a specific non-atomic access they should just code two 32 bit accesses. > In case of readq() / writeq() it's defined by the order of inclusion: > > 1) > include <...non-atomic...> > include <linux/io.h> > > Always non-atomic will be used. I'm afraid you're wrong about this. The io-64-nonatomic-xx header includes linux/io.h. Thus the order of the includes doesn't matter and it will always auto switch. In any case, making an interface do different things depending on the order of include files is *completely* insane. > P.S. I have done a table of comparison between IO accessors in Linux > kernel and it looks hell out of being consistent. There are a few corner oddities but it's really not that bad. Most things are done for a reason if you dig into them. Logan
On Mon, Jul 31, 2017 at 7:31 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > On 31/07/17 10:10 AM, Andy Shevchenko wrote: >> Some drivers (hardware) would like to have non-atomic MMIO accesses >> when readq() defined > > Huh? But that's the whole point of the io64-nonatomic header. If a > driver wants a specific non-atomic access they should just code two 32 > bit accesses. You mean to call them directly as lo_hi_XXX() or hi_lo_XXX() ? Yes it would work. >> In case of readq() / writeq() it's defined by the order of inclusion: >> >> 1) >> include <...non-atomic...> >> include <linux/io.h> >> >> Always non-atomic will be used. > > I'm afraid you're wrong about this. The io-64-nonatomic-xx header > includes linux/io.h. Thus the order of the includes doesn't matter and > it will always auto switch. In any case, making an interface do > different things depending on the order of include files is *completely* > insane. Yes, you are right. I was thinking about something unrelated.
On 31/07/17 11:58 AM, Andy Shevchenko wrote: > On Mon, Jul 31, 2017 at 7:31 PM, Logan Gunthorpe <logang@deltatee.com> wrote: >> On 31/07/17 10:10 AM, Andy Shevchenko wrote: >>> Some drivers (hardware) would like to have non-atomic MMIO accesses >>> when readq() defined >> >> Huh? But that's the whole point of the io64-nonatomic header. If a >> driver wants a specific non-atomic access they should just code two 32 >> bit accesses. > You mean to call them directly as lo_hi_XXX() or hi_lo_XXX() ? > Yes it would work. I suppose you could do that too but I really meant just using two io32 calls. That's the most explicit way to indicate you want a non-atomic access. Logan
On Mon, Jul 31, 2017 at 9:00 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > On 31/07/17 11:58 AM, Andy Shevchenko wrote: >> On Mon, Jul 31, 2017 at 7:31 PM, Logan Gunthorpe <logang@deltatee.com> wrote: >>> On 31/07/17 10:10 AM, Andy Shevchenko wrote: >>>> Some drivers (hardware) would like to have non-atomic MMIO accesses >>>> when readq() defined >>> >>> Huh? But that's the whole point of the io64-nonatomic header. If a >>> driver wants a specific non-atomic access they should just code two 32 >>> bit accesses. > >> You mean to call them directly as lo_hi_XXX() or hi_lo_XXX() ? >> Yes it would work. > > I suppose you could do that too but I really meant just using two io32 > calls. That's the most explicit way to indicate you want a non-atomic > access. Per commit 3a044178cccf they are exactly created for this kind of cases.
On 31/07/17 12:03 PM, Andy Shevchenko wrote: > > Per commit 3a044178cccf they are exactly created for this kind of cases. > Sure, ok, and my patchset provides the same set of functions to satisfy such a use. Logan
On Mon, Jul 31, 2017 at 9:04 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > > > On 31/07/17 12:03 PM, Andy Shevchenko wrote: >> >> Per commit 3a044178cccf they are exactly created for this kind of cases. >> > > Sure, ok, and my patchset provides the same set of functions to satisfy > such a use. Okay, please, Cc me for next version, I think I need fresh view on it. Thanks!
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index af074923d598..4cc420cfaa78 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -788,8 +788,10 @@ extern void __iounmap_at(void *ea, unsigned long size); #define mmio_read16be(addr) readw_be(addr) #define mmio_read32be(addr) readl_be(addr) +#define mmio_read64be(addr) readq_be(addr) #define mmio_write16be(val, addr) writew_be(val, addr) #define mmio_write32be(val, addr) writel_be(val, addr) +#define mmio_write64be(val, addr) writeq_be(val, addr) #define mmio_insb(addr, dst, count) readsb(addr, dst, count) #define mmio_insw(addr, dst, count) readsw(addr, dst, count) #define mmio_insl(addr, dst, count) readsl(addr, dst, count) diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h index 650fede33c25..30edebf627fe 100644 --- a/include/asm-generic/iomap.h +++ b/include/asm-generic/iomap.h @@ -30,9 +30,16 @@ extern unsigned int ioread16(void __iomem *); extern unsigned int ioread16be(void __iomem *); extern unsigned int ioread32(void __iomem *); extern unsigned int ioread32be(void __iomem *); -#ifdef CONFIG_64BIT -extern u64 ioread64(void __iomem *); -extern u64 ioread64be(void __iomem *); + +#ifdef readq +#define ioread64_lo_hi ioread64_lo_hi +#define ioread64_hi_lo ioread64_hi_lo +#define ioread64be_lo_hi ioread64be_lo_hi +#define ioread64be_hi_lo ioread64be_hi_lo +extern u64 ioread64_lo_hi(void __iomem *addr); +extern u64 ioread64_hi_lo(void __iomem *addr); +extern u64 ioread64be_lo_hi(void __iomem *addr); +extern u64 ioread64be_hi_lo(void __iomem *addr); #endif extern void iowrite8(u8, void __iomem *); @@ -40,9 +47,16 @@ extern void iowrite16(u16, void __iomem *); extern void iowrite16be(u16, void __iomem *); extern void iowrite32(u32, void __iomem *); extern void iowrite32be(u32, void __iomem *); -#ifdef CONFIG_64BIT -extern void iowrite64(u64, void __iomem *); -extern void iowrite64be(u64, void __iomem *); + +#ifdef writeq +#define iowrite64_lo_hi iowrite64_lo_hi +#define iowrite64_hi_lo iowrite64_hi_lo +#define iowrite64be_lo_hi iowrite64be_lo_hi +#define iowrite64be_hi_lo iowrite64be_hi_lo +extern void iowrite64_lo_hi(u64 val, void __iomem *addr); +extern void iowrite64_hi_lo(u64 val, void __iomem *addr); +extern void iowrite64be_lo_hi(u64 val, void __iomem *addr); +extern void iowrite64be_hi_lo(u64 val, void __iomem *addr); #endif /* diff --git a/lib/iomap.c b/lib/iomap.c index fc3dcb4b238e..b993400d60bd 100644 --- a/lib/iomap.c +++ b/lib/iomap.c @@ -66,6 +66,7 @@ static void bad_io_access(unsigned long port, const char *access) #ifndef mmio_read16be #define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr)) #define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr)) +#define mmio_read64be(addr) be64_to_cpu(__raw_readq(addr)) #endif unsigned int ioread8(void __iomem *addr) @@ -99,6 +100,80 @@ EXPORT_SYMBOL(ioread16be); EXPORT_SYMBOL(ioread32); EXPORT_SYMBOL(ioread32be); +#ifdef readq +static u64 pio_read64_lo_hi(unsigned long port) +{ + u64 lo, hi; + + lo = inl(port); + hi = inl(port + sizeof(u32)); + + return lo | (hi << 32); +} + +static u64 pio_read64_hi_lo(unsigned long port) +{ + u64 lo, hi; + + hi = inl(port + sizeof(u32)); + lo = inl(port); + + return lo | (hi << 32); +} + +static u64 pio_read64be_lo_hi(unsigned long port) +{ + u64 lo, hi; + + lo = pio_read32be(port + sizeof(u32)); + hi = pio_read32be(port); + + return lo | (hi << 32); +} + +static u64 pio_read64be_hi_lo(unsigned long port) +{ + u64 lo, hi; + + hi = pio_read32be(port); + lo = pio_read32be(port + sizeof(u32)); + + return lo | (hi << 32); +} + +u64 ioread64_lo_hi(void __iomem *addr) +{ + IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr)); + return 0xffffffffffffffffLL; +} + +u64 ioread64_hi_lo(void __iomem *addr) +{ + IO_COND(addr, return pio_read64_hi_lo(port), return readq(addr)); + return 0xffffffffffffffffLL; +} + +u64 ioread64be_lo_hi(void __iomem *addr) +{ + IO_COND(addr, return pio_read64be_lo_hi(port), + return mmio_read64be(addr)); + return 0xffffffffffffffffLL; +} + +u64 ioread64be_hi_lo(void __iomem *addr) +{ + IO_COND(addr, return pio_read64be_hi_lo(port), + return mmio_read64be(addr)); + return 0xffffffffffffffffLL; +} + +EXPORT_SYMBOL(ioread64_lo_hi); +EXPORT_SYMBOL(ioread64_hi_lo); +EXPORT_SYMBOL(ioread64be_lo_hi); +EXPORT_SYMBOL(ioread64be_hi_lo); + +#endif /* readq */ + #ifndef pio_write16be #define pio_write16be(val,port) outw(swab16(val),port) #define pio_write32be(val,port) outl(swab32(val),port) @@ -107,6 +182,7 @@ EXPORT_SYMBOL(ioread32be); #ifndef mmio_write16be #define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port) #define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port) +#define mmio_write64be(val,port) __raw_writeq(be64_to_cpu(val),port) #endif void iowrite8(u8 val, void __iomem *addr) @@ -135,6 +211,62 @@ EXPORT_SYMBOL(iowrite16be); EXPORT_SYMBOL(iowrite32); EXPORT_SYMBOL(iowrite32be); +#ifdef writeq +static void pio_write64_lo_hi(u64 val, unsigned long port) +{ + outl(val, port); + outl(val >> 32, port + sizeof(u32)); +} + +static void pio_write64_hi_lo(u64 val, unsigned long port) +{ + outl(val >> 32, port + sizeof(u32)); + outl(val, port); +} + +static void pio_write64be_lo_hi(u64 val, unsigned long port) +{ + pio_write32be(val, port + sizeof(u32)); + pio_write32be(val >> 32, port); +} + +static void pio_write64be_hi_lo(u64 val, unsigned long port) +{ + pio_write32be(val >> 32, port); + pio_write32be(val, port + sizeof(u32)); +} + +void iowrite64_lo_hi(u64 val, void __iomem *addr) +{ + IO_COND(addr, pio_write64_lo_hi(val, port), + writeq(val, addr)); +} + +void iowrite64_hi_lo(u64 val, void __iomem *addr) +{ + IO_COND(addr, pio_write64_hi_lo(val, port), + writeq(val, addr)); +} + +void iowrite64be_lo_hi(u64 val, void __iomem *addr) +{ + IO_COND(addr, pio_write64be_lo_hi(val, port), + mmio_write64be(val, addr)); +} + +void iowrite64be_hi_lo(u64 val, void __iomem *addr) +{ + IO_COND(addr, pio_write64be_hi_lo(val, port), + mmio_write64be(val, addr)); +} + +EXPORT_SYMBOL(iowrite64_lo_hi); +EXPORT_SYMBOL(iowrite64_hi_lo); +EXPORT_SYMBOL(iowrite64be_lo_hi); +EXPORT_SYMBOL(iowrite64be_hi_lo); + +#endif /* readq */ + /* * These are the "repeat MMIO read/write" functions. * Note the "__raw" accesses, since we don't want to
In order to provide non-atomic functions for io{read|write}64 that will use readq and writeq when appropriate. We define a number of variants of these functions in the generic iomap that will do non-atomic operations on pio but atomic operations on mmio. These functions are only defined if readq and writeq are defined. If they are not, then the wrappers that always use non-atomic operations from include/linux/io-64-nonatomic*.h will be used. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Suresh Warrier <warrier@linux.vnet.ibm.com> Cc: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/include/asm/io.h | 2 + include/asm-generic/iomap.h | 26 +++++++-- lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 6 deletions(-)