Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 21:41:34 +0900
From:   Stafford Horne <shorne@gmail.com>
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Openrisc <openrisc@lists.librecores.org>,
        Jonas Bonn <jonas@southpole.se>,
        Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>,
        Palmer Dabbelt <palmer@rivosinc.com>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v2 1/2] openrisc: Add pci bus support
Message-ID: <YtAO/nxcsjjc8M/c@antec>
References: <20220714042736.2133067-1-shorne@gmail.com>
 <20220714042736.2133067-2-shorne@gmail.com>
 <CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com>
Precedence: bulk

On Thu, Jul 14, 2022 at 09:56:44AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 14, 2022 at 6:27 AM Stafford Horne <shorne@gmail.com> wrote:
> >
> > This patch adds required definitions to allow for PCI buses on OpenRISC.
> > This is being in the QEMU virt platform.
> >
> > OpenRISC does not have IO ports so this defines PCI IO to be allowed in
> > any range.  Keeping PIO_RESERVED defined as 0 allows OpenRISC to use
> > MMIO for all IO.
> 
> Ok, this looks better.
> 
> > Also, since commit 66bcd06099bb ("parport_pc: Also enable driver for PCI
> > systems") all platforms that support PCI also need to support parallel
> > port.  We add a generic header to support parallel port drivers.
> 
> The parport_pc driver is actually one of the things that doesn't work without
> I/O ports, so at least the description here is misleading. We should really
> have Kconfig logic to enforce this, but that is a separate topic.

OK, let me try to fix up this comment in v3.

> > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> > index e814df4c483c..327241988819 100644
> > --- a/arch/openrisc/Kconfig
> > +++ b/arch/openrisc/Kconfig
> > @@ -21,7 +21,9 @@ config OPENRISC
> >         select GENERIC_IRQ_PROBE
> >         select GENERIC_IRQ_SHOW
> >         select GENERIC_IOMAP
> > +       select GENERIC_PCI_IOMAP
> >         select GENERIC_CPU_DEVICES
> 
> > @@ -46,9 +50,6 @@ config MMU
> >  config GENERIC_HWEIGHT
> >         def_bool y
> >
> > -config NO_IOPORT_MAP
> > -       def_bool y
> > -

I tried the below patch on top of this but I get failures, as the __pci_ioport_map
uses ioport_map.

    lib/pci_iomap.c: In function 'pci_iomap_range':
      CC      drivers/i2c/i2c-core-base.o
    ./include/asm-generic/pci_iomap.h:29:41: error: implicit declaration of function 'ioport_map'; did you mean 'ioremap'? [-Werror=implicit-function-declaration]
       29 | #define __pci_ioport_map(dev, port, nr) ioport_map((port), (nr))
	  |                                         ^~~~~~~~~~
    lib/pci_iomap.c:44:24: note: in expansion of macro '__pci_ioport_map'
       44 |                 return __pci_ioport_map(dev, start, len);
	  |                        ^~~~~~~~~~~~~~~~


diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index 327241988819..c7f282f60f64 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -20,7 +20,6 @@ config OPENRISC
        select GENERIC_IRQ_CHIP
        select GENERIC_IRQ_PROBE
        select GENERIC_IRQ_SHOW
-       select GENERIC_IOMAP
        select GENERIC_PCI_IOMAP
        select GENERIC_CPU_DEVICES
        select HAVE_PCI
@@ -50,6 +49,9 @@ config MMU
 config GENERIC_HWEIGHT
        def_bool y
 
+config NO_IOPORT_MAP
+       def_bool y
+
 # For now, use generic checksum functions
 #These can be reimplemented in assembly later if so inclined
 config GENERIC_CSUM


> GENERIC_IOMAP makes no sense without PIO, and I think you also
> need to keep the NO_IOPORT_MAP. I think you still want
> GENERIC_PCI_IOMAP, which in the absence of the other two
> should turn just return an __iomem pointer for memory resource
> and NULL for i/o resources.

OK.

If we keep NO_IOPORT_MAP, it causes HAS_IOPORT_MAP to be false and it removes
the definition of ioport_map which still seems to be needed at link time.  Maybe
thats an issue though.

> > +static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > +{
> > +       /* no legacy IRQs on or1k */
> > +       return -ENODEV;
> > +}
> 
> The comment seems misleading, as "legacy IRQs" normally refers to
> non-MSI interrupts, which you do support. It's only the legacy IDE
> interrupts that are not supported.

OK, I might have copied this from another architecture.  I will check if those
comments should be updated too.

> I see that the asm-generic/pci.h file is now completely useless,
> as it only has a single function left in it, and this one is wrong
> on most architectures -- it only works when you have PC-style
> interrupt numbers. Out of the five architectures that include
> asm-generic/pci.h  (m68k, s390, sparc, x86, xtensa), I would
> expect only x86 to use this version, and maybe a few sparc
> machines.
> 
> Can I ask you to move out the existing asm-generic/pci.h
> code into those architectures, and add a new file in its place
> that you can use as-is on openrisc? Ideally we should
> be able to also remove most of the contents of asm/pci.h
> on arm64 and riscv. If you have conflicting settings, the normal
> way to handle them in asm-generic headers is like

Yeah, that sounds like a good plan,

> #ifndef PCIBIOS_MIN_IO
> #define PCIBIOS_MIN_IO 0
> #endif
> 
> #ifndef pcibios_assign_all_busses
> #define pcibios_assign_all_busses() 1
> #endif

OK.

-Stafford