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

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

 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 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..ec58d4a9ed05
--- /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 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 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 1/3] MIPS: ralink: Define PCI_IOBASE

[Sergei Shtylyov]

Hello!

On 13.06.2021 18:56, 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..ec58d4a9ed05
> --- /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)

    Why this sudden switch to spaces for indentation? Previous lines were 
(correctly) indented with tabs...

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

MBR, Sergei

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

[Sergio Paracuellos]

Hi Sergei,

On Mon, Jun 14, 2021 at 11:13 AM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
>
> Hello!
>
> On 13.06.2021 18:56, 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..ec58d4a9ed05
> > --- /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)
>
>     Why this sudden switch to spaces for indentation? Previous lines were
> (correctly) indented with tabs...

I don't really know what could have happened, honestly. I will fix
spaces into tabs and send v2.

>
> > +
> > +#include <asm/mach-generic/spaces.h>
> > +#endif
>
> MBR, Sergei

Best regards,
    Sergio Paracuellos