2022-02-13 18:18:23

by Maciej W. Rozycki

[permalink] [raw]
Subject: [PATCH] parport_pc: Also enable driver for PCI systems

Nowadays PC-style parallel ports come in the form of PCI and PCIe option
cards and there are some combined parallel/serial option cards as well
that we handle in the parport subsystem. There is nothing in particular
that would prevent them from being used in any system equipped with PCI
or PCIe connectivity, except that we do not permit the PARPORT_PC config
option to be selected for platforms for which ARCH_MIGHT_HAVE_PC_PARPORT
has not been set for.

The only PCI platforms that actually can't make use of PC-style parallel
port hardware are those newer PCIe systems that have no support for I/O
cycles in the host bridge, required by such parallel ports. An example
of such a host bridge is the POWER9 PHB4 device, but it is an exception
rather than the norm. Also it is not clear whether the serial port side
of devices enabled by the PARPORT_SERIAL option uses port I/O or MMIO.
Therefore for PCI platforms PARPORT_PC should generally be available,
except for Super I/O solutions, which are either ISA or platform
devices.

Make the PARPORT_PC option selectable also for PCI systems then, however
limit the availability of PARPORT_PC_SUPERIO to platforms that have
ARCH_MIGHT_HAVE_PC_PARPORT enabled. Update platforms accordingly for
the required <asm/parport.h> header.

Signed-off-by: Maciej W. Rozycki <[email protected]>
---
Hi,

I have verified this lightly by booting a kernel with PARPORT_PC and
PARPORT_SERIAL enabled on a RISC-V HiFive Unmatched system. While I do
have a PCIe parallel port option available that I could use with my RISC-V
machine (based on the OxSemi OXPCIe952 chip) it is currently plugged in
the wrong system, and both machines are in my remote lab I have currently
no visit scheduled to in the near future. For the record the device
reports as:

PCI parallel port detected: 1415:c118, I/O at 0x1000(0x1008), IRQ 18
parport1: PC-style at 0x1000 (0x1008), irq 18, using FIFO [PCSPP,TRISTATE,COMPAT,EPP,ECP]

in the other system. I'll see if I can verify it with the Unmatched at
the next opportunity, though it seems like an overkill to me given that a
PC-style parallel port is a generic PCIe device. The OXPCIe952 implements
a multifunction device, so it doesn't rely on PARPORT_SERIAL.

NB platforms to be updated for <asm/parport.h> generation were chosen by
the presence of the HAVE_PCI or FORCE_PCI option from ones that do not
already have or generate that header. Let me know if I got anything wrong
here.

Maciej
---
arch/arm64/include/asm/Kbuild | 1 +
arch/csky/include/asm/Kbuild | 1 +
arch/riscv/include/asm/Kbuild | 1 +
arch/s390/include/asm/Kbuild | 1 +
arch/um/include/asm/Kbuild | 1 +
arch/xtensa/include/asm/Kbuild | 1 +
drivers/parport/Kconfig | 4 ++--
7 files changed, 8 insertions(+), 2 deletions(-)

linux-parport-pc-pci.diff
Index: linux-macro/arch/arm64/include/asm/Kbuild
===================================================================
--- linux-macro.orig/arch/arm64/include/asm/Kbuild
+++ linux-macro/arch/arm64/include/asm/Kbuild
@@ -3,6 +3,7 @@ generic-y += early_ioremap.h
generic-y += mcs_spinlock.h
generic-y += qrwlock.h
generic-y += qspinlock.h
+generic-y += parport.h
generic-y += user.h

