[PATCH] parisc: Use the generic IO helpers

[PATCH] parisc: Use the generic IO helpers

[Linus Walleij]

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(-)

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
-- 
2.37.2

Re: [PATCH] parisc: Use the generic IO helpers

[Arnd Bergmann]

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

Re: [PATCH] parisc: Use the generic IO helpers

[Linus Walleij]

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

Re: [PATCH] parisc: Use the generic IO helpers

[Arnd Bergmann]

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)

Re: [PATCH] parisc: Use the generic IO helpers

[Linus Walleij]

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