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

[Sergio Paracuellos <sergio.paracuellos@gmail.com>]

On Fri, Sep 24, 2021 at 6:45 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Arnd,
>
> On Fri, Sep 24, 2021 at 3:28 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Fri, Sep 24, 2021 at 2:46 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Fri, Sep 24, 2021 at 1:39 PM Arnd Bergmann <arnd@arndb.de> 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].

Sorry, forgot to add links:

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=159697474db41732ef3b6c2e8d9395f09d1f659e
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=50fb34eca2944fd67493717c9fbda125336f1655

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