RE: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method

[Gabriele Paoloni]

Hi Bjorn

> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org]
> Sent: 26 May 2017 21:58
> To: Gabriele Paoloni
> Cc: catalin.mari...@arm.com; will.dea...@arm.com; robh...@kernel.org;
> frowand.l...@gmail.com; bhelg...@google.com; raf...@kernel.org;
> a...@arndb.de; linux-arm-ker...@lists.infradead.org;
> lorenzo.pieral...@arm.com; mark.rutl...@arm.com; miny...@acm.org;
> b...@kernel.crashing.org; John Garry; linux-kernel@vger.kernel.org;
> xuwei (O); Linuxarm; linux-a...@vger.kernel.org; zhichang.yuan; linux-
> p...@vger.kernel.org; o...@lixom.net; brian.star...@arm.com
> Subject: Re: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method
> 
> On Thu, May 25, 2017 at 12:37:22PM +0100, Gabriele Paoloni wrote:
> > From: "zhichang.yuan" <yuanzhich...@hisilicon.com>
> >
> > 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 <yuanzhich...@hisilicon.com>
> > Signed-off-by: Gabriele Paoloni <gabriele.paol...@huawei.com>
> > ---
> >  include/asm-generic/io.h  |  50 +
> >  include/linux/logic_pio.h | 110 ++
> >  lib/Kconfig   |  26 +
> >  lib/Makefile  |   2 +
> >  lib/logic_pio.c   | 280
> ++
> >  5 files changed, 468 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..f7fbec3 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > ...
> 
> >  #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
> 
> Cosmetic: could these ifdefs all be collected in one place, e.g.,
> 
>   #ifdef CONFIG_INDIRECT_PIO
>   #define inb logic_inb
>   #define inw logic_inw
>   #define inl logic_inl
>   ...
>   #endif
> 
> to avoid cluttering every one of the default definitions?  Could the
> collection be in logic_pio.h itself, next to the extern declarations?

Yes I think it should be doable. I will rework this in the next patchset

> 
> > +#else
> >  #define inw inw
> >  static inline u16 inw(unsigned long addr)
> >  {
> > return readw(PCI_IOBASE + addr);
> >  }
> > +#endif /* CONFIG_INDIRECT_PIO */
> >  #endif
> 
> >  #ifndef insb_p
> > diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
> > new file mode 100644
> > index 000..8e4dc65
> > --- /dev/null
> > +++ b/include/linux/logic_pio.h
> > ...
> 
> > +extern u8 logic_inb(unsigned long addr);
> 
> I think you only build the definitions for these if
> CONFIG_INDIRECT_PIO, so the declarations could be under that #idef,
> too.

Yes agreed

> 
> In PCI code, I omit the "extern" from function declarations.  This
> isn't PCI code, and I don't know if there's a real consensus on this,
> but there is some precedent: 5bd085b5fbd8 ("x86: remove "extern" from
> function prototypes in ")
> 

To be honest I have no clue...

If you look at include/asm-generic/io.h we 

RE: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method

[Gabriele Paoloni]

Hi Bjorn

> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org]
> Sent: 26 May 2017 21:58
> To: Gabriele Paoloni
> Cc: catalin.mari...@arm.com; will.dea...@arm.com; robh...@kernel.org;
> frowand.l...@gmail.com; bhelg...@google.com; raf...@kernel.org;
> a...@arndb.de; linux-arm-ker...@lists.infradead.org;
> lorenzo.pieral...@arm.com; mark.rutl...@arm.com; miny...@acm.org;
> b...@kernel.crashing.org; John Garry; linux-kernel@vger.kernel.org;
> xuwei (O); Linuxarm; linux-a...@vger.kernel.org; zhichang.yuan; linux-
> p...@vger.kernel.org; o...@lixom.net; brian.star...@arm.com
> Subject: Re: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method
> 
> On Thu, May 25, 2017 at 12:37:22PM +0100, 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  |  50 +
> >  include/linux/logic_pio.h | 110 ++
> >  lib/Kconfig   |  26 +
> >  lib/Makefile  |   2 +
> >  lib/logic_pio.c   | 280
> ++
> >  5 files changed, 468 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..f7fbec3 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > ...
> 
> >  #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
> 
> Cosmetic: could these ifdefs all be collected in one place, e.g.,
> 
>   #ifdef CONFIG_INDIRECT_PIO
>   #define inb logic_inb
>   #define inw logic_inw
>   #define inl logic_inl
>   ...
>   #endif
> 
> to avoid cluttering every one of the default definitions?  Could the
> collection be in logic_pio.h itself, next to the extern declarations?

Yes I think it should be doable. I will rework this in the next patchset

> 
> > +#else
> >  #define inw inw
> >  static inline u16 inw(unsigned long addr)
> >  {
> > return readw(PCI_IOBASE + addr);
> >  }
> > +#endif /* CONFIG_INDIRECT_PIO */
> >  #endif
> 
> >  #ifndef insb_p
> > diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
> > new file mode 100644
> > index 000..8e4dc65
> > --- /dev/null
> > +++ b/include/linux/logic_pio.h
> > ...
> 
> > +extern u8 logic_inb(unsigned long addr);
> 
> I think you only build the definitions for these if
> CONFIG_INDIRECT_PIO, so the declarations could be under that #idef,
> too.

Yes agreed

