All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2 0/4] PCI: dwc: add prefetchable memory range support
@ 2018-11-07 10:08 Z.q. Hou
  2018-11-07 10:09 ` [PATCHv2 1/4] PCI: dwc: fix potential memory leak Z.q. Hou
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Z.q. Hou @ 2018-11-07 10:08 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, lorenzo.pieralisi, jingoohan1,
	gustavo.pimentel
  Cc: Roy Zang, Mingkai Hu, M.h. Lian, Z.q. Hou

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

This patch set is to add prefetchable memory range support, patch 4/4.
Patch 3/4 is to initialize the number of viewport for layerscape PCIe.
BTW, fix 2 bugs, see patch 1/4 and 2/4.

Hou Zhiqiang (4):
  PCI: dwc: fix potential memory leak
  PCI: dwc: fix 4GiB outbound window size truncated to zero issue
  PCI: layerscape: initialize the number of viewport
  PCI: dwc: add prefetchable memory range support

 drivers/pci/controller/dwc/pci-layerscape.c   |   3 +
 .../pci/controller/dwc/pcie-designware-host.c | 110 ++++++++++++++----
 drivers/pci/controller/dwc/pcie-designware.c  |   4 +-
 drivers/pci/controller/dwc/pcie-designware.h  |  11 +-
 4 files changed, 104 insertions(+), 24 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCHv2 1/4] PCI: dwc: fix potential memory leak
  2018-11-07 10:08 [PATCHv2 0/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
@ 2018-11-07 10:09 ` Z.q. Hou
  2018-12-05 15:40   ` Lorenzo Pieralisi
  2018-11-07 10:09 ` [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue Z.q. Hou
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Z.q. Hou @ 2018-11-07 10:09 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, lorenzo.pieralisi, jingoohan1,
	gustavo.pimentel
  Cc: Roy Zang, Mingkai Hu, M.h. Lian, Z.q. Hou

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Free the allocated pci_host_bridge struct when failed to get
host bridge resources, and free the resource windows before
free the bridge.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
V2:
 - Reworded the subject.

 drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 29a05759a294..ecacce016489 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -353,7 +353,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
 					&bridge->windows, &pp->io_base);
 	if (ret)
-		return ret;
+		goto error;
 
 	ret = devm_request_pci_bus_resources(dev, &bridge->windows);
 	if (ret)
@@ -502,6 +502,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	return 0;
 
 error:
+	pci_free_resource_list(&bridge->windows);
 	pci_free_host_bridge(bridge);
 	return ret;
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue
  2018-11-07 10:08 [PATCHv2 0/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
  2018-11-07 10:09 ` [PATCHv2 1/4] PCI: dwc: fix potential memory leak Z.q. Hou
@ 2018-11-07 10:09 ` Z.q. Hou
  2018-12-05 16:01   ` Lorenzo Pieralisi
  2018-11-07 10:09 ` [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport Z.q. Hou
  2018-11-07 10:09 ` [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
  3 siblings, 1 reply; 17+ messages in thread
From: Z.q. Hou @ 2018-11-07 10:09 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, lorenzo.pieralisi, jingoohan1,
	gustavo.pimentel
  Cc: Roy Zang, Mingkai Hu, M.h. Lian, Z.q. Hou

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

The current type of mem_size is 'u32', so when resource_size()
return 4G it will be truncated to zero. This patch fix it by
changing its type to 'u64'.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
V2:
 - Reworded the subject.

 drivers/pci/controller/dwc/pcie-designware.c | 4 ++--
 drivers/pci/controller/dwc/pcie-designware.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 2153956a0b20..7ac5989c23ef 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -106,7 +106,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
 
 static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
 					     int type, u64 cpu_addr,
-					     u64 pci_addr, u32 size)
+					     u64 pci_addr, u64 size)
 {
 	u32 retries, val;
 
@@ -141,7 +141,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
 }
 
 void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
-			       u64 cpu_addr, u64 pci_addr, u32 size)
+			       u64 cpu_addr, u64 pci_addr, u64 size)
 {
 	u32 retries, val;
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 9f1a5e399b70..a438c3879aa9 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -153,7 +153,7 @@ struct pcie_port {
 	u32			io_size;
 	u64			mem_base;
 	phys_addr_t		mem_bus_addr;
-	u32			mem_size;
+	u64			mem_size;
 	struct resource		*cfg;
 	struct resource		*io;
 	struct resource		*mem;
@@ -238,7 +238,7 @@ int dw_pcie_link_up(struct dw_pcie *pci);
 int dw_pcie_wait_for_link(struct dw_pcie *pci);
 void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
 			       int type, u64 cpu_addr, u64 pci_addr,
-			       u32 size);
+			       u64 size);
 int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
 			     u64 cpu_addr, enum dw_pcie_as_type as_type);
 void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport
  2018-11-07 10:08 [PATCHv2 0/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
  2018-11-07 10:09 ` [PATCHv2 1/4] PCI: dwc: fix potential memory leak Z.q. Hou
  2018-11-07 10:09 ` [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue Z.q. Hou
@ 2018-11-07 10:09 ` Z.q. Hou
  2018-11-22 11:16   ` Lorenzo Pieralisi
  2018-11-07 10:09 ` [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
  3 siblings, 1 reply; 17+ messages in thread
From: Z.q. Hou @ 2018-11-07 10:09 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, lorenzo.pieralisi, jingoohan1,
	gustavo.pimentel
  Cc: Roy Zang, Mingkai Hu, M.h. Lian, Z.q. Hou

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

FSL implements 6 viewports on Layerscape series SoCs PCIe
controllers.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
V2:
 - Reworded the subject and commit description.

 drivers/pci/controller/dwc/pci-layerscape.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
index 3724d3ef7008..69f3f1a5a782 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -280,6 +280,9 @@ static int __init ls_add_pcie_port(struct ls_pcie *pcie)
 
 	pp->ops = pcie->drvdata->ops;
 
+	/* FSL implements 6 windows */
+	pci->num_viewport = 6;
+
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "failed to initialize host\n");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
  2018-11-07 10:08 [PATCHv2 0/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
                   ` (2 preceding siblings ...)
  2018-11-07 10:09 ` [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport Z.q. Hou
@ 2018-11-07 10:09 ` Z.q. Hou
  2018-11-21 17:36   ` Gustavo Pimentel
  2018-11-28 17:59   ` Lorenzo Pieralisi
  3 siblings, 2 replies; 17+ messages in thread
From: Z.q. Hou @ 2018-11-07 10:09 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, lorenzo.pieralisi, jingoohan1,
	gustavo.pimentel
  Cc: Roy Zang, Mingkai Hu, M.h. Lian, Z.q. Hou

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

The current code only support non-prefetchable memory range,
as the non-prefetchable memory range must not be greater than
4GiB, one viewport can cover it, which supports upto 4GiB.

To support prefetchable memory range, which is upto 64-bit
memory space and can be greater than 4GiB, so we need multiple
viewports. And added separate vars to store prefetchable memory
range info to prevent overriding the non-prefetchable memory
range info.

And this patch explicitly assigned the last (if there are only
2 viewports) or last 2 viewports for CFG and I/O windows and the
rests for MEM windows.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
V2:
 - Reworded the subject and commit description.
 - Fix the prefetchable memory range overriding non-perfetchable
   memory range issue by adding vars to store prefetchable memory
   range info.

 .../pci/controller/dwc/pcie-designware-host.c | 107 ++++++++++++++----
 drivers/pci/controller/dwc/pcie-designware.h  |   7 ++
 2 files changed, 95 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ecacce016489..328aa40a6609 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -346,6 +346,35 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		dev_err(dev, "Missing *config* reg space\n");
 	}
 
+	/*
+	 * If vendor's platform driver has set the num_viewport and it is
+	 * not less than 2, skip getting the num_viewport from DT here.
+	 */
+	if (pci->num_viewport < 2) {
+		ret = of_property_read_u32(np, "num-viewport",
+					   &pci->num_viewport);
+		if (ret || pci->num_viewport < 2)
+			pci->num_viewport = 2;
+	}
+
+	/*
+	 * if there are only 2 viewports, assign the last viewport for
+	 * both CFG and IO window, otherwise assign the last 2 viewport
+	 * for CFG and IO window specific. And the rest viewports are
+	 * assigned to MEM windows.
+	 */
+	if (pci->num_viewport == 2) {
+		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
+		pp->mem_wins = 1;
+	} else {
+		pp->cfg_idx = pci->num_viewport - 1;
+		pp->io_idx = pci->num_viewport - 2;
+		pp->mem_wins = pci->num_viewport - 2;
+	}
+
+	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
+		pp->cfg_idx, pp->io_idx, pp->mem_wins);
+
 	bridge = pci_alloc_host_bridge(0);
 	if (!bridge)
 		return -ENOMEM;
@@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
 			}
 			break;
 		case IORESOURCE_MEM:
-			pp->mem = win->res;
-			pp->mem->name = "MEM";
-			pp->mem_size = resource_size(pp->mem);
-			pp->mem_bus_addr = pp->mem->start - win->offset;
+			if (win->res->flags & IORESOURCE_PREFETCH) {
+				pp->mem_perf = win->res;
+				pp->mem_perf->name = "MEM perf";
+				pp->mem_perf_size = resource_size(pp->mem_perf);
+				pp->mem_perf_bus_addr = pp->mem_perf->start -
+							win->offset;
+				pp->mem_perf_base = pp->mem_perf->start;
+			} else {
+				pp->mem = win->res;
+				pp->mem->name = "MEM";
+				pp->mem_size = resource_size(pp->mem);
+				pp->mem_bus_addr = pp->mem->start - win->offset;
+				pp->mem_base = pp->mem->start;
+			}
 			break;
 		case 0:
 			pp->cfg = win->res;
@@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
-	pp->mem_base = pp->mem->start;
-
 	if (!pp->va_cfg0_base) {
 		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
 					pp->cfg0_base, pp->cfg0_size);
@@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 		va_cfg_base = pp->va_cfg1_base;
 	}
 
-	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
+	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
 				  type, cpu_addr,
 				  busdev, cfg_size);
 	ret = dw_pcie_read(va_cfg_base + where, size, val);
-	if (pci->num_viewport <= 2)
-		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
+	if (pp->cfg_idx == pp->io_idx)
+		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
 					  PCIE_ATU_TYPE_IO, pp->io_base,
 					  pp->io_bus_addr, pp->io_size);
 
@@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 		va_cfg_base = pp->va_cfg1_base;
 	}
 
-	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
+	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
 				  type, cpu_addr,
 				  busdev, cfg_size);
 	ret = dw_pcie_write(va_cfg_base + where, size, val);
-	if (pci->num_viewport <= 2)
-		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
+	if (pp->cfg_idx == pp->io_idx)
+		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
 					  PCIE_ATU_TYPE_IO, pp->io_base,
 					  pp->io_bus_addr, pp->io_size);
 
@@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val, ctrl, num_ctrls;
+	u64 remain_size, base, win_size;
+	phys_addr_t bus_addr;
+	int i;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 
 	dw_pcie_setup(pci);
@@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 		dev_dbg(pci->dev, "iATU unroll: %s\n",
 			pci->iatu_unroll_enabled ? "enabled" : "disabled");
 
-		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
-					  PCIE_ATU_TYPE_MEM, pp->mem_base,
-					  pp->mem_bus_addr, pp->mem_size);
-		if (pci->num_viewport > 2)
-			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
-						  PCIE_ATU_TYPE_IO, pp->io_base,
-						  pp->io_bus_addr, pp->io_size);
+		/*
+		 * The maximum region size is 4 GB, and a region
+		 * must not cross a 4 GB boundary.
+		 */
+		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
+		win_size = min(win_size, pp->mem_size);
+		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
+					  pp->mem_base, pp->mem_bus_addr,
+					  win_size);
+		dev_dbg(pci->dev,
+			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa, size = %llx\n",
+			0, pp->mem_base, &pp->mem_bus_addr, win_size);
+
+		/* Prefetchable range can be 64bit space */
+		remain_size = pp->mem_perf_size;
+		base = pp->mem_perf_base;
+		bus_addr = pp->mem_perf_bus_addr;
+		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
+			win_size = SZ_4G - (base & (SZ_4G - 1));
+			win_size = min(win_size, remain_size);
+			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
+						  base, bus_addr, win_size);
+			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx, bus_addr = %pa, size = %llx\n",
+				i, base, &bus_addr, win_size);
+
+			base += win_size;
+			bus_addr += win_size;
+			remain_size -= win_size;
+		}
+
+		if (remain_size > 0)
+			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
+
+		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
+					  pp->io_base, pp->io_bus_addr,
+					  pp->io_size);
 	}
 
 	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index a438c3879aa9..0197f67f82b7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -148,15 +148,22 @@ struct pcie_port {
 	u64			cfg1_base;
 	void __iomem		*va_cfg1_base;
 	u32			cfg1_size;
+	u32			cfg_idx;
 	resource_size_t		io_base;
 	phys_addr_t		io_bus_addr;
 	u32			io_size;
+	u32			io_idx;
 	u64			mem_base;
 	phys_addr_t		mem_bus_addr;
 	u64			mem_size;
+	u64			mem_perf_base;
+	phys_addr_t		mem_perf_bus_addr;
+	u64			mem_perf_size;
+	u32			mem_wins;
 	struct resource		*cfg;
 	struct resource		*io;
 	struct resource		*mem;
+	struct resource		*mem_perf;
 	struct resource		*busn;
 	int			irq;
 	const struct dw_pcie_host_ops *ops;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
  2018-11-07 10:09 ` [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
@ 2018-11-21 17:36   ` Gustavo Pimentel
  2018-11-22  1:03     ` Z.q. Hou
  2018-11-28 17:59   ` Lorenzo Pieralisi
  1 sibling, 1 reply; 17+ messages in thread
From: Gustavo Pimentel @ 2018-11-21 17:36 UTC (permalink / raw)
  To: Z.q. Hou, linux-pci, linux-kernel, bhelgaas, lorenzo.pieralisi,
	jingoohan1, gustavo.pimentel
  Cc: Roy Zang, Mingkai Hu, M.h. Lian

On 07/11/2018 10:09, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> 
> The current code only support non-prefetchable memory range,
> as the non-prefetchable memory range must not be greater than
> 4GiB, one viewport can cover it, which supports upto 4GiB.
> 
> To support prefetchable memory range, which is upto 64-bit
> memory space and can be greater than 4GiB, so we need multiple
> viewports. And added separate vars to store prefetchable memory
> range info to prevent overriding the non-prefetchable memory
> range info.
> 
> And this patch explicitly assigned the last (if there are only
> 2 viewports) or last 2 viewports for CFG and I/O windows and the
> rests for MEM windows.
> 
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> V2:
>  - Reworded the subject and commit description.
>  - Fix the prefetchable memory range overriding non-perfetchable
>    memory range issue by adding vars to store prefetchable memory
>    range info.
> 
>  .../pci/controller/dwc/pcie-designware-host.c | 107 ++++++++++++++----
>  drivers/pci/controller/dwc/pcie-designware.h  |   7 ++
>  2 files changed, 95 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ecacce016489..328aa40a6609 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -346,6 +346,35 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		dev_err(dev, "Missing *config* reg space\n");
>  	}
>  
> +	/*
> +	 * If vendor's platform driver has set the num_viewport and it is
> +	 * not less than 2, skip getting the num_viewport from DT here.
> +	 */
> +	if (pci->num_viewport < 2) {
> +		ret = of_property_read_u32(np, "num-viewport",
> +					   &pci->num_viewport);
> +		if (ret || pci->num_viewport < 2)
> +			pci->num_viewport = 2;
> +	}
> +
> +	/*
> +	 * if there are only 2 viewports, assign the last viewport for
> +	 * both CFG and IO window, otherwise assign the last 2 viewport
> +	 * for CFG and IO window specific. And the rest viewports are
> +	 * assigned to MEM windows.
> +	 */
> +	if (pci->num_viewport == 2) {
> +		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
> +		pp->mem_wins = 1;
> +	} else {
> +		pp->cfg_idx = pci->num_viewport - 1;
> +		pp->io_idx = pci->num_viewport - 2;
> +		pp->mem_wins = pci->num_viewport - 2;
> +	}
> +
> +	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
> +		pp->cfg_idx, pp->io_idx, pp->mem_wins);
> +
>  	bridge = pci_alloc_host_bridge(0);
>  	if (!bridge)
>  		return -ENOMEM;
> @@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  			}
>  			break;
>  		case IORESOURCE_MEM:
> -			pp->mem = win->res;
> -			pp->mem->name = "MEM";
> -			pp->mem_size = resource_size(pp->mem);
> -			pp->mem_bus_addr = pp->mem->start - win->offset;
> +			if (win->res->flags & IORESOURCE_PREFETCH) {
> +				pp->mem_perf = win->res;
> +				pp->mem_perf->name = "MEM perf";
> +				pp->mem_perf_size = resource_size(pp->mem_perf);
> +				pp->mem_perf_bus_addr = pp->mem_perf->start -
> +							win->offset;
> +				pp->mem_perf_base = pp->mem_perf->start;
> +			} else {
> +				pp->mem = win->res;
> +				pp->mem->name = "MEM";
> +				pp->mem_size = resource_size(pp->mem);
> +				pp->mem_bus_addr = pp->mem->start - win->offset;
> +				pp->mem_base = pp->mem->start;
> +			}
>  			break;
>  		case 0:
>  			pp->cfg = win->res;
> @@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		}
>  	}
>  
> -	pp->mem_base = pp->mem->start;
> -
>  	if (!pp->va_cfg0_base) {
>  		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
>  					pp->cfg0_base, pp->cfg0_size);
> @@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  		va_cfg_base = pp->va_cfg1_base;
>  	}
>  
> -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
>  				  type, cpu_addr,
>  				  busdev, cfg_size);
>  	ret = dw_pcie_read(va_cfg_base + where, size, val);
> -	if (pci->num_viewport <= 2)
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	if (pp->cfg_idx == pp->io_idx)
> +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
>  					  PCIE_ATU_TYPE_IO, pp->io_base,
>  					  pp->io_bus_addr, pp->io_size);
>  
> @@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  		va_cfg_base = pp->va_cfg1_base;
>  	}
>  
> -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
>  				  type, cpu_addr,
>  				  busdev, cfg_size);
>  	ret = dw_pcie_write(va_cfg_base + where, size, val);
> -	if (pci->num_viewport <= 2)
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	if (pp->cfg_idx == pp->io_idx)
> +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
>  					  PCIE_ATU_TYPE_IO, pp->io_base,
>  					  pp->io_bus_addr, pp->io_size);
>  
> @@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
>  void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
>  	u32 val, ctrl, num_ctrls;
> +	u64 remain_size, base, win_size;
> +	phys_addr_t bus_addr;
> +	int i;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  
>  	dw_pcie_setup(pci);
> @@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		dev_dbg(pci->dev, "iATU unroll: %s\n",
>  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
>  
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> -					  pp->mem_bus_addr, pp->mem_size);
> -		if (pci->num_viewport > 2)
> -			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
> -						  PCIE_ATU_TYPE_IO, pp->io_base,
> -						  pp->io_bus_addr, pp->io_size);
> +		/*
> +		 * The maximum region size is 4 GB, and a region
> +		 * must not cross a 4 GB boundary.
> +		 */
> +		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
> +		win_size = min(win_size, pp->mem_size);
> +		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
> +					  pp->mem_base, pp->mem_bus_addr,
> +					  win_size);
> +		dev_dbg(pci->dev,
> +			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa, size = %llx\n",
> +			0, pp->mem_base, &pp->mem_bus_addr, win_size);
> +
> +		/* Prefetchable range can be 64bit space */
> +		remain_size = pp->mem_perf_size;
> +		base = pp->mem_perf_base;
> +		bus_addr = pp->mem_perf_bus_addr;
> +		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
> +			win_size = SZ_4G - (base & (SZ_4G - 1));
> +			win_size = min(win_size, remain_size);
> +			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> +						  base, bus_addr, win_size);
> +			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx, bus_addr = %pa, size = %llx\n",
> +				i, base, &bus_addr, win_size);
> +
> +			base += win_size;
> +			bus_addr += win_size;
> +			remain_size -= win_size;
> +		}
> +
> +		if (remain_size > 0)
> +			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
> +
> +		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
> +					  pp->io_base, pp->io_bus_addr,
> +					  pp->io_size);
>  	}
>  
>  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index a438c3879aa9..0197f67f82b7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -148,15 +148,22 @@ struct pcie_port {
>  	u64			cfg1_base;
>  	void __iomem		*va_cfg1_base;
>  	u32			cfg1_size;
> +	u32			cfg_idx;
>  	resource_size_t		io_base;
>  	phys_addr_t		io_bus_addr;
>  	u32			io_size;
> +	u32			io_idx;
>  	u64			mem_base;
>  	phys_addr_t		mem_bus_addr;
>  	u64			mem_size;
> +	u64			mem_perf_base;
> +	phys_addr_t		mem_perf_bus_addr;
> +	u64			mem_perf_size;
> +	u32			mem_wins;
>  	struct resource		*cfg;
>  	struct resource		*io;
>  	struct resource		*mem;
> +	struct resource		*mem_perf;
>  	struct resource		*busn;
>  	int			irq;
>  	const struct dw_pcie_host_ops *ops;
> 

Hi,

It worked perfectly in my setup, however my current configuration have
non-prefetchable memory less than 4Gb. In overall the code seems good.

Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Regards,
Gustavo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
  2018-11-21 17:36   ` Gustavo Pimentel
@ 2018-11-22  1:03     ` Z.q. Hou
  0 siblings, 0 replies; 17+ messages in thread
From: Z.q. Hou @ 2018-11-22  1:03 UTC (permalink / raw)
  To: Gustavo Pimentel, linux-pci, linux-kernel, bhelgaas,
	lorenzo.pieralisi, jingoohan1
  Cc: Roy Zang, Mingkai Hu, M.h. Lian

Hi Gustavo,

Thanks a lot for your testing and ACK!

Regards,
Zhiqiang

> -----Original Message-----
> From: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Sent: 2018年11月22日 1:37
> To: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com
> Cc: Roy Zang <roy.zang@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; M.h.
> Lian <minghuan.lian@nxp.com>
> Subject: Re: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
> 
> On 07/11/2018 10:09, Z.q. Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > The current code only support non-prefetchable memory range, as the
> > non-prefetchable memory range must not be greater than 4GiB, one
> > viewport can cover it, which supports upto 4GiB.
> >
> > To support prefetchable memory range, which is upto 64-bit memory
> > space and can be greater than 4GiB, so we need multiple viewports. And
> > added separate vars to store prefetchable memory range info to prevent
> > overriding the non-prefetchable memory range info.
> >
> > And this patch explicitly assigned the last (if there are only
> > 2 viewports) or last 2 viewports for CFG and I/O windows and the rests
> > for MEM windows.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > ---
> > V2:
> >  - Reworded the subject and commit description.
> >  - Fix the prefetchable memory range overriding non-perfetchable
> >    memory range issue by adding vars to store prefetchable memory
> >    range info.
> >
> >  .../pci/controller/dwc/pcie-designware-host.c | 107 ++++++++++++++----
> >  drivers/pci/controller/dwc/pcie-designware.h  |   7 ++
> >  2 files changed, 95 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index ecacce016489..328aa40a6609 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -346,6 +346,35 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		dev_err(dev, "Missing *config* reg space\n");
> >  	}
> >
> > +	/*
> > +	 * If vendor's platform driver has set the num_viewport and it is
> > +	 * not less than 2, skip getting the num_viewport from DT here.
> > +	 */
> > +	if (pci->num_viewport < 2) {
> > +		ret = of_property_read_u32(np, "num-viewport",
> > +					   &pci->num_viewport);
> > +		if (ret || pci->num_viewport < 2)
> > +			pci->num_viewport = 2;
> > +	}
> > +
> > +	/*
> > +	 * if there are only 2 viewports, assign the last viewport for
> > +	 * both CFG and IO window, otherwise assign the last 2 viewport
> > +	 * for CFG and IO window specific. And the rest viewports are
> > +	 * assigned to MEM windows.
> > +	 */
> > +	if (pci->num_viewport == 2) {
> > +		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
> > +		pp->mem_wins = 1;
> > +	} else {
> > +		pp->cfg_idx = pci->num_viewport - 1;
> > +		pp->io_idx = pci->num_viewport - 2;
> > +		pp->mem_wins = pci->num_viewport - 2;
> > +	}
> > +
> > +	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
> > +		pp->cfg_idx, pp->io_idx, pp->mem_wins);
> > +
> >  	bridge = pci_alloc_host_bridge(0);
> >  	if (!bridge)
> >  		return -ENOMEM;
> > @@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  			}
> >  			break;
> >  		case IORESOURCE_MEM:
> > -			pp->mem = win->res;
> > -			pp->mem->name = "MEM";
> > -			pp->mem_size = resource_size(pp->mem);
> > -			pp->mem_bus_addr = pp->mem->start - win->offset;
> > +			if (win->res->flags & IORESOURCE_PREFETCH) {
> > +				pp->mem_perf = win->res;
> > +				pp->mem_perf->name = "MEM perf";
> > +				pp->mem_perf_size = resource_size(pp->mem_perf);
> > +				pp->mem_perf_bus_addr = pp->mem_perf->start -
> > +							win->offset;
> > +				pp->mem_perf_base = pp->mem_perf->start;
> > +			} else {
> > +				pp->mem = win->res;
> > +				pp->mem->name = "MEM";
> > +				pp->mem_size = resource_size(pp->mem);
> > +				pp->mem_bus_addr = pp->mem->start - win->offset;
> > +				pp->mem_base = pp->mem->start;
> > +			}
> >  			break;
> >  		case 0:
> >  			pp->cfg = win->res;
> > @@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		}
> >  	}
> >
> > -	pp->mem_base = pp->mem->start;
> > -
> >  	if (!pp->va_cfg0_base) {
> >  		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
> >  					pp->cfg0_base, pp->cfg0_size);
> > @@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct
> pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	ret = dw_pcie_read(va_cfg_base + where, size, val);
> > -	if (pci->num_viewport <= 2)
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pp->cfg_idx == pp->io_idx)
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> >  					  pp->io_bus_addr, pp->io_size);
> >
> > @@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct
> pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	ret = dw_pcie_write(va_cfg_base + where, size, val);
> > -	if (pci->num_viewport <= 2)
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pp->cfg_idx == pp->io_idx)
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> >  					  pp->io_bus_addr, pp->io_size);
> >
> > @@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct
> > dw_pcie *pci)  void dw_pcie_setup_rc(struct pcie_port *pp)  {
> >  	u32 val, ctrl, num_ctrls;
> > +	u64 remain_size, base, win_size;
> > +	phys_addr_t bus_addr;
> > +	int i;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> >  	dw_pcie_setup(pci);
> > @@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >  		dev_dbg(pci->dev, "iATU unroll: %s\n",
> >  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
> >
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> > -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> > -					  pp->mem_bus_addr, pp->mem_size);
> > -		if (pci->num_viewport > 2)
> > -			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
> > -						  PCIE_ATU_TYPE_IO, pp->io_base,
> > -						  pp->io_bus_addr, pp->io_size);
> > +		/*
> > +		 * The maximum region size is 4 GB, and a region
> > +		 * must not cross a 4 GB boundary.
> > +		 */
> > +		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
> > +		win_size = min(win_size, pp->mem_size);
> > +		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
> > +					  pp->mem_base, pp->mem_bus_addr,
> > +					  win_size);
> > +		dev_dbg(pci->dev,
> > +			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa,
> size = %llx\n",
> > +			0, pp->mem_base, &pp->mem_bus_addr, win_size);
> > +
> > +		/* Prefetchable range can be 64bit space */
> > +		remain_size = pp->mem_perf_size;
> > +		base = pp->mem_perf_base;
> > +		bus_addr = pp->mem_perf_bus_addr;
> > +		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
> > +			win_size = SZ_4G - (base & (SZ_4G - 1));
> > +			win_size = min(win_size, remain_size);
> > +			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > +						  base, bus_addr, win_size);
> > +			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx,
> bus_addr = %pa, size = %llx\n",
> > +				i, base, &bus_addr, win_size);
> > +
> > +			base += win_size;
> > +			bus_addr += win_size;
> > +			remain_size -= win_size;
> > +		}
> > +
> > +		if (remain_size > 0)
> > +			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
> > +
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
> > +					  pp->io_base, pp->io_bus_addr,
> > +					  pp->io_size);
> >  	}
> >
> >  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); diff --git
> > a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index a438c3879aa9..0197f67f82b7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -148,15 +148,22 @@ struct pcie_port {
> >  	u64			cfg1_base;
> >  	void __iomem		*va_cfg1_base;
> >  	u32			cfg1_size;
> > +	u32			cfg_idx;
> >  	resource_size_t		io_base;
> >  	phys_addr_t		io_bus_addr;
> >  	u32			io_size;
> > +	u32			io_idx;
> >  	u64			mem_base;
> >  	phys_addr_t		mem_bus_addr;
> >  	u64			mem_size;
> > +	u64			mem_perf_base;
> > +	phys_addr_t		mem_perf_bus_addr;
> > +	u64			mem_perf_size;
> > +	u32			mem_wins;
> >  	struct resource		*cfg;
> >  	struct resource		*io;
> >  	struct resource		*mem;
> > +	struct resource		*mem_perf;
> >  	struct resource		*busn;
> >  	int			irq;
> >  	const struct dw_pcie_host_ops *ops;
> >
> 
> Hi,
> 
> It worked perfectly in my setup, however my current configuration have
> non-prefetchable memory less than 4Gb. In overall the code seems good.
> 
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> 
> Regards,
> Gustavo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport
  2018-11-07 10:09 ` [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport Z.q. Hou
@ 2018-11-22 11:16   ` Lorenzo Pieralisi
  2018-11-23  6:01     ` Z.q. Hou
  0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2018-11-22 11:16 UTC (permalink / raw)
  To: Z.q. Hou
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

On Wed, Nov 07, 2018 at 10:09:16AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> 
> FSL implements 6 viewports on Layerscape series SoCs PCIe
> controllers.
> 
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> V2:
>  - Reworded the subject and commit description.
> 
>  drivers/pci/controller/dwc/pci-layerscape.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index 3724d3ef7008..69f3f1a5a782 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -280,6 +280,9 @@ static int __init ls_add_pcie_port(struct ls_pcie *pcie)
>  
>  	pp->ops = pcie->drvdata->ops;
>  
> +	/* FSL implements 6 windows */
> +	pci->num_viewport = 6;

There is a DT property to configure this value, I am not sure it was the
right thing to do to add it but since it is there use it.

I will not consider this patch for merging.

Lorenzo

>  	ret = dw_pcie_host_init(pp);
>  	if (ret) {
>  		dev_err(dev, "failed to initialize host\n");
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport
  2018-11-22 11:16   ` Lorenzo Pieralisi
@ 2018-11-23  6:01     ` Z.q. Hou
  0 siblings, 0 replies; 17+ messages in thread
From: Z.q. Hou @ 2018-11-23  6:01 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

Hi Lorenzo,

Thanks a lot for your comments!

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: 2018年11月22日 19:17
> To: Z.q. Hou <zhiqiang.hou@nxp.com>
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>
> Subject: Re: [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport
> 
> On Wed, Nov 07, 2018 at 10:09:16AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > FSL implements 6 viewports on Layerscape series SoCs PCIe controllers.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > ---
> > V2:
> >  - Reworded the subject and commit description.
> >
> >  drivers/pci/controller/dwc/pci-layerscape.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> > b/drivers/pci/controller/dwc/pci-layerscape.c
> > index 3724d3ef7008..69f3f1a5a782 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -280,6 +280,9 @@ static int __init ls_add_pcie_port(struct ls_pcie
> > *pcie)
> >
> >  	pp->ops = pcie->drvdata->ops;
> >
> > +	/* FSL implements 6 windows */
> > +	pci->num_viewport = 6;
> 
> There is a DT property to configure this value, I am not sure it was the right
> thing to do to add it but since it is there use it.
> 
> I will not consider this patch for merging.

I will submit a separate patch to add the num_viewport to Layerscape PCIe's DT nodes, and please also merge this patch, so the new kernel can compatible with old DTB.

> 
> Lorenzo
> 
> >  	ret = dw_pcie_host_init(pp);
> >  	if (ret) {
> >  		dev_err(dev, "failed to initialize host\n");
> > --
> > 2.17.1
> >

Thanks,
Zhiqiang

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
  2018-11-07 10:09 ` [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
  2018-11-21 17:36   ` Gustavo Pimentel
@ 2018-11-28 17:59   ` Lorenzo Pieralisi
  2018-12-11 10:21     ` Z.q. Hou
  1 sibling, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2018-11-28 17:59 UTC (permalink / raw)
  To: Z.q. Hou
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

On Wed, Nov 07, 2018 at 10:09:21AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> 
> The current code only support non-prefetchable memory range,
> as the non-prefetchable memory range must not be greater than
> 4GiB, one viewport can cover it, which supports upto 4GiB.
> 
> To support prefetchable memory range, which is upto 64-bit
> memory space and can be greater than 4GiB, so we need multiple
> viewports. And added separate vars to store prefetchable memory
> range info to prevent overriding the non-prefetchable memory
> range info.
> 
> And this patch explicitly assigned the last (if there are only
> 2 viewports) or last 2 viewports for CFG and I/O windows and the
> rests for MEM windows.
> 
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> V2:
>  - Reworded the subject and commit description.
>  - Fix the prefetchable memory range overriding non-perfetchable
>    memory range issue by adding vars to store prefetchable memory
>    range info.
> 
>  .../pci/controller/dwc/pcie-designware-host.c | 107 ++++++++++++++----
>  drivers/pci/controller/dwc/pcie-designware.h  |   7 ++
>  2 files changed, 95 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ecacce016489..328aa40a6609 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -346,6 +346,35 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		dev_err(dev, "Missing *config* reg space\n");
>  	}
>  
> +	/*
> +	 * If vendor's platform driver has set the num_viewport and it is
> +	 * not less than 2, skip getting the num_viewport from DT here.

That's not what the device tree bindings specify. If DT does contain
the property you *must* read it.

> +	 */
> +	if (pci->num_viewport < 2) {
> +		ret = of_property_read_u32(np, "num-viewport",
> +					   &pci->num_viewport);
> +		if (ret || pci->num_viewport < 2)
> +			pci->num_viewport = 2;
> +	}
> +
> +	/*
> +	 * if there are only 2 viewports, assign the last viewport for
> +	 * both CFG and IO window, otherwise assign the last 2 viewport

Gah. Can anyone explain to me how this driver works if only two
viewports are available ? What happens if an IO access happens at
the same time of a config access (that fiddles with the outbound
memory windows) ?

> +	 * for CFG and IO window specific. And the rest viewports are
> +	 * assigned to MEM windows.
> +	 */
> +	if (pci->num_viewport == 2) {
> +		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
> +		pp->mem_wins = 1;
> +	} else {
> +		pp->cfg_idx = pci->num_viewport - 1;
> +		pp->io_idx = pci->num_viewport - 2;
> +		pp->mem_wins = pci->num_viewport - 2;
> +	}
> +
> +	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
> +		pp->cfg_idx, pp->io_idx, pp->mem_wins);
> +
>  	bridge = pci_alloc_host_bridge(0);
>  	if (!bridge)
>  		return -ENOMEM;
> @@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  			}
>  			break;
>  		case IORESOURCE_MEM:
> -			pp->mem = win->res;
> -			pp->mem->name = "MEM";
> -			pp->mem_size = resource_size(pp->mem);
> -			pp->mem_bus_addr = pp->mem->start - win->offset;
> +			if (win->res->flags & IORESOURCE_PREFETCH) {
> +				pp->mem_perf = win->res;
> +				pp->mem_perf->name = "MEM perf";

Nit: Why "perf" and not "pref" ?

It is confusing but that's the least of this patch problems.

> +				pp->mem_perf_size = resource_size(pp->mem_perf);
> +				pp->mem_perf_bus_addr = pp->mem_perf->start -
> +							win->offset;
> +				pp->mem_perf_base = pp->mem_perf->start;
> +			} else {
> +				pp->mem = win->res;
> +				pp->mem->name = "MEM";
> +				pp->mem_size = resource_size(pp->mem);
> +				pp->mem_bus_addr = pp->mem->start - win->offset;
> +				pp->mem_base = pp->mem->start;
> +			}
>  			break;
>  		case 0:
>  			pp->cfg = win->res;
> @@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		}
>  	}
>  
> -	pp->mem_base = pp->mem->start;
> -
>  	if (!pp->va_cfg0_base) {
>  		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
>  					pp->cfg0_base, pp->cfg0_size);
> @@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  		va_cfg_base = pp->va_cfg1_base;
>  	}
>  
> -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
>  				  type, cpu_addr,
>  				  busdev, cfg_size);
>  	ret = dw_pcie_read(va_cfg_base + where, size, val);
> -	if (pci->num_viewport <= 2)
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	if (pp->cfg_idx == pp->io_idx)
> +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
>  					  PCIE_ATU_TYPE_IO, pp->io_base,
>  					  pp->io_bus_addr, pp->io_size);

See above, even though this is not related to this patch.

>  
> @@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  		va_cfg_base = pp->va_cfg1_base;
>  	}
>  
> -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
>  				  type, cpu_addr,
>  				  busdev, cfg_size);
>  	ret = dw_pcie_write(va_cfg_base + where, size, val);
> -	if (pci->num_viewport <= 2)
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> +	if (pp->cfg_idx == pp->io_idx)
> +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
>  					  PCIE_ATU_TYPE_IO, pp->io_base,
>  					  pp->io_bus_addr, pp->io_size);
>  
> @@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
>  void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
>  	u32 val, ctrl, num_ctrls;
> +	u64 remain_size, base, win_size;
> +	phys_addr_t bus_addr;
> +	int i;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  
>  	dw_pcie_setup(pci);
> @@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		dev_dbg(pci->dev, "iATU unroll: %s\n",
>  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
>  
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> -					  pp->mem_bus_addr, pp->mem_size);
> -		if (pci->num_viewport > 2)
> -			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
> -						  PCIE_ATU_TYPE_IO, pp->io_base,
> -						  pp->io_bus_addr, pp->io_size);
> +		/*
> +		 * The maximum region size is 4 GB, and a region
> +		 * must not cross a 4 GB boundary.
> +		 */
> +		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
> +		win_size = min(win_size, pp->mem_size);
> +		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
> +					  pp->mem_base, pp->mem_bus_addr,
> +					  win_size);
> +		dev_dbg(pci->dev,
> +			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa, size = %llx\n",
> +			0, pp->mem_base, &pp->mem_bus_addr, win_size);
> +
> +		/* Prefetchable range can be 64bit space */
> +		remain_size = pp->mem_perf_size;

