[PATCH v2 0/2] OpenRISC support for virt platform with PCI

[PATCH v2 0/2] OpenRISC support for virt platform with PCI

[Stafford Horne]

Hello,

This is a set of kernel patches which I have been using to support the new QEMU
virt platform [1].  The virt platform supports PCI hence I have a patch here to
add PCI support to OpenRISC.

A defconfig is also included which turns on drivers supported devices include
virt network cards and the goldfish RTC.

[1] https://github.com/stffrdhrn/qemu/tree/or1k-virt-3

Changes since v1:

 - Remove PIO support as suggest by Arnd

Stafford Horne (2):
  openrisc: Add pci bus support
  openrisc: Add virt defconfig

 arch/openrisc/Kconfig                |   7 +-
 arch/openrisc/configs/virt_defconfig | 108 +++++++++++++++++++++++++++
 arch/openrisc/include/asm/Kbuild     |   1 +
 arch/openrisc/include/asm/io.h       |   2 +-
 arch/openrisc/include/asm/pci.h      |  36 +++++++++
 5 files changed, 150 insertions(+), 4 deletions(-)
 create mode 100644 arch/openrisc/configs/virt_defconfig
 create mode 100644 arch/openrisc/include/asm/pci.h

-- 
2.36.1

[PATCH v2 1/2] openrisc: Add pci bus support

[Stafford Horne]

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.

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.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
Since v1:
 - Revert definition of IO_SPACE_LIMIT

 arch/openrisc/Kconfig            |  7 ++++---
 arch/openrisc/include/asm/Kbuild |  1 +
 arch/openrisc/include/asm/io.h   |  2 +-
 arch/openrisc/include/asm/pci.h  | 36 ++++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+), 4 deletions(-)
 create mode 100644 arch/openrisc/include/asm/pci.h

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
+	select HAVE_PCI
 	select HAVE_UID16
 	select GENERIC_ATOMIC64
 	select GENERIC_CLOCKEVENTS_BROADCAST
@@ -32,6 +34,8 @@ config OPENRISC
 	select CPU_NO_EFFICIENT_FFS if !OPENRISC_HAVE_INST_FF1
 	select ARCH_USE_QUEUED_RWLOCKS
 	select OMPIC if SMP
+	select PCI_DOMAINS_GENERIC if PCI
+	select PCI_MSI if PCI
 	select ARCH_WANT_FRAME_POINTERS
 	select GENERIC_IRQ_MULTI_HANDLER
 	select MMU_GATHER_NO_RANGE if MMU
@@ -46,9 +50,6 @@ 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
diff --git a/arch/openrisc/include/asm/Kbuild b/arch/openrisc/include/asm/Kbuild
index 3386b9c1c073..c8c99b554ca4 100644
--- a/arch/openrisc/include/asm/Kbuild
+++ b/arch/openrisc/include/asm/Kbuild
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 generic-y += extable.h
 generic-y += kvm_para.h
+generic-y += parport.h
 generic-y += spinlock_types.h
 generic-y += spinlock.h
 generic-y += qrwlock_types.h
diff --git a/arch/openrisc/include/asm/io.h b/arch/openrisc/include/asm/io.h
index c298061c70a7..625ac6ad1205 100644
--- a/arch/openrisc/include/asm/io.h
+++ b/arch/openrisc/include/asm/io.h
@@ -17,7 +17,7 @@
 #include <linux/types.h>
 
 /*
- * PCI: can we really do 0 here if we have no port IO?
+ * PCI: We do not use IO ports in OpenRISC
  */
 #define IO_SPACE_LIMIT		0
 
