[PATCH v4 0/5] PCI endpoint 64-bit BAR fixes

[PATCH v4 0/5] PCI endpoint 64-bit BAR fixes

[Niklas Cassel]

PCI endpoint fixes to improve the way 64-bit BARs are handled.


There are still future improvements that could be made:

pci-epf-test.c always allocates space for
6 BARs, even when using 64-bit BARs (which
really only requires us to allocate 3 BARs).

pcitest.sh will print "NOT OKAY" for BAR1,
BAR3, and BAR5 when using 64-bit BARs.
This could probably be improved to say
something like "N/A (64-bit BAR)".

Niklas Cassel (5):
  PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
  PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
    properly
  PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
    properly
  PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was
    set-up
  misc: pci_endpoint_test: Handle 64-bit BARs properly

 drivers/misc/pci_endpoint_test.c              | 12 +++++++-----
 drivers/pci/cadence/pcie-cadence-ep.c         |  2 +-
 drivers/pci/dwc/pcie-designware-ep.c          | 22 ++++++++++++++++++----
 drivers/pci/endpoint/functions/pci-epf-test.c | 22 +++++++++++++++-------
 4 files changed, 41 insertions(+), 17 deletions(-)

-- 
2.14.2

[PATCH v4 1/5] PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

[Niklas Cassel]

If a BAR supports 64-bit width or not depends on the hardware,
and should thus not depend on sizeof(dma_addr_t).

Since this driver is generic, default to always using BAR width
of 32-bits. 64-bit BARs can easily be tested by replacing
PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
in bar_flags.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
Note to Lorenzo/Bjorn:
It is not trivial to convert the bar_size + bar_flags +
struct pci_epf->bar member array to an array of struct resources,
since we need to be able to store the addresses returned
by dma_alloc_coherent(), which is of type dma_addr_t.
struct resource uses resource_size_t, which is defined as phys_addr_t.
E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.

 drivers/pci/endpoint/functions/pci-epf-test.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 800da09d9005..7c70433b11a7 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -71,6 +71,14 @@ struct pci_epf_test_data {
 };
 
 static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
+static int bar_flags[] = {
+	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
+};
 
 static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 {
@@ -358,7 +366,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
 
 static int pci_epf_test_set_bar(struct pci_epf *epf)
 {
-	int flags;
 	int bar;
 	int ret;
 	struct pci_epf_bar *epf_bar;
@@ -367,15 +374,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 
-	flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
-	if (sizeof(dma_addr_t) == 0x8)
-		flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
-
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
 		epf_bar = &epf->bar[bar];
 		ret = pci_epc_set_bar(epc, epf->func_no, bar,
 				      epf_bar->phys_addr,
-				      epf_bar->size, flags);
+				      epf_bar->size, bar_flags[bar]);
 		if (ret) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
 			dev_err(dev, "failed to set BAR%d\n", bar);
-- 
2.14.2

[PATCH v4 2/5] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

[Niklas Cassel]

Since a 64-bit BAR consists of a BAR pair, we need to write to both
BARs in the BAR pair to setup the BAR properly.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 9236b998327f..946bbdf53c4d 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -136,8 +136,15 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 		return ret;
 
 	dw_pcie_dbi_ro_wr_en(pci);
-	dw_pcie_writel_dbi2(pci, reg, size - 1);
-	dw_pcie_writel_dbi(pci, reg, flags);
+	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+		dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
+		dw_pcie_writel_dbi(pci, reg, flags);
+		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
+		dw_pcie_writel_dbi(pci, reg + 4, 0);
+	} else {
+		dw_pcie_writel_dbi2(pci, reg, size - 1);
+		dw_pcie_writel_dbi(pci, reg, flags);
+	}
 	dw_pcie_dbi_ro_wr_dis(pci);
 
 	return 0;
-- 
2.14.2

[PATCH v4 3/5] PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs properly

[Niklas Cassel]

