From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1948875AbdE0BPj (ORCPT ); Fri, 26 May 2017 21:15:39 -0400 Received: from mail.kernel.org ([198.145.29.99]:50780 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S944949AbdEZU5r (ORCPT ); Fri, 26 May 2017 16:57:47 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 13824239EB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Fri, 26 May 2017 15:57:44 -0500 From: Bjorn Helgaas To: Gabriele Paoloni Cc: catalin.marinas@arm.com, will.deacon@arm.com, robh+dt@kernel.org, frowand.list@gmail.com, bhelgaas@google.com, rafael@kernel.org, arnd@arndb.de, linux-arm-kernel@lists.infradead.org, lorenzo.pieralisi@arm.com, mark.rutland@arm.com, minyard@acm.org, benh@kernel.crashing.org, john.garry@huawei.com, linux-kernel@vger.kernel.org, xuwei5@hisilicon.com, linuxarm@huawei.com, linux-acpi@vger.kernel.org, "zhichang.yuan" , linux-pci@vger.kernel.org, olof@lixom.net, brian.starkey@arm.com Subject: Re: [PATCH v9 1/7] LIB: Introduce a generic PIO mapping method Message-ID: <20170526205744.GA21531@bhelgaas-glaptop.roam.corp.google.com> References: <1495712248-5232-1-git-send-email-gabriele.paoloni@huawei.com> <1495712248-5232-2-git-send-email-gabriele.paoloni@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1495712248-5232-2-git-send-email-gabriele.paoloni@huawei.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 0000000..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 0000000..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