[PATCH v2 0/3] staging: mt7621-pci: define ralink PCI_IOBASE to avoid manually ranges parsing

[PATCH v2 0/3] staging: mt7621-pci: define ralink PCI_IOBASE to avoid manually ranges parsing

[Sergio Paracuellos]

Ralink MIPS platforms do not define PCI_IOBASE. This ends up in
pci generic apis not working with io resources when calls to function
'of_pci_range_to_resource' are performed because internall function
'pci_address_to_pio()' is call and it results in getting 'OF_BAD_ADDR'
as result. If we define PCI_IOBASE pci generic apis properly works for
ralink pci controllers. In this particular case, we can remove all
manually ranges and resource from driver code decresing LOC and being
more standard.

In the future, this is also useful for mips pci drivers which are still
using pci legacy apis. After having PCI_IOBASE defined, only defining 
'pci_address_to_pio' for PCI_LEGACY might be remaining to also make 
work 'pci-rt3883', 'pci-mt7620' among others. Sadly I don't have devices
to test that so I haven't write the code by myself.

Thanks in advance for your time.

Changes in v2:
  - Fix PATCH 1 indentation converting spaces into tabs.

Best regards,
     Sergio Paracuellos

Sergio Paracuellos (3):
  MIPS: ralink: Define PCI_IOBASE
  staging: mt7621-pci: remove 'mt7621_pci_parse_request_of_pci_ranges'
  staging: mt7621-dts: fix pci address for PCI memory range

 arch/mips/include/asm/mach-ralink/spaces.h |  10 +++
 drivers/staging/mt7621-dts/mt7621.dtsi     |   2 +-
 drivers/staging/mt7621-pci/pci-mt7621.c    | 100 ++++++---------------
 3 files changed, 38 insertions(+), 74 deletions(-)
 create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h

-- 
2.25.1

[PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
required by generic PCI drivers to make memory mapped I/O range
work. Hence define it for ralink architectures to be able to
avoid parsing manually IO ranges in PCI generic driver code.
Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
call using '0xa0000000' as address, so use the same address in
the definition to align things.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h

diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
new file mode 100644
index 000000000000..87d085c9ad61
--- /dev/null
+++ b/arch/mips/include/asm/mach-ralink/spaces.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_MACH_RALINK_SPACES_H_
+#define __ASM_MACH_RALINK_SPACES_H_
+
+#define PCI_IOBASE	_AC(0xa0000000, UL)
+#define PCI_IOSIZE	SZ_16M
+#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
+
+#include <asm/mach-generic/spaces.h>
+#endif
-- 
2.25.1

[PATCH v2 2/3] staging: mt7621-pci: remove 'mt7621_pci_parse_request_of_pci_ranges'

[Sergio Paracuellos]

After 'PCI_IOBASE' is defined for ralink, ranges are properly parsed
using pci generic APIS and there is no need to parse anything
manually. So function 'mt7621_pci_parse_request_of_pci_ranges'
used for this can be enterely removed. Since we have to configure
iocu memory regions and pci io windows resources must be retrieved
accordly from 'bridge->windows' but there is no need to store
anything as driver private data.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 100 +++++++-----------------
 1 file changed, 27 insertions(+), 73 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index b0b5700cbfec..691030e1a5ed 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -86,10 +86,7 @@ struct mt7621_pcie_port {
 /**
  * struct mt7621_pcie - PCIe host information
  * @base: IO Mapped Register Base
- * @io: IO resource
- * @mem: pointer to non-prefetchable memory resource
  * @dev: Pointer to PCIe device
- * @io_map_base: virtual memory base address for io
  * @ports: pointer to PCIe port information
  * @resets_inverted: depends on chip revision
  * reset lines are inverted.
@@ -97,9 +94,6 @@ struct mt7621_pcie_port {
 struct mt7621_pcie {
 	void __iomem *base;
 	struct device *dev;
-	struct resource io;
-	struct resource *mem;
-	unsigned long io_map_base;
 	struct list_head ports;
 	bool resets_inverted;
 };
@@ -213,75 +207,33 @@ static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
 		reset_control_assert(port->pcie_rst);
 }
 
-static void setup_cm_memory_region(struct mt7621_pcie *pcie)
+static int setup_cm_memory_region(struct pci_host_bridge *host)
 {
-	struct resource *mem_resource = pcie->mem;
+	struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
 	struct device *dev = pcie->dev;
+	struct resource_entry *entry;
 	resource_size_t mask;
 
+	entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
+	if (!entry) {
+		dev_err(dev, "Cannot get memory resource\n");
+		return -EINVAL;
+	}
+
 	if (mips_cps_numiocu(0)) {
 		/*
 		 * FIXME: hardware doesn't accept mask values with 1s after
 		 * 0s (e.g. 0xffef), so it would be great to warn if that's
 		 * about to happen
 		 */
-		mask = ~(mem_resource->end - mem_resource->start);
+		mask = ~(entry->res->end - entry->res->start);
 
-		write_gcr_reg1_base(mem_resource->start);
+		write_gcr_reg1_base(entry->res->start);
 		write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
 		dev_info(dev, "PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
 			 (unsigned long long)read_gcr_reg1_base(),
 			 (unsigned long long)read_gcr_reg1_mask());
 	}
-}
-
-static int mt7621_pci_parse_request_of_pci_ranges(struct pci_host_bridge *host)
-{
-	struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
-	struct device *dev = pcie->dev;
-	struct device_node *node = dev->of_node;
-	struct of_pci_range_parser parser;
-	struct resource_entry *entry;
-	struct of_pci_range range;
-	LIST_HEAD(res);
-
-	if (of_pci_range_parser_init(&parser, node)) {
-		dev_err(dev, "missing \"ranges\" property\n");
-		return -EINVAL;
-	}
-
-	/*
-	 * IO_SPACE_LIMIT for MIPS is 0xffff but this platform uses IO at
-	 * upper address 0x001e160000. of_pci_range_to_resource does not work
-	 * well for MIPS platforms that don't define PCI_IOBASE, so set the IO
-	 * resource manually instead.
-	 */
-	for_each_of_pci_range(&parser, &range) {
-		switch (range.flags & IORESOURCE_TYPE_BITS) {
-		case IORESOURCE_IO:
-			pcie->io_map_base =
-				(unsigned long)ioremap(range.cpu_addr,
-						       range.size);
-			pcie->io.name = node->full_name;
-			pcie->io.flags = range.flags;
-			pcie->io.start = range.cpu_addr;
-			pcie->io.end = range.cpu_addr + range.size - 1;
-			pcie->io.parent = pcie->io.child = pcie->io.sibling = NULL;
-			set_io_port_base(pcie->io_map_base);
-			break;
-		}
-	}
-
-	entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
-	if (!entry) {
-		dev_err(dev, "Cannot get memory resource");
-		return -EINVAL;
-	}
-
-	pcie->mem = entry->res;
-	pci_add_resource(&res, &pcie->io);
-	pci_add_resource(&res, entry->res);
-	list_splice_init(&res, &host->windows);
 
 	return 0;
 }
@@ -510,15 +462,23 @@ static void mt7621_pcie_enable_port(struct mt7621_pcie_port *port)
 	write_config(pcie, slot, PCIE_FTS_NUM, val);
 }
 
-static int mt7621_pcie_enable_ports(struct mt7621_pcie *pcie)
+static int mt7621_pcie_enable_ports(struct pci_host_bridge *host)
 {
+	struct mt7621_pcie *pcie = pci_host_bridge_priv(host);
 	struct device *dev = pcie->dev;
 	struct mt7621_pcie_port *port;
+	struct resource_entry *entry;
 	int err;
 
+	entry = resource_list_first_type(&host->windows, IORESOURCE_IO);
+	if (!entry) {
+		dev_err(dev, "Cannot get io resource\n");
+		return -EINVAL;
+	}
+
 	/* Setup MEMWIN and IOWIN */
 	pcie_write(pcie, 0xffffffff, RALINK_PCI_MEMBASE);
-	pcie_write(pcie, pcie->io.start, RALINK_PCI_IOBASE);
+	pcie_write(pcie, entry->res->start, RALINK_PCI_IOBASE);
 
 	list_for_each_entry(port, &pcie->ports, list) {
 		if (port->enabled) {
@@ -581,25 +541,19 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = mt7621_pci_parse_request_of_pci_ranges(bridge);
-	if (err) {
-		dev_err(dev, "Error requesting pci resources from ranges");
-		goto remove_resets;
-	}
-
-	/* set resources limits */
-	ioport_resource.start = pcie->io.start;
-	ioport_resource.end = pcie->io.end;
-
 	mt7621_pcie_init_ports(pcie);
 
-	err = mt7621_pcie_enable_ports(pcie);
+	err = mt7621_pcie_enable_ports(bridge);
 	if (err) {
 		dev_err(dev, "Error enabling pcie ports\n");
 		goto remove_resets;
 	}
 
-	setup_cm_memory_region(pcie);
+	err = setup_cm_memory_region(bridge);
+	if (err) {
+		dev_err(dev, "Error setting up iocu mem regions\n");
+		goto remove_resets;
+	}
 
 	return mt7621_pcie_register_host(bridge);
 
-- 
2.25.1

[PATCH v2 3/3] staging: mt7621-dts: fix pci address for PCI memory range

[Sergio Paracuellos]

Driver code call 'devm_of_pci_get_host_bridge_resources'
to get resources and properly fill 'bridge->windows' and
'bridge->dma_ranges'. After parsing the ranges and store
as resources, at the end it makes a call to pci function
'pci_add_resource_offset' to set the offset for the
memory resource. To calculate offset, resource start address
subtracts pci address of the range. MT7621 does not need
any offset for the memory resource. Moreover, setting an
offset got into 'WARN_ON' calls from pci devices driver code.
Until now memory range pci_addr was being '0x00000000' and
res->start is '0x60000000' but becase pci controller driver
was manually setting resources and adding them using pci function
'pci_add_resource' where a zero is passed as offset, things
was properly working. Since PCI_IOBASE is defined now for
ralink we don't set nothing manually anymore so we have to
properly fix PCI address for this range to make things work
and the new pci address must be set to '0x60000000'. Doing
in this way the subtract result obtain zero as offset
and pci device driver code properly works.

Fixes: d59578da2bb8 ("staging: mt7621-dts: add dts files")
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index ecfe2f2cf75a..f15ea61851b2 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -489,7 +489,7 @@ pcie: pcie@1e140000 {
 
 		device_type = "pci";
 
-		ranges = <0x02000000 0 0x00000000 0x60000000 0 0x10000000>, /* pci memory */
+		ranges = <0x02000000 0 0x60000000 0x60000000 0 0x10000000>, /* pci memory */
 			 <0x01000000 0 0x00000000 0x1e160000 0 0x00010000>; /* io space */
 
 		#interrupt-cells = <1>;
-- 
2.25.1

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Greg KH]

On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> required by generic PCI drivers to make memory mapped I/O range
> work. Hence define it for ralink architectures to be able to
> avoid parsing manually IO ranges in PCI generic driver code.
> Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> call using '0xa0000000' as address, so use the same address in
> the definition to align things.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> 
> diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> new file mode 100644
> index 000000000000..87d085c9ad61
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_MACH_RALINK_SPACES_H_
> +#define __ASM_MACH_RALINK_SPACES_H_
> +
> +#define PCI_IOBASE	_AC(0xa0000000, UL)
> +#define PCI_IOSIZE	SZ_16M
> +#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
> +
> +#include <asm/mach-generic/spaces.h>
> +#endif
> -- 
> 2.25.1
> 
> 

I really can't take a change outside of drivers/staging/ for a staging
driver.

So unless you can get a MIPS maintainer to ACK this, I can't accept
this change, sorry.

greg k-h

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

On Tue, Jun 15, 2021 at 1:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > required by generic PCI drivers to make memory mapped I/O range
> > work. Hence define it for ralink architectures to be able to
> > avoid parsing manually IO ranges in PCI generic driver code.
> > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > call using '0xa0000000' as address, so use the same address in
> > the definition to align things.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> >
> > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > new file mode 100644
> > index 000000000000..87d085c9ad61
> > --- /dev/null
> > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > +#define __ASM_MACH_RALINK_SPACES_H_
> > +
> > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > +#define PCI_IOSIZE   SZ_16M
> > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > +
> > +#include <asm/mach-generic/spaces.h>
> > +#endif
> > --
> > 2.25.1
> >
> >
>
> I really can't take a change outside of drivers/staging/ for a staging
> driver.
>
> So unless you can get a MIPS maintainer to ACK this, I can't accept
> this change, sorry.

I know, I was expecting Thomas to ACK this, if it is ok. Thomas, what
do you think about this?

Best regards,
    Sergio Paracuellos

>
> greg k-h

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Thomas Bogendoerfer]

On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> required by generic PCI drivers to make memory mapped I/O range
> work. Hence define it for ralink architectures to be able to
> avoid parsing manually IO ranges in PCI generic driver code.
> Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> call using '0xa0000000' as address, so use the same address in
> the definition to align things.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> 
> diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> new file mode 100644
> index 000000000000..87d085c9ad61
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_MACH_RALINK_SPACES_H_
> +#define __ASM_MACH_RALINK_SPACES_H_
> +
> +#define PCI_IOBASE	_AC(0xa0000000, UL)
> +#define PCI_IOSIZE	SZ_16M
> +#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
> +
> +#include <asm/mach-generic/spaces.h>
> +#endif
> -- 
> 2.25.1

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

On Tue, Jun 15, 2021 at 3:09 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > required by generic PCI drivers to make memory mapped I/O range
> > work. Hence define it for ralink architectures to be able to
> > avoid parsing manually IO ranges in PCI generic driver code.
> > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > call using '0xa0000000' as address, so use the same address in
> > the definition to align things.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> >
> > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > new file mode 100644
> > index 000000000000..87d085c9ad61
> > --- /dev/null
> > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > +#define __ASM_MACH_RALINK_SPACES_H_
> > +
> > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > +#define PCI_IOSIZE   SZ_16M
> > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > +
> > +#include <asm/mach-generic/spaces.h>
> > +#endif
> > --
> > 2.25.1
>
> Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

Fastest response ever :) Thanks!

>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Greg KH]

On Tue, Jun 15, 2021 at 03:12:59PM +0200, Sergio Paracuellos wrote:
> On Tue, Jun 15, 2021 at 3:09 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > required by generic PCI drivers to make memory mapped I/O range
> > > work. Hence define it for ralink architectures to be able to
> > > avoid parsing manually IO ranges in PCI generic driver code.
> > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > call using '0xa0000000' as address, so use the same address in
> > > the definition to align things.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > ---
> > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > >
> > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > new file mode 100644
> > > index 000000000000..87d085c9ad61
> > > --- /dev/null
> > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > @@ -0,0 +1,10 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > +
> > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > +#define PCI_IOSIZE   SZ_16M
> > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > +
> > > +#include <asm/mach-generic/spaces.h>
> > > +#endif
> > > --
> > > 2.25.1
> >
> > Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> 
> Fastest response ever :) Thanks!

Thanks, all now queued up.

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Thomas Bogendoerfer]

On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> required by generic PCI drivers to make memory mapped I/O range
> work. Hence define it for ralink architectures to be able to
> avoid parsing manually IO ranges in PCI generic driver code.
> Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> call using '0xa0000000' as address, so use the same address in
> the definition to align things.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> 
> diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> new file mode 100644
> index 000000000000..87d085c9ad61
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_MACH_RALINK_SPACES_H_
> +#define __ASM_MACH_RALINK_SPACES_H_
> +
> +#define PCI_IOBASE	_AC(0xa0000000, UL)
> +#define PCI_IOSIZE	SZ_16M
> +#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
> +
> +#include <asm/mach-generic/spaces.h>
> +#endif

does this really work for you ? I tried the same trick for RB532
and the generated IO addresses are wrong...

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

Hi Thomas,

On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > required by generic PCI drivers to make memory mapped I/O range
> > work. Hence define it for ralink architectures to be able to
> > avoid parsing manually IO ranges in PCI generic driver code.
> > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > call using '0xa0000000' as address, so use the same address in
> > the definition to align things.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> >
> > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > new file mode 100644
> > index 000000000000..87d085c9ad61
> > --- /dev/null
> > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > +#define __ASM_MACH_RALINK_SPACES_H_
> > +
> > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > +#define PCI_IOSIZE   SZ_16M
> > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > +
> > +#include <asm/mach-generic/spaces.h>
> > +#endif
>
> does this really work for you ? I tried the same trick for RB532
> and the generated IO addresses are wrong...

I got pci io resources assigned without complaints from the pci core
code. I don't have real pci card that uses I/O bars but this is what I
see in the boot (I added some traces when I was testing this):

[   16.128328] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
[   16.141595] mt7621-pci 1e140000.pcie:   No bus range found for
/pcie@1e140000, using [bus 00-ff]
[   16.159047] mt7621-pci 1e140000.pcie:      MEM
0x0060000000..0x006fffffff -> 0x0060000000
[   16.189608] mt7621-pci 1e140000.pcie:       IO
0x001e160000..0x001e16ffff -> 0x0000000000
[   16.205847] pci_address_to_pio: addr: 0x1e160000
[   16.214992] LOGIC PIO: PIO TO CPUADDR: ADDR: 0x1e160000 -  addr
HW_START: 0x1e160000 + RANGE IO: 0x00000000
[   16.234344] OF: IO START returned by pci_address_to_pio: 0x0-0xffff
[   16.496670] mt7621-pci 1e140000.pcie: PCIE0 enabled
[   16.506230] mt7621-pci 1e140000.pcie: PCIE1 enabled
[   16.515930] mt7621-pci 1e140000.pcie: PCIE2 enabled
[   16.525604] mt7621-pci 1e140000.pcie: PCI coherence region base:
0x60000000, mask/settings: 0xf0000002
[   16.544284] mt7621-pci 1e140000.pcie: PCI host bridge to bus 0000:00
[   16.556823] pci_bus 0000:00: root bus resource [bus 00-ff]
[   16.567678] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
[   16.581321] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[   16.593658] pci 0000:00:00.0: [0e8d:0801] type 01 class 0x060400
[   16.605533] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x7fffffff]
[   16.617969] pci 0000:00:00.0: reg 0x14: [mem 0x00000000-0x0000ffff]
[   16.630490] pci 0000:00:00.0: supports D1
[   16.638344] pci 0000:00:00.0: PME# supported from D0 D1 D3hot
[   16.650599] pci 0000:00:01.0: [0e8d:0801] type 01 class 0x060400
[   16.662488] pci 0000:00:01.0: reg 0x10: [mem 0x00000000-0x7fffffff]
[   16.674885] pci 0000:00:01.0: reg 0x14: [mem 0x00000000-0x0000ffff]
[   16.687411] pci 0000:00:01.0: supports D1
[   16.695241] pci 0000:00:01.0: PME# supported from D0 D1 D3hot
[   16.707497] pci 0000:00:02.0: [0e8d:0801] type 01 class 0x060400
[   16.719387] pci 0000:00:02.0: reg 0x10: [mem 0x00000000-0x7fffffff]
[   16.731786] pci 0000:00:02.0: reg 0x14: [mem 0x00000000-0x0000ffff]
[   16.744301] pci 0000:00:02.0: supports D1
[   16.752171] pci 0000:00:02.0: PME# supported from D0 D1 D3hot
[   16.765367] pci 0000:00:00.0: bridge configuration invalid ([bus
00-00]), reconfiguring
[   16.781193] pci 0000:00:01.0: bridge configuration invalid ([bus
00-00]), reconfiguring
[   16.797068] pci 0000:00:02.0: bridge configuration invalid ([bus
00-00]), reconfiguring
[   16.813255] pci 0000:01:00.0: [1b21:0611] type 00 class 0x010185
[   16.825112] pci 0000:01:00.0: reg 0x10: [io  0x0000-0x0007]
[   16.836145] pci 0000:01:00.0: reg 0x14: [io  0x0000-0x0003]
[   16.847201] pci 0000:01:00.0: reg 0x18: [io  0x0000-0x0007]
[   16.858258] pci 0000:01:00.0: reg 0x1c: [io  0x0000-0x0003]
[   16.869325] pci 0000:01:00.0: reg 0x20: [io  0x0000-0x000f]
[   16.880373] pci 0000:01:00.0: reg 0x24: [mem 0x00000000-0x000001ff]
[   16.892975] pci 0000:01:00.0: 2.000 Gb/s available PCIe bandwidth,
limited by 2.5 GT/s PCIe x1 link at 0000:00:00.0 (capable of 4.000
Gb/s with 5.0 GT/s PCIe x1 link)
[   16.947735] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[   16.958035] pci 0000:00:00.0:   bridge window [io  0x0000-0x0fff]
[   16.970104] pci 0000:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
[   16.983577] pci 0000:00:00.0:   bridge window [mem
0x00000000-0x000fffff pref]
[   16.997928] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
[   17.011323] pci 0000:02:00.0: [1b21:0611] type 00 class 0x010185
[   17.023177] pci 0000:02:00.0: reg 0x10: [io  0x0000-0x0007]
[   17.034212] pci 0000:02:00.0: reg 0x14: [io  0x0000-0x0003]
[   17.045261] pci 0000:02:00.0: reg 0x18: [io  0x0000-0x0007]
[   17.056326] pci 0000:02:00.0: reg 0x1c: [io  0x0000-0x0003]
[   17.067405] pci 0000:02:00.0: reg 0x20: [io  0x0000-0x000f]
[   17.078442] pci 0000:02:00.0: reg 0x24: [mem 0x00000000-0x000001ff]
[   17.091042] pci 0000:02:00.0: 2.000 Gb/s available PCIe bandwidth,
limited by 2.5 GT/s PCIe x1 link at 0000:00:01.0 (capable of 4.000
Gb/s with 5.0 GT/s PCIe x1 link)
[   17.147757] pci 0000:00:01.0: PCI bridge to [bus 02-ff]
[   17.158059] pci 0000:00:01.0:   bridge window [io  0x0000-0x0fff]
[   17.170127] pci 0000:00:01.0:   bridge window [mem 0x00000000-0x000fffff]
[   17.183601] pci 0000:00:01.0:   bridge window [mem
0x00000000-0x000fffff pref]
[   17.197951] pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
[   17.211347] pci 0000:03:00.0: [1b21:0611] type 00 class 0x010185
[   17.223202] pci 0000:03:00.0: reg 0x10: [io  0x0000-0x0007]
[   17.234235] pci 0000:03:00.0: reg 0x14: [io  0x0000-0x0003]
[   17.245284] pci 0000:03:00.0: reg 0x18: [io  0x0000-0x0007]
[   17.256349] pci 0000:03:00.0: reg 0x1c: [io  0x0000-0x0003]
[   17.267418] pci 0000:03:00.0: reg 0x20: [io  0x0000-0x000f]
[   17.278463] pci 0000:03:00.0: reg 0x24: [mem 0x00000000-0x000001ff]
[   17.291064] pci 0000:03:00.0: 2.000 Gb/s available PCIe bandwidth,
limited by 2.5 GT/s PCIe x1 link at 0000:00:02.0 (capable of 4.000
Gb/s with 5.0 GT/s PCIe x1 link)
[   17.347738] pci 0000:00:02.0: PCI bridge to [bus 03-ff]
[   17.358037] pci 0000:00:02.0:   bridge window [io  0x0000-0x0fff]
[   17.370107] pci 0000:00:02.0:   bridge window [mem 0x00000000-0x000fffff]
[   17.383580] pci 0000:00:02.0:   bridge window [mem
0x00000000-0x000fffff pref]
[   17.397930] pci_bus 0000:03: busn_res: [bus 03-ff] end is updated to 03
[   17.411132] pci 0000:00:00.0: BAR 0: no space for [mem size 0x80000000]
[   17.424186] pci 0000:00:00.0: BAR 0: failed to assign [mem size 0x80000000]
[   17.438017] pci 0000:00:01.0: BAR 0: no space for [mem size 0x80000000]
[   17.451148] pci 0000:00:01.0: BAR 0: failed to assign [mem size 0x80000000]
[   17.464970] pci 0000:00:02.0: BAR 0: no space for [mem size 0x80000000]
[   17.478109] pci 0000:00:02.0: BAR 0: failed to assign [mem size 0x80000000]
[   17.491934] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
[   17.505407] pci 0000:00:00.0: BAR 9: assigned [mem
0x60100000-0x601fffff pref]
[   17.519752] pci 0000:00:01.0: BAR 8: assigned [mem 0x60200000-0x602fffff]
[   17.533233] pci 0000:00:01.0: BAR 9: assigned [mem
0x60300000-0x603fffff pref]
[   17.547583] pci 0000:00:02.0: BAR 8: assigned [mem 0x60400000-0x604fffff]
[   17.561049] pci 0000:00:02.0: BAR 9: assigned [mem
0x60500000-0x605fffff pref]
[   17.575396] pci 0000:00:00.0: BAR 1: assigned [mem 0x60600000-0x6060ffff]
[   17.588880] pci 0000:00:01.0: BAR 1: assigned [mem 0x60610000-0x6061ffff]
[   17.602354] pci 0000:00:02.0: BAR 1: assigned [mem 0x60620000-0x6062ffff]
[   17.615840] pci 0000:00:00.0: BAR 7: assigned [io  0x0000-0x0fff]
[   17.627927] pci 0000:00:01.0: BAR 7: assigned [io  0x1000-0x1fff]
[   17.640017] pci 0000:00:02.0: BAR 7: assigned [io  0x2000-0x2fff]
[   17.652131] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
[   17.665595] pci 0000:01:00.0: BAR 4: assigned [io  0x0000-0x000f]
[   17.677700] pci 0000:01:00.0: BAR 0: assigned [io  0x0010-0x0017]
[   17.689792] pci 0000:01:00.0: BAR 2: assigned [io  0x0018-0x001f]
[   17.701883] pci 0000:01:00.0: BAR 1: assigned [io  0x0020-0x0023]
[   17.713985] pci 0000:01:00.0: BAR 3: assigned [io  0x0024-0x0027]
[   17.726080] pci 0000:00:00.0: PCI bridge to [bus 01]
[   17.735924] pci 0000:00:00.0:   bridge window [io  0x0000-0x0fff]
[   17.748029] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
[   17.761494] pci 0000:00:00.0:   bridge window [mem
0x60100000-0x601fffff pref]
[   17.775852] pci 0000:02:00.0: BAR 5: assigned [mem 0x60200000-0x602001ff]
[   17.789326] pci 0000:02:00.0: BAR 4: assigned [io  0x1000-0x100f]
[   17.801416] pci 0000:02:00.0: BAR 0: assigned [io  0x1010-0x1017]
[   17.813525] pci 0000:02:00.0: BAR 2: assigned [io  0x1018-0x101f]
[   17.825608] pci 0000:02:00.0: BAR 1: assigned [io  0x1020-0x1023]
[   17.837711] pci 0000:02:00.0: BAR 3: assigned [io  0x1024-0x1027]
[   17.849804] pci 0000:00:01.0: PCI bridge to [bus 02]
[   17.859642] pci 0000:00:01.0:   bridge window [io  0x1000-0x1fff]
[   17.871752] pci 0000:00:01.0:   bridge window [mem 0x60200000-0x602fffff]
[   17.885217] pci 0000:00:01.0:   bridge window [mem
0x60300000-0x603fffff pref]
[   17.899573] pci 0000:03:00.0: BAR 5: assigned [mem 0x60400000-0x604001ff]
[   17.913051] pci 0000:03:00.0: BAR 4: assigned [io  0x2000-0x200f]
[   17.925141] pci 0000:03:00.0: BAR 0: assigned [io  0x2010-0x2017]
[   17.937246] pci 0000:03:00.0: BAR 2: assigned [io  0x2018-0x201f]
[   17.949339] pci 0000:03:00.0: BAR 1: assigned [io  0x2020-0x2023]
[   17.961433] pci 0000:03:00.0: BAR 3: assigned [io  0x2024-0x2027]
[   17.973529] pci 0000:00:02.0: PCI bridge to [bus 03]
[   17.983367] pci 0000:00:02.0:   bridge window [io  0x2000-0x2fff]
[   17.995470] pci 0000:00:02.0:   bridge window [mem 0x60400000-0x604fffff]
[   18.008956] pci 0000:00:02.0:   bridge window [mem
0x60500000-0x605fffff pref]

So the returned address range is the one that is finally being assigned.

Is this wrong?

Best regards,
    Sergio Paracuellos

>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Thomas Bogendoerfer]

On Thu, Jul 29, 2021 at 01:21:45PM +0200, Sergio Paracuellos wrote:
> Hi Thomas,
> 
> On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > required by generic PCI drivers to make memory mapped I/O range
> > > work. Hence define it for ralink architectures to be able to
> > > avoid parsing manually IO ranges in PCI generic driver code.
> > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > call using '0xa0000000' as address, so use the same address in
> > > the definition to align things.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > ---
> > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > >
> > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > new file mode 100644
> > > index 000000000000..87d085c9ad61
> > > --- /dev/null
> > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > @@ -0,0 +1,10 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > +
> > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > +#define PCI_IOSIZE   SZ_16M
> > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > +
> > > +#include <asm/mach-generic/spaces.h>
> > > +#endif
> >
> > does this really work for you ? I tried the same trick for RB532
> > and the generated IO addresses are wrong...
> 
> I got pci io resources assigned without complaints from the pci core
> code. I don't have real pci card that uses I/O bars but this is what I
> see in the boot (I added some traces when I was testing this):

resource handling works, but the addresses generated for IO access
are wrong, because the iomap tries to ioremap it to a fixed
virtual address (PCI_IOBASE), which can't work for KSEG1 addresses.

> Is this wrong?

to get it working this way, we would need to put PCI_IOBASE somewhere
into KSEG2, which I don't like since it will create TLB entries for IO
addresses, which most of the time isn't needed on MIPS because of
access via KSEG1.

I'd much prefer to make the devm_pci_remap_iospace() in drivers/pci/of.c
optional. Something like this

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index a143b02b2dcd..657aef39bf63 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -564,12 +564,14 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
 
                switch (resource_type(res)) {
                case IORESOURCE_IO:
+#ifdef PCI_IOBASE
                        err = devm_pci_remap_iospace(dev, res, iobase);
                        if (err) {
                                dev_warn(dev, "error %d: failed to map resource %pR\n",
                                         err, res);
                                resource_list_destroy_entry(win);
                        }
+#endif
                        break;
                case IORESOURCE_MEM:
                        res_valid |= !(res->flags & IORESOURCE_PREFETCH);


This together with an increased IO space via

#define IO_SPACE_LIMIT 0x1fffffff

gives me a working PCI bus on the RB532.

No idea, if the patch would be accepted by the PCI maintainers.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

Hi Thomas,

Thanks for the explanation.

On Fri, Jul 30, 2021 at 10:30 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Thu, Jul 29, 2021 at 01:21:45PM +0200, Sergio Paracuellos wrote:
> > Hi Thomas,
> >
> > On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > > required by generic PCI drivers to make memory mapped I/O range
> > > > work. Hence define it for ralink architectures to be able to
> > > > avoid parsing manually IO ranges in PCI generic driver code.
> > > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > > call using '0xa0000000' as address, so use the same address in
> > > > the definition to align things.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > ---
> > > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > > >
> > > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > new file mode 100644
> > > > index 000000000000..87d085c9ad61
> > > > --- /dev/null
> > > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > @@ -0,0 +1,10 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > > +
> > > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > > +#define PCI_IOSIZE   SZ_16M
> > > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > > +
> > > > +#include <asm/mach-generic/spaces.h>
> > > > +#endif
> > >
> > > does this really work for you ? I tried the same trick for RB532
> > > and the generated IO addresses are wrong...
> >
> > I got pci io resources assigned without complaints from the pci core
> > code. I don't have real pci card that uses I/O bars but this is what I
> > see in the boot (I added some traces when I was testing this):
>
> resource handling works, but the addresses generated for IO access
> are wrong, because the iomap tries to ioremap it to a fixed
> virtual address (PCI_IOBASE), which can't work for KSEG1 addresses.

What I don't understand is why the kernel is not complaining about
remapping this if "devm_pci_remap_iospace" can't work. I cannot see
any complaints in my dmesg log.

>
> > Is this wrong?
>
> to get it working this way, we would need to put PCI_IOBASE somewhere
> into KSEG2, which I don't like since it will create TLB entries for IO
> addresses, which most of the time isn't needed on MIPS because of
> access via KSEG1.

If I am understanding you correctly, you mean to change PCI_IOBASE
location to be somewhere in KSEG2 and set up the new io base address
through "set_io_port_base", right?
TLB entries should be out of this. Yes, I don't like this either.

> I'd much prefer to make the devm_pci_remap_iospace() in drivers/pci/of.c
> optional. Something like this
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index a143b02b2dcd..657aef39bf63 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -564,12 +564,14 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
>
>                 switch (resource_type(res)) {
>                 case IORESOURCE_IO:
> +#ifdef PCI_IOBASE
>                         err = devm_pci_remap_iospace(dev, res, iobase);
>                         if (err) {
>                                 dev_warn(dev, "error %d: failed to map resource %pR\n",
>                                          err, res);
>                                 resource_list_destroy_entry(win);
>                         }
> +#endif
>                         break;
>                 case IORESOURCE_MEM:
>                         res_valid |= !(res->flags & IORESOURCE_PREFETCH);
>

I don't know which architectures are not currently defining PCI_IOBASE
and might need this to be called (or if this is also not possible at
all scenario), honestly.

>
> This together with an increased IO space via
>
> #define IO_SPACE_LIMIT 0x1fffffff

I guess we can also redefine the weak "pci_address_to_pio" symbol for
mips to avoid this change, but only to do a "return (unsigned long)
address;" it might make no sense also :).

>
> gives me a working PCI bus on the RB532.
>
> No idea, if the patch would be accepted by the PCI maintainers.
>

Let's ask about this, just to be sure...

+cc Bjorn Helgaas and Lorenzo Pieralisi.

Bjorn, Lorenzo. Sorry to bother you. What do you think about this?
What should be the correct approach to handle this? Does the
"pci_parse_request_of_pci_ranges" change sound reasonable for you?

Thanks in advance for your time and clarification.

Best regards,
    Sergio Paracuellos

> Thomas.

>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

Hi Thomas,

On Fri, Jul 30, 2021 at 10:30 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Thu, Jul 29, 2021 at 01:21:45PM +0200, Sergio Paracuellos wrote:
> > Hi Thomas,
> >
> > On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > > required by generic PCI drivers to make memory mapped I/O range
> > > > work. Hence define it for ralink architectures to be able to
> > > > avoid parsing manually IO ranges in PCI generic driver code.
> > > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > > call using '0xa0000000' as address, so use the same address in
> > > > the definition to align things.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > ---
> > > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > > >
> > > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > new file mode 100644
> > > > index 000000000000..87d085c9ad61
> > > > --- /dev/null
> > > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > @@ -0,0 +1,10 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > > +
> > > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > > +#define PCI_IOSIZE   SZ_16M
> > > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > > +
> > > > +#include <asm/mach-generic/spaces.h>
> > > > +#endif
> > >
> > > does this really work for you ? I tried the same trick for RB532
> > > and the generated IO addresses are wrong...
> >
> > I got pci io resources assigned without complaints from the pci core
> > code. I don't have real pci card that uses I/O bars but this is what I
> > see in the boot (I added some traces when I was testing this):
>
> resource handling works, but the addresses generated for IO access
> are wrong, because the iomap tries to ioremap it to a fixed
> virtual address (PCI_IOBASE), which can't work for KSEG1 addresses.
>
> > Is this wrong?
>
> to get it working this way, we would need to put PCI_IOBASE somewhere
> into KSEG2, which I don't like since it will create TLB entries for IO
> addresses, which most of the time isn't needed on MIPS because of
> access via KSEG1.
>
> I'd much prefer to make the devm_pci_remap_iospace() in drivers/pci/of.c
> optional. Something like this
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index a143b02b2dcd..657aef39bf63 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -564,12 +564,14 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
>
>                 switch (resource_type(res)) {
>                 case IORESOURCE_IO:
> +#ifdef PCI_IOBASE
>                         err = devm_pci_remap_iospace(dev, res, iobase);
>                         if (err) {
>                                 dev_warn(dev, "error %d: failed to map resource %pR\n",
>                                          err, res);
>                                 resource_list_destroy_entry(win);
>                         }
> +#endif
>                         break;
>                 case IORESOURCE_MEM:
>                         res_valid |= !(res->flags & IORESOURCE_PREFETCH);
>
>
> This together with an increased IO space via
>
> #define IO_SPACE_LIMIT 0x1fffffff
>
> gives me a working PCI bus on the RB532.

BTW, I have tested your changes and they result in a no working pci
for mt7621. I get a resource collision error:

mt7621-pci 1e140000.pcie: resource collision: [io
0x1e160000-0x1e16ffff] conflicts with PCI IO [io  0x0000-0xffff]

My changes:
 - avoid PCI_IOBASE to be defined.
 - avoid map in pci_parse_request_of_pci_ranges
 - Change spaces.h to have the new IO_SPACE_LIMIT to 0x1fffffff

Am I missing something?

Thanks,
   Sergio Paracuellos
>
> No idea, if the patch would be accepted by the PCI maintainers.
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

On Fri, Jul 30, 2021 at 12:22 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Thomas,
>
> On Fri, Jul 30, 2021 at 10:30 AM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Thu, Jul 29, 2021 at 01:21:45PM +0200, Sergio Paracuellos wrote:
> > > Hi Thomas,
> > >
> > > On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
> > > <tsbogend@alpha.franken.de> wrote:
> > > >
> > > > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > > > required by generic PCI drivers to make memory mapped I/O range
> > > > > work. Hence define it for ralink architectures to be able to
> > > > > avoid parsing manually IO ranges in PCI generic driver code.
> > > > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > > > call using '0xa0000000' as address, so use the same address in
> > > > > the definition to align things.
> > > > >
> > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > ---
> > > > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > > > >
> > > > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > > new file mode 100644
> > > > > index 000000000000..87d085c9ad61
> > > > > --- /dev/null
> > > > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > > @@ -0,0 +1,10 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > > > +
> > > > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > > > +#define PCI_IOSIZE   SZ_16M
> > > > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > > > +
> > > > > +#include <asm/mach-generic/spaces.h>
> > > > > +#endif
> > > >
> > > > does this really work for you ? I tried the same trick for RB532
> > > > and the generated IO addresses are wrong...
> > >
> > > I got pci io resources assigned without complaints from the pci core
> > > code. I don't have real pci card that uses I/O bars but this is what I
> > > see in the boot (I added some traces when I was testing this):
> >
> > resource handling works, but the addresses generated for IO access
> > are wrong, because the iomap tries to ioremap it to a fixed
> > virtual address (PCI_IOBASE), which can't work for KSEG1 addresses.
> >
> > > Is this wrong?
> >
> > to get it working this way, we would need to put PCI_IOBASE somewhere
> > into KSEG2, which I don't like since it will create TLB entries for IO
> > addresses, which most of the time isn't needed on MIPS because of
> > access via KSEG1.
> >
> > I'd much prefer to make the devm_pci_remap_iospace() in drivers/pci/of.c
> > optional. Something like this
> >
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index a143b02b2dcd..657aef39bf63 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -564,12 +564,14 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
> >
> >                 switch (resource_type(res)) {
> >                 case IORESOURCE_IO:
> > +#ifdef PCI_IOBASE
> >                         err = devm_pci_remap_iospace(dev, res, iobase);
> >                         if (err) {
> >                                 dev_warn(dev, "error %d: failed to map resource %pR\n",
> >                                          err, res);
> >                                 resource_list_destroy_entry(win);
> >                         }
> > +#endif
> >                         break;
> >                 case IORESOURCE_MEM:
> >                         res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> >
> >
> > This together with an increased IO space via
> >
> > #define IO_SPACE_LIMIT 0x1fffffff
> >
> > gives me a working PCI bus on the RB532.
>
> BTW, I have tested your changes and they result in a no working pci
> for mt7621. I get a resource collision error:
>
> mt7621-pci 1e140000.pcie: resource collision: [io
> 0x1e160000-0x1e16ffff] conflicts with PCI IO [io  0x0000-0xffff]
>
> My changes:
>  - avoid PCI_IOBASE to be defined.
>  - avoid map in pci_parse_request_of_pci_ranges
>  - Change spaces.h to have the new IO_SPACE_LIMIT to 0x1fffffff
>
> Am I missing something?

I am forced to set ioport_resource stuff at the beginning of the probe
function to avoid this collision error:

/* set resources limits */
ioport_resource.start = 0x1e160000;
ioport_resource.end = 0x1e16ffff;

Taking into account that we expect pci core code to properly parse and
get the ranges and so on, if we are forced to do something like this
(or reading manually ranges for device tree) in driver code looks
redundant for me...

That said, I get the following pci working platform with your changes
and this initial ioport_resource set up:

[   16.129859] mt7621-pci 1e140000.pcie: host bridge /pcie@1e140000 ranges:
[   16.143130] mt7621-pci 1e140000.pcie:   No bus range found for
/pcie@1e140000, using [bus 00-ff]
[   16.160568] mt7621-pci 1e140000.pcie:      MEM
0x0060000000..0x006fffffff -> 0x0060000000
[   16.176783] OF: IO START returned by pci_address_to_pio:
0x60000000-0x6fffffff
[   16.191133] mt7621-pci 1e140000.pcie:       IO
0x001e160000..0x001e16ffff -> 0x0000000000
[   16.207358] OF: IO START returned by pci_address_to_pio:
0x1e160000-0x1e16ffff
[   16.462156] mt7621-pci 1e140000.pcie: PCIE0 enabled
[   16.471713] mt7621-pci 1e140000.pcie: PCIE1 enabled
[   16.481422] mt7621-pci 1e140000.pcie: PCIE2 enabled
[   16.491095] mt7621-pci 1e140000.pcie: PCI coherence region base:
0x60000000, mask/settings: 0xf0000002
[   16.509802] mt7621-pci 1e140000.pcie: PCI host bridge to bus 0000:00
[   16.522365] pci_bus 0000:00: root bus resource [bus 00-ff]
[   16.533204] pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]
[   16.546855] pci_bus 0000:00: root bus resource [io
0x1e160000-0x1e16ffff] (bus address [0x0000-0xffff])
[   16.565751] pci 0000:00:00.0: [0e8d:0801] type 01 class 0x060400
[   16.577629] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x7fffffff]
[   16.590063] pci 0000:00:00.0: reg 0x14: [mem 0x00000000-0x0000ffff]
[   16.602580] pci 0000:00:00.0: supports D1
[   16.610413] pci 0000:00:00.0: PME# supported from D0 D1 D3hot
[   16.622887] pci 0000:00:01.0: [0e8d:0801] type 01 class 0x060400
[   16.634799] pci 0000:00:01.0: reg 0x10: [mem 0x00000000-0x7fffffff]
[   16.647186] pci 0000:00:01.0: reg 0x14: [mem 0x00000000-0x0000ffff]
[   16.659697] pci 0000:00:01.0: supports D1
[   16.667559] pci 0000:00:01.0: PME# supported from D0 D1 D3hot
[   16.679943] pci 0000:00:02.0: [0e8d:0801] type 01 class 0x060400
[   16.691851] pci 0000:00:02.0: reg 0x10: [mem 0x00000000-0x7fffffff]
[   16.704240] pci 0000:00:02.0: reg 0x14: [mem 0x00000000-0x0000ffff]
[   16.716751] pci 0000:00:02.0: supports D1
[   16.724610] pci 0000:00:02.0: PME# supported from D0 D1 D3hot
[   16.737915] pci 0000:00:00.0: bridge configuration invalid ([bus
00-00]), reconfiguring
[   16.753756] pci 0000:00:01.0: bridge configuration invalid ([bus
00-00]), reconfiguring
[   16.769631] pci 0000:00:02.0: bridge configuration invalid ([bus
00-00]), reconfiguring
[   16.785811] pci 0000:01:00.0: [1b21:0611] type 00 class 0x010185
[   16.797692] pci 0000:01:00.0: reg 0x10: [io  0x1e160000-0x1e160007]
[   16.810082] pci 0000:01:00.0: reg 0x14: [io  0x1e160000-0x1e160003]
[   16.822533] pci 0000:01:00.0: reg 0x18: [io  0x1e160000-0x1e160007]
[   16.834965] pci 0000:01:00.0: reg 0x1c: [io  0x1e160000-0x1e160003]
[   16.847405] pci 0000:01:00.0: reg 0x20: [io  0x1e160000-0x1e16000f]
[   16.859847] pci 0000:01:00.0: reg 0x24: [mem 0x00000000-0x000001ff]
[   16.872446] pci 0000:01:00.0: 2.000 Gb/s available PCIe bandwidth,
limited by 2.5 GT/s PCIe x1 link at 0000:00:00.0 (capable of 4.000
Gb/s with 5.0 GT/s PCIe x1 link)
[   16.933210] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[   16.943503] pci 0000:00:00.0:   bridge window [io  0x1e160000-0x1e160fff]
[   16.956963] pci 0000:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
[   16.970442] pci 0000:00:00.0:   bridge window [mem
0x00000000-0x000fffff pref]
[   16.984789] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
[   16.998209] pci 0000:02:00.0: [1b21:0611] type 00 class 0x010185
[   17.010096] pci 0000:02:00.0: reg 0x10: [io  0x1e160000-0x1e160007]
[   17.022483] pci 0000:02:00.0: reg 0x14: [io  0x1e160000-0x1e160003]
[   17.034923] pci 0000:02:00.0: reg 0x18: [io  0x1e160000-0x1e160007]
[   17.047364] pci 0000:02:00.0: reg 0x1c: [io  0x1e160000-0x1e160003]
[   17.059814] pci 0000:02:00.0: reg 0x20: [io  0x1e160000-0x1e16000f]
[   17.072246] pci 0000:02:00.0: reg 0x24: [mem 0x00000000-0x000001ff]
[   17.084844] pci 0000:02:00.0: 2.000 Gb/s available PCIe bandwidth,
limited by 2.5 GT/s PCIe x1 link at 0000:00:01.0 (capable of 4.000
Gb/s with 5.0 GT/s PCIe x1 link)
[   17.143234] pci 0000:00:01.0: PCI bridge to [bus 02-ff]
[   17.153527] pci 0000:00:01.0:   bridge window [io  0x1e160000-0x1e160fff]
[   17.166987] pci 0000:00:01.0:   bridge window [mem 0x00000000-0x000fffff]
[   17.180466] pci 0000:00:01.0:   bridge window [mem
0x00000000-0x000fffff pref]
[   17.194813] pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
[   17.208231] pci 0000:03:00.0: [1b21:0611] type 00 class 0x010185
[   17.220107] pci 0000:03:00.0: reg 0x10: [io  0x1e160000-0x1e160007]
[   17.232524] pci 0000:03:00.0: reg 0x14: [io  0x1e160000-0x1e160003]
[   17.244947] pci 0000:03:00.0: reg 0x18: [io  0x1e160000-0x1e160007]
[   17.257386] pci 0000:03:00.0: reg 0x1c: [io  0x1e160000-0x1e160003]
[   17.269839] pci 0000:03:00.0: reg 0x20: [io  0x1e160000-0x1e16000f]
[   17.282269] pci 0000:03:00.0: reg 0x24: [mem 0x00000000-0x000001ff]
[   17.294868] pci 0000:03:00.0: 2.000 Gb/s available PCIe bandwidth,
limited by 2.5 GT/s PCIe x1 link at 0000:00:02.0 (capable of 4.000
Gb/s with 5.0 GT/s PCIe x1 link)
[   17.353206] pci 0000:00:02.0: PCI bridge to [bus 03-ff]
[   17.363509] pci 0000:00:02.0:   bridge window [io  0x1e160000-0x1e160fff]
[   17.376962] pci 0000:00:02.0:   bridge window [mem 0x00000000-0x000fffff]
[   17.390438] pci 0000:00:02.0:   bridge window [mem
0x00000000-0x000fffff pref]
[   17.404785] pci_bus 0000:03: busn_res: [bus 03-ff] end is updated to 03
[   17.417985] pci 0000:00:00.0: BAR 0: no space for [mem size 0x80000000]
[   17.431044] pci 0000:00:00.0: BAR 0: failed to assign [mem size 0x80000000]
[   17.444872] pci 0000:00:01.0: BAR 0: no space for [mem size 0x80000000]
[   17.458000] pci 0000:00:01.0: BAR 0: failed to assign [mem size 0x80000000]
[   17.471827] pci 0000:00:02.0: BAR 0: no space for [mem size 0x80000000]
[   17.484964] pci 0000:00:02.0: BAR 0: failed to assign [mem size 0x80000000]
[   17.498787] pci 0000:00:00.0: BAR 8: assigned [mem 0x60000000-0x600fffff]
[   17.512264] pci 0000:00:00.0: BAR 9: assigned [mem
0x60100000-0x601fffff pref]
[   17.526607] pci 0000:00:01.0: BAR 8: assigned [mem 0x60200000-0x602fffff]
[   17.540085] pci 0000:00:01.0: BAR 9: assigned [mem
0x60300000-0x603fffff pref]
[   17.554434] pci 0000:00:02.0: BAR 8: assigned [mem 0x60400000-0x604fffff]
[   17.567907] pci 0000:00:02.0: BAR 9: assigned [mem
0x60500000-0x605fffff pref]
[   17.582248] pci 0000:00:00.0: BAR 1: assigned [mem 0x60600000-0x6060ffff]
[   17.595732] pci 0000:00:01.0: BAR 1: assigned [mem 0x60610000-0x6061ffff]
[   17.609212] pci 0000:00:02.0: BAR 1: assigned [mem 0x60620000-0x6062ffff]
[   17.622693] pci 0000:00:00.0: BAR 7: assigned [io  0x1e160000-0x1e160fff]
[   17.636162] pci 0000:00:01.0: BAR 7: assigned [io  0x1e161000-0x1e161fff]
[   17.649639] pci 0000:00:02.0: BAR 7: assigned [io  0x1e162000-0x1e162fff]
[   17.663130] pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
[   17.676602] pci 0000:01:00.0: BAR 4: assigned [io  0x1e160000-0x1e16000f]
[   17.690086] pci 0000:01:00.0: BAR 0: assigned [io  0x1e160010-0x1e160017]
[   17.703558] pci 0000:01:00.0: BAR 2: assigned [io  0x1e160018-0x1e16001f]
[   17.717036] pci 0000:01:00.0: BAR 1: assigned [io  0x1e160020-0x1e160023]
[   17.730519] pci 0000:01:00.0: BAR 3: assigned [io  0x1e160024-0x1e160027]
[   17.743999] pci 0000:00:00.0: PCI bridge to [bus 01]
[   17.753836] pci 0000:00:00.0:   bridge window [io  0x1e160000-0x1e160fff]
[   17.767314] pci 0000:00:00.0:   bridge window [mem 0x60000000-0x600fffff]
[   17.780793] pci 0000:00:00.0:   bridge window [mem
0x60100000-0x601fffff pref]
[   17.795145] pci 0000:02:00.0: BAR 5: assigned [mem 0x60200000-0x602001ff]
[   17.808628] pci 0000:02:00.0: BAR 4: assigned [io  0x1e161000-0x1e16100f]
[   17.822123] pci 0000:02:00.0: BAR 0: assigned [io  0x1e161010-0x1e161017]
[   17.835575] pci 0000:02:00.0: BAR 2: assigned [io  0x1e161018-0x1e16101f]
[   17.849060] pci 0000:02:00.0: BAR 1: assigned [io  0x1e161020-0x1e161023]
[   17.862541] pci 0000:02:00.0: BAR 3: assigned [io  0x1e161024-0x1e161027]
[   17.876010] pci 0000:00:01.0: PCI bridge to [bus 02]
[   17.885854] pci 0000:00:01.0:   bridge window [io  0x1e161000-0x1e161fff]
[   17.899333] pci 0000:00:01.0:   bridge window [mem 0x60200000-0x602fffff]
[   17.912810] pci 0000:00:01.0:   bridge window [mem
0x60300000-0x603fffff pref]
[   17.927171] pci 0000:03:00.0: BAR 5: assigned [mem 0x60400000-0x604001ff]
[   17.940639] pci 0000:03:00.0: BAR 4: assigned [io  0x1e162000-0x1e16200f]
[   17.954118] pci 0000:03:00.0: BAR 0: assigned [io  0x1e162010-0x1e162017]
[   17.967601] pci 0000:03:00.0: BAR 2: assigned [io  0x1e162018-0x1e16201f]
[   17.981074] pci 0000:03:00.0: BAR 1: assigned [io  0x1e162020-0x1e162023]
[   17.994560] pci 0000:03:00.0: BAR 3: assigned [io  0x1e162024-0x1e162027]
[   18.008030] pci 0000:00:02.0: PCI bridge to [bus 03]
[   18.017873] pci 0000:00:02.0:   bridge window [io  0x1e162000-0x1e162fff]
[   18.031352] pci 0000:00:02.0:   bridge window [mem 0x60400000-0x604fffff]
[   18.044830] pci 0000:00:02.0:   bridge window [mem
0x60500000-0x605fffff pref]


Best regards,
    Sergio Paracuellos

>
> Thanks,
>    Sergio Paracuellos
> >
> > No idea, if the patch would be accepted by the PCI maintainers.
> >
> > Thomas.
> >
> > --
> > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> > good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Thomas Bogendoerfer]

On Fri, Jul 30, 2021 at 01:15:36PM +0200, Sergio Paracuellos wrote:
> On Fri, Jul 30, 2021 at 12:22 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Thomas,
> >
> > On Fri, Jul 30, 2021 at 10:30 AM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Thu, Jul 29, 2021 at 01:21:45PM +0200, Sergio Paracuellos wrote:
> > > > Hi Thomas,
> > > >
> > > > On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
> > > > <tsbogend@alpha.franken.de> wrote:
> > > > >
> > > > > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > > > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > > > > required by generic PCI drivers to make memory mapped I/O range
> > > > > > work. Hence define it for ralink architectures to be able to
> > > > > > avoid parsing manually IO ranges in PCI generic driver code.
> > > > > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > > > > call using '0xa0000000' as address, so use the same address in
> > > > > > the definition to align things.
> > > > > >
> > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > ---
> > > > > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > > > > >
> > > > > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..87d085c9ad61
> > > > > > --- /dev/null
> > > > > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > > > @@ -0,0 +1,10 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > > > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > > > > +
> > > > > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > > > > +#define PCI_IOSIZE   SZ_16M
> > > > > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > > > > +
> > > > > > +#include <asm/mach-generic/spaces.h>
> > > > > > +#endif
> > > > >
> > > > > does this really work for you ? I tried the same trick for RB532
> > > > > and the generated IO addresses are wrong...
> > > >
> > > > I got pci io resources assigned without complaints from the pci core
> > > > code. I don't have real pci card that uses I/O bars but this is what I
> > > > see in the boot (I added some traces when I was testing this):
> > >
> > > resource handling works, but the addresses generated for IO access
> > > are wrong, because the iomap tries to ioremap it to a fixed
> > > virtual address (PCI_IOBASE), which can't work for KSEG1 addresses.
> > >
> > > > Is this wrong?
> > >
> > > to get it working this way, we would need to put PCI_IOBASE somewhere
> > > into KSEG2, which I don't like since it will create TLB entries for IO
> > > addresses, which most of the time isn't needed on MIPS because of
> > > access via KSEG1.
> > >
> > > I'd much prefer to make the devm_pci_remap_iospace() in drivers/pci/of.c
> > > optional. Something like this
> > >
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index a143b02b2dcd..657aef39bf63 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -564,12 +564,14 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
> > >
> > >                 switch (resource_type(res)) {
> > >                 case IORESOURCE_IO:
> > > +#ifdef PCI_IOBASE
> > >                         err = devm_pci_remap_iospace(dev, res, iobase);
> > >                         if (err) {
> > >                                 dev_warn(dev, "error %d: failed to map resource %pR\n",
> > >                                          err, res);
> > >                                 resource_list_destroy_entry(win);
> > >                         }
> > > +#endif
> > >                         break;
> > >                 case IORESOURCE_MEM:
> > >                         res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > >
> > >
> > > This together with an increased IO space via
> > >
> > > #define IO_SPACE_LIMIT 0x1fffffff
> > >
> > > gives me a working PCI bus on the RB532.
> >
> > BTW, I have tested your changes and they result in a no working pci
> > for mt7621. I get a resource collision error:
> >
> > mt7621-pci 1e140000.pcie: resource collision: [io
> > 0x1e160000-0x1e16ffff] conflicts with PCI IO [io  0x0000-0xffff]
> >
> > My changes:
> >  - avoid PCI_IOBASE to be defined.
> >  - avoid map in pci_parse_request_of_pci_ranges
> >  - Change spaces.h to have the new IO_SPACE_LIMIT to 0x1fffffff
> >
> > Am I missing something?
> 
> I am forced to set ioport_resource stuff at the beginning of the probe
> function to avoid this collision error:
> 
> /* set resources limits */
> ioport_resource.start = 0x1e160000;
> ioport_resource.end = 0x1e16ffff;

yes, that's then neeed, I'm using

        ioport_resource.start = 0;
        ioport_resource.end = ~0UL;

but 

ioport_resource.end = IO_SPACE_LIMIT;

would be more correct.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergio Paracuellos]

On Fri, Jul 30, 2021 at 2:39 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Fri, Jul 30, 2021 at 01:15:36PM +0200, Sergio Paracuellos wrote:
> > On Fri, Jul 30, 2021 at 12:22 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > Hi Thomas,
> > >
> > > On Fri, Jul 30, 2021 at 10:30 AM Thomas Bogendoerfer
> > > <tsbogend@alpha.franken.de> wrote:
> > > >
> > > > On Thu, Jul 29, 2021 at 01:21:45PM +0200, Sergio Paracuellos wrote:
> > > > > Hi Thomas,
> > > > >
> > > > > On Thu, Jul 29, 2021 at 12:02 PM Thomas Bogendoerfer
> > > > > <tsbogend@alpha.franken.de> wrote:
> > > > > >
> > > > > > On Mon, Jun 14, 2021 at 12:06:15PM +0200, Sergio Paracuellos wrote:
> > > > > > > PCI_IOBASE is used to create VM maps for PCI I/O ports, it is
> > > > > > > required by generic PCI drivers to make memory mapped I/O range
> > > > > > > work. Hence define it for ralink architectures to be able to
> > > > > > > avoid parsing manually IO ranges in PCI generic driver code.
> > > > > > > Function 'plat_mem_setup' for ralink is using 'set_io_port_base'
> > > > > > > call using '0xa0000000' as address, so use the same address in
> > > > > > > the definition to align things.
> > > > > > >
> > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > > ---
> > > > > > >  arch/mips/include/asm/mach-ralink/spaces.h | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > >  create mode 100644 arch/mips/include/asm/mach-ralink/spaces.h
> > > > > > >
> > > > > > > diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..87d085c9ad61
> > > > > > > --- /dev/null
> > > > > > > +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> > > > > > > @@ -0,0 +1,10 @@
> > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > > +#ifndef __ASM_MACH_RALINK_SPACES_H_
> > > > > > > +#define __ASM_MACH_RALINK_SPACES_H_
> > > > > > > +
> > > > > > > +#define PCI_IOBASE   _AC(0xa0000000, UL)
> > > > > > > +#define PCI_IOSIZE   SZ_16M
> > > > > > > +#define IO_SPACE_LIMIT       (PCI_IOSIZE - 1)
> > > > > > > +
> > > > > > > +#include <asm/mach-generic/spaces.h>
> > > > > > > +#endif
> > > > > >
> > > > > > does this really work for you ? I tried the same trick for RB532
> > > > > > and the generated IO addresses are wrong...
> > > > >
> > > > > I got pci io resources assigned without complaints from the pci core
> > > > > code. I don't have real pci card that uses I/O bars but this is what I
> > > > > see in the boot (I added some traces when I was testing this):
> > > >
> > > > resource handling works, but the addresses generated for IO access
> > > > are wrong, because the iomap tries to ioremap it to a fixed
> > > > virtual address (PCI_IOBASE), which can't work for KSEG1 addresses.
> > > >
> > > > > Is this wrong?
> > > >
> > > > to get it working this way, we would need to put PCI_IOBASE somewhere
> > > > into KSEG2, which I don't like since it will create TLB entries for IO
> > > > addresses, which most of the time isn't needed on MIPS because of
> > > > access via KSEG1.
> > > >
> > > > I'd much prefer to make the devm_pci_remap_iospace() in drivers/pci/of.c
> > > > optional. Something like this
> > > >
> > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > index a143b02b2dcd..657aef39bf63 100644
> > > > --- a/drivers/pci/of.c
> > > > +++ b/drivers/pci/of.c
> > > > @@ -564,12 +564,14 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
> > > >
> > > >                 switch (resource_type(res)) {
> > > >                 case IORESOURCE_IO:
> > > > +#ifdef PCI_IOBASE
> > > >                         err = devm_pci_remap_iospace(dev, res, iobase);
> > > >                         if (err) {
> > > >                                 dev_warn(dev, "error %d: failed to map resource %pR\n",
> > > >                                          err, res);
> > > >                                 resource_list_destroy_entry(win);
> > > >                         }
> > > > +#endif
> > > >                         break;
> > > >                 case IORESOURCE_MEM:
> > > >                         res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > > >
> > > >
> > > > This together with an increased IO space via
> > > >
> > > > #define IO_SPACE_LIMIT 0x1fffffff
> > > >
> > > > gives me a working PCI bus on the RB532.
> > >
> > > BTW, I have tested your changes and they result in a no working pci
> > > for mt7621. I get a resource collision error:
> > >
> > > mt7621-pci 1e140000.pcie: resource collision: [io
> > > 0x1e160000-0x1e16ffff] conflicts with PCI IO [io  0x0000-0xffff]
> > >
> > > My changes:
> > >  - avoid PCI_IOBASE to be defined.
> > >  - avoid map in pci_parse_request_of_pci_ranges
> > >  - Change spaces.h to have the new IO_SPACE_LIMIT to 0x1fffffff
> > >
> > > Am I missing something?
> >
> > I am forced to set ioport_resource stuff at the beginning of the probe
> > function to avoid this collision error:
> >
> > /* set resources limits */
> > ioport_resource.start = 0x1e160000;
> > ioport_resource.end = 0x1e16ffff;
>
> yes, that's then neeed, I'm using
>
>         ioport_resource.start = 0;
>         ioport_resource.end = ~0UL;
>
> but
>
> ioport_resource.end = IO_SPACE_LIMIT;
>
> would be more correct.

Makes sense. I guess we can add this assignment in the main ralink
code platform to avoid defining this for every single pci driver.

>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]