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

[Sergio Paracuellos]

Hi Arnd,

On Wed, Sep 22, 2021 at 8:07 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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.

No, that is what I tried originally defining PCI_IOBASE as
_AC(0xa0000000, UL) [0] which is the same as KSEG1 [1] that ends in
'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 see. So, if it can not be shared, would defining 'pci_remap_iospace'
as 'weak' acceptable? Doing in this way I guess I can redefine the
symbol for mips to have the same I currently have but without the
ifdef in the core APIs...

>
> 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?

For example, this board is using all available three pci ports [2] and I get:

root@gnubee:~# cat /proc/ioports
1e160000-1e16ffff : pcie@1e140000
  1e160000-1e160fff : PCI Bus 0000:01
    1e160000-1e16000f : 0000:01:00.0
      1e160000-1e16000f : ahci
    1e160010-1e160017 : 0000:01:00.0
      1e160010-1e160017 : ahci
    1e160018-1e16001f : 0000:01:00.0
      1e160018-1e16001f : ahci
    1e160020-1e160023 : 0000:01:00.0
      1e160020-1e160023 : ahci
    1e160024-1e160027 : 0000:01:00.0
      1e160024-1e160027 : ahci
  1e161000-1e161fff : PCI Bus 0000:02
    1e161000-1e16100f : 0000:02:00.0
      1e161000-1e16100f : ahci
    1e161010-1e161017 : 0000:02:00.0
      1e161010-1e161017 : ahci
    1e161018-1e16101f : 0000:02:00.0
      1e161018-1e16101f : ahci
    1e161020-1e161023 : 0000:02:00.0
      1e161020-1e161023 : ahci
    1e161024-1e161027 : 0000:02:00.0
      1e161024-1e161027 : ahci
  1e162000-1e162fff : PCI Bus 0000:03
    1e162000-1e16200f : 0000:03:00.0
      1e162000-1e16200f : ahci
    1e162010-1e162017 : 0000:03:00.0
      1e162010-1e162017 : ahci
    1e162018-1e16201f : 0000:03:00.0
      1e162018-1e16201f : ahci
    1e162020-1e162023 : 0000:03:00.0
      1e162020-1e162023 : ahci
    1e162024-1e162027 : 0000:03:00.0
      1e162024-1e162027 : ahci
root@gnubee:~#

>
> 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 ;-)

I don't really have any pci card with IO resources to test, but this
SoC is extensively used in openWRT and I remember people who had such
cards with I/O because they were asking me for I/O since it did not
work properly, so I guess I can decide that but if it can work I
prefer to try to make it work :). I searched a bit for the link with
that conversation but I was not able to find it :(.

Thanks for your feedback and time :).

Best regards,
    Sergio Paracuellos

>
>          Arnd

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=159697474db41732ef3b6c2e8d9395f09d1f659e
[1]: https://elixir.bootlin.com/linux/v5.15-rc2/source/arch/mips/include/asm/addrspace.h#L99
[2]: https://patchwork.kernel.org/project/linux-pci/patch/20210922050035.18162-2-sergio.paracuellos@xxxxxxxxx/