[PATCH v3 0/3] pci: endpoint: vntb: add MSI doorbell support

[PATCH v3 0/3] pci: endpoint: vntb: add MSI doorbell support

[Frank Li]

Since commit 1c3b002c6bf68 PCI: endpoint: Add RC-to-EP doorbell support
using platform MSI controller, PCI EP can get notification from Host.

VNTB use this feature to reduce ping latency.

The first patch impove epf core API to allow set any MMIO address to specfic
bar.

The second patch add MSI doorbell support.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v3:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v2: https://lore.kernel.org/r/20250915-vntb_msi_doorbell-v2-0-ca71605e3444@nxp.com

Changes in v2:
- add help funciton to get bar's inbounce size
- fix miss x8 when caculate bits
- Link to v1: https://lore.kernel.org/r/20250815-vntb_msi_doorbell-v1-0-32df6c4bf96c@nxp.com

---
Frank Li (3):
      PCI: endpoint: Add helper function pci_epf_get_bar_required_size()
      PCI: endpoint: Add API pci_epf_assign_bar_space()
      PCI: endpoint: pci-epf-vntb: Add MSI doorbell support

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 149 ++++++++++++++++++++---
 drivers/pci/endpoint/pci-epf-core.c           | 168 +++++++++++++++++++++-----
 include/linux/pci-epf.h                       |   6 +
 3 files changed, 275 insertions(+), 48 deletions(-)
---
base-commit: c2a282d1fccc53a989da61a5da4f03c9d67ee99a
change-id: 20250812-vntb_msi_doorbell-bf0fbac6d6d7

Best regards,
--
Frank Li <Frank.Li@nxp.com>

[PATCH v3 1/3] PCI: endpoint: Add helper function pci_epf_get_bar_required_size()

[Frank Li]

Introduce pci_epf_get_bar_required_size() to retrieve the required BAR
size and memory size. Prepare for adding support to set an MMIO address to
a specific BAR.

Use two variables 'aligned_bar_size' and 'aligned_mem_size' to avoid
confuse.

No functional changes.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v3
- change return value to int.
- use two pointers return bar size aligned and memory start address aligned
- update comments about why need memory align size. Actually iATU require
start address match aligned requirement. Since kernel return align to
size's address.
- use two varible aligned_bar_size and aligned_mem_size to avoid confuse
use 'size'.

change in v2
- new patch
---
 drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index d54e18872aefc07c655c94c104a347328ff7a432..2cd0257831f9885a4381c087ed8f3326f5960966 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -208,6 +208,49 @@ void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf)
 }
 EXPORT_SYMBOL_GPL(pci_epf_remove_vepf);
 
+static int
+pci_epf_get_bar_required_size(struct pci_epf *epf, size_t size,
+			      size_t *aligned_bar_size,
+			      size_t *aligned_mem_size,
+			      enum pci_barno bar,
+			      const struct pci_epc_features *epc_features,
+			      enum pci_epc_interface_type type)
+{
+	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
+	size_t align = epc_features->align;
+
+	if (size < 128)
+		size = 128;
+
+	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
+	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
+		size = SZ_1M;
+
+	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
+		if (size > bar_fixed_size) {
+			dev_err(&epf->dev,
+				"requested BAR size is larger than fixed size\n");
+			return -ENOMEM;
+		}
+		size = bar_fixed_size;
+	} else {
+		/* BAR size must be power of two */
+		size = roundup_pow_of_two(size);
+	}
+
+	*aligned_bar_size = size;
+
+	/*
+	 * The EPC's BAR start address must meet alignment requirements. In most
+	 * cases, the alignment will match the BAR size. However, differences
+	 * can occur—for example, when the fixed BAR size (e.g., 128 bytes) is
+	 * smaller than the required alignment (e.g., 4 KB).
+	 */
+	*aligned_mem_size = align ? ALIGN(size, align) : size;
+
+	return 0;
+}
+
 /**
  * pci_epf_free_space() - free the allocated PCI EPF register space
  * @epf: the EPF device from whom to free the memory
@@ -264,40 +307,17 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 			  const struct pci_epc_features *epc_features,
 			  enum pci_epc_interface_type type)
 {
-	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
-	size_t aligned_size, align = epc_features->align;
+	size_t aligned_mem_size, aligned_bar_size;
 	struct pci_epf_bar *epf_bar;
 	dma_addr_t phys_addr;
 	struct pci_epc *epc;
 	struct device *dev;
 	void *space;
 
-	if (size < 128)
-		size = 128;
-
-	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
-	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
-		size = SZ_1M;
-
-	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
-		if (size > bar_fixed_size) {
-			dev_err(&epf->dev,
-				"requested BAR size is larger than fixed size\n");
-			return NULL;
-		}
-		size = bar_fixed_size;
-	} else {
-		/* BAR size must be power of two */
-		size = roundup_pow_of_two(size);
-	}
-
-	/*
-	 * Allocate enough memory to accommodate the iATU alignment
-	 * requirement.  In most cases, this will be the same as .size but
-	 * it might be different if, for example, the fixed size of a BAR
-	 * is smaller than align.
-	 */
-	aligned_size = align ? ALIGN(size, align) : size;
+	if (pci_epf_get_bar_required_size(epf, size, &aligned_bar_size,
+					  &aligned_mem_size, bar,
+					  epc_features, type))
+		return NULL;
 
 	if (type == PRIMARY_INTERFACE) {
 		epc = epf->epc;
@@ -308,7 +328,9 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 	}
 
 	dev = epc->dev.parent;
-	space = dma_alloc_coherent(dev, aligned_size, &phys_addr, GFP_KERNEL);
+
+	space = dma_alloc_coherent(dev, aligned_mem_size,
+				   &phys_addr, GFP_KERNEL);
 	if (!space) {
 		dev_err(dev, "failed to allocate mem space\n");
 		return NULL;
@@ -316,8 +338,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 
 	epf_bar[bar].phys_addr = phys_addr;
 	epf_bar[bar].addr = space;
-	epf_bar[bar].size = size;
-	epf_bar[bar].aligned_size = aligned_size;
+	epf_bar[bar].size = aligned_bar_size;
+	epf_bar[bar].aligned_size = aligned_mem_size;
 	epf_bar[bar].barno = bar;
 	if (upper_32_bits(size) || epc_features->bar[bar].only_64bit)
 		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;

-- 
2.34.1

[PATCH v3 2/3] PCI: endpoint: Add API pci_epf_assign_bar_space()

[Frank Li]

Add pci_epf_assign_bar_space() to allow setting any physical address as
inbound memory space, such as an MSI message base address.

Since PCI BAR size must be a power of two, the input MMIO range
[inbound_addr, inbound_addr + size) is mapped by finding n such that
[base, base + 2^n) covers the range. The base address is also required
to be aligned to 2^n.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
chagne in v3.
- update function name to pci_epf_assign_bar_space()
- s/allocated/assigned/
- add check when align down input address to memory align require, may not
bar's size can't cover required ragion.

change in v2
- add new API pci_epf_set_inbound_space()
- fix bits 8 * size_of(dma_addr_t);
---
 drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++++++++++++++++
 include/linux/pci-epf.h             |  6 +++
 2 files changed, 90 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 2cd0257831f9885a4381c087ed8f3326f5960966..b5c33708358130b2d056ecb7e7cb602a76360875 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -350,6 +350,90 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 }
 EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
 
+/**
+ * pci_epf_assign_bar_space() - Assign PCI EPF BAR space
+ * @epf: the EPF device to whom allocate the memory
+ * @size: the size of the memory that has to be assigned
+ * @bar: the BAR number corresponding to the assigned register space
+ * @epc_features: the features provided by the EPC specific to this EPF
+ * @type: Identifies if the assignment is for primary EPC or secondary EPC
+ * @bar_addr: Address to be assigned for the @bar
+ *
+ * Invoke to assigned memory for the PCI EPF register space.
+ * Flag PCI_BASE_ADDRESS_MEM_TYPE_64 will automatically get set if the BAR
+ * can only be a 64-bit BAR, or if the requested size is larger than 2 GB.
+ */
+int pci_epf_assign_bar_space(struct pci_epf *epf, size_t size,
+			     enum pci_barno bar,
+			     const struct pci_epc_features *epc_features,
+			     enum pci_epc_interface_type type,
+			     dma_addr_t bar_addr)
+{
+	size_t aligned_bar_size, aligned_mem_size;
+	struct pci_epf_bar *epf_bar;
+	struct pci_epc *epc;
+	dma_addr_t limit;
+	int pos;
+
+	if (!size)
+		return -EINVAL;
+
+	limit = bar_addr + size - 1;
+
+	/*
+	 *  Bits:		15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
+	 *  bar_addr:		U  U  U  U  U  U  0 X X X X X X X X X
+	 *  limit:		U  U  U  U  U  U  1 X X X X X X X X X
+	 *
+	 *  U means address bits have not change in Range [bar_addr, limit]
+	 *  X means bit 0 or 1.
+	 *
+	 *  bar_addr^limit	0  0  0  0  0  0  1 X X X X X X X X X
+	 *  Find first bit 1 pos from MSB, 2 ^ pos windows will cover
+	 *  [bar_Addr, limit] range.
+	 */
+	for (pos = 8 * sizeof(dma_addr_t) - 1; pos > 0; pos--)
+		if ((limit ^ bar_addr) & BIT_ULL(pos))
+			break;
+
+	if (pos == 8 * sizeof(dma_addr_t) - 1)
+		return -EINVAL;
+
+	if (pci_epf_get_bar_required_size(epf, BIT_ULL(pos + 1),
+					  &aligned_bar_size,
+					  &aligned_mem_size, bar,
+					  epc_features, type))
+		return -ENOMEM;
+
+	if (size == 0)
+		return -EINVAL;
+
+	if (type == PRIMARY_INTERFACE) {
+		epc = epf->epc;
+		epf_bar = epf->bar;
+	} else {
+		epc = epf->sec_epc;
+		epf_bar = epf->sec_epc_bar;
+	}
+
+	epf_bar[bar].phys_addr = ALIGN_DOWN(bar_addr, aligned_mem_size);
+
+	if (epf_bar[bar].phys_addr + aligned_bar_size < limit)
+		return -ENOMEM;
+
+	epf_bar[bar].addr = NULL;
+	epf_bar[bar].size = aligned_bar_size;
+	epf_bar[bar].aligned_size = aligned_mem_size;
+	epf_bar[bar].barno = bar;
+	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 0;
+}
+EXPORT_SYMBOL_GPL(pci_epf_assign_bar_space);
+
 static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
 {
 	struct config_group *group, *tmp;
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 2e85504ba2baf93827224884ca19ae2bd0e6906b..d2cdcc06d366ce61db0d7633105965e50cb49a7b 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -242,6 +242,12 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
 			enum pci_epc_interface_type type);
 
+int pci_epf_assign_bar_space(struct pci_epf *epf, size_t size,
+			     enum pci_barno bar,
+			     const struct pci_epc_features *epc_features,
+			     enum pci_epc_interface_type type,
+			     dma_addr_t inbound_addr);
+
 int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
 			       u64 addr, dma_addr_t *base, size_t *off);
 int pci_epf_bind(struct pci_epf *epf);

-- 
2.34.1

[PATCH v3 3/3] PCI: endpoint: pci-epf-vntb: Add MSI doorbell support

[Frank Li]

Add MSI doorbell support to reduce latency between PCI host and EP.

Before this change:
  ping 169.254.172.137
  64 bytes from 169.254.172.137: icmp_seq=1 ttl=64 time=0.575 ms
  64 bytes from 169.254.172.137: icmp_seq=2 ttl=64 time=1.80 ms
  64 bytes from 169.254.172.137: icmp_seq=3 ttl=64 time=8.19 ms
  64 bytes from 169.254.172.137: icmp_seq=4 ttl=64 time=2.00 ms

After this change:
  ping 169.254.144.71
  64 bytes from 169.254.144.71: icmp_seq=1 ttl=64 time=0.215 ms
  64 bytes from 169.254.144.71: icmp_seq=2 ttl=64 time=0.456 ms
  64 bytes from 169.254.144.71: icmp_seq=3 ttl=64 time=0.448 ms

Change u64 db to atomic_64 because difference doorbell may happen at the
same time.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v3
- update api pci_epf_assign_bar_space
- remove dead code for db 0.

change in v2
- update api pci_epf_set_inbound_space
- atomic_64 should be enough, which just record doorbell events, which is
similar with W1C irq status register.
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 149 +++++++++++++++++++++++---
 1 file changed, 132 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 83e9ab10f9c4fc2b485d5463faa2172500f12999..efa00752eba0585426e18e52028101fb4e7d2570 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -36,11 +36,13 @@
  * PCIe Root Port                        PCI EP
  */
 
+#include <linux/atomic.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 
+#include <linux/pci-ep-msi.h>
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
 #include <linux/ntb.h>
@@ -126,12 +128,13 @@ struct epf_ntb {
 	u32 db_count;
 	u32 spad_count;
 	u64 mws_size[MAX_MW];
-	u64 db;
+	atomic64_t db;
 	u32 vbus_number;
 	u16 vntb_pid;
 	u16 vntb_vid;
 
 	bool linkup;
+	bool msi_doorbell;
 	u32 spad_size;
 
 	enum pci_barno epf_ntb_bar[VNTB_BAR_NUM];
@@ -258,9 +261,9 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 
 	ntb = container_of(work, struct epf_ntb, cmd_handler.work);
 
-	for (i = 1; i < ntb->db_count; i++) {
+	for (i = 1; i < ntb->db_count && !ntb->msi_doorbell; i++) {
 		if (ntb->epf_db[i]) {
-			ntb->db |= 1 << (i - 1);
+			atomic64_or(1 << (i - 1), &ntb->db);
 			ntb_db_event(&ntb->ntb, i);
 			ntb->epf_db[i] = 0;
 		}
@@ -319,7 +322,21 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 
 reset_handler:
 	queue_delayed_work(kpcintb_workqueue, &ntb->cmd_handler,
-			   msecs_to_jiffies(5));
+			   ntb->msi_doorbell ? msecs_to_jiffies(500) : msecs_to_jiffies(5));
+}
+
+static irqreturn_t epf_ntb_doorbell_handler(int irq, void *data)
+{
+	struct epf_ntb *ntb = data;
+	int i = 0;
+
+	for (i = 1; i < ntb->db_count; i++)
+		if (irq == ntb->epf->db_msg[i].virq) {
+			atomic64_or(1 << (i - 1), &ntb->db);
+			ntb_db_event(&ntb->ntb, i);
+		}
+
+	return IRQ_HANDLED;
 }
 
 /**
@@ -500,6 +517,89 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 	return 0;
 }
 
+static int epf_ntb_db_bar_init_msi_doorbell(struct epf_ntb *ntb,
+					    struct pci_epf_bar *db_bar,
+					    const struct pci_epc_features *epc_features,
+					    enum pci_barno barno)
+{
+	struct pci_epf *epf = ntb->epf;
+	dma_addr_t low, high;
+	struct msi_msg *msg;
+	size_t sz;
+	int ret;
+	int i;
+
+	ret = pci_epf_alloc_doorbell(epf,  ntb->db_count);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ntb->db_count; i++) {
+		ret = request_irq(epf->db_msg[i].virq, epf_ntb_doorbell_handler,
+				  0, "vntb_db", ntb);
+
+		if (ret) {
+			dev_err(&epf->dev,
+				"Failed to request doorbell IRQ: %d\n",
+				epf->db_msg[i].virq);
+			goto err_request_irq;
+		}
+	}
+
+	msg = &epf->db_msg[0].msg;
+
+	high = 0;
+	low = (u64)msg->address_hi << 32 | msg->address_lo;
+
+	for (i = 0; i < ntb->db_count; i++) {
+		struct msi_msg *msg = &epf->db_msg[i].msg;
+		dma_addr_t addr = (u64)msg->address_hi << 32 | msg->address_lo;
+
+		low = min(low, addr);
+		high = max(high, addr);
+	}
+
+	sz = high - low + sizeof(u32);
+
+	ret = pci_epf_assign_bar_space(epf, sz, barno, epc_features, 0, low);
+
+	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, db_bar);
+	if (ret) {
+		dev_err(&epf->dev, "Doorbell BAR set failed\n");
+		goto err_request_irq;
+	}
+
+	for (i = 0; i < ntb->db_count; i++) {
+		struct msi_msg *msg = &epf->db_msg[i].msg;
+		dma_addr_t addr;
+		size_t offset;
+
+		ret = pci_epf_align_inbound_addr(epf, db_bar->barno,
+				((u64)msg->address_hi << 32) | msg->address_lo,
+				&addr, &offset);
+
+		if (ret) {
+			ntb->msi_doorbell = false;
+			goto err_request_irq;
+		}
+
+		ntb->reg->db_data[i] = msg->data;
+		ntb->reg->db_offset[i] = offset;
+	}
+
+	ntb->reg->db_entry_size = 0;
+
+	ntb->msi_doorbell = true;
+
+	return 0;
+
+err_request_irq:
+	for (i--; i >= 0; i--)
+		free_irq(epf->db_msg[i].virq, ntb);
+
+	pci_epf_free_doorbell(ntb->epf);
+	return ret;
+}
+
 /**
  * epf_ntb_db_bar_init() - Configure Doorbell window BARs
  * @ntb: NTB device that facilitates communication between HOST and VHOST
@@ -520,22 +620,27 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 					    ntb->epf->func_no,
 					    ntb->epf->vfunc_no);
 	barno = ntb->epf_ntb_bar[BAR_DB];
-
-	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, epc_features, 0);
-	if (!mw_addr) {
-		dev_err(dev, "Failed to allocate OB address\n");
-		return -ENOMEM;
-	}
-
-	ntb->epf_db = mw_addr;
-
 	epf_bar = &ntb->epf->bar[barno];
 
-	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, epf_bar);
+	ret = epf_ntb_db_bar_init_msi_doorbell(ntb, epf_bar, epc_features, barno);
 	if (ret) {
-		dev_err(dev, "Doorbell BAR set failed\n");
+		/* fall back to polling mode */
+		mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, epc_features, 0);
+		if (!mw_addr) {
+			dev_err(dev, "Failed to allocate OB address\n");
+			return -ENOMEM;
+		}
+
+		ntb->epf_db = mw_addr;
+
+		ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
+				      ntb->epf->vfunc_no, epf_bar);
+		if (ret) {
+			dev_err(dev, "Doorbell BAR set failed\n");
 			goto err_alloc_peer_mem;
+		}
 	}
+
 	return ret;
 
 err_alloc_peer_mem:
@@ -554,6 +659,16 @@ static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 {
 	enum pci_barno barno;
 
+	if (ntb->msi_doorbell) {
+		int i;
+
+		for (i = 0; i < ntb->db_count; i++)
+			free_irq(ntb->epf->db_msg[i].virq, ntb);
+	}
+
+	if (ntb->epf->db_msg)
+		pci_epf_free_doorbell(ntb->epf);
+
 	barno = ntb->epf_ntb_bar[BAR_DB];
 	pci_epf_free_space(ntb->epf, ntb->epf_db, barno, 0);
 	pci_epc_clear_bar(ntb->epf->epc,
@@ -1268,7 +1383,7 @@ static u64 vntb_epf_db_read(struct ntb_dev *ndev)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 
-	return ntb->db;
+	return atomic64_read(&ntb->db);
 }
 
 static int vntb_epf_mw_get_align(struct ntb_dev *ndev, int pidx, int idx,
@@ -1308,7 +1423,7 @@ static int vntb_epf_db_clear(struct ntb_dev *ndev, u64 db_bits)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 
-	ntb->db &= ~db_bits;
+	atomic64_and(~db_bits, &ntb->db);
 	return 0;
 }
 

-- 
2.34.1

Re: [PATCH v3 1/3] PCI: endpoint: Add helper function pci_epf_get_bar_required_size()

[Niklas Cassel]

On Thu, Sep 25, 2025 at 01:01:47PM -0400, Frank Li wrote:
> Introduce pci_epf_get_bar_required_size() to retrieve the required BAR
> size and memory size. Prepare for adding support to set an MMIO address to
> a specific BAR.
> 
> Use two variables 'aligned_bar_size' and 'aligned_mem_size' to avoid
> confuse.

s/confuse/confusion/


> 
> No functional changes.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change in v3
> - change return value to int.
> - use two pointers return bar size aligned and memory start address aligned
> - update comments about why need memory align size. Actually iATU require
> start address match aligned requirement. Since kernel return align to
> size's address.
> - use two varible aligned_bar_size and aligned_mem_size to avoid confuse
> use 'size'.
> 
> change in v2
> - new patch
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++--------------
>  1 file changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index d54e18872aefc07c655c94c104a347328ff7a432..2cd0257831f9885a4381c087ed8f3326f5960966 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -208,6 +208,49 @@ void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf)
>  }
>  EXPORT_SYMBOL_GPL(pci_epf_remove_vepf);
>  
> +static int
> +pci_epf_get_bar_required_size(struct pci_epf *epf, size_t size,
> +			      size_t *aligned_bar_size,
> +			      size_t *aligned_mem_size,
> +			      enum pci_barno bar,
> +			      const struct pci_epc_features *epc_features,
> +			      enum pci_epc_interface_type type)
> +{
> +	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
> +	size_t align = epc_features->align;
> +
> +	if (size < 128)
> +		size = 128;
> +
> +	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
> +	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
> +		size = SZ_1M;
> +
> +	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
> +		if (size > bar_fixed_size) {
> +			dev_err(&epf->dev,
> +				"requested BAR size is larger than fixed size\n");
> +			return -ENOMEM;
> +		}
> +		size = bar_fixed_size;
> +	} else {
> +		/* BAR size must be power of two */
> +		size = roundup_pow_of_two(size);
> +	}
> +
> +	*aligned_bar_size = size;

I think this name is wrong.
The BAR size has not been aligned to anything.
The BAR size has to be a power of two, but that is a requirement of the PCI
specification, so that in an inherent property of a BAR.

Perhaps just name it size or bar_size?


Kind regards,
Niklas

Re: [PATCH v3 1/3] PCI: endpoint: Add helper function pci_epf_get_bar_required_size()

[Frank Li]

On Fri, Sep 26, 2025 at 02:31:42PM +0200, Niklas Cassel wrote:
> On Thu, Sep 25, 2025 at 01:01:47PM -0400, Frank Li wrote:
> > Introduce pci_epf_get_bar_required_size() to retrieve the required BAR
> > size and memory size. Prepare for adding support to set an MMIO address to
> > a specific BAR.
> >
> > Use two variables 'aligned_bar_size' and 'aligned_mem_size' to avoid
> > confuse.
>
> s/confuse/confusion/
>
>
> >
> > No functional changes.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change in v3
> > - change return value to int.
> > - use two pointers return bar size aligned and memory start address aligned
> > - update comments about why need memory align size. Actually iATU require
> > start address match aligned requirement. Since kernel return align to
> > size's address.
> > - use two varible aligned_bar_size and aligned_mem_size to avoid confuse
> > use 'size'.
> >
> > change in v2
> > - new patch
> > ---
> >  drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++--------------
> >  1 file changed, 53 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index d54e18872aefc07c655c94c104a347328ff7a432..2cd0257831f9885a4381c087ed8f3326f5960966 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -208,6 +208,49 @@ void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epf_remove_vepf);
> >
> > +static int
> > +pci_epf_get_bar_required_size(struct pci_epf *epf, size_t size,
> > +			      size_t *aligned_bar_size,
> > +			      size_t *aligned_mem_size,
> > +			      enum pci_barno bar,
> > +			      const struct pci_epc_features *epc_features,
> > +			      enum pci_epc_interface_type type)
> > +{
> > +	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
> > +	size_t align = epc_features->align;
> > +
> > +	if (size < 128)
> > +		size = 128;
> > +
> > +	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
> > +	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
> > +		size = SZ_1M;
> > +
> > +	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
> > +		if (size > bar_fixed_size) {
> > +			dev_err(&epf->dev,
> > +				"requested BAR size is larger than fixed size\n");
> > +			return -ENOMEM;
> > +		}
> > +		size = bar_fixed_size;
> > +	} else {
> > +		/* BAR size must be power of two */
> > +		size = roundup_pow_of_two(size);
> > +	}
> > +
> > +	*aligned_bar_size = size;
>
> I think this name is wrong.
> The BAR size has not been aligned to anything.
> The BAR size has to be a power of two, but that is a requirement of the PCI
> specification, so that in an inherent property of a BAR.
>
> Perhaps just name it size or bar_size?

there already have 'size' for input.  It should match epc required's size.

how about 'epc_bar_size'?

Frank

>
>
> Kind regards,
> Niklas

Re: [PATCH v3 1/3] PCI: endpoint: Add helper function pci_epf_get_bar_required_size()

[Niklas Cassel]

On Fri, Sep 26, 2025 at 10:56:46AM -0400, Frank Li wrote:
> On Fri, Sep 26, 2025 at 02:31:42PM +0200, Niklas Cassel wrote:
> > On Thu, Sep 25, 2025 at 01:01:47PM -0400, Frank Li wrote:
> > > Introduce pci_epf_get_bar_required_size() to retrieve the required BAR
> > > size and memory size. Prepare for adding support to set an MMIO address to
> > > a specific BAR.
> > >
> > > Use two variables 'aligned_bar_size' and 'aligned_mem_size' to avoid
> > > confuse.
> >
> > s/confuse/confusion/
> >
> >
> > >
> > > No functional changes.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > change in v3
> > > - change return value to int.
> > > - use two pointers return bar size aligned and memory start address aligned
> > > - update comments about why need memory align size. Actually iATU require
> > > start address match aligned requirement. Since kernel return align to
> > > size's address.
> > > - use two varible aligned_bar_size and aligned_mem_size to avoid confuse
> > > use 'size'.
> > >
> > > change in v2
> > > - new patch
> > > ---
> > >  drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++--------------
> > >  1 file changed, 53 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > index d54e18872aefc07c655c94c104a347328ff7a432..2cd0257831f9885a4381c087ed8f3326f5960966 100644
> > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > @@ -208,6 +208,49 @@ void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf)
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_epf_remove_vepf);
> > >
> > > +static int
> > > +pci_epf_get_bar_required_size(struct pci_epf *epf, size_t size,
> > > +			      size_t *aligned_bar_size,
> > > +			      size_t *aligned_mem_size,
> > > +			      enum pci_barno bar,
> > > +			      const struct pci_epc_features *epc_features,
> > > +			      enum pci_epc_interface_type type)
> > > +{
> > > +	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
> > > +	size_t align = epc_features->align;
> > > +
> > > +	if (size < 128)
> > > +		size = 128;
> > > +
> > > +	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
> > > +	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
> > > +		size = SZ_1M;
> > > +
> > > +	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
> > > +		if (size > bar_fixed_size) {
> > > +			dev_err(&epf->dev,
> > > +				"requested BAR size is larger than fixed size\n");
> > > +			return -ENOMEM;
> > > +		}
> > > +		size = bar_fixed_size;
> > > +	} else {
> > > +		/* BAR size must be power of two */
> > > +		size = roundup_pow_of_two(size);
> > > +	}
> > > +
> > > +	*aligned_bar_size = size;
> >
> > I think this name is wrong.
> > The BAR size has not been aligned to anything.
> > The BAR size has to be a power of two, but that is a requirement of the PCI
> > specification, so that in an inherent property of a BAR.
> >
> > Perhaps just name it size or bar_size?
> 
> there already have 'size' for input.  It should match epc required's size.

Why do you need both "size_t size" and "size_t *bar_size"?

Isn't it enough with "size_t *bar_size" ?

The user can supply a value, and the function could update that value.


Kind regards,
Niklas

Re: [PATCH v3 1/3] PCI: endpoint: Add helper function pci_epf_get_bar_required_size()

[Frank Li]

On Fri, Sep 26, 2025 at 05:16:57PM +0200, Niklas Cassel wrote:
> On Fri, Sep 26, 2025 at 10:56:46AM -0400, Frank Li wrote:
> > On Fri, Sep 26, 2025 at 02:31:42PM +0200, Niklas Cassel wrote:
> > > On Thu, Sep 25, 2025 at 01:01:47PM -0400, Frank Li wrote:
> > > > Introduce pci_epf_get_bar_required_size() to retrieve the required BAR
> > > > size and memory size. Prepare for adding support to set an MMIO address to
> > > > a specific BAR.
> > > >
> > > > Use two variables 'aligned_bar_size' and 'aligned_mem_size' to avoid
> > > > confuse.
> > >
> > > s/confuse/confusion/
> > >
> > >
> > > >
> > > > No functional changes.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > change in v3
> > > > - change return value to int.
> > > > - use two pointers return bar size aligned and memory start address aligned
> > > > - update comments about why need memory align size. Actually iATU require
> > > > start address match aligned requirement. Since kernel return align to
> > > > size's address.
> > > > - use two varible aligned_bar_size and aligned_mem_size to avoid confuse
> > > > use 'size'.
> > > >
> > > > change in v2
> > > > - new patch
> > > > ---
> > > >  drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++--------------
> > > >  1 file changed, 53 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > > index d54e18872aefc07c655c94c104a347328ff7a432..2cd0257831f9885a4381c087ed8f3326f5960966 100644
> > > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > > @@ -208,6 +208,49 @@ void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pci_epf_remove_vepf);
> > > >
> > > > +static int
> > > > +pci_epf_get_bar_required_size(struct pci_epf *epf, size_t size,
> > > > +			      size_t *aligned_bar_size,
> > > > +			      size_t *aligned_mem_size,
> > > > +			      enum pci_barno bar,
> > > > +			      const struct pci_epc_features *epc_features,
> > > > +			      enum pci_epc_interface_type type)
> > > > +{
> > > > +	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
> > > > +	size_t align = epc_features->align;
> > > > +
> > > > +	if (size < 128)
> > > > +		size = 128;
> > > > +
> > > > +	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
> > > > +	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
> > > > +		size = SZ_1M;
> > > > +
> > > > +	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
> > > > +		if (size > bar_fixed_size) {
> > > > +			dev_err(&epf->dev,
> > > > +				"requested BAR size is larger than fixed size\n");
> > > > +			return -ENOMEM;
> > > > +		}
> > > > +		size = bar_fixed_size;
> > > > +	} else {
> > > > +		/* BAR size must be power of two */
> > > > +		size = roundup_pow_of_two(size);
> > > > +	}
> > > > +
> > > > +	*aligned_bar_size = size;
> > >
> > > I think this name is wrong.
> > > The BAR size has not been aligned to anything.
> > > The BAR size has to be a power of two, but that is a requirement of the PCI
> > > specification, so that in an inherent property of a BAR.
> > >
> > > Perhaps just name it size or bar_size?
> >
> > there already have 'size' for input.  It should match epc required's size.
>
> Why do you need both "size_t size" and "size_t *bar_size"?
>
> Isn't it enough with "size_t *bar_size" ?
>
> The user can supply a value, and the function could update that value.

If not 'aligned_mem_size' in list, it looks fine. But after add
'aligned_mem_size', I think use difference varible for two outputs will be
clear and consistent and easy to understand.

Frank
>
>
> Kind regards,
> Niklas

Re: [PATCH v3 1/3] PCI: endpoint: Add helper function pci_epf_get_bar_required_size()

[Niklas Cassel]

On 26 September 2025 19:10:16 CEST, Frank Li <Frank.li@nxp.com> wrote:
>On Fri, Sep 26, 2025 at 05:16:57PM +0200, Niklas Cassel wrote:
>> On Fri, Sep 26, 2025 at 10:56:46AM -0400, Frank Li wrote:
>> > On Fri, Sep 26, 2025 at 02:31:42PM +0200, Niklas Cassel wrote:
>> > > On Thu, Sep 25, 2025 at 01:01:47PM -0400, Frank Li wrote:
>> > > > Introduce pci_epf_get_bar_required_size() to retrieve the required BAR
>> > > > size and memory size. Prepare for adding support to set an MMIO address to
>> > > > a specific BAR.
>> > > >
>> > > > Use two variables 'aligned_bar_size' and 'aligned_mem_size' to avoid
>> > > > confuse.
>> > >
>> > > s/confuse/confusion/
>> > >
>> > >
>> > > >
>> > > > No functional changes.
>> > > >
>> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
>> > > > ---
>> > > > change in v3
>> > > > - change return value to int.
>> > > > - use two pointers return bar size aligned and memory start address aligned
>> > > > - update comments about why need memory align size. Actually iATU require
>> > > > start address match aligned requirement. Since kernel return align to
>> > > > size's address.
>> > > > - use two varible aligned_bar_size and aligned_mem_size to avoid confuse
>> > > > use 'size'.
>> > > >
>> > > > change in v2
>> > > > - new patch
>> > > > ---
>> > > >  drivers/pci/endpoint/pci-epf-core.c | 84 +++++++++++++++++++++++--------------
>> > > >  1 file changed, 53 insertions(+), 31 deletions(-)
>> > > >
>> > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
>> > > > index d54e18872aefc07c655c94c104a347328ff7a432..2cd0257831f9885a4381c087ed8f3326f5960966 100644
>> > > > --- a/drivers/pci/endpoint/pci-epf-core.c
>> > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
>> > > > @@ -208,6 +208,49 @@ void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf)
>> > > >  }
>> > > >  EXPORT_SYMBOL_GPL(pci_epf_remove_vepf);
>> > > >
>> > > > +static int
>> > > > +pci_epf_get_bar_required_size(struct pci_epf *epf, size_t size,
>> > > > +			      size_t *aligned_bar_size,
>> > > > +			      size_t *aligned_mem_size,
>> > > > +			      enum pci_barno bar,
>> > > > +			      const struct pci_epc_features *epc_features,
>> > > > +			      enum pci_epc_interface_type type)
>> > > > +{
>> > > > +	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
>> > > > +	size_t align = epc_features->align;
>> > > > +
>> > > > +	if (size < 128)
>> > > > +		size = 128;
>> > > > +
>> > > > +	/* According to PCIe base spec, min size for a resizable BAR is 1 MB. */
>> > > > +	if (epc_features->bar[bar].type == BAR_RESIZABLE && size < SZ_1M)
>> > > > +		size = SZ_1M;
>> > > > +
>> > > > +	if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) {
>> > > > +		if (size > bar_fixed_size) {
>> > > > +			dev_err(&epf->dev,
>> > > > +				"requested BAR size is larger than fixed size\n");
>> > > > +			return -ENOMEM;
>> > > > +		}
>> > > > +		size = bar_fixed_size;
>> > > > +	} else {
>> > > > +		/* BAR size must be power of two */
>> > > > +		size = roundup_pow_of_two(size);
>> > > > +	}
>> > > > +
>> > > > +	*aligned_bar_size = size;
>> > >
>> > > I think this name is wrong.
>> > > The BAR size has not been aligned to anything.
>> > > The BAR size has to be a power of two, but that is a requirement of the PCI
>> > > specification, so that in an inherent property of a BAR.
>> > >
>> > > Perhaps just name it size or bar_size?
>> >
>> > there already have 'size' for input.  It should match epc required's size.
>>
>> Why do you need both "size_t size" and "size_t *bar_size"?
>>
>> Isn't it enough with "size_t *bar_size" ?
>>
>> The user can supply a value, and the function could update that value.
>
>If not 'aligned_mem_size' in list, it looks fine. But after add
>'aligned_mem_size', I think use difference varible for two outputs will be
>clear and consistent and easy to understand.


What am trying to say is:
Why not make "size_t *bar_size" both an input and an output?


Kind regards,
Niklas