Re: [PATCH v3] PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined

[Arnd Bergmann]

On Wed, Sep 22, 2021 at 7:42 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
> On Wed, Sep 22, 2021 at 5:47 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >
> > I'm not completely sure where your platform fits in here, it sounds like you
> > address them using a machine specific physical address as the base in
> > inb() plus the port number as an offset, is that correct?
>
> I guess none of the above options? I will try to explain this as per
> my understanding.
>
> [+cc Thomas Bogendoerfer as mips maintainer and with better knowledge
> of mips platforms than me]
>
> On MIPS I/O ports are memory mapped, so we access them using normal
> load/store instructions.
> Mips 'plat_mem_setup()' function does a 'set_io_port_base(KSEG1)'.
> There, variable 'mips_io_port_base'
> is set then using this address which is a virtual address to which all
> ports are being mapped.
> KSEG1 addresses are uncached and are not translated by the MMU. This
> KSEG1 range is directly mapped in physical space starting with address
> 0x0.
> Because of this reason, defining PCI_IOBASE as KSEG1 won't work since,
> at the end 'pci_parse_request_of_pci_ranges' tries to remap to a fixed
> virtual address (PCI_IOBASE). This can't work for KSEG1 addresses.
> What happens if I try to do that is that I get bad addresses at pci
> enumeration for IO resources. Mips ralink mt7621 SoC (which is the one
> I am using and trying to mainline the driver from staging) have I/O at
> address 0x1e160000. So instead of getting this address for pcie IO
> BARS I get a range from 0x0000 to 0xffff since 'pci_adress_to_pio' in
> that case got that range 0x0-0xffff which is wrong. To have this
> working this way we would need to put PCI_IOBASE somewhere into KSEG2
> which will result in creating TLB entries for IO addresses, which most
> of the time isn't needed on MIPS because of access via KSEG1. Instead
> of that, what happens when I avoid defining PCI_IOBASE and set
> IO_SPACE_LIMIT  (See [0] and [1] commits already added to staging tree
> which was part of this patch series for context of what works with
> this patch together) all works properly. There have also been some
> patches accepted in the past which avoid this
> 'pci_parse_request_of_pci_ranges' call since it is not working for
> most pci legacy drivers of arch/mips for ralinks platform [2].
>
> So I am not sure what should be the correct approach to properly make
> this work (this one works for me and I cannot see others better) but I
> will be happy to try whatever you propose for me to do.

Ok, thank you for the detailed explanation.

I suppose you can use the generic infrastructure in asm-generic/io.h
if you "#define PCI_IOBASE mips_io_port_base". In this case, you
should have an architecture specific implementation of
pci_remap_iospace() that assigns mips_io_port_base.
pci_remap_iospace() was originally meant as an architecture
specific helper, but it moved into generic code after all architectures
had the same requirements. If MIPS has different requirements,
then it should not be shared.

I don't yet understand how you deal with having multiple PCIe
host bridge devices if they have distinct I/O port ranges.
Without remapping to dynamic virtual addresses, does
that mean that every MMIO register between the first and
last PCIe bridge also shows up in /dev/ioport? Or do you
only support port I/O on the first PCIe host bridge?

Note that you could also decide to completely sidestep the
problem by just defining port I/O to be unavailable on MIPS
when probing a generic host bridge driver. Most likely this
is never going to be used anyway, and it will be rather hard
to test if you don't already have the need ;-)

         Arnd