diff mbox series

[v2] parport_pc: Also enable driver for PCI systems

Message ID alpine.DEB.2.21.2202141955550.34636@angie.orcam.me.uk (mailing list archive)
State New, archived
Headers show
Series [v2] parport_pc: Also enable driver for PCI systems | expand

Commit Message

Maciej W. Rozycki Feb. 14, 2022, 8:16 p.m. UTC
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.  Notably, 
this includes the s390 arch, which has port I/O accessors that cause 
compilation warnings (promoted to errors with `-Werror'), and there are 
other cases such as the POWER9 PHB4 device, though this one has variable 
port I/O accessors that depend on the particular system.  Also it is not 
clear whether the serial port side of devices enabled by PARPORT_SERIAL 
uses port I/O or MMIO.  Finally Super I/O solutions are always either 
ISA or platform devices.

Make the PARPORT_PC option selectable also for PCI systems then, except 
for the s390 arch, however limit the availability of PARPORT_PC_SUPERIO 
to platforms that enable ARCH_MIGHT_HAVE_PC_PARPORT.  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, except for s390, now excluded.  Let 
me know if I got anything wrong here.

  Maciej

Changes from v1:

- Exclude s390 systems, update the change description accordingly.
---
 arch/arm64/include/asm/Kbuild  |    1 +
 arch/csky/include/asm/Kbuild   |    1 +
 arch/riscv/include/asm/Kbuild  |    1 +
 arch/um/include/asm/Kbuild     |    1 +
 arch/xtensa/include/asm/Kbuild |    1 +
 drivers/parport/Kconfig        |    4 ++--
 6 files changed, 7 insertions(+), 2 deletions(-)

linux-parport-pc-pci.diff

Comments

Christoph Hellwig Feb. 15, 2022, 7:04 a.m. UTC | #1
> ===================================================================
> --- 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

Instead of adding generic-y just ad a mandatory-y in
include/asm-generic/Kbuild.
Maciej W. Rozycki Feb. 15, 2022, 9:11 a.m. UTC | #2
On Mon, 14 Feb 2022, Christoph Hellwig wrote:

> > ===================================================================
> > --- 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
> 
> Instead of adding generic-y just ad a mandatory-y in
> include/asm-generic/Kbuild.

 I'm inconvinced.  Not all archs want it, 5 don't.

  Maciej
Christoph Hellwig Feb. 16, 2022, 3:28 p.m. UTC | #3
On Tue, Feb 15, 2022 at 09:11:45AM +0000, Maciej W. Rozycki wrote:
> On Mon, 14 Feb 2022, Christoph Hellwig wrote:
> 
> > > ===================================================================
> > > --- 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
> > 
> > Instead of adding generic-y just ad a mandatory-y in
> > include/asm-generic/Kbuild.
> 
>  I'm inconvinced.  Not all archs want it, 5 don't.

Which is exactly what mandatory-y is for.  Provide the asm-generic
version by default, but let architectures override it.
Maciej W. Rozycki Feb. 16, 2022, 4:35 p.m. UTC | #4
On Wed, 16 Feb 2022, Christoph Hellwig wrote:

> > > Instead of adding generic-y just ad a mandatory-y in
> > > include/asm-generic/Kbuild.
> > 
> >  I'm inconvinced.  Not all archs want it, 5 don't.
> 
> Which is exactly what mandatory-y is for.  Provide the asm-generic
> version by default, but let architectures override it.

 I don't think so.  Those 5 architectures don't want it at all; 7 other 
ones have their own versions.

 Otherwise we could blanket-list all asm-generic headers as mandatory-y.

  Maciej
Arnd Bergmann Feb. 16, 2022, 5:17 p.m. UTC | #5
On Wed, Feb 16, 2022 at 5:35 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Wed, 16 Feb 2022, Christoph Hellwig wrote:
>
> > > > Instead of adding generic-y just ad a mandatory-y in
> > > > include/asm-generic/Kbuild.
> > >
> > >  I'm inconvinced.  Not all archs want it, 5 don't.
> >
> > Which is exactly what mandatory-y is for.  Provide the asm-generic
> > version by default, but let architectures override it.
>
>  I don't think so.  Those 5 architectures don't want it at all; 7 other
> ones have their own versions.
>
>  Otherwise we could blanket-list all asm-generic headers as mandatory-y.

I think ideally the PCI driver should be a separate file from the rest, or
possibly it could get split up even further.

parport_pc_probe_port()/parport_pc_unregister_port() are already exported
by the driver and used by some of the front-ends. The parport_pc_pci_driver
looks like it could easily go into one file using module_pci_driver(), while
the platform driver stays in the existing file and the legacy detection logic
goes into a third one. The powerpc and sparc versions could technically
also be separate drivers, but I wouldn't take the rework that far.

       Arnd
Maciej W. Rozycki March 1, 2022, 8:43 p.m. UTC | #6
On Mon, 14 Feb 2022, Maciej W. Rozycki 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.

 Ping for:

<https://lore.kernel.org/lkml/alpine.DEB.2.21.2202141955550.34636@angie.orcam.me.uk/>

  Maciej
Icenowy Zheng March 2, 2022, 1:19 p.m. UTC | #7
在 2022-02-14星期一的 20:16 +0000,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.  Notably,
> this includes the s390 arch, which has port I/O accessors that cause 
> compilation warnings (promoted to errors with `-Werror'), and there
> are 
> other cases such as the POWER9 PHB4 device, though this one has
> variable 
> port I/O accessors that depend on the particular system.  Also it is
> not 
> clear whether the serial port side of devices enabled by
> PARPORT_SERIAL 
> uses port I/O or MMIO.  Finally Super I/O solutions are always either
> ISA or platform devices.

Just spot this patch in linux-riscv mailing list, I think there's a
pending patchset that tries to add a HAS_IOPORT Kconfig option, which
can be used in this situation.

> 
> Make the PARPORT_PC option selectable also for PCI systems then,
> except 
> for the s390 arch, however limit the availability of
> PARPORT_PC_SUPERIO 
> to platforms that enable ARCH_MIGHT_HAVE_PC_PARPORT.  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, except for s390, now excluded. 
> Let 
> me know if I got anything wrong here.
> 
>   Maciej
> 
> Changes from v1:
> 
> - Exclude s390 systems, update the change description accordingly.
> ---
>  arch/arm64/include/asm/Kbuild  |    1 +
>  arch/csky/include/asm/Kbuild   |    1 +
>  arch/riscv/include/asm/Kbuild  |    1 +
>  arch/um/include/asm/Kbuild     |    1 +
>  arch/xtensa/include/asm/Kbuild |    1 +
>  drivers/parport/Kconfig        |    4 ++--
>  6 files changed, 7 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/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 && !S390)
>         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
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Maciej W. Rozycki March 2, 2022, 1:45 p.m. UTC | #8
On Wed, 2 Mar 2022, Icenowy Zheng 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.  Notably,
> > this includes the s390 arch, which has port I/O accessors that cause 
> > compilation warnings (promoted to errors with `-Werror'), and there
> > are 
> > other cases such as the POWER9 PHB4 device, though this one has
> > variable 
> > port I/O accessors that depend on the particular system.  Also it is
> > not 
> > clear whether the serial port side of devices enabled by
> > PARPORT_SERIAL 
> > uses port I/O or MMIO.  Finally Super I/O solutions are always either
> > ISA or platform devices.
> 
> Just spot this patch in linux-riscv mailing list, I think there's a
> pending patchset that tries to add a HAS_IOPORT Kconfig option, which
> can be used in this situation.

 Thanks for your input.

 That has been actually discussed already with a conclusion that more work 
is required to have HAS_IOPORT supported, see the thread starting from: 
<https://lore.kernel.org/lkml/CAMuHMdW-utcFzCZTgqONjxs=U662nF0=aBQu7Zi7FBQouwiA3g@mail.gmail.com/>. 
(there's a reference to the HAS_IOPORT patchset there as well).  Once that 
has been sorted configuration conditions for the parport driver can be 
updated accordingly.  For the time being the !S390 qualification should 
do.

  Maciej
Sudip Mukherjee March 2, 2022, 8:44 p.m. UTC | #9
On Mon, Feb 14, 2022 at 8:16 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.  Notably,
> this includes the s390 arch, which has port I/O accessors that cause
> compilation warnings (promoted to errors with `-Werror'), and there are
> other cases such as the POWER9 PHB4 device, though this one has variable
> port I/O accessors that depend on the particular system.  Also it is not
> clear whether the serial port side of devices enabled by PARPORT_SERIAL
> uses port I/O or MMIO.  Finally Super I/O solutions are always either
> ISA or platform devices.
>
> Make the PARPORT_PC option selectable also for PCI systems then, except
> for the s390 arch, however limit the availability of PARPORT_PC_SUPERIO
> to platforms that enable ARCH_MIGHT_HAVE_PC_PARPORT.  Update platforms
> accordingly for the required <asm/parport.h> header.
>
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>

Acked-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>

Usually parport patches goes via Greg's tree. Adding Greg.
Maciej W. Rozycki March 16, 2022, 6:45 p.m. UTC | #10
On Wed, 2 Mar 2022, Sudip Mukherjee wrote:

> > Make the PARPORT_PC option selectable also for PCI systems then, except
> > for the s390 arch, however limit the availability of PARPORT_PC_SUPERIO
> > to platforms that enable ARCH_MIGHT_HAVE_PC_PARPORT.  Update platforms
> > accordingly for the required <asm/parport.h> header.
> >
> > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> 
> Acked-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> 
> Usually parport patches goes via Greg's tree. Adding Greg.

 Thank you.  I have since been able to move my parport card to my RISC-V 
machine, and the result is as follows:

parport_pc 0000:07:00.0: enabling device (0000 -> 0001)
PCI parallel port detected: 1415:c118, I/O at 0x8(0x4), IRQ 38
parport0: PC-style at 0x8 (0x4), irq 38, using FIFO [PCSPP,TRISTATE,COMPAT,EPP,ECP]
lp0: using parport0 (interrupt-driven).

with the patch from:

<https://lore.kernel.org/lkml/alpine.DEB.2.21.2202260044180.25061@angie.orcam.me.uk/>

also applied so as to prevent I/O port 0 from being assigned by PCI code 
that wouldn't actually work (so allocation for the two BARs is at 0x4 and 
0x8 instead).

  Maciej
diff mbox series

Patch

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/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 && !S390)
 	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