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

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 2:46 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
> On Fri, Sep 24, 2021 at 1:39 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Fri, Sep 24, 2021 at 12:15 PM Sergio Paracuellos
>
> > I meant RALINK_PCI_IOBASE. We do need to write both, to clarify:
> >
> > RALINK_PCI_IOBASE must be set to match the *bus* address in DT,
> > so ideally '0', but any value should work as long as these two match.
> >
> > PCI_IOBASE/mips_io_port_base must be set to the *CPU* address
> > in DT, so that must be 0x1e160000, possibly converted from
> > physical to a virtual __iomem address (this is where my MIPS
> > knowledge ends).
>
> Understood. I have tried the following:
>
> I have added the following at the beggining of the pci host driver to
> match what you are describing above:
>
> unsigned long vaddr = (unsigned long)ioremap(PCI_IOBASE, 0x10000);
> set_io_port_base(vaddr);
>
> dev_info(dev, "Setting base to PCI_IOBASE: 0x%x -> mips_io_port_base
> 0x%lx", PCI_IOBASE, mips_io_port_base);
>
> PCI_IOBASE is the physical cpu address. Hence, 0x1e160000
> set_io_port_base sets 'mips_io_port_base' to the virtual address where
> 'PCI_IOBASE' has been mapped (vaddr).

Ok, sounds good. I would still suggest using
"#define PCI_IOBASE mips_io_port_base", so it has the same meaning
as on other architectures (the virtual address of port 0), and replace
the hardcoded base with the CPU address you read from DT to
make that code more portable. As a general rule, DT-enabled drivers
should contain no hardcoded addresses.

> However, nothing seems to change:
>
> mt7621-pci 1e140000.pcie: Setting base to PCI_IOBASE: 0x1e160000 ->
> mips_io_port_base 0xbe160000
>                                                 ^^^
>                                                  This seems aligned
> with what you are saying. mips_io_port_base have now a proper virtual
> addr for 0x1e160000

Ok.

            Arnd