| Message ID | 20250130134825.2646380-1-julian@outer-limits.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | parisc: Remove memcpy_fromio | expand |
On Thu, Jan 30, 2025, at 14:48, Julian Vetter wrote: > Fully migrate parisc to the IO functions from lib/iomem_copy.c. In a > recent patch the functions memset_io and memcpy_toio were removed, but > the memcpy_fromio was kept, because for very short sequences it does > half word accesses, whereas the functions in lib/iomem_copy.c do byte > accesses until the memory is naturally aligned and then do machine word > accesses. But I don't think the single half-word access merits keeping > the arch specific implementation, so, remove it as well. > > Signed-off-by: Julian Vetter <julian@outer-limits.org> Acked-by: Arnd Bergmann <arnd@arndb.de> This one looks fairly obvious. It might be nice to also clean up the {in,out}s{b,w,l} helper functions in the same file, but I don't understand why those are special in the first place. Those functions have been unchanged since before the git history and there are some comments that I don't find helpful. One thing they do is to deal with unaligned memory buffers, which the generic ones don't, but that could be easily added using get_unaligned/put_unaligned, expecting the compiler to optimize the memory side of the transfer. Arnd
On 1/31/25 08:28, Arnd Bergmann wrote: > On Thu, Jan 30, 2025, at 14:48, Julian Vetter wrote: >> Fully migrate parisc to the IO functions from lib/iomem_copy.c. In a >> recent patch the functions memset_io and memcpy_toio were removed, but >> the memcpy_fromio was kept, because for very short sequences it does >> half word accesses, whereas the functions in lib/iomem_copy.c do byte >> accesses until the memory is naturally aligned and then do machine word >> accesses. But I don't think the single half-word access merits keeping >> the arch specific implementation, so, remove it as well. >> >> Signed-off-by: Julian Vetter <julian@outer-limits.org> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > This one looks fairly obvious. It might be nice to also > clean up the {in,out}s{b,w,l} helper functions in the same > file, but I don't understand why those are special > in the first place. > Not sure, either. But some comments contain: "just using the inlined version of the inw() breaks things". This is clear, and what you point out below already, these parisc specific functions deal with unaligned buffers, so using, e.g., the generic "inw()" will surely break things. > Those functions have been unchanged since before the git > history and there are some comments that I don't find helpful. > One thing they do is to deal with unaligned memory buffers, > which the generic ones don't, but that could be easily added > using get_unaligned/put_unaligned, expecting the compiler > to optimize the memory side of the transfer. I'm not sure what you suggest. Do you mean adding get_unaligned/put_unaligned in the generic include/asm-generic/io.h functions, or just adding get_unaligned/put_unaligned to the body of the parisc specific functions? Instead of all the "switch... case 0x2..." code? Julian > > Arnd
On Mon, Feb 3, 2025, at 11:21, Julian Vetter wrote: > On 1/31/25 08:28, Arnd Bergmann wrote: >> Those functions have been unchanged since before the git >> history and there are some comments that I don't find helpful. >> One thing they do is to deal with unaligned memory buffers, >> which the generic ones don't, but that could be easily added >> using get_unaligned/put_unaligned, expecting the compiler >> to optimize the memory side of the transfer. > > I'm not sure what you suggest. Do you mean adding > get_unaligned/put_unaligned in the generic include/asm-generic/io.h > functions, or just adding get_unaligned/put_unaligned to the body of the > parisc specific functions? Instead of all the "switch... case 0x2..." code? I meant adding put_unaligned/get_unaligned like this: --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -422,7 +422,8 @@ static inline void readsw(const volatile void __iomem *addr, void *buffer, do { u16 x = __raw_readw(addr); - *buf++ = x; + put_unaligned(x, buf); + buf++; } while (--count); } } This has no effect on architectures that can handle unaligned load/store on memory, and should behave the same way as the parisc code otherwise. On powerpc and possibly others, there needs to be a barrier (eieio() on powerpc) inside of the loop, according to Segher's earlier comments. Arnd
On 1/31/25 08:28, Arnd Bergmann wrote: > On Thu, Jan 30, 2025, at 14:48, Julian Vetter wrote: >> Fully migrate parisc to the IO functions from lib/iomem_copy.c. In a >> recent patch the functions memset_io and memcpy_toio were removed, but >> the memcpy_fromio was kept, because for very short sequences it does >> half word accesses, whereas the functions in lib/iomem_copy.c do byte >> accesses until the memory is naturally aligned and then do machine word >> accesses. But I don't think the single half-word access merits keeping >> the arch specific implementation, so, remove it as well. >> >> Signed-off-by: Julian Vetter <julian@outer-limits.org> > > Acked-by: Arnd Bergmann <arnd@arndb.de> Thanks! I've added the patch to the parisc git tree. Helge
diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h index 3143cf29ce27..61173a2b38e4 100644 --- a/arch/parisc/include/asm/io.h +++ b/arch/parisc/include/asm/io.h @@ -135,9 +135,6 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr) #define pci_iounmap pci_iounmap -void memcpy_fromio(void *dst, const volatile void __iomem *src, int count); -#define memcpy_fromio memcpy_fromio - /* Port-space IO */ #define inb_p inb diff --git a/arch/parisc/kernel/parisc_ksyms.c b/arch/parisc/kernel/parisc_ksyms.c index 1c366b0d3134..509146a52725 100644 --- a/arch/parisc/kernel/parisc_ksyms.c +++ b/arch/parisc/kernel/parisc_ksyms.c @@ -43,7 +43,6 @@ EXPORT_SYMBOL($global$); #endif #include <asm/io.h> -EXPORT_SYMBOL(memcpy_fromio); extern void $$divI(void); extern void $$divU(void); diff --git a/arch/parisc/lib/io.c b/arch/parisc/lib/io.c index 6e81200dc87a..3c7e617f5a93 100644 --- a/arch/parisc/lib/io.c +++ b/arch/parisc/lib/io.c @@ -12,67 +12,6 @@ #include <linux/module.h> #include <asm/io.h> -/* -** Copies a block of memory from a device in an efficient manner. -** Assumes the device can cope with 32-bit transfers. If it can't, -** don't use this function. -** -** CR16 counts on C3000 reading 256 bytes from Symbios 896 RAM: -** 27341/64 = 427 cyc per int -** 61311/128 = 478 cyc per short -** 122637/256 = 479 cyc per byte -** Ergo bus latencies dominant (not transfer size). -** Minimize total number of transfers at cost of CPU cycles. -** TODO: only look at src alignment and adjust the stores to dest. -*/ -void memcpy_fromio(void *dst, const volatile void __iomem *src, int count) -{ - /* first compare alignment of src/dst */ - if ( (((unsigned long)dst ^ (unsigned long)src) & 1) || (count < 2) ) - goto bytecopy; - - if ( (((unsigned long)dst ^ (unsigned long)src) & 2) || (count < 4) ) - goto shortcopy; - - /* Then check for misaligned start address */ - if ((unsigned long)src & 1) { - *(u8 *)dst = readb(src); - src++; - dst++; - count--; - if (count < 2) goto bytecopy; - } - - if ((unsigned long)src & 2) { - *(u16 *)dst = __raw_readw(src); - src += 2; - dst += 2; - count -= 2; - } - - while (count > 3) { - *(u32 *)dst = __raw_readl(src); - dst += 4; - src += 4; - count -= 4; - } - - shortcopy: - while (count > 1) { - *(u16 *)dst = __raw_readw(src); - src += 2; - dst += 2; - count -= 2; - } - - bytecopy: - while (count--) { - *(char *)dst = readb(src); - src++; - dst++; - } -} - /* * Read COUNT 8-bit bytes from port PORT into memory starting at * SRC.
Fully migrate parisc to the IO functions from lib/iomem_copy.c. In a recent patch the functions memset_io and memcpy_toio were removed, but the memcpy_fromio was kept, because for very short sequences it does half word accesses, whereas the functions in lib/iomem_copy.c do byte accesses until the memory is naturally aligned and then do machine word accesses. But I don't think the single half-word access merits keeping the arch specific implementation, so, remove it as well. Signed-off-by: Julian Vetter <julian@outer-limits.org> --- arch/parisc/include/asm/io.h | 3 -- arch/parisc/kernel/parisc_ksyms.c | 1 - arch/parisc/lib/io.c | 61 ------------------------------- 3 files changed, 65 deletions(-)