Skip to contentSkip to conversation

PCI endpoint 64-bit BAR fixes

PATCH In review Niklas Cassel 7 participants 37 messages Mar 28, 2018 – Apr 3, 2018

Subsystems (3)
PCI Char and misc drivers PCI endpoint
lore
Share & tools
lore

Niklas Cassel

author
#0 lore raw

v5 cover PCI endpoint 64-bit BAR fixes

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 (12):
PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()
PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid
PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set
PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is
not set
PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
properly
PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was
set-up
PCI: endpoint: Handle 64-bit BARs properly
PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take
struct *epf_bar
PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when
clearing
PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
properly
misc: pci_endpoint_test: Handle 64-bit BARs properly

 drivers/misc/pci_endpoint_test.c              | 12 +++++----
 drivers/pci/cadence/pcie-cadence-ep.c         | 15 ++++++++---
 drivers/pci/dwc/pcie-designware-ep.c          | 36 +++++++++++++++++++++------
 drivers/pci/endpoint/functions/pci-epf-test.c | 28 +++++++++++++--------
 drivers/pci/endpoint/pci-epc-core.c           | 32 +++++++++++++++---------
 drivers/pci/endpoint/pci-epf-core.c           |  4 +++
 include/linux/pci-epc.h                       | 11 ++++----
 include/linux/pci-epf.h                       |  2 ++
 8 files changed, 95 insertions(+), 45 deletions(-)
Signature · 1 line
2.14.2
author
#1 lore raw

PATCH v5 1/12 PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

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

If a certain hardware doesn't support 64-bit BARs, its epc->ops->set_bar() implementation should return -EINVAL when PCI_BASE_ADDRESS_MEM_TYPE_64 is set.

We can't change pci_epc_set_bar() to only set PCI_BASE_ADDRESS_MEM_TYPE_64 based on size, since 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, he should be able to do that.

However, since pci-epf-test is simply a test and not an API, we can set PCI_BASE_ADDRESS_MEM_TYPE_64 in pci-epf-test itself only based on size.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/pci/endpoint/functions/pci-epf-test.c +7 -5
index 64d8a17f8094..f6c0c59b1bc8 100644
@@ -70,7 +70,7 @@ struct pci_epf_test_data {
 	bool		linkup_notifier;
 };
 
-static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
+static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
 
 static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 {

@@ -367,12 +367,14 @@ 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];
+
+		flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
+		flags |= upper_32_bits(epf_bar->size) ?
+			PCI_BASE_ADDRESS_MEM_TYPE_64 :
+			PCI_BASE_ADDRESS_MEM_TYPE_32;
+
 		ret = pci_epc_set_bar(epc, epf->func_no, bar,
 				      epf_bar->phys_addr,
 				      epf_bar->size, flags);
Signature · 1 line
2.14.2
Acked-by on v5 1/12
#2 lore raw

Re: [PATCH v5 01/12] PCI: endpoint: BAR width should not depend on sizeof dma_addr_t

Quoted 13 lines · 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).

If a certain hardware doesn't support 64-bit BARs, its epc->ops->set_bar() implementation should return -EINVAL when PCI_BASE_ADDRESS_MEM_TYPE_64 is set.

We can't change pci_epc_set_bar() to only set PCI_BASE_ADDRESS_MEM_TYPE_64 based on size, since 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, he should be able to do that.

Quoted 5 lines

However, since pci-epf-test is simply a test and not an API, we can set PCI_BASE_ADDRESS_MEM_TYPE_64 in pci-epf-test itself only based on size.

Signed-off-by Niklas Cassel <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 37 lines
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/pci/endpoint/functions/pci-epf-test.c +7 -5
index 64d8a17f8094..f6c0c59b1bc8 100644
@@ -70,7 +70,7 @@ struct pci_epf_test_data {
 	bool		linkup_notifier;
 };
 
-static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
+static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
 
 static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 {

@@ -367,12 +367,14 @@ 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];
+
+		flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
+		flags |= upper_32_bits(epf_bar->size) ?
+			PCI_BASE_ADDRESS_MEM_TYPE_64 :
+			PCI_BASE_ADDRESS_MEM_TYPE_32;
+
 		ret = pci_epc_set_bar(epc, epf->func_no, bar,
 				      epf_bar->phys_addr,
 				      epf_bar->size, flags);
author
#3 lore raw

PATCH v5 2/12 PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()

In reply to Niklas Cassel

Add barno and flags to struct epf_bar.
That way we can simplify epc->ops->set_bar()/pci_epc_set_bar() by passing a struct *epf_bar instead of a whole lot of arguments.

This is needed so that epc->ops->set_bar() implementations can modify BAR flags. Will be utilized in a succeeding patch.

Signed-off-by Niklas Cassel <[email protected]>

---
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/cadence/pcie-cadence-ep.c         |  9 ++++++---
 drivers/pci/dwc/pcie-designware-ep.c          |  8 +++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  8 ++------
 drivers/pci/endpoint/pci-epc-core.c           | 10 ++++------
 drivers/pci/endpoint/pci-epf-core.c           |  4 ++++
 include/linux/pci-epc.h                       |  6 ++----
 include/linux/pci-epf.h                       |  2 ++
 7 files changed, 25 insertions(+), 22 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +6 -3
index 3c3a97743453..cef36cd6b710 100644
@@ -77,16 +77,19 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn,
 	return 0;
 }
 
-static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
-				dma_addr_t bar_phys, size_t size, int flags)
+static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
+				struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	dma_addr_t bar_phys = epf_bar->phys_addr;
+	enum pci_barno bar = epf_bar->barno;
+	int flags = epf_bar->flags;
 	u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
 	u64 sz;
 
 	/* BAR size is 2^(aperture + 7) */
-	sz = max_t(size_t, size, CDNS_PCIE_EP_MIN_APERTURE);
+	sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
 	/*
 	 * roundup_pow_of_two() returns an unsigned long, which is not suited
 	 * for 64bit values.
drivers/pci/dwc/pcie-designware-ep.c +5 -3
index 9236b998327f..5a0bb53c795c 100644
@@ -117,12 +117,14 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
 }
 
 static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
-			      enum pci_barno bar,
-			      dma_addr_t bar_phys, size_t size, int flags)
+			      struct pci_epf_bar *epf_bar)
 {
 	int ret;
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar = epf_bar->barno;
+	size_t size = epf_bar->size;
+	int flags = epf_bar->flags;
 	enum dw_pcie_as_type as_type;
 	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 

@@ -131,7 +133,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 	else
 		as_type = DW_PCIE_AS_IO;
 
-	ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
+	ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
 	if (ret)
 		return ret;
drivers/pci/endpoint/functions/pci-epf-test.c +2 -6
index f6c0c59b1bc8..91274779e59f 100644
@@ -358,7 +358,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;

@@ -370,14 +369,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
 		epf_bar = &epf->bar[bar];
 
-		flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
-		flags |= upper_32_bits(epf_bar->size) ?
+		epf_bar->flags |= upper_32_bits(epf_bar->size) ?
 			PCI_BASE_ADDRESS_MEM_TYPE_64 :
 			PCI_BASE_ADDRESS_MEM_TYPE_32;
 
-		ret = pci_epc_set_bar(epc, epf->func_no, bar,
-				      epf_bar->phys_addr,
-				      epf_bar->size, flags);
+		ret = pci_epc_set_bar(epc, epf->func_no, epf_bar);
 		if (ret) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
 			dev_err(dev, "failed to set BAR%d\n", bar);
drivers/pci/endpoint/pci-epc-core.c +4 -6
index e245bba0ab53..784e33d6f229 100644
@@ -300,14 +300,12 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
  * pci_epc_set_bar() - configure BAR in order for host to assign PCI addr space
  * @epc: the EPC device on which BAR has to be configured
  * @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be configured
- * @size: the size of the addr space
- * @flags: specify memory allocation/io allocation/32bit address/64 bit address
+ * @epf_bar: the struct epf_bar that contains the BAR information
  *
  * Invoke to configure the BAR of the endpoint device.
  */
-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)
+int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
+		    struct pci_epf_bar *epf_bar)
 {
 	int ret;
 	unsigned long irq_flags;

@@ -319,7 +317,7 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
 		return 0;
 
 	spin_lock_irqsave(&epc->lock, irq_flags);
-	ret = epc->ops->set_bar(epc, func_no, bar, bar_phys, size, flags);
+	ret = epc->ops->set_bar(epc, func_no, epf_bar);
 	spin_unlock_irqrestore(&epc->lock, irq_flags);
 
 	return ret;
drivers/pci/endpoint/pci-epf-core.c +4 -0
index 59ed29e550e9..465b5f058b6d 100644
@@ -98,6 +98,8 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar)
 
 	epf->bar[bar].phys_addr = 0;
 	epf->bar[bar].size = 0;
+	epf->bar[bar].barno = 0;
+	epf->bar[bar].flags = 0;
 }
 EXPORT_SYMBOL_GPL(pci_epf_free_space);
 

@@ -126,6 +128,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
 
 	epf->bar[bar].phys_addr = phys_addr;
 	epf->bar[bar].size = size;
+	epf->bar[bar].barno = bar;
+	epf->bar[bar].flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
 
 	return space;
 }
include/linux/pci-epc.h +2 -4
index a1a5e5df0f66..75bae8aabbf9 100644
@@ -39,8 +39,7 @@ struct pci_epc_ops {
 	int	(*write_header)(struct pci_epc *epc, u8 func_no,
 				struct pci_epf_header *hdr);
 	int	(*set_bar)(struct pci_epc *epc, u8 func_no,
-			   enum pci_barno bar,
-			   dma_addr_t bar_phys, size_t size, int flags);
+			   struct pci_epf_bar *epf_bar);
 	void	(*clear_bar)(struct pci_epc *epc, u8 func_no,
 			     enum pci_barno bar);
 	int	(*map_addr)(struct pci_epc *epc, u8 func_no,

@@ -127,8 +126,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
 int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
 			 struct pci_epf_header *hdr);
 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);
+		    struct pci_epf_bar *epf_bar);
 void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
 int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
 		     phys_addr_t phys_addr,
include/linux/pci-epf.h +2 -0
index e897bf076701..f7d6f4883f8b 100644
@@ -97,6 +97,8 @@ struct pci_epf_driver {
 struct pci_epf_bar {
 	dma_addr_t	phys_addr;
 	size_t		size;
+	enum pci_barno	barno;
+	int		flags;
 };
 
 /**
Signature · 1 line
2.14.2
Reviewed-by on v5 2/12
#4 lore raw

Re: [PATCH v5 02/12] PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()

Hi Niklas,

Quoted 194 lines · Niklas Cassel

Add barno and flags to struct epf_bar.
That way we can simplify epc->ops->set_bar()/pci_epc_set_bar() by passing a struct *epf_bar instead of a whole lot of arguments.

This is needed so that epc->ops->set_bar() implementations can modify BAR flags. Will be utilized in a succeeding patch.

Signed-off-by Niklas Cassel <[email protected]>

---
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/cadence/pcie-cadence-ep.c         |  9 ++++++---
 drivers/pci/dwc/pcie-designware-ep.c          |  8 +++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  8 ++------
 drivers/pci/endpoint/pci-epc-core.c           | 10 ++++------
 drivers/pci/endpoint/pci-epf-core.c           |  4 ++++
 include/linux/pci-epc.h                       |  6 ++----
 include/linux/pci-epf.h                       |  2 ++
 7 files changed, 25 insertions(+), 22 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +6 -3
index 3c3a97743453..cef36cd6b710 100644
@@ -77,16 +77,19 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn,
 	return 0;
 }
 
-static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
-				dma_addr_t bar_phys, size_t size, int flags)
+static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
+				struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	dma_addr_t bar_phys = epf_bar->phys_addr;
+	enum pci_barno bar = epf_bar->barno;
+	int flags = epf_bar->flags;
 	u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
 	u64 sz;
 
 	/* BAR size is 2^(aperture + 7) */
-	sz = max_t(size_t, size, CDNS_PCIE_EP_MIN_APERTURE);
+	sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
 	/*
 	 * roundup_pow_of_two() returns an unsigned long, which is not suited
 	 * for 64bit values.
drivers/pci/dwc/pcie-designware-ep.c +5 -3
index 9236b998327f..5a0bb53c795c 100644
@@ -117,12 +117,14 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
 }
 
 static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
-			      enum pci_barno bar,
-			      dma_addr_t bar_phys, size_t size, int flags)
+			      struct pci_epf_bar *epf_bar)
 {
 	int ret;
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar = epf_bar->barno;
+	size_t size = epf_bar->size;
+	int flags = epf_bar->flags;
 	enum dw_pcie_as_type as_type;
 	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 

@@ -131,7 +133,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 	else
 		as_type = DW_PCIE_AS_IO;
 
-	ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
+	ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
 	if (ret)
 		return ret;
drivers/pci/endpoint/functions/pci-epf-test.c +2 -6
index f6c0c59b1bc8..91274779e59f 100644
@@ -358,7 +358,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;

@@ -370,14 +369,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
 		epf_bar = &epf->bar[bar];
 
-		flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
-		flags |= upper_32_bits(epf_bar->size) ?
+		epf_bar->flags |= upper_32_bits(epf_bar->size) ?
 			PCI_BASE_ADDRESS_MEM_TYPE_64 :
 			PCI_BASE_ADDRESS_MEM_TYPE_32;
 
-		ret = pci_epc_set_bar(epc, epf->func_no, bar,
-				      epf_bar->phys_addr,
-				      epf_bar->size, flags);
+		ret = pci_epc_set_bar(epc, epf->func_no, epf_bar);
 		if (ret) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
 			dev_err(dev, "failed to set BAR%d\n", bar);
drivers/pci/endpoint/pci-epc-core.c +4 -6
index e245bba0ab53..784e33d6f229 100644
@@ -300,14 +300,12 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
  * pci_epc_set_bar() - configure BAR in order for host to assign PCI addr space
  * @epc: the EPC device on which BAR has to be configured
  * @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be configured
- * @size: the size of the addr space
- * @flags: specify memory allocation/io allocation/32bit address/64 bit address
+ * @epf_bar: the struct epf_bar that contains the BAR information
  *
  * Invoke to configure the BAR of the endpoint device.
  */
-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)
+int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
+		    struct pci_epf_bar *epf_bar)
 {
 	int ret;
 	unsigned long irq_flags;

@@ -319,7 +317,7 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
 		return 0;
 
 	spin_lock_irqsave(&epc->lock, irq_flags);
-	ret = epc->ops->set_bar(epc, func_no, bar, bar_phys, size, flags);
+	ret = epc->ops->set_bar(epc, func_no, epf_bar);
 	spin_unlock_irqrestore(&epc->lock, irq_flags);
 
 	return ret;
drivers/pci/endpoint/pci-epf-core.c +4 -0
index 59ed29e550e9..465b5f058b6d 100644
@@ -98,6 +98,8 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar)
 
 	epf->bar[bar].phys_addr = 0;
 	epf->bar[bar].size = 0;
+	epf->bar[bar].barno = 0;
+	epf->bar[bar].flags = 0;
 }
 EXPORT_SYMBOL_GPL(pci_epf_free_space);
 

