[PATCH v2 0/9] PCI: endpoint: set prefetchable bit for 64-bit BARs

[PATCH v2 0/9] PCI: endpoint: set prefetchable bit for 64-bit BARs

[Niklas Cassel]

Shradha Todi wanted to add prefetchable BAR support by adding more things
to epc_features:
https://lore.kernel.org/linux-pci/20240228134448.56372-1-shradha.t@samsung.com/T/#t

The series starts off with some generic cleanups and fixes which was needed
to make the implementation of the actual feature easier.

The final patch in the series sets the prefetchable bit for all backing memory
that the EPF core allocates for a 64-bit BAR.


Kind regards,
Niklas


Changes since V2:
-Addressed my own comments.
-Added additional clean up patches.


Niklas Cassel (9):
  PCI: endpoint: pci-epf-test: Fix incorrect loop increment
  PCI: endpoint: Allocate a 64-bit BAR if that is the only option
  PCI: endpoint: pci-epf-test: Remove superfluous code
  PCI: endpoint: pci-epf-test: Simplify pci_epf_test_alloc_space() loop
  PCI: endpoint: pci-epf-test: Simplify pci_epf_test_set_bar() loop
  PCI: endpoint: pci-epf-test: Clean up pci_epf_test_unbind()
  PCI: cadence: Set a 64-bit BAR if requested
  PCI: rockchip-ep: Set a 64-bit BAR if requested
  PCI: endpoint: Set prefetch when allocating memory for 64-bit BARs

 .../pci/controller/cadence/pcie-cadence-ep.c  |  5 +-
 drivers/pci/controller/pcie-rockchip-ep.c     |  2 +-
 drivers/pci/endpoint/functions/pci-epf-test.c | 67 +++++++------------
 drivers/pci/endpoint/pci-epf-core.c           | 10 ++-
 4 files changed, 33 insertions(+), 51 deletions(-)

-- 
2.44.0

[PATCH v2 1/9] PCI: endpoint: pci-epf-test: Fix incorrect loop increment

[Niklas Cassel]

pci_epf_test_alloc_space() currently skips the BAR succeeding a 64-bit BAR
if the 64-bit flag is set, before calling pci_epf_alloc_space().

However, pci_epf_alloc_space() will set the 64-bit flag if we request an
allocation of 4 GB or larger, even if it wasn't set before the allocation.

Thus, we need to check the flag also after pci_epf_alloc_space().

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index cd4ffb39dcdc..01ba088849cc 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -865,6 +865,12 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 			dev_err(dev, "Failed to allocate space for BAR%d\n",
 				bar);
 		epf_test->reg[bar] = base;
+
+		/*
+		 * pci_epf_alloc_space() might have given us a 64-bit BAR,
+		 * if we requested a size larger than 4 GB.
+		 */
+		add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
 	}
 
 	return 0;
-- 
2.44.0

[PATCH v2 2/9] PCI: endpoint: Allocate a 64-bit BAR if that is the only option

[Niklas Cassel]

pci_epf_alloc_space() already sets the 64-bit flag if the BAR size is
larger than 4GB, even if the caller did not explicitly request a 64-bit
BAR.

Thus, let pci_epf_alloc_space() also set the 64-bit flag if the hardware
description says that the specific BAR can only be 64-bit.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 3 ++-
 drivers/pci/endpoint/pci-epf-core.c           | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 01ba088849cc..8c9802b9b835 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -868,7 +868,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 
 		/*
 		 * pci_epf_alloc_space() might have given us a 64-bit BAR,
-		 * if we requested a size larger than 4 GB.
+		 * either because the BAR can only be a 64-bit BAR, or if
+		 * we requested a size larger than 4 GB.
 		 */
 		add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
 	}
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 0a28a0b0911b..e7dbbeb1f0de 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -304,9 +304,10 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 	epf_bar[bar].addr = space;
 	epf_bar[bar].size = size;
 	epf_bar[bar].barno = bar;
-	epf_bar[bar].flags |= upper_32_bits(size) ?
-				PCI_BASE_ADDRESS_MEM_TYPE_64 :
-				PCI_BASE_ADDRESS_MEM_TYPE_32;
+	if (upper_32_bits(size) || epc_features->bar[bar].only_64bit)
+		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
+	else
+		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_32;
 
 	return space;
 }
-- 
2.44.0

[PATCH v2 3/9] PCI: endpoint: pci-epf-test: Remove superfluous code