Since a 64-bit BAR consists of a BAR pair, we need to write to both
BARs in the BAR pair to clear the BAR properly.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 946bbdf53c4d..b20b2651caf9 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -22,11 +22,18 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 {
 	u32 reg;
+	u32 val;
 
 	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
+	val = dw_pcie_readl_dbi(pci, reg);
 	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writel_dbi2(pci, reg, 0x0);
 	dw_pcie_writel_dbi(pci, reg, 0x0);
+	if (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
+	    (val & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+		dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
+		dw_pcie_writel_dbi(pci, reg + 4, 0x0);
+	}
 	dw_pcie_dbi_ro_wr_dis(pci);
 }
 
-- 
2.14.2

[PATCH v4 4/5] PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was set-up

[Niklas Cassel]

In order to properly handle 64-bit BARs, we need to know what BAR width
that was actually set-up by specific pci_epc_set_bar() implementations.

This is done so that we can know if we need to skip a BAR,
since a 64-bit BAR consists of a BAR pair.

It is important to know the BAR width that was actually set-up,
since some drivers, like the Cadence EP controller, does not
simply look at PCI_BASE_ADDRESS_MEM_TYPE_64, as it configures
all BARs larger than 2G as 64-bit.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/cadence/pcie-cadence-ep.c         | 2 +-
 drivers/pci/dwc/pcie-designware-ep.c          | 4 ++--
 drivers/pci/endpoint/functions/pci-epf-test.c | 7 ++++++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index 3c3a97743453..0e4cc4cca56d 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -135,7 +135,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
 		CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, ctrl));
 	cdns_pcie_writel(pcie, reg, cfg);
 
-	return 0;
+	return is_64bits ? 1 : 0;
 }
 
 static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index b20b2651caf9..f3c19f8ff8e5 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -139,7 +139,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 		as_type = DW_PCIE_AS_IO;
 
 	ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	dw_pcie_dbi_ro_wr_en(pci);
@@ -154,7 +154,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 	}
 	dw_pcie_dbi_ro_wr_dis(pci);
 
-	return 0;
+	return (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 1 : 0;
 }
 
 static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 7c70433b11a7..09878e011284 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -379,12 +379,17 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 		ret = pci_epc_set_bar(epc, epf->func_no, bar,
 				      epf_bar->phys_addr,
 				      epf_bar->size, bar_flags[bar]);
-		if (ret) {
+		if (ret < 0) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
 			dev_err(dev, "failed to set BAR%d\n", bar);
 			if (bar == test_reg_bar)
 				return ret;
 		}
+		/*
+		 * pci_epc_set_bar() returns 1 if a 64-bit BAR was set-up,
+		 * or 0 if a 32-bit BAR was set-up.
+		 */
+		bar += ret;
 	}
 
 	return 0;
-- 
2.14.2

[PATCH v4 5/5] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Niklas Cassel]

A 64-bit BAR consists of a BAR pair, where the second BAR has the
upper bits, so we cannot simply call pci_ioremap_bar() on every single
BAR index.

The second BAR in a BAR pair will not have the IORESOURCE_MEM resource
flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
resource flag set.

pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR1
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR3
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR5

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/misc/pci_endpoint_test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 320276f42653..fe8897e64635 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -534,12 +534,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	}
 
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
-		base = pci_ioremap_bar(pdev, bar);
-		if (!base) {
-			dev_err(dev, "failed to read BAR%d\n", bar);
-			WARN_ON(bar == test_reg_bar);
+		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
+			base = pci_ioremap_bar(pdev, bar);
+			if (!base) {
+				dev_err(dev, "failed to read BAR%d\n", bar);
+				WARN_ON(bar == test_reg_bar);
+			}
+			test->bar[bar] = base;
 		}
-		test->bar[bar] = base;
 	}
 
 	test->base = test->bar[test_reg_bar];
-- 
2.14.2

Re: [PATCH v4 5/5] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Greg Kroah-Hartman]

On Thu, Mar 08, 2018 at 02:33:30PM +0100, Niklas Cassel wrote:
> A 64-bit BAR consists of a BAR pair, where the second BAR has the
> upper bits, so we cannot simply call pci_ioremap_bar() on every single
> BAR index.
> 
> The second BAR in a BAR pair will not have the IORESOURCE_MEM resource
> flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
> resource flag set.
> 
> pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
> pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
> pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR1
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR3
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR5
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Where are the 4 other patches in this series?  I can't just only apply
the last one, right?

Please be careful when sending patches to properly notify everyone...

this is now dropped from my patch queue.

greg k-h

Re: [PATCH v4 5/5] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Niklas Cassel]