> 
> In PCI code, I omit the "extern" from function declarations.  This
> isn't PCI code, and I don't know if there's a real consensus on this,
> but there is some precedent: 5bd085b5fbd8 ("x86: remove "extern" from
> function prototypes in ")
> 

To be honest I have no clue...

If you look at include/asm-generic/io.h we have extern declarations...

BTW I can remove the extern and then let's see if somebody complains...

Re: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method

[Bjorn Helgaas]

On Thu, May 25, 2017 at 12:37:22PM +0100, 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  |  50 +
>  include/linux/logic_pio.h | 110 ++
>  lib/Kconfig   |  26 +
>  lib/Makefile  |   2 +
>  lib/logic_pio.c   | 280 
> ++
>  5 files changed, 468 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..f7fbec3 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> ...  

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

Cosmetic: could these ifdefs all be collected in one place, e.g.,

  #ifdef CONFIG_INDIRECT_PIO
  #define inb logic_inb
  #define inw logic_inw
  #define inl logic_inl
  ...
  #endif

to avoid cluttering every one of the default definitions?  Could the
collection be in logic_pio.h itself, next to the extern declarations?

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

>  #ifndef insb_p
> diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
> new file mode 100644
> index 000..8e4dc65
> --- /dev/null
> +++ b/include/linux/logic_pio.h
> ...

> +extern u8 logic_inb(unsigned long addr);

I think you only build the definitions for these if
CONFIG_INDIRECT_PIO, so the declarations could be under that #idef,
too.

In PCI code, I omit the "extern" from function declarations.  This
isn't PCI code, and I don't know if there's a real consensus on this,
but there is some precedent: 5bd085b5fbd8 ("x86: remove "extern" from
function prototypes in ")

> +#ifdef CONFIG_LOGIC_PIO
> +extern struct logic_pio_hwaddr
> +*find_io_range_by_fwnode(struct fwnode_handle *fwnode);

If you have to split the line (this one would fit without the
"extern"), the "*" goes with the type, e.g.,

  struct logic_pio_hwaddr *
  find_io_range_by_fwnode(struct fwnode_handle *fwnode);

More occurrences below.

> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> new file mode 100644
> index 000..4a960cd
> --- /dev/null
> +++ b/lib/logic_pio.c
> ...

> +#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
> +#define BUILD_LOGIC_PIO(bw, type)\
> +type logic_in##bw(unsigned long addr)\
> +{\
> + type ret = -1;\
> +\
> + if (addr < MMIO_UPPER_LIMIT) {\
> + ret = read##bw(PCI_IOBASE + addr);\
> + } else {\
> + struct logic_pio_hwaddr *entry = find_io_range(addr);\
> +\
> + if (entry && entry->ops)\
> + ret = entry->ops->pfin(entry->devpara,\
> + addr, sizeof(type));\
> + else\
> + WARN_ON_ONCE(1);\
> + }   \
> + return ret;\
> +}\

I think these would be slightly easier to read if the line continuation
backslashes were aligned at the right, as with
ACPI_DECLARE_PROBE_ENTRY(), __atomic_op_acquire(), DECLARE_EWMA(),
etc.

Bjorn

Re: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method

[Bjorn Helgaas]

On Thu, May 25, 2017 at 12:37:22PM +0100, 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  |  50 +
>  include/linux/logic_pio.h | 110 ++
>  lib/Kconfig   |  26 +
>  lib/Makefile  |   2 +
>  lib/logic_pio.c   | 280 
> ++
>  5 files changed, 468 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..f7fbec3 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> ...  

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

Cosmetic: could these ifdefs all be collected in one place, e.g.,

  #ifdef CONFIG_INDIRECT_PIO
  #define inb logic_inb
  #define inw logic_inw
  #define inl logic_inl
  ...
  #endif

to avoid cluttering every one of the default definitions?  Could the
collection be in logic_pio.h itself, next to the extern declarations?

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

>  #ifndef insb_p
> diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
> new file mode 100644
> index 000..8e4dc65
> --- /dev/null
> +++ b/include/linux/logic_pio.h
> ...

> +extern u8 logic_inb(unsigned long addr);

I think you only build the definitions for these if
CONFIG_INDIRECT_PIO, so the declarations could be under that #idef,
too.

In PCI code, I omit the "extern" from function declarations.  This
isn't PCI code, and I don't know if there's a real consensus on this,
but there is some precedent: 5bd085b5fbd8 ("x86: remove "extern" from
function prototypes in ")

> +#ifdef CONFIG_LOGIC_PIO
> +extern struct logic_pio_hwaddr
> +*find_io_range_by_fwnode(struct fwnode_handle *fwnode);

If you have to split the line (this one would fit without the
"extern"), the "*" goes with the type, e.g.,

  struct logic_pio_hwaddr *
  find_io_range_by_fwnode(struct fwnode_handle *fwnode);

More occurrences below.

> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> new file mode 100644
> index 000..4a960cd
> --- /dev/null
> +++ b/lib/logic_pio.c
> ...

> +#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
> +#define BUILD_LOGIC_PIO(bw, type)\
> +type logic_in##bw(unsigned long addr)\
> +{\
> + type ret = -1;\
> +\
> + if (addr < MMIO_UPPER_LIMIT) {\
> + ret = read##bw(PCI_IOBASE + addr);\
> + } else {\
> + struct logic_pio_hwaddr *entry = find_io_range(addr);\
> +\
> + if (entry && entry->ops)\
> + ret = entry->ops->pfin(entry->devpara,\
> + addr, sizeof(type));\
> + else\
> + WARN_ON_ONCE(1);\
> + }   \
> + return ret;\
> +}\

I think these would be slightly easier to read if the line continuation
backslashes were aligned at the right, as with
ACPI_DECLARE_PROBE_ENTRY(), __atomic_op_acquire(), DECLARE_EWMA(),
etc.

Bjorn