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

From: Arnd Bergmann
Date: Fri Sep 24 2021 - 13:04:43 EST

On Fri, Sep 24, 2021 at 6:45 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
> 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
> >
> > 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 */
> #define PCI_IOBASE mips_io_port_base
> #define PCI_IOSIZE SZ_16M

As a minor comment, I would make the PCI_IOSIZE only 64KB in this
case, unless plan to support ralink/mediatek SoCs that have a multiple
PCIe domains with distinct 64KB windows.

> 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) {
> vaddr = (unsigned long)ioremap(range.cpu_addr, range.size);
> set_io_port_base(vaddr);
> ret = 0;
> break;
> }
> }
> return ret;
> }

This looks like it does the right thing, but conceptually this would belong into
the mips specific pci_remap_iospace() as we discussed a few emails back,
not inside the driver. pci_remap_iospace() does get the CPU address
as an argument, so it just needs to ioremap()/set_io_port_base() it.

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

I think in both cases the core APIs should do what we need, with the
change to the mips pci_remap_iospace() I mention. If there is anything
missing in the core API that you need, we can discuss extending those,
e.g. to store additional data in the pci_host_bridge structure.