RE: [PATCH v10 1/9] LIB: Introduce a generic PIO mapping method

[Gabriele Paoloni]

Hi Corey

Many Thanks for your comments

[...]

> >   #define IO_SPACE_LIMIT 0x
> >   #endif
> >
> > +#include 
> 
> This whole thing would be a lot simpler if you had:
> 
> #ifdef CONFIG_INDIRECT_PIO
> #define inb logic_inb
> #define outb logic outb
> .
> .
> #endif /* CONFIG_INDIRECT_PIO */
> 
> and let the "ifndef XXX" below handle not enabling the standard code.
> You could even put that in logic_pio.h to avoid polluting io.h.

Agreed. I'll move it into "logic_pio.h"

> 
> You might have to add "#ifndef inb", etc. above, but I still think it
> would
> be better.

Yes agreed also on adding the "#ifndef ***" around each function re-definition
in logic_pio.h

This will be fixed in v11

> 
> I'm not sure if this wouldn't be better done in arm64/include/asm/io.h,
> though.
> A specific machine may want to only do this in ranges, for instance.

No I don’t think so...this io functions re-0definition has nothing to do
with ARM architecture and I think it is better to keep everything in
logic_pio.h including the file from "include/asm-generic/io.h"

Cheers
Gab

> 
> -corey

[...]

> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -59,6 +59,32 @@ config ARCH_USE_CMPXCHG_LOCKREF
> >   config ARCH_HAS_FAST_MULTIPLIER
> > bool
> >
> > +config LOGIC_PIO
> > +   bool "Generic logical I/O management"
> > +   def_bool y if PCI && !X86 && !IA64 && !POWERPC
> > +   help
> > + For some architectures, there are no IO space. To support the
> > + accesses to legacy I/O devices on those architectures, kernel
> > + implemented the memory mapped I/O mechanism based on bridge bus
> > + supports. But for some buses which do not support MMIO, the
> > + peripherals there should be accessed with device-specific way.
> > + To abstract those different I/O accesses into unified I/O
> accessors,
> > + this option provide a generic I/O space management way after
> mapping
> > + the device I/O to system logical/fake I/O and help to hide all
> the
> > + hardware detail.
> > +
> > +config INDIRECT_PIO
> > +   bool "Access I/O in non-MMIO mode" if LOGIC_PIO
> > +   help
> > + On some platforms where no separate I/O space exist, there are
> I/O
> > + hosts which can not be accessed in MMIO mode. Based on
> LOGIC_PIO
> > + mechanism, the host-local I/O resource can be mapped into
> system
> > + logic PIO space shared with MMIO hosts, such as PCI/PCIE, then
> system
> > + can access the I/O devices with the mapped logic PIO through
> I/O
> > + accessors.
> > + This way has a little I/O performance cost. Please make sure
> your
> > + devices really need this configure item enabled.
> > +
> 
> If this is always available on the hisilicon chips, I think you would
> want to just always
> enable this on those chips.

In patch 6/9 we have 

+config HISILICON_LPC
+   bool "Support for ISA I/O space on Hisilicon Hip0X"
+   depends on (ARM64 && (ARCH_HISI || COMPILE_TEST))
+   select LOGIC_PIO
+   select INDIRECT_PIO

So the LPC host controller driver is selecting INDIRECT_PIO...

> 
> -corey
> 

Cheers
Gab

RE: [PATCH v10 1/9] LIB: Introduce a generic PIO mapping method

[Gabriele Paoloni]

Hi Corey

Many Thanks for your comments

[...]

> >   #define IO_SPACE_LIMIT 0x
> >   #endif
> >
> > +#include 
> 
> This whole thing would be a lot simpler if you had:
> 
> #ifdef CONFIG_INDIRECT_PIO
> #define inb logic_inb
> #define outb logic outb
> .
> .
> #endif /* CONFIG_INDIRECT_PIO */
> 
> and let the "ifndef XXX" below handle not enabling the standard code.
> You could even put that in logic_pio.h to avoid polluting io.h.

Agreed. I'll move it into "logic_pio.h"

> 
> You might have to add "#ifndef inb", etc. above, but I still think it
> would
> be better.

Yes agreed also on adding the "#ifndef ***" around each function re-definition
in logic_pio.h

This will be fixed in v11

> 
> I'm not sure if this wouldn't be better done in arm64/include/asm/io.h,
> though.
> A specific machine may want to only do this in ranges, for instance.

No I don’t think so...this io functions re-0definition has nothing to do
with ARM architecture and I think it is better to keep everything in
logic_pio.h including the file from "include/asm-generic/io.h"

Cheers
Gab

> 
> -corey

[...]

> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -59,6 +59,32 @@ config ARCH_USE_CMPXCHG_LOCKREF
> >   config ARCH_HAS_FAST_MULTIPLIER
> > bool
> >
> > +config LOGIC_PIO
> > +   bool "Generic logical I/O management"
> > +   def_bool y if PCI && !X86 && !IA64 && !POWERPC
> > +   help
> > + For some architectures, there are no IO space. To support the
> > + accesses to legacy I/O devices on those architectures, kernel
> > + implemented the memory mapped I/O mechanism based on bridge bus
> > + supports. But for some buses which do not support MMIO, the
> > + peripherals there should be accessed with device-specific way.
> > + To abstract those different I/O accesses into unified I/O
> accessors,
> > + this option provide a generic I/O space management way after
> mapping
> > + the device I/O to system logical/fake I/O and help to hide all
> the
> > + hardware detail.
> > +
> > +config INDIRECT_PIO
> > +   bool "Access I/O in non-MMIO mode" if LOGIC_PIO
> > +   help
> > + On some platforms where no separate I/O space exist, there are
> I/O
> > + hosts which can not be accessed in MMIO mode. Based on
> LOGIC_PIO
> > + mechanism, the host-local I/O resource can be mapped into
> system
> > + logic PIO space shared with MMIO hosts, such as PCI/PCIE, then
> system
> > + can access the I/O devices with the mapped logic PIO through
> I/O
> > + accessors.
> > + This way has a little I/O performance cost. Please make sure
> your
> > + devices really need this configure item enabled.
> > +
> 
> If this is always available on the hisilicon chips, I think you would
> want to just always
> enable this on those chips.

In patch 6/9 we have 

+config HISILICON_LPC
+   bool "Support for ISA I/O space on Hisilicon Hip0X"
+   depends on (ARM64 && (ARCH_HISI || COMPILE_TEST))
+   select LOGIC_PIO
+   select INDIRECT_PIO

So the LPC host controller driver is selecting INDIRECT_PIO...

> 
> -corey
> 

Cheers
Gab

Re: [PATCH v10 1/9] LIB: Introduce a generic PIO mapping method

[Corey Minyard]

On 10/27/2017 11:11 AM, Gabriele Paoloni wrote:

From: "zhichang.yuan" 

In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
pci_pio_to_address()"), a new I/O space management was supported. With
that driver, the I/O ranges configured for PCI/PCIE hosts on some
architectures can be mapped to logical PIO, converted easily between
CPU address and the corresponding logicial PIO. Based on this, PCI
I/O devices can be accessed in a memory read/write way through the
unified in/out accessors.

But on some archs/platforms, there are bus hosts which access I/O
peripherals with host-local I/O port addresses rather than memory
addresses after memory-mapped.
To support those devices, a more generic I/O mapping method is introduced
here. Through this patch, both the CPU addresses and the host-local port
can be mapped into the logical PIO space with different logical/fake PIOs.
After this, all the I/O accesses to either PCI MMIO devices or host-local
I/O peripherals can be unified into the existing I/O accessors defined in
asm-generic/io.h and be redirected to the right device-specific hooks
based on the input logical PIO.

