[PATCH v8 0/6] PCI: EP: Add RC-to-EP doorbell with platform MSI controller

[PATCH v8 0/6] PCI: EP: Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

┌────────────┐   ┌───────────────────────────────────┐   ┌────────────────┐
│            │   │                                   │   │                │
│            │   │ PCI Endpoint                      │   │ PCI Host       │
│            │   │                                   │   │                │
│            │◄──┤ 1.platform_msi_domain_alloc_irqs()│   │                │
│            │   │                                   │   │                │
│ MSI        ├──►│ 2.write_msi_msg()                 ├──►├─BAR<n>         │
│ Controller │   │   update doorbell register address│   │                │
│            │   │   for BAR                         │   │                │
│            │   │                                   │   │ 3. Write BAR<n>│
│            │◄──┼───────────────────────────────────┼───┤                │
│            │   │                                   │   │                │
│            ├──►│ 4.Irq Handle                      │   │                │
│            │   │                                   │   │                │
│            │   │                                   │   │                │
└────────────┘   └───────────────────────────────────┘   └────────────────┘

This patches based on old https://lore.kernel.org/imx/20221124055036.1630573-1-Frank.Li@nxp.com/

Original patch only target to vntb driver. But actually it is common
method.

This patches add new API to pci-epf-core, so any EP driver can use it.

Previous v2 discussion here.
https://lore.kernel.org/imx/20230911220920.1817033-1-Frank.Li@nxp.com/

Changes in v8:
- update helper function name to pci_epf_align_inbound_addr()
- Link to v7: https://lore.kernel.org/r/20241114-ep-msi-v7-0-d4ac7aafbd2c@nxp.com

Changes in v7:
- Add helper function pci_epf_align_addr();
- Link to v6: https://lore.kernel.org/r/20241112-ep-msi-v6-0-45f9722e3c2a@nxp.com

Changes in v6:
- change doorbell_addr to doorbell_offset
- use round_down()
- add Niklas's test by tag
- rebase to pci/endpoint
- Link to v5: https://lore.kernel.org/r/20241108-ep-msi-v5-0-a14951c0d007@nxp.com

Changes in v5:
- Move request_irq to epf test function driver for more flexiable user case
- Add fixed size bar handler
- Some minor improvememtn to see each patches's changelog.
- Link to v4: https://lore.kernel.org/r/20241031-ep-msi-v4-0-717da2d99b28@nxp.com

Changes in v4:
- Remove patch genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs()
- Use new method to avoid compatible problem.
  Add new command DOORBELL_ENABLE and DOORBELL_DISABLE.
  pcitest -B send DOORBELL_ENABLE first, EP test function driver try to
remap one of BAR_N (except test register bar) to ITS MSI MMIO space. Old
driver don't support new command, so failure return, not side effect.
  After test, DOORBELL_DISABLE command send out to recover original map, so
pcitest bar test can pass as normal.
- Other detail change see each patches's change log
- Link to v3: https://lore.kernel.org/r/20241015-ep-msi-v3-0-cedc89a16c1a@nxp.com

Change from v2 to v3
- Fixed manivannan's comments
- Move common part to pci-ep-msi.c and pci-ep-msi.h
- rebase to 6.12-rc1
- use RevID to distingiush old version

mkdir /sys/kernel/config/pci_ep/functions/pci_epf_test/func1
echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/msi_interrupts
echo 0x080c > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/deviceid
echo 0x1957 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/vendorid
echo 1 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/revid
^^^^^^ to enable platform msi support.
ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/func1 /sys/kernel/config/pci_ep/controllers/4c380000.pcie-ep

- use new device ID, which identify support doorbell to avoid broken
compatility.

    Enable doorbell support only for PCI_DEVICE_ID_IMX8_DB, while other devices
    keep the same behavior as before.

           EP side             RC with old driver      RC with new driver
    PCI_DEVICE_ID_IMX8_DB          no probe              doorbell enabled
    Other device ID             doorbell disabled*       doorbell disabled*

    * Behavior remains unchanged.

Change from v1 to v2
- Add missed patch for endpont/pci-epf-test.c
- Move alloc and free to epc driver from epf.
- Provide general help function for EPC driver to alloc platform msi irq.
- Fixed manivannan's comments.

To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Krzysztof Wilczyński <kw@linux.com>
To: Kishon Vijay Abraham I <kishon@kernel.org>
To: Bjorn Helgaas <bhelgaas@google.com>
To: Arnd Bergmann <arnd@arndb.de>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: Niklas Cassel <cassel@kernel.org>
Cc: cassel@kernel.org
Cc: dlemoal@kernel.org
Cc: maz@kernel.org
Cc: tglx@linutronix.de
Cc: jdmason@kudzu.us

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (6):
      PCI: endpoint: Add pci_epc_get_fn() API for customizable filtering
      PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller
      PCI: endpoint: Add pci_epf_align_addr() helper for address alignment
      PCI: endpoint: pci-epf-test: Add doorbell test support
      misc: pci_endpoint_test: Add doorbell test case
      tools: PCI: Add 'B' option for test doorbell

 drivers/misc/pci_endpoint_test.c              |  71 ++++++++++++++++
 drivers/pci/endpoint/Makefile                 |   2 +-
 drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
 drivers/pci/endpoint/pci-ep-msi.c             |  99 ++++++++++++++++++++++
 drivers/pci/endpoint/pci-epc-core.c           |  23 ++++-
 drivers/pci/endpoint/pci-epf-core.c           |  45 ++++++++++
 include/linux/pci-ep-msi.h                    |  15 ++++
 include/linux/pci-epc.h                       |   2 +
 include/linux/pci-epf.h                       |  30 +++++++
 include/uapi/linux/pcitest.h                  |   1 +
 tools/pci/pcitest.c                           |  16 +++-
 11 files changed, 417 insertions(+), 4 deletions(-)
---
base-commit: f5373677e13177cfc7875f44a864f9a1db751df9
change-id: 20241010-ep-msi-8b4cab33b1be

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

[PATCH v8 1/6] PCI: endpoint: Add pci_epc_get_fn() API for customizable filtering

[Frank Li]

Introduce the pci_epc_get_fn() API, allowing a custom filter callback
function to be passed. Reimplement pci_epc_get() using pci_epc_get_fn()
with a match name callback. Prepare the codebase for adding RC-to-EP
doorbell support.

Add DEFINE_FREE(pci_epc_put) to implement scope based auto cleanup.

Tested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v3 to v8
- none
---
 drivers/pci/endpoint/pci-epc-core.c | 23 +++++++++++++++++++++--
 include/linux/pci-epc.h             |  2 ++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index bed7c7d1fe3c3..f5538c007678e 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -48,6 +48,11 @@ void pci_epc_put(struct pci_epc *epc)
 }
 EXPORT_SYMBOL_GPL(pci_epc_put);
 
+static bool pci_epc_match_name(struct device *dev, void *name)
+{
+	return strcmp(dev_name(dev), name) == 0;
+}
+
 /**
  * pci_epc_get() - get the PCI endpoint controller
  * @epc_name: device name of the endpoint controller
@@ -56,6 +61,20 @@ EXPORT_SYMBOL_GPL(pci_epc_put);
  * endpoint controller
  */
 struct pci_epc *pci_epc_get(const char *epc_name)
+{
+	return pci_epc_get_fn(pci_epc_match_name, (void *)epc_name);
+}
+EXPORT_SYMBOL_GPL(pci_epc_get);
+
+/**
+ * pci_epc_get_fn() - get the PCI endpoint controller
+ * @fn: callback match filter function, return true when matched
+ * @param: parameter for callback function fn
+ *
+ * Invoke to get struct pci_epc * corresponding to the device name of the
+ * endpoint controller
+ */
+struct pci_epc *pci_epc_get_fn(bool (*fn)(struct device *dev, void *param), void *param)
 {
 	int ret = -EINVAL;
 	struct pci_epc *epc;
@@ -64,7 +83,7 @@ struct pci_epc *pci_epc_get(const char *epc_name)
 
 	class_dev_iter_init(&iter, &pci_epc_class, NULL, NULL);
 	while ((dev = class_dev_iter_next(&iter))) {
-		if (strcmp(epc_name, dev_name(dev)))
+		if (!fn(dev, param))
 			continue;
 
 		epc = to_pci_epc(dev);
@@ -82,7 +101,7 @@ struct pci_epc *pci_epc_get(const char *epc_name)
 	class_dev_iter_exit(&iter);
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(pci_epc_get);
+EXPORT_SYMBOL_GPL(pci_epc_get_fn);
 
 /**
  * pci_epc_get_first_free_bar() - helper to get first unreserved BAR
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index de8cc3658220b..d3d3b8c914614 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -300,7 +300,9 @@ pci_epc_get_first_free_bar(const struct pci_epc_features *epc_features);
 enum pci_barno pci_epc_get_next_free_bar(const struct pci_epc_features
 					 *epc_features, enum pci_barno bar);
 struct pci_epc *pci_epc_get(const char *epc_name);
+struct pci_epc *pci_epc_get_fn(bool (*fn)(struct device *dev, void *param), void *param);
 void pci_epc_put(struct pci_epc *epc);
+DEFINE_FREE(pci_epc_put, void *, if (!(_T)) pci_epc_put(_T))
 
 int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
 		     size_t size, size_t page_size);

-- 
2.34.1

[PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

Doorbell feature is implemented by mapping the EP's MSI interrupt
controller message address to a dedicated BAR in the EPC core. It is the
responsibility of the EPF driver to pass the actual message data to be
written by the host to the doorbell BAR region through its own logic.

Tested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v5 to v8
-none

Change from v4 to v5
- Remove request_irq() in pci_epc_alloc_doorbell() and leave to EP function
driver, so ep function driver can register differece call back function for
difference doorbell events and set irq affinity to differece CPU core.
- Improve error message when MSI allocate failure.

Change from v3 to v4
- msi change to use msi_get_virq() avoid use msi_for_each_desc().
- add new struct for pci_epf_doorbell_msg to msi msg,virq and irq name.
- move mutex lock to epc function
- initialize variable at declear place.
- passdown epf to epc*() function to simplify code.
---
 drivers/pci/endpoint/Makefile     |  2 +-
 drivers/pci/endpoint/pci-ep-msi.c | 99 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci-ep-msi.h        | 15 ++++++
 include/linux/pci-epf.h           | 16 +++++++
 4 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
index 95b2fe47e3b06..a1ccce440c2c5 100644
--- a/drivers/pci/endpoint/Makefile
+++ b/drivers/pci/endpoint/Makefile
@@ -5,4 +5,4 @@
 
 obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
 obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
-					   pci-epc-mem.o functions/
+					   pci-epc-mem.o pci-ep-msi.o functions/
diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
new file mode 100644
index 0000000000000..7868a529dce37
--- /dev/null
+++ b/drivers/pci/endpoint/pci-ep-msi.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Endpoint *Controller* (EPC) MSI library
+ *
+ * Copyright (C) 2024 NXP
+ * Author: Frank Li <Frank.Li@nxp.com>
+ */
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/pci-epc.h>
+#include <linux/pci-epf.h>
+#include <linux/pci-ep-cfs.h>
+#include <linux/pci-ep-msi.h>
+
+static bool pci_epc_match_parent(struct device *dev, void *param)
+{
+	return dev->parent == param;
+}
+
+static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	struct pci_epc *epc __free(pci_epc_put) = NULL;
+	struct pci_epf *epf;
+
+	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
+	if (!epc)
+		return;
+
+	/* Only support one EPF for doorbell */
+	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
+
+	if (epf && epf->db_msg && desc->msi_index < epf->num_db)
+		memcpy(&epf->db_msg[desc->msi_index].msg, msg, sizeof(*msg));
+}
+
+static void pci_epc_free_doorbell(struct pci_epc *epc, struct pci_epf *epf)
+{
+	guard(mutex)(&epc->lock);
+
+	platform_device_msi_free_irqs_all(epc->dev.parent);
+}
+
+static int pci_epc_alloc_doorbell(struct pci_epc *epc, struct pci_epf *epf)
+{
+	struct device *dev = epc->dev.parent;
+	u16 num_db = epf->num_db;
+	int i = 0;
+	int ret;
+
+	guard(mutex)(&epc->lock);
+
+	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
+	if (ret) {
+		dev_err(dev, "Failed to allocate MSI, may miss 'msi-parent' at your DTS\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < num_db; i++)
+		epf->db_msg[i].virq = msi_get_virq(dev, i);
+
+	return 0;
+};
+
+int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
+{
+	struct pci_epc *epc = epf->epc;
+	void *msg;
+	int ret;
+
+	msg = kcalloc(num_db, sizeof(struct pci_epf_doorbell_msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	epf->num_db = num_db;
+	epf->db_msg = msg;
+
+	ret = pci_epc_alloc_doorbell(epc, epf);
+	if (ret)
+		kfree(msg);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
+
+void pci_epf_free_doorbell(struct pci_epf *epf)
+{
+	struct pci_epc *epc = epf->epc;
+
+	pci_epc_free_doorbell(epc, epf);
+
+	kfree(epf->db_msg);
+	epf->db_msg = NULL;
+	epf->num_db = 0;
+}
+EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
diff --git a/include/linux/pci-ep-msi.h b/include/linux/pci-ep-msi.h
new file mode 100644
index 0000000000000..f0cfecf491199
--- /dev/null
+++ b/include/linux/pci-ep-msi.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PCI Endpoint *Function* side MSI header file
+ *
+ * Copyright (C) 2024 NXP
+ * Author: Frank Li <Frank.Li@nxp.com>
+ */
+
+#ifndef __PCI_EP_MSI__
+#define __PCI_EP_MSI__
+
+int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
+void pci_epf_free_doorbell(struct pci_epf *epf);
+
+#endif /* __PCI_EP_MSI__ */
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 18a3aeb62ae4e..5374e6515ffa0 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -12,6 +12,7 @@
 #include <linux/configfs.h>
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
+#include <linux/msi.h>
 #include <linux/pci.h>
 
 struct pci_epf;
@@ -125,6 +126,17 @@ struct pci_epf_bar {
 	int		flags;
 };
 
+/**
+ * struct pci_epf_doorbell_msg - represents doorbell message
+ * @msi_msg: MSI message
+ * @virq: irq number of this doorbell MSI message
+ * @name: irq name for doorbell interrupt
+ */
+struct pci_epf_doorbell_msg {
+	struct msi_msg msg;
+	int virq;
+};
+
 /**
  * struct pci_epf - represents the PCI EPF device
  * @dev: the PCI EPF device
@@ -152,6 +164,8 @@ struct pci_epf_bar {
  * @vfunction_num_map: bitmap to manage virtual function number
  * @pci_vepf: list of virtual endpoint functions associated with this function
  * @event_ops: Callbacks for capturing the EPC events
+ * @db_msg: data for MSI from RC side
+ * @num_db: number of doorbells
  */
 struct pci_epf {
 	struct device		dev;
@@ -182,6 +196,8 @@ struct pci_epf {
 	unsigned long		vfunction_num_map;
 	struct list_head	pci_vepf;
 	const struct pci_epc_event_ops *event_ops;
+	struct pci_epf_doorbell_msg *db_msg;
+	u16 num_db;
 };
 
 /**

-- 
2.34.1

[PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper for address alignment

[Frank Li]

Introduce the helper function pci_epf_align_addr() to adjust addresses
according to PCI BAR alignment requirements, converting addresses into base
and offset values.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v7 to v8
- change name to pci_epf_align_inbound_addr()
- update comment said only need for memory, which not allocated by
pci_epf_alloc_space().

change from v6 to v7
- new patch
---
 drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
 include/linux/pci-epf.h             | 14 ++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 8fa2797d4169a..4dfc218ebe20b 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
 }
 EXPORT_SYMBOL_GPL(pci_epf_create);
 
+/**
+ * pci_epf_align_inbound_addr() - Get base address and offset that match bar's
+ *			  alignment requirement
+ * @epf: the EPF device
+ * @addr: the address of the memory
+ * @bar: the BAR number corresponding to map addr
+ * @base: return base address, which match BAR's alignment requirement, nothing
+ *	  return if NULL
+ * @off: return offset, nothing return if NULL
+ *
+ * Helper function to convert input 'addr' to base and offset, which match
+ * BAR's alignment requirement.
+ *
+ * The pci_epf_alloc_space() function already accounts for alignment. This is
+ * primarily intended for use with other memory regions not allocated by
+ * pci_epf_alloc_space(), such as peripheral register spaces or the trigger
+ * address for a platform MSI controller.
+ */
+int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
+			       u64 addr, u64 *base, size_t *off)
+{
+	const struct pci_epc_features *epc_features;
+	u64 align;
+
+	epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
+	if (!epc_features) {
+		dev_err(&epf->dev, "epc_features not implemented\n");
+		return -EOPNOTSUPP;
+	}
+
+	align = epc_features->align;
+	align = align ? align : 128;
+	if (epc_features->bar[bar].type == BAR_FIXED)
+		align = max(epc_features->bar[bar].fixed_size, align);
+
+	if (base)
+		*base = round_down(addr, align);
+
+	if (off)
+		*off = addr & (align - 1);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_epf_align_inbound_addr);
+
 static void pci_epf_dev_release(struct device *dev)
 {
 	struct pci_epf *epf = to_pci_epf(dev);
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 5374e6515ffa0..eff73ccb5e702 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -238,6 +238,20 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 			  enum pci_epc_interface_type type);
 void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
 			enum pci_epc_interface_type type);
+
+int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
+			       u64 addr, u64 *base, size_t *off);
+static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
+						   u32 low, u32 high, u64 *base, size_t *off)
+{
+	u64 addr = high;
+
+	addr <<= 32;
+	addr |= low;
+
+	return pci_epf_align_inbound_addr(epf, bar, addr, base, off);
+}
+
 int pci_epf_bind(struct pci_epf *epf);
 void pci_epf_unbind(struct pci_epf *epf);
 int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);

-- 
2.34.1

[PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
doorbell address space.

Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
callback handler by writing doorbell_data to the mapped doorbell_bar's
address space.

Set doorbell_done in the doorbell callback to indicate completion.

To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
to map one bar's inbound address to MSI space. the command
COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.

	 	Host side new driver	Host side old driver

EP: new driver      S				F
EP: old driver      F				F

S: If EP side support MSI, 'pcitest -B' return success.
   If EP side doesn't support MSI, the same to 'F'.

F: 'pcitest -B' return failure, other case as usual.

Tested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v7 to v8
- rename to pci_epf_align_inbound_addr_lo_hi()

Change from v6 to v7
- use help function pci_epf_align_addr_lo_hi()

Change from v5 to v6
- rename doorbell_addr to doorbell_offset

Chagne from v4 to v5
- Add doorbell free at unbind function.
- Move msi irq handler to here to more complex user case, such as differece
doorbell can use difference handler function.
- Add Niklas's code to handle fixed bar's case. If need add your signed-off
tag or co-developer tag, please let me know.

change from v3 to v4
- remove revid requirement
- Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
- call pci_epc_set_bar() to map inbound address to MSI space only at
COMMAND_ENABLE_DOORBELL.
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index ef6677f34116e..410b2f4bb7ce7 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -11,12 +11,14 @@
 #include <linux/dmaengine.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/msi.h>
 #include <linux/slab.h>
 #include <linux/pci_ids.h>
 #include <linux/random.h>
 
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
+#include <linux/pci-ep-msi.h>
 #include <linux/pci_regs.h>
 
 #define IRQ_TYPE_INTX			0
@@ -29,6 +31,8 @@
 #define COMMAND_READ			BIT(3)
 #define COMMAND_WRITE			BIT(4)
 #define COMMAND_COPY			BIT(5)
+#define COMMAND_ENABLE_DOORBELL		BIT(6)
+#define COMMAND_DISABLE_DOORBELL	BIT(7)
 
 #define STATUS_READ_SUCCESS		BIT(0)
 #define STATUS_READ_FAIL		BIT(1)
@@ -39,6 +43,11 @@
 #define STATUS_IRQ_RAISED		BIT(6)
 #define STATUS_SRC_ADDR_INVALID		BIT(7)
 #define STATUS_DST_ADDR_INVALID		BIT(8)
+#define STATUS_DOORBELL_SUCCESS		BIT(9)
+#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
+#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
+#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
+#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
 
 #define FLAG_USE_DMA			BIT(0)
 
@@ -74,6 +83,9 @@ struct pci_epf_test_reg {
 	u32	irq_type;
 	u32	irq_number;
 	u32	flags;
+	u32	doorbell_bar;
+	u32	doorbell_offset;
+	u32	doorbell_data;
 } __packed;
 
 static struct pci_epf_header test_header = {
@@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
 	}
 }
 
+static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
+{
+	enum pci_barno bar = reg->doorbell_bar;
+	struct pci_epf *epf = epf_test->epf;
+	struct pci_epc *epc = epf->epc;
+	struct pci_epf_bar db_bar;
+	struct msi_msg *msg;
+	size_t offset;
+	int ret;
+
+	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
+		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
+		return;
+	}
+
+	msg = &epf->db_msg[0].msg;
+	ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
+					       &db_bar.phys_addr, &offset);
+
+	if (ret) {
+		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
+		return;
+	}
+
+	reg->doorbell_offset = offset;
+
+	db_bar.barno = bar;
+	db_bar.size = epf->bar[bar].size;
+	db_bar.flags = epf->bar[bar].flags;
+	db_bar.addr = NULL;
+
+	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar);
+	if (!ret)
+		reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS;
+	else
+		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
+}
+
+static void pci_epf_disable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
+{
+	enum pci_barno bar = reg->doorbell_bar;
+	struct pci_epf *epf = epf_test->epf;
+	struct pci_epc *epc = epf->epc;
+	int ret;
+
+	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
+		reg->status |= STATUS_DOORBELL_DISABLE_FAIL;
+		return;
+	}
+
+	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &epf->bar[bar]);
+	if (ret)
+		reg->status |= STATUS_DOORBELL_DISABLE_FAIL;
+	else
+		reg->status |= STATUS_DOORBELL_DISABLE_SUCCESS;
+}
+
 static void pci_epf_test_cmd_handler(struct work_struct *work)
 {
 	u32 command;
@@ -688,6 +757,14 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 		pci_epf_test_copy(epf_test, reg);
 		pci_epf_test_raise_irq(epf_test, reg);
 		break;
+	case COMMAND_ENABLE_DOORBELL:
+		pci_epf_enable_doorbell(epf_test, reg);
+		pci_epf_test_raise_irq(epf_test, reg);
+		break;
+	case COMMAND_DISABLE_DOORBELL:
+		pci_epf_disable_doorbell(epf_test, reg);
+		pci_epf_test_raise_irq(epf_test, reg);
+		break;
 	default:
 		dev_err(dev, "Invalid command 0x%x\n", command);
 		break;
@@ -822,6 +899,18 @@ static int pci_epf_test_link_down(struct pci_epf *epf)
 	return 0;
 }
 
