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;
MIME-Version: 1.0
References: <20220714042736.2133067-1-shorne@gmail.com> <20220714042736.2133067-2-shorne@gmail.com>
In-Reply-To: <20220714042736.2133067-2-shorne@gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 14 Jul 2022 09:56:44 +0200
Message-ID: <CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] openrisc: Add pci bus support
To:     Stafford Horne <shorne@gmail.com>
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>,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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

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.

> +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.

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

#ifndef PCIBIOS_MIN_IO
#define PCIBIOS_MIN_IO 0
#endif

#ifndef pcibios_assign_all_busses
#define pcibios_assign_all_busses() 1
#endif

       Arnd