Date:   Thu, 23 Sep 2021 07:51:20 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        linux-pci <linux-pci@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-staging@...ts.linux.dev, gregkh <gregkh@...uxfoundation.org>,
        Liviu Dudau <Liviu.Dudau@....com>,
        Rob Herring <robh@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Subject: Re: [PATCH v3] PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined

 isOn Wed, Sep 22, 2021 at 10:55 PM Sergio Paracuellos
<sergio.paracuellos@...il.com> wrote:
> On Wed, Sep 22, 2021 at 9:57 PM Arnd Bergmann <arnd@...db.de> wrote:
> >
> > On Wed, Sep 22, 2021 at 8:40 PM Sergio Paracuellos
> > <sergio.paracuellos@...il.com> wrote:
> > > On Wed, Sep 22, 2021 at 8:07 PM Arnd Bergmann <arnd@...db.de> wrote:
> > > > On Wed, Sep 22, 2021 at 7:42 PM Sergio Paracuellos
> > > > 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'.
> >
> > Defining it as KSEG1 would be problematic because that means that
> > the Linux-visible port numbers are offset from the bus-visible ones.
> >
> > You really want PCI_IOBASE to point to the address of port 0.
>
> Do you mean that doing
>
> #define PCI_IOBASE mips_io_port_base
>
> would have different result that doing what I did
>
> #define PCI_IOBASE _AC(0xa0000000, UL)
>
> ?
>
> I am not really understanding this yet (I think I need a bit of sleep
> time :)), but I will test this tomorrow and come back to you again
> with results.

Both would let devices access the registers, but they are different
regarding the bus translations you have to program into the
host bridge, and how to access the hardcoded port numbers.

> > > > 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 would hope to kill off the __weak functions, and prefer using an #ifdef
> > around the generic implementation. One way to do it is to define
> > a macro with the same name, such as
> >
> > #define pci_remap_iospace pci_remap_iospace
>
> I guess this should be defined in arch/mips/include/asm/pci.h?

Yes, that would be a good place for that, possibly next to
the (static inline) definition.

> > and then use #ifdef around the C function to see if that's already defined.
>
> I see. That would work, I guess. But I don't really understand why
> this approach would be better than this patch changes itself. Looks
> more hacky to me. As Bjorn pointed out in a previous version of this
> patch [0], this case is the same as the one in
> 'acpi_pci_root_remap_iospace' and the same approach is used there...

The acpi_pci_root_remap_iospace() does this because on that code is
shared with x86 and ia64, where the port numbers are accessed using
separate instructions that do not translate into MMIO addresses at all.

On MIPS, the port access eventually does translate into MMIO, and
you need a way to communicate the mapping between the host
bridge and the architecture specific code.

This is particularly important since we want the host bridge driver
to be portable. If you set up the mapping differently between e.g.
mt7621 and mt7623, they are not able to use the same driver
code for setting pci_host_bridge->io_offset and for programming
the inbound translation registers.

> > I only see one host bridge here though, and it has a single
> > I/O port range, so maybe all three ports are inside of
> > a single PCI domain?
>
> See this cover letter [1] with a fantastic ascii art :) to a better
> understanding of this pci topology. Yes, there is one host bridge and
> from here three virtual bridges where at most three endpoints can be
> connected.

Ok, so you don't have the problem I was referring to. A lot of
SoCs actually have multiple host bridges, but only one root
port per host bridge, because they are based on licensed IP
blocks that don't support a normal topology like the one you have.

> > Having high numbers for the I/O ports is definitely a
> > problem as I mentioned. Anything that tries to access
> > PC-style legacy devices on the low port numbers
> > will now directly go on the bus accessing MMIO
> > registers that it shouldn't, either causing a CPU exception
> > or (worse) undefined behavior from random register
> > accesses.
>
> All I/O port addresses for ralink SoCs are in higher addresses than
> default IO_SPACE_LIMIT 0xFFFF, that's why we have to also change this
> limit together with this patch changes. Nothing to do with this, is an
> architectural thing of these SoCs.

I don't understand. What I see above is that the host bridge
has the region 1e160000-1e16ffff registered, so presumably
1e160000 is actually the start of the window into the host bridge.
If you set PCI_IOBASE to that location, the highest port number
would become 0x2027, which is under 0xffff.

       Arnd

Powered by blists - more mailing lists