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

[Sergio Paracuellos]

Hi Arnd,

On Fri, Sep 24, 2021 at 3:28 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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.

Yes, it must be cleaned. I was only explaining a possible way to proceed.

So, the changes would be:
1) Reverting already added two commits in staging-tree [0] and [1].
(two revert patches)
2) Setting PCI_IOBASE to 'mips_io_port_base' so the spaces.h become: (one patch)

$ cat arch/mips/include/asm/mach-ralink/spaces.h
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef __ASM_MACH_RALINK_SPACES_H_
#define __ASM_MACH_RALINK_SPACES_H_

#define PCI_IOBASE    mips_io_port_base
#define PCI_IOSIZE    SZ_16M
#define IO_SPACE_LIMIT  (PCI_IOSIZE - 1)

#include <asm/mach-generic/spaces.h>
#endif

3) Change the value written in RALINK_PCI_IOBASE to be sure the value
written takes into account address before linux port translation (one
patch):

pcie_write(pcie, entry->res->start - entry->offset, RALINK_PCI_IOBASE);

4) Virtually Map cpu physical address 0x1e160000 and set
'mips_io_port_base' to that virtual address. Something like the
following (one patch):

static int mt7621_set_io(struct device *dev)
{
    struct device_node *node = dev->of_node;
    struct of_pci_range_parser parser;
    struct of_pci_range range;
    unsigned long vaddr;
    int ret = -EINVAL;

    ret = of_pci_range_parser_init(&parser, node);
    if (ret)
        return ret;

    for_each_of_pci_range(&parser, &range) {
        switch (range.flags & IORESOURCE_TYPE_BITS) {
        case IORESOURCE_IO:
            vaddr = (unsigned long)ioremap(range.cpu_addr, range.size);
            set_io_port_base(vaddr);
            ret = 0;
            break;
        }
    }

    return ret;
}

static int mt7621_pci_probe(struct platform_device *pdev)
{
  ...
    err = mt7621_set_io(dev);
    if (err) {
        dev_err(dev, "error setting io\n");
        return err;
    }
...
    return 0;
}

And now my concerns:
1) We have to read DT range IO values in the driver and those values
will be also parsed by core apis but converting them to linux io
ports. Should this be done by the driver or is there a better way to
abstract this to don't do things twice?
2) 'set_io_port_base()' function does what we want but it is only
mips. We already have the iocu stuff there and the driver is mips
anyway, but it is worth to comment this just in case.

Thoughts?

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

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