[PATCH] parport_pc: Also enable driver for PCI systems

[PATCH] parport_pc: Also enable driver for PCI systems

[Maciej W. Rozycki]

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 <macro@orcam.me.uk>
---
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

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

[Geert Uytterhoeven]

Hi Maciej,

CC Niklas

On Sun, Feb 13, 2022 at 1:45 PM Maciej W. Rozycki <macro@orcam.me.uk> 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/20211227164317.4146918-1-schnelle@linux.ibm.com/

> --- 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 -- geert@linux-m68k.org

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

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

[Niklas Schnelle]

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 <macro@orcam.me.uk> 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/20211227164317.4146918-1-schnelle@linux.ibm.com/

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 -- geert@linux-m68k.org
> 
> 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

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

[Maciej W. Rozycki]

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/20211227164317.4146918-1-schnelle@linux.ibm.com/

 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

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

[Maciej W. Rozycki]

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

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

[Niklas Schnelle]

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.

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

[Niklas Schnelle]

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/20211227164317.4146918-1-schnelle@linux.ibm.com/
> 
>  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.

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

[Maciej W. Rozycki]

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