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

[PATCH v3 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:
-Fixed warning when building with W=1 reported by kernel test robot.


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 | 70 +++++++------------
 drivers/pci/endpoint/pci-epf-core.c           | 10 ++-
 4 files changed, 33 insertions(+), 54 deletions(-)

-- 
2.44.0

[PATCH v3 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:
-Fixed warning when building with W=1 reported by kernel test robot.


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 | 70 +++++++------------
 drivers/pci/endpoint/pci-epf-core.c           | 10 ++-
 4 files changed, 33 insertions(+), 54 deletions(-)

-- 
2.44.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 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:
-Fixed warning when building with W=1 reported by kernel test robot.


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 | 70 +++++++------------
 drivers/pci/endpoint/pci-epf-core.c           | 10 ++-
 4 files changed, 33 insertions(+), 54 deletions(-)

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 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 v3 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 v3 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 v3 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 v3 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 | 21 ++++---------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 20c79610712d..91bbfcb1b3ed 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -709,31 +709,18 @@ 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);
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
-	const struct pci_epc_features *epc_features;
-
-	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 v3 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 91bbfcb1b3ed..fbe14c7232c7 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 v3 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 v3 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 v3 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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 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 v3 1/9] PCI: endpoint: pci-epf-test: Fix incorrect loop increment

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:53AM +0100, Niklas Cassel wrote:
> 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.
> 

Max BAR size is 1MB currently, so I believe you are adding the check just for
the sake of correctness. If so, please mention it explicitly.

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:54AM +0100, Niklas Cassel wrote:
> 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.
> 

Could you please update the kdoc of pci_epf_alloc_space() to reflec the same?

> Signed-off-by: Niklas Cassel <cassel@kernel.org>

With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:55AM +0100, Niklas Cassel wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:56AM +0100, Niklas Cassel wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:57AM +0100, Niklas Cassel wrote:
> 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.
> 

That's a valid assumption. But...

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

Looks like pcie-cadence-ep is setting the PCI_BASE_ADDRESS_MEM_TYPE_64 flag if
the requested size is >2GB (I don't know why 2GB instead of 4GB in the first
place).

- Mani

> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++---------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 20c79610712d..91bbfcb1b3ed 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -709,31 +709,18 @@ 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);
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> -	const struct pci_epc_features *epc_features;
> -
> -	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
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:58AM +0100, Niklas Cassel wrote:
> 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().
> 

I've proposed to move the clear_bar and free_space code to separate helper
functions in my series [1]. If this series gets merged first (it really makes
sense), then this patch can be dropped now.

- Mani

> 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 91bbfcb1b3ed..fbe14c7232c7 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
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:57:59AM +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

2 GB

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

Okay, here you are fixing this driver. But this should be moved before patch
5/9. With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:58:00AM +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.
> 
> It is however possible that an EPF driver configures a BAR as 64-bit,
> even if the requested size is < 4 GB.

2 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>

Ah, I missed this one. But the same comment as previous one applies to this
patch also.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:58:00AM +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.
> 
> It is however possible that an EPF driver configures a BAR as 64-bit,
> even if the requested size is < 4 GB.

2 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>

Ah, I missed this one. But the same comment as previous one applies to this
patch also.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Manivannan Sadhasivam]

On Wed, Mar 13, 2024 at 11:58:00AM +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.
> 
> It is however possible that an EPF driver configures a BAR as 64-bit,
> even if the requested size is < 4 GB.

2 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>

Ah, I missed this one. But the same comment as previous one applies to this
patch also.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Manivannan Sadhasivam]

+ Arnd

On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
> From the PCIe 6.0 base spec:

It'd be good to mention the section also.

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

I'm not sure this is correct. Memory returned by 'dma_alloc_coherent' is not the
'normal memory' but rather 'consistent/coherent memory'. Here the question is,
can the memory returned by dma_alloc_coherent() be prefetched or write-combined
on all architectures.

I hope Arnd can answer this question.

- Mani

> 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 v3 9/9] PCI: endpoint: Set prefetch when allocating memory for 64-bit BARs

[Arnd Bergmann]

On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
>> "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".
>> 
>
> I'm not sure this is correct. Memory returned by 'dma_alloc_coherent' is not the
> 'normal memory' but rather 'consistent/coherent memory'. Here the question is,
> can the memory returned by dma_alloc_coherent() be prefetched or write-combined
> on all architectures.
>
> I hope Arnd can answer this question.

I think there are three separate questions here when talking about
a scenario where a PCI master accesses memory behind a PCI endpoint:

- The CPU on the host side ususally uses ioremap() for mapping
  the PCI BAR of the device. If the BAR is marked as prefetchable,
  we usually allow mapping it using ioremap_wc() for write-combining
  or ioremap_wt() for a write-through mappings that allow both
  write-combining and prefetching. On some architectures, these
  all fall back to normal register mappings which do none of these.
  If it uses write-combining or prefetching, the host side driver
  will need to manually serialize against concurrent access from
  the endpoint side.

- The endpoint device accessing a buffer in memory is controlled
  by the endpoint driver and may decide to prefetch data into a
  local cache independent of the other two. I don't know if any
  of the suppored endpoint devices actually do that. A prefetch
  from the PCI host side would appear as a normal transaction here.

- The local CPU on the endpoint side may access the same buffer as
  the endpoint device. On low-end SoCs the DMA from the PCI
  endpoint is not coherent with the CPU caches, so the CPU may
  need to map it as uncacheable to allow data consistency with
  a the CPU on the PCI host side. On higher-end SoCs (e.g. most
  non-ARM ones) DMA is coherent with the caches, so the CPU
  on the endpoint side may map the buffer as cached and
  still be coherent with a CPU on the PCI host side that has
  mapped it with ioremap().

       Arnd

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

[Niklas Cassel]

Hello all,

On Fri, Mar 15, 2024 at 06:29:52PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
> > On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
> >> "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".
> >> 
> >
> > I'm not sure this is correct. Memory returned by 'dma_alloc_coherent' is not the
> > 'normal memory' but rather 'consistent/coherent memory'. Here the question is,
> > can the memory returned by dma_alloc_coherent() be prefetched or write-combined
> > on all architectures.
> >
> > I hope Arnd can answer this question.
> 
> I think there are three separate questions here when talking about
> a scenario where a PCI master accesses memory behind a PCI endpoint:

I think the question is if the PCI epf-core, which runs on the endpoint
side, and which calls dma_alloc_coherent() to allocate backing memory for
a BAR, can set/mark the Prefetchable bit for the BAR (if we also set/mark
the BAR as a 64-bit BAR).

The PCIe 6.0 spec, 7.5.1.2.1 Base Address Registers (Offset 10h - 24h),
states:
"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."

Does not backing memory allocated for a specific BAR using
dma_alloc_coherent() on the EP side behave like normal memory from the
host's point of view?



On the host side, this will mean that the host driver sees the
Prefetchable bit, and as according to:
https://docs.kernel.org/driver-api/device-io.html
The host might map the BAR using ioremap_wc().

Looking specifically at drivers/misc/pci_endpoint_test.c, it maps the
BARs using pci_ioremap_bar():
https://elixir.bootlin.com/linux/v6.8/source/drivers/pci/pci.c#L252
which will not map it using ioremap_wc().
(But the code we have in the PCI epf-core must of course work with host
side drivers other than pci_endpoint_test.c as well.)


> 
> - The CPU on the host side ususally uses ioremap() for mapping
>   the PCI BAR of the device. If the BAR is marked as prefetchable,
>   we usually allow mapping it using ioremap_wc() for write-combining
>   or ioremap_wt() for a write-through mappings that allow both
>   write-combining and prefetching. On some architectures, these
>   all fall back to normal register mappings which do none of these.
>   If it uses write-combining or prefetching, the host side driver
>   will need to manually serialize against concurrent access from
>   the endpoint side.
> 
> - The endpoint device accessing a buffer in memory is controlled
>   by the endpoint driver and may decide to prefetch data into a
>   local cache independent of the other two. I don't know if any
>   of the suppored endpoint devices actually do that. A prefetch
>   from the PCI host side would appear as a normal transaction here.
> 
> - The local CPU on the endpoint side may access the same buffer as
>   the endpoint device. On low-end SoCs the DMA from the PCI
>   endpoint is not coherent with the CPU caches, so the CPU may

I don't follow. When doing DMA *from* the endpoint, then the DMA HW
on the EP side will read or write data to a buffer allocated on the
host side (most likely using dma_alloc_coherent()), but what does
that got to do with how the EP configures the BARs that it exposes?


>   need to map it as uncacheable to allow data consistency with
>   a the CPU on the PCI host side. On higher-end SoCs (e.g. most
>   non-ARM ones) DMA is coherent with the caches, so the CPU
>   on the endpoint side may map the buffer as cached and
>   still be coherent with a CPU on the PCI host side that has
>   mapped it with ioremap().


Kind regards,
Niklas

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

[Manivannan Sadhasivam]

On Sun, Mar 17, 2024 at 12:54:11PM +0100, Niklas Cassel wrote:
> Hello all,
> 
> On Fri, Mar 15, 2024 at 06:29:52PM +0100, Arnd Bergmann wrote:
> > On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
> > > On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
> > >> "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".
> > >> 
> > >
> > > I'm not sure this is correct. Memory returned by 'dma_alloc_coherent' is not the
> > > 'normal memory' but rather 'consistent/coherent memory'. Here the question is,
> > > can the memory returned by dma_alloc_coherent() be prefetched or write-combined
> > > on all architectures.
> > >
> > > I hope Arnd can answer this question.
> > 
> > I think there are three separate questions here when talking about
> > a scenario where a PCI master accesses memory behind a PCI endpoint:
> 
> I think the question is if the PCI epf-core, which runs on the endpoint
> side, and which calls dma_alloc_coherent() to allocate backing memory for
> a BAR, can set/mark the Prefetchable bit for the BAR (if we also set/mark
> the BAR as a 64-bit BAR).
> 
> The PCIe 6.0 spec, 7.5.1.2.1 Base Address Registers (Offset 10h - 24h),
> states:
> "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."
> 
> Does not backing memory allocated for a specific BAR using
> dma_alloc_coherent() on the EP side behave like normal memory from the
> host's point of view?
> 
> 
> 
> On the host side, this will mean that the host driver sees the
> Prefetchable bit, and as according to:
> https://docs.kernel.org/driver-api/device-io.html
> The host might map the BAR using ioremap_wc().
> 
> Looking specifically at drivers/misc/pci_endpoint_test.c, it maps the
> BARs using pci_ioremap_bar():
> https://elixir.bootlin.com/linux/v6.8/source/drivers/pci/pci.c#L252
> which will not map it using ioremap_wc().
> (But the code we have in the PCI epf-core must of course work with host
> side drivers other than pci_endpoint_test.c as well.)
> 
> 

Right. I don't see any problem with the host side assumption. But my question
is, is it OK to advertise the coherent memory allocated on the endpoint as
prefetchable to the host.

As you quoted the spec,

"Any device that has a range that behaves like normal memory should mark
the range as prefetchable."

Here, the coherent memory allocated by the device(endpoint) won't behave as a
normal memory on the _endpoint_. But I'm certainly not sure if there are any
implications in exposing this memory as a 'normal memory' to the host.

- Mani

> > 
> > - The CPU on the host side ususally uses ioremap() for mapping
> >   the PCI BAR of the device. If the BAR is marked as prefetchable,
> >   we usually allow mapping it using ioremap_wc() for write-combining
> >   or ioremap_wt() for a write-through mappings that allow both
> >   write-combining and prefetching. On some architectures, these
> >   all fall back to normal register mappings which do none of these.
> >   If it uses write-combining or prefetching, the host side driver
> >   will need to manually serialize against concurrent access from
> >   the endpoint side.
> > 
> > - The endpoint device accessing a buffer in memory is controlled
> >   by the endpoint driver and may decide to prefetch data into a
> >   local cache independent of the other two. I don't know if any
> >   of the suppored endpoint devices actually do that. A prefetch
> >   from the PCI host side would appear as a normal transaction here.
> > 
> > - The local CPU on the endpoint side may access the same buffer as
> >   the endpoint device. On low-end SoCs the DMA from the PCI
> >   endpoint is not coherent with the CPU caches, so the CPU may
> 
> I don't follow. When doing DMA *from* the endpoint, then the DMA HW
> on the EP side will read or write data to a buffer allocated on the
> host side (most likely using dma_alloc_coherent()), but what does
> that got to do with how the EP configures the BARs that it exposes?
> 
> 
> >   need to map it as uncacheable to allow data consistency with
> >   a the CPU on the PCI host side. On higher-end SoCs (e.g. most
> >   non-ARM ones) DMA is coherent with the caches, so the CPU
> >   on the endpoint side may map the buffer as cached and
> >   still be coherent with a CPU on the PCI host side that has
> >   mapped it with ioremap().
> 
> 
> Kind regards,
> Niklas

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Fri, Mar 15, 2024 at 06:29:52PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
> > On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
> >> "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".
> >> 
> >
> > I'm not sure this is correct. Memory returned by 'dma_alloc_coherent' is not the
> > 'normal memory' but rather 'consistent/coherent memory'. Here the question is,
> > can the memory returned by dma_alloc_coherent() be prefetched or write-combined
> > on all architectures.
> >
> > I hope Arnd can answer this question.
> 
> I think there are three separate questions here when talking about
> a scenario where a PCI master accesses memory behind a PCI endpoint:
> 
> - The CPU on the host side ususally uses ioremap() for mapping
>   the PCI BAR of the device. If the BAR is marked as prefetchable,
>   we usually allow mapping it using ioremap_wc() for write-combining
>   or ioremap_wt() for a write-through mappings that allow both
>   write-combining and prefetching. On some architectures, these
>   all fall back to normal register mappings which do none of these.
>   If it uses write-combining or prefetching, the host side driver
>   will need to manually serialize against concurrent access from
>   the endpoint side.
> 
> - The endpoint device accessing a buffer in memory is controlled
>   by the endpoint driver and may decide to prefetch data into a
>   local cache independent of the other two. I don't know if any
>   of the suppored endpoint devices actually do that. A prefetch
>   from the PCI host side would appear as a normal transaction here.
> 
> - The local CPU on the endpoint side may access the same buffer as
>   the endpoint device. On low-end SoCs the DMA from the PCI
>   endpoint is not coherent with the CPU caches, so the CPU may
>   need to map it as uncacheable to allow data consistency with
>   a the CPU on the PCI host side. On higher-end SoCs (e.g. most
>   non-ARM ones) DMA is coherent with the caches, so the CPU
>   on the endpoint side may map the buffer as cached and
>   still be coherent with a CPU on the PCI host side that has
>   mapped it with ioremap().
> 

Thanks Arnd for the reply.

But I'm not sure I got the answer I was looking for. So let me rephrase my
question a bit.

For BAR memory, PCIe spec states that,

'A PCI Express Function requesting Memory Space through a BAR must set the BAR's
Prefetchable bit unless the range contains locations with read side effects or
locations in which the Function does not tolerate write merging'

So here, spec refers the backing memory allocated on the endpoint side as the
'range' i.e, the BAR memory allocated on the host that gets mapped on the
endpoint.

Currently on the endpoint side, we use dma_alloc_coherent() to allocate the
memory for each BAR and map it using iATU.

So I want to know if the memory range allocated in the endpoint through
dma_alloc_coherent() satisfies the above two conditions in PCIe spec on all
architectures:

1. No Read side effects
2. Tolerates write merging

I believe the reason why we are allocating the coherent memory on the endpoint
first up is not all PCIe controllers are DMA coherent as you said above.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Arnd Bergmann]

On Mon, Mar 18, 2024, at 05:30, Manivannan Sadhasivam wrote:
> On Fri, Mar 15, 2024 at 06:29:52PM +0100, Arnd Bergmann wrote:
>> On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
>> > On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
>
> But I'm not sure I got the answer I was looking for. So let me rephrase my
> question a bit.
>
> For BAR memory, PCIe spec states that,
>
> 'A PCI Express Function requesting Memory Space through a BAR must set the BAR's
> Prefetchable bit unless the range contains locations with read side effects or
> locations in which the Function does not tolerate write merging'
>
> So here, spec refers the backing memory allocated on the endpoint side as the
> 'range' i.e, the BAR memory allocated on the host that gets mapped on the
> endpoint.
>
> Currently on the endpoint side, we use dma_alloc_coherent() to allocate the
> memory for each BAR and map it using iATU.
>
> So I want to know if the memory range allocated in the endpoint through
> dma_alloc_coherent() satisfies the above two conditions in PCIe spec on all
> architectures:
>
> 1. No Read side effects
> 2. Tolerates write merging
>
> I believe the reason why we are allocating the coherent memory on the endpoint
> first up is not all PCIe controllers are DMA coherent as you said above.

As far as I can tell, we never have read side effects for memory
backed BARs, but the write merging is something that depends on
how the memory is used:

If you have anything in that memory that relies on ordering,
you probably want to map it as coherent on the endpoint side,
and non-prefetchable on the host controller side, and then
use the normal rmb()/wmb() barriers on both ends between
serialized accesses. An example of this would be having blocks
of data separate from metadata that says whether the data is
valid.

If you don't care about ordering on that level, I would use
dma_map_sg() on the endpoint side and prefetchable mapping on
the host side, with the endpoint using dma_sync_*() to pass
buffer ownership between the two sides, as controlled by some
other communication method (non-prefetchable BAR, MSI, ...).

     Arnd

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

[Arnd Bergmann]

On Sun, Mar 17, 2024, at 12:54, Niklas Cassel wrote:
> On Fri, Mar 15, 2024 at 06:29:52PM +0100, Arnd Bergmann wrote:
>> On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
>> 
>> I think there are three separate questions here when talking about
>> a scenario where a PCI master accesses memory behind a PCI endpoint:
>
> I think the question is if the PCI epf-core, which runs on the endpoint
> side, and which calls dma_alloc_coherent() to allocate backing memory for
> a BAR, can set/mark the Prefetchable bit for the BAR (if we also set/mark
> the BAR as a 64-bit BAR).
>
> The PCIe 6.0 spec, 7.5.1.2.1 Base Address Registers (Offset 10h - 24h),
> states:
> "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."
>
> Does not backing memory allocated for a specific BAR using
> dma_alloc_coherent() on the EP side behave like normal memory from the
> host's point of view?

I'm not sure I follow this logic: If the device wants the
buffer to act like "normal memory", then it can be marked
as prefetchable and mapped into the host as write-combining,
but I think in this case you *don't* want it to be coherent
on the endpoint side either but use a streaming mapping with
explicit cache management instead.

Conversely, if the endpoint side requires a coherent mapping,
then I think you will want a strictly ordered (non-wc,
non-frefetchable) mapping on the host side as well.

It would be helpful to have actual endpoint function drivers
in the kernel rather than just the test drivers to see what type
of serialization you actually want for best performance on
both sides.

Can you give a specific example of an endpoint that you are
actually interested in, maybe just one that we have a host-side
device driver for in tree?

> On the host side, this will mean that the host driver sees the
> Prefetchable bit, and as according to:
> https://docs.kernel.org/driver-api/device-io.html
> The host might map the BAR using ioremap_wc().
>
> Looking specifically at drivers/misc/pci_endpoint_test.c, it maps the
> BARs using pci_ioremap_bar():
> https://elixir.bootlin.com/linux/v6.8/source/drivers/pci/pci.c#L252
> which will not map it using ioremap_wc().
> (But the code we have in the PCI epf-core must of course work with host
> side drivers other than pci_endpoint_test.c as well.)

It is to some degree architecture specific here. On powerpc
and i386 with MTTRs, any prefetchable BAR will be mapped as
write-combining IIRC, but on arm and arm64 it only depends on
whether the host side driver uses ioremap() or ioremap_wc().

>> - The local CPU on the endpoint side may access the same buffer as
>>   the endpoint device. On low-end SoCs the DMA from the PCI
>>   endpoint is not coherent with the CPU caches, so the CPU may
>
> I don't follow. When doing DMA *from* the endpoint, then the DMA HW
> on the EP side will read or write data to a buffer allocated on the
> host side (most likely using dma_alloc_coherent()), but what does
> that got to do with how the EP configures the BARs that it exposes?

I meant doing DMA to the memory of the endpoint side, not the
host side. DMA to the host side memory is completely separate
from this question.

     Arnd

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

[Niklas Cassel]

Hello Arnd,

On Mon, Mar 18, 2024 at 08:25:36AM +0100, Arnd Bergmann wrote:
> 
> I'm not sure I follow this logic: If the device wants the
> buffer to act like "normal memory", then it can be marked
> as prefetchable and mapped into the host as write-combining,
> but I think in this case you *don't* want it to be coherent
> on the endpoint side either but use a streaming mapping with
> explicit cache management instead.
> 
> Conversely, if the endpoint side requires a coherent mapping,
> then I think you will want a strictly ordered (non-wc,
> non-frefetchable) mapping on the host side as well.
> 
> It would be helpful to have actual endpoint function drivers
> in the kernel rather than just the test drivers to see what type
> of serialization you actually want for best performance on
> both sides.

Yes, that would be nice.

This specific API, pci_epf_alloc_space(), is only used by the
following drivers:
drivers/pci/endpoint/functions/pci-epf-test.c
drivers/pci/endpoint/functions/pci-epf-ntb.c
drivers/pci/endpoint/functions/pci-epf-vntb.c

pci_epf_alloc_space() is only used to allocate backing
memory for the BARs.


> 
> Can you give a specific example of an endpoint that you are
> actually interested in, maybe just one that we have a host-side
> device driver for in tree?

I personally just care about pci-epf-test, but obviously I don't
want to regress any other user of pci_epf_alloc_space().

Looking at the endpoint side driver:
drivers/pci/endpoint/functions/pci-epf-test.c
and the host side driver:
drivers/misc/pci_endpoint_test.c

On the RC side, allocating buffers that the EP will DMA to is
done using: kzalloc() + dma_map_single().

On EP side:
drivers/pci/endpoint/functions/pci-epf-test.c
uses dma_map_single() when using DMA, and signals completion using MSI.

On EP side:
When reading/writing to the BARs, it simply does:
READ_ONCE()/WRITE_ONCE():
https://github.com/torvalds/linux/blob/v6.8/drivers/pci/endpoint/functions/pci-epf-test.c#L643-L648

There is no dma_sync(), so the pci-test-epf driver currently seems to
depend on the backing memory being allocated by dma_alloc_coherent().


> If you don't care about ordering on that level, I would use
> dma_map_sg() on the endpoint side and prefetchable mapping on
> the host side, with the endpoint using dma_sync_*() to pass
> buffer ownership between the two sides, as controlled by some
> other communication method (non-prefetchable BAR, MSI, ...).

I don't think that there is no big reason why pci-epf-test is
implemented using dma_alloc_coherent() rather than dma_sync()
for the memory backing the BARs, but that is the way it is.

Since I don't feel like totally rewriting pci-epf-test, and since
you say that we shouldn't use dma_alloc_coherent() for the memory
backing the BARs together with exporting the BAR as prefetchable,
I will drop this patch from the series in the next revision.


Kind regards,
Niklas

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

[Arnd Bergmann]

On Mon, Mar 18, 2024, at 16:13, Niklas Cassel wrote:
> On Mon, Mar 18, 2024 at 08:25:36AM +0100, Arnd Bergmann wrote:
>
> I personally just care about pci-epf-test, but obviously I don't
> want to regress any other user of pci_epf_alloc_space().
>
> Looking at the endpoint side driver:
> drivers/pci/endpoint/functions/pci-epf-test.c
> and the host side driver:
> drivers/misc/pci_endpoint_test.c
>
> On the RC side, allocating buffers that the EP will DMA to is
> done using: kzalloc() + dma_map_single().
>
> On EP side:
> drivers/pci/endpoint/functions/pci-epf-test.c
> uses dma_map_single() when using DMA, and signals completion using MSI.
>
> On EP side:
> When reading/writing to the BARs, it simply does:
> READ_ONCE()/WRITE_ONCE():
> https://github.com/torvalds/linux/blob/v6.8/drivers/pci/endpoint/functions/pci-epf-test.c#L643-L648
>
> There is no dma_sync(), so the pci-test-epf driver currently seems to
> depend on the backing memory being allocated by dma_alloc_coherent().

From my reading of that function, this is really some kind
of command buffer that implements individual structured
registers and can be accessed from both sides at the same
time, so it would not actually make sense with the streaming
interface and wc/prefetchable access in place of explicit
READ_ONCE/WRITE_ONCE and readl/writel accesses.

>> If you don't care about ordering on that level, I would use
>> dma_map_sg() on the endpoint side and prefetchable mapping on
>> the host side, with the endpoint using dma_sync_*() to pass
>> buffer ownership between the two sides, as controlled by some
>> other communication method (non-prefetchable BAR, MSI, ...).
>
> I don't think that there is no big reason why pci-epf-test is
> implemented using dma_alloc_coherent() rather than dma_sync()
> for the memory backing the BARs, but that is the way it is.
>
> Since I don't feel like totally rewriting pci-epf-test, and since
> you say that we shouldn't use dma_alloc_coherent() for the memory
> backing the BARs together with exporting the BAR as prefetchable,
> I will drop this patch from the series in the next revision.

Ok. It might still be useful to extend the driver to also
allow transferring streaming data through a BAR on the
endpoint side. From what I can tell, it currently supports
using either slave DMA or a RC side buffer that ioremapped
into the endpoint, but that uses a regular ioremap() as well.
Mapping the RC side buffer as WC should make it possible to
transfer data from EP to RC more efficiently, but for the RC
to EP transfers you really want the buffer to be allocated on
the EP, so you can ioremap_wc() it to the RC for a memcpy_toio,
or cacheable read from the EP.

      Arnd

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

[Manivannan Sadhasivam]

On Mon, Mar 18, 2024 at 07:44:21AM +0100, Arnd Bergmann wrote:
> On Mon, Mar 18, 2024, at 05:30, Manivannan Sadhasivam wrote:
> > On Fri, Mar 15, 2024 at 06:29:52PM +0100, Arnd Bergmann wrote:
> >> On Fri, Mar 15, 2024, at 07:44, Manivannan Sadhasivam wrote:
> >> > On Wed, Mar 13, 2024 at 11:58:01AM +0100, Niklas Cassel wrote:
> >
> > But I'm not sure I got the answer I was looking for. So let me rephrase my
> > question a bit.
> >
> > For BAR memory, PCIe spec states that,
> >
> > 'A PCI Express Function requesting Memory Space through a BAR must set the BAR's
> > Prefetchable bit unless the range contains locations with read side effects or
> > locations in which the Function does not tolerate write merging'
> >
> > So here, spec refers the backing memory allocated on the endpoint side as the
> > 'range' i.e, the BAR memory allocated on the host that gets mapped on the
> > endpoint.
> >
> > Currently on the endpoint side, we use dma_alloc_coherent() to allocate the
> > memory for each BAR and map it using iATU.
> >
> > So I want to know if the memory range allocated in the endpoint through
> > dma_alloc_coherent() satisfies the above two conditions in PCIe spec on all
> > architectures:
> >
> > 1. No Read side effects
> > 2. Tolerates write merging
> >
> > I believe the reason why we are allocating the coherent memory on the endpoint
> > first up is not all PCIe controllers are DMA coherent as you said above.
> 
> As far as I can tell, we never have read side effects for memory
> backed BARs, but the write merging is something that depends on
> how the memory is used:
> 
> If you have anything in that memory that relies on ordering,
> you probably want to map it as coherent on the endpoint side,
> and non-prefetchable on the host controller side, and then
> use the normal rmb()/wmb() barriers on both ends between
> serialized accesses. An example of this would be having blocks
> of data separate from metadata that says whether the data is
> valid.
> 
> If you don't care about ordering on that level, I would use
> dma_map_sg() on the endpoint side and prefetchable mapping on
> the host side, with the endpoint using dma_sync_*() to pass
> buffer ownership between the two sides, as controlled by some
> other communication method (non-prefetchable BAR, MSI, ...).
> 

Right now, there are only Test and a couple of NTB drivers making use of the
pci_epf_alloc_space() API and they do not need streaming DMA.

So to conclude, we should just live with coherent allocation/non-prefetch for
now and extend it to streaming DMA/prefetch once we have a function driver that
needs it.

Thanks a lot for your inputs!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Mon, Mar 18, 2024 at 04:49:07PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 18, 2024, at 16:13, Niklas Cassel wrote:
> > On Mon, Mar 18, 2024 at 08:25:36AM +0100, Arnd Bergmann wrote:
> >
> > I personally just care about pci-epf-test, but obviously I don't
> > want to regress any other user of pci_epf_alloc_space().
> >
> > Looking at the endpoint side driver:
> > drivers/pci/endpoint/functions/pci-epf-test.c
> > and the host side driver:
> > drivers/misc/pci_endpoint_test.c
> >
> > On the RC side, allocating buffers that the EP will DMA to is
> > done using: kzalloc() + dma_map_single().
> >
> > On EP side:
> > drivers/pci/endpoint/functions/pci-epf-test.c
> > uses dma_map_single() when using DMA, and signals completion using MSI.
> >
> > On EP side:
> > When reading/writing to the BARs, it simply does:
> > READ_ONCE()/WRITE_ONCE():
> > https://github.com/torvalds/linux/blob/v6.8/drivers/pci/endpoint/functions/pci-epf-test.c#L643-L648
> >
> > There is no dma_sync(), so the pci-test-epf driver currently seems to
> > depend on the backing memory being allocated by dma_alloc_coherent().
> 
> From my reading of that function, this is really some kind
> of command buffer that implements individual structured
> registers and can be accessed from both sides at the same
> time, so it would not actually make sense with the streaming
> interface and wc/prefetchable access in place of explicit
> READ_ONCE/WRITE_ONCE and readl/writel accesses.
> 

Right. We should stick to the current implementation for now until a function
driver with streaming DMA usecase comes in.

- Mani

> >> If you don't care about ordering on that level, I would use
> >> dma_map_sg() on the endpoint side and prefetchable mapping on
> >> the host side, with the endpoint using dma_sync_*() to pass
> >> buffer ownership between the two sides, as controlled by some
> >> other communication method (non-prefetchable BAR, MSI, ...).
> >
> > I don't think that there is no big reason why pci-epf-test is
> > implemented using dma_alloc_coherent() rather than dma_sync()
> > for the memory backing the BARs, but that is the way it is.
> >
> > Since I don't feel like totally rewriting pci-epf-test, and since
> > you say that we shouldn't use dma_alloc_coherent() for the memory
> > backing the BARs together with exporting the BAR as prefetchable,
> > I will drop this patch from the series in the next revision.
> 
> Ok. It might still be useful to extend the driver to also
> allow transferring streaming data through a BAR on the
> endpoint side. From what I can tell, it currently supports
> using either slave DMA or a RC side buffer that ioremapped
> into the endpoint, but that uses a regular ioremap() as well.
> Mapping the RC side buffer as WC should make it possible to
> transfer data from EP to RC more efficiently, but for the RC
> to EP transfers you really want the buffer to be allocated on
> the EP, so you can ioremap_wc() it to the RC for a memcpy_toio,
> or cacheable read from the EP.
> 
>       Arnd

-- 
மணிவண்ணன் சதாசிவம்

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

[Niklas Cassel]

On Fri, Mar 15, 2024 at 10:50:45AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:57:53AM +0100, Niklas Cassel wrote:
> > 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.
> > 
> 
> Max BAR size is 1MB currently, so I believe you are adding the check just for
> the sake of correctness. If so, please mention it explicitly.

The BAR size defined in:
static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
will only be used if the BAR is not fixed size.
(A fixed BAR size will override the size in static size_t bar_size[])

So a platform could have a fixed size BAR of e.g. 4GB.

Although, you could argue that if a platform has a fixed BAR size of 4GB,
they should have also marked the BAR as "only 64-bit".
(Considering that the largest size supported by a 32-bit BAR is 2GB.)

I will drop this patch in V4.


Kind regards,
Niklas


> 
> - Mani
> 
> > 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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

[Niklas Cassel]

On Fri, Mar 15, 2024 at 11:09:03AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:57:57AM +0100, Niklas Cassel wrote:
> > 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.
> > 
> 
> That's a valid assumption. But...
> 
> > 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.)
> > 
> 
> Looks like pcie-cadence-ep is setting the PCI_BASE_ADDRESS_MEM_TYPE_64 flag if
> the requested size is >2GB (I don't know why 2GB instead of 4GB in the first
> place).

That is dead code that will never be able to execute.

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 > 2 GB.

Yes, 2GB!
The author of f25b5fae29d4 is an idiot (yes, it is me).
Anyway, the code itself in that commit is doing the right thing...

2GB is the maximum size of a 32-bit BAR.

So, since the the code in pcie-cadence-ep is dead code, I don't see
any problem with this commit.


Kind regards,
Niklas


> 
> - Mani
> 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++---------------
> >  1 file changed, 4 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 20c79610712d..91bbfcb1b3ed 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -709,31 +709,18 @@ 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);
> >  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> > -	const struct pci_epc_features *epc_features;
> > -
> > -	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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

[Niklas Cassel]

On Fri, Mar 15, 2024 at 11:12:55AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:57:58AM +0100, Niklas Cassel wrote:
> > 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().
> > 
> 
> I've proposed to move the clear_bar and free_space code to separate helper
> functions in my series [1]. If this series gets merged first (it really makes
> sense), then this patch can be dropped now.

I've been a bit busy this week, hopefully I will have time to review your two
outstanding series before the end of the week.

I do think that this series is smaller and less complex than your two series,
so if you ask me, I think it would make sense if this series (the respin of
this series) goes first.


Kind regards,
Niklas

> 
> - Mani
> 
> > 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 91bbfcb1b3ed..fbe14c7232c7 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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

[Niklas Cassel]

On Fri, Mar 15, 2024 at 11:16:16AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:57:59AM +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
> 
> 2 GB

Will fix in V4.


> 
> > 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>
> 
> Okay, here you are fixing this driver. But this should be moved before patch
> 5/9. With that,

As I wrote in my reply to patch 5/9, I don't think we need to move them
around.

The code that sets flag PCI_BASE_ADDRESS_MEM_TYPE_64 in this driver is dead code,
it couldn't happen before this commit. So unless you insist, I think it is better
to keep the current ordering, such that all pci-epf-test patches are after each
other.


Kind regards,
Niklas

> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> - Mani
> 
> > ---
> >  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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

On Wed, Mar 13, 2024 at 11:58:00AM +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.
> 
> 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.

Does this (and the similar cadence patch) need a Fixes: tag for
f25b5fae29d4?

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

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

[Bjorn Helgaas]

On Wed, Mar 13, 2024 at 11:58:00AM +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.
> 
> 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.

Does this (and the similar cadence patch) need a Fixes: tag for
f25b5fae29d4?

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Bjorn Helgaas]

On Wed, Mar 13, 2024 at 11:58:00AM +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.
> 
> 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.

Does this (and the similar cadence patch) need a Fixes: tag for
f25b5fae29d4?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> ...

> --- 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;

Completely unrelated to *these* patches, but the BAR_CFG_CTRL
definitions in both cadence and rockchip lead to some awkward case
analysis:

  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7

  if (is_64bits && is_prefetch)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
  else if (is_prefetch)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
  else if (is_64bits)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
  else
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;

that *could* be just something like this:

  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1

  ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
  if (is_64bits)
    ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
  if (is_prefetch)
    ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;

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

[Bjorn Helgaas]

On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> ...

> --- 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;

Completely unrelated to *these* patches, but the BAR_CFG_CTRL
definitions in both cadence and rockchip lead to some awkward case
analysis:

  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7

  if (is_64bits && is_prefetch)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
  else if (is_prefetch)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
  else if (is_64bits)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
  else
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;

that *could* be just something like this:

  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1

  ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
  if (is_64bits)
    ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
  if (is_prefetch)
    ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Bjorn Helgaas]

On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> ...

> --- 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;

Completely unrelated to *these* patches, but the BAR_CFG_CTRL
definitions in both cadence and rockchip lead to some awkward case
analysis:

  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7

  if (is_64bits && is_prefetch)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
  else if (is_prefetch)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
  else if (is_64bits)
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
  else
          ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;

that *could* be just something like this:

  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
  #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1

  ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
  if (is_64bits)
    ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
  if (is_prefetch)
    ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Niklas Cassel]

On Fri, Apr 12, 2024 at 12:51:27PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2024 at 11:58:00AM +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.
> > 
> > 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.
> 
> Does this (and the similar cadence patch) need a Fixes: tag for
> f25b5fae29d4?

I don't think so.

Both patches are about respecting the configuration requested by an EPF
driver.

So if an EPF driver requests a 64-bit BAR, the EPC driver should configure
that. (Regardless of the size that the EPF driver requests for the BAR.)

If we really want a Fixes-tag, I would imagine that it will be the respective
initial commits that added these drivers (pcie-cadence-ep.c and
pcie-rockchip-ep.c), as it has been this way since then.

If you look at the EPF drivers we currently have, they will currently only
request a 64-bit BAR if any of the BARs can only be configured as a 64-bit
BAR because of hardware limitiations.

$ git grep only_64bit

Neither of these two drivers have any such hardware limitiations,
so these commits are currently a bit pointless.

However, the drivers should of course do the right thing, because other
EPC drivers might look at them and copy their code.

And who knows, maybe sometime in the future there will be an EPF driver
that will explicitly request a 64-bit BAR, regardless of size.

TL;DR: I don't think these two commits are worth backporting.


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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Niklas Cassel]

On Fri, Apr 12, 2024 at 12:51:27PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2024 at 11:58:00AM +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.
> > 
> > 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.
> 
> Does this (and the similar cadence patch) need a Fixes: tag for
> f25b5fae29d4?

I don't think so.

Both patches are about respecting the configuration requested by an EPF
driver.

So if an EPF driver requests a 64-bit BAR, the EPC driver should configure
that. (Regardless of the size that the EPF driver requests for the BAR.)

If we really want a Fixes-tag, I would imagine that it will be the respective
initial commits that added these drivers (pcie-cadence-ep.c and
pcie-rockchip-ep.c), as it has been this way since then.

If you look at the EPF drivers we currently have, they will currently only
request a 64-bit BAR if any of the BARs can only be configured as a 64-bit
BAR because of hardware limitiations.

$ git grep only_64bit

Neither of these two drivers have any such hardware limitiations,
so these commits are currently a bit pointless.

However, the drivers should of course do the right thing, because other
EPC drivers might look at them and copy their code.

And who knows, maybe sometime in the future there will be an EPF driver
that will explicitly request a 64-bit BAR, regardless of size.

TL;DR: I don't think these two commits are worth backporting.


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

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

[Niklas Cassel]

On Fri, Apr 12, 2024 at 12:51:27PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2024 at 11:58:00AM +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.
> > 
> > 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.
> 
> Does this (and the similar cadence patch) need a Fixes: tag for
> f25b5fae29d4?

I don't think so.

Both patches are about respecting the configuration requested by an EPF
driver.

So if an EPF driver requests a 64-bit BAR, the EPC driver should configure
that. (Regardless of the size that the EPF driver requests for the BAR.)

If we really want a Fixes-tag, I would imagine that it will be the respective
initial commits that added these drivers (pcie-cadence-ep.c and
pcie-rockchip-ep.c), as it has been this way since then.

If you look at the EPF drivers we currently have, they will currently only
request a 64-bit BAR if any of the BARs can only be configured as a 64-bit
BAR because of hardware limitiations.

$ git grep only_64bit

Neither of these two drivers have any such hardware limitiations,
so these commits are currently a bit pointless.

However, the drivers should of course do the right thing, because other
EPC drivers might look at them and copy their code.

And who knows, maybe sometime in the future there will be an EPF driver
that will explicitly request a 64-bit BAR, regardless of size.

TL;DR: I don't think these two commits are worth backporting.


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

On Fri, Apr 12, 2024 at 11:39:39PM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 12:51:27PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2024 at 11:58:00AM +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.
> > > 
> > > 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.
> > 
> > Does this (and the similar cadence patch) need a Fixes: tag for
> > f25b5fae29d4?
> 
> I don't think so.
> 
> Both patches are about respecting the configuration requested by an EPF
> driver.
> 
> So if an EPF driver requests a 64-bit BAR, the EPC driver should configure
> that. (Regardless of the size that the EPF driver requests for the BAR.)
> 
> If we really want a Fixes-tag, I would imagine that it will be the respective
> initial commits that added these drivers (pcie-cadence-ep.c and
> pcie-rockchip-ep.c), as it has been this way since then.
> 
> If you look at the EPF drivers we currently have, they will currently only
> request a 64-bit BAR if any of the BARs can only be configured as a 64-bit
> BAR because of hardware limitiations.
> 
> $ git grep only_64bit
> 
> Neither of these two drivers have any such hardware limitiations,
> so these commits are currently a bit pointless.
> 
> However, the drivers should of course do the right thing, because other
> EPC drivers might look at them and copy their code.
> 
> And who knows, maybe sometime in the future there will be an EPF driver
> that will explicitly request a 64-bit BAR, regardless of size.
> 
> TL;DR: I don't think these two commits are worth backporting.

OK, thanks!

> > > 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
> > > 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Bjorn Helgaas]

On Fri, Apr 12, 2024 at 11:39:39PM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 12:51:27PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2024 at 11:58:00AM +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.
> > > 
> > > 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.
> > 
> > Does this (and the similar cadence patch) need a Fixes: tag for
> > f25b5fae29d4?
> 
> I don't think so.
> 
> Both patches are about respecting the configuration requested by an EPF
> driver.
> 
> So if an EPF driver requests a 64-bit BAR, the EPC driver should configure
> that. (Regardless of the size that the EPF driver requests for the BAR.)
> 
> If we really want a Fixes-tag, I would imagine that it will be the respective
> initial commits that added these drivers (pcie-cadence-ep.c and
> pcie-rockchip-ep.c), as it has been this way since then.
> 
> If you look at the EPF drivers we currently have, they will currently only
> request a 64-bit BAR if any of the BARs can only be configured as a 64-bit
> BAR because of hardware limitiations.
> 
> $ git grep only_64bit
> 
> Neither of these two drivers have any such hardware limitiations,
> so these commits are currently a bit pointless.
> 
> However, the drivers should of course do the right thing, because other
> EPC drivers might look at them and copy their code.
> 
> And who knows, maybe sometime in the future there will be an EPF driver
> that will explicitly request a 64-bit BAR, regardless of size.
> 
> TL;DR: I don't think these two commits are worth backporting.

OK, thanks!

> > > 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
> > > 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Bjorn Helgaas]

On Fri, Apr 12, 2024 at 11:39:39PM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 12:51:27PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2024 at 11:58:00AM +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.
> > > 
> > > 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.
> > 
> > Does this (and the similar cadence patch) need a Fixes: tag for
> > f25b5fae29d4?
> 
> I don't think so.
> 
> Both patches are about respecting the configuration requested by an EPF
> driver.
> 
> So if an EPF driver requests a 64-bit BAR, the EPC driver should configure
> that. (Regardless of the size that the EPF driver requests for the BAR.)
> 
> If we really want a Fixes-tag, I would imagine that it will be the respective
> initial commits that added these drivers (pcie-cadence-ep.c and
> pcie-rockchip-ep.c), as it has been this way since then.
> 
> If you look at the EPF drivers we currently have, they will currently only
> request a 64-bit BAR if any of the BARs can only be configured as a 64-bit
> BAR because of hardware limitiations.
> 
> $ git grep only_64bit
> 
> Neither of these two drivers have any such hardware limitiations,
> so these commits are currently a bit pointless.
> 
> However, the drivers should of course do the right thing, because other
> EPC drivers might look at them and copy their code.
> 
> And who knows, maybe sometime in the future there will be an EPF driver
> that will explicitly request a 64-bit BAR, regardless of size.
> 
> TL;DR: I don't think these two commits are worth backporting.

OK, thanks!

> > > 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
> > > 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Niklas Cassel]

On Fri, Apr 12, 2024 at 01:59:01PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> > ...
> 
> > --- 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;
> 
> Completely unrelated to *these* patches, but the BAR_CFG_CTRL
> definitions in both cadence and rockchip lead to some awkward case
> analysis:
> 
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7
> 
>   if (is_64bits && is_prefetch)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
>   else if (is_prefetch)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
>   else if (is_64bits)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
>   else
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;
> 
> that *could* be just something like this:
> 
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1
> 
>   ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
>   if (is_64bits)
>     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
>   if (is_prefetch)
>     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;


If you send the cleanup patches, I will send the Reviewed-by tags ;)


Kind regards,
Niklas

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

[Niklas Cassel]

On Fri, Apr 12, 2024 at 01:59:01PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> > ...
> 
> > --- 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;
> 
> Completely unrelated to *these* patches, but the BAR_CFG_CTRL
> definitions in both cadence and rockchip lead to some awkward case
> analysis:
> 
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7
> 
>   if (is_64bits && is_prefetch)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
>   else if (is_prefetch)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
>   else if (is_64bits)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
>   else
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;
> 
> that *could* be just something like this:
> 
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1
> 
>   ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
>   if (is_64bits)
>     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
>   if (is_prefetch)
>     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;


If you send the cleanup patches, I will send the Reviewed-by tags ;)


Kind regards,
Niklas

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Niklas Cassel]

On Fri, Apr 12, 2024 at 01:59:01PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> > ...
> 
> > --- 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;
> 
> Completely unrelated to *these* patches, but the BAR_CFG_CTRL
> definitions in both cadence and rockchip lead to some awkward case
> analysis:
> 
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7
> 
>   if (is_64bits && is_prefetch)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
>   else if (is_prefetch)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
>   else if (is_64bits)
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
>   else
>           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;
> 
> that *could* be just something like this:
> 
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
>   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1
> 
>   ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
>   if (is_64bits)
>     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
>   if (is_prefetch)
>     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;


If you send the cleanup patches, I will send the Reviewed-by tags ;)


Kind regards,
Niklas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

On Sat, Apr 13, 2024 at 12:02:12AM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 01:59:01PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> > > ...
> > 
> > > --- 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;
> > 
> > Completely unrelated to *these* patches, but the BAR_CFG_CTRL
> > definitions in both cadence and rockchip lead to some awkward case
> > analysis:
> > 
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7
> > 
> >   if (is_64bits && is_prefetch)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
> >   else if (is_prefetch)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
> >   else if (is_64bits)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
> >   else
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;
> > 
> > that *could* be just something like this:
> > 
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1
> > 
> >   ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
> >   if (is_64bits)
> >     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
> >   if (is_prefetch)
> >     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;
> 
> If you send the cleanup patches, I will send the Reviewed-by tags ;)

Fair enough :)

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

[Bjorn Helgaas]

On Sat, Apr 13, 2024 at 12:02:12AM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 01:59:01PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> > > ...
> > 
> > > --- 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;
> > 
> > Completely unrelated to *these* patches, but the BAR_CFG_CTRL
> > definitions in both cadence and rockchip lead to some awkward case
> > analysis:
> > 
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7
> > 
> >   if (is_64bits && is_prefetch)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
> >   else if (is_prefetch)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
> >   else if (is_64bits)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
> >   else
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;
> > 
> > that *could* be just something like this:
> > 
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1
> > 
> >   ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
> >   if (is_64bits)
> >     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
> >   if (is_prefetch)
> >     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;
> 
> If you send the cleanup patches, I will send the Reviewed-by tags ;)

Fair enough :)

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Bjorn Helgaas]

On Sat, Apr 13, 2024 at 12:02:12AM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 01:59:01PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2024 at 11:58:00AM +0100, Niklas Cassel wrote:
> > > ...
> > 
> > > --- 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;
> > 
> > Completely unrelated to *these* patches, but the BAR_CFG_CTRL
> > definitions in both cadence and rockchip lead to some awkward case
> > analysis:
> > 
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS              0x4
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS     0x5
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS              0x6
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS     0x7
> > 
> >   if (is_64bits && is_prefetch)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
> >   else if (is_prefetch)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
> >   else if (is_64bits)
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_64BITS;
> >   else
> >           ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM_32BITS;
> > 
> > that *could* be just something like this:
> > 
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM           0x4
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS        0x2
> >   #define ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH      0x1
> > 
> >   ctrl = ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_MEM;
> >   if (is_64bits)
> >     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_64BITS;
> >   if (is_prefetch)
> >     ctrl |= ROCKCHIP_PCIE_CORE_BAR_CFG_CTRL_PREFETCH;
> 
> If you send the cleanup patches, I will send the Reviewed-by tags ;)

Fair enough :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel