| Message ID | 20220831214447.273178-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | parisc: Use the generic IO helpers | expand |
On Wed, Aug 31, 2022, at 11:44 PM, Linus Walleij wrote: > @@ -135,35 +135,43 @@ static inline unsigned char __raw_readb(const > volatile void __iomem *addr) > { > return (*(volatile unsigned char __force *) (addr)); > } > +#define __raw_readb __raw_readb > static inline unsigned short __raw_readw(const volatile void __iomem > *addr) > { > return *(volatile unsigned short __force *) addr; > } > +#define __raw_readw __raw_readw These are the same as the asm-generic version, so it might be nice to just use those and remove the duplicates. The readl() etc wrappers around them are more complicated in the generic version, and may require to #define the __io_ar()/__io_bw(() etc to nothing to avoid adding extra barriers. Not sure if we want to go so far, or if parisc is actually correct here: Most RISC architectures do need barriers between a readl/writel and a corresponding DMA, so pa-risc would be an exception here for not needing them. > #include <asm-generic/iomap.h> > +/* These get provided from <asm-generic/iomap.h> */ > +#define ioport_map ioport_map > +#define ioport_unmap ioport_unmap > +#define ioread8 ioread8 > +#define ioread16 ioread16 > +#define ioread32 ioread32 > +#define ioread16be ioread16be > +#define ioread32be ioread32be > +#define iowrite8 iowrite8 > +#define iowrite16 iowrite16 > +#define iowrite32 iowrite32 > +#define iowrite16be iowrite16be > +#define iowrite32be iowrite32be > +#define ioread8_rep ioread8_rep > +#define ioread16_rep ioread16_rep > +#define ioread32_rep ioread32_rep > +#define iowrite8_rep iowrite8_rep > +#define iowrite16_rep iowrite16_rep > +#define iowrite32_rep iowrite32_rep You should not need these overrides here, since the definitions in asm-generic/io.h are only relevant for the !CONFIG_GENERIC_IOMAP case, i.e. architectures that can access port I/O through MMIO rather than special helper functions or instructions. Somewhat unrelated to your series, I suppose it would be great if we could move the "#include <asm-generic/iomap.h>" to include/asm-generic/io.h itself for the CONFIG_GENERIC_IOMAP case. Hopefully each architecture uses one case or the other. Arnd
On Thu, Sep 1, 2022 at 9:35 AM Arnd Bergmann <arnd@arndb.de> wrote: > > #include <asm-generic/iomap.h> > > +/* These get provided from <asm-generic/iomap.h> */ > > +#define ioport_map ioport_map > > +#define ioport_unmap ioport_unmap > > +#define ioread8 ioread8 > > +#define ioread16 ioread16 > > +#define ioread32 ioread32 > > +#define ioread16be ioread16be > > +#define ioread32be ioread32be > > +#define iowrite8 iowrite8 > > +#define iowrite16 iowrite16 > > +#define iowrite32 iowrite32 > > +#define iowrite16be iowrite16be > > +#define iowrite32be iowrite32be > > +#define ioread8_rep ioread8_rep > > +#define ioread16_rep ioread16_rep > > +#define ioread32_rep ioread32_rep > > +#define iowrite8_rep iowrite8_rep > > +#define iowrite16_rep iowrite16_rep > > +#define iowrite32_rep iowrite32_rep > > You should not need these overrides here, since the > definitions in asm-generic/io.h are only relevant > for the !CONFIG_GENERIC_IOMAP case, i.e. architectures > that can access port I/O through MMIO rather than > special helper functions or instructions. parisc does not select GENERIC_IOMAP. Are you saying that it should? That seems like an invasive change to me... Fixed the rest and resending. Yours, Linus Walleij
On Thu, Sep 1, 2022, at 1:56 PM, Linus Walleij wrote: > On Thu, Sep 1, 2022 at 9:35 AM Arnd Bergmann <arnd@arndb.de> wrote: >> You should not need these overrides here, since the >> definitions in asm-generic/io.h are only relevant >> for the !CONFIG_GENERIC_IOMAP case, i.e. architectures >> that can access port I/O through MMIO rather than >> special helper functions or instructions. > > parisc does not select GENERIC_IOMAP. > > Are you saying that it should? > > That seems like an invasive change to me... You are right, I missed that part. So parisc just uses the declarations from asm-generic/iomap.h but has its own definitions instead of the lib/iomap.c ones. According to the comment, the parisc version was originally meant to handle additional special cases besides MMIO or PIO, but this seems to have never happened. The current version looks basically equivalent to the generic version to me, except for a small bug I found (see patch below). Changing parisc to GENERIC_IOMAP is clearly something we can do, but I agree that it would be out of scope for your series. Arnd --- a/arch/parisc/lib/iomap.c +++ b/arch/parisc/lib/iomap.c @@ -212,12 +212,12 @@ static void iomem_write32be(u32 datum, void __iomem *addr) static void iomem_write64(u64 datum, void __iomem *addr) { - writel(datum, addr); + writeq(datum, addr); } static void iomem_write64be(u64 datum, void __iomem *addr) { - __raw_writel(datum, addr); + __raw_writeq(datum, addr); } static void iomem_read8r(const void __iomem *addr, void *dst, unsigned long count)
On Thu, Sep 1, 2022 at 2:20 PM Arnd Bergmann <arnd@arndb.de> wrote: > According to the comment, the parisc version was originally > meant to handle additional special cases besides MMIO or > PIO, but this seems to have never happened. The current > version looks basically equivalent to the generic version > to me, except for a small bug I found (see patch below). I saw this too, but didn't know what to do with it. I'll include your patch as two of us clearly think it looks like a bug. The other pecularity is that iomap and the parisc iommu unconditionally defines and uses 64bit MMIO accessors, so I had to quirk <asm-generic/io.h> with a special "give me 64bit IO even though I'm 32bit" mechanism, we can discuss this after I posted v2. > Changing parisc to GENERIC_IOMAP is clearly something we > can do, but I agree that it would be out of scope for > your series. That's for later :) Yours, Linus Walleij
diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h index 42ffb60a6ea9..2b04473c97e3 100644 --- a/arch/parisc/include/asm/io.h +++ b/arch/parisc/include/asm/io.h @@ -135,35 +135,43 @@ static inline unsigned char __raw_readb(const volatile void __iomem *addr) { return (*(volatile unsigned char __force *) (addr)); } +#define __raw_readb __raw_readb static inline unsigned short __raw_readw(const volatile void __iomem *addr) { return *(volatile unsigned short __force *) addr; } +#define __raw_readw __raw_readw static inline unsigned int __raw_readl(const volatile void __iomem *addr) { return *(volatile unsigned int __force *) addr; } +#define __raw_readl __raw_readl static inline unsigned long long __raw_readq(const volatile void __iomem *addr) { return *(volatile unsigned long long __force *) addr; } +#define __raw_readq __raw_readq static inline void __raw_writeb(unsigned char b, volatile void __iomem *addr) { *(volatile unsigned char __force *) addr = b; } +#define __raw_writeb __raw_writeb static inline void __raw_writew(unsigned short b, volatile void __iomem *addr) { *(volatile unsigned short __force *) addr = b; } +#define __raw_writew __raw_writew static inline void __raw_writel(unsigned int b, volatile void __iomem *addr) { *(volatile unsigned int __force *) addr = b; } +#define __raw_writel __raw_writel static inline void __raw_writeq(unsigned long long b, volatile void __iomem *addr) { *(volatile unsigned long long __force *) addr = b; } +#define __raw_writeq __raw_writeq static inline unsigned char readb(const volatile void __iomem *addr) { @@ -193,6 +201,7 @@ static inline void writew(unsigned short w, volatile void __iomem *addr) static inline void writel(unsigned int l, volatile void __iomem *addr) { __raw_writel((__u32 __force) cpu_to_le32(l), addr); + } static inline void writeq(unsigned long long q, volatile void __iomem *addr) { @@ -220,6 +229,9 @@ static inline void writeq(unsigned long long q, volatile void __iomem *addr) void memset_io(volatile void __iomem *addr, unsigned char val, int count); void memcpy_fromio(void *dst, const volatile void __iomem *src, int count); void memcpy_toio(volatile void __iomem *dst, const void *src, int count); +#define memset_io memset_io +#define memcpy_fromio memcpy_fromio +#define memcpy_toio memcpy_toio /* Port-space IO */ @@ -241,10 +253,15 @@ extern void eisa_out32(unsigned int data, unsigned short port); extern unsigned char inb(int addr); extern unsigned short inw(int addr); extern unsigned int inl(int addr); - extern void outb(unsigned char b, int addr); extern void outw(unsigned short b, int addr); extern void outl(unsigned int b, int addr); +#define inb inb +#define inw inw +#define inl inl +#define outb outb +#define outw outw +#define outl outl #elif defined(CONFIG_EISA) #define inb eisa_in8 #define inw eisa_in16 @@ -270,7 +287,9 @@ static inline int inl(unsigned long addr) BUG(); return -1; } - +#define inb inb +#define inw inw +#define inl inl #define outb(x, y) ({(void)(x); (void)(y); BUG(); 0;}) #define outw(x, y) ({(void)(x); (void)(y); BUG(); 0;}) #define outl(x, y) ({(void)(x); (void)(y); BUG(); 0;}) @@ -285,7 +304,12 @@ extern void insl (unsigned long port, void *dst, unsigned long count); extern void outsb (unsigned long port, const void *src, unsigned long count); extern void outsw (unsigned long port, const void *src, unsigned long count); extern void outsl (unsigned long port, const void *src, unsigned long count); - +#define insb insb +#define insw insw +#define insl insl +#define outsb outsb +#define outsw outsw +#define outsl outsl /* IO Port space is : BBiiii where BB is HBA number. */ #define IO_SPACE_LIMIT 0x00ffffff @@ -307,6 +331,25 @@ extern void iowrite64(u64 val, void __iomem *addr); extern void iowrite64be(u64 val, void __iomem *addr); #include <asm-generic/iomap.h> +/* These get provided from <asm-generic/iomap.h> */ +#define ioport_map ioport_map +#define ioport_unmap ioport_unmap +#define ioread8 ioread8 +#define ioread16 ioread16 +#define ioread32 ioread32 +#define ioread16be ioread16be +#define ioread32be ioread32be +#define iowrite8 iowrite8 +#define iowrite16 iowrite16 +#define iowrite32 iowrite32 +#define iowrite16be iowrite16be +#define iowrite32be iowrite32be +#define ioread8_rep ioread8_rep +#define ioread16_rep ioread16_rep +#define ioread32_rep ioread32_rep +#define iowrite8_rep iowrite8_rep +#define iowrite16_rep iowrite16_rep +#define iowrite32_rep iowrite32_rep /* * Convert a physical pointer to a virtual kernel pointer for /dev/mem @@ -316,4 +359,6 @@ extern void iowrite64be(u64 val, void __iomem *addr); extern int devmem_is_allowed(unsigned long pfn); +#include <asm-generic/io.h> + #endif
This enables the parisc to use <asm-generic/io.h> to fill in the missing (undefined) [read|write]sq I/O accessor functions. This is needed if parisc[64] ever wants to uses CONFIG_REGMAP_MMIO which has been patches to use accelerated _noinc accessors such as readsq/writesq that parisc64, while being a 64bit platform, as of now not yet provide. This comes with the requirement that everything the architecture already provides needs to be defined, rather than just being, say, static inline functions. Bite the bullet and just provide the definitions and make it work. Compile-tested on parisc32 and parisc64. Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/linux-arm-kernel/62fcc351.hAyADB%2FY8JTxz+kh%25lkp@intel.com/ Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com> Cc: Helge Deller <deller@gmx.de> Cc: linux-parisc@vger.kernel.org Cc: linux-arch@vger.kernel.org Cc: Mark Brown <broonie@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- arch/parisc/include/asm/io.h | 51 +++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-)