On Fri, Sep 24, 2021 at 6:45 PM Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> wrote: > > 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]. 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