+static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data)
+{
+	struct pci_epf_test *epf_test = data;
+	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
+	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+
+	reg->status |= STATUS_DOORBELL_SUCCESS;
+	pci_epf_test_raise_irq(epf_test, reg);
+
+	return IRQ_HANDLED;
+}
+
 static const struct pci_epc_event_ops pci_epf_test_event_ops = {
 	.epc_init = pci_epf_test_epc_init,
 	.epc_deinit = pci_epf_test_epc_deinit,
@@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	if (ret)
 		return ret;
 
+	ret = pci_epf_alloc_doorbell(epf, 1);
+	if (!ret) {
+		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+		struct msi_msg *msg = &epf->db_msg[0].msg;
+		enum pci_barno bar;
+
+		bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
+
+		ret = request_irq(epf->db_msg[0].virq, pci_epf_test_doorbell_handler, 0,
+				  "pci-test-doorbell", epf_test);
+		if (ret) {
+			dev_err(&epf->dev,
+				"Failed to request irq %d, doorbell feature is not supported\n",
+				epf->db_msg[0].virq);
+			return 0;
+		}
+
+		reg->doorbell_data = msg->data;
+		reg->doorbell_bar = bar;
+	}
+
 	return 0;
 }
 
 static void pci_epf_test_unbind(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar];
 	struct pci_epc *epc = epf->epc;
 
 	cancel_delayed_work_sync(&epf_test->cmd_handler);
@@ -934,6 +1045,12 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
 		pci_epf_test_clean_dma_chan(epf_test);
 		pci_epf_test_clear_bar(epf);
 	}
+
+	if (reg->doorbell_bar > 0) {
+		free_irq(epf->db_msg[0].virq, epf_test);
+		pci_epf_free_doorbell(epf);
+	}
+
 	pci_epf_test_free_space(epf);
 }
 

-- 
2.34.1

[PATCH v8 5/6] misc: pci_endpoint_test: Add doorbell test case

[Frank Li]

Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR,
and PCIE_ENDPOINT_TEST_DB_DATA.

Trigger the doorbell by writing data from PCI_ENDPOINT_TEST_DB_DATA to the
address provided by PCI_ENDPOINT_TEST_DB_OFFSET and wait for endpoint
feedback.

Add two command to COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL
to enable EP side's doorbell support and avoid compatible problem.

		Host side new driver	Host side old driver
EP: new driver		S			F
EP: old driver		F			F

S: If EP side support MSI, 'pcitest -B' return success.
   If EP side doesn't support MSI, the same to 'F'.

F: 'pcitest -B' return failure, other case as usual.

Tested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change form v6 to v8
- none

Change from v5 to v6
- %s/PCI_ENDPOINT_TEST_DB_ADDR/PCI_ENDPOINT_TEST_DB_OFFSET/g

Change from v4 to v5
- remove unused varible
- add irq_type at pci_endpoint_test_doorbell();

change from v3 to v4
- Add COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
- Remove new DID requirement.
---
 drivers/misc/pci_endpoint_test.c | 71 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/pcitest.h     |  1 +
 2 files changed, 72 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee2..dc766055aa594 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -42,6 +42,8 @@
 #define COMMAND_READ				BIT(3)
 #define COMMAND_WRITE				BIT(4)
 #define COMMAND_COPY				BIT(5)
+#define COMMAND_ENABLE_DOORBELL			BIT(6)
+#define COMMAND_DISABLE_DOORBELL		BIT(7)
 
 #define PCI_ENDPOINT_TEST_STATUS		0x8
 #define STATUS_READ_SUCCESS			BIT(0)
@@ -53,6 +55,11 @@
 #define STATUS_IRQ_RAISED			BIT(6)
 #define STATUS_SRC_ADDR_INVALID			BIT(7)
 #define STATUS_DST_ADDR_INVALID			BIT(8)
+#define STATUS_DOORBELL_SUCCESS			BIT(9)
+#define STATUS_DOORBELL_ENABLE_SUCCESS		BIT(10)
+#define STATUS_DOORBELL_ENABLE_FAIL		BIT(11)
+#define STATUS_DOORBELL_DISABLE_SUCCESS		BIT(12)
+#define STATUS_DOORBELL_DISABLE_FAIL		BIT(13)
 
 #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0x0c
 #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
@@ -67,6 +74,10 @@
 #define PCI_ENDPOINT_TEST_IRQ_NUMBER		0x28
 
 #define PCI_ENDPOINT_TEST_FLAGS			0x2c
+#define PCI_ENDPOINT_TEST_DB_BAR		0x30
+#define PCI_ENDPOINT_TEST_DB_OFFSET		0x34
+#define PCI_ENDPOINT_TEST_DB_DATA		0x38
+
 #define FLAG_USE_DMA				BIT(0)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
@@ -108,6 +119,7 @@ enum pci_barno {
 	BAR_3,
 	BAR_4,
 	BAR_5,
+	NO_BAR = -1,
 };
 
 struct pci_endpoint_test {
@@ -746,6 +758,62 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	return false;
 }
 
+static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
+{
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	int irq_type = test->irq_type;
+	enum pci_barno bar;
+	u32 data, status;
+	u32 addr;
+
+	if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) {
+		dev_err(dev, "Invalid IRQ type option\n");
+		return false;
+	}
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+				 COMMAND_ENABLE_DOORBELL);
+
+	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
+
+	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+	if (status & STATUS_DOORBELL_ENABLE_FAIL)
+		return false;
+
+	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
+	addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET);
+	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
+
+	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
+
+	writel(data, test->bar[bar] + addr);
+
+	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
+
+	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+				 COMMAND_DISABLE_DOORBELL);
+
+	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
+
+	status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+
+	if ((status & STATUS_DOORBELL_SUCCESS) &&
+	    (status & STATUS_DOORBELL_DISABLE_SUCCESS))
+		return true;
+
+	return false;
+}
+
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 				    unsigned long arg)
 {
@@ -793,6 +861,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	case PCITEST_CLEAR_IRQ:
 		ret = pci_endpoint_test_clear_irq(test);
 		break;
+	case PCITEST_DOORBELL:
+		ret = pci_endpoint_test_doorbell(test);
+		break;
 	}
 
 ret:
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index 94b46b043b536..06d9f548b510e 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -21,6 +21,7 @@
 #define PCITEST_SET_IRQTYPE	_IOW('P', 0x8, int)
 #define PCITEST_GET_IRQTYPE	_IO('P', 0x9)
 #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
+#define PCITEST_DOORBELL	_IO('P', 0x11)
 
 #define PCITEST_FLAGS_USE_DMA	0x00000001
 

-- 
2.34.1

[PATCH v8 6/6] tools: PCI: Add 'B' option for test doorbell

[Frank Li]

Add doorbell test support.

Tested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3 to v8
- none
---
 tools/pci/pcitest.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
index 470258009ddc2..bbe26ebbfd945 100644
--- a/tools/pci/pcitest.c
+++ b/tools/pci/pcitest.c
@@ -34,6 +34,7 @@ struct pci_test {
 	bool		copy;
 	unsigned long	size;
 	bool		use_dma;
+	bool		doorbell;
 };
 
 static int run_test(struct pci_test *test)
@@ -147,6 +148,15 @@ static int run_test(struct pci_test *test)
 			fprintf(stdout, "%s\n", result[ret]);
 	}
 