On Thu, Mar 15, 2018 at 02:22:13PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 08, 2018 at 02:33:30PM +0100, Niklas Cassel wrote:
> > A 64-bit BAR consists of a BAR pair, where the second BAR has the
> > upper bits, so we cannot simply call pci_ioremap_bar() on every single
> > BAR index.
> > 
> > The second BAR in a BAR pair will not have the IORESOURCE_MEM resource
> > flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
> > resource flag set.
> > 
> > pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
> > pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
> > pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
> > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
> > pci-endpoint-test 0000:01:00.0: failed to read BAR1
> > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
> > pci-endpoint-test 0000:01:00.0: failed to read BAR3
> > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
> > pci-endpoint-test 0000:01:00.0: failed to read BAR5
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> > ---
> >  drivers/misc/pci_endpoint_test.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> Where are the 4 other patches in this series?  I can't just only apply
> the last one, right?

http://patchwork.ozlabs.org/project/linux-pci/list/?series=32658

I'm expecting all patches to go via Lorenzo's pci tree.

> 
> Please be careful when sending patches to properly notify everyone...

I'm using scripts from Joe Perches that run get_maintainer.pl,
these scripts are then used as --to-cmd and --cc-cmd for git send-email:
https://lkml.org/lkml/2016/9/14/482

(I did have to modify them so they handle rerolled patch series.)
BTW. Joe, do you have updated versions of your scripts?


Greg, I'm assuming that you wouldn't want to be cc'd on all patches in the
series, since all except the last one are pci related.
However, perhaps it would be nice to add all those who are cc'd in the
individual patches, as cc to the cover letter, that way it might be easier
to figure out what tree will most likely merge a certain patch series.



Kind regards,
Niklas

Re: [PATCH v4 1/5] PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

[Lorenzo Pieralisi]

On Thu, Mar 08, 2018 at 02:33:26PM +0100, Niklas Cassel wrote:
> If a BAR supports 64-bit width or not depends on the hardware,
> and should thus not depend on sizeof(dma_addr_t).
> 
> Since this driver is generic, default to always using BAR width
> of 32-bits. 64-bit BARs can easily be tested by replacing
> PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> in bar_flags.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
> Note to Lorenzo/Bjorn:
> It is not trivial to convert the bar_size + bar_flags +
> struct pci_epf->bar member array to an array of struct resources,
> since we need to be able to store the addresses returned
> by dma_alloc_coherent(), which is of type dma_addr_t.
> struct resource uses resource_size_t, which is defined as phys_addr_t.
> E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
> 
>  drivers/pci/endpoint/functions/pci-epf-test.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 800da09d9005..7c70433b11a7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -71,6 +71,14 @@ struct pci_epf_test_data {
>  };
>  
>  static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> +static int bar_flags[] = {
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> +};

Niklas,

I think you are almost there, I have one question though to address
that can even simplify the patchset.

If, according to your own commit logs (and my reading of the code), the
Cadence driver makes a decision on the BAR size just by checking the
corresponding region size (I would be happy to hear the reason
underpinning that choice, BTW), why can't we do the same for DWC (ie to
let the DWC driver decides whether a BAR should be 64 or 32 bits ?)

This would mean that in this patch we would not bother about the BAR
32/64 size flag at all.

Thoughts ?

Lorenzo

>  
>  static int pci_epf_test_copy(struct pci_epf_test *epf_test)
>  {
> @@ -358,7 +366,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>  
>  static int pci_epf_test_set_bar(struct pci_epf *epf)
>  {
> -	int flags;
>  	int bar;
>  	int ret;
>  	struct pci_epf_bar *epf_bar;
> @@ -367,15 +374,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  
> -	flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
> -	if (sizeof(dma_addr_t) == 0x8)
> -		flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -
>  	for (bar = BAR_0; bar <= BAR_5; bar++) {
>  		epf_bar = &epf->bar[bar];
>  		ret = pci_epc_set_bar(epc, epf->func_no, bar,
>  				      epf_bar->phys_addr,
> -				      epf_bar->size, flags);
> +				      epf_bar->size, bar_flags[bar]);
>  		if (ret) {
>  			pci_epf_free_space(epf, epf_test->reg[bar], bar);
>  			dev_err(dev, "failed to set BAR%d\n", bar);
> -- 
> 2.14.2
> 

Re: [PATCH v4 1/5] PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

[Niklas Cassel]

On Fri, Mar 16, 2018 at 06:02:20PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Mar 08, 2018 at 02:33:26PM +0100, Niklas Cassel wrote:
> > If a BAR supports 64-bit width or not depends on the hardware,
> > and should thus not depend on sizeof(dma_addr_t).
> > 
> > Since this driver is generic, default to always using BAR width
> > of 32-bits. 64-bit BARs can easily be tested by replacing
> > PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> > in bar_flags.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> > ---
> > Note to Lorenzo/Bjorn:
> > It is not trivial to convert the bar_size + bar_flags +
> > struct pci_epf->bar member array to an array of struct resources,
> > since we need to be able to store the addresses returned
> > by dma_alloc_coherent(), which is of type dma_addr_t.
> > struct resource uses resource_size_t, which is defined as phys_addr_t.
> > E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
> > 
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 800da09d9005..7c70433b11a7 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -71,6 +71,14 @@ struct pci_epf_test_data {
> >  };
> >  
> >  static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> > +static int bar_flags[] = {
> > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> > +};
> 
> Niklas,
> 
> I think you are almost there, I have one question though to address
> that can even simplify the patchset.
> 
> If, according to your own commit logs (and my reading of the code), the
> Cadence driver makes a decision on the BAR size just by checking the
> corresponding region size (I would be happy to hear the reason
> underpinning that choice, BTW), why can't we do the same for DWC (ie to
> let the DWC driver decides whether a BAR should be 64 or 32 bits ?)

We could, but I think that would be a mistake.

The API that the user/"endpoint function" has available to configure
the BARs:
pci_epc_set_bar()

If the user, for some reason, wants to configure a BAR with a
64-bit width, even though the BAR size is less than 4 GB,
I think that the API should allow that.

This would not be possible if pci_epc_set_bar() would start to
ignore PCI_BASE_ADDRESS_MEM_TYPE_64 from the flags parameter
(and BAR width thus only being controlled by the size parameter).

int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
	            dma_addr_t bar_phys, size_t size, int flags);



Best regards,
Niklas

Re: [PATCH v4 1/5] PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

[Lorenzo Pieralisi]

On Sat, Mar 17, 2018 at 01:55:13AM +0100, Niklas Cassel wrote:
> On Fri, Mar 16, 2018 at 06:02:20PM +0000, Lorenzo Pieralisi wrote:
> > On Thu, Mar 08, 2018 at 02:33:26PM +0100, Niklas Cassel wrote:
> > > If a BAR supports 64-bit width or not depends on the hardware,
> > > and should thus not depend on sizeof(dma_addr_t).
> > > 
> > > Since this driver is generic, default to always using BAR width
> > > of 32-bits. 64-bit BARs can easily be tested by replacing
> > > PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> > > in bar_flags.
> > > 
> > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> > > ---
> > > Note to Lorenzo/Bjorn:
> > > It is not trivial to convert the bar_size + bar_flags +
> > > struct pci_epf->bar member array to an array of struct resources,
> > > since we need to be able to store the addresses returned
> > > by dma_alloc_coherent(), which is of type dma_addr_t.
> > > struct resource uses resource_size_t, which is defined as phys_addr_t.
> > > E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
> > > 
> > >  drivers/pci/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > index 800da09d9005..7c70433b11a7 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > @@ -71,6 +71,14 @@ struct pci_epf_test_data {
> > >  };
> > >  
> > >  static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> > > +static int bar_flags[] = {
> > > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> > > +};
> > 
> > Niklas,
> > 
> > I think you are almost there, I have one question though to address
> > that can even simplify the patchset.
> > 
> > If, according to your own commit logs (and my reading of the code), the
> > Cadence driver makes a decision on the BAR size just by checking the
> > corresponding region size (I would be happy to hear the reason
> > underpinning that choice, BTW), why can't we do the same for DWC (ie to
> > let the DWC driver decides whether a BAR should be 64 or 32 bits ?)
> 
> We could, but I think that would be a mistake.
> 
> The API that the user/"endpoint function" has available to configure
> the BARs:
> pci_epc_set_bar()
> 
> If the user, for some reason, wants to configure a BAR with a
> 64-bit width, even though the BAR size is less than 4 GB,
> I think that the API should allow that.

Yes, that's a good point, thanks a lot for the explanation.

Lorenzo

> This would not be possible if pci_epc_set_bar() would start to
> ignore PCI_BASE_ADDRESS_MEM_TYPE_64 from the flags parameter
> (and BAR width thus only being controlled by the size parameter).
> 
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
> 	            dma_addr_t bar_phys, size_t size, int flags);
> 
> 
> 
> Best regards,
> Niklas

Re: [PATCH v4 0/5] PCI endpoint 64-bit BAR fixes

[Lorenzo Pieralisi]

On Thu, Mar 08, 2018 at 02:33:25PM +0100, Niklas Cassel wrote:
> PCI endpoint fixes to improve the way 64-bit BARs are handled.
> 
> 
> There are still future improvements that could be made:
> 
> pci-epf-test.c always allocates space for
> 6 BARs, even when using 64-bit BARs (which
> really only requires us to allocate 3 BARs).
> 
> pcitest.sh will print "NOT OKAY" for BAR1,
> BAR3, and BAR5 when using 64-bit BARs.
> This could probably be improved to say
> something like "N/A (64-bit BAR)".
> 
> Niklas Cassel (5):
>   PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
>   PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
>     properly
>   PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
>     properly
>   PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was
>     set-up
>   misc: pci_endpoint_test: Handle 64-bit BARs properly
> 
>  drivers/misc/pci_endpoint_test.c              | 12 +++++++-----
>  drivers/pci/cadence/pcie-cadence-ep.c         |  2 +-
>  drivers/pci/dwc/pcie-designware-ep.c          | 22 ++++++++++++++++++----
>  drivers/pci/endpoint/functions/pci-epf-test.c | 22 +++++++++++++++-------
>  4 files changed, 41 insertions(+), 17 deletions(-)

Niklas,

I am fine with the series but I'd need Kishon ACKs (and Greg's for the
last patch), I am ready to queue them then.

Thanks,
Lorenzo

Re: [PATCH v4 5/5] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Niklas Cassel]

Hello Greg,

Lorenzo	is fine with this series
( https://marc.info/?l=linux-kernel&m=152147837619191&w=2 )

However, he wants your ack on this patch before merging.

Could you please have a look at this patch?


Kind regards,
Niklas


On Thu, Mar 08, 2018 at 02:33:30PM +0100, Niklas Cassel wrote:
> A 64-bit BAR consists of a BAR pair, where the second BAR has the
> upper bits, so we cannot simply call pci_ioremap_bar() on every single
> BAR index.
> 
> The second BAR in a BAR pair will not have the IORESOURCE_MEM resource
> flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
> resource flag set.
> 
> pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
> pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
> pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR1
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR3
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR5
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 320276f42653..fe8897e64635 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -534,12 +534,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  	}
>  
>  	for (bar = BAR_0; bar <= BAR_5; bar++) {
> -		base = pci_ioremap_bar(pdev, bar);
> -		if (!base) {
> -			dev_err(dev, "failed to read BAR%d\n", bar);
> -			WARN_ON(bar == test_reg_bar);
> +		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> +			base = pci_ioremap_bar(pdev, bar);
> +			if (!base) {
> +				dev_err(dev, "failed to read BAR%d\n", bar);
> +				WARN_ON(bar == test_reg_bar);
> +			}
> +			test->bar[bar] = base;
>  		}
> -		test->bar[bar] = base;
>  	}
>  
>  	test->base = test->bar[test_reg_bar];
> -- 
> 2.14.2
> 

Re: [PATCH v4 0/5] PCI endpoint 64-bit BAR fixes

[Niklas Cassel]

On Mon, Mar 19, 2018 at 04:52:34PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Mar 08, 2018 at 02:33:25PM +0100, Niklas Cassel wrote:
> > PCI endpoint fixes to improve the way 64-bit BARs are handled.
> > 
> > 
> > There are still future improvements that could be made:
> > 
> > pci-epf-test.c always allocates space for
> > 6 BARs, even when using 64-bit BARs (which
> > really only requires us to allocate 3 BARs).
> > 
> > pcitest.sh will print "NOT OKAY" for BAR1,
> > BAR3, and BAR5 when using 64-bit BARs.
> > This could probably be improved to say
> > something like "N/A (64-bit BAR)".
> > 
> > Niklas Cassel (5):
> >   PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
> >   PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
> >     properly
> >   PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
> >     properly
> >   PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was
> >     set-up
> >   misc: pci_endpoint_test: Handle 64-bit BARs properly
> > 
> >  drivers/misc/pci_endpoint_test.c              | 12 +++++++-----
> >  drivers/pci/cadence/pcie-cadence-ep.c         |  2 +-
> >  drivers/pci/dwc/pcie-designware-ep.c          | 22 ++++++++++++++++++----
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 22 +++++++++++++++-------
> >  4 files changed, 41 insertions(+), 17 deletions(-)
> 
> Niklas,
> 
> I am fine with the series but I'd need Kishon ACKs (and Greg's for the
> last patch), I am ready to queue them then.


Hello Kishon,

could you please have a look at this series?


Kind regards,
Niklas

Re: [PATCH v4 1/5] PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

[Kishon Vijay Abraham I]

On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> If a BAR supports 64-bit width or not depends on the hardware,
> and should thus not depend on sizeof(dma_addr_t).
> 
> Since this driver is generic, default to always using BAR width
> of 32-bits. 64-bit BARs can easily be tested by replacing
> PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> in bar_flags.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> Note to Lorenzo/Bjorn:
> It is not trivial to convert the bar_size + bar_flags +
> struct pci_epf->bar member array to an array of struct resources,
> since we need to be able to store the addresses returned
> by dma_alloc_coherent(), which is of type dma_addr_t.
> struct resource uses resource_size_t, which is defined as phys_addr_t.
> E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
> 
>  drivers/pci/endpoint/functions/pci-epf-test.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 800da09d9005..7c70433b11a7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -71,6 +71,14 @@ struct pci_epf_test_data {
>  };
>  
>  static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> +static int bar_flags[] = {
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> +	PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> +};
>  
>  static int pci_epf_test_copy(struct pci_epf_test *epf_test)
>  {
> @@ -358,7 +366,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>  
>  static int pci_epf_test_set_bar(struct pci_epf *epf)
>  {
> -	int flags;
>  	int bar;
>  	int ret;
>  	struct pci_epf_bar *epf_bar;
> @@ -367,15 +374,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  
> -	flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
> -	if (sizeof(dma_addr_t) == 0x8)
> -		flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -
>  	for (bar = BAR_0; bar <= BAR_5; bar++) {
>  		epf_bar = &epf->bar[bar];
>  		ret = pci_epc_set_bar(epc, epf->func_no, bar,
>  				      epf_bar->phys_addr,
> -				      epf_bar->size, flags);
> +				      epf_bar->size, bar_flags[bar]);
>  		if (ret) {
>  			pci_epf_free_space(epf, epf_test->reg[bar], bar);
>  			dev_err(dev, "failed to set BAR%d\n", bar);
> 

Re: [PATCH v4 2/5] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

[Kishon Vijay Abraham I]

Hi Niklas,

On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, we need to write to both
> BARs in the BAR pair to setup the BAR properly.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 9236b998327f..946bbdf53c4d 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -136,8 +136,15 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
>  		return ret;
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
> -	dw_pcie_writel_dbi2(pci, reg, size - 1);
> -	dw_pcie_writel_dbi(pci, reg, flags);
> +	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +		dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> +		dw_pcie_writel_dbi(pci, reg, flags);
> +		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> +		dw_pcie_writel_dbi(pci, reg + 4, 0);

I think we should check in pci_epc_set_bar to make sure BAR_5 cannot have
PCI_BASE_ADDRESS_MEM_TYPE_64 flag set as this might lead undesired result.

Thanks
Kishon

Re: [PATCH v4 3/5] PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs properly

[Kishon Vijay Abraham I]

Hi Niklas,

On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, we need to write to both
> BARs in the BAR pair to clear the BAR properly.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-ep.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 946bbdf53c4d..b20b2651caf9 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -22,11 +22,18 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
>  void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>  {
>  	u32 reg;
> +	u32 val;
>  
>  	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> +	val = dw_pcie_readl_dbi(pci, reg);
>  	dw_pcie_dbi_ro_wr_en(pci);
>  	dw_pcie_writel_dbi2(pci, reg, 0x0);
>  	dw_pcie_writel_dbi(pci, reg, 0x0);
> +	if (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
> +	    (val & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> +		dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
> +		dw_pcie_writel_dbi(pci, reg + 4, 0x0);
> +	}

same comment as previous patch apply here too.

Thanks
Kishon

Re: [PATCH v4 5/5] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Kishon Vijay Abraham I]

On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> A 64-bit BAR consists of a BAR pair, where the second BAR has the
> upper bits, so we cannot simply call pci_ioremap_bar() on every single
> BAR index.
> 
> The second BAR in a BAR pair will not have the IORESOURCE_MEM resource
> flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
> resource flag set.
> 
> pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
> pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
> pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR1
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR3
> pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
> pci-endpoint-test 0000:01:00.0: failed to read BAR5
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 320276f42653..fe8897e64635 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -534,12 +534,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  	}
>  
>  	for (bar = BAR_0; bar <= BAR_5; bar++) {
> -		base = pci_ioremap_bar(pdev, bar);
> -		if (!base) {
> -			dev_err(dev, "failed to read BAR%d\n", bar);
> -			WARN_ON(bar == test_reg_bar);
> +		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> +			base = pci_ioremap_bar(pdev, bar);
> +			if (!base) {
> +				dev_err(dev, "failed to read BAR%d\n", bar);
> +				WARN_ON(bar == test_reg_bar);
> +			}
> +			test->bar[bar] = base;
>  		}
> -		test->bar[bar] = base;
>  	}
>  
>  	test->base = test->bar[test_reg_bar];
> 

Re: [PATCH v4 4/5] PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was set-up

[Kishon Vijay Abraham I]

Hi Niklas,

On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> In order to properly handle 64-bit BARs, we need to know what BAR width
> that was actually set-up by specific pci_epc_set_bar() implementations.
> 
> This is done so that we can know if we need to skip a BAR,
> since a 64-bit BAR consists of a BAR pair.
> 
> It is important to know the BAR width that was actually set-up,
> since some drivers, like the Cadence EP controller, does not
> simply look at PCI_BASE_ADDRESS_MEM_TYPE_64, as it configures
> all BARs larger than 2G as 64-bit.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/cadence/pcie-cadence-ep.c         | 2 +-
>  drivers/pci/dwc/pcie-designware-ep.c          | 4 ++--
>  drivers/pci/endpoint/functions/pci-epf-test.c | 7 ++++++-
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index 3c3a97743453..0e4cc4cca56d 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -135,7 +135,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
>  		CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, ctrl));
>  	cdns_pcie_writel(pcie, reg, cfg);
>  
> -	return 0;
> +	return is_64bits ? 1 : 0;
>  }
>  
>  static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index b20b2651caf9..f3c19f8ff8e5 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -139,7 +139,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
>  		as_type = DW_PCIE_AS_IO;
>  
>  	ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
> @@ -154,7 +154,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
>  	}
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  
> -	return 0;
> +	return (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 1 : 0;
>  }
>  
>  static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 7c70433b11a7..09878e011284 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -379,12 +379,17 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  		ret = pci_epc_set_bar(epc, epf->func_no, bar,
>  				      epf_bar->phys_addr,
>  				      epf_bar->size, bar_flags[bar]);
> -		if (ret) {
> +		if (ret < 0) {
>  			pci_epf_free_space(epf, epf_test->reg[bar], bar);
>  			dev_err(dev, "failed to set BAR%d\n", bar);
>  			if (bar == test_reg_bar)
>  				return ret;
>  		}
> +		/*
> +		 * pci_epc_set_bar() returns 1 if a 64-bit BAR was set-up,
> +		 * or 0 if a 32-bit BAR was set-up.
> +		 */
> +		bar += ret;

I'd prefer having a new argument bool *is_64bit in set_bar rather than having
to work with return value of set_bar.

Even otherwise, can't we set PCI_BASE_ADDRESS_MEM_TYPE_64 based on size in
pci-epf-test itself?

Thanks
Kishon

Re: [PATCH v4 5/5] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Greg Kroah-Hartman]

On Wed, Mar 21, 2018 at 04:59:31AM +0100, Niklas Cassel wrote:
> Hello Greg,
> 
> Lorenzo	is fine with this series
> ( https://marc.info/?l=linux-kernel&m=152147837619191&w=2 )
> 
> However, he wants your ack on this patch before merging.

No need for my ack, if you all want to maintain this driver, it's fine
with me, go ahead and merge what you want :)

greg k-h