diff --git a/arch/openrisc/include/asm/pci.h b/arch/openrisc/include/asm/pci.h
new file mode 100644
index 000000000000..e0865d2f3f42
--- /dev/null
+++ b/arch/openrisc/include/asm/pci.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_OPENRISC_PCI_H
+#define __ASM_OPENRISC_PCI_H
+
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/dma-mapping.h>
+
+#include <asm/io.h>
+
+#define PCIBIOS_MIN_IO		0
+#define PCIBIOS_MIN_MEM		0
+
+/* OpenRISC bootloaders do not initialize PCI bus */
+#define pcibios_assign_all_busses() 1
+
+#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
+
+extern int isa_dma_bridge_buggy;
+
+#ifdef CONFIG_PCI
+static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
+{
+	/* no legacy IRQs on or1k */
+	return -ENODEV;
+}
+
+static inline int pci_proc_domain(struct pci_bus *bus)
+{
+	/* always show the domain in /proc */
+	return 1;
+}
+#endif /* CONFIG_PCI */
+
+#endif /* __ASM_OPENRISC_PCI_H */
-- 
2.36.1

[PATCH v2 2/2] openrisc: Add virt defconfig

[Stafford Horne]

I have been developing a new qemu virt platform to help with more
efficient toolchain and kernel testing [1].

This patch adds the defconfig which is needed to support booting
linux on the platform.

[1] https://lore.kernel.org/qemu-devel/YpwNtowUTxRbh2Uq@antec/T/#m6db180b0d682785fb320e4a05345c12a063e0c47

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/configs/virt_defconfig | 108 +++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 arch/openrisc/configs/virt_defconfig

diff --git a/arch/openrisc/configs/virt_defconfig b/arch/openrisc/configs/virt_defconfig
new file mode 100644
index 000000000000..c1b69166c500
--- /dev/null
+++ b/arch/openrisc/configs/virt_defconfig
@@ -0,0 +1,108 @@
+CONFIG_SYSVIPC=y
+CONFIG_POSIX_MQUEUE=y
+CONFIG_NO_HZ_IDLE=y
+CONFIG_LOG_BUF_SHIFT=14
+CONFIG_CGROUPS=y
+CONFIG_BLK_DEV_INITRD=y
+CONFIG_OPENRISC_HAVE_INST_CMOV=y
+CONFIG_OPENRISC_HAVE_INST_ROR=y
+CONFIG_OPENRISC_HAVE_INST_RORI=y
+CONFIG_OPENRISC_HAVE_INST_SEXT=y
+CONFIG_NR_CPUS=8
+CONFIG_SMP=y
+CONFIG_HZ_100=y
+# CONFIG_OPENRISC_NO_SPR_SR_DSX is not set
+# CONFIG_COMPAT_BRK is not set
+CONFIG_NET=y
+CONFIG_PACKET=y
+CONFIG_UNIX=y
+CONFIG_INET=y
+# CONFIG_INET_DIAG is not set
+CONFIG_TCP_CONG_ADVANCED=y
+# CONFIG_TCP_CONG_BIC is not set
+# CONFIG_TCP_CONG_CUBIC is not set
+# CONFIG_TCP_CONG_WESTWOOD is not set
+# CONFIG_TCP_CONG_HTCP is not set
+# CONFIG_IPV6 is not set
+# CONFIG_WIRELESS is not set
+CONFIG_PCI=y
+CONFIG_PCIEPORTBUS=y
+CONFIG_PCI_HOST_GENERIC=y
+CONFIG_DEVTMPFS=y
+CONFIG_DEVTMPFS_MOUNT=y
+# CONFIG_PREVENT_FIRMWARE_BUILD is not set
+CONFIG_BLK_DEV_LOOP=y
+CONFIG_BLK_DEV_NBD=y
+CONFIG_VIRTIO_BLK=y
+CONFIG_NETDEVICES=y
+CONFIG_VIRTIO_NET=y
+CONFIG_ETHOC=y
+CONFIG_MICREL_PHY=y
+# CONFIG_WLAN is not set
+CONFIG_INPUT_MOUSEDEV=y
+# CONFIG_LEGACY_PTYS is not set
+CONFIG_SERIAL_8250=y
+CONFIG_SERIAL_8250_CONSOLE=y
+CONFIG_SERIAL_OF_PLATFORM=y
+CONFIG_VIRTIO_CONSOLE=y
+# CONFIG_HW_RANDOM is not set
+CONFIG_POWER_RESET=y
+CONFIG_POWER_RESET_SYSCON=y
+CONFIG_POWER_RESET_SYSCON_POWEROFF=y
+CONFIG_SYSCON_REBOOT_MODE=y
+# CONFIG_HWMON is not set
+CONFIG_DRM=y
+# CONFIG_DRM_FBDEV_EMULATION is not set
+CONFIG_DRM_VIRTIO_GPU=y
+CONFIG_FB=y
+CONFIG_FIRMWARE_EDID=y
+CONFIG_FRAMEBUFFER_CONSOLE=y
+CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY=y
+CONFIG_LOGO=y
+CONFIG_SOUND=y
+CONFIG_SND=y
+CONFIG_SND_INTEL8X0=y
+CONFIG_SND_INTEL8X0M=y
+CONFIG_SND_SOC=y
+CONFIG_SND_VIRTIO=y
+# CONFIG_HID_A4TECH is not set
+# CONFIG_HID_BELKIN is not set
+# CONFIG_HID_CHERRY is not set
+# CONFIG_HID_CHICONY is not set
+# CONFIG_HID_CYPRESS is not set
+# CONFIG_HID_EZKEY is not set
+# CONFIG_HID_ITE is not set
+# CONFIG_HID_KENSINGTON is not set
+# CONFIG_HID_REDRAGON is not set
+# CONFIG_HID_MICROSOFT is not set
+# CONFIG_HID_MONTEREY is not set
+CONFIG_USB=y
+CONFIG_USB_XHCI_HCD=y
+CONFIG_USB_XHCI_PLATFORM=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_EHCI_HCD_PLATFORM=y
+CONFIG_USB_OHCI_HCD=y
+CONFIG_USB_OHCI_HCD_PLATFORM=y
+CONFIG_USB_SERIAL=y
+CONFIG_USB_GADGET=y
+CONFIG_TYPEC=y
+CONFIG_RTC_CLASS=y
+CONFIG_RTC_DRV_GOLDFISH=y
+CONFIG_VIRT_DRIVERS=y
+CONFIG_VIRTIO_PCI=y
+# CONFIG_VIRTIO_PCI_LEGACY is not set
+CONFIG_VIRTIO_INPUT=y
+CONFIG_VIRTIO_MMIO=y
+CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES=y
+CONFIG_EXT3_FS=y
+CONFIG_EXT3_FS_POSIX_ACL=y
+# CONFIG_DNOTIFY is not set
+CONFIG_MSDOS_FS=y
+CONFIG_VFAT_FS=y
+CONFIG_EXFAT_FS=y
+CONFIG_TMPFS=y
+CONFIG_NFS_FS=y
+CONFIG_UNICODE=y
+CONFIG_PRINTK_TIME=y
+CONFIG_DYNAMIC_DEBUG=y
+CONFIG_FTRACE=y
-- 
2.36.1

Re: [PATCH v2 1/2] openrisc: Add pci bus support

[Arnd Bergmann]

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

Re: [PATCH v2 1/2] openrisc: Add pci bus support

[Stafford Horne]

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

Re: [PATCH v2 1/2] openrisc: Add pci bus support

[Arnd Bergmann]

On Thu, Jul 14, 2022 at 2:41 PM Stafford Horne <shorne@gmail.com> wrote:
> On Thu, Jul 14, 2022 at 09:56:44AM +0200, Arnd Bergmann wrote:
> > > 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);
>           |                        ^~~~~~~~~~~~~~~~
>

Ah, I see. So setting NO_IOPORT_MAP without GENERIC_PCI_IOMAP
probably just works, but then you'd also build all the driver that use
ioport_map() when they cannot work.

Maybe add another

#define __pci_ioport_map(dev, port, nr) NULL

case to include/asm-generic/pci_iomap.h, or add an #ifdef to lib
pci_iomap_range() to not call it in this case.

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

This is the intention of CONFIG_NO_IOPORT_MAP, it's meant to be set
on architectures that have no way of defining ioport_map() in a sensible
way.

        Arnd