Signed-off-by: zhichang.yuan 
Signed-off-by: Gabriele Paoloni 
---
  include/asm-generic/io.h  |  26 +
  include/linux/logic_pio.h | 118 +++
  lib/Kconfig   |  26 +
  lib/Makefile  |   2 +
  lib/logic_pio.c   | 286 ++
  5 files changed, 458 insertions(+)
  create mode 100644 include/linux/logic_pio.h
  create mode 100644 lib/logic_pio.c

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ef015e..334e5db 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -351,6 +351,8 @@ static inline void writesq(volatile void __iomem *addr, 
const void *buffer,
  #define IO_SPACE_LIMIT 0x
  #endif
  
+#include 


This whole thing would be a lot simpler if you had:

#ifdef CONFIG_INDIRECT_PIO
#define inb logic_inb
#define outb logic outb
.
.
#endif /* CONFIG_INDIRECT_PIO */

and let the "ifndef XXX" below handle not enabling the standard code.
You could even put that in logic_pio.h to avoid polluting io.h.

You might have to add "#ifndef inb", etc. above, but I still think it would
be better.

I'm not sure if this wouldn't be better done in arm64/include/asm/io.h, 
though.

A specific machine may want to only do this in ranges, for instance.

-corey


+
  /*
   * {in,out}{b,w,l}() access little endian I/O. {in,out}{b,w,l}_p() can be
   * implemented on hardware that needs an additional delay for I/O accesses to
@@ -358,51 +360,75 @@ static inline void writesq(volatile void __iomem *addr, 
const void *buffer,
   */
  
  #ifndef inb

+#ifdef CONFIG_INDIRECT_PIO
+#define inb logic_inb
+#else
  #define inb inb
  static inline u8 inb(unsigned long addr)
  {
return readb(PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef inw

+#ifdef CONFIG_INDIRECT_PIO
+#define inw logic_inw
+#else
  #define inw inw
  static inline u16 inw(unsigned long addr)
  {
return readw(PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef inl

+#ifdef CONFIG_INDIRECT_PIO
+#define inl logic_inl
+#else
  #define inl inl
  static inline u32 inl(unsigned long addr)
  {
return readl(PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef outb

+#ifdef CONFIG_INDIRECT_PIO
+#define outb logic_outb
+#else
  #define outb outb
  static inline void outb(u8 value, unsigned long addr)
  {
writeb(value, PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef outw

+#ifdef CONFIG_INDIRECT_PIO
+#define outw logic_outw
+#else
  #define outw outw
  static inline void outw(u16 value, unsigned long addr)
  {
writew(value, PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef outl

+#ifdef CONFIG_INDIRECT_PIO
+#define outl logic_outl
+#else
  #define outl outl
  static inline void outl(u32 value, unsigned long addr)
  {
writel(value, PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef inb_p

diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
new file mode 100644
index 000..f0a6f15
--- /dev/null
+++ b/include/linux/logic_pio.h
@@ -0,0 +1,118 @@
+/*
+ * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Author: Gabriele Paoloni 
+ * Author: Zhichang Yuan 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  

Re: [PATCH v10 1/9] LIB: Introduce a generic PIO mapping method

[Corey Minyard]

On 10/27/2017 11:11 AM, Gabriele Paoloni wrote:

From: "zhichang.yuan" 

In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
pci_pio_to_address()"), a new I/O space management was supported. With
that driver, the I/O ranges configured for PCI/PCIE hosts on some
architectures can be mapped to logical PIO, converted easily between
CPU address and the corresponding logicial PIO. Based on this, PCI
I/O devices can be accessed in a memory read/write way through the
unified in/out accessors.

But on some archs/platforms, there are bus hosts which access I/O
peripherals with host-local I/O port addresses rather than memory
addresses after memory-mapped.
To support those devices, a more generic I/O mapping method is introduced
here. Through this patch, both the CPU addresses and the host-local port
can be mapped into the logical PIO space with different logical/fake PIOs.
After this, all the I/O accesses to either PCI MMIO devices or host-local
I/O peripherals can be unified into the existing I/O accessors defined in
asm-generic/io.h and be redirected to the right device-specific hooks
based on the input logical PIO.

Signed-off-by: zhichang.yuan 
Signed-off-by: Gabriele Paoloni 
---
  include/asm-generic/io.h  |  26 +
  include/linux/logic_pio.h | 118 +++
  lib/Kconfig   |  26 +
  lib/Makefile  |   2 +
  lib/logic_pio.c   | 286 ++
  5 files changed, 458 insertions(+)
  create mode 100644 include/linux/logic_pio.h
  create mode 100644 lib/logic_pio.c

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ef015e..334e5db 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -351,6 +351,8 @@ static inline void writesq(volatile void __iomem *addr, 
const void *buffer,
  #define IO_SPACE_LIMIT 0x
  #endif
  
+#include 


This whole thing would be a lot simpler if you had:

#ifdef CONFIG_INDIRECT_PIO
#define inb logic_inb
#define outb logic outb
.
.
#endif /* CONFIG_INDIRECT_PIO */

and let the "ifndef XXX" below handle not enabling the standard code.
You could even put that in logic_pio.h to avoid polluting io.h.

You might have to add "#ifndef inb", etc. above, but I still think it would
be better.

I'm not sure if this wouldn't be better done in arm64/include/asm/io.h, 
though.

A specific machine may want to only do this in ranges, for instance.

-corey


+
  /*
   * {in,out}{b,w,l}() access little endian I/O. {in,out}{b,w,l}_p() can be
   * implemented on hardware that needs an additional delay for I/O accesses to
@@ -358,51 +360,75 @@ static inline void writesq(volatile void __iomem *addr, 
const void *buffer,
   */
  
  #ifndef inb

+#ifdef CONFIG_INDIRECT_PIO
+#define inb logic_inb
+#else
  #define inb inb
  static inline u8 inb(unsigned long addr)
  {
return readb(PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef inw

+#ifdef CONFIG_INDIRECT_PIO
+#define inw logic_inw
+#else
  #define inw inw
  static inline u16 inw(unsigned long addr)
  {
return readw(PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef inl

+#ifdef CONFIG_INDIRECT_PIO
+#define inl logic_inl
+#else
  #define inl inl
  static inline u32 inl(unsigned long addr)
  {
return readl(PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef outb

+#ifdef CONFIG_INDIRECT_PIO
+#define outb logic_outb
+#else
  #define outb outb
  static inline void outb(u8 value, unsigned long addr)
  {
writeb(value, PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef outw

+#ifdef CONFIG_INDIRECT_PIO
+#define outw logic_outw
+#else
  #define outw outw
  static inline void outw(u16 value, unsigned long addr)
  {
writew(value, PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef outl

+#ifdef CONFIG_INDIRECT_PIO
+#define outl logic_outl
+#else
  #define outl outl
  static inline void outl(u32 value, unsigned long addr)
  {
writel(value, PCI_IOBASE + addr);
  }
+#endif /* CONFIG_INDIRECT_PIO */
  #endif
  
  #ifndef inb_p

diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
new file mode 100644
index 000..f0a6f15
--- /dev/null
+++ b/include/linux/logic_pio.h
@@ -0,0 +1,118 @@
+/*
+ * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Author: Gabriele Paoloni 
+ * Author: Zhichang Yuan 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along