unallocated/free_size ?

> +		base = pp->mem_perf_base;
> +		bus_addr = pp->mem_perf_bus_addr;
> +		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
> +			win_size = SZ_4G - (base & (SZ_4G - 1));
> +			win_size = min(win_size, remain_size);
> +			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> +						  base, bus_addr, win_size);
> +			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx, bus_addr = %pa, size = %llx\n",
> +				i, base, &bus_addr, win_size);
> +
> +			base += win_size;
> +			bus_addr += win_size;
> +			remain_size -= win_size;
> +		}
> +
> +		if (remain_size > 0)
> +			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
> +
> +		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
> +					  pp->io_base, pp->io_bus_addr,
> +					  pp->io_size);
>  	}
>  
>  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index a438c3879aa9..0197f67f82b7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -148,15 +148,22 @@ struct pcie_port {
>  	u64			cfg1_base;
>  	void __iomem		*va_cfg1_base;
>  	u32			cfg1_size;
> +	u32			cfg_idx;
>  	resource_size_t		io_base;
>  	phys_addr_t		io_bus_addr;
>  	u32			io_size;
> +	u32			io_idx;
>  	u64			mem_base;
>  	phys_addr_t		mem_bus_addr;
>  	u64			mem_size;
> +	u64			mem_perf_base;

This is a phys_addr_t

> +	phys_addr_t		mem_perf_bus_addr;

pci_bus_addr_t ?

Lorenzo

> +	u64			mem_perf_size;
> +	u32			mem_wins;
>  	struct resource		*cfg;
>  	struct resource		*io;
>  	struct resource		*mem;
> +	struct resource		*mem_perf;
>  	struct resource		*busn;
>  	int			irq;
>  	const struct dw_pcie_host_ops *ops;
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 1/4] PCI: dwc: fix potential memory leak
  2018-11-07 10:09 ` [PATCHv2 1/4] PCI: dwc: fix potential memory leak Z.q. Hou
@ 2018-12-05 15:40   ` Lorenzo Pieralisi
  2018-12-06  1:08     ` Z.q. Hou
  0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2018-12-05 15:40 UTC (permalink / raw)
  To: Z.q. Hou
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

On Wed, Nov 07, 2018 at 10:09:04AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> 
> Free the allocated pci_host_bridge struct when failed to get
> host bridge resources, and free the resource windows before
> free the bridge.
> 
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> V2:
>  - Reworded the subject.
> 
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 29a05759a294..ecacce016489 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -353,7 +353,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
>  					&bridge->windows, &pp->io_base);
>  	if (ret)
> -		return ret;
> +		goto error;
>  
>  	ret = devm_request_pci_bus_resources(dev, &bridge->windows);
>  	if (ret)
> @@ -502,6 +502,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	return 0;
>  
>  error:
> +	pci_free_resource_list(&bridge->windows);

This would cause a double-free, devm_request_pci_bus_resource already
takes care of freeing resources, patch dropped.

Lorenzo

>  	pci_free_host_bridge(bridge);
>  	return ret;
>  }
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue
  2018-11-07 10:09 ` [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue Z.q. Hou
@ 2018-12-05 16:01   ` Lorenzo Pieralisi
  2018-12-06  1:25     ` Z.q. Hou
  0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2018-12-05 16:01 UTC (permalink / raw)
  To: Z.q. Hou
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

On Wed, Nov 07, 2018 at 10:09:10AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> 
> The current type of mem_size is 'u32', so when resource_size()
> return 4G it will be truncated to zero. This patch fix it by
> changing its type to 'u64'.
> 
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> V2:
>  - Reworded the subject.
> 
>  drivers/pci/controller/dwc/pcie-designware.c | 4 ++--
>  drivers/pci/controller/dwc/pcie-designware.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

I would like to add a Fixes: tag.

is

edd45e396829 ("PCI: dwc: designware: Move _unroll configurations to a
separate function")

the commit you are fixing ?

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 2153956a0b20..7ac5989c23ef 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -106,7 +106,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg,
>  
>  static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
>  					     int type, u64 cpu_addr,
> -					     u64 pci_addr, u32 size)
> +					     u64 pci_addr, u64 size)
>  {
>  	u32 retries, val;
>  
> @@ -141,7 +141,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
>  }
>  
>  void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> -			       u64 cpu_addr, u64 pci_addr, u32 size)
> +			       u64 cpu_addr, u64 pci_addr, u64 size)
>  {
>  	u32 retries, val;
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 9f1a5e399b70..a438c3879aa9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -153,7 +153,7 @@ struct pcie_port {
>  	u32			io_size;
>  	u64			mem_base;
>  	phys_addr_t		mem_bus_addr;
> -	u32			mem_size;
> +	u64			mem_size;
>  	struct resource		*cfg;
>  	struct resource		*io;
>  	struct resource		*mem;
> @@ -238,7 +238,7 @@ int dw_pcie_link_up(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);
>  void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
>  			       int type, u64 cpu_addr, u64 pci_addr,
> -			       u32 size);
> +			       u64 size);
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
>  			     u64 cpu_addr, enum dw_pcie_as_type as_type);
>  void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCHv2 1/4] PCI: dwc: fix potential memory leak
  2018-12-05 15:40   ` Lorenzo Pieralisi
@ 2018-12-06  1:08     ` Z.q. Hou
  0 siblings, 0 replies; 17+ messages in thread
From: Z.q. Hou @ 2018-12-06  1:08 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

Hi Lorenzo,

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: 2018年12月5日 23:40
> To: Z.q. Hou <zhiqiang.hou@nxp.com>
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>
> Subject: Re: [PATCHv2 1/4] PCI: dwc: fix potential memory leak
> 
> On Wed, Nov 07, 2018 at 10:09:04AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > Free the allocated pci_host_bridge struct when failed to get host
> > bridge resources, and free the resource windows before free the
> > bridge.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > ---
> > V2:
> >  - Reworded the subject.
> >
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 29a05759a294..ecacce016489 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -353,7 +353,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
> >  					&bridge->windows, &pp->io_base);
> >  	if (ret)
> > -		return ret;
> > +		goto error;
> >
> >  	ret = devm_request_pci_bus_resources(dev, &bridge->windows);
> >  	if (ret)
> > @@ -502,6 +502,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  	return 0;
> >
> >  error:
> > +	pci_free_resource_list(&bridge->windows);
> 
> This would cause a double-free, devm_request_pci_bus_resource already
> takes care of freeing resources, patch dropped.

Yes, I just understood.

> Lorenzo
> 
> >  	pci_free_host_bridge(bridge);
> >  	return ret;
> >  }
> > --
> > 2.17.1
> >
Thanks,
Zhiqiang

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue
  2018-12-05 16:01   ` Lorenzo Pieralisi
@ 2018-12-06  1:25     ` Z.q. Hou
  2018-12-11 14:40       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 17+ messages in thread
From: Z.q. Hou @ 2018-12-06  1:25 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

Hi Lorenzo,

Thanks a lot for your comments!

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: 2018年12月6日 0:02
> To: Z.q. Hou <zhiqiang.hou@nxp.com>
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>
> Subject: Re: [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size
> truncated to zero issue
> 
> On Wed, Nov 07, 2018 at 10:09:10AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > The current type of mem_size is 'u32', so when resource_size() return
> > 4G it will be truncated to zero. This patch fix it by changing its
> > type to 'u64'.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > ---
> > V2:
> >  - Reworded the subject.
> >
> >  drivers/pci/controller/dwc/pcie-designware.c | 4 ++--
> > drivers/pci/controller/dwc/pcie-designware.h | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> I would like to add a Fixes: tag.
> 
> is
> 
> edd45e396829 ("PCI: dwc: designware: Move _unroll configurations to a
> separate function")
> 
> the commit you are fixing ?

Yes, will add the Fixes, and I think it fixes the original patch: 340cba6092c2 ("pci: Add PCIe driver for Samsung Exynos").

> Thanks,
> Lorenzo
> 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > b/drivers/pci/controller/dwc/pcie-designware.c
> > index 2153956a0b20..7ac5989c23ef 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -106,7 +106,7 @@ static void dw_pcie_writel_ob_unroll(struct
> > dw_pcie *pci, u32 index, u32 reg,
> >
> >  static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int
> index,
> >  					     int type, u64 cpu_addr,
> > -					     u64 pci_addr, u32 size)
> > +					     u64 pci_addr, u64 size)
> >  {
> >  	u32 retries, val;
> >
> > @@ -141,7 +141,7 @@ static void
> > dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,  }
> >
> >  void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > -			       u64 cpu_addr, u64 pci_addr, u32 size)
> > +			       u64 cpu_addr, u64 pci_addr, u64 size)
> >  {
> >  	u32 retries, val;
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 9f1a5e399b70..a438c3879aa9 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -153,7 +153,7 @@ struct pcie_port {
> >  	u32			io_size;
> >  	u64			mem_base;
> >  	phys_addr_t		mem_bus_addr;
> > -	u32			mem_size;
> > +	u64			mem_size;
> >  	struct resource		*cfg;
> >  	struct resource		*io;
> >  	struct resource		*mem;
> > @@ -238,7 +238,7 @@ int dw_pcie_link_up(struct dw_pcie *pci);  int
> > dw_pcie_wait_for_link(struct dw_pcie *pci);  void
> > dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
> >  			       int type, u64 cpu_addr, u64 pci_addr,
> > -			       u32 size);
> > +			       u64 size);
> >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
> >  			     u64 cpu_addr, enum dw_pcie_as_type as_type);  void
> > dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > --
> > 2.17.1
> >

Thanks,
Zhiqiang

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
  2018-11-28 17:59   ` Lorenzo Pieralisi
@ 2018-12-11 10:21     ` Z.q. Hou
  2018-12-11 15:19       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 17+ messages in thread
From: Z.q. Hou @ 2018-12-11 10:21 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

Hi Lorenzo,

Thanks a lot for your comments!

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: 2018年11月29日 2:00
> To: Z.q. Hou <zhiqiang.hou@nxp.com>
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>
> Subject: Re: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
> 
> On Wed, Nov 07, 2018 at 10:09:21AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > The current code only support non-prefetchable memory range, as the
> > non-prefetchable memory range must not be greater than 4GiB, one
> > viewport can cover it, which supports upto 4GiB.
> >
> > To support prefetchable memory range, which is upto 64-bit memory
> > space and can be greater than 4GiB, so we need multiple viewports. And
> > added separate vars to store prefetchable memory range info to prevent
> > overriding the non-prefetchable memory range info.
> >
> > And this patch explicitly assigned the last (if there are only
> > 2 viewports) or last 2 viewports for CFG and I/O windows and the rests
> > for MEM windows.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > ---
> > V2:
> >  - Reworded the subject and commit description.
> >  - Fix the prefetchable memory range overriding non-perfetchable
> >    memory range issue by adding vars to store prefetchable memory
> >    range info.
> >
> >  .../pci/controller/dwc/pcie-designware-host.c | 107 ++++++++++++++----
> >  drivers/pci/controller/dwc/pcie-designware.h  |   7 ++
> >  2 files changed, 95 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index ecacce016489..328aa40a6609 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -346,6 +346,35 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		dev_err(dev, "Missing *config* reg space\n");
> >  	}
> >
> > +	/*
> > +	 * If vendor's platform driver has set the num_viewport and it is
> > +	 * not less than 2, skip getting the num_viewport from DT here.
> 
> That's not what the device tree bindings specify. If DT does contain the
> property you *must* read it.

Yes, will change back to *must* read.

> 
> > +	 */
> > +	if (pci->num_viewport < 2) {
> > +		ret = of_property_read_u32(np, "num-viewport",
> > +					   &pci->num_viewport);
> > +		if (ret || pci->num_viewport < 2)
> > +			pci->num_viewport = 2;
> > +	}
> > +
> > +	/*
> > +	 * if there are only 2 viewports, assign the last viewport for
> > +	 * both CFG and IO window, otherwise assign the last 2 viewport
> 
> Gah. Can anyone explain to me how this driver works if only two viewports
> are available ? What happens if an IO access happens at the same time of a
> config access (that fiddles with the outbound memory windows) ?

You are right it has potential IO transaction drop issue, but DWC PCIe databook does give a default value 2 of viewport number.

 
> > +	 * for CFG and IO window specific. And the rest viewports are
> > +	 * assigned to MEM windows.
> > +	 */
> > +	if (pci->num_viewport == 2) {
> > +		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
> > +		pp->mem_wins = 1;
> > +	} else {
> > +		pp->cfg_idx = pci->num_viewport - 1;
> > +		pp->io_idx = pci->num_viewport - 2;
> > +		pp->mem_wins = pci->num_viewport - 2;
> > +	}
> > +
> > +	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
> > +		pp->cfg_idx, pp->io_idx, pp->mem_wins);
> > +
> >  	bridge = pci_alloc_host_bridge(0);
> >  	if (!bridge)
> >  		return -ENOMEM;
> > @@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  			}
> >  			break;
> >  		case IORESOURCE_MEM:
> > -			pp->mem = win->res;
> > -			pp->mem->name = "MEM";
> > -			pp->mem_size = resource_size(pp->mem);
> > -			pp->mem_bus_addr = pp->mem->start - win->offset;
> > +			if (win->res->flags & IORESOURCE_PREFETCH) {
> > +				pp->mem_perf = win->res;
> > +				pp->mem_perf->name = "MEM perf";
> 
> Nit: Why "perf" and not "pref" ?
> 
> It is confusing but that's the least of this patch problems.

My bad, it is typo, will fix in next version.

> 
> > +				pp->mem_perf_size = resource_size(pp->mem_perf);
> > +				pp->mem_perf_bus_addr = pp->mem_perf->start -
> > +							win->offset;
> > +				pp->mem_perf_base = pp->mem_perf->start;
> > +			} else {
> > +				pp->mem = win->res;
> > +				pp->mem->name = "MEM";
> > +				pp->mem_size = resource_size(pp->mem);
> > +				pp->mem_bus_addr = pp->mem->start - win->offset;
> > +				pp->mem_base = pp->mem->start;
> > +			}
> >  			break;
> >  		case 0:
> >  			pp->cfg = win->res;
> > @@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		}
> >  	}
> >
> > -	pp->mem_base = pp->mem->start;
> > -
> >  	if (!pp->va_cfg0_base) {
> >  		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
> >  					pp->cfg0_base, pp->cfg0_size);
> > @@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct
> pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	ret = dw_pcie_read(va_cfg_base + where, size, val);
> > -	if (pci->num_viewport <= 2)
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pp->cfg_idx == pp->io_idx)
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> >  					  pp->io_bus_addr, pp->io_size);
> 
> See above, even though this is not related to this patch.

I think it is more reasonable to check if the CFG and IO shared viewport than the viewport number.

> 
> >
> > @@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct
> pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	ret = dw_pcie_write(va_cfg_base + where, size, val);
> > -	if (pci->num_viewport <= 2)
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pp->cfg_idx == pp->io_idx)
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> >  					  pp->io_bus_addr, pp->io_size);
> >
> > @@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct
> > dw_pcie *pci)  void dw_pcie_setup_rc(struct pcie_port *pp)  {
> >  	u32 val, ctrl, num_ctrls;
> > +	u64 remain_size, base, win_size;
> > +	phys_addr_t bus_addr;
> > +	int i;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> >  	dw_pcie_setup(pci);
> > @@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >  		dev_dbg(pci->dev, "iATU unroll: %s\n",
> >  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
> >
> > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> > -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> > -					  pp->mem_bus_addr, pp->mem_size);
> > -		if (pci->num_viewport > 2)
> > -			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
> > -						  PCIE_ATU_TYPE_IO, pp->io_base,
> > -						  pp->io_bus_addr, pp->io_size);
> > +		/*
> > +		 * The maximum region size is 4 GB, and a region
> > +		 * must not cross a 4 GB boundary.
> > +		 */
> > +		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
> > +		win_size = min(win_size, pp->mem_size);
> > +		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
> > +					  pp->mem_base, pp->mem_bus_addr,
> > +					  win_size);
> > +		dev_dbg(pci->dev,
> > +			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa,
> size = %llx\n",
> > +			0, pp->mem_base, &pp->mem_bus_addr, win_size);
> > +
> > +		/* Prefetchable range can be 64bit space */
> > +		remain_size = pp->mem_perf_size;
> 
> unallocated/free_size ?

Sorry, I don't understand your question.

> 
> > +		base = pp->mem_perf_base;
> > +		bus_addr = pp->mem_perf_bus_addr;
> > +		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
> > +			win_size = SZ_4G - (base & (SZ_4G - 1));
> > +			win_size = min(win_size, remain_size);
> > +			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > +						  base, bus_addr, win_size);
> > +			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx,
> bus_addr = %pa, size = %llx\n",
> > +				i, base, &bus_addr, win_size);
> > +
> > +			base += win_size;
> > +			bus_addr += win_size;
> > +			remain_size -= win_size;
> > +		}
> > +
> > +		if (remain_size > 0)
> > +			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
> > +
> > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
> > +					  pp->io_base, pp->io_bus_addr,
> > +					  pp->io_size);
> >  	}
> >
> >  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); diff --git
> > a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index a438c3879aa9..0197f67f82b7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -148,15 +148,22 @@ struct pcie_port {
> >  	u64			cfg1_base;
> >  	void __iomem		*va_cfg1_base;
> >  	u32			cfg1_size;
> > +	u32			cfg_idx;
> >  	resource_size_t		io_base;
> >  	phys_addr_t		io_bus_addr;
> >  	u32			io_size;
> > +	u32			io_idx;
> >  	u64			mem_base;
> >  	phys_addr_t		mem_bus_addr;
> >  	u64			mem_size;
> > +	u64			mem_perf_base;
> 
> This is a phys_addr_t
> 
> > +	phys_addr_t		mem_perf_bus_addr;
> 
> pci_bus_addr_t ?

Will fix in next version.

> 
> Lorenzo
> 
> > +	u64			mem_perf_size;
> > +	u32			mem_wins;
> >  	struct resource		*cfg;
> >  	struct resource		*io;
> >  	struct resource		*mem;
> > +	struct resource		*mem_perf;
> >  	struct resource		*busn;
> >  	int			irq;
> >  	const struct dw_pcie_host_ops *ops;
> > --
> > 2.17.1
> >

Thanks,
Zhiqiang

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue
  2018-12-06  1:25     ` Z.q. Hou
@ 2018-12-11 14:40       ` Lorenzo Pieralisi
  0 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2018-12-11 14:40 UTC (permalink / raw)
  To: Z.q. Hou
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, gustavo.pimentel,
	Roy Zang, Mingkai Hu, M.h. Lian

On Thu, Dec 06, 2018 at 01:25:17AM +0000, Z.q. Hou wrote:
> Hi Lorenzo,
> 
> Thanks a lot for your comments!
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Sent: 2018??12??6?? 0:02
> > To: Z.q. Hou <zhiqiang.hou@nxp.com>
> > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> > bhelgaas@google.com; jingoohan1@gmail.com;
> > gustavo.pimentel@synopsys.com; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> > <mingkai.hu@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>
> > Subject: Re: [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size
> > truncated to zero issue
> > 
> > On Wed, Nov 07, 2018 at 10:09:10AM +0000, Z.q. Hou wrote:
> > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > >
> > > The current type of mem_size is 'u32', so when resource_size() return
> > > 4G it will be truncated to zero. This patch fix it by changing its
> > > type to 'u64'.
> > >
> > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > ---
> > > V2:
> > >  - Reworded the subject.
> > >
> > >  drivers/pci/controller/dwc/pcie-designware.c | 4 ++--
> > > drivers/pci/controller/dwc/pcie-designware.h | 4 ++--
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > I would like to add a Fixes: tag.
> > 
> > is
> > 
> > edd45e396829 ("PCI: dwc: designware: Move _unroll configurations to a
> > separate function")
> > 
> > the commit you are fixing ?
> 
> Yes, will add the Fixes, and I think it fixes the original patch: 340cba6092c2 ("pci: Add PCIe driver for Samsung Exynos").

I will add it myself.

Thanks,
Lorenzo

> > Thanks,
> > Lorenzo
> > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > > b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 2153956a0b20..7ac5989c23ef 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -106,7 +106,7 @@ static void dw_pcie_writel_ob_unroll(struct
> > > dw_pcie *pci, u32 index, u32 reg,
> > >
> > >  static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int
> > index,
> > >  					     int type, u64 cpu_addr,
> > > -					     u64 pci_addr, u32 size)
> > > +					     u64 pci_addr, u64 size)
> > >  {
> > >  	u32 retries, val;
> > >
> > > @@ -141,7 +141,7 @@ static void
> > > dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,  }
> > >
> > >  void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > -			       u64 cpu_addr, u64 pci_addr, u32 size)
> > > +			       u64 cpu_addr, u64 pci_addr, u64 size)
> > >  {
> > >  	u32 retries, val;
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 9f1a5e399b70..a438c3879aa9 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -153,7 +153,7 @@ struct pcie_port {
> > >  	u32			io_size;
> > >  	u64			mem_base;
> > >  	phys_addr_t		mem_bus_addr;
> > > -	u32			mem_size;
> > > +	u64			mem_size;
> > >  	struct resource		*cfg;
> > >  	struct resource		*io;
> > >  	struct resource		*mem;
> > > @@ -238,7 +238,7 @@ int dw_pcie_link_up(struct dw_pcie *pci);  int
> > > dw_pcie_wait_for_link(struct dw_pcie *pci);  void
> > > dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
> > >  			       int type, u64 cpu_addr, u64 pci_addr,
> > > -			       u32 size);
> > > +			       u64 size);
> > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
> > >  			     u64 cpu_addr, enum dw_pcie_as_type as_type);  void
> > > dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > > --
> > > 2.17.1
> > >
> 
> Thanks,
> Zhiqiang

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support
  2018-12-11 10:21     ` Z.q. Hou
@ 2018-12-11 15:19       ` Lorenzo Pieralisi
  0 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2018-12-11 15:19 UTC (permalink / raw)
  To: Z.q. Hou, gustavo.pimentel
  Cc: linux-pci, linux-kernel, bhelgaas, jingoohan1, Roy Zang,
	Mingkai Hu, M.h. Lian

On Tue, Dec 11, 2018 at 10:21:08AM +0000, Z.q. Hou wrote:

[...]

> > > +	 */
> > > +	if (pci->num_viewport < 2) {
> > > +		ret = of_property_read_u32(np, "num-viewport",
> > > +					   &pci->num_viewport);
> > > +		if (ret || pci->num_viewport < 2)
> > > +			pci->num_viewport = 2;
> > > +	}
> > > +
> > > +	/*
> > > +	 * if there are only 2 viewports, assign the last viewport for
> > > +	 * both CFG and IO window, otherwise assign the last 2 viewport
> > 
> > Gah. Can anyone explain to me how this driver works if only two
> > viewports are available ? What happens if an IO access happens at
> > the same time of a config access (that fiddles with the outbound
> > memory windows) ?

Guys, this is utterly broken. If the IP does not have enough address
decoders it is better not to support IO at all rather implementing
it with this broken code that can fail anytime.

This ought to be fixed and it is not a problem with this patch it
is a *bug* in the mainline kernel.

Lorenzo

> You are right it has potential IO transaction drop issue, but DWC PCIe databook does give a default value 2 of viewport number.
> 
>  
> > > +	 * for CFG and IO window specific. And the rest viewports are
> > > +	 * assigned to MEM windows.
> > > +	 */
> > > +	if (pci->num_viewport == 2) {
> > > +		pp->cfg_idx = pp->io_idx = PCIE_ATU_REGION_INDEX1;
> > > +		pp->mem_wins = 1;
> > > +	} else {
> > > +		pp->cfg_idx = pci->num_viewport - 1;
> > > +		pp->io_idx = pci->num_viewport - 2;
> > > +		pp->mem_wins = pci->num_viewport - 2;
> > > +	}
> > > +
> > > +	dev_dbg(dev, "CFG win id: %d, I/O win id: %d, Total MEM win: %d\n",
> > > +		pp->cfg_idx, pp->io_idx, pp->mem_wins);
> > > +
> > >  	bridge = pci_alloc_host_bridge(0);
> > >  	if (!bridge)
> > >  		return -ENOMEM;
> > > @@ -377,10 +406,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > >  			}
> > >  			break;
> > >  		case IORESOURCE_MEM:
> > > -			pp->mem = win->res;
> > > -			pp->mem->name = "MEM";
> > > -			pp->mem_size = resource_size(pp->mem);
> > > -			pp->mem_bus_addr = pp->mem->start - win->offset;
> > > +			if (win->res->flags & IORESOURCE_PREFETCH) {
> > > +				pp->mem_perf = win->res;
> > > +				pp->mem_perf->name = "MEM perf";
> > 
> > Nit: Why "perf" and not "pref" ?
> > 
> > It is confusing but that's the least of this patch problems.
> 
> My bad, it is typo, will fix in next version.
> 
> > 
> > > +				pp->mem_perf_size = resource_size(pp->mem_perf);
> > > +				pp->mem_perf_bus_addr = pp->mem_perf->start -
> > > +							win->offset;
> > > +				pp->mem_perf_base = pp->mem_perf->start;
> > > +			} else {
> > > +				pp->mem = win->res;
> > > +				pp->mem->name = "MEM";
> > > +				pp->mem_size = resource_size(pp->mem);
> > > +				pp->mem_bus_addr = pp->mem->start - win->offset;
> > > +				pp->mem_base = pp->mem->start;
> > > +			}
> > >  			break;
> > >  		case 0:
> > >  			pp->cfg = win->res;
> > > @@ -406,8 +445,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > >  		}
> > >  	}
> > >
> > > -	pp->mem_base = pp->mem->start;
> > > -
> > >  	if (!pp->va_cfg0_base) {
> > >  		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
> > >  					pp->cfg0_base, pp->cfg0_size);
> > > @@ -534,12 +571,12 @@ static int dw_pcie_rd_other_conf(struct
> > pcie_port *pp, struct pci_bus *bus,
> > >  		va_cfg_base = pp->va_cfg1_base;
> > >  	}
> > >
> > > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> > >  				  type, cpu_addr,
> > >  				  busdev, cfg_size);
> > >  	ret = dw_pcie_read(va_cfg_base + where, size, val);
> > > -	if (pci->num_viewport <= 2)
> > > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > > +	if (pp->cfg_idx == pp->io_idx)
> > > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> > >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> > >  					  pp->io_bus_addr, pp->io_size);
> > 
> > See above, even though this is not related to this patch.
> 
> I think it is more reasonable to check if the CFG and IO shared viewport than the viewport number.
> 
> > 
> > >
> > > @@ -573,12 +610,12 @@ static int dw_pcie_wr_other_conf(struct
> > pcie_port *pp, struct pci_bus *bus,
> > >  		va_cfg_base = pp->va_cfg1_base;
> > >  	}
> > >
> > > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > > +	dw_pcie_prog_outbound_atu(pci, pp->cfg_idx,
> > >  				  type, cpu_addr,
> > >  				  busdev, cfg_size);
> > >  	ret = dw_pcie_write(va_cfg_base + where, size, val);
> > > -	if (pci->num_viewport <= 2)
> > > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > > +	if (pp->cfg_idx == pp->io_idx)
> > > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx,
> > >  					  PCIE_ATU_TYPE_IO, pp->io_base,
> > >  					  pp->io_bus_addr, pp->io_size);
> > >
> > > @@ -652,6 +689,9 @@ static u8 dw_pcie_iatu_unroll_enabled(struct
> > > dw_pcie *pci)  void dw_pcie_setup_rc(struct pcie_port *pp)  {
> > >  	u32 val, ctrl, num_ctrls;
> > > +	u64 remain_size, base, win_size;
> > > +	phys_addr_t bus_addr;
> > > +	int i;
> > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > >
> > >  	dw_pcie_setup(pci);
> > > @@ -700,13 +740,42 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> > >  		dev_dbg(pci->dev, "iATU unroll: %s\n",
> > >  			pci->iatu_unroll_enabled ? "enabled" : "disabled");
> > >
> > > -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> > > -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> > > -					  pp->mem_bus_addr, pp->mem_size);
> > > -		if (pci->num_viewport > 2)
> > > -			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
> > > -						  PCIE_ATU_TYPE_IO, pp->io_base,
> > > -						  pp->io_bus_addr, pp->io_size);
> > > +		/*
> > > +		 * The maximum region size is 4 GB, and a region
> > > +		 * must not cross a 4 GB boundary.
> > > +		 */
> > > +		win_size = SZ_4G - (pp->mem_base & (SZ_4G - 1));
> > > +		win_size = min(win_size, pp->mem_size);
> > > +		dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_MEM,
> > > +					  pp->mem_base, pp->mem_bus_addr,
> > > +					  win_size);
> > > +		dev_dbg(pci->dev,
> > > +			"iATU: non-pref MEM: win = %d: base = %llx, bus_addr = %pa,
> > size = %llx\n",
> > > +			0, pp->mem_base, &pp->mem_bus_addr, win_size);
> > > +
> > > +		/* Prefetchable range can be 64bit space */
> > > +		remain_size = pp->mem_perf_size;
> > 
> > unallocated/free_size ?
> 
> Sorry, I don't understand your question.
> 
> > 
> > > +		base = pp->mem_perf_base;
> > > +		bus_addr = pp->mem_perf_bus_addr;
> > > +		for (i = 1; remain_size > 0 && i < pp->mem_wins; i++) {
> > > +			win_size = SZ_4G - (base & (SZ_4G - 1));
> > > +			win_size = min(win_size, remain_size);
> > > +			dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > > +						  base, bus_addr, win_size);
> > > +			dev_dbg(pci->dev, "iATU: pref MEM: win = %d: base = %llx,
> > bus_addr = %pa, size = %llx\n",
> > > +				i, base, &bus_addr, win_size);
> > > +
> > > +			base += win_size;
> > > +			bus_addr += win_size;
> > > +			remain_size -= win_size;
> > > +		}
> > > +
> > > +		if (remain_size > 0)
> > > +			dev_info(pci->dev, "iATU: MEM window isn't enough\n");
> > > +
> > > +		dw_pcie_prog_outbound_atu(pci, pp->io_idx, PCIE_ATU_TYPE_IO,
> > > +					  pp->io_base, pp->io_bus_addr,
> > > +					  pp->io_size);
> > >  	}
> > >
> > >  	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); diff --git
> > > a/drivers/pci/controller/dwc/pcie-designware.h
> > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > index a438c3879aa9..0197f67f82b7 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -148,15 +148,22 @@ struct pcie_port {
> > >  	u64			cfg1_base;
> > >  	void __iomem		*va_cfg1_base;
> > >  	u32			cfg1_size;
> > > +	u32			cfg_idx;
> > >  	resource_size_t		io_base;
> > >  	phys_addr_t		io_bus_addr;
> > >  	u32			io_size;
> > > +	u32			io_idx;
> > >  	u64			mem_base;
> > >  	phys_addr_t		mem_bus_addr;
> > >  	u64			mem_size;
> > > +	u64			mem_perf_base;
> > 
> > This is a phys_addr_t
> > 
> > > +	phys_addr_t		mem_perf_bus_addr;
> > 
> > pci_bus_addr_t ?
> 
> Will fix in next version.
> 
> > 
> > Lorenzo
> > 
> > > +	u64			mem_perf_size;
> > > +	u32			mem_wins;
> > >  	struct resource		*cfg;
> > >  	struct resource		*io;
> > >  	struct resource		*mem;
> > > +	struct resource		*mem_perf;
> > >  	struct resource		*busn;
> > >  	int			irq;
> > >  	const struct dw_pcie_host_ops *ops;
> > > --
> > > 2.17.1
> > >
> 
> Thanks,
> Zhiqiang

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-12-11 15:19 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 10:08 [PATCHv2 0/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
2018-11-07 10:09 ` [PATCHv2 1/4] PCI: dwc: fix potential memory leak Z.q. Hou
2018-12-05 15:40   ` Lorenzo Pieralisi
2018-12-06  1:08     ` Z.q. Hou
2018-11-07 10:09 ` [PATCHv2 2/4] PCI: dwc: fix 4GiB outbound window size truncated to zero issue Z.q. Hou
2018-12-05 16:01   ` Lorenzo Pieralisi
2018-12-06  1:25     ` Z.q. Hou
2018-12-11 14:40       ` Lorenzo Pieralisi
2018-11-07 10:09 ` [PATCHv2 3/4] PCI: layerscape: initialize the number of viewport Z.q. Hou
2018-11-22 11:16   ` Lorenzo Pieralisi
2018-11-23  6:01     ` Z.q. Hou
2018-11-07 10:09 ` [PATCHv2 4/4] PCI: dwc: add prefetchable memory range support Z.q. Hou
2018-11-21 17:36   ` Gustavo Pimentel
2018-11-22  1:03     ` Z.q. Hou
2018-11-28 17:59   ` Lorenzo Pieralisi
2018-12-11 10:21     ` Z.q. Hou
2018-12-11 15:19       ` Lorenzo Pieralisi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.