@@ -126,6 +128,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
 
 	epf->bar[bar].phys_addr = phys_addr;
 	epf->bar[bar].size = size;
+	epf->bar[bar].barno = bar;
+	epf->bar[bar].flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
 
 	return space;
 }
include/linux/pci-epc.h +2 -4
index a1a5e5df0f66..75bae8aabbf9 100644
@@ -39,8 +39,7 @@ struct pci_epc_ops {
 	int	(*write_header)(struct pci_epc *epc, u8 func_no,
 				struct pci_epf_header *hdr);
 	int	(*set_bar)(struct pci_epc *epc, u8 func_no,
-			   enum pci_barno bar,
-			   dma_addr_t bar_phys, size_t size, int flags);
+			   struct pci_epf_bar *epf_bar);
 	void	(*clear_bar)(struct pci_epc *epc, u8 func_no,
 			     enum pci_barno bar);
 	int	(*map_addr)(struct pci_epc *epc, u8 func_no,

@@ -127,8 +126,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
 int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
 			 struct pci_epf_header *hdr);
 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);
+		    struct pci_epf_bar *epf_bar);
 void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
 int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
 		     phys_addr_t phys_addr,
include/linux/pci-epf.h +0 -0
index e897bf076701..f7d6f4883f8b 100644

include/linux/pci-epf.h struct pci_epf_driver { · line 104

Quoted 8 lines
include/linux/pci-epf.h +2 -0
@@ -97,6 +97,8 @@ struct pci_epf_driver {
 struct pci_epf_bar {
 	dma_addr_t	phys_addr;
 	size_t		size;
+	enum pci_barno	barno;
+	int		flags;
 };
 
 /**

Seems good to me. :)

Reviewed-by Gustavo Pimentel <[email protected]>
Acked-by on v5 2/12
#5 lore raw

Re: [PATCH v5 02/12] PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()

In reply to Niklas Cassel
Quoted 8 lines · Niklas Cassel

Add barno and flags to struct epf_bar.
That way we can simplify epc->ops->set_bar()/pci_epc_set_bar() by passing a struct *epf_bar instead of a whole lot of arguments.

This is needed so that epc->ops->set_bar() implementations can modify BAR flags. Will be utilized in a succeeding patch.

Signed-off-by Niklas Cassel <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 194 lines

---
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/cadence/pcie-cadence-ep.c         |  9 ++++++---
 drivers/pci/dwc/pcie-designware-ep.c          |  8 +++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  8 ++------
 drivers/pci/endpoint/pci-epc-core.c           | 10 ++++------
 drivers/pci/endpoint/pci-epf-core.c           |  4 ++++
 include/linux/pci-epc.h                       |  6 ++----
 include/linux/pci-epf.h                       |  2 ++
 7 files changed, 25 insertions(+), 22 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +6 -3
index 3c3a97743453..cef36cd6b710 100644
@@ -77,16 +77,19 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn,
 	return 0;
 }
 
-static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
-				dma_addr_t bar_phys, size_t size, int flags)
+static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
+				struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	dma_addr_t bar_phys = epf_bar->phys_addr;
+	enum pci_barno bar = epf_bar->barno;
+	int flags = epf_bar->flags;
 	u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
 	u64 sz;
 
 	/* BAR size is 2^(aperture + 7) */
-	sz = max_t(size_t, size, CDNS_PCIE_EP_MIN_APERTURE);
+	sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
 	/*
 	 * roundup_pow_of_two() returns an unsigned long, which is not suited
 	 * for 64bit values.
drivers/pci/dwc/pcie-designware-ep.c +5 -3
index 9236b998327f..5a0bb53c795c 100644
@@ -117,12 +117,14 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
 }
 
 static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
-			      enum pci_barno bar,
-			      dma_addr_t bar_phys, size_t size, int flags)
+			      struct pci_epf_bar *epf_bar)
 {
 	int ret;
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar = epf_bar->barno;
+	size_t size = epf_bar->size;
+	int flags = epf_bar->flags;
 	enum dw_pcie_as_type as_type;
 	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 

@@ -131,7 +133,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 	else
 		as_type = DW_PCIE_AS_IO;
 
-	ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
+	ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
 	if (ret)
 		return ret;
drivers/pci/endpoint/functions/pci-epf-test.c +2 -6
index f6c0c59b1bc8..91274779e59f 100644
@@ -358,7 +358,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;

@@ -370,14 +369,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
 		epf_bar = &epf->bar[bar];
 
-		flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
-		flags |= upper_32_bits(epf_bar->size) ?
+		epf_bar->flags |= upper_32_bits(epf_bar->size) ?
 			PCI_BASE_ADDRESS_MEM_TYPE_64 :
 			PCI_BASE_ADDRESS_MEM_TYPE_32;
 
-		ret = pci_epc_set_bar(epc, epf->func_no, bar,
-				      epf_bar->phys_addr,
-				      epf_bar->size, flags);
+		ret = pci_epc_set_bar(epc, epf->func_no, epf_bar);
 		if (ret) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
 			dev_err(dev, "failed to set BAR%d\n", bar);
drivers/pci/endpoint/pci-epc-core.c +4 -6
index e245bba0ab53..784e33d6f229 100644
@@ -300,14 +300,12 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
  * pci_epc_set_bar() - configure BAR in order for host to assign PCI addr space
  * @epc: the EPC device on which BAR has to be configured
  * @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be configured
- * @size: the size of the addr space
- * @flags: specify memory allocation/io allocation/32bit address/64 bit address
+ * @epf_bar: the struct epf_bar that contains the BAR information
  *
  * Invoke to configure the BAR of the endpoint device.
  */
-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)
+int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
+		    struct pci_epf_bar *epf_bar)
 {
 	int ret;
 	unsigned long irq_flags;

@@ -319,7 +317,7 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
 		return 0;
 
 	spin_lock_irqsave(&epc->lock, irq_flags);
-	ret = epc->ops->set_bar(epc, func_no, bar, bar_phys, size, flags);
+	ret = epc->ops->set_bar(epc, func_no, epf_bar);
 	spin_unlock_irqrestore(&epc->lock, irq_flags);
 
 	return ret;
drivers/pci/endpoint/pci-epf-core.c +4 -0
index 59ed29e550e9..465b5f058b6d 100644
@@ -98,6 +98,8 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar)
 
 	epf->bar[bar].phys_addr = 0;
 	epf->bar[bar].size = 0;
+	epf->bar[bar].barno = 0;
+	epf->bar[bar].flags = 0;
 }
 EXPORT_SYMBOL_GPL(pci_epf_free_space);
 

@@ -126,6 +128,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
 
 	epf->bar[bar].phys_addr = phys_addr;
 	epf->bar[bar].size = size;
+	epf->bar[bar].barno = bar;
+	epf->bar[bar].flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
 
 	return space;
 }
include/linux/pci-epc.h +2 -4
index a1a5e5df0f66..75bae8aabbf9 100644
@@ -39,8 +39,7 @@ struct pci_epc_ops {
 	int	(*write_header)(struct pci_epc *epc, u8 func_no,
 				struct pci_epf_header *hdr);
 	int	(*set_bar)(struct pci_epc *epc, u8 func_no,
-			   enum pci_barno bar,
-			   dma_addr_t bar_phys, size_t size, int flags);
+			   struct pci_epf_bar *epf_bar);
 	void	(*clear_bar)(struct pci_epc *epc, u8 func_no,
 			     enum pci_barno bar);
 	int	(*map_addr)(struct pci_epc *epc, u8 func_no,

@@ -127,8 +126,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
 int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
 			 struct pci_epf_header *hdr);
 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);
+		    struct pci_epf_bar *epf_bar);
 void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
 int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
 		     phys_addr_t phys_addr,
include/linux/pci-epf.h +2 -0
index e897bf076701..f7d6f4883f8b 100644
@@ -97,6 +97,8 @@ struct pci_epf_driver {
 struct pci_epf_bar {
 	dma_addr_t	phys_addr;
 	size_t		size;
+	enum pci_barno	barno;
+	int		flags;
 };
 
 /**
author
#6 lore raw

PATCH v5 3/12 PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid

In reply to Niklas Cassel

Since a 64-bit BAR consists of a BAR pair, and since there is no BAR after BAR_5, BAR_5 cannot be 64-bits wide.

This sanity check is done in pci_epc_set_bar(), so that we don't need to do this sanity check in all epc->ops->set_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/pci-epc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +3 -1
index 784e33d6f229..109d75f0b7d2 100644
@@ -310,7 +310,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 	int ret;
 	unsigned long irq_flags;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
+	    (epf_bar->barno == BAR_5 &&
+	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
 		return -EINVAL;
 
 	if (!epc->ops->set_bar)
Signature · 1 line
2.14.2
Acked-by on v5 3/12
#7 lore raw

Re: [PATCH v5 03/12] PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid

Quoted 16 lines · Niklas Cassel

Since a 64-bit BAR consists of a BAR pair, and since there is no BAR after BAR_5, BAR_5 cannot be 64-bits wide.

This sanity check is done in pci_epc_set_bar(), so that we don't need to do this sanity check in all epc->ops->set_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/pci-epc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +0 -0
index 784e33d6f229..109d75f0b7d2 100644

drivers/pci/endpoint/pci-epc-core.c int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, · line 316

Quoted 8 lines
drivers/pci/endpoint/pci-epc-core.c +3 -1
@@ -310,7 +310,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 	int ret;
 	unsigned long irq_flags;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
+	    (epf_bar->barno == BAR_5 &&
+	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
 		return -EINVAL;

It's getting a bit lengthy. I'd prefer two separate ifs as that might be legible. But otherwise

Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 3 lines
drivers/pci/endpoint/pci-epc-core.c +0 -0
 	if (!epc->ops->set_bar)
author
#8 lore raw

PATCH v5 4/12 PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set

In reply to Niklas Cassel

If flag PCI_BASE_ADDRESS_SPACE_IO is set, also having any PCI_BASE_ADDRESS_MEM_* bit set is invalid.

This sanity check is done in pci_epc_set_bar(), so that we don't need to do this sanity check in all epc->ops->set_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/pci-epc-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +4 -1
index 109d75f0b7d2..40eea20d21f9 100644
@@ -309,10 +309,13 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 {
 	int ret;
 	unsigned long irq_flags;
+	int flags = epf_bar->flags;
 
 	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
 	    (epf_bar->barno == BAR_5 &&
-	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+	     flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
+	    (flags & PCI_BASE_ADDRESS_SPACE_IO &&
+	     flags & PCI_BASE_ADDRESS_IO_MASK))
 		return -EINVAL;
 
 	if (!epc->ops->set_bar)
Signature · 1 line
2.14.2
Acked-by on v5 4/12
#9 lore raw

Re: [PATCH v5 04/12] PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set

Quoted 18 lines · Niklas Cassel

If flag PCI_BASE_ADDRESS_SPACE_IO is set, also having any PCI_BASE_ADDRESS_MEM_* bit set is invalid.

This sanity check is done in pci_epc_set_bar(), so that we don't need to do this sanity check in all epc->ops->set_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/pci-epc-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +0 -0
index 109d75f0b7d2..40eea20d21f9 100644
@@ -309,10 +309,13 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 {
 	int ret;

drivers/pci/endpoint/pci-epc-core.c int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, · line 319

Quoted 10 lines
drivers/pci/endpoint/pci-epc-core.c +4 -1
 	unsigned long irq_flags;
+	int flags = epf_bar->flags;
 
 	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
 	    (epf_bar->barno == BAR_5 &&
-	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+	     flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
+	    (flags & PCI_BASE_ADDRESS_SPACE_IO &&
+	     flags & PCI_BASE_ADDRESS_IO_MASK))
 		return -EINVAL;

If possible I'd like this to be split. But otherwise

Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 3 lines
drivers/pci/endpoint/pci-epc-core.c +0 -0
 	if (!epc->ops->set_bar)
author
#10 lore raw

PATCH v5 5/12 PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is not set

In reply to Niklas Cassel

Setting a BAR size > 4 GB is invalid if PCI_BASE_ADDRESS_MEM_TYPE_64 flag is not set.

This sanity check is done in pci_epc_set_bar(), so that we don't need to do this sanity check in all epc->ops->set_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/pci-epc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +3 -1
index 40eea20d21f9..8637822605ff 100644
@@ -315,7 +315,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 	    (epf_bar->barno == BAR_5 &&
 	     flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
 	    (flags & PCI_BASE_ADDRESS_SPACE_IO &&
-	     flags & PCI_BASE_ADDRESS_IO_MASK))
+	     flags & PCI_BASE_ADDRESS_IO_MASK) ||
+	    (upper_32_bits(epf_bar->size) &&
+	     !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
 		return -EINVAL;
 
 	if (!epc->ops->set_bar)
Signature · 1 line
2.14.2
Acked-by on v5 5/12
#11 lore raw

Re: [PATCH v5 05/12] PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is not set

Quoted 7 lines · Niklas Cassel

Setting a BAR size > 4 GB is invalid if PCI_BASE_ADDRESS_MEM_TYPE_64 flag is not set.

This sanity check is done in pci_epc_set_bar(), so that we don't need to do this sanity check in all epc->ops->set_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 20 lines
---
 drivers/pci/endpoint/pci-epc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +3 -1
index 40eea20d21f9..8637822605ff 100644
@@ -315,7 +315,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 	    (epf_bar->barno == BAR_5 &&
 	     flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
 	    (flags & PCI_BASE_ADDRESS_SPACE_IO &&
-	     flags & PCI_BASE_ADDRESS_IO_MASK))
+	     flags & PCI_BASE_ADDRESS_IO_MASK) ||
+	    (upper_32_bits(epf_bar->size) &&
+	     !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
 		return -EINVAL;
 
 	if (!epc->ops->set_bar)
author
#12 lore raw

PATCH v5 6/12 PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

In reply to 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +9 -2
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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;
Signature · 1 line
2.14.2
Reviewed-by on v5 6/12
#13 lore raw

Re: [PATCH v5 06/12] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

Hi Niklas,

Quoted 21 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +2 -2
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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));

drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, · line 152

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +7 -0
+		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;

Seems good to me. :)

Reviewed-by Gustavo Pimentel <[email protected]>
#14 lore raw

Re: [PATCH v5 06/12] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

In reply to Niklas Cassel

Hi,

Quoted 17 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +0 -1
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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);

drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, · line 149

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +9 -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);
+	}

I think this should work too?

	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
	dw_pcie_writel_dbi(pci, reg, flags);

	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
		dw_pcie_writel_dbi(pci, reg + 4, 0);
	}

Thanks
Kishon

#15 lore raw

Re: [PATCH v5 06/12] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

Quoted 33 lines · Kishon Vijay Abraham I

Hi,

Quoted 17 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +0 -1
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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);

drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, · line 149

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +9 -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);
+	}

I think this should work too?

Quoted 8 lines
	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
	dw_pcie_writel_dbi(pci, reg, flags);

	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
		dw_pcie_writel_dbi(pci, reg + 4, 0);
	}

Hello Kishon,

I agree, your suggestion is more neat.

Kishon, please tell me if you insist that the long if-statement in pci_epc_set_bar() should be split, since there are 5 different conditions. Because imho, having 5 succeeding if-statements isn't clearer than having 1 long if-statement.

If Kishon agrees with me, then the review comment in this mail seems to be the only review comment.
And in that case, perhaps Lorenzo wouldn't mind fixing this up. Or perhaps Lorenzo prefers if I reroll the whole patch series?

Kind regards,
Niklas

#16 lore raw

Re: [PATCH v5 06/12] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

Quoted 43 lines · Niklas Cassel
Quoted 33 lines · Kishon Vijay Abraham I

Hi,

Quoted 17 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +0 -1
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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);

drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, · line 149

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +9 -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);
+	}

I think this should work too?

Quoted 8 lines
	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
	dw_pcie_writel_dbi(pci, reg, flags);

	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
		dw_pcie_writel_dbi(pci, reg + 4, 0);
	}
Quoted 8 lines

Hello Kishon,

I agree, your suggestion is more neat.

Kishon, please tell me if you insist that the long if-statement in pci_epc_set_bar() should be split, since there are 5 different conditions. Because imho, having 5 succeeding if-statements isn't

I'm okay as it is as well if Lorenzo/Bjorn is also fine with it.

Thanks
Kishon

#17 lore raw

Re: [PATCH v5 06/12] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

In reply to Niklas Cassel
Quoted 53 lines · Niklas Cassel
Quoted 33 lines · Kishon Vijay Abraham I

Hi,

Quoted 17 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +0 -1
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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);

drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, · line 149

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +9 -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);
+	}

I think this should work too?

Quoted 8 lines
	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
	dw_pcie_writel_dbi(pci, reg, flags);

	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
		dw_pcie_writel_dbi(pci, reg + 4, 0);
	}

Hello Kishon,

I agree, your suggestion is more neat.

Kishon, please tell me if you insist that the long if-statement in pci_epc_set_bar() should be split, since there are 5 different conditions. Because imho, having 5 succeeding if-statements isn't clearer than having 1 long if-statement.

Quoted 4 lines

If Kishon agrees with me, then the review comment in this mail seems to be the only review comment.
And in that case, perhaps Lorenzo wouldn't mind fixing this up. Or perhaps Lorenzo prefers if I reroll the whole patch series?

I updated it myself in my pci/endpoint branch, please have a look, I can't guarantee we can merge this for this cycle though, I will ask Bjorn; apologies I could not be online for a while.

Lorenzo

Quoted 2 lines

Kind regards,
Niklas

#18 lore raw

Re: [PATCH v5 06/12] PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs properly

Quoted 54 lines · Lorenzo Pieralisi
Quoted 53 lines · Niklas Cassel
Quoted 33 lines · Kishon Vijay Abraham I

Hi,

Quoted 17 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +0 -1
index 5a0bb53c795c..571b90f88d84 100644
@@ -138,8 +138,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);

drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, · line 149

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +9 -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);
+	}

I think this should work too?

Quoted 8 lines
	dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
	dw_pcie_writel_dbi(pci, reg, flags);

	if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
		dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
		dw_pcie_writel_dbi(pci, reg + 4, 0);
	}

Hello Kishon,

I agree, your suggestion is more neat.

Kishon, please tell me if you insist that the long if-statement in pci_epc_set_bar() should be split, since there are 5 different conditions. Because imho, having 5 succeeding if-statements isn't clearer than having 1 long if-statement.

Quoted 8 lines
Quoted 4 lines

If Kishon agrees with me, then the review comment in this mail seems to be the only review comment.
And in that case, perhaps Lorenzo wouldn't mind fixing this up. Or perhaps Lorenzo prefers if I reroll the whole patch series?

I updated it myself in my pci/endpoint branch, please have a look, I can't guarantee we can merge this for this cycle though, I will ask Bjorn; apologies I could not be online for a while.

Hello Lorenzo,

your pci/endpoint branch looks good.

There is no rush, merge it whenever you think is best.

Have in mind that there is EP support @ patchwork for Rockchip and for pcie-designware-plat, so make sure to juggle all the branches with care :)

Kind regards,
Niklas

author
#19 lore raw

PATCH v5 7/12 PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was set-up

In reply to Niklas Cassel

cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means that a 64-bit BAR can be set-up, even when the flag PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.

If a 64-bit BAR was set-up, set the flag PCI_BASE_ADDRESS_MEM_TYPE_64, so that the calling function can know what BAR width that was actually set-up.

I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to fix, since there might be a reason why this flag is ignored.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/pci/cadence/pcie-cadence-ep.c +3 -0
index cef36cd6b710..2905e098678c 100644
@@ -106,6 +106,9 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
 		if (is_64bits && (bar & 1))
 			return -EINVAL;
 
+		if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
+
 		if (is_64bits && is_prefetch)
 			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
 		else if (is_prefetch)
Signature · 1 line
2.14.2
#20 lore raw

RE: [PATCH v5 07/12] PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was set-up

Quoted 2 lines · Niklas Cassel

cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means th=

at a 64-bit BAR can be set-up, even when the flag

Quoted 3 lines

PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.

If a 64-bit BAR was set-up, set the flag PCI_BASE_ADDRESS_MEM_TYPE_64, so=

that the calling function can know what BAR width that was actually set-up=
.

Quoted 1 line

I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag PCI_BASE_AD=

DRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to fix, since there = might be a reason why > this flag is ignored.
Will investigate and fix this in future patch

drivers/pci/cadence/=

Quoted 6 lines
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
 1 file changed, 3 insertions(+)

=20

drivers/pci/cadence/= renamed from drivers/pci/cadence/pcie-cadence-ep.c +0 -0

pcie-cadence-ep.c

drivers/pci/cadence/pcie-cadence-ep.c static int cdns_pcie_ep_set_bar(struct pci_epc *epc, =

Quoted 4 lines

index cef36cd6b710..2905e098678c 100644

drivers/pci/cadence/pcie-cadence-ep.c +0 -0
@@ -106,6 +106,9 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, =

u8 fn,

Quoted 11 lines
drivers/pci/cadence/pcie-cadence-ep.c +0 -0
 		if (is_64bits && (bar & 1))
 			return -EINVAL;

=20

drivers/pci/cadence/pcie-cadence-ep.c +3 -1
+		if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+			epf_bar->flags |=3D PCI_BASE_ADDRESS_MEM_TYPE_64;
+
 		if (is_64bits && is_prefetch)
 			ctrl =3D CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
 		else if (is_prefetch)
--

2.14.2

Change looks good to me.

Acked-by Alan Douglas
#21 lore raw

Re: [PATCH v5 07/12] PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was set-up

[+cc Lorenzo]

Quoted 22 lines · Alan Douglas
Quoted 7 lines · Niklas Cassel

cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means that a 64-bit BAR can be set-up, even when the flag PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.

If a 64-bit BAR was set-up, set the flag PCI_BASE_ADDRESS_MEM_TYPE_64, so that the calling function can know what BAR width that was actually set-up.

I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to fix, since there might be a reason why > this flag is ignored.

Will investigate and fix this in future patch

Quoted 13 lines
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/pci/cadence/pcie-cadence-ep.c +0 -0
index cef36cd6b710..2905e098678c 100644
@@ -106,6 +106,9 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
 		if (is_64bits && (bar & 1))
 			return -EINVAL;
Quoted 9 lines
Quoted 8 lines
drivers/pci/cadence/pcie-cadence-ep.c +3 -1
+		if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
+
 		if (is_64bits && is_prefetch)
 			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
 		else if (is_prefetch)
--

2.14.2

Change looks good to me.

It will be helpful to Lorenzo if you spell this out, e.g.,

Acked-by Alan Douglas <[email protected]>

Also, it looks like we need a MAINTAINERS update to add drivers/pci/cadence/ to the "PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" filename patterns.

Acked-by
#22 lore raw

RE: [PATCH v5 07/12] PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was set-up

Quoted 20 lines · Bjorn Helgaas
Quoted 6 lines · Alan Douglas
Quoted 6 lines · Niklas Cassel

cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means that a 64-bit BAR can be set-up, even when the flag PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.

If a 64-bit BAR was set-up, set the flag

PCI_BASE_ADDRESS_MEM_TYPE_64, so that the calling function can know what BAR width that was actually set-up.

Quoted 2 lines
Quoted 2 lines

I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag

PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to fix, since there might be a reason why > this flag is ignored.

Quoted 7 lines

Will investigate and fix this in future patch

Quoted 5 lines
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
 1 file changed, 3 insertions(+)
Quoted 5 lines
Quoted 1 line

Change looks good to me.

=20
It will be helpful to Lorenzo if you spell this out, e.g., =20

Acked-by Alan Douglas <[email protected]>
Acked-by Alan Douglas <[email protected]>
Quoted 2 lines

=20
Also, it looks like we need a MAINTAINERS update to add drivers/pci/caden=

ce/

Quoted 2 lines

to the "PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" filename patterns.

Lorenzo please let me know if I should create a patch for this, it sounds g= ood to me.

author
#23 lore raw

PATCH v5 8/12 PCI: endpoint: Handle 64-bit BARs properly

In reply to Niklas Cassel

If a 64-bit BAR was set-up, we need to skip a BAR, since a 64-bit BAR consists of a BAR pair.

We need to check what BAR width the epc->ops->set_bar() specific implementation actually did set-up, since some drivers, like the Cadence EP controller, sometimes sets up a 64-bit BAR, even though a 32-bit BAR was requested.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/pci/endpoint/functions/pci-epf-test.c +7 -0
index 91274779e59f..d46e3ebabb8e 100644
@@ -380,6 +380,13 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 			if (bar == test_reg_bar)
 				return ret;
 		}
+		/*
+		 * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
+		 * if the specific implementation required a 64-bit BAR,
+		 * even if we only requested a 32-bit BAR.
+		 */
+		if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+			bar++;
 	}
 
 	return 0;
Signature · 1 line
2.14.2
Acked-by on v5 8/12
#24 lore raw

Re: [PATCH v5 08/12] PCI: endpoint: Handle 64-bit BARs properly

Quoted 9 lines · Niklas Cassel

If a 64-bit BAR was set-up, we need to skip a BAR, since a 64-bit BAR consists of a BAR pair.

We need to check what BAR width the epc->ops->set_bar() specific implementation actually did set-up, since some drivers, like the Cadence EP controller, sometimes sets up a 64-bit BAR, even though a 32-bit BAR was requested.

Signed-off-by Niklas Cassel <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 23 lines
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/pci/endpoint/functions/pci-epf-test.c +7 -0
index 91274779e59f..d46e3ebabb8e 100644
@@ -380,6 +380,13 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 			if (bar == test_reg_bar)
 				return ret;
 		}
+		/*
+		 * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
+		 * if the specific implementation required a 64-bit BAR,
+		 * even if we only requested a 32-bit BAR.
+		 */
+		if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+			bar++;
 	}
 
 	return 0;
author
#25 lore raw

PATCH v5 9/12 PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar

In reply to Niklas Cassel

Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.

This is needed so that epc->ops->clear_bar() can clear the BAR pair, if the BAR is 64-bits wide.

This also makes it possible for pci_epc_clear_bar() to sanity check the flags.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c         |  3 ++-
 drivers/pci/dwc/pcie-designware-ep.c          | 13 ++++++++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  5 ++++-
 drivers/pci/endpoint/pci-epc-core.c           |  7 ++++---
 include/linux/pci-epc.h                       |  5 +++--
 5 files changed, 23 insertions(+), 10 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +2 -1
index 2905e098678c..3d8283e450a9 100644
@@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
 }
 
 static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
-				   enum pci_barno bar)
+				   struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	enum pci_barno bar = epf_bar->barno;
 	u32 reg, cfg, b, ctrl;
 
 	if (bar < BAR_4) {
drivers/pci/dwc/pcie-designware-ep.c +10 -3
index 571b90f88d84..cc4d8381c1dc 100644
@@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 	pci_epc_linkup(epc);
 }
 
-void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
+				   int flags)
 {
 	u32 reg;
 

@@ -30,6 +31,11 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 	dw_pcie_dbi_ro_wr_dis(pci);
 }
 
+void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+{
+	__dw_pcie_ep_reset_bar(pci, bar, 0);
+}
+
 static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
 				   struct pci_epf_header *hdr)
 {

@@ -104,13 +110,14 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr,
 }
 
 static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
-				 enum pci_barno bar)
+				 struct pci_epf_bar *epf_bar)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar = epf_bar->barno;
 	u32 atu_index = ep->bar_to_atu[bar];
 
-	dw_pcie_ep_reset_bar(pci, bar);
+	__dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
 
 	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
 	clear_bit(atu_index, ep->ib_window_map);
drivers/pci/endpoint/functions/pci-epf-test.c +4 -1
index d46e3ebabb8e..7cef85124325 100644
@@ -344,14 +344,17 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
 	struct pci_epc *epc = epf->epc;
+	struct pci_epf_bar *epf_bar;
 	int bar;
 
 	cancel_delayed_work(&epf_test->cmd_handler);
 	pci_epc_stop(epc);
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
+		epf_bar = &epf->bar[bar];
+
 		if (epf_test->reg[bar]) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
-			pci_epc_clear_bar(epc, epf->func_no, bar);
+			pci_epc_clear_bar(epc, epf->func_no, epf_bar);
 		}
 	}
 }
drivers/pci/endpoint/pci-epc-core.c +4 -3
index 8637822605ff..eccc942043cb 100644
@@ -276,11 +276,12 @@ EXPORT_SYMBOL_GPL(pci_epc_map_addr);
  * pci_epc_clear_bar() - reset the BAR
  * @epc: the EPC device for which the BAR has to be cleared
  * @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be reset
+ * @epf_bar: the struct epf_bar that contains the BAR information
  *
  * Invoke to reset the BAR of the endpoint device.
  */
-void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
+void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
+		       struct pci_epf_bar *epf_bar)
 {
 	unsigned long flags;
 

@@ -291,7 +292,7 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
 		return;
 
 	spin_lock_irqsave(&epc->lock, flags);
-	epc->ops->clear_bar(epc, func_no, bar);
+	epc->ops->clear_bar(epc, func_no, epf_bar);
 	spin_unlock_irqrestore(&epc->lock, flags);
 }
 EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
include/linux/pci-epc.h +3 -2
index 75bae8aabbf9..af657ca58b70 100644
@@ -41,7 +41,7 @@ struct pci_epc_ops {
 	int	(*set_bar)(struct pci_epc *epc, u8 func_no,
 			   struct pci_epf_bar *epf_bar);
 	void	(*clear_bar)(struct pci_epc *epc, u8 func_no,
-			     enum pci_barno bar);
+			     struct pci_epf_bar *epf_bar);
 	int	(*map_addr)(struct pci_epc *epc, u8 func_no,
 			    phys_addr_t addr, u64 pci_addr, size_t size);
 	void	(*unmap_addr)(struct pci_epc *epc, u8 func_no,

@@ -127,7 +127,8 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
 			 struct pci_epf_header *hdr);
 int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 		    struct pci_epf_bar *epf_bar);
-void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
+void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
+		       struct pci_epf_bar *epf_bar);
 int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
 		     phys_addr_t phys_addr,
 		     u64 pci_addr, size_t size);
Signature · 1 line
2.14.2
Reviewed-by on v5 9/12
#26 lore raw

Re: [PATCH v5 09/12] PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar

Hi Niklas,

Quoted 143 lines · Niklas Cassel

Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.

This is needed so that epc->ops->clear_bar() can clear the BAR pair, if the BAR is 64-bits wide.

This also makes it possible for pci_epc_clear_bar() to sanity check the flags.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c         |  3 ++-
 drivers/pci/dwc/pcie-designware-ep.c          | 13 ++++++++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  5 ++++-
 drivers/pci/endpoint/pci-epc-core.c           |  7 ++++---
 include/linux/pci-epc.h                       |  5 +++--
 5 files changed, 23 insertions(+), 10 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +2 -1
index 2905e098678c..3d8283e450a9 100644
@@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
 }
 
 static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
-				   enum pci_barno bar)
+				   struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	enum pci_barno bar = epf_bar->barno;
 	u32 reg, cfg, b, ctrl;
 
 	if (bar < BAR_4) {
drivers/pci/dwc/pcie-designware-ep.c +10 -3
index 571b90f88d84..cc4d8381c1dc 100644
@@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 	pci_epc_linkup(epc);
 }
 
-void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
+				   int flags)
 {
 	u32 reg;
 

@@ -30,6 +31,11 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 	dw_pcie_dbi_ro_wr_dis(pci);
 }
 
+void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+{
+	__dw_pcie_ep_reset_bar(pci, bar, 0);
+}
+
 static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
 				   struct pci_epf_header *hdr)
 {

@@ -104,13 +110,14 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr,
 }
 
 static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
-				 enum pci_barno bar)
+				 struct pci_epf_bar *epf_bar)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar = epf_bar->barno;
 	u32 atu_index = ep->bar_to_atu[bar];
 
-	dw_pcie_ep_reset_bar(pci, bar);
+	__dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
 
 	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
 	clear_bit(atu_index, ep->ib_window_map);
drivers/pci/endpoint/functions/pci-epf-test.c +4 -1
index d46e3ebabb8e..7cef85124325 100644
@@ -344,14 +344,17 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
 	struct pci_epc *epc = epf->epc;
+	struct pci_epf_bar *epf_bar;
 	int bar;
 
 	cancel_delayed_work(&epf_test->cmd_handler);
 	pci_epc_stop(epc);
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
+		epf_bar = &epf->bar[bar];
+
 		if (epf_test->reg[bar]) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar);
-			pci_epc_clear_bar(epc, epf->func_no, bar);
+			pci_epc_clear_bar(epc, epf->func_no, epf_bar);
 		}
 	}
 }
drivers/pci/endpoint/pci-epc-core.c +4 -3
index 8637822605ff..eccc942043cb 100644
@@ -276,11 +276,12 @@ EXPORT_SYMBOL_GPL(pci_epc_map_addr);
  * pci_epc_clear_bar() - reset the BAR
  * @epc: the EPC device for which the BAR has to be cleared
  * @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be reset
+ * @epf_bar: the struct epf_bar that contains the BAR information
  *
  * Invoke to reset the BAR of the endpoint device.
  */
-void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
+void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
+		       struct pci_epf_bar *epf_bar)
 {
 	unsigned long flags;
 

@@ -291,7 +292,7 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
 		return;
 
 	spin_lock_irqsave(&epc->lock, flags);
-	epc->ops->clear_bar(epc, func_no, bar);
+	epc->ops->clear_bar(epc, func_no, epf_bar);
 	spin_unlock_irqrestore(&epc->lock, flags);
 }
 EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
include/linux/pci-epc.h +1 -1
index 75bae8aabbf9..af657ca58b70 100644
@@ -41,7 +41,7 @@ struct pci_epc_ops {
 	int	(*set_bar)(struct pci_epc *epc, u8 func_no,
 			   struct pci_epf_bar *epf_bar);
 	void	(*clear_bar)(struct pci_epc *epc, u8 func_no,
-			     enum pci_barno bar);
+			     struct pci_epf_bar *epf_bar);
 	int	(*map_addr)(struct pci_epc *epc, u8 func_no,
 			    phys_addr_t addr, u64 pci_addr, size_t size);
 	void	(*unmap_addr)(struct pci_epc *epc, u8 func_no,

include/linux/pci-epc.h int pci_epc_write_header(struct pci_epc *epc, u8 func_no, · line 134

Quoted 9 lines
include/linux/pci-epc.h +2 -1
@@ -127,7 +127,8 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
 			 struct pci_epf_header *hdr);
 int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
 		    struct pci_epf_bar *epf_bar);
-void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
+void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
+		       struct pci_epf_bar *epf_bar);
 int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
 		     phys_addr_t phys_addr,
 		     u64 pci_addr, size_t size);

Seems good to me. :)

Reviewed-by Gustavo Pimentel <[email protected]>
#27 lore raw

Re: [PATCH v5 09/12] PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar

In reply to Niklas Cassel

Hi Niklas,

Quoted 39 lines · Niklas Cassel

Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.

This is needed so that epc->ops->clear_bar() can clear the BAR pair, if the BAR is 64-bits wide.

This also makes it possible for pci_epc_clear_bar() to sanity check the flags.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c         |  3 ++-
 drivers/pci/dwc/pcie-designware-ep.c          | 13 ++++++++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  5 ++++-
 drivers/pci/endpoint/pci-epc-core.c           |  7 ++++---
 include/linux/pci-epc.h                       |  5 +++--
 5 files changed, 23 insertions(+), 10 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +2 -1
index 2905e098678c..3d8283e450a9 100644
@@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
 }
 
 static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
-				   enum pci_barno bar)
+				   struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	enum pci_barno bar = epf_bar->barno;
 	u32 reg, cfg, b, ctrl;
 
 	if (bar < BAR_4) {
drivers/pci/dwc/pcie-designware-ep.c +0 -0
index 571b90f88d84..cc4d8381c1dc 100644

drivers/pci/dwc/pcie-designware-ep.c void dw_pcie_ep_linkup(struct dw_pcie_ep *ep) · line 23

Quoted 6 lines
drivers/pci/dwc/pcie-designware-ep.c +2 -1
@@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 	pci_epc_linkup(epc);
 }
 
-void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
+				   int flags)

Looks like the 'flags' are not used anywhere here?

Thanks
Kishon

#28 lore raw

Re: [PATCH v5 09/12] PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar

Quoted 45 lines · Kishon Vijay Abraham I

Hi Niklas,

Quoted 42 lines · Niklas Cassel

Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.

This is needed so that epc->ops->clear_bar() can clear the BAR pair, if the BAR is 64-bits wide.

This also makes it possible for pci_epc_clear_bar() to sanity check the flags.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/cadence/pcie-cadence-ep.c         |  3 ++-
 drivers/pci/dwc/pcie-designware-ep.c          | 13 ++++++++++---
 drivers/pci/endpoint/functions/pci-epf-test.c |  5 ++++-
 drivers/pci/endpoint/pci-epc-core.c           |  7 ++++---
 include/linux/pci-epc.h                       |  5 +++--
 5 files changed, 23 insertions(+), 10 deletions(-)
drivers/pci/cadence/pcie-cadence-ep.c +2 -1
index 2905e098678c..3d8283e450a9 100644
@@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
 }
 
 static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
-				   enum pci_barno bar)
+				   struct pci_epf_bar *epf_bar)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	enum pci_barno bar = epf_bar->barno;
 	u32 reg, cfg, b, ctrl;
 
 	if (bar < BAR_4) {
drivers/pci/dwc/pcie-designware-ep.c +0 -0
index 571b90f88d84..cc4d8381c1dc 100644
@@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 	pci_epc_linkup(epc);
 }
Quoted 5 lines

drivers/pci/dwc/pcie-designware-ep.c void dw_pcie_ep_linkup(struct dw_pcie_ep *ep) · line 23

Quoted 3 lines
drivers/pci/dwc/pcie-designware-ep.c +2 -1
-void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
+				   int flags)

Looks like the 'flags' are not used anywhere here?

Hello Kishon,

That is correct, this patch is simply refactoring, flags is first used in patch 11/12, since I didn't want to refactor + add new code in the same commit.

Kind regards,
Niklas

author
#29 lore raw

PATCH v5 10/12 PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when clearing

In reply to Niklas Cassel

Since a 64-bit BAR consists of a BAR pair, and since there is no BAR after BAR_5, BAR_5 cannot be 64-bits wide.

This sanity check is done in pci_epc_clear_bar(), so that we don't need to do this sanity check in all epc->ops->clear_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>

---
Kishon made a review comment that he wanted this:
https://marc.info/?l=linux-kernel&m=152161168226203

Personally, I don't think that this check is needed, since pci_epc_set_bar() already does this check, and no one should modify the flags after pci_epc_set_bar() has been called.

If everyone agrees, then this patch could be dropped.

 drivers/pci/endpoint/pci-epc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +3 -1
index eccc942043cb..b0ee42739c3c 100644
@@ -285,7 +285,9 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
 {
 	unsigned long flags;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
+	    (epf_bar->barno == BAR_5 &&
+	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
 		return;
 
 	if (!epc->ops->clear_bar)
Signature · 1 line
2.14.2
Acked-by on v5 10/12
#30 lore raw

Re: [PATCH v5 10/12] PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when clearing

Quoted 7 lines · Niklas Cassel

Since a 64-bit BAR consists of a BAR pair, and since there is no BAR after BAR_5, BAR_5 cannot be 64-bits wide.

This sanity check is done in pci_epc_clear_bar(), so that we don't need to do this sanity check in all epc->ops->clear_bar() implementations.

Signed-off-by Niklas Cassel <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 30 lines

---
Kishon made a review comment that he wanted this:
https://marc.info/?l=linux-kernel&m=152161168226203

Personally, I don't think that this check is needed, since pci_epc_set_bar() already does this check, and no one should modify the flags after pci_epc_set_bar() has been called.

If everyone agrees, then this patch could be dropped.

 drivers/pci/endpoint/pci-epc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/endpoint/pci-epc-core.c +3 -1
index eccc942043cb..b0ee42739c3c 100644
@@ -285,7 +285,9 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
 {
 	unsigned long flags;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
+	    (epf_bar->barno == BAR_5 &&
+	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
 		return;
 
 	if (!epc->ops->clear_bar)
author
#31 lore raw

PATCH v5 11/12 PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs properly

In reply to 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/pci/dwc/pcie-designware-ep.c +4 -0
index cc4d8381c1dc..4d304e3ccf24 100644
@@ -28,6 +28,10 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
 	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writel_dbi2(pci, reg, 0x0);
 	dw_pcie_writel_dbi(pci, reg, 0x0);
+	if (flags & 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);
 }
Signature · 1 line
2.14.2
Reviewed-by on v5 11/12
#32 lore raw

Re: [PATCH v5 11/12] PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs properly

Hi Niklas,

Quoted 14 lines · 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 <[email protected]>
---
 drivers/pci/dwc/pcie-designware-ep.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/pci/dwc/pcie-designware-ep.c +0 -0
index cc4d8381c1dc..4d304e3ccf24 100644

drivers/pci/dwc/pcie-designware-ep.c static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar, · line 37

Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +4 -0
@@ -28,6 +28,10 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
 	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writel_dbi2(pci, reg, 0x0);
 	dw_pcie_writel_dbi(pci, reg, 0x0);
+	if (flags & 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);
 }

Seems good to me. :)

Reviewed-by Gustavo Pimentel <[email protected]>
Acked-by on v5 11/12
#33 lore raw

Re: [PATCH v5 11/12] PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs properly

In reply to Niklas Cassel
Quoted 4 lines · 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 <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
Quoted 20 lines
---
 drivers/pci/dwc/pcie-designware-ep.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/pci/dwc/pcie-designware-ep.c +4 -0
index cc4d8381c1dc..4d304e3ccf24 100644
@@ -28,6 +28,10 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
 	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writel_dbi2(pci, reg, 0x0);
 	dw_pcie_writel_dbi(pci, reg, 0x0);
+	if (flags & 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);
 }
author
#34 lore raw

PATCH v5 12/12 misc: pci_endpoint_test: Handle 64-bit BARs properly

In reply to 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 <[email protected]>
Acked-by Kishon Vijay Abraham I <[email protected]>
---
 drivers/misc/pci_endpoint_test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/misc/pci_endpoint_test.c +7 -5
index 320276f42653..fe8897e64635 100644
@@ -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];
Signature · 1 line
2.14.2
Tested-by on the v5 series
#35 lore raw
In reply to Niklas Cassel

Hi Niklas,

Quoted 34 lines · 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 (12):
PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()
PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid
PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set
PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is
not set
PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
properly
PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was
set-up
PCI: endpoint: Handle 64-bit BARs properly
PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take
struct *epf_bar
PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when
clearing
PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
properly
misc: pci_endpoint_test: Handle 64-bit BARs properly

Quoted 10 lines
 drivers/misc/pci_endpoint_test.c              | 12 +++++----
 drivers/pci/cadence/pcie-cadence-ep.c         | 15 ++++++++---
 drivers/pci/dwc/pcie-designware-ep.c          | 36 +++++++++++++++++++++------
 drivers/pci/endpoint/functions/pci-epf-test.c | 28 +++++++++++++--------
 drivers/pci/endpoint/pci-epc-core.c           | 32 +++++++++++++++---------
 drivers/pci/endpoint/pci-epf-core.c           |  4 +++
 include/linux/pci-epc.h                       | 11 ++++----
 include/linux/pci-epf.h                       |  2 ++
 8 files changed, 95 insertions(+), 45 deletions(-)

For the whole series:

Tested-by Gustavo Pimentel <[email protected]>

Regards,
Gustavo

#36 lore raw
Quoted 48 lines · Gustavo Pimentel

Hi Niklas,

Quoted 34 lines · 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 (12):
PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()
PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid
PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set
PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is
not set
PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
properly
PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was
set-up
PCI: endpoint: Handle 64-bit BARs properly
PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take
struct *epf_bar
PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when
clearing
PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
properly
misc: pci_endpoint_test: Handle 64-bit BARs properly

Quoted 10 lines
 drivers/misc/pci_endpoint_test.c              | 12 +++++----
 drivers/pci/cadence/pcie-cadence-ep.c         | 15 ++++++++---
 drivers/pci/dwc/pcie-designware-ep.c          | 36 +++++++++++++++++++++------
 drivers/pci/endpoint/functions/pci-epf-test.c | 28 +++++++++++++--------
 drivers/pci/endpoint/pci-epc-core.c           | 32 +++++++++++++++---------
 drivers/pci/endpoint/pci-epf-core.c           |  4 +++
 include/linux/pci-epc.h                       | 11 ++++----
 include/linux/pci-epf.h                       |  2 ++
 8 files changed, 95 insertions(+), 45 deletions(-)
Quoted 3 lines

For the whole series:

Tested-by Gustavo Pimentel <[email protected]>

Hello Gustavo,

Thanks a lot for testing!

Kind regards,
Niklas