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

[Arnd Bergmann]

On Thu, Sep 23, 2021 at 4:55 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
> On Thu, Sep 23, 2021 at 3:26 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > >
> > > > > mt7621-pci 1e140000.pcie:       IO 0x001e160000..0x001e16ffff -> 0x001e160000
> > > > > LOGIC PIO: PIO TO CPUADDR: ADDR: 0x1e160000 -  addr HW_START:
> > > > > 0x1e160000 + RANGE IO: 0x00000000
> > >
> > > Why is my RANGE IO start transformed here to 0x0? Should not be the
> > > one defined in dts 0x001e160000?
>
> >
> > Can you show the exact property in your device tree? It sounds like the
> > problem is an incorrect entry in the ranges, unless the chip is hardwired
> > to the bus address in an unusual way.
>
> Here it is:
>
> ranges = <0x02000000 0 0x60000000 0x60000000 0 0x10000000>,  /* pci memory */
>                <0x01000000 0 0x1e160000 0x1e160000 0 0x00010000>;  /*
> io space */

Ok, got it. So the memory space uses a normal zero offset with cpu address
equal to bus address, but the I/O space has a rather usual mapping of
bus address equal to the window into the mmio space, with an offset of
0x1e160000.

The normal way to do this would be map the PCI port range 0-0x10000
into CPU address 0x1e160000, like:

ranges = <0x02000000 0 0x60000000 0x60000000 0 0x10000000>,  /* pci memory */
               <0x01000000 0 0x1e160000 0 0 0x00010000>;

Do you know where that mapping is set up? Is this a hardware setting,
or is there a way to change the inbound I/O port ranges to the
normal mapping?

> > > Currently, no. But if they were ideally moved to work in the same way
> > > mt7621 would be the same case. Mt7621 is device tree based PCI host
> > > bridge driver that uses pci core apis but is still mips since it has
> > > to properly set IO coherency units which is a mips thing...
> >
> > I don't know what those IO coherency units are, but I would think that
> > if you have to do some extra things on MIPS but not ARM, then those
> > should be done from the common PCI host bridge code and stubbed out
> > on architectures that don't need them.
>
> Simple definition here [0]. And we must adjust them in the driver here
> [1]. Mips code, yes, but cannot be done in another way. Is related
> with address of the memory resource and must be set up. In any other
> case the pci subsystem won't work. I initially submitted this driver
> from staging to arch/mips but I was told that even though it is mips
> code, as it is device tree based and use common pci core apis, it
> would be better to move it to 'drivers/pci/controller'. But I still
> need the mips specific code for the iocu thing.

It does sound like this could be reworked into an architecture
specific helper that is defined for MIPS but just an empty stub
for everything else. Or you can just have an #ifdef in your
driver to at least enable compile-testing it on other architecture
if not completely sharing it with others. This is certainly a less
important point compared to the I/O port mapping.

> > It is possible that the hardware (or bootloader) designers
> > misunderstood what the window is about, and hardcoded it so
> > that the port number on the bus is the same as the physical
> > address as seen from the CPU. If this is the case and you
> > can't change it to a sane value, you have to put the 1:1
> > translation into the DT and would actually get the strange
> > port numbers 0x1e161000-0x1e16100f from that nonzero offset.
>
> Yes, and that pci_add_resource_offset() is called inside
> devm_of_pci_get_host_bridge_resources() after parsing the ranges and
> storing them as resources.  To calculate that offset passed around,
> subtracts: res->start - range.pci_addr [2], so looking into my ranges
> my offset should be zero. And I have added a trace just to confirm and
> there are zero:
>
> mt7621-pci 1e140000.pcie:      MEM 0x0060000000..0x006fffffff -> 0x0060000000
> OF: IO START returned by pci_address_to_pio: 0x60000000-0x6fffffff
> PCI: OF: OFFSET -> RES START 0x60000000 - PCI ADDRESS 0x60000000 -> 0x0
> mt7621-pci 1e140000.pcie:       IO 0x001e160000..0x001e16ffff -> 0x001e160000
> OF: IO START returned by pci_address_to_pio: 0x1e160000-0x1e16ffff
> PCI: OF: OFFSET -> RES START 0x1e160000 - PCI ADDRESS 0x1e160000 -> 0x0
>
> But if I define PCI_IOBASE I get my I/O range start set to zero but
> also the offset?? Why this substract is not getting '0x1e160000' as
> offset here?
>
> LOGIC PIO: PIO TO CPUADDR: ADDR: 0x1e160000 -  addr HW_START:
> 0x1e160000 + RANGE IO: 0x00000000
> OF: IO START returned by pci_address_to_pio: 0x0-0xffff
> PCI: OF: OFFSET -> RES START 0x0 - PCI ADDRESS 0x1e160000 -> 0x0

I'm not completely following what each of the numbers in your log refer
to.  The main thing you still need is to have the hardware set up
so it matches the ranges property, and the io_offset matching that.

With PCI_IOBASE=0x1e160000, there are two possible ways this
can work:

a) according to the ranges property you listed above:
    linux port numbers 0-0xffff, pci port numbers 0x1e160000-0x1e16ffff,
    io_offset=0x1e160000 (possibly negative that number, I can never
    keep track)

b) the normal way with the ranges according to my reply above, works only
    if the hardware mapping window can be reprogrammed that way:
    linux port numbers 0-0xffff, pci port numbers 0-0xffff,
    io_offset=0

         Arnd