Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 14 Feb 2022 20:42:05 +0000 (GMT)
From:   "Maciej W. Rozycki" <macro@orcam.me.uk>
To:     Niklas Schnelle <schnelle@linux.ibm.com>
cc:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Guo Ren <guoren@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Jeff Dike <jdike@addtoit.com>,
        Richard Weinberger <richard@nod.at>,
        Anton Ivanov <anton.ivanov@cambridgegreys.com>,
        Chris Zankel <chris@zankel.net>,
        Max Filippov <jcmvbkbc@gmail.com>,
        linux-arm-kernel@lists.infradead.org, linux-csky@vger.kernel.org,
        linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org,
        linux-um@lists.infradead.org, linux-xtensa@linux-xtensa.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems
In-Reply-To: <624931c5da90387324a97be57745b13af4738bda.camel@linux.ibm.com>
Message-ID: <alpine.DEB.2.21.2202142018060.34636@angie.orcam.me.uk>
References: <alpine.DEB.2.21.2202122313460.34636@angie.orcam.me.uk>  <CAMuHMdW-utcFzCZTgqONjxs=U662nF0=aBQu7Zi7FBQouwiA3g@mail.gmail.com>  <alpine.DEB.2.21.2202140833290.34636@angie.orcam.me.uk>
 <624931c5da90387324a97be57745b13af4738bda.camel@linux.ibm.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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