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