[Niklas Cassel]

The pci-epf-test does no special configuration at all, it simply requests
a 64-bit BAR if the hardware requires it. However, this flag is now
automatically set when allocating a BAR that can only be a 64-bit BAR,
so we can drop pci_epf_configure_bar() completely.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 8c9802b9b835..7dc9704128dc 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -877,19 +877,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	return 0;
 }
 
-static void pci_epf_configure_bar(struct pci_epf *epf,
-				  const struct pci_epc_features *epc_features)
-{
-	struct pci_epf_bar *epf_bar;
-	int i;
-
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		epf_bar = &epf->bar[i];
-		if (epc_features->bar[i].only_64bit)
-			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
-	}
-}
-
 static int pci_epf_test_bind(struct pci_epf *epf)
 {
 	int ret;
@@ -914,7 +901,6 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	test_reg_bar = pci_epc_get_first_free_bar(epc_features);
 	if (test_reg_bar < 0)
 		return -EINVAL;
-	pci_epf_configure_bar(epf, epc_features);
 
 	epf_test->test_reg_bar = test_reg_bar;
 	epf_test->epc_features = epc_features;
-- 
2.44.0

[PATCH v2 4/9] PCI: endpoint: pci-epf-test: Simplify pci_epf_test_alloc_space() loop

[Niklas Cassel]

Make pci-epf-test use pci_epc_get_next_free_bar() just like pci-epf-ntb.c
and pci-epf-vntb.c.

Using pci_epc_get_next_free_bar() also makes it more obvious that
pci-epf-test does no special configuration at all.

This way, the code is more consistent between EPF drivers, and pci-epf-test
does not need to explicitly check if the BAR is reserved, or if the index
belongs to a BAR succeeding a 64-bit only BAR.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 7dc9704128dc..20c79610712d 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -823,8 +823,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	size_t pba_size = 0;
 	bool msix_capable;
 	void *base;
-	int bar, add;
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
+	enum pci_barno bar;
 	const struct pci_epc_features *epc_features;
 	size_t test_reg_size;
 
@@ -849,16 +849,14 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	}
 	epf_test->reg[test_reg_bar] = base;
 
-	for (bar = 0; bar < PCI_STD_NUM_BARS; bar += add) {
-		epf_bar = &epf->bar[bar];
-		add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
+	for (bar = BAR_0; bar < PCI_STD_NUM_BARS; bar++) {
+		bar = pci_epc_get_next_free_bar(epc_features, bar);
+		if (bar == NO_BAR)
+			break;
 
 		if (bar == test_reg_bar)
 			continue;
 
-		if (epc_features->bar[bar].type == BAR_RESERVED)
-			continue;
-
 		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
 					   epc_features, PRIMARY_INTERFACE);
 		if (!base)
@@ -871,7 +869,9 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 		 * either because the BAR can only be a 64-bit BAR, or if
 		 * we requested a size larger than 4 GB.
 		 */
-		add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
+		epf_bar = &epf->bar[bar];
+		if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+			bar++;
 	}
 
 	return 0;
-- 
2.44.0

[PATCH v2 5/9] PCI: endpoint: pci-epf-test: Simplify pci_epf_test_set_bar() loop

[Niklas Cassel]

Simplify the loop in pci_epf_test_set_bar().
If we allocated memory for the BAR, we need to call set_bar() for that
BAR, if we did not allocated memory for that BAR, we need to skip.
It is as simple as that. This also matches the logic in
pci_epf_test_unbind().

A 64-bit BAR will still only be one allocation, with the BAR succeeding
the 64-bit BAR being null.

While at it, remove the misleading comment.
A EPC .set_bar() callback should never change the epf_bar->flags.
(E.g. to set a 64-bit BAR if we requested a 32-bit BAR.)

A .set_bar() callback should do what we request it to do.
If it can't satisfy the request, it should return an error.

If platform has a specific requirement, e.g. that a certain BAR has to
be a 64-bit BAR, then it should specify that by setting the .only_64bit
flag for that specific BAR in epc_features->bar[], such that
pci_epf_alloc_space() will return a epf_bar with the 64-bit flag set.
(Such that .set_bar() will receive a request to set a 64-bit BAR.)

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 20c79610712d..05b9bc1e89cd 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -709,9 +709,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
 
 static int pci_epf_test_set_bar(struct pci_epf *epf)
 {
-	int bar, add;
-	int ret;
-	struct pci_epf_bar *epf_bar;
+	int bar, ret;
 	struct pci_epc *epc = epf->epc;
 	struct device *dev = &epf->dev;
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -720,20 +718,12 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 
 	epc_features = epf_test->epc_features;
 
-	for (bar = 0; bar < PCI_STD_NUM_BARS; bar += add) {
-		epf_bar = &epf->bar[bar];
-		/*
-		 * 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.
-		 */
-		add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
-
-		if (epc_features->bar[bar].type == BAR_RESERVED)
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		if (!epf_test->reg[bar])
 			continue;
 
 		ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
-				      epf_bar);
+				      &epf->bar[bar]);
 		if (ret) {
 			pci_epf_free_space(epf, epf_test->reg[bar], bar,
 					   PRIMARY_INTERFACE);
-- 
2.44.0

[PATCH v2 6/9] PCI: endpoint: pci-epf-test: Clean up pci_epf_test_unbind()

[Niklas Cassel]

Clean up pci_epf_test_unbind() by using a continue if we did not allocate
memory for the BAR index. This reduces the indentation level by one.

This makes pci_epf_test_unbind() more similar to pci_epf_test_set_bar().

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 05b9bc1e89cd..2c1d10afb1ae 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -690,20 +690,18 @@ 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_epf_test_clean_dma_chan(epf_test);
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		epf_bar = &epf->bar[bar];
+		if (!epf_test->reg[bar])
+			continue;
 
-		if (epf_test->reg[bar]) {
-			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
-					  epf_bar);
-			pci_epf_free_space(epf, epf_test->reg[bar], bar,
-					   PRIMARY_INTERFACE);
-		}
+		pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
+				  &epf->bar[bar]);
+		pci_epf_free_space(epf, epf_test->reg[bar], bar,
+				   PRIMARY_INTERFACE);
 	}
 }
 
-- 
2.44.0

[PATCH v2 7/9] PCI: cadence: Set a 64-bit BAR if requested

[Niklas Cassel]

Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB
is invalid if 64-bit flag is not set") it has been impossible to get the
.set_bar() callback with a BAR size > 4 GB, if the BAR was also not
requested to be configured as a 64-bit BAR.

Thus, forcing setting the 64-bit flag for BARs larger than 4 GB in the
lower level driver is dead code and can be removed.

It is however possible that an EPF driver configures a BAR as 64-bit,
even if the requested size is < 4 GB.

Respect the requested BAR configuration, just like how it is already
repected with regards to the prefetchable bit.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 2d0a8d78bffb..de10e5edd1b0 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -99,14 +99,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
 		ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS;
 	} else {
 		bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH);
-		bool is_64bits = sz > SZ_2G;
+		bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64);
 
 		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)
-- 
2.44.0

[PATCH v2 8/9] PCI: rockchip-ep: Set a 64-bit BAR if requested

[Niklas Cassel]

Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB
is invalid if 64-bit flag is not set") it has been impossible to get the
.set_bar() callback with a BAR size > 4 GB, if the BAR was also not
requested to be configured as a 64-bit BAR.

It is however possible that an EPF driver configures a BAR as 64-bit,
even if the requested size is < 4 GB.

Respect the requested BAR configuration, just like how it is already
repected with regards to the prefetchable bit.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index c9046e97a1d2..57472cf48997 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -153,7 +153,7 @@ static int rockchip_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
 		ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_IO_32BITS;
 	} else {
 		bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH);
-		bool is_64bits = sz > SZ_2G;
+		bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64);
 
 		if (is_64bits && (bar & 1))
 			return -EINVAL;
-- 
2.44.0

[PATCH v2 9/9] PCI: endpoint: Set prefetch when allocating memory for 64-bit BARs

[Niklas Cassel]

From the PCIe 6.0 base spec:
"Generally only 64-bit BARs are good candidates, since only Legacy
Endpoints are permitted to set the Prefetchable bit in 32-bit BARs,
and most scalable platforms map all 32-bit Memory BARs into
non-prefetchable Memory Space regardless of the Prefetchable bit value."

"For a PCI Express Endpoint, 64-bit addressing must be supported for all
BARs that have the Prefetchable bit Set. 32-bit addressing is permitted
for all BARs that do not have the Prefetchable bit Set."

"Any device that has a range that behaves like normal memory should mark
the range as prefetchable. A linear frame buffer in a graphics device is
an example of a range that should be marked prefetchable."

The PCIe spec tells us that we should have the prefetchable bit set for
64-bit BARs backed by "normal memory". The backing memory that we allocate
for a 64-bit BAR using pci_epf_alloc_space() (which calls
dma_alloc_coherent()) is obviously "normal memory".

Thus, set the prefetchable bit when allocating backing memory for a 64-bit
BAR.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/pci-epf-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index e7dbbeb1f0de..20d2bde0747c 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -309,6 +309,9 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 	else
 		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_32;
 
+	if (epf_bar[bar].flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+
 	return space;
 }
 EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
-- 
2.44.0

Re: [PATCH v2 5/9] PCI: endpoint: pci-epf-test: Simplify pci_epf_test_set_bar() loop

[kernel test robot]

Hi Niklas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on next-20240312]
[cannot apply to pci/for-linus mani-mhi/mhi-next linus/master v6.8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Niklas-Cassel/PCI-endpoint-pci-epf-test-Fix-incorrect-loop-increment/20240312-185512
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20240312105152.3457899-6-cassel%40kernel.org
patch subject: [PATCH v2 5/9] PCI: endpoint: pci-epf-test: Simplify pci_epf_test_set_bar() loop
config: arc-randconfig-001-20240313 (https://download.01.org/0day-ci/archive/20240313/202403131004.Z9epuI06-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240313/202403131004.Z9epuI06-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403131004.Z9epuI06-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pci/endpoint/functions/pci-epf-test.c: In function 'pci_epf_test_set_bar':
>> drivers/pci/endpoint/functions/pci-epf-test.c:717:40: warning: variable 'epc_features' set but not used [-Wunused-but-set-variable]
     717 |         const struct pci_epc_features *epc_features;
         |                                        ^~~~~~~~~~~~


vim +/epc_features +717 drivers/pci/endpoint/functions/pci-epf-test.c

349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  709  
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  710  static int pci_epf_test_set_bar(struct pci_epf *epf)
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  711  {
2287a91bfd63cb Niklas Cassel          2024-03-12  712  	int bar, ret;
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  713  	struct pci_epc *epc = epf->epc;
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  714  	struct device *dev = &epf->dev;
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  715  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
3235b994950d84 Kishon Vijay Abraham I 2017-08-18  716  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
2c04c5b8eef797 Kishon Vijay Abraham I 2019-01-14 @717  	const struct pci_epc_features *epc_features;
2c04c5b8eef797 Kishon Vijay Abraham I 2019-01-14  718  
2c04c5b8eef797 Kishon Vijay Abraham I 2019-01-14  719  	epc_features = epf_test->epc_features;
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  720  
2287a91bfd63cb Niklas Cassel          2024-03-12  721  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
2287a91bfd63cb Niklas Cassel          2024-03-12  722  		if (!epf_test->reg[bar])
2c04c5b8eef797 Kishon Vijay Abraham I 2019-01-14  723  			continue;
2c04c5b8eef797 Kishon Vijay Abraham I 2019-01-14  724  
53fd3cbe5e9d79 Kishon Vijay Abraham I 2021-08-19  725  		ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
2287a91bfd63cb Niklas Cassel          2024-03-12  726  				      &epf->bar[bar]);
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  727  		if (ret) {
63840ff5322373 Kishon Vijay Abraham I 2021-02-02  728  			pci_epf_free_space(epf, epf_test->reg[bar], bar,
63840ff5322373 Kishon Vijay Abraham I 2021-02-02  729  					   PRIMARY_INTERFACE);
798c0441bec8c4 Gustavo Pimentel       2018-05-14  730  			dev_err(dev, "Failed to set BAR%d\n", bar);
3235b994950d84 Kishon Vijay Abraham I 2017-08-18  731  			if (bar == test_reg_bar)
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  732  				return ret;
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  733  		}
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  734  	}
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  735  
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  736  	return 0;
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  737  }
349e7a85b25fa6 Kishon Vijay Abraham I 2017-03-27  738  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 7/9] PCI: cadence: Set a 64-bit BAR if requested

[Bjorn Helgaas]

[+cc Shawn for Rockchip question]

On Tue, Mar 12, 2024 at 11:51:47AM +0100, Niklas Cassel wrote:
> Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB
> is invalid if 64-bit flag is not set") it has been impossible to get the
> .set_bar() callback with a BAR size > 4 GB, if the BAR was also not
> requested to be configured as a 64-bit BAR.
> 
> Thus, forcing setting the 64-bit flag for BARs larger than 4 GB in the
> lower level driver is dead code and can be removed.
> 
> It is however possible that an EPF driver configures a BAR as 64-bit,
> even if the requested size is < 4 GB.
> 
> Respect the requested BAR configuration, just like how it is already
> repected with regards to the prefetchable bit.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 2d0a8d78bffb..de10e5edd1b0 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -99,14 +99,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>  		ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS;
>  	} else {
>  		bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH);
> -		bool is_64bits = sz > SZ_2G;
> +		bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64);
>  
>  		if (is_64bits && (bar & 1))
>  			return -EINVAL;

Not relevant to *this* patch, but it looks like this code assumes that
a 64-bit BAR must consist of an even-numbered DWORD followed by an
odd-numbered one, e.g., it could be BAR 0 and BAR 1, but not BAR 1 and
BAR 2.

I don't think the PCIe spec requires that.  Does the Cadence hardware
require this?

What about Rockchip, which has similar code in
rockchip_pcie_ep_set_bar()?

FWIW, dw_pcie_ep_set_bar() doesn't enforce this restriction.

> -		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.44.0
> 

Re: [PATCH v2 7/9] PCI: cadence: Set a 64-bit BAR if requested

[Niklas Cassel]

Hello Bjorn,

On Thu, May 16, 2024 at 03:49:07PM -0500, Bjorn Helgaas wrote:
> [+cc Shawn for Rockchip question]
> 
> On Tue, Mar 12, 2024 at 11:51:47AM +0100, Niklas Cassel wrote:
> > Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB
> > is invalid if 64-bit flag is not set") it has been impossible to get the
> > .set_bar() callback with a BAR size > 4 GB, if the BAR was also not
> > requested to be configured as a 64-bit BAR.
> > 
> > Thus, forcing setting the 64-bit flag for BARs larger than 4 GB in the
> > lower level driver is dead code and can be removed.
> > 
> > It is however possible that an EPF driver configures a BAR as 64-bit,
> > even if the requested size is < 4 GB.
> > 
> > Respect the requested BAR configuration, just like how it is already
> > repected with regards to the prefetchable bit.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > index 2d0a8d78bffb..de10e5edd1b0 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > @@ -99,14 +99,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> >  		ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS;
> >  	} else {
> >  		bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH);
> > -		bool is_64bits = sz > SZ_2G;
> > +		bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64);
> >  
> >  		if (is_64bits && (bar & 1))
> >  			return -EINVAL;
> 
> Not relevant to *this* patch, but it looks like this code assumes that
> a 64-bit BAR must consist of an even-numbered DWORD followed by an
> odd-numbered one, e.g., it could be BAR 0 and BAR 1, but not BAR 1 and
> BAR 2.
> 
> I don't think the PCIe spec requires that.  Does the Cadence hardware
> require this?

The PCIe spec does not seems to require this:

"A Type 0 Configuration Space Header has six DWORD locations allocated for
Base Address registers starting at offset 10h in Configuration Space. [...]
A Function may use any of the locations to implement Base Address registers.
An implemented 64-bit Base Address register consumes two consecutive DWORD
locations."


> What about Rockchip, which has similar code in
> rockchip_pcie_ep_set_bar()?
> 
> FWIW, dw_pcie_ep_set_bar() doesn't enforce this restriction.

The Synopsys PCIe controller does have this limitation:

From the DWC databook:

3.5.7.1.1 Overview

Base Address Registers (Offset: 0x100x24)

The controller provides three pairs of 32-bit BARs for each implemented
function. Each pair (BARs 0 and 1, BARs 2 and 3, BARs 4 and 5) can be
configured as follows:
-One 64-bit BAR: For example, BARs 0 and 1 are combined to form a single
 64-bit BAR.
-Two 32-bit BARs: For example, BARs 0 and 1 are two independent 32-bit BARs.
-One 32-bit BAR: For example, BAR0 is a 32-bit BAR and BAR1 is either
 disabled or removed from the controller altogether to reduce gate count.

So dw_pcie_ep_set_bar() should probably be fixed to enforce this requirement.

(PCI patches for DWC appear to have a tendency to be redirected to /dev/null,
so personally I'm not planning on sending out a patch to enforce this.)


Kind regards,
Niklas