generated-y += cpucaps.h
Index: linux-macro/arch/csky/include/asm/Kbuild
===================================================================
--- linux-macro.orig/arch/csky/include/asm/Kbuild
+++ linux-macro/arch/csky/include/asm/Kbuild
@@ -4,5 +4,6 @@ generic-y += extable.h
generic-y += gpio.h
generic-y += kvm_para.h
generic-y += qrwlock.h
+generic-y += parport.h
generic-y += user.h
generic-y += vmlinux.lds.h
Index: linux-macro/arch/riscv/include/asm/Kbuild
===================================================================
--- linux-macro.orig/arch/riscv/include/asm/Kbuild
+++ linux-macro/arch/riscv/include/asm/Kbuild
@@ -2,5 +2,6 @@
generic-y += early_ioremap.h
generic-y += flat.h
generic-y += kvm_para.h
+generic-y += parport.h
generic-y += user.h
generic-y += vmlinux.lds.h
Index: linux-macro/arch/s390/include/asm/Kbuild
===================================================================
--- linux-macro.orig/arch/s390/include/asm/Kbuild
+++ linux-macro/arch/s390/include/asm/Kbuild
@@ -8,3 +8,4 @@ generic-y += asm-offsets.h
generic-y += export.h
generic-y += kvm_types.h
generic-y += mcs_spinlock.h
+generic-y += parport.h
Index: linux-macro/arch/um/include/asm/Kbuild
===================================================================
--- linux-macro.orig/arch/um/include/asm/Kbuild
+++ linux-macro/arch/um/include/asm/Kbuild
@@ -17,6 +17,7 @@ generic-y += mcs_spinlock.h
generic-y += mmiowb.h
generic-y += module.lds.h
generic-y += param.h
+generic-y += parport.h
generic-y += percpu.h
generic-y += preempt.h
generic-y += softirq_stack.h
Index: linux-macro/arch/xtensa/include/asm/Kbuild
===================================================================
--- linux-macro.orig/arch/xtensa/include/asm/Kbuild
+++ linux-macro/arch/xtensa/include/asm/Kbuild
@@ -4,6 +4,7 @@ generic-y += extable.h
generic-y += kvm_para.h
generic-y += mcs_spinlock.h
generic-y += param.h
+generic-y += parport.h
generic-y += qrwlock.h
generic-y += qspinlock.h
generic-y += user.h
Index: linux-macro/drivers/parport/Kconfig
===================================================================
--- linux-macro.orig/drivers/parport/Kconfig
+++ linux-macro/drivers/parport/Kconfig
@@ -42,7 +42,7 @@ if PARPORT

config PARPORT_PC
tristate "PC-style hardware"
- depends on ARCH_MIGHT_HAVE_PC_PARPORT
+ depends on ARCH_MIGHT_HAVE_PC_PARPORT || PCI
help
You should say Y here if you have a PC-style parallel port. All
IBM PC compatible computers and some Alphas have PC-style
@@ -77,7 +77,7 @@ config PARPORT_PC_FIFO

config PARPORT_PC_SUPERIO
bool "SuperIO chipset support"
- depends on PARPORT_PC && !PARISC
+ depends on ARCH_MIGHT_HAVE_PC_PARPORT && PARPORT_PC && !PARISC
help
Saying Y here enables some probes for Super-IO chipsets in order to
find out things like base addresses, IRQ lines and DMA channels. It


2022-02-14 09:38:37

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

Hi Maciej,

CC Niklas

On Sun, Feb 13, 2022 at 1:45 PM Maciej W. Rozycki <[email protected]> wrote:
> Nowadays PC-style parallel ports come in the form of PCI and PCIe option
> cards and there are some combined parallel/serial option cards as well
> that we handle in the parport subsystem. There is nothing in particular
> that would prevent them from being used in any system equipped with PCI
> or PCIe connectivity, except that we do not permit the PARPORT_PC config
> option to be selected for platforms for which ARCH_MIGHT_HAVE_PC_PARPORT
> has not been set for.
>
> The only PCI platforms that actually can't make use of PC-style parallel
> port hardware are those newer PCIe systems that have no support for I/O
> cycles in the host bridge, required by such parallel ports. An example
> of such a host bridge is the POWER9 PHB4 device, but it is an exception
> rather than the norm. Also it is not clear whether the serial port side

Note that this hardware dependency is being addressed in
"[RFC 00/32] Kconfig: Introduce HAS_IOPORT and LEGACY_PCI options"
https://lore.kernel.org/all/[email protected]/

> --- linux-macro.orig/drivers/parport/Kconfig
> +++ linux-macro/drivers/parport/Kconfig
> @@ -42,7 +42,7 @@ if PARPORT
>
> config PARPORT_PC
> tristate "PC-style hardware"
> - depends on ARCH_MIGHT_HAVE_PC_PARPORT
> + depends on ARCH_MIGHT_HAVE_PC_PARPORT || PCI
> help
> You should say Y here if you have a PC-style parallel port. All
> IBM PC compatible computers and some Alphas have PC-style
> @@ -77,7 +77,7 @@ config PARPORT_PC_FIFO
>
> config PARPORT_PC_SUPERIO
> bool "SuperIO chipset support"
> - depends on PARPORT_PC && !PARISC
> + depends on ARCH_MIGHT_HAVE_PC_PARPORT && PARPORT_PC && !PARISC
> help
> Saying Y here enables some probes for Super-IO chipsets in order to
> find out things like base addresses, IRQ lines and DMA channels. It
>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-02-14 10:02:17

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 14 Feb 2022, Geert Uytterhoeven wrote:

> > The only PCI platforms that actually can't make use of PC-style parallel
> > port hardware are those newer PCIe systems that have no support for I/O
> > cycles in the host bridge, required by such parallel ports. An example
> > of such a host bridge is the POWER9 PHB4 device, but it is an exception
> > rather than the norm. Also it is not clear whether the serial port side
>
> Note that this hardware dependency is being addressed in
> "[RFC 00/32] Kconfig: Introduce HAS_IOPORT and LEGACY_PCI options"
> https://lore.kernel.org/all/[email protected]/

Thanks for the pointer, I have missed the series in the LKML flood!

The idea sounds good, although it's not clear to me if a config option is
enough to get it properly covered as I don't know offhand if a single say
ppc64le kernel can't run on systems that do and do not have support for
port I/O over PCIe. I got hit with that the hard way with a distribution
kernel where a driver tried to do port I/O and poked at a random location
in the address space causing weird errors to be reported by the system
firmware.

Also I have skimmed over the series and there appears to be confusion
between legacy PCI and conventional PCI, which are obviously not the same.
For example this piece:

diff --git a/drivers/net/fddi/Kconfig b/drivers/net/fddi/Kconfig
index 846bf41c2717..1753c08d6423 100644
--- a/drivers/net/fddi/Kconfig
+++ b/drivers/net/fddi/Kconfig
@@ -5,7 +5,7 @@

config FDDI
tristate "FDDI driver support"
- depends on PCI || EISA || TC
+ depends on LEGACY_PCI || EISA || TC
help
Fiber Distributed Data Interface is a high speed local area network
design; essentially a replacement for high speed Ethernet. FDDI can
@@ -29,7 +29,7 @@ config DEFZA

config DEFXX
tristate "Digital DEFTA/DEFEA/DEFPA adapter support"
- depends on FDDI && (PCI || EISA || TC)
+ depends on FDDI && (LEGACY_PCI || EISA || TC)
help
This is support for the DIGITAL series of TURBOchannel (DEFTA),
EISA (DEFEA) and PCI (DEFPA) controllers which can connect you

is clearly wrong. While the DEFPA card is a conventional PCI option it
does support MMIO and does run with my legacy-free POWER9 system just
fine (with a suitable PCIe-to-PCI bridge installed at 0031:01:00.0):

# lspci
0000:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0001:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0001:01:00.0 Serial controller: Oxford Semiconductor Ltd OXPCIe952 Dual Native 950 UART
0002:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0002:01:00.0 Serial Attached SCSI controller: Adaptec Series 8 12G SAS/PCIe 3 (rev 01)
0003:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0003:01:00.0 USB controller: Texas Instruments TUSB73x0 SuperSpeed USB 3.0 xHCI Host Controller (rev 02)
0004:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0004:01:00.0 Ethernet controller: Broadcom Inc. and subsidiaries NetXtreme BCM5719 Gigabit Ethernet PCIe (rev 01)
0004:01:00.1 Ethernet controller: Broadcom Inc. and subsidiaries NetXtreme BCM5719 Gigabit Ethernet PCIe (rev 01)
0005:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0005:01:00.0 PCI bridge: ASPEED Technology, Inc. AST1150 PCI-to-PCI Bridge (rev 04)
0005:02:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 41)
0030:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0030:01:00.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
0030:02:00.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
0030:02:01.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
0030:02:02.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
0030:02:03.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
0030:03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
0030:04:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
0030:05:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
0030:06:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
0031:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0031:01:00.0 PCI bridge: Texas Instruments XIO2000(A)/XIO2200A PCI Express-to-PCI Bridge (rev 03)
0031:02:04.0 FDDI network controller: Digital Equipment Corporation PCI-to-PDQ Interface Chip [PFI] FDDI (DEFPA) (rev 02)
0031:02:05.0 ATM network controller: Microsemi / PMC / IDT IDT77201/77211 155Mbps ATM SAR Controller [NICStAR] (rev 03)
0032:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
0033:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
# dmesg | grep 0031:02:04.0 | cut -c16-
pci 0031:02:04.0: [1011:000f] type 00 class 0x020200
pci 0031:02:04.0: reg 0x10: [mem 0x620c080020000-0x620c08002007f]
pci 0031:02:04.0: reg 0x14: [io 0x0000-0x007f]
pci 0031:02:04.0: reg 0x18: [mem 0x620c080030000-0x620c08003ffff]
pci 0031:02:04.0: BAR0 [mem size 0x00000080]: requesting alignment to 0x10000
pci 0031:02:04.0: BAR 0: assigned [mem 0x620c080020000-0x620c08002007f]
pci 0031:02:04.0: BAR 2: assigned [mem 0x620c080030000-0x620c08003ffff]
pci 0031:02:04.0: BAR 1: no space for [io size 0x0080]
pci 0031:02:04.0: BAR 1: failed to assign [io size 0x0080]
pci 0031:02:04.0: Configured PE#fc
pci 0031:02:04.0: Adding to iommu group 9
defxx 0031:02:04.0: enabling device (0140 -> 0142)
0031:02:04.0: DEFPA at MMIO addr = 0x620c080020000, IRQ = 57, Hardware addr = 00-60-6d-93-91-98
0031:02:04.0: registered as fddi0
$ ip link show dev fddi0
4: fddi0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 4470 qdisc pfifo_fast
state UNKNOWN mode DEFAULT group default qlen 100
link/fddi 00:60:6d:93:91:98 brd ff:ff:ff:ff:ff:ff
$

While older versions of the driver did have to be explicitly configured
for MMIO rather than port I/O, a feature added with commit e89a2cfb7d7b
("[TC] defxx: TURBOchannel support"), the driver has been improved with
commit 795e272e5474 ("FDDI: defxx: Implement dynamic CSR I/O address space
selection") and the selection of the I/O space to use now fully automatic.

Then what about the other FDDI driver there, SKFP? It's not marked as
LEGACY_PCI, although it's not selectable anyway due to the dependency of
FDDI on LEGACY_PCI.

Niklas, what was the criterion for placing the LEGACY_PCI dependency?

Also do you plan to post an updated series anytime soon? I'm asking
because like with the m68k port also the MIPS one needs a more finegrained
approach and I suspect there may be other corner cases and I'd rather look
at the most recent version of your series. Otherwise I'll have a look
through your original submission, but it may have to wait until the next
weekend due to my other commitments.

Maciej

2022-02-14 11:28:25

by Niklas Schnelle

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 2022-02-14 at 09:20 +0100, Geert Uytterhoeven wrote:
> Hi Maciej,
>
> CC Niklas
>
> On Sun, Feb 13, 2022 at 1:45 PM Maciej W. Rozycki <[email protected]> wrote:
> > Nowadays PC-style parallel ports come in the form of PCI and PCIe option
> > cards and there are some combined parallel/serial option cards as well
> > that we handle in the parport subsystem. There is nothing in particular
> > that would prevent them from being used in any system equipped with PCI
> > or PCIe connectivity, except that we do not permit the PARPORT_PC config
> > option to be selected for platforms for which ARCH_MIGHT_HAVE_PC_PARPORT
> > has not been set for.
> >
> > The only PCI platforms that actually can't make use of PC-style parallel
> > port hardware are those newer PCIe systems that have no support for I/O
> > cycles in the host bridge, required by such parallel ports. An example
> > of such a host bridge is the POWER9 PHB4 device, but it is an exception
> > rather than the norm. Also it is not clear whether the serial port side
>
> Note that this hardware dependency is being addressed in
> "[RFC 00/32] Kconfig: Introduce HAS_IOPORT and LEGACY_PCI options"
> https://lore.kernel.org/all/[email protected]/

Thanks, for Cc'ing me. Note that this series is currently in kind of a
hold as we haven't yet found a clear direction yet. There was some
clear opposition for the LEGACY_PCI option introduced in that series
and only little support for HAS_IOPORT.

>
> > --- linux-macro.orig/drivers/parport/Kconfig
> > +++ linux-macro/drivers/parport/Kconfig
> > @@ -42,7 +42,7 @@ if PARPORT
> >
> > config PARPORT_PC
> > tristate "PC-style hardware"
> > - depends on ARCH_MIGHT_HAVE_PC_PARPORT
> > + depends on ARCH_MIGHT_HAVE_PC_PARPORT || PCI

This would allow selecting PARPORT_PC on s390 e.g. for allyesconfig and
randconfig and like POWER9 we definitely do not support I/O port
access.

We will also get warnings when compiling with clang. A problem my
series was originally started to address. So I'd really like to see a
better solution here for such a change. With the HAS_IOPORT option from
my series this would be simple but we don't currently have that though
maybe this is also an argument for introducing HAS_IOPORT even if we
don't add the LEGACY_PCI option.

> > help
> > You should say Y here if you have a PC-style parallel port. All
> > IBM PC compatible computers and some Alphas have PC-style
> > @@ -77,7 +77,7 @@ config PARPORT_PC_FIFO
> >
> > config PARPORT_PC_SUPERIO
> > bool "SuperIO chipset support"
> > - depends on PARPORT_PC && !PARISC
> > + depends on ARCH_MIGHT_HAVE_PC_PARPORT && PARPORT_PC && !PARISC
> > help
> > Saying Y here enables some probes for Super-IO chipsets in order to
> > find out things like base addresses, IRQ lines and DMA channels. It
> >
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds


2022-02-14 14:02:13

by Niklas Schnelle

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 2022-02-14 at 09:21 +0000, Maciej W. Rozycki wrote:
> On Mon, 14 Feb 2022, Niklas Schnelle wrote:
>
> > > > --- linux-macro.orig/drivers/parport/Kconfig
> > > > +++ linux-macro/drivers/parport/Kconfig
> > > > @@ -42,7 +42,7 @@ if PARPORT
> > > >
> > > > config PARPORT_PC
> > > > tristate "PC-style hardware"
> > > > - depends on ARCH_MIGHT_HAVE_PC_PARPORT
> > > > + depends on ARCH_MIGHT_HAVE_PC_PARPORT || PCI
> >
> > This would allow selecting PARPORT_PC on s390 e.g. for allyesconfig and
> > randconfig and like POWER9 we definitely do not support I/O port
> > access.
>
> I guess we'll have to stop it with !S390 then short-term. I don't think
> an issue with s390 should be blocking all the other platforms that can use
> these drivers just fine. I'll post v2 with that installed if you agree.
>
> Maciej

Though a bit ugly that sounds fine by me. Thanks.


2022-02-14 15:00:13

by Niklas Schnelle

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 2022-02-14 at 09:16 +0000, Maciej W. Rozycki wrote:
> On Mon, 14 Feb 2022, Geert Uytterhoeven wrote:
>
> > > The only PCI platforms that actually can't make use of PC-style parallel
> > > port hardware are those newer PCIe systems that have no support for I/O
> > > cycles in the host bridge, required by such parallel ports. An example
> > > of such a host bridge is the POWER9 PHB4 device, but it is an exception
> > > rather than the norm. Also it is not clear whether the serial port side
> >
> > Note that this hardware dependency is being addressed in
> > "[RFC 00/32] Kconfig: Introduce HAS_IOPORT and LEGACY_PCI options"
> > https://lore.kernel.org/all/[email protected]/
>
> Thanks for the pointer, I have missed the series in the LKML flood!
>
> The idea sounds good, although it's not clear to me if a config option is
> enough to get it properly covered as I don't know offhand if a single say
> ppc64le kernel can't run on systems that do and do not have support for
> port I/O over PCIe. I got hit with that the hard way with a distribution
> kernel where a driver tried to do port I/O and poked at a random location
> in the address space causing weird errors to be reported by the system
> firmware.
>
> Also I have skimmed over the series and there appears to be confusion
> between legacy PCI and conventional PCI, which are obviously not the same.
> For example this piece:
>
> diff --git a/drivers/net/fddi/Kconfig b/drivers/net/fddi/Kconfig
> index 846bf41c2717..1753c08d6423 100644
> --- a/drivers/net/fddi/Kconfig
> +++ b/drivers/net/fddi/Kconfig
> @@ -5,7 +5,7 @@
>
> config FDDI
> tristate "FDDI driver support"
> - depends on PCI || EISA || TC
> + depends on LEGACY_PCI || EISA || TC
> help
> Fiber Distributed Data Interface is a high speed local area network
> design; essentially a replacement for high speed Ethernet. FDDI can
> @@ -29,7 +29,7 @@ config DEFZA
>
> config DEFXX
> tristate "Digital DEFTA/DEFEA/DEFPA adapter support"
> - depends on FDDI && (PCI || EISA || TC)
> + depends on FDDI && (LEGACY_PCI || EISA || TC)
> help
> This is support for the DIGITAL series of TURBOchannel (DEFTA),
> EISA (DEFEA) and PCI (DEFPA) controllers which can connect you
>
> is clearly wrong. While the DEFPA card is a conventional PCI option it
> does support MMIO and does run with my legacy-free POWER9 system just
> fine (with a suitable PCIe-to-PCI bridge installed at 0031:01:00.0):
>
> # lspci
> 0000:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0001:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0001:01:00.0 Serial controller: Oxford Semiconductor Ltd OXPCIe952 Dual Native 950 UART
> 0002:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0002:01:00.0 Serial Attached SCSI controller: Adaptec Series 8 12G SAS/PCIe 3 (rev 01)
> 0003:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0003:01:00.0 USB controller: Texas Instruments TUSB73x0 SuperSpeed USB 3.0 xHCI Host Controller (rev 02)
> 0004:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0004:01:00.0 Ethernet controller: Broadcom Inc. and subsidiaries NetXtreme BCM5719 Gigabit Ethernet PCIe (rev 01)
> 0004:01:00.1 Ethernet controller: Broadcom Inc. and subsidiaries NetXtreme BCM5719 Gigabit Ethernet PCIe (rev 01)
> 0005:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0005:01:00.0 PCI bridge: ASPEED Technology, Inc. AST1150 PCI-to-PCI Bridge (rev 04)
> 0005:02:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 41)
> 0030:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0030:01:00.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
> 0030:02:00.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
> 0030:02:01.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
> 0030:02:02.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
> 0030:02:03.0 PCI bridge: PMC-Sierra Inc. PM8562 Switchtec PFX-L 32xG3 Fanout-Lite PCIe Gen3 Switch
> 0030:03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
> 0030:04:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
> 0030:05:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
> 0030:06:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller SM981/PM981/PM983
> 0031:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0031:01:00.0 PCI bridge: Texas Instruments XIO2000(A)/XIO2200A PCI Express-to-PCI Bridge (rev 03)
> 0031:02:04.0 FDDI network controller: Digital Equipment Corporation PCI-to-PDQ Interface Chip [PFI] FDDI (DEFPA) (rev 02)
> 0031:02:05.0 ATM network controller: Microsemi / PMC / IDT IDT77201/77211 155Mbps ATM SAR Controller [NICStAR] (rev 03)
> 0032:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> 0033:00:00.0 PCI bridge: IBM POWER9 Host Bridge (PHB4)
> # dmesg | grep 0031:02:04.0 | cut -c16-
> pci 0031:02:04.0: [1011:000f] type 00 class 0x020200
> pci 0031:02:04.0: reg 0x10: [mem 0x620c080020000-0x620c08002007f]
> pci 0031:02:04.0: reg 0x14: [io 0x0000-0x007f]
> pci 0031:02:04.0: reg 0x18: [mem 0x620c080030000-0x620c08003ffff]
> pci 0031:02:04.0: BAR0 [mem size 0x00000080]: requesting alignment to 0x10000
> pci 0031:02:04.0: BAR 0: assigned [mem 0x620c080020000-0x620c08002007f]
> pci 0031:02:04.0: BAR 2: assigned [mem 0x620c080030000-0x620c08003ffff]
> pci 0031:02:04.0: BAR 1: no space for [io size 0x0080]
> pci 0031:02:04.0: BAR 1: failed to assign [io size 0x0080]
> pci 0031:02:04.0: Configured PE#fc
> pci 0031:02:04.0: Adding to iommu group 9
> defxx 0031:02:04.0: enabling device (0140 -> 0142)
> 0031:02:04.0: DEFPA at MMIO addr = 0x620c080020000, IRQ = 57, Hardware addr = 00-60-6d-93-91-98
> 0031:02:04.0: registered as fddi0
> $ ip link show dev fddi0
> 4: fddi0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 4470 qdisc pfifo_fast
> state UNKNOWN mode DEFAULT group default qlen 100
> link/fddi 00:60:6d:93:91:98 brd ff:ff:ff:ff:ff:ff
> $
>
> While older versions of the driver did have to be explicitly configured
> for MMIO rather than port I/O, a feature added with commit e89a2cfb7d7b
> ("[TC] defxx: TURBOchannel support"), the driver has been improved with
> commit 795e272e5474 ("FDDI: defxx: Implement dynamic CSR I/O address space
> selection") and the selection of the I/O space to use now fully automatic.

Very interesting and thanks for the input! On s390 we really only have
very few different PCI devices and I can only test another hand full
with my private x86 and ARM systems.

>
> Then what about the other FDDI driver there, SKFP? It's not marked as
> LEGACY_PCI, although it's not selectable anyway due to the dependency of
> FDDI on LEGACY_PCI.
>
> Niklas, what was the criterion for placing the LEGACY_PCI dependency?

Hmm, honestly I haven't really worked on this recently. There were some
open questions from Bjorn towards Arnd and I was waiting for his reply
but I guess he missed those. I think what you noticed was the main
problem, there wasn't really a clear set of criteria for LEGACY_PCI and
even for HAS_IOPORT we missed some uses if they were not compiled on
s390's allyesconfig due to other dependencies.

>
> Also do you plan to post an updated series anytime soon? I'm asking
> because like with the m68k port also the MIPS one needs a more finegrained
> approach and I suspect there may be other corner cases and I'd rather look
> at the most recent version of your series. Otherwise I'll have a look
> through your original submission, but it may have to wait until the next
> weekend due to my other commitments.
>
> Maciej

That sounds like you do see a need for something like HAS_IOPORT too,
correct? Maybe with some input what you need and possibly stripping the
LEGACY_PCI option it might make sense to do a new version. Rather than
possibly getting in your way could directly work in your input.


2022-02-14 15:23:04

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 14 Feb 2022, Niklas Schnelle wrote:

> > > --- linux-macro.orig/drivers/parport/Kconfig
> > > +++ linux-macro/drivers/parport/Kconfig
> > > @@ -42,7 +42,7 @@ if PARPORT
> > >
> > > config PARPORT_PC
> > > tristate "PC-style hardware"
> > > - depends on ARCH_MIGHT_HAVE_PC_PARPORT
> > > + depends on ARCH_MIGHT_HAVE_PC_PARPORT || PCI
>
> This would allow selecting PARPORT_PC on s390 e.g. for allyesconfig and
> randconfig and like POWER9 we definitely do not support I/O port
> access.

I guess we'll have to stop it with !S390 then short-term. I don't think
an issue with s390 should be blocking all the other platforms that can use
these drivers just fine. I'll post v2 with that installed if you agree.

Maciej

2022-02-14 21:28:47

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 14 Feb 2022, Niklas Schnelle wrote:

> > While older versions of the driver did have to be explicitly configured
> > for MMIO rather than port I/O, a feature added with commit e89a2cfb7d7b
> > ("[TC] defxx: TURBOchannel support"), the driver has been improved with
> > commit 795e272e5474 ("FDDI: defxx: Implement dynamic CSR I/O address space
> > selection") and the selection of the I/O space to use now fully automatic.
>
> Very interesting and thanks for the input! On s390 we really only have
> very few different PCI devices and I can only test another hand full
> with my private x86 and ARM systems.

Note that for TURBOchannel support, which is likewise MMIO only, the
driver has this:

#if defined(CONFIG_EISA) || defined(CONFIG_PCI)
#define dfx_use_mmio bp->mmio
#else
#define dfx_use_mmio true
#endif

so if your proposal to add HAS_IOPORT goes forward it'll be enough if we
update the condition to:

#if defined(CONFIG_HAS_IOPORT)

or maybe even rewrite the entire piece as:

#define dfx_use_mmio (!IS_ENABLED(CONFIG_HAS_IOPORT) || bp->mmio)

and all the port I/O stuff will be optimised away by the compiler. The
only part of the driver that actually cannot do without port I/O is EISA
support, which uses the EISA slot port I/O space for BAR accesses even
if the actual CSR block has been set up to be decoded in the MMIO space.

> > Then what about the other FDDI driver there, SKFP? It's not marked as
> > LEGACY_PCI, although it's not selectable anyway due to the dependency of
> > FDDI on LEGACY_PCI.
> >
> > Niklas, what was the criterion for placing the LEGACY_PCI dependency?
>
> Hmm, honestly I haven't really worked on this recently. There were some
> open questions from Bjorn towards Arnd and I was waiting for his reply
> but I guess he missed those. I think what you noticed was the main
> problem, there wasn't really a clear set of criteria for LEGACY_PCI and
> even for HAS_IOPORT we missed some uses if they were not compiled on
> s390's allyesconfig due to other dependencies.

A dynamic boolean variable might be good having for platforms which may
or may not have PCI port I/O available depending on the specific system
model in addition to a compile-time constant of HAS_IOPORT. I looked
into it briefly in the context of the POWER9 system when I got it back
in 2020, but figured out it wasn't straightforward enough and decided I
could not afford the time for a proper investigation.

> > Also do you plan to post an updated series anytime soon? I'm asking
> > because like with the m68k port also the MIPS one needs a more finegrained
> > approach and I suspect there may be other corner cases and I'd rather look
> > at the most recent version of your series. Otherwise I'll have a look
> > through your original submission, but it may have to wait until the next
> > weekend due to my other commitments.
>
> That sounds like you do see a need for something like HAS_IOPORT too,
> correct? Maybe with some input what you need and possibly stripping the
> LEGACY_PCI option it might make sense to do a new version. Rather than
> possibly getting in your way could directly work in your input.

Yes, it does seem to me like a good direction, but will surely require
some coordination from platform and driver maintainers, as it's not
always easy for someone not familiar with a specific piece what the
context is (such as with the defxx driver as I noted above).

Maciej