+	if (test->doorbell) {
+		ret = ioctl(fd, PCITEST_DOORBELL, 0);
+		fprintf(stdout, "Ringing doorbell on the EP\t\t");
+		if (ret < 0)
+			fprintf(stdout, "TEST FAILED\n");
+		else
+			fprintf(stdout, "%s\n", result[ret]);
+	}
+
 	fflush(stdout);
 	close(fd);
 	return (ret < 0) ? ret : 1 - ret; /* return 0 if test succeeded */
@@ -172,7 +182,7 @@ int main(int argc, char **argv)
 	/* set default endpoint device */
 	test->device = "/dev/pci-endpoint-test.0";
 
-	while ((c = getopt(argc, argv, "D:b:m:x:i:deIlhrwcs:")) != EOF)
+	while ((c = getopt(argc, argv, "D:b:m:x:i:BdeIlhrwcs:")) != EOF)
 	switch (c) {
 	case 'D':
 		test->device = optarg;
@@ -222,6 +232,9 @@ int main(int argc, char **argv)
 	case 'd':
 		test->use_dma = true;
 		continue;
+	case 'B':
+		test->doorbell = true;
+		continue;
 	case 'h':
 	default:
 usage:
@@ -241,6 +254,7 @@ int main(int argc, char **argv)
 			"\t-w			Write buffer test\n"
 			"\t-c			Copy buffer test\n"
 			"\t-s <size>		Size of buffer {default: 100KB}\n"
+			"\t-B			Doorbell test\n"
 			"\t-h			Print this help message\n",
 			argv[0]);
 		return -EINVAL;

-- 
2.34.1

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote:
> Doorbell feature is implemented by mapping the EP's MSI interrupt
> controller message address to a dedicated BAR in the EPC core. It is the
> responsibility of the EPF driver to pass the actual message data to be
> written by the host to the doorbell BAR region through its own logic.
> 
> Tested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change from v5 to v8
> -none
> 
> Change from v4 to v5
> - Remove request_irq() in pci_epc_alloc_doorbell() and leave to EP function
> driver, so ep function driver can register differece call back function for
> difference doorbell events and set irq affinity to differece CPU core.
> - Improve error message when MSI allocate failure.
> 
> Change from v3 to v4
> - msi change to use msi_get_virq() avoid use msi_for_each_desc().
> - add new struct for pci_epf_doorbell_msg to msi msg,virq and irq name.
> - move mutex lock to epc function
> - initialize variable at declear place.
> - passdown epf to epc*() function to simplify code.
> ---
>  drivers/pci/endpoint/Makefile     |  2 +-
>  drivers/pci/endpoint/pci-ep-msi.c | 99 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-ep-msi.h        | 15 ++++++
>  include/linux/pci-epf.h           | 16 +++++++
>  4 files changed, 131 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> index 95b2fe47e3b06..a1ccce440c2c5 100644
> --- a/drivers/pci/endpoint/Makefile
> +++ b/drivers/pci/endpoint/Makefile
> @@ -5,4 +5,4 @@
>  
>  obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
>  obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
> -					   pci-epc-mem.o functions/
> +					   pci-epc-mem.o pci-ep-msi.o functions/
> diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> new file mode 100644
> index 0000000000000..7868a529dce37
> --- /dev/null
> +++ b/drivers/pci/endpoint/pci-ep-msi.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI Endpoint *Controller* (EPC) MSI library
> + *
> + * Copyright (C) 2024 NXP
> + * Author: Frank Li <Frank.Li@nxp.com>
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>

Please sort alphabetically.

> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/pci-epc.h>
> +#include <linux/pci-epf.h>
> +#include <linux/pci-ep-cfs.h>
> +#include <linux/pci-ep-msi.h>
> +
> +static bool pci_epc_match_parent(struct device *dev, void *param)
> +{
> +	return dev->parent == param;
> +}
> +
> +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct pci_epc *epc __free(pci_epc_put) = NULL;
> +	struct pci_epf *epf;
> +
> +	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);

You were passing 'epc->dev.parent' to platform_device_msi_init_and_alloc_irqs().
So 'desc->dev' should be the EPC parent, right? If so, you can do:

	epc = pci_epc_get(dev_name(msi_desc_to_dev(desc)));

since we are reusing the parent dev name for EPC.

> +	if (!epc)
> +		return;
> +
> +	/* Only support one EPF for doorbell */
> +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);

Why don't you impose this restriction in pci_epf_alloc_doorbell() itself?

> +
> +	if (epf && epf->db_msg && desc->msi_index < epf->num_db)
> +		memcpy(&epf->db_msg[desc->msi_index].msg, msg, sizeof(*msg));
> +}
> +
> +static void pci_epc_free_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> +{
> +	guard(mutex)(&epc->lock);
> +
> +	platform_device_msi_free_irqs_all(epc->dev.parent);
> +}
> +
> +static int pci_epc_alloc_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> +{
> +	struct device *dev = epc->dev.parent;
> +	u16 num_db = epf->num_db;
> +	int i = 0;
> +	int ret;
> +
> +	guard(mutex)(&epc->lock);
> +
> +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate MSI, may miss 'msi-parent' at your DTS\n");

No need to mention 'msi-parent'. Just 'Failed to allocate MSI' is enough.

> +		return -ENOMEM;

-ENODEV?

- Mani

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

Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper for address alignment

[Manivannan Sadhasivam]

On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> Introduce the helper function pci_epf_align_addr() to adjust addresses

pci_epf_align_inbound_addr()?

> according to PCI BAR alignment requirements, converting addresses into base
> and offset values.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change from v7 to v8
> - change name to pci_epf_align_inbound_addr()
> - update comment said only need for memory, which not allocated by
> pci_epf_alloc_space().
> 
> change from v6 to v7
> - new patch
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
>  include/linux/pci-epf.h             | 14 ++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 8fa2797d4169a..4dfc218ebe20b 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(pci_epf_create);
>  
> +/**
> + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's

BAR's

> + *			  alignment requirement
> + * @epf: the EPF device
> + * @addr: the address of the memory
> + * @bar: the BAR number corresponding to map addr
> + * @base: return base address, which match BAR's alignment requirement, nothing
> + *	  return if NULL

Below, you are updating 'base' only if it is not NULL. Why would anyone call
this API with 'base' and 'offset' set to NULL?

> + * @off: return offset, nothing return if NULL
> + *
> + * Helper function to convert input 'addr' to base and offset, which match
> + * BAR's alignment requirement.
> + *
> + * The pci_epf_alloc_space() function already accounts for alignment. This is
> + * primarily intended for use with other memory regions not allocated by
> + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger
> + * address for a platform MSI controller.
> + */
> +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> +			       u64 addr, u64 *base, size_t *off)
> +{
> +	const struct pci_epc_features *epc_features;
> +	u64 align;
> +
> +	epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
> +	if (!epc_features) {
> +		dev_err(&epf->dev, "epc_features not implemented\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	align = epc_features->align;
> +	align = align ? align : 128;
> +	if (epc_features->bar[bar].type == BAR_FIXED)
> +		align = max(epc_features->bar[bar].fixed_size, align);
> +
> +	if (base)
> +		*base = round_down(addr, align);
> +
> +	if (off)
> +		*off = addr & (align - 1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_align_inbound_addr);
> +
>  static void pci_epf_dev_release(struct device *dev)
>  {
>  	struct pci_epf *epf = to_pci_epf(dev);
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 5374e6515ffa0..eff73ccb5e702 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -238,6 +238,20 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  			  enum pci_epc_interface_type type);
>  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>  			enum pci_epc_interface_type type);
> +
> +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> +			       u64 addr, u64 *base, size_t *off);
> +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> +						   u32 low, u32 high, u64 *base, size_t *off)

Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller
could pass u64 address directly.

- Mani

> +{
> +	u64 addr = high;
> +
> +	addr <<= 32;
> +	addr |= low;
> +
> +	return pci_epf_align_inbound_addr(epf, bar, addr, base, off);
> +}
> +
>  int pci_epf_bind(struct pci_epf *epf);
>  void pci_epf_unbind(struct pci_epf *epf);
>  int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> 
> -- 
> 2.34.1
> 

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

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Manivannan Sadhasivam]

On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a

I don't see 'doorbell_done' defined anywhere.

> doorbell address space.
> 
> Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> callback handler by writing doorbell_data to the mapped doorbell_bar's
> address space.
> 
> Set doorbell_done in the doorbell callback to indicate completion.
> 

Same here.

> To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL

'avoid breaking compatibility between host and endpoint,...'

> and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> to map one bar's inbound address to MSI space. the command
> COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> 
> 	 	Host side new driver	Host side old driver
> 
> EP: new driver      S				F
> EP: old driver      F				F

So the last case of old EP and host drivers will fail?

> 
> S: If EP side support MSI, 'pcitest -B' return success.
>    If EP side doesn't support MSI, the same to 'F'.
> 
> F: 'pcitest -B' return failure, other case as usual.
> 
> Tested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v7 to v8
> - rename to pci_epf_align_inbound_addr_lo_hi()
> 
> Change from v6 to v7
> - use help function pci_epf_align_addr_lo_hi()
> 
> Change from v5 to v6
> - rename doorbell_addr to doorbell_offset
> 
> Chagne from v4 to v5
> - Add doorbell free at unbind function.
> - Move msi irq handler to here to more complex user case, such as differece
> doorbell can use difference handler function.
> - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> tag or co-developer tag, please let me know.
> 
> change from v3 to v4
> - remove revid requirement
> - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> - call pci_epc_set_bar() to map inbound address to MSI space only at
> COMMAND_ENABLE_DOORBELL.
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index ef6677f34116e..410b2f4bb7ce7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -11,12 +11,14 @@
>  #include <linux/dmaengine.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  #include <linux/slab.h>
>  #include <linux/pci_ids.h>
>  #include <linux/random.h>
>  
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> +#include <linux/pci-ep-msi.h>
>  #include <linux/pci_regs.h>
>  
>  #define IRQ_TYPE_INTX			0
> @@ -29,6 +31,8 @@
>  #define COMMAND_READ			BIT(3)
>  #define COMMAND_WRITE			BIT(4)
>  #define COMMAND_COPY			BIT(5)
> +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> +#define COMMAND_DISABLE_DOORBELL	BIT(7)
>  
>  #define STATUS_READ_SUCCESS		BIT(0)
>  #define STATUS_READ_FAIL		BIT(1)
> @@ -39,6 +43,11 @@
>  #define STATUS_IRQ_RAISED		BIT(6)
>  #define STATUS_SRC_ADDR_INVALID		BIT(7)
>  #define STATUS_DST_ADDR_INVALID		BIT(8)
> +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
>  
>  #define FLAG_USE_DMA			BIT(0)
>  
> @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
>  	u32	irq_type;
>  	u32	irq_number;
>  	u32	flags;
> +	u32	doorbell_bar;
> +	u32	doorbell_offset;
> +	u32	doorbell_data;
>  } __packed;
>  
>  static struct pci_epf_header test_header = {
> @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  	}
>  }
>  
> +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> +{
> +	enum pci_barno bar = reg->doorbell_bar;
> +	struct pci_epf *epf = epf_test->epf;
> +	struct pci_epc *epc = epf->epc;
> +	struct pci_epf_bar db_bar;

db_bar = {};

> +	struct msi_msg *msg;
> +	size_t offset;
> +	int ret;
> +
> +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {

What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
allocated proper BAR already.

> +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> +		return;
> +	}
> +
> +	msg = &epf->db_msg[0].msg;
> +	ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
> +					       &db_bar.phys_addr, &offset);
> +
> +	if (ret) {
> +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> +		return;
> +	}
> +
> +	reg->doorbell_offset = offset;
> +
> +	db_bar.barno = bar;
> +	db_bar.size = epf->bar[bar].size;
> +	db_bar.flags = epf->bar[bar].flags;
> +	db_bar.addr = NULL;

Not needed if you initialize above.

> +
> +	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar);
> +	if (!ret)
> +		reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS;
> +	else
> +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> +}
> +

[...]

>  static const struct pci_epc_event_ops pci_epf_test_event_ops = {
>  	.epc_init = pci_epf_test_epc_init,
>  	.epc_deinit = pci_epf_test_epc_deinit,
> @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	if (ret)
>  		return ret;
>  
> +	ret = pci_epf_alloc_doorbell(epf, 1);
> +	if (!ret) {
> +		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +		struct msi_msg *msg = &epf->db_msg[0].msg;
> +		enum pci_barno bar;
> +
> +		bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);

NO_BAR check?

- Mani

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

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Niklas Cassel]

On 24 November 2024 08:11:00 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote:
>> +static int pci_epc_alloc_doorbell(struct pci_epc *epc, struct pci_epf *epf)
>> +{
>> +	struct device *dev = epc->dev.parent;
>> +	u16 num_db = epf->num_db;
>> +	int i = 0;
>> +	int ret;
>> +
>> +	guard(mutex)(&epc->lock);
>> +
>> +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to allocate MSI, may miss 'msi-parent' at your DTS\n");
>
>No need to mention 'msi-parent'. Just 'Failed to allocate MSI' is enough.

If you look at the existing pcie_ep device tree nodes for all SoCs, you will see that it is very rare to see an EP node which specifies msi-parent.

I guess that makes sense, since before this series, AFAICT, msi-parent was completely unused, so there was no need for an EP node to specify it.

I'm okay to change the error print as you suggested, but in that case I really think that we should add a comment above the check, something suggestive like:

/*
 * The pcie_ep DT node has to specify
 * 'msi-parent' for EP doorbell support to work.
 * Right now only GIC ITS is supported.
 * If you have GIC ITS and reached this print,
 * perhaps you are missing 'msi-parent' in DT?
 */
if (ret) {
        dev_err(dev, "Failed to allocate MSI\n");
        return -ENODEV;
}


Kind regards,
Niklas

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Sun, Nov 24, 2024 at 10:56:38AM +0100, Niklas Cassel wrote:
> 
> 
> On 24 November 2024 08:11:00 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> >On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote:
> >> +static int pci_epc_alloc_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> >> +{
> >> +	struct device *dev = epc->dev.parent;
> >> +	u16 num_db = epf->num_db;
> >> +	int i = 0;
> >> +	int ret;
> >> +
> >> +	guard(mutex)(&epc->lock);
> >> +
> >> +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to allocate MSI, may miss 'msi-parent' at your DTS\n");
> >
> >No need to mention 'msi-parent'. Just 'Failed to allocate MSI' is enough.
> 
> If you look at the existing pcie_ep device tree nodes for all SoCs, you will see that it is very rare to see an EP node which specifies msi-parent.
> 
> I guess that makes sense, since before this series, AFAICT, msi-parent was completely unused, so there was no need for an EP node to specify it.
> 
> I'm okay to change the error print as you suggested, but in that case I really think that we should add a comment above the check, something suggestive like:
> 
> /*
>  * The pcie_ep DT node has to specify
>  * 'msi-parent' for EP doorbell support to work.
>  * Right now only GIC ITS is supported.
>  * If you have GIC ITS and reached this print,
>  * perhaps you are missing 'msi-parent' in DT?
>  */

Looks good to me (except that the comment needs to fit 80 columns) :)

- Mani

> if (ret) {
>         dev_err(dev, "Failed to allocate MSI\n");
>         return -ENODEV;
> }
> 
> 
> Kind regards,
> Niklas

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

Re: [PATCH v8 5/6] misc: pci_endpoint_test: Add doorbell test case

[Manivannan Sadhasivam]

On Sat, Nov 16, 2024 at 09:40:45AM -0500, Frank Li wrote:
> Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR,
> and PCIE_ENDPOINT_TEST_DB_DATA.
> 
> Trigger the doorbell by writing data from PCI_ENDPOINT_TEST_DB_DATA to the
> address provided by PCI_ENDPOINT_TEST_DB_OFFSET and wait for endpoint
> feedback.
> 
> Add two command to COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL
> to enable EP side's doorbell support and avoid compatible problem.

Can you explain the 'compatible problem' and how this patch avoids it? Just for
the sake of completeness.

> 
> 		Host side new driver	Host side old driver
> EP: new driver		S			F
> EP: old driver		F			F
> 
> S: If EP side support MSI, 'pcitest -B' return success.
>    If EP side doesn't support MSI, the same to 'F'.
> 
> F: 'pcitest -B' return failure, other case as usual.
> 
> Tested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change form v6 to v8
> - none
> 
> Change from v5 to v6
> - %s/PCI_ENDPOINT_TEST_DB_ADDR/PCI_ENDPOINT_TEST_DB_OFFSET/g
> 
> Change from v4 to v5
> - remove unused varible
> - add irq_type at pci_endpoint_test_doorbell();
> 
> change from v3 to v4
> - Add COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> - Remove new DID requirement.
> ---
>  drivers/misc/pci_endpoint_test.c | 71 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/pcitest.h     |  1 +
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee2..dc766055aa594 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -42,6 +42,8 @@
>  #define COMMAND_READ				BIT(3)
>  #define COMMAND_WRITE				BIT(4)
>  #define COMMAND_COPY				BIT(5)
> +#define COMMAND_ENABLE_DOORBELL			BIT(6)
> +#define COMMAND_DISABLE_DOORBELL		BIT(7)
>  
>  #define PCI_ENDPOINT_TEST_STATUS		0x8
>  #define STATUS_READ_SUCCESS			BIT(0)
> @@ -53,6 +55,11 @@
>  #define STATUS_IRQ_RAISED			BIT(6)
>  #define STATUS_SRC_ADDR_INVALID			BIT(7)
>  #define STATUS_DST_ADDR_INVALID			BIT(8)
> +#define STATUS_DOORBELL_SUCCESS			BIT(9)
> +#define STATUS_DOORBELL_ENABLE_SUCCESS		BIT(10)
> +#define STATUS_DOORBELL_ENABLE_FAIL		BIT(11)
> +#define STATUS_DOORBELL_DISABLE_SUCCESS		BIT(12)
> +#define STATUS_DOORBELL_DISABLE_FAIL		BIT(13)
>  
>  #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0x0c
>  #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
> @@ -67,6 +74,10 @@
>  #define PCI_ENDPOINT_TEST_IRQ_NUMBER		0x28
>  
>  #define PCI_ENDPOINT_TEST_FLAGS			0x2c
> +#define PCI_ENDPOINT_TEST_DB_BAR		0x30
> +#define PCI_ENDPOINT_TEST_DB_OFFSET		0x34
> +#define PCI_ENDPOINT_TEST_DB_DATA		0x38
> +
>  #define FLAG_USE_DMA				BIT(0)
>  
>  #define PCI_DEVICE_ID_TI_AM654			0xb00c
> @@ -108,6 +119,7 @@ enum pci_barno {
>  	BAR_3,
>  	BAR_4,
>  	BAR_5,
> +	NO_BAR = -1,

I really hate duplicating this enum definition both in EPF driver and here.
Maybe we should move this to pci.h?

>  };
>  
>  struct pci_endpoint_test {
> @@ -746,6 +758,62 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
>  	return false;
>  }
>  
> +static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
> +{
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +	int irq_type = test->irq_type;
> +	enum pci_barno bar;
> +	u32 data, status;
> +	u32 addr;
> +
> +	if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) {
> +		dev_err(dev, "Invalid IRQ type option\n");
> +		return false;
> +	}

Is this check necessary?

> +
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> +				 COMMAND_ENABLE_DOORBELL);
> +
> +	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
> +
> +	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> +	if (status & STATUS_DOORBELL_ENABLE_FAIL)
> +		return false;

I think we should add a error print here and below.

> +
> +	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
> +	addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET);
> +	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> +
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> +
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
> +
> +	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> +
> +	writel(data, test->bar[bar] + addr);
> +
> +	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
> +
> +	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> +
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> +				 COMMAND_DISABLE_DOORBELL);
> +
> +	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
> +
> +	status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> +
> +	if ((status & STATUS_DOORBELL_SUCCESS) &&
> +	    (status & STATUS_DOORBELL_DISABLE_SUCCESS))
> +		return true;

Usual convention is to check for error and return true at the end.

> +
> +	return false;
> +}
> +
>  static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  				    unsigned long arg)
>  {
> @@ -793,6 +861,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  	case PCITEST_CLEAR_IRQ:
>  		ret = pci_endpoint_test_clear_irq(test);
>  		break;
> +	case PCITEST_DOORBELL:
> +		ret = pci_endpoint_test_doorbell(test);
> +		break;
>  	}
>  
>  ret:
> diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
> index 94b46b043b536..06d9f548b510e 100644
> --- a/include/uapi/linux/pcitest.h
> +++ b/include/uapi/linux/pcitest.h
> @@ -21,6 +21,7 @@
>  #define PCITEST_SET_IRQTYPE	_IOW('P', 0x8, int)
>  #define PCITEST_GET_IRQTYPE	_IO('P', 0x9)
>  #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
> +#define PCITEST_DOORBELL	_IO('P', 0x11)

I think defining PCITEST_CLEAR_IRQ as 0x10 was a mistake. It should've been 0xa.
But since it is a uapi, we cannot change it. Atleast add new ones starting from
0xa.

Niklas's consecutive BAR patch adds a new ioctl for 0xa, but we can fix the
conflict later depending on which patch gets merged first.

- Mani

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

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Niklas Cassel]

On 24 November 2024 14:17:01 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>On Sun, Nov 24, 2024 at 10:56:38AM +0100, Niklas Cassel wrote:
>> 
>> I'm okay to change the error print as you suggested, but in that case I really think that we should add a comment above the check, something suggestive like:
>> 
>> /*
>>  * The pcie_ep DT node has to specify
>>  * 'msi-parent' for EP doorbell support to work.
>>  * Right now only GIC ITS is supported.
>>  * If you have GIC ITS and reached this print,
>>  * perhaps you are missing 'msi-parent' in DT?
>>  */
>
>Looks good to me (except that the comment needs to fit 80 columns) :)

Sorry about that, I wrote the reply from my phone :D


Kind regards,
Niklas

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Sun, Nov 24, 2024 at 12:41:00PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote:
> > Doorbell feature is implemented by mapping the EP's MSI interrupt
> > controller message address to a dedicated BAR in the EPC core. It is the
> > responsibility of the EPF driver to pass the actual message data to be
> > written by the host to the doorbell BAR region through its own logic.
> >
> > Tested-by: Niklas Cassel <cassel@kernel.org>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v5 to v8
> > -none
> >
> > Change from v4 to v5
> > - Remove request_irq() in pci_epc_alloc_doorbell() and leave to EP function
> > driver, so ep function driver can register differece call back function for
> > difference doorbell events and set irq affinity to differece CPU core.
> > - Improve error message when MSI allocate failure.
> >
> > Change from v3 to v4
> > - msi change to use msi_get_virq() avoid use msi_for_each_desc().
> > - add new struct for pci_epf_doorbell_msg to msi msg,virq and irq name.
> > - move mutex lock to epc function
> > - initialize variable at declear place.
> > - passdown epf to epc*() function to simplify code.
> > ---
> >  drivers/pci/endpoint/Makefile     |  2 +-
> >  drivers/pci/endpoint/pci-ep-msi.c | 99 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-ep-msi.h        | 15 ++++++
> >  include/linux/pci-epf.h           | 16 +++++++
> >  4 files changed, 131 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> > index 95b2fe47e3b06..a1ccce440c2c5 100644
> > --- a/drivers/pci/endpoint/Makefile
> > +++ b/drivers/pci/endpoint/Makefile
> > @@ -5,4 +5,4 @@
> >
> >  obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
> >  obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
> > -					   pci-epc-mem.o functions/
> > +					   pci-epc-mem.o pci-ep-msi.o functions/
> > diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> > new file mode 100644
> > index 0000000000000..7868a529dce37
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PCI Endpoint *Controller* (EPC) MSI library
> > + *
> > + * Copyright (C) 2024 NXP
> > + * Author: Frank Li <Frank.Li@nxp.com>
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/device.h>
> > +#include <linux/slab.h>
>
> Please sort alphabetically.
>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/pci-epc.h>
> > +#include <linux/pci-epf.h>
> > +#include <linux/pci-ep-cfs.h>
> > +#include <linux/pci-ep-msi.h>
> > +
> > +static bool pci_epc_match_parent(struct device *dev, void *param)
> > +{
> > +	return dev->parent == param;
> > +}
> > +
> > +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > +	struct pci_epc *epc __free(pci_epc_put) = NULL;
> > +	struct pci_epf *epf;
> > +
> > +	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
>
> You were passing 'epc->dev.parent' to platform_device_msi_init_and_alloc_irqs().
> So 'desc->dev' should be the EPC parent, right? If so, you can do:
>
> 	epc = pci_epc_get(dev_name(msi_desc_to_dev(desc)));
>
> since we are reusing the parent dev name for EPC.

I think it is not good to depend on hidden situation, "name is the same."
May it change in future because no one will realize here depend on the same
name and just think it is trivial update for device name.

>
> > +	if (!epc)
> > +		return;
> > +
> > +	/* Only support one EPF for doorbell */
> > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
>
> Why don't you impose this restriction in pci_epf_alloc_doorbell() itself?

This is callback from platform_device_msi_init_and_alloc_irqs(). So it is
actually restriction at pci_epf_alloc_doorbell().

>
> > +
> > +	if (epf && epf->db_msg && desc->msi_index < epf->num_db)
> > +		memcpy(&epf->db_msg[desc->msi_index].msg, msg, sizeof(*msg));
> > +}
> > +
> > +static void pci_epc_free_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> > +{
> > +	guard(mutex)(&epc->lock);
> > +
> > +	platform_device_msi_free_irqs_all(epc->dev.parent);
> > +}
> > +
> > +static int pci_epc_alloc_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> > +{
> > +	struct device *dev = epc->dev.parent;
> > +	u16 num_db = epf->num_db;
> > +	int i = 0;
> > +	int ret;
> > +
> > +	guard(mutex)(&epc->lock);
> > +
> > +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to allocate MSI, may miss 'msi-parent' at your DTS\n");
>
> No need to mention 'msi-parent'. Just 'Failed to allocate MSI' is enough.
>
> > +		return -ENOMEM;
>
> -ENODEV?
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 5/6] misc: pci_endpoint_test: Add doorbell test case

[Frank Li]

On Sun, Nov 24, 2024 at 07:21:28PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Nov 16, 2024 at 09:40:45AM -0500, Frank Li wrote:
> > Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR,
> > and PCIE_ENDPOINT_TEST_DB_DATA.
> >
> > Trigger the doorbell by writing data from PCI_ENDPOINT_TEST_DB_DATA to the
> > address provided by PCI_ENDPOINT_TEST_DB_OFFSET and wait for endpoint
> > feedback.
> >
> > Add two command to COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL
> > to enable EP side's doorbell support and avoid compatible problem.
>
> Can you explain the 'compatible problem' and how this patch avoids it? Just for
> the sake of completeness.
>
> >
> > 		Host side new driver	Host side old driver
> > EP: new driver		S			F
> > EP: old driver		F			F
> >
> > S: If EP side support MSI, 'pcitest -B' return success.
> >    If EP side doesn't support MSI, the same to 'F'.
> >
> > F: 'pcitest -B' return failure, other case as usual.
> >
> > Tested-by: Niklas Cassel <cassel@kernel.org>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change form v6 to v8
> > - none
> >
> > Change from v5 to v6
> > - %s/PCI_ENDPOINT_TEST_DB_ADDR/PCI_ENDPOINT_TEST_DB_OFFSET/g
> >
> > Change from v4 to v5
> > - remove unused varible
> > - add irq_type at pci_endpoint_test_doorbell();
> >
> > change from v3 to v4
> > - Add COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > - Remove new DID requirement.
> > ---
> >  drivers/misc/pci_endpoint_test.c | 71 ++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/pcitest.h     |  1 +
> >  2 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index 3aaaf47fa4ee2..dc766055aa594 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -42,6 +42,8 @@
> >  #define COMMAND_READ				BIT(3)
> >  #define COMMAND_WRITE				BIT(4)
> >  #define COMMAND_COPY				BIT(5)
> > +#define COMMAND_ENABLE_DOORBELL			BIT(6)
> > +#define COMMAND_DISABLE_DOORBELL		BIT(7)
> >
> >  #define PCI_ENDPOINT_TEST_STATUS		0x8
> >  #define STATUS_READ_SUCCESS			BIT(0)
> > @@ -53,6 +55,11 @@
> >  #define STATUS_IRQ_RAISED			BIT(6)
> >  #define STATUS_SRC_ADDR_INVALID			BIT(7)
> >  #define STATUS_DST_ADDR_INVALID			BIT(8)
> > +#define STATUS_DOORBELL_SUCCESS			BIT(9)
> > +#define STATUS_DOORBELL_ENABLE_SUCCESS		BIT(10)
> > +#define STATUS_DOORBELL_ENABLE_FAIL		BIT(11)
> > +#define STATUS_DOORBELL_DISABLE_SUCCESS		BIT(12)
> > +#define STATUS_DOORBELL_DISABLE_FAIL		BIT(13)
> >
> >  #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0x0c
> >  #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
> > @@ -67,6 +74,10 @@
> >  #define PCI_ENDPOINT_TEST_IRQ_NUMBER		0x28
> >
> >  #define PCI_ENDPOINT_TEST_FLAGS			0x2c
> > +#define PCI_ENDPOINT_TEST_DB_BAR		0x30
> > +#define PCI_ENDPOINT_TEST_DB_OFFSET		0x34
> > +#define PCI_ENDPOINT_TEST_DB_DATA		0x38
> > +
> >  #define FLAG_USE_DMA				BIT(0)
> >
> >  #define PCI_DEVICE_ID_TI_AM654			0xb00c
> > @@ -108,6 +119,7 @@ enum pci_barno {
> >  	BAR_3,
> >  	BAR_4,
> >  	BAR_5,
> > +	NO_BAR = -1,
>
> I really hate duplicating this enum definition both in EPF driver and here.
> Maybe we should move this to pci.h?

Yes, It is not related this patch. It should belong clean up patch. We
can do it later.

>
> >  };
> >
> >  struct pci_endpoint_test {
> > @@ -746,6 +758,62 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
> >  	return false;
> >  }
> >
> > +static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
> > +{
> > +	struct pci_dev *pdev = test->pdev;
> > +	struct device *dev = &pdev->dev;
> > +	int irq_type = test->irq_type;
> > +	enum pci_barno bar;
> > +	u32 data, status;
> > +	u32 addr;
> > +
> > +	if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) {
> > +		dev_err(dev, "Invalid IRQ type option\n");
> > +		return false;
> > +	}
>
> Is this check necessary?

Suppose yes, it pass down from RC driver side, we should not suppose it
alwasy do correct things.

>
> > +
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> > +				 COMMAND_ENABLE_DOORBELL);
> > +
> > +	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
> > +
> > +	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> > +	if (status & STATUS_DOORBELL_ENABLE_FAIL)
> > +		return false;
>
> I think we should add a error print here and below.
>
> > +
> > +	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
> > +	addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET);
> > +	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> > +
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> > +
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
> > +
> > +	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> > +
> > +	writel(data, test->bar[bar] + addr);
> > +
> > +	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
> > +
> > +	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> > +
> > +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> > +				 COMMAND_DISABLE_DOORBELL);
> > +
> > +	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
> > +
> > +	status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> > +
> > +	if ((status & STATUS_DOORBELL_SUCCESS) &&
> > +	    (status & STATUS_DOORBELL_DISABLE_SUCCESS))
> > +		return true;
>
> Usual convention is to check for error and return true at the end.
>
> > +
> > +	return false;
> > +}
> > +
> >  static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
> >  				    unsigned long arg)
> >  {
> > @@ -793,6 +861,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
> >  	case PCITEST_CLEAR_IRQ:
> >  		ret = pci_endpoint_test_clear_irq(test);
> >  		break;
> > +	case PCITEST_DOORBELL:
> > +		ret = pci_endpoint_test_doorbell(test);
> > +		break;
> >  	}
> >
> >  ret:
> > diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
> > index 94b46b043b536..06d9f548b510e 100644
> > --- a/include/uapi/linux/pcitest.h
> > +++ b/include/uapi/linux/pcitest.h
> > @@ -21,6 +21,7 @@
> >  #define PCITEST_SET_IRQTYPE	_IOW('P', 0x8, int)
> >  #define PCITEST_GET_IRQTYPE	_IO('P', 0x9)
> >  #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
> > +#define PCITEST_DOORBELL	_IO('P', 0x11)
>
> I think defining PCITEST_CLEAR_IRQ as 0x10 was a mistake. It should've been 0xa.
> But since it is a uapi, we cannot change it. Atleast add new ones starting from
> 0xa.
>

Yes, thanks.

> Niklas's consecutive BAR patch adds a new ioctl for 0xa, but we can fix the
> conflict later depending on which patch gets merged first.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
>
> I don't see 'doorbell_done' defined anywhere.
>
> > doorbell address space.
> >
> > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > address space.
> >
> > Set doorbell_done in the doorbell callback to indicate completion.
> >
>
> Same here.
>
> > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
>
> 'avoid breaking compatibility between host and endpoint,...'
>
> > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > to map one bar's inbound address to MSI space. the command
> > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> >
> > 	 	Host side new driver	Host side old driver
> >
> > EP: new driver      S				F
> > EP: old driver      F				F
>
> So the last case of old EP and host drivers will fail?

doorbell test will fail if old EP.

>
> >
> > S: If EP side support MSI, 'pcitest -B' return success.
> >    If EP side doesn't support MSI, the same to 'F'.
> >
> > F: 'pcitest -B' return failure, other case as usual.
> >
> > Tested-by: Niklas Cassel <cassel@kernel.org>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v7 to v8
> > - rename to pci_epf_align_inbound_addr_lo_hi()
> >
> > Change from v6 to v7
> > - use help function pci_epf_align_addr_lo_hi()
> >
> > Change from v5 to v6
> > - rename doorbell_addr to doorbell_offset
> >
> > Chagne from v4 to v5
> > - Add doorbell free at unbind function.
> > - Move msi irq handler to here to more complex user case, such as differece
> > doorbell can use difference handler function.
> > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > tag or co-developer tag, please let me know.
> >
> > change from v3 to v4
> > - remove revid requirement
> > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > COMMAND_ENABLE_DOORBELL.
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index ef6677f34116e..410b2f4bb7ce7 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -11,12 +11,14 @@
> >  #include <linux/dmaengine.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > +#include <linux/msi.h>
> >  #include <linux/slab.h>
> >  #include <linux/pci_ids.h>
> >  #include <linux/random.h>
> >
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> > +#include <linux/pci-ep-msi.h>
> >  #include <linux/pci_regs.h>
> >
> >  #define IRQ_TYPE_INTX			0
> > @@ -29,6 +31,8 @@
> >  #define COMMAND_READ			BIT(3)
> >  #define COMMAND_WRITE			BIT(4)
> >  #define COMMAND_COPY			BIT(5)
> > +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> > +#define COMMAND_DISABLE_DOORBELL	BIT(7)
> >
> >  #define STATUS_READ_SUCCESS		BIT(0)
> >  #define STATUS_READ_FAIL		BIT(1)
> > @@ -39,6 +43,11 @@
> >  #define STATUS_IRQ_RAISED		BIT(6)
> >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> > +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> > +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
> >
> >  #define FLAG_USE_DMA			BIT(0)
> >
> > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> >  	u32	irq_type;
> >  	u32	irq_number;
> >  	u32	flags;
> > +	u32	doorbell_bar;
> > +	u32	doorbell_offset;
> > +	u32	doorbell_data;
> >  } __packed;
> >
> >  static struct pci_epf_header test_header = {
> > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> >  	}
> >  }
> >
> > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > +{
> > +	enum pci_barno bar = reg->doorbell_bar;
> > +	struct pci_epf *epf = epf_test->epf;
> > +	struct pci_epc *epc = epf->epc;
> > +	struct pci_epf_bar db_bar;
>
> db_bar = {};
>
> > +	struct msi_msg *msg;
> > +	size_t offset;
> > +	int ret;
> > +
> > +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
>
> What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> allocated proper BAR already.

Not check it at call pci_epf_alloc_doorbell() because it optional feature.
It return failure when it actually use it.

>
> > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > +		return;
> > +	}
> > +
> > +	msg = &epf->db_msg[0].msg;
> > +	ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
> > +					       &db_bar.phys_addr, &offset);
> > +
> > +	if (ret) {
> > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > +		return;
> > +	}
> > +
> > +	reg->doorbell_offset = offset;
> > +
> > +	db_bar.barno = bar;
> > +	db_bar.size = epf->bar[bar].size;
> > +	db_bar.flags = epf->bar[bar].flags;
> > +	db_bar.addr = NULL;
>
> Not needed if you initialize above.
>
> > +
> > +	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar);
> > +	if (!ret)
> > +		reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS;
> > +	else
> > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > +}
> > +
>
> [...]
>
> >  static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> >  	.epc_init = pci_epf_test_epc_init,
> >  	.epc_deinit = pci_epf_test_epc_deinit,
> > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = pci_epf_alloc_doorbell(epf, 1);
> > +	if (!ret) {
> > +		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> > +		struct msi_msg *msg = &epf->db_msg[0].msg;
> > +		enum pci_barno bar;
> > +
> > +		bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
>
> NO_BAR check?

This is optional feature, It should check when use it.

Frank

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

Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper for address alignment

[Frank Li]

On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> > Introduce the helper function pci_epf_align_addr() to adjust addresses
>
> pci_epf_align_inbound_addr()?
>
> > according to PCI BAR alignment requirements, converting addresses into base
> > and offset values.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v7 to v8
> > - change name to pci_epf_align_inbound_addr()
> > - update comment said only need for memory, which not allocated by
> > pci_epf_alloc_space().
> >
> > change from v6 to v7
> > - new patch
> > ---
> >  drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-epf.h             | 14 ++++++++++++
> >  2 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 8fa2797d4169a..4dfc218ebe20b 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epf_create);
> >
> > +/**
> > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's
>
> BAR's
>
> > + *			  alignment requirement
> > + * @epf: the EPF device
> > + * @addr: the address of the memory
> > + * @bar: the BAR number corresponding to map addr
> > + * @base: return base address, which match BAR's alignment requirement, nothing
> > + *	  return if NULL
>
> Below, you are updating 'base' only if it is not NULL. Why would anyone call
> this API with 'base' and 'offset' set to NULL?

Some time, they may just want one of two.

>
> > + * @off: return offset, nothing return if NULL
> > + *
> > + * Helper function to convert input 'addr' to base and offset, which match
> > + * BAR's alignment requirement.
> > + *
> > + * The pci_epf_alloc_space() function already accounts for alignment. This is
> > + * primarily intended for use with other memory regions not allocated by
> > + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger
> > + * address for a platform MSI controller.
> > + */
> > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> > +			       u64 addr, u64 *base, size_t *off)
> > +{
> > +	const struct pci_epc_features *epc_features;
> > +	u64 align;
> > +
> > +	epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
> > +	if (!epc_features) {
> > +		dev_err(&epf->dev, "epc_features not implemented\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	align = epc_features->align;
> > +	align = align ? align : 128;
> > +	if (epc_features->bar[bar].type == BAR_FIXED)
> > +		align = max(epc_features->bar[bar].fixed_size, align);
> > +
> > +	if (base)
> > +		*base = round_down(addr, align);
> > +
> > +	if (off)
> > +		*off = addr & (align - 1);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_align_inbound_addr);
> > +
> >  static void pci_epf_dev_release(struct device *dev)
> >  {
> >  	struct pci_epf *epf = to_pci_epf(dev);
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 5374e6515ffa0..eff73ccb5e702 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -238,6 +238,20 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> >  			  enum pci_epc_interface_type type);
> >  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> >  			enum pci_epc_interface_type type);
> > +
> > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> > +			       u64 addr, u64 *base, size_t *off);
> > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> > +						   u32 low, u32 high, u64 *base, size_t *off)
>
> Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller
> could pass u64 address directly.


msi message sperate low32 and high32.  (h << 32 | low) is quite easy to
cause build warning.  it should be ((u64) h << 32) | low. Avoid copy this
logic code at many EPF places.

Frank

>
> - Mani
>
> > +{
> > +	u64 addr = high;
> > +
> > +	addr <<= 32;
> > +	addr |= low;
> > +
> > +	return pci_epf_align_inbound_addr(epf, bar, addr, base, off);
> > +}
> > +
> >  int pci_epf_bind(struct pci_epf *epf);
> >  void pci_epf_unbind(struct pci_epf *epf);
> >  int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> >
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Mon, Nov 25, 2024 at 01:03:37PM -0500, Frank Li wrote:
> On Sun, Nov 24, 2024 at 12:41:00PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote:
> > > Doorbell feature is implemented by mapping the EP's MSI interrupt
> > > controller message address to a dedicated BAR in the EPC core. It is the
> > > responsibility of the EPF driver to pass the actual message data to be
> > > written by the host to the doorbell BAR region through its own logic.
> > >
> > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > change from v5 to v8
> > > -none
> > >
> > > Change from v4 to v5
> > > - Remove request_irq() in pci_epc_alloc_doorbell() and leave to EP function
> > > driver, so ep function driver can register differece call back function for
> > > difference doorbell events and set irq affinity to differece CPU core.
> > > - Improve error message when MSI allocate failure.
> > >
> > > Change from v3 to v4
> > > - msi change to use msi_get_virq() avoid use msi_for_each_desc().
> > > - add new struct for pci_epf_doorbell_msg to msi msg,virq and irq name.
> > > - move mutex lock to epc function
> > > - initialize variable at declear place.
> > > - passdown epf to epc*() function to simplify code.
> > > ---
> > >  drivers/pci/endpoint/Makefile     |  2 +-
> > >  drivers/pci/endpoint/pci-ep-msi.c | 99 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pci-ep-msi.h        | 15 ++++++
> > >  include/linux/pci-epf.h           | 16 +++++++
> > >  4 files changed, 131 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> > > index 95b2fe47e3b06..a1ccce440c2c5 100644
> > > --- a/drivers/pci/endpoint/Makefile
> > > +++ b/drivers/pci/endpoint/Makefile
> > > @@ -5,4 +5,4 @@
> > >
> > >  obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
> > >  obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
> > > -					   pci-epc-mem.o functions/
> > > +					   pci-epc-mem.o pci-ep-msi.o functions/
> > > diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> > > new file mode 100644
> > > index 0000000000000..7868a529dce37
> > > --- /dev/null
> > > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > > @@ -0,0 +1,99 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PCI Endpoint *Controller* (EPC) MSI library
> > > + *
> > > + * Copyright (C) 2024 NXP
> > > + * Author: Frank Li <Frank.Li@nxp.com>
> > > + */
> > > +
> > > +#include <linux/cleanup.h>
> > > +#include <linux/device.h>
> > > +#include <linux/slab.h>
> >
> > Please sort alphabetically.
> >
> > > +#include <linux/module.h>
> > > +#include <linux/msi.h>
> > > +#include <linux/pci-epc.h>
> > > +#include <linux/pci-epf.h>
> > > +#include <linux/pci-ep-cfs.h>
> > > +#include <linux/pci-ep-msi.h>
> > > +
> > > +static bool pci_epc_match_parent(struct device *dev, void *param)
> > > +{
> > > +	return dev->parent == param;
> > > +}
> > > +
> > > +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > +{
> > > +	struct pci_epc *epc __free(pci_epc_put) = NULL;
> > > +	struct pci_epf *epf;
> > > +
> > > +	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
> >
> > You were passing 'epc->dev.parent' to platform_device_msi_init_and_alloc_irqs().
> > So 'desc->dev' should be the EPC parent, right? If so, you can do:
> >
> > 	epc = pci_epc_get(dev_name(msi_desc_to_dev(desc)));
> >
> > since we are reusing the parent dev name for EPC.
> 
> I think it is not good to depend on hidden situation, "name is the same."
> May it change in future because no one will realize here depend on the same
> name and just think it is trivial update for device name.
> 

No one should change the EPC name just like that. The name is exposed to
configfs interface and the existing userspace scripts rely on that. So changing
the name will break them.

I'd strongly suggest you to use the existing API instead of adding a new one for
the same purpose.

> >
> > > +	if (!epc)
> > > +		return;
> > > +
> > > +	/* Only support one EPF for doorbell */
> > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> >
> > Why don't you impose this restriction in pci_epf_alloc_doorbell() itself?
> 
> This is callback from platform_device_msi_init_and_alloc_irqs(). So it is
> actually restriction at pci_epf_alloc_doorbell().
> 

I don't know how to parse this last sentence. But my question was why don't you
impose this one EPF restriction in pci_epf_alloc_doorbell() before allocating
the MSI domain using platform_device_msi_init_and_alloc_irqs()? This way if
someone calls pci_epf_alloc_doorbell() multi EPF, it will fail.

- Mani

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

Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper for address alignment

[Manivannan Sadhasivam]

On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote:
> On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> > > Introduce the helper function pci_epf_align_addr() to adjust addresses
> >
> > pci_epf_align_inbound_addr()?
> >
> > > according to PCI BAR alignment requirements, converting addresses into base
> > > and offset values.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > change from v7 to v8
> > > - change name to pci_epf_align_inbound_addr()
> > > - update comment said only need for memory, which not allocated by
> > > pci_epf_alloc_space().
> > >
> > > change from v6 to v7
> > > - new patch
> > > ---
> > >  drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
> > >  include/linux/pci-epf.h             | 14 ++++++++++++
> > >  2 files changed, 59 insertions(+)
> > >
> > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > index 8fa2797d4169a..4dfc218ebe20b 100644
> > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_epf_create);
> > >
> > > +/**
> > > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's
> >
> > BAR's
> >
> > > + *			  alignment requirement
> > > + * @epf: the EPF device
> > > + * @addr: the address of the memory
> > > + * @bar: the BAR number corresponding to map addr
> > > + * @base: return base address, which match BAR's alignment requirement, nothing
> > > + *	  return if NULL
> >
> > Below, you are updating 'base' only if it is not NULL. Why would anyone call
> > this API with 'base' and 'offset' set to NULL?
> 
> Some time, they may just want one of two.
> 

What would be the purpose? I fail to see it.

> >
> > > + * @off: return offset, nothing return if NULL
> > > + *
> > > + * Helper function to convert input 'addr' to base and offset, which match
> > > + * BAR's alignment requirement.
> > > + *
> > > + * The pci_epf_alloc_space() function already accounts for alignment. This is
> > > + * primarily intended for use with other memory regions not allocated by
> > > + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger
> > > + * address for a platform MSI controller.
> > > + */
> > > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> > > +			       u64 addr, u64 *base, size_t *off)
> > > +{
> > > +	const struct pci_epc_features *epc_features;
> > > +	u64 align;
> > > +
> > > +	epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
> > > +	if (!epc_features) {
> > > +		dev_err(&epf->dev, "epc_features not implemented\n");
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	align = epc_features->align;
> > > +	align = align ? align : 128;
> > > +	if (epc_features->bar[bar].type == BAR_FIXED)
> > > +		align = max(epc_features->bar[bar].fixed_size, align);
> > > +
> > > +	if (base)
> > > +		*base = round_down(addr, align);
> > > +
> > > +	if (off)
> > > +		*off = addr & (align - 1);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_epf_align_inbound_addr);
> > > +
> > >  static void pci_epf_dev_release(struct device *dev)
> > >  {
> > >  	struct pci_epf *epf = to_pci_epf(dev);
> > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > index 5374e6515ffa0..eff73ccb5e702 100644
> > > --- a/include/linux/pci-epf.h
> > > +++ b/include/linux/pci-epf.h
> > > @@ -238,6 +238,20 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> > >  			  enum pci_epc_interface_type type);
> > >  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> > >  			enum pci_epc_interface_type type);
> > > +
> > > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> > > +			       u64 addr, u64 *base, size_t *off);
> > > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> > > +						   u32 low, u32 high, u64 *base, size_t *off)
> >
> > Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller
> > could pass u64 address directly.
> 
> 
> msi message sperate low32 and high32.  (h << 32 | low) is quite easy to
> cause build warning.  it should be ((u64) h << 32) | low. Avoid copy this
> logic code at many EPF places.
> 

There is absolutely no overhead in doing so. Also the concern for me is,
pci_epf_align_inbound_addr() is exported but only used within
pci_epf_align_inbound_addr_lo_hi(). This causes confusion. So I'd prefer to have
a single exported API that is used by the callers.

- Mani

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

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Manivannan Sadhasivam]

On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> >
> > I don't see 'doorbell_done' defined anywhere.
> >
> > > doorbell address space.
> > >
> > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > > address space.
> > >
> > > Set doorbell_done in the doorbell callback to indicate completion.
> > >
> >
> > Same here.
> >
> > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> >
> > 'avoid breaking compatibility between host and endpoint,...'
> >
> > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > > to map one bar's inbound address to MSI space. the command
> > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> > >
> > > 	 	Host side new driver	Host side old driver
> > >
> > > EP: new driver      S				F
> > > EP: old driver      F				F
> >
> > So the last case of old EP and host drivers will fail?
> 
> doorbell test will fail if old EP.
> 

How come there would be doorbell test if it is an old host driver?

> >
> > >
> > > S: If EP side support MSI, 'pcitest -B' return success.
> > >    If EP side doesn't support MSI, the same to 'F'.
> > >
> > > F: 'pcitest -B' return failure, other case as usual.
> > >
> > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > Change from v7 to v8
> > > - rename to pci_epf_align_inbound_addr_lo_hi()
> > >
> > > Change from v6 to v7
> > > - use help function pci_epf_align_addr_lo_hi()
> > >
> > > Change from v5 to v6
> > > - rename doorbell_addr to doorbell_offset
> > >
> > > Chagne from v4 to v5
> > > - Add doorbell free at unbind function.
> > > - Move msi irq handler to here to more complex user case, such as differece
> > > doorbell can use difference handler function.
> > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > > tag or co-developer tag, please let me know.
> > >
> > > change from v3 to v4
> > > - remove revid requirement
> > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > > COMMAND_ENABLE_DOORBELL.
> > > ---
> > >  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> > >  1 file changed, 117 insertions(+)
> > >
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > index ef6677f34116e..410b2f4bb7ce7 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > @@ -11,12 +11,14 @@
> > >  #include <linux/dmaengine.h>
> > >  #include <linux/io.h>
> > >  #include <linux/module.h>
> > > +#include <linux/msi.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/pci_ids.h>
> > >  #include <linux/random.h>
> > >
> > >  #include <linux/pci-epc.h>
> > >  #include <linux/pci-epf.h>
> > > +#include <linux/pci-ep-msi.h>
> > >  #include <linux/pci_regs.h>
> > >
> > >  #define IRQ_TYPE_INTX			0
> > > @@ -29,6 +31,8 @@
> > >  #define COMMAND_READ			BIT(3)
> > >  #define COMMAND_WRITE			BIT(4)
> > >  #define COMMAND_COPY			BIT(5)
> > > +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> > > +#define COMMAND_DISABLE_DOORBELL	BIT(7)
> > >
> > >  #define STATUS_READ_SUCCESS		BIT(0)
> > >  #define STATUS_READ_FAIL		BIT(1)
> > > @@ -39,6 +43,11 @@
> > >  #define STATUS_IRQ_RAISED		BIT(6)
> > >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> > >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> > > +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> > > +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > > +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
> > >
> > >  #define FLAG_USE_DMA			BIT(0)
> > >
> > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> > >  	u32	irq_type;
> > >  	u32	irq_number;
> > >  	u32	flags;
> > > +	u32	doorbell_bar;
> > > +	u32	doorbell_offset;
> > > +	u32	doorbell_data;
> > >  } __packed;
> > >
> > >  static struct pci_epf_header test_header = {
> > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> > >  	}
> > >  }
> > >
> > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > > +{
> > > +	enum pci_barno bar = reg->doorbell_bar;
> > > +	struct pci_epf *epf = epf_test->epf;
> > > +	struct pci_epc *epc = epf->epc;
> > > +	struct pci_epf_bar db_bar;
> >
> > db_bar = {};
> >
> > > +	struct msi_msg *msg;
> > > +	size_t offset;
> > > +	int ret;
> > > +
> > > +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> >
> > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> > allocated proper BAR already.
> 
> Not check it at call pci_epf_alloc_doorbell() because it optional feature.

What is 'optional feature' here? allocating doorbell?

> It return failure when it actually use it.
> 

So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()?

> >
> > > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > > +		return;
> > > +	}
> > > +
> > > +	msg = &epf->db_msg[0].msg;
> > > +	ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
> > > +					       &db_bar.phys_addr, &offset);
> > > +
> > > +	if (ret) {
> > > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > > +		return;
> > > +	}
> > > +
> > > +	reg->doorbell_offset = offset;
> > > +
> > > +	db_bar.barno = bar;
> > > +	db_bar.size = epf->bar[bar].size;
> > > +	db_bar.flags = epf->bar[bar].flags;
> > > +	db_bar.addr = NULL;
> >
> > Not needed if you initialize above.
> >
> > > +
> > > +	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar);
> > > +	if (!ret)
> > > +		reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS;
> > > +	else
> > > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > > +}
> > > +
> >
> > [...]
> >
> > >  static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> > >  	.epc_init = pci_epf_test_epc_init,
> > >  	.epc_deinit = pci_epf_test_epc_deinit,
> > > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> > >  	if (ret)
> > >  		return ret;
> > >
> > > +	ret = pci_epf_alloc_doorbell(epf, 1);
> > > +	if (!ret) {
> > > +		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> > > +		struct msi_msg *msg = &epf->db_msg[0].msg;
> > > +		enum pci_barno bar;
> > > +
> > > +		bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
> >
> > NO_BAR check?
> 
> This is optional feature, It should check when use it.
> 

NO. Why would you call request_irq() if the doorbell BAR is not available? It
doesn't make sense.

- Mani

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

Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper for address alignment

[Niklas Cassel]

On Tue, Nov 26, 2024 at 09:49:03AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> > > > Introduce the helper function pci_epf_align_addr() to adjust addresses
> > >
> > > pci_epf_align_inbound_addr()?
> > >
> > > > according to PCI BAR alignment requirements, converting addresses into base
> > > > and offset values.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > change from v7 to v8
> > > > - change name to pci_epf_align_inbound_addr()
> > > > - update comment said only need for memory, which not allocated by
> > > > pci_epf_alloc_space().
> > > >
> > > > change from v6 to v7
> > > > - new patch
> > > > ---
> > > >  drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
> > > >  include/linux/pci-epf.h             | 14 ++++++++++++
> > > >  2 files changed, 59 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > > index 8fa2797d4169a..4dfc218ebe20b 100644
> > > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pci_epf_create);
> > > >
> > > > +/**
> > > > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's
> > >
> > > BAR's
> > >
> > > > + *			  alignment requirement
> > > > + * @epf: the EPF device
> > > > + * @addr: the address of the memory
> > > > + * @bar: the BAR number corresponding to map addr
> > > > + * @base: return base address, which match BAR's alignment requirement, nothing
> > > > + *	  return if NULL
> > >
> > > Below, you are updating 'base' only if it is not NULL. Why would anyone call
> > > this API with 'base' and 'offset' set to NULL?
> > 
> > Some time, they may just want one of two.
> > 
> 
> What would be the purpose? I fail to see it.

Currently, the only user of this function is the call:
ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
				       &db_bar.phys_addr, &offset);

Which doesn't send in NULL as either 'base' or 'offset', so these NULL
checks do currently look meaningless to me. I suggest to just kill them.


Kind regards,
Niklas

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> > >
> > > I don't see 'doorbell_done' defined anywhere.
> > >
> > > > doorbell address space.
> > > >
> > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > > > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > > > address space.
> > > >
> > > > Set doorbell_done in the doorbell callback to indicate completion.
> > > >
> > >
> > > Same here.
> > >
> > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> > >
> > > 'avoid breaking compatibility between host and endpoint,...'
> > >
> > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > > > to map one bar's inbound address to MSI space. the command
> > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> > > >
> > > > 	 	Host side new driver	Host side old driver
> > > >
> > > > EP: new driver      S				F
> > > > EP: old driver      F				F
> > >
> > > So the last case of old EP and host drivers will fail?
> > 
> > doorbell test will fail if old EP.
> > 
> 
> How come there would be doorbell test if it is an old host driver?

I also don't understand this.

The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE
can only be sent if there is a new host driver.

Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously
return "Invalid command" if the EP driver is old.

If EP driver is new, DOORBELL_ENABLE will only return success if the SoC
has support for GIC ITS and has configured DTS with msi-parent
(i.e. if the pci_epf_alloc_doorbell() call was successful).


> 
> > >
> > > >
> > > > S: If EP side support MSI, 'pcitest -B' return success.
> > > >    If EP side doesn't support MSI, the same to 'F'.
> > > >
> > > > F: 'pcitest -B' return failure, other case as usual.
> > > >
> > > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > Change from v7 to v8
> > > > - rename to pci_epf_align_inbound_addr_lo_hi()
> > > >
> > > > Change from v6 to v7
> > > > - use help function pci_epf_align_addr_lo_hi()
> > > >
> > > > Change from v5 to v6
> > > > - rename doorbell_addr to doorbell_offset
> > > >
> > > > Chagne from v4 to v5
> > > > - Add doorbell free at unbind function.
> > > > - Move msi irq handler to here to more complex user case, such as differece
> > > > doorbell can use difference handler function.
> > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > > > tag or co-developer tag, please let me know.
> > > >
> > > > change from v3 to v4
> > > > - remove revid requirement
> > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > > > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > > > COMMAND_ENABLE_DOORBELL.
> > > > ---
> > > >  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> > > >  1 file changed, 117 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > index ef6677f34116e..410b2f4bb7ce7 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > @@ -11,12 +11,14 @@
> > > >  #include <linux/dmaengine.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/msi.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/pci_ids.h>
> > > >  #include <linux/random.h>
> > > >
> > > >  #include <linux/pci-epc.h>
> > > >  #include <linux/pci-epf.h>
> > > > +#include <linux/pci-ep-msi.h>
> > > >  #include <linux/pci_regs.h>
> > > >
> > > >  #define IRQ_TYPE_INTX			0
> > > > @@ -29,6 +31,8 @@
> > > >  #define COMMAND_READ			BIT(3)
> > > >  #define COMMAND_WRITE			BIT(4)
> > > >  #define COMMAND_COPY			BIT(5)
> > > > +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> > > > +#define COMMAND_DISABLE_DOORBELL	BIT(7)
> > > >
> > > >  #define STATUS_READ_SUCCESS		BIT(0)
> > > >  #define STATUS_READ_FAIL		BIT(1)
> > > > @@ -39,6 +43,11 @@
> > > >  #define STATUS_IRQ_RAISED		BIT(6)
> > > >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> > > >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > > > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> > > > +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > > > +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
> > > >
> > > >  #define FLAG_USE_DMA			BIT(0)
> > > >
> > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> > > >  	u32	irq_type;
> > > >  	u32	irq_number;
> > > >  	u32	flags;
> > > > +	u32	doorbell_bar;
> > > > +	u32	doorbell_offset;
> > > > +	u32	doorbell_data;
> > > >  } __packed;
> > > >
> > > >  static struct pci_epf_header test_header = {
> > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> > > >  	}
> > > >  }
> > > >
> > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > > > +{
> > > > +	enum pci_barno bar = reg->doorbell_bar;
> > > > +	struct pci_epf *epf = epf_test->epf;
> > > > +	struct pci_epc *epc = epf->epc;
> > > > +	struct pci_epf_bar db_bar;
> > >
> > > db_bar = {};
> > >
> > > > +	struct msi_msg *msg;
> > > > +	size_t offset;
> > > > +	int ret;
> > > > +
> > > > +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> > >
> > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> > > allocated proper BAR already.
> > 
> > Not check it at call pci_epf_alloc_doorbell() because it optional feature.
> 
> What is 'optional feature' here? allocating doorbell?
> 
> > It return failure when it actually use it.
> > 
> 
> So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()?

This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at
.bind() time.

DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver
could theoretically send these even if pci_epf_alloc_doorbell() failed.


pci_epf_test_cmd_handler() additions looks like this:

+	case COMMAND_ENABLE_DOORBELL:
+		pci_epf_enable_doorbell(epf_test, reg);
+		pci_epf_test_raise_irq(epf_test, reg);
+		break;
+	case COMMAND_DISABLE_DOORBELL:
+		pci_epf_disable_doorbell(epf_test, reg);
+		pci_epf_test_raise_irq(epf_test, reg);
+		break;

so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell()
unconditionally, without any check to see if the doorbell was allocated.

We could move the was doorbell allocated check (if (!epf->db_msg)) to
pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler()
more messy, so personally I think it is fine to keep the doorbell allocated
check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell().


I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call
to pci_epf_enable_doorbell():
https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/

His reply is here::
https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/

"it may be too frequent to allocate and free msi resources when call
pci_epf_enable_doorbell()/pci_epf_disable_doorbell()."

I don't think that is a good argument, as presumably (in the normal case) an
EPF driver will enable doorbell in the beginning, and then keep it enabled.

However, one point could be that pci-epf-test currently does all allocations
(the allocations for the backing memory) in .bind(), so in one way it makes
sense to also allocate the doorbell in .bind().

To play devil's advocate, I guess you could argue that doorbell feature is
optional, while allocating backing memory for BARs is not, so it makes sense
that they are not allocated at the same time.

I guess it is up to the EPF maintainer to decide what he thinks is best :)


Kind regards,
Niklas

Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper for address alignment

[Niklas Cassel]

On Tue, Nov 26, 2024 at 09:49:03AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> > > > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> > > > +						   u32 low, u32 high, u64 *base, size_t *off)
> > >
> > > Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller
> > > could pass u64 address directly.
> > 
> > 
> > msi message sperate low32 and high32.  (h << 32 | low) is quite easy to
> > cause build warning.  it should be ((u64) h << 32) | low. Avoid copy this
> > logic code at many EPF places.
> > 
> 
> There is absolutely no overhead in doing so. Also the concern for me is,
> pci_epf_align_inbound_addr() is exported but only used within
> pci_epf_align_inbound_addr_lo_hi(). This causes confusion. So I'd prefer to have
> a single exported API that is used by the callers.

Yes, other EPF drivers will need to copy the line:
pci_epf_align_inbound_addr(..., ((u64) h << 32) | low, ...)
instead of:
pci_epf_align_inbound_addr_lo_hi(..., low, high, ...)

which I think is fine to be honest.

Probably simplest thing is just to kill
pci_epf_align_inbound_addr_lo_hi().


Kind regards,
Niklas

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Manivannan Sadhasivam]

On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote:
> On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> > > >
> > > > I don't see 'doorbell_done' defined anywhere.
> > > >
> > > > > doorbell address space.
> > > > >
> > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > > > > address space.
> > > > >
> > > > > Set doorbell_done in the doorbell callback to indicate completion.
> > > > >
> > > >
> > > > Same here.
> > > >
> > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> > > >
> > > > 'avoid breaking compatibility between host and endpoint,...'
> > > >
> > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > > > > to map one bar's inbound address to MSI space. the command
> > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> > > > >
> > > > > 	 	Host side new driver	Host side old driver
> > > > >
> > > > > EP: new driver      S				F
> > > > > EP: old driver      F				F
> > > >
> > > > So the last case of old EP and host drivers will fail?
> > > 
> > > doorbell test will fail if old EP.
> > > 
> > 
> > How come there would be doorbell test if it is an old host driver?
> 
> I also don't understand this.
> 
> The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE
> can only be sent if there is a new host driver.
> 
> Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously
> return "Invalid command" if the EP driver is old.
> 
> If EP driver is new, DOORBELL_ENABLE will only return success if the SoC
> has support for GIC ITS and has configured DTS with msi-parent
> (i.e. if the pci_epf_alloc_doorbell() call was successful).
> 
> 
> > 
> > > >
> > > > >
> > > > > S: If EP side support MSI, 'pcitest -B' return success.
> > > > >    If EP side doesn't support MSI, the same to 'F'.
> > > > >
> > > > > F: 'pcitest -B' return failure, other case as usual.
> > > > >
> > > > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > > Change from v7 to v8
> > > > > - rename to pci_epf_align_inbound_addr_lo_hi()
> > > > >
> > > > > Change from v6 to v7
> > > > > - use help function pci_epf_align_addr_lo_hi()
> > > > >
> > > > > Change from v5 to v6
> > > > > - rename doorbell_addr to doorbell_offset
> > > > >
> > > > > Chagne from v4 to v5
> > > > > - Add doorbell free at unbind function.
> > > > > - Move msi irq handler to here to more complex user case, such as differece
> > > > > doorbell can use difference handler function.
> > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > > > > tag or co-developer tag, please let me know.
> > > > >
> > > > > change from v3 to v4
> > > > > - remove revid requirement
> > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > > > > COMMAND_ENABLE_DOORBELL.
> > > > > ---
> > > > >  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> > > > >  1 file changed, 117 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > index ef6677f34116e..410b2f4bb7ce7 100644
> > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > @@ -11,12 +11,14 @@
> > > > >  #include <linux/dmaengine.h>
> > > > >  #include <linux/io.h>
> > > > >  #include <linux/module.h>
> > > > > +#include <linux/msi.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/pci_ids.h>
> > > > >  #include <linux/random.h>
> > > > >
> > > > >  #include <linux/pci-epc.h>
> > > > >  #include <linux/pci-epf.h>
> > > > > +#include <linux/pci-ep-msi.h>
> > > > >  #include <linux/pci_regs.h>
> > > > >
> > > > >  #define IRQ_TYPE_INTX			0
> > > > > @@ -29,6 +31,8 @@
> > > > >  #define COMMAND_READ			BIT(3)
> > > > >  #define COMMAND_WRITE			BIT(4)
> > > > >  #define COMMAND_COPY			BIT(5)
> > > > > +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> > > > > +#define COMMAND_DISABLE_DOORBELL	BIT(7)
> > > > >
> > > > >  #define STATUS_READ_SUCCESS		BIT(0)
> > > > >  #define STATUS_READ_FAIL		BIT(1)
> > > > > @@ -39,6 +43,11 @@
> > > > >  #define STATUS_IRQ_RAISED		BIT(6)
> > > > >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> > > > >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > > > > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> > > > > +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > > > > +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
> > > > >
> > > > >  #define FLAG_USE_DMA			BIT(0)
> > > > >
> > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> > > > >  	u32	irq_type;
> > > > >  	u32	irq_number;
> > > > >  	u32	flags;
> > > > > +	u32	doorbell_bar;
> > > > > +	u32	doorbell_offset;
> > > > > +	u32	doorbell_data;
> > > > >  } __packed;
> > > > >
> > > > >  static struct pci_epf_header test_header = {
> > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> > > > >  	}
> > > > >  }
> > > > >
> > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > > > > +{
> > > > > +	enum pci_barno bar = reg->doorbell_bar;
> > > > > +	struct pci_epf *epf = epf_test->epf;
> > > > > +	struct pci_epc *epc = epf->epc;
> > > > > +	struct pci_epf_bar db_bar;
> > > >
> > > > db_bar = {};
> > > >
> > > > > +	struct msi_msg *msg;
> > > > > +	size_t offset;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> > > >
> > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> > > > allocated proper BAR already.
> > > 
> > > Not check it at call pci_epf_alloc_doorbell() because it optional feature.
> > 
> > What is 'optional feature' here? allocating doorbell?
> > 
> > > It return failure when it actually use it.
> > > 
> > 
> > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()?
> 
> This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at
> .bind() time.
> 
> DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver
> could theoretically send these even if pci_epf_alloc_doorbell() failed.
> 
> 
> pci_epf_test_cmd_handler() additions looks like this:
> 
> +	case COMMAND_ENABLE_DOORBELL:
> +		pci_epf_enable_doorbell(epf_test, reg);
> +		pci_epf_test_raise_irq(epf_test, reg);
> +		break;
> +	case COMMAND_DISABLE_DOORBELL:
> +		pci_epf_disable_doorbell(epf_test, reg);
> +		pci_epf_test_raise_irq(epf_test, reg);
> +		break;
> 
> so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell()
> unconditionally, without any check to see if the doorbell was allocated.
> 
> We could move the was doorbell allocated check (if (!epf->db_msg)) to
> pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler()
> more messy, so personally I think it is fine to keep the doorbell allocated
> check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell().
> 
> 
> I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call
> to pci_epf_enable_doorbell():
> https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/
> 
> His reply is here::
> https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/
> 
> "it may be too frequent to allocate and free msi resources when call
> pci_epf_enable_doorbell()/pci_epf_disable_doorbell()."
> 
> I don't think that is a good argument, as presumably (in the normal case) an
> EPF driver will enable doorbell in the beginning, and then keep it enabled.
> 
> However, one point could be that pci-epf-test currently does all allocations
> (the allocations for the backing memory) in .bind(), so in one way it makes
> sense to also allocate the doorbell in .bind().
> 
> To play devil's advocate, I guess you could argue that doorbell feature is
> optional, while allocating backing memory for BARs is not, so it makes sense
> that they are not allocated at the same time.
> 

I like the idea of calling pci_epf_alloc_doorbell() in
pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense
to call these APIs too frequently.

- Mani

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

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> > >
> > > I don't see 'doorbell_done' defined anywhere.
> > >
> > > > doorbell address space.
> > > >
> > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > > > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > > > address space.
> > > >
> > > > Set doorbell_done in the doorbell callback to indicate completion.
> > > >
> > >
> > > Same here.
> > >
> > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> > >
> > > 'avoid breaking compatibility between host and endpoint,...'
> > >
> > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > > > to map one bar's inbound address to MSI space. the command
> > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> > > >
> > > > 	 	Host side new driver	Host side old driver
> > > >
> > > > EP: new driver      S				F
> > > > EP: old driver      F				F
> > >
> > > So the last case of old EP and host drivers will fail?
> >
> > doorbell test will fail if old EP.
> >
>
> How come there would be doorbell test if it is an old host driver?

fail by unsupport ioctrl(). It should "implicit default behavior".

Frank

>
> > >
> > > >
> > > > S: If EP side support MSI, 'pcitest -B' return success.
> > > >    If EP side doesn't support MSI, the same to 'F'.
> > > >
> > > > F: 'pcitest -B' return failure, other case as usual.
> > > >
> > > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > Change from v7 to v8
> > > > - rename to pci_epf_align_inbound_addr_lo_hi()
> > > >
> > > > Change from v6 to v7
> > > > - use help function pci_epf_align_addr_lo_hi()
> > > >
> > > > Change from v5 to v6
> > > > - rename doorbell_addr to doorbell_offset
> > > >
> > > > Chagne from v4 to v5
> > > > - Add doorbell free at unbind function.
> > > > - Move msi irq handler to here to more complex user case, such as differece
> > > > doorbell can use difference handler function.
> > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > > > tag or co-developer tag, please let me know.
> > > >
> > > > change from v3 to v4
> > > > - remove revid requirement
> > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > > > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > > > COMMAND_ENABLE_DOORBELL.
> > > > ---
> > > >  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> > > >  1 file changed, 117 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > index ef6677f34116e..410b2f4bb7ce7 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > @@ -11,12 +11,14 @@
> > > >  #include <linux/dmaengine.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/msi.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/pci_ids.h>
> > > >  #include <linux/random.h>
> > > >
> > > >  #include <linux/pci-epc.h>
> > > >  #include <linux/pci-epf.h>
> > > > +#include <linux/pci-ep-msi.h>
> > > >  #include <linux/pci_regs.h>
> > > >
> > > >  #define IRQ_TYPE_INTX			0
> > > > @@ -29,6 +31,8 @@
> > > >  #define COMMAND_READ			BIT(3)
> > > >  #define COMMAND_WRITE			BIT(4)
> > > >  #define COMMAND_COPY			BIT(5)
> > > > +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> > > > +#define COMMAND_DISABLE_DOORBELL	BIT(7)
> > > >
> > > >  #define STATUS_READ_SUCCESS		BIT(0)
> > > >  #define STATUS_READ_FAIL		BIT(1)
> > > > @@ -39,6 +43,11 @@
> > > >  #define STATUS_IRQ_RAISED		BIT(6)
> > > >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> > > >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > > > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> > > > +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > > > +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
> > > >
> > > >  #define FLAG_USE_DMA			BIT(0)
> > > >
> > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> > > >  	u32	irq_type;
> > > >  	u32	irq_number;
> > > >  	u32	flags;
> > > > +	u32	doorbell_bar;
> > > > +	u32	doorbell_offset;
> > > > +	u32	doorbell_data;
> > > >  } __packed;
> > > >
> > > >  static struct pci_epf_header test_header = {
> > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> > > >  	}
> > > >  }
> > > >
> > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > > > +{
> > > > +	enum pci_barno bar = reg->doorbell_bar;
> > > > +	struct pci_epf *epf = epf_test->epf;
> > > > +	struct pci_epc *epc = epf->epc;
> > > > +	struct pci_epf_bar db_bar;
> > >
> > > db_bar = {};
> > >
> > > > +	struct msi_msg *msg;
> > > > +	size_t offset;
> > > > +	int ret;
> > > > +
> > > > +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> > >
> > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> > > allocated proper BAR already.
> >
> > Not check it at call pci_epf_alloc_doorbell() because it optional feature.
>
> What is 'optional feature' here? allocating doorbell?

Not all platform support ITS. If hardware have not ITS support,
pci_epf_alloc_doorbell() will return fail.

>
> > It return failure when it actually use it.
> >
>
> So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()?

Suppose yes because host don't know if EP support doorbell.

>
> > >
> > > > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	msg = &epf->db_msg[0].msg;
> > > > +	ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
> > > > +					       &db_bar.phys_addr, &offset);
> > > > +
> > > > +	if (ret) {
> > > > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	reg->doorbell_offset = offset;
> > > > +
> > > > +	db_bar.barno = bar;
> > > > +	db_bar.size = epf->bar[bar].size;
> > > > +	db_bar.flags = epf->bar[bar].flags;
> > > > +	db_bar.addr = NULL;
> > >
> > > Not needed if you initialize above.
> > >
> > > > +
> > > > +	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar);
> > > > +	if (!ret)
> > > > +		reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS;
> > > > +	else
> > > > +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> > > > +}
> > > > +
> > >
> > > [...]
> > >
> > > >  static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> > > >  	.epc_init = pci_epf_test_epc_init,
> > > >  	.epc_deinit = pci_epf_test_epc_deinit,
> > > > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > > +	ret = pci_epf_alloc_doorbell(epf, 1);
> > > > +	if (!ret) {
> > > > +		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> > > > +		struct msi_msg *msg = &epf->db_msg[0].msg;
> > > > +		enum pci_barno bar;
> > > > +
> > > > +		bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
> > >
> > > NO_BAR check?
> >
> > This is optional feature, It should check when use it.
> >
>
> NO. Why would you call request_irq() if the doorbell BAR is not available? It
> doesn't make sense.

Maybe reasonable now. But it will be not true if we support map two
seperate memory regions to a bar in future.  Anyway I can add check here.

Frank

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

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

On Tue, Nov 26, 2024 at 06:11:12PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote:
> > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> > > > >
> > > > > I don't see 'doorbell_done' defined anywhere.
> > > > >
> > > > > > doorbell address space.
> > > > > >
> > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > > > > > address space.
> > > > > >
> > > > > > Set doorbell_done in the doorbell callback to indicate completion.
> > > > > >
> > > > >
> > > > > Same here.
> > > > >
> > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> > > > >
> > > > > 'avoid breaking compatibility between host and endpoint,...'
> > > > >
> > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > > > > > to map one bar's inbound address to MSI space. the command
> > > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> > > > > >
> > > > > > 	 	Host side new driver	Host side old driver
> > > > > >
> > > > > > EP: new driver      S				F
> > > > > > EP: old driver      F				F
> > > > >
> > > > > So the last case of old EP and host drivers will fail?
> > > >
> > > > doorbell test will fail if old EP.
> > > >
> > >
> > > How come there would be doorbell test if it is an old host driver?
> >
> > I also don't understand this.
> >
> > The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE
> > can only be sent if there is a new host driver.
> >
> > Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously
> > return "Invalid command" if the EP driver is old.
> >
> > If EP driver is new, DOORBELL_ENABLE will only return success if the SoC
> > has support for GIC ITS and has configured DTS with msi-parent
> > (i.e. if the pci_epf_alloc_doorbell() call was successful).
> >
> >
> > >
> > > > >
> > > > > >
> > > > > > S: If EP side support MSI, 'pcitest -B' return success.
> > > > > >    If EP side doesn't support MSI, the same to 'F'.
> > > > > >
> > > > > > F: 'pcitest -B' return failure, other case as usual.
> > > > > >
> > > > > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > ---
> > > > > > Change from v7 to v8
> > > > > > - rename to pci_epf_align_inbound_addr_lo_hi()
> > > > > >
> > > > > > Change from v6 to v7
> > > > > > - use help function pci_epf_align_addr_lo_hi()
> > > > > >
> > > > > > Change from v5 to v6
> > > > > > - rename doorbell_addr to doorbell_offset
> > > > > >
> > > > > > Chagne from v4 to v5
> > > > > > - Add doorbell free at unbind function.
> > > > > > - Move msi irq handler to here to more complex user case, such as differece
> > > > > > doorbell can use difference handler function.
> > > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > > > > > tag or co-developer tag, please let me know.
> > > > > >
> > > > > > change from v3 to v4
> > > > > > - remove revid requirement
> > > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > > > > > COMMAND_ENABLE_DOORBELL.
> > > > > > ---
> > > > > >  drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> > > > > >  1 file changed, 117 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > > index ef6677f34116e..410b2f4bb7ce7 100644
> > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > > @@ -11,12 +11,14 @@
> > > > > >  #include <linux/dmaengine.h>
> > > > > >  #include <linux/io.h>
> > > > > >  #include <linux/module.h>
> > > > > > +#include <linux/msi.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/pci_ids.h>
> > > > > >  #include <linux/random.h>
> > > > > >
> > > > > >  #include <linux/pci-epc.h>
> > > > > >  #include <linux/pci-epf.h>
> > > > > > +#include <linux/pci-ep-msi.h>
> > > > > >  #include <linux/pci_regs.h>
> > > > > >
> > > > > >  #define IRQ_TYPE_INTX			0
> > > > > > @@ -29,6 +31,8 @@
> > > > > >  #define COMMAND_READ			BIT(3)
> > > > > >  #define COMMAND_WRITE			BIT(4)
> > > > > >  #define COMMAND_COPY			BIT(5)
> > > > > > +#define COMMAND_ENABLE_DOORBELL		BIT(6)
> > > > > > +#define COMMAND_DISABLE_DOORBELL	BIT(7)
> > > > > >
> > > > > >  #define STATUS_READ_SUCCESS		BIT(0)
> > > > > >  #define STATUS_READ_FAIL		BIT(1)
> > > > > > @@ -39,6 +43,11 @@
> > > > > >  #define STATUS_IRQ_RAISED		BIT(6)
> > > > > >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> > > > > >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > > > > > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> > > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS	BIT(10)
> > > > > > +#define STATUS_DOORBELL_ENABLE_FAIL	BIT(11)
> > > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > > > > > +#define STATUS_DOORBELL_DISABLE_FAIL	BIT(13)
> > > > > >
> > > > > >  #define FLAG_USE_DMA			BIT(0)
> > > > > >
> > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> > > > > >  	u32	irq_type;
> > > > > >  	u32	irq_number;
> > > > > >  	u32	flags;
> > > > > > +	u32	doorbell_bar;
> > > > > > +	u32	doorbell_offset;
> > > > > > +	u32	doorbell_data;
> > > > > >  } __packed;
> > > > > >
> > > > > >  static struct pci_epf_header test_header = {
> > > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> > > > > >  	}
> > > > > >  }
> > > > > >
> > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > > > > > +{
> > > > > > +	enum pci_barno bar = reg->doorbell_bar;
> > > > > > +	struct pci_epf *epf = epf_test->epf;
> > > > > > +	struct pci_epc *epc = epf->epc;
> > > > > > +	struct pci_epf_bar db_bar;
> > > > >
> > > > > db_bar = {};
> > > > >
> > > > > > +	struct msi_msg *msg;
> > > > > > +	size_t offset;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> > > > >
> > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> > > > > allocated proper BAR already.
> > > >
> > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature.
> > >
> > > What is 'optional feature' here? allocating doorbell?
> > >
> > > > It return failure when it actually use it.
> > > >
> > >
> > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()?
> >
> > This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at
> > .bind() time.
> >
> > DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver
> > could theoretically send these even if pci_epf_alloc_doorbell() failed.
> >
> >
> > pci_epf_test_cmd_handler() additions looks like this:
> >
> > +	case COMMAND_ENABLE_DOORBELL:
> > +		pci_epf_enable_doorbell(epf_test, reg);
> > +		pci_epf_test_raise_irq(epf_test, reg);
> > +		break;
> > +	case COMMAND_DISABLE_DOORBELL:
> > +		pci_epf_disable_doorbell(epf_test, reg);
> > +		pci_epf_test_raise_irq(epf_test, reg);
> > +		break;
> >
> > so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell()
> > unconditionally, without any check to see if the doorbell was allocated.
> >
> > We could move the was doorbell allocated check (if (!epf->db_msg)) to
> > pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler()
> > more messy, so personally I think it is fine to keep the doorbell allocated
> > check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell().
> >
> >
> > I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call
> > to pci_epf_enable_doorbell():
> > https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/
> >
> > His reply is here::
> > https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/
> >
> > "it may be too frequent to allocate and free msi resources when call
> > pci_epf_enable_doorbell()/pci_epf_disable_doorbell()."
> >
> > I don't think that is a good argument, as presumably (in the normal case) an
> > EPF driver will enable doorbell in the beginning, and then keep it enabled.
> >
> > However, one point could be that pci-epf-test currently does all allocations
> > (the allocations for the backing memory) in .bind(), so in one way it makes
> > sense to also allocate the doorbell in .bind().
> >
> > To play devil's advocate, I guess you could argue that doorbell feature is
> > optional, while allocating backing memory for BARs is not, so it makes sense
> > that they are not allocated at the same time.
> >
>
> I like the idea of calling pci_epf_alloc_doorbell() in
> pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense
> to call these APIs too frequently.

I not sure what's you means and direction for next version.

Ideally, host driver should use doorbell for every command because it will
avoid EP's side polling reg change.

It still is problem to waste a bar to do doorbell. Ideally, we can append
doorbell ITS register after test bar, which require map two segmemt memory
region to bar0.

This patch just go first step. If we can append to ITS to bar0 in future,
pci_epf_alloc_doorbell() will become more reasonable at bind() function.

Frank

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

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Tue, Nov 26, 2024 at 09:44:49AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 01:03:37PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 12:41:00PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote:
> > > > Doorbell feature is implemented by mapping the EP's MSI interrupt
> > > > controller message address to a dedicated BAR in the EPC core. It is the
> > > > responsibility of the EPF driver to pass the actual message data to be
> > > > written by the host to the doorbell BAR region through its own logic.
> > > >
> > > > Tested-by: Niklas Cassel <cassel@kernel.org>
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > change from v5 to v8
> > > > -none
> > > >
> > > > Change from v4 to v5
> > > > - Remove request_irq() in pci_epc_alloc_doorbell() and leave to EP function
> > > > driver, so ep function driver can register differece call back function for
> > > > difference doorbell events and set irq affinity to differece CPU core.
> > > > - Improve error message when MSI allocate failure.
> > > >
> > > > Change from v3 to v4
> > > > - msi change to use msi_get_virq() avoid use msi_for_each_desc().
> > > > - add new struct for pci_epf_doorbell_msg to msi msg,virq and irq name.
> > > > - move mutex lock to epc function
> > > > - initialize variable at declear place.
> > > > - passdown epf to epc*() function to simplify code.
> > > > ---
> > > >  drivers/pci/endpoint/Makefile     |  2 +-
> > > >  drivers/pci/endpoint/pci-ep-msi.c | 99 +++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/pci-ep-msi.h        | 15 ++++++
> > > >  include/linux/pci-epf.h           | 16 +++++++
> > > >  4 files changed, 131 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> > > > index 95b2fe47e3b06..a1ccce440c2c5 100644
> > > > --- a/drivers/pci/endpoint/Makefile
> > > > +++ b/drivers/pci/endpoint/Makefile
> > > > @@ -5,4 +5,4 @@
> > > >
> > > >  obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
> > > >  obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
> > > > -					   pci-epc-mem.o functions/
> > > > +					   pci-epc-mem.o pci-ep-msi.o functions/
> > > > diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> > > > new file mode 100644
> > > > index 0000000000000..7868a529dce37
> > > > --- /dev/null
> > > > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > > > @@ -0,0 +1,99 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * PCI Endpoint *Controller* (EPC) MSI library
> > > > + *
> > > > + * Copyright (C) 2024 NXP
> > > > + * Author: Frank Li <Frank.Li@nxp.com>
> > > > + */
> > > > +
> > > > +#include <linux/cleanup.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/slab.h>
> > >
> > > Please sort alphabetically.
> > >
> > > > +#include <linux/module.h>
> > > > +#include <linux/msi.h>
> > > > +#include <linux/pci-epc.h>
> > > > +#include <linux/pci-epf.h>
> > > > +#include <linux/pci-ep-cfs.h>
> > > > +#include <linux/pci-ep-msi.h>
> > > > +
> > > > +static bool pci_epc_match_parent(struct device *dev, void *param)
> > > > +{
> > > > +	return dev->parent == param;
> > > > +}
> > > > +
> > > > +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > +{
> > > > +	struct pci_epc *epc __free(pci_epc_put) = NULL;
> > > > +	struct pci_epf *epf;
> > > > +
> > > > +	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
> > >
> > > You were passing 'epc->dev.parent' to platform_device_msi_init_and_alloc_irqs().
> > > So 'desc->dev' should be the EPC parent, right? If so, you can do:
> > >
> > > 	epc = pci_epc_get(dev_name(msi_desc_to_dev(desc)));
> > >
> > > since we are reusing the parent dev name for EPC.
> >
> > I think it is not good to depend on hidden situation, "name is the same."
> > May it change in future because no one will realize here depend on the same
> > name and just think it is trivial update for device name.
> >
>
> No one should change the EPC name just like that. The name is exposed to
> configfs interface and the existing userspace scripts rely on that. So changing
> the name will break them.
>
> I'd strongly suggest you to use the existing API instead of adding a new one for
> the same purpose.
>
> > >
> > > > +	if (!epc)
> > > > +		return;
> > > > +
> > > > +	/* Only support one EPF for doorbell */
> > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > >
> > > Why don't you impose this restriction in pci_epf_alloc_doorbell() itself?
> >
> > This is callback from platform_device_msi_init_and_alloc_irqs(). So it is
> > actually restriction at pci_epf_alloc_doorbell().
> >
>
> I don't know how to parse this last sentence. But my question was why don't you
> impose this one EPF restriction in pci_epf_alloc_doorbell() before allocating
> the MSI domain using platform_device_msi_init_and_alloc_irqs()? This way if
> someone calls pci_epf_alloc_doorbell() multi EPF, it will fail.

Yes, it is limitation for current platfrom msi framework. It is totally
equal.

call stack is
	pci_epf_alloc_doorbell()
	     platform_device_msi_init_and_alloc_irqs()
		...
		pci_epc_write_msi_msg()



It is totally equal return at pci_epc_write_msi_msg() or return before
platform_device_msi_init_and_alloc_irqs().

why check epf in pci_epc_write_msi_msg() is because it use epf in here.
pci_epf_alloc_doorbell() never use epf variable. If check unused variable
in pci_epf_alloc_doorbell(), user don't know why and what's exactly
restrict it.

Frank

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

Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Tue, Nov 26, 2024 at 12:19:01PM -0500, Frank Li wrote:

[...]

> > > > > +	/* Only support one EPF for doorbell */
> > > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > >
> > > > Why don't you impose this restriction in pci_epf_alloc_doorbell() itself?
> > >
> > > This is callback from platform_device_msi_init_and_alloc_irqs(). So it is
> > > actually restriction at pci_epf_alloc_doorbell().
> > >
> >
> > I don't know how to parse this last sentence. But my question was why don't you
> > impose this one EPF restriction in pci_epf_alloc_doorbell() before allocating
> > the MSI domain using platform_device_msi_init_and_alloc_irqs()? This way if
> > someone calls pci_epf_alloc_doorbell() multi EPF, it will fail.
> 
> Yes, it is limitation for current platfrom msi framework. It is totally
> equal.
> 
> call stack is
> 	pci_epf_alloc_doorbell()
> 	     platform_device_msi_init_and_alloc_irqs()
> 		...
> 		pci_epc_write_msi_msg()
> 
> 
> 
> It is totally equal return at pci_epc_write_msi_msg() or return before
> platform_device_msi_init_and_alloc_irqs().
> 

No, these two are not the same. pci_epc_write_msi_msg() will only be called
when enabling the MSI, which is too late IMO. Why are you insisting in
calling platform_device_msi_init_and_alloc_irqs() for multi EPF? I don't quite
understand.

If the platform cannot support it, then it should not be called in first place.

> why check epf in pci_epc_write_msi_msg() is because it use epf in here.
> pci_epf_alloc_doorbell() never use epf variable. If check unused variable
> in pci_epf_alloc_doorbell(), user don't know why and what's exactly
> restrict it.
> 

This is not a good argument and doesn't make sense, sorry. You should not call
platform_device_msi_init_and_alloc_irqs() for multi EPF.

- Mani

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

Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

On Tue, Nov 26, 2024 at 11:55:13AM -0500, Frank Li wrote:
> On Tue, Nov 26, 2024 at 06:11:12PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote:
> > > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> > > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> >
> > I like the idea of calling pci_epf_alloc_doorbell() in
> > pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense
> > to call these APIs too frequently.
> 
> I not sure what's you means and direction for next version.

Move pci_epf_alloc_doorbell() from .bind() to pci_epf_enable_doorbell().
Move pci_epf_free_doorbell() from .unbind() to pci_epf_disable_doorbell().

If the pci_epf_alloc_doorbell() call within pci_epf_enable_doorbell() fails,
let pci_epf_enable_doorbell() set STATUS_DOORBELL_ENABLE_FAIL.


> This patch just go first step. If we can append to ITS to bar0 in future,
> pci_epf_alloc_doorbell() will become more reasonable at bind() function.

To be fair, we are probably quite far away from supporting a BAR with two
backing memory areas. It would require a lot of changes in the PCI endpoint
framework, and a lot of changes in the DWC driver.

And even if we do add all the support for it, why can't we keep the
doorbell allocation in pci_epf_enable_doorbell() ?


Kind regards,
Niklas