[PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[PATCH 0/3] 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. 

The key point is comments from Thomas Gleixner, who suggest use new
PCI/IMS. But arm platform change still not be merged yet.

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm

So I still use existed method implement RC to EP doorbell.

If Thomas Gleixner want to continue work on devmsi-v2-arm, I can help test
and update this patch.

Frank Li (3):
  PCI: endpoint: Add RC-to-EP doorbell support using platform MSI
    controller
  misc: pci_endpoint_test: Add doorbell test case
  tools: PCI: Add 'B' option for test doorbell

 drivers/misc/pci_endpoint_test.c    |  41 +++++++++++
 drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
 include/linux/pci-epf.h             |  16 ++++
 include/uapi/linux/pcitest.h        |   1 +
 tools/pci/pcitest.c                 |  16 +++-
 5 files changed, 182 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

This commit introduces a common method for sending messages from the Root Complex
(RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
by the PCI host to the message address of the platform MSI interrupt controller
in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
an IRQ at the EP. This implementation serves as a common method for all endpoint
function drivers.

However, it currently supports only one EP physical function due to limitations
in ARM MSI/IMS readiness.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
 include/linux/pci-epf.h             |  16 ++++
 2 files changed, 125 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 355a6f56fcea..94ac82bf84c5 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -6,10 +6,12 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
+#include <linux/irqreturn.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/msi.h>
 
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
@@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 }
 EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
 
+static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
+{
+	struct pci_epf *epf = data;
+
+	if (epf->event_ops && epf->event_ops->doorbell)
+		epf->event_ops->doorbell(epf, irq - epf->virq_base);
+
+	return IRQ_HANDLED;
+}
+
+static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
+	struct pci_epf *epf;
+
+	/* Todo: Need check correct epf if multi epf supported */
+	list_for_each_entry(epf, &epc->pci_epf, list) {
+		if (epf->msg && desc->msi_index < epf->num_msgs)
+			epf->msg[desc->msi_index] = *msg;
+	}
+}
+
+int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
+{
+	struct irq_domain *domain;
+	struct pci_epc *epc;
+	struct device *dev;
+	int virq;
+	int ret;
+	int i;
+
+	epc = epf->epc;
+	dev = &epc->dev;
+
+	/*
+	 * Current only support 1 function.
+	 * PCI IMS(interrupt message store) ARM support have not been
+	 * ready yet.
+	 */
+	if (epc->function_num_map != 1)
+		return -EOPNOTSUPP;
+
+	domain = dev_get_msi_domain(dev->parent);
+	if (!domain)
+		return -EOPNOTSUPP;
+	dev_set_msi_domain(dev, domain);
+
+	/* use parent of_node to get device id information */
+	dev->of_node = dev->parent->of_node;
+
+	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
+	if (!epf->msg)
+		return -ENOMEM;
+
+	epf->num_msgs = num_msgs;
+
+	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
+	if (ret) {
+		dev_err(dev, "Can't allocate MSI from system MSI controller\n");
+		goto err_mem;
+	}
+
+	for (i = 0; i < num_msgs; i++) {
+		virq = msi_get_virq(dev, i);
+		if (i == 0)
+			epf->virq_base = virq;
+
+		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
+				  "pci-epf-doorbell", epf);
+
+		if (ret) {
+			dev_err(dev, "Failure request doorbell IRQ\n");
+			goto err_irq;
+		}
+	}
+
+	epf->num_msgs = num_msgs;
+	return ret;
+
+err_irq:
+	platform_msi_domain_free_irqs(dev);
+err_mem:
+	kfree(epf->msg);
+	epf->msg = NULL;
+	epf->num_msgs = 0;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
+
+void pci_epf_free_doorbell(struct pci_epf *epf)
+{
+	struct pci_epc *epc;
+	int i;
+
+	epc = epf->epc;
+
+	for (i = 0; i < epf->num_msgs; i++)
+		free_irq(epf->virq_base + i, epf);
+
+	platform_msi_domain_free_irqs(&epc->dev);
+	kfree(epf->msg);
+	epf->msg = NULL;
+	epf->num_msgs = 0;
+}
+EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
+
 static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
 {
 	struct config_group *group, *tmp;
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index b8441db2fa52..e187e3ee48d2 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -75,6 +75,7 @@ struct pci_epf_ops {
 struct pci_epc_event_ops {
 	int (*core_init)(struct pci_epf *epf);
 	int (*link_up)(struct pci_epf *epf);
+	int (*doorbell)(struct pci_epf *epf, int index);
 };
 
 /**
@@ -173,6 +174,9 @@ struct pci_epf {
 	unsigned long		vfunction_num_map;
 	struct list_head	pci_vepf;
 	const struct pci_epc_event_ops *event_ops;
+	struct msi_msg *msg;
+	u16 num_msgs;
+	int virq_base;
 };
 
 /**
@@ -216,4 +220,16 @@ 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);
 void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
+int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
+void pci_epf_free_doorbell(struct pci_epf *epf);
+
+static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
+{
+	return epf->msg;
+}
+
+static inline u16 epf_get_msg_num(struct pci_epf *epf)
+{
+	return epf->num_msgs;
+}
 #endif /* __LINUX_PCI_EPF_H */
-- 
2.34.1

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Wed, Apr 26, 2023 at 04:34:34PM -0400, Frank Li wrote:
> This commit introduces a common method for sending messages from the Root Complex
> (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
> such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
> by the PCI host to the message address of the platform MSI interrupt controller
> in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
> an IRQ at the EP. This implementation serves as a common method for all endpoint
> function drivers.
> 
> However, it currently supports only one EP physical function due to limitations
> in ARM MSI/IMS readiness.
> 

I've provided generic comments below, but I will do one more thorough review
after seeing epf-test driver patch.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
>  include/linux/pci-epf.h             |  16 ++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 355a6f56fcea..94ac82bf84c5 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -6,10 +6,12 @@
>   * Author: Kishon Vijay Abraham I <kishon@ti.com>
>   */
>  
> +#include <linux/irqreturn.h>

Why is this needed?

>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  }
>  EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
>  
> +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)

static irqreturn_t

s/pci_epf_interrupt_handler/pci_epf_doorbell_handler

> +{
> +	struct pci_epf *epf = data;
> +
> +	if (epf->event_ops && epf->event_ops->doorbell)
> +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> +	struct pci_epf *epf;
> +
> +	/* Todo: Need check correct epf if multi epf supported */
> +	list_for_each_entry(epf, &epc->pci_epf, list) {
> +		if (epf->msg && desc->msi_index < epf->num_msgs)
> +			epf->msg[desc->msi_index] = *msg;
> +	}
> +}
> +
> +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> +{
> +	struct irq_domain *domain;
> +	struct pci_epc *epc;
> +	struct device *dev;
> +	int virq;
> +	int ret;
> +	int i;
> +
> +	epc = epf->epc;
> +	dev = &epc->dev;

"epc_dev" to make it explicit

> +
> +	/*
> +	 * Current only support 1 function.

What does this mean exactly? Even a single EPC can support multiple EPFs

> +	 * PCI IMS(interrupt message store) ARM support have not been
> +	 * ready yet.

No need to mention platform irq controller name.

> +	 */
> +	if (epc->function_num_map != 1)

Why can't you use, epf->func_no?

> +		return -EOPNOTSUPP;
> +
> +	domain = dev_get_msi_domain(dev->parent);
> +	if (!domain)
> +		return -EOPNTSUPP;

Newline

> +	dev_set_msi_domain(dev, domain);
> +
> +	/* use parent of_node to get device id information */
> +	dev->of_node = dev->parent->of_node;
> +

Why do you need of_node assignment inside EPF core?

> +	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> +	if (!epf->msg)
> +		return -ENOMEM;
> +
> +	epf->num_msgs = num_msgs;
> +

Move this to the start of the function, after checks.

> +	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> +	if (ret) {
> +		dev_err(dev, "Can't allocate MSI from system MSI controller\n");

"Failed to allocate MSI"

> +		goto err_mem;

err_free_mem

> +	}
> +
> +	for (i = 0; i < num_msgs; i++) {
> +		virq = msi_get_virq(dev, i);
> +		if (i == 0)
> +			epf->virq_base = virq;
> +
> +		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
> +				  "pci-epf-doorbell", epf);

IRQ name should have an index, otherwise all of them will have the same name.

> +
> +		if (ret) {
> +			dev_err(dev, "Failure request doorbell IRQ\n");

"Failed to request doorbell"

> +			goto err_irq;

err_free_irq

> +		}
> +	}
> +
> +	epf->num_msgs = num_msgs;

Newline

> +	return ret;
> +
> +err_irq:
> +	platform_msi_domain_free_irqs(dev);
> +err_mem:
> +	kfree(epf->msg);
> +	epf->msg = NULL;
> +	epf->num_msgs = 0;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> +
> +void pci_epf_free_doorbell(struct pci_epf *epf)
> +{
> +	struct pci_epc *epc;
> +	int i;
> +
> +	epc = epf->epc;
> +
> +	for (i = 0; i < epf->num_msgs; i++)
> +		free_irq(epf->virq_base + i, epf);
> +
> +	platform_msi_domain_free_irqs(&epc->dev);
> +	kfree(epf->msg);
> +	epf->msg = NULL;
> +	epf->num_msgs = 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> +
>  static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
>  {
>  	struct config_group *group, *tmp;
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index b8441db2fa52..e187e3ee48d2 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -75,6 +75,7 @@ struct pci_epf_ops {
>  struct pci_epc_event_ops {
>  	int (*core_init)(struct pci_epf *epf);
>  	int (*link_up)(struct pci_epf *epf);
> +	int (*doorbell)(struct pci_epf *epf, int index);
>  };
>  
>  /**
> @@ -173,6 +174,9 @@ struct pci_epf {
>  	unsigned long		vfunction_num_map;
>  	struct list_head	pci_vepf;
>  	const struct pci_epc_event_ops *event_ops;
> +	struct msi_msg *msg;
> +	u16 num_msgs;
> +	int virq_base;
>  };
>  
>  /**
> @@ -216,4 +220,16 @@ 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);
>  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> +void pci_epf_free_doorbell(struct pci_epf *epf);
> +
> +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> +{
> +	return epf->msg;
> +}
> +
> +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> +{
> +	return epf->num_msgs;
> +}

I don't see a need for these two functions as they are doing just dereferences.

- Mani

>  #endif /* __LINUX_PCI_EPF_H */
> -- 
> 2.34.1
> 

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

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Sat, Sep 02, 2023 at 10:22:25AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 26, 2023 at 04:34:34PM -0400, Frank Li wrote:
> > This commit introduces a common method for sending messages from the Root Complex
> > (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
> > such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
> > by the PCI host to the message address of the platform MSI interrupt controller
> > in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
> > an IRQ at the EP. This implementation serves as a common method for all endpoint
> > function drivers.
> > 
> > However, it currently supports only one EP physical function due to limitations
> > in ARM MSI/IMS readiness.
> > 
> 
> I've provided generic comments below, but I will do one more thorough review
> after seeing epf-test driver patch.
> 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> >  include/linux/pci-epf.h             |  16 ++++
> >  2 files changed, 125 insertions(+)
> > 
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 355a6f56fcea..94ac82bf84c5 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -6,10 +6,12 @@
> >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> >   */
> >  
> > +#include <linux/irqreturn.h>
> 
> Why is this needed?
> 
> >  #include <linux/device.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> > +#include <linux/msi.h>
> >  
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> > @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
> >  
> > +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
> 
> static irqreturn_t
> 
> s/pci_epf_interrupt_handler/pci_epf_doorbell_handler
> 
> > +{
> > +	struct pci_epf *epf = data;
> > +
> > +	if (epf->event_ops && epf->event_ops->doorbell)
> > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > +	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> > +	struct pci_epf *epf;
> > +
> > +	/* Todo: Need check correct epf if multi epf supported */
> > +	list_for_each_entry(epf, &epc->pci_epf, list) {
> > +		if (epf->msg && desc->msi_index < epf->num_msgs)
> > +			epf->msg[desc->msi_index] = *msg;
> > +	}
> > +}
> > +
> > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> > +{
> > +	struct irq_domain *domain;
> > +	struct pci_epc *epc;
> > +	struct device *dev;
> > +	int virq;
> > +	int ret;
> > +	int i;
> > +
> > +	epc = epf->epc;
> > +	dev = &epc->dev;
> 
> "epc_dev" to make it explicit
> 
> > +
> > +	/*
> > +	 * Current only support 1 function.
> 
> What does this mean exactly? Even a single EPC can support multiple EPFs
> 

Please ignore above comment.

- Mani

> > +	 * PCI IMS(interrupt message store) ARM support have not been
> > +	 * ready yet.
> 
> No need to mention platform irq controller name.
> 
> > +	 */
> > +	if (epc->function_num_map != 1)
> 
> Why can't you use, epf->func_no?
> 
> > +		return -EOPNOTSUPP;
> > +
> > +	domain = dev_get_msi_domain(dev->parent);
> > +	if (!domain)
> > +		return -EOPNTSUPP;
> 
> Newline
> 
> > +	dev_set_msi_domain(dev, domain);
> > +
> > +	/* use parent of_node to get device id information */
> > +	dev->of_node = dev->parent->of_node;
> > +
> 
> Why do you need of_node assignment inside EPF core?
> 
> > +	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> > +	if (!epf->msg)
> > +		return -ENOMEM;
> > +
> > +	epf->num_msgs = num_msgs;
> > +
> 
> Move this to the start of the function, after checks.
> 
> > +	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> > +	if (ret) {
> > +		dev_err(dev, "Can't allocate MSI from system MSI controller\n");
> 
> "Failed to allocate MSI"
> 
> > +		goto err_mem;
> 
> err_free_mem
> 
> > +	}
> > +
> > +	for (i = 0; i < num_msgs; i++) {
> > +		virq = msi_get_virq(dev, i);
> > +		if (i == 0)
> > +			epf->virq_base = virq;
> > +
> > +		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
> > +				  "pci-epf-doorbell", epf);
> 
> IRQ name should have an index, otherwise all of them will have the same name.
> 
> > +
> > +		if (ret) {
> > +			dev_err(dev, "Failure request doorbell IRQ\n");
> 
> "Failed to request doorbell"
> 
> > +			goto err_irq;
> 
> err_free_irq
> 
> > +		}
> > +	}
> > +
> > +	epf->num_msgs = num_msgs;
> 
> Newline
> 
> > +	return ret;
> > +
> > +err_irq:
> > +	platform_msi_domain_free_irqs(dev);
> > +err_mem:
> > +	kfree(epf->msg);
> > +	epf->msg = NULL;
> > +	epf->num_msgs = 0;
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > +
> > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > +{
> > +	struct pci_epc *epc;
> > +	int i;
> > +
> > +	epc = epf->epc;
> > +
> > +	for (i = 0; i < epf->num_msgs; i++)
> > +		free_irq(epf->virq_base + i, epf);
> > +
> > +	platform_msi_domain_free_irqs(&epc->dev);
> > +	kfree(epf->msg);
> > +	epf->msg = NULL;
> > +	epf->num_msgs = 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > +
> >  static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> >  {
> >  	struct config_group *group, *tmp;
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index b8441db2fa52..e187e3ee48d2 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> >  struct pci_epc_event_ops {
> >  	int (*core_init)(struct pci_epf *epf);
> >  	int (*link_up)(struct pci_epf *epf);
> > +	int (*doorbell)(struct pci_epf *epf, int index);
> >  };
> >  
> >  /**
> > @@ -173,6 +174,9 @@ struct pci_epf {
> >  	unsigned long		vfunction_num_map;
> >  	struct list_head	pci_vepf;
> >  	const struct pci_epc_event_ops *event_ops;
> > +	struct msi_msg *msg;
> > +	u16 num_msgs;
> > +	int virq_base;
> >  };
> >  
> >  /**
> > @@ -216,4 +220,16 @@ 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);
> >  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > +
> > +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> > +{
> > +	return epf->msg;
> > +}
> > +
> > +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> > +{
> > +	return epf->num_msgs;
> > +}
> 
> I don't see a need for these two functions as they are doing just dereferences.
> 
> - Mani
> 
> >  #endif /* __LINUX_PCI_EPF_H */
> > -- 
> > 2.34.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Sat, Sep 02, 2023 at 10:23:28AM +0530, Manivannan Sadhasivam wrote:
> On Sat, Sep 02, 2023 at 10:22:25AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Apr 26, 2023 at 04:34:34PM -0400, Frank Li wrote:
> > > This commit introduces a common method for sending messages from the Root Complex
> > > (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
> > > such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
> > > by the PCI host to the message address of the platform MSI interrupt controller
> > > in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
> > > an IRQ at the EP. This implementation serves as a common method for all endpoint
> > > function drivers.
> > > 
> > > However, it currently supports only one EP physical function due to limitations
> > > in ARM MSI/IMS readiness.
> > > 
> > 
> > I've provided generic comments below, but I will do one more thorough review
> > after seeing epf-test driver patch.
> > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> > >  include/linux/pci-epf.h             |  16 ++++
> > >  2 files changed, 125 insertions(+)
> > > 
> > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > index 355a6f56fcea..94ac82bf84c5 100644
> > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > @@ -6,10 +6,12 @@
> > >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> > >   */
> > >  
> > > +#include <linux/irqreturn.h>
> > 
> > Why is this needed?
> > 
> > >  #include <linux/device.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/module.h>
> > > +#include <linux/msi.h>
> > >  
> > >  #include <linux/pci-epc.h>
> > >  #include <linux/pci-epf.h>
> > > @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
> > >  
> > > +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
> > 
> > static irqreturn_t
> > 
> > s/pci_epf_interrupt_handler/pci_epf_doorbell_handler
> > 
> > > +{
> > > +	struct pci_epf *epf = data;
> > > +
> > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > +{
> > > +	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> > > +	struct pci_epf *epf;
> > > +
> > > +	/* Todo: Need check correct epf if multi epf supported */
> > > +	list_for_each_entry(epf, &epc->pci_epf, list) {
> > > +		if (epf->msg && desc->msi_index < epf->num_msgs)
> > > +			epf->msg[desc->msi_index] = *msg;
> > > +	}
> > > +}
> > > +
> > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> > > +{
> > > +	struct irq_domain *domain;
> > > +	struct pci_epc *epc;
> > > +	struct device *dev;
> > > +	int virq;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	epc = epf->epc;
> > > +	dev = &epc->dev;
> > 
> > "epc_dev" to make it explicit

All other place use 'dev', I think better keep the consistent.

Frank
> > 
> > > +
> > > +	/*
> > > +	 * Current only support 1 function.
> > 
> > What does this mean exactly? Even a single EPC can support multiple EPFs
> > 
> 
> Please ignore above comment.
> 
> - Mani
> 
> > > +	 * PCI IMS(interrupt message store) ARM support have not been
> > > +	 * ready yet.
> > 
> > No need to mention platform irq controller name.

what's means?

> > 
> > > +	 */
> > > +	if (epc->function_num_map != 1)
> > 
> > Why can't you use, epf->func_no?
> > 
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	domain = dev_get_msi_domain(dev->parent);
> > > +	if (!domain)
> > > +		return -EOPNTSUPP;
> > 
> > Newline
> > 
> > > +	dev_set_msi_domain(dev, domain);
> > > +
> > > +	/* use parent of_node to get device id information */
> > > +	dev->of_node = dev->parent->of_node;
> > > +
> > 
> > Why do you need of_node assignment inside EPF core?

GIC need it to allocate a MSI irq to platform devices.
I think it may improve if IMS support.

Frank

> > 
> > > +	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> > > +	if (!epf->msg)
> > > +		return -ENOMEM;
> > > +
> > > +	epf->num_msgs = num_msgs;
> > > +
> > 
> > Move this to the start of the function, after checks.
> > 
> > > +	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> > > +	if (ret) {
> > > +		dev_err(dev, "Can't allocate MSI from system MSI controller\n");
> > 
> > "Failed to allocate MSI"
> > 
> > > +		goto err_mem;
> > 
> > err_free_mem
> > 
> > > +	}
> > > +
> > > +	for (i = 0; i < num_msgs; i++) {
> > > +		virq = msi_get_virq(dev, i);
> > > +		if (i == 0)
> > > +			epf->virq_base = virq;
> > > +
> > > +		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
> > > +				  "pci-epf-doorbell", epf);
> > 
> > IRQ name should have an index, otherwise all of them will have the same name.
> > 
> > > +
> > > +		if (ret) {
> > > +			dev_err(dev, "Failure request doorbell IRQ\n");
> > 
> > "Failed to request doorbell"
> > 
> > > +			goto err_irq;
> > 
> > err_free_irq
> > 
> > > +		}
> > > +	}
> > > +
> > > +	epf->num_msgs = num_msgs;
> > 
> > Newline
> > 
> > > +	return ret;
> > > +
> > > +err_irq:
> > > +	platform_msi_domain_free_irqs(dev);
> > > +err_mem:
> > > +	kfree(epf->msg);
> > > +	epf->msg = NULL;
> > > +	epf->num_msgs = 0;
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > > +
> > > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > > +{
> > > +	struct pci_epc *epc;
> > > +	int i;
> > > +
> > > +	epc = epf->epc;
> > > +
> > > +	for (i = 0; i < epf->num_msgs; i++)
> > > +		free_irq(epf->virq_base + i, epf);
> > > +
> > > +	platform_msi_domain_free_irqs(&epc->dev);
> > > +	kfree(epf->msg);
> > > +	epf->msg = NULL;
> > > +	epf->num_msgs = 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > > +
> > >  static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> > >  {
> > >  	struct config_group *group, *tmp;
> > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > index b8441db2fa52..e187e3ee48d2 100644
> > > --- a/include/linux/pci-epf.h
> > > +++ b/include/linux/pci-epf.h
> > > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> > >  struct pci_epc_event_ops {
> > >  	int (*core_init)(struct pci_epf *epf);
> > >  	int (*link_up)(struct pci_epf *epf);
> > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > >  };
> > >  
> > >  /**
> > > @@ -173,6 +174,9 @@ struct pci_epf {
> > >  	unsigned long		vfunction_num_map;
> > >  	struct list_head	pci_vepf;
> > >  	const struct pci_epc_event_ops *event_ops;
> > > +	struct msi_msg *msg;
> > > +	u16 num_msgs;
> > > +	int virq_base;
> > >  };
> > >  
> > >  /**
> > > @@ -216,4 +220,16 @@ 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);
> > >  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > > +
> > > +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> > > +{
> > > +	return epf->msg;
> > > +}
> > > +
> > > +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> > > +{
> > > +	return epf->num_msgs;
> > > +}
> > 
> > I don't see a need for these two functions as they are doing just dereferences.
> > 
> > - Mani
> > 
> > >  #endif /* __LINUX_PCI_EPF_H */
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Wed, Sep 06, 2023 at 12:24:50AM -0400, Frank Li wrote:
> On Sat, Sep 02, 2023 at 10:23:28AM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Sep 02, 2023 at 10:22:25AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Apr 26, 2023 at 04:34:34PM -0400, Frank Li wrote:
> > > > This commit introduces a common method for sending messages from the Root Complex
> > > > (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
> > > > such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
> > > > by the PCI host to the message address of the platform MSI interrupt controller
> > > > in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
> > > > an IRQ at the EP. This implementation serves as a common method for all endpoint
> > > > function drivers.
> > > > 
> > > > However, it currently supports only one EP physical function due to limitations
> > > > in ARM MSI/IMS readiness.
> > > > 
> > > 
> > > I've provided generic comments below, but I will do one more thorough review
> > > after seeing epf-test driver patch.
> > > 
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> > > >  include/linux/pci-epf.h             |  16 ++++
> > > >  2 files changed, 125 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > > index 355a6f56fcea..94ac82bf84c5 100644
> > > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > > @@ -6,10 +6,12 @@
> > > >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> > > >   */
> > > >  
> > > > +#include <linux/irqreturn.h>
> > > 
> > > Why is this needed?
> > > 
> > > >  #include <linux/device.h>
> > > >  #include <linux/dma-mapping.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/msi.h>
> > > >  
> > > >  #include <linux/pci-epc.h>
> > > >  #include <linux/pci-epf.h>
> > > > @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
> > > >  
> > > > +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
> > > 
> > > static irqreturn_t
> > > 
> > > s/pci_epf_interrupt_handler/pci_epf_doorbell_handler
> > > 
> > > > +{
> > > > +	struct pci_epf *epf = data;
> > > > +
> > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > +{
> > > > +	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> > > > +	struct pci_epf *epf;
> > > > +
> > > > +	/* Todo: Need check correct epf if multi epf supported */
> > > > +	list_for_each_entry(epf, &epc->pci_epf, list) {
> > > > +		if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > +			epf->msg[desc->msi_index] = *msg;
> > > > +	}
> > > > +}
> > > > +
> > > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> > > > +{
> > > > +	struct irq_domain *domain;
> > > > +	struct pci_epc *epc;
> > > > +	struct device *dev;
> > > > +	int virq;
> > > > +	int ret;
> > > > +	int i;
> > > > +
> > > > +	epc = epf->epc;
> > > > +	dev = &epc->dev;
> > > 
> > > "epc_dev" to make it explicit
> 
> All other place use 'dev', I think better keep the consistent.
> 
> Frank
> > > 
> > > > +
> > > > +	/*
> > > > +	 * Current only support 1 function.
> > > 
> > > What does this mean exactly? Even a single EPC can support multiple EPFs
> > > 
> > 
> > Please ignore above comment.
> > 
> > - Mani
> > 
> > > > +	 * PCI IMS(interrupt message store) ARM support have not been
> > > > +	 * ready yet.
> > > 
> > > No need to mention platform irq controller name.
> 
> what's means?
> 

"PCI IMS ARM support" is not needed. Just say that only one EPF is supported.

> > > 
> > > > +	 */
> > > > +	if (epc->function_num_map != 1)
> > > 
> > > Why can't you use, epf->func_no?
> > > 
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	domain = dev_get_msi_domain(dev->parent);
> > > > +	if (!domain)
> > > > +		return -EOPNTSUPP;
> > > 
> > > Newline
> > > 
> > > > +	dev_set_msi_domain(dev, domain);
> > > > +
> > > > +	/* use parent of_node to get device id information */
> > > > +	dev->of_node = dev->parent->of_node;
> > > > +
> > > 
> > > Why do you need of_node assignment inside EPF core?
> 
> GIC need it to allocate a MSI irq to platform devices.
> I think it may improve if IMS support.
> 

Can't you assign it in the EPF driver itself? I do not want any OF reference in
the EPF core since it has no OF support.

- Mani

> Frank
> 
> > > 
> > > > +	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> > > > +	if (!epf->msg)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	epf->num_msgs = num_msgs;
> > > > +
> > > 
> > > Move this to the start of the function, after checks.
> > > 
> > > > +	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Can't allocate MSI from system MSI controller\n");
> > > 
> > > "Failed to allocate MSI"
> > > 
> > > > +		goto err_mem;
> > > 
> > > err_free_mem
> > > 
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < num_msgs; i++) {
> > > > +		virq = msi_get_virq(dev, i);
> > > > +		if (i == 0)
> > > > +			epf->virq_base = virq;
> > > > +
> > > > +		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
> > > > +				  "pci-epf-doorbell", epf);
> > > 
> > > IRQ name should have an index, otherwise all of them will have the same name.
> > > 
> > > > +
> > > > +		if (ret) {
> > > > +			dev_err(dev, "Failure request doorbell IRQ\n");
> > > 
> > > "Failed to request doorbell"
> > > 
> > > > +			goto err_irq;
> > > 
> > > err_free_irq
> > > 
> > > > +		}
> > > > +	}
> > > > +
> > > > +	epf->num_msgs = num_msgs;
> > > 
> > > Newline
> > > 
> > > > +	return ret;
> > > > +
> > > > +err_irq:
> > > > +	platform_msi_domain_free_irqs(dev);
> > > > +err_mem:
> > > > +	kfree(epf->msg);
> > > > +	epf->msg = NULL;
> > > > +	epf->num_msgs = 0;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > > > +
> > > > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > > > +{
> > > > +	struct pci_epc *epc;
> > > > +	int i;
> > > > +
> > > > +	epc = epf->epc;
> > > > +
> > > > +	for (i = 0; i < epf->num_msgs; i++)
> > > > +		free_irq(epf->virq_base + i, epf);
> > > > +
> > > > +	platform_msi_domain_free_irqs(&epc->dev);
> > > > +	kfree(epf->msg);
> > > > +	epf->msg = NULL;
> > > > +	epf->num_msgs = 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > > > +
> > > >  static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> > > >  {
> > > >  	struct config_group *group, *tmp;
> > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > index b8441db2fa52..e187e3ee48d2 100644
> > > > --- a/include/linux/pci-epf.h
> > > > +++ b/include/linux/pci-epf.h
> > > > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> > > >  struct pci_epc_event_ops {
> > > >  	int (*core_init)(struct pci_epf *epf);
> > > >  	int (*link_up)(struct pci_epf *epf);
> > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -173,6 +174,9 @@ struct pci_epf {
> > > >  	unsigned long		vfunction_num_map;
> > > >  	struct list_head	pci_vepf;
> > > >  	const struct pci_epc_event_ops *event_ops;
> > > > +	struct msi_msg *msg;
> > > > +	u16 num_msgs;
> > > > +	int virq_base;
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -216,4 +220,16 @@ 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);
> > > >  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> > > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > > > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > > > +
> > > > +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> > > > +{
> > > > +	return epf->msg;
> > > > +}
> > > > +
> > > > +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> > > > +{
> > > > +	return epf->num_msgs;
> > > > +}
> > > 
> > > I don't see a need for these two functions as they are doing just dereferences.
> > > 
> > > - Mani
> > > 
> > > >  #endif /* __LINUX_PCI_EPF_H */
> > > > -- 
> > > > 2.34.1
> > > > 
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Wed, Sep 06, 2023 at 05:56:05PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Sep 06, 2023 at 12:24:50AM -0400, Frank Li wrote:
> > On Sat, Sep 02, 2023 at 10:23:28AM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Sep 02, 2023 at 10:22:25AM +0530, Manivannan Sadhasivam wrote:
> > > > On Wed, Apr 26, 2023 at 04:34:34PM -0400, Frank Li wrote:
> > > > > This commit introduces a common method for sending messages from the Root Complex
> > > > > (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
> > > > > such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
> > > > > by the PCI host to the message address of the platform MSI interrupt controller
> > > > > in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
> > > > > an IRQ at the EP. This implementation serves as a common method for all endpoint
> > > > > function drivers.
> > > > > 
> > > > > However, it currently supports only one EP physical function due to limitations
> > > > > in ARM MSI/IMS readiness.
> > > > > 
> > > > 
> > > > I've provided generic comments below, but I will do one more thorough review
> > > > after seeing epf-test driver patch.
> > > > 
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >  drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> > > > >  include/linux/pci-epf.h             |  16 ++++
> > > > >  2 files changed, 125 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > > > index 355a6f56fcea..94ac82bf84c5 100644
> > > > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > > > @@ -6,10 +6,12 @@
> > > > >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> > > > >   */
> > > > >  
> > > > > +#include <linux/irqreturn.h>
> > > > 
> > > > Why is this needed?
> > > > 
> > > > >  #include <linux/device.h>
> > > > >  #include <linux/dma-mapping.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/module.h>
> > > > > +#include <linux/msi.h>
> > > > >  
> > > > >  #include <linux/pci-epc.h>
> > > > >  #include <linux/pci-epf.h>
> > > > > @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
> > > > >  
> > > > > +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
> > > > 
> > > > static irqreturn_t
> > > > 
> > > > s/pci_epf_interrupt_handler/pci_epf_doorbell_handler
> > > > 
> > > > > +{
> > > > > +	struct pci_epf *epf = data;
> > > > > +
> > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > +{
> > > > > +	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> > > > > +	struct pci_epf *epf;
> > > > > +
> > > > > +	/* Todo: Need check correct epf if multi epf supported */
> > > > > +	list_for_each_entry(epf, &epc->pci_epf, list) {
> > > > > +		if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > +			epf->msg[desc->msi_index] = *msg;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> > > > > +{
> > > > > +	struct irq_domain *domain;
> > > > > +	struct pci_epc *epc;
> > > > > +	struct device *dev;
> > > > > +	int virq;
> > > > > +	int ret;
> > > > > +	int i;
> > > > > +
> > > > > +	epc = epf->epc;
> > > > > +	dev = &epc->dev;
> > > > 
> > > > "epc_dev" to make it explicit
> > 
> > All other place use 'dev', I think better keep the consistent.
> > 
> > Frank
> > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * Current only support 1 function.
> > > > 
> > > > What does this mean exactly? Even a single EPC can support multiple EPFs
> > > > 
> > > 
> > > Please ignore above comment.
> > > 
> > > - Mani
> > > 
> > > > > +	 * PCI IMS(interrupt message store) ARM support have not been
> > > > > +	 * ready yet.
> > > > 
> > > > No need to mention platform irq controller name.
> > 
> > what's means?
> > 
> 
> "PCI IMS ARM support" is not needed. Just say that only one EPF is supported.
> 
> > > > 
> > > > > +	 */
> > > > > +	if (epc->function_num_map != 1)
> > > > 
> > > > Why can't you use, epf->func_no?
> > > > 
> > > > > +		return -EOPNOTSUPP;
> > > > > +
> > > > > +	domain = dev_get_msi_domain(dev->parent);
> > > > > +	if (!domain)
> > > > > +		return -EOPNTSUPP;
> > > > 
> > > > Newline
> > > > 
> > > > > +	dev_set_msi_domain(dev, domain);
> > > > > +
> > > > > +	/* use parent of_node to get device id information */
> > > > > +	dev->of_node = dev->parent->of_node;
> > > > > +
> > > > 
> > > > Why do you need of_node assignment inside EPF core?
> > 
> > GIC need it to allocate a MSI irq to platform devices.
> > I think it may improve if IMS support.
> > 
> 
> Can't you assign it in the EPF driver itself? I do not want any OF reference in
> the EPF core since it has no OF support.

If that, Each EPF driver need do duplicate work. 

IMS will support per-device MSI domain, then we can implement customized
MSI irq allocated. But so far, it is simplest solution, we can update it
after IMS implementation at kernel. Only one place need be changed.

Frank Li 

> 
> - Mani
> 
> > Frank
> > 
> > > > 
> > > > > +	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> > > > > +	if (!epf->msg)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	epf->num_msgs = num_msgs;
> > > > > +
> > > > 
> > > > Move this to the start of the function, after checks.
> > > > 
> > > > > +	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Can't allocate MSI from system MSI controller\n");
> > > > 
> > > > "Failed to allocate MSI"
> > > > 
> > > > > +		goto err_mem;
> > > > 
> > > > err_free_mem
> > > > 
> > > > > +	}
> > > > > +
> > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > > +		virq = msi_get_virq(dev, i);
> > > > > +		if (i == 0)
> > > > > +			epf->virq_base = virq;
> > > > > +
> > > > > +		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
> > > > > +				  "pci-epf-doorbell", epf);
> > > > 
> > > > IRQ name should have an index, otherwise all of them will have the same name.
> > > > 
> > > > > +
> > > > > +		if (ret) {
> > > > > +			dev_err(dev, "Failure request doorbell IRQ\n");
> > > > 
> > > > "Failed to request doorbell"
> > > > 
> > > > > +			goto err_irq;
> > > > 
> > > > err_free_irq
> > > > 
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	epf->num_msgs = num_msgs;
> > > > 
> > > > Newline
> > > > 
> > > > > +	return ret;
> > > > > +
> > > > > +err_irq:
> > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > +err_mem:
> > > > > +	kfree(epf->msg);
> > > > > +	epf->msg = NULL;
> > > > > +	epf->num_msgs = 0;
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > > > > +
> > > > > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > > > > +{
> > > > > +	struct pci_epc *epc;
> > > > > +	int i;
> > > > > +
> > > > > +	epc = epf->epc;
> > > > > +
> > > > > +	for (i = 0; i < epf->num_msgs; i++)
> > > > > +		free_irq(epf->virq_base + i, epf);
> > > > > +
> > > > > +	platform_msi_domain_free_irqs(&epc->dev);
> > > > > +	kfree(epf->msg);
> > > > > +	epf->msg = NULL;
> > > > > +	epf->num_msgs = 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > > > > +
> > > > >  static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> > > > >  {
> > > > >  	struct config_group *group, *tmp;
> > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > index b8441db2fa52..e187e3ee48d2 100644
> > > > > --- a/include/linux/pci-epf.h
> > > > > +++ b/include/linux/pci-epf.h
> > > > > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> > > > >  struct pci_epc_event_ops {
> > > > >  	int (*core_init)(struct pci_epf *epf);
> > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > @@ -173,6 +174,9 @@ struct pci_epf {
> > > > >  	unsigned long		vfunction_num_map;
> > > > >  	struct list_head	pci_vepf;
> > > > >  	const struct pci_epc_event_ops *event_ops;
> > > > > +	struct msi_msg *msg;
> > > > > +	u16 num_msgs;
> > > > > +	int virq_base;
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > @@ -216,4 +220,16 @@ 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);
> > > > >  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> > > > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > > > > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > > > > +
> > > > > +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> > > > > +{
> > > > > +	return epf->msg;
> > > > > +}
> > > > > +
> > > > > +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> > > > > +{
> > > > > +	return epf->num_msgs;
> > > > > +}
> > > > 
> > > > I don't see a need for these two functions as they are doing just dereferences.
> > > > 
> > > > - Mani
> > > > 
> > > > >  #endif /* __LINUX_PCI_EPF_H */
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Wed, Sep 06, 2023 at 10:33:37AM -0400, Frank Li wrote:
> On Wed, Sep 06, 2023 at 05:56:05PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Sep 06, 2023 at 12:24:50AM -0400, Frank Li wrote:
> > > On Sat, Sep 02, 2023 at 10:23:28AM +0530, Manivannan Sadhasivam wrote:
> > > > On Sat, Sep 02, 2023 at 10:22:25AM +0530, Manivannan Sadhasivam wrote:
> > > > > On Wed, Apr 26, 2023 at 04:34:34PM -0400, Frank Li wrote:
> > > > > > This commit introduces a common method for sending messages from the Root Complex
> > > > > > (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt controller,
> > > > > > such as ARM GIC, as an EP doorbell. Maps the memory assigned for the BAR region
> > > > > > by the PCI host to the message address of the platform MSI interrupt controller
> > > > > > in the PCI EP. As a result, when the PCI RC writes to the BAR region, it triggers
> > > > > > an IRQ at the EP. This implementation serves as a common method for all endpoint
> > > > > > function drivers.
> > > > > > 
> > > > > > However, it currently supports only one EP physical function due to limitations
> > > > > > in ARM MSI/IMS readiness.
> > > > > > 
> > > > > 
> > > > > I've provided generic comments below, but I will do one more thorough review
> > > > > after seeing epf-test driver patch.
> > > > > 
> > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > ---
> > > > > >  drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> > > > > >  include/linux/pci-epf.h             |  16 ++++
> > > > > >  2 files changed, 125 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > > > > index 355a6f56fcea..94ac82bf84c5 100644
> > > > > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > > > > @@ -6,10 +6,12 @@
> > > > > >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> > > > > >   */
> > > > > >  
> > > > > > +#include <linux/irqreturn.h>
> > > > > 
> > > > > Why is this needed?
> > > > > 
> > > > > >  #include <linux/device.h>
> > > > > >  #include <linux/dma-mapping.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/module.h>
> > > > > > +#include <linux/msi.h>
> > > > > >  
> > > > > >  #include <linux/pci-epc.h>
> > > > > >  #include <linux/pci-epf.h>
> > > > > > @@ -300,6 +302,113 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(pci_epf_alloc_space);
> > > > > >  
> > > > > > +static enum irqreturn pci_epf_interrupt_handler(int irq, void *data)
> > > > > 
> > > > > static irqreturn_t
> > > > > 
> > > > > s/pci_epf_interrupt_handler/pci_epf_doorbell_handler
> > > > > 
> > > > > > +{
> > > > > > +	struct pci_epf *epf = data;
> > > > > > +
> > > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > > > +
> > > > > > +	return IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > > > > +static void pci_epf_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > > +{
> > > > > > +	struct pci_epc *epc = container_of(desc->dev, struct pci_epc, dev);
> > > > > > +	struct pci_epf *epf;
> > > > > > +
> > > > > > +	/* Todo: Need check correct epf if multi epf supported */
> > > > > > +	list_for_each_entry(epf, &epc->pci_epf, list) {
> > > > > > +		if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > > +			epf->msg[desc->msi_index] = *msg;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_msgs)
> > > > > > +{
> > > > > > +	struct irq_domain *domain;
> > > > > > +	struct pci_epc *epc;
> > > > > > +	struct device *dev;
> > > > > > +	int virq;
> > > > > > +	int ret;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	epc = epf->epc;
> > > > > > +	dev = &epc->dev;
> > > > > 
> > > > > "epc_dev" to make it explicit
> > > 
> > > All other place use 'dev', I think better keep the consistent.
> > > 
> > > Frank
> > > > > 
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Current only support 1 function.
> > > > > 
> > > > > What does this mean exactly? Even a single EPC can support multiple EPFs
> > > > > 
> > > > 
> > > > Please ignore above comment.
> > > > 
> > > > - Mani
> > > > 
> > > > > > +	 * PCI IMS(interrupt message store) ARM support have not been
> > > > > > +	 * ready yet.
> > > > > 
> > > > > No need to mention platform irq controller name.
> > > 
> > > what's means?
> > > 
> > 
> > "PCI IMS ARM support" is not needed. Just say that only one EPF is supported.
> > 
> > > > > 
> > > > > > +	 */
> > > > > > +	if (epc->function_num_map != 1)
> > > > > 
> > > > > Why can't you use, epf->func_no?
> > > > > 
> > > > > > +		return -EOPNOTSUPP;
> > > > > > +
> > > > > > +	domain = dev_get_msi_domain(dev->parent);
> > > > > > +	if (!domain)
> > > > > > +		return -EOPNTSUPP;
> > > > > 
> > > > > Newline
> > > > > 
> > > > > > +	dev_set_msi_domain(dev, domain);
> > > > > > +
> > > > > > +	/* use parent of_node to get device id information */
> > > > > > +	dev->of_node = dev->parent->of_node;
> > > > > > +
> > > > > 
> > > > > Why do you need of_node assignment inside EPF core?
> > > 
> > > GIC need it to allocate a MSI irq to platform devices.
> > > I think it may improve if IMS support.
> > > 
> > 
> > Can't you assign it in the EPF driver itself? I do not want any OF reference in
> > the EPF core since it has no OF support.
> 
> If that, Each EPF driver need do duplicate work. 
> 

Yes, and that's how it should be. EPF core has no job in supplying the of_node.
It is the responsibility of the EPF drivers as they depend on OF for platform
support.

- Mani

> IMS will support per-device MSI domain, then we can implement customized
> MSI irq allocated. But so far, it is simplest solution, we can update it
> after IMS implementation at kernel. Only one place need be changed.
> 
> Frank Li 
> 
> > 
> > - Mani
> > 
> > > Frank
> > > 
> > > > > 
> > > > > > +	epf->msg = kcalloc(num_msgs, sizeof(struct msi_msg), GFP_KERNEL);
> > > > > > +	if (!epf->msg)
> > > > > > +		return -ENOMEM;
> > > > > > +
> > > > > > +	epf->num_msgs = num_msgs;
> > > > > > +
> > > > > 
> > > > > Move this to the start of the function, after checks.
> > > > > 
> > > > > > +	ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epf_write_msi_msg);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "Can't allocate MSI from system MSI controller\n");
> > > > > 
> > > > > "Failed to allocate MSI"
> > > > > 
> > > > > > +		goto err_mem;
> > > > > 
> > > > > err_free_mem
> > > > > 
> > > > > > +	}
> > > > > > +
> > > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > > > +		virq = msi_get_virq(dev, i);
> > > > > > +		if (i == 0)
> > > > > > +			epf->virq_base = virq;
> > > > > > +
> > > > > > +		ret = request_irq(virq, pci_epf_interrupt_handler, 0,
> > > > > > +				  "pci-epf-doorbell", epf);
> > > > > 
> > > > > IRQ name should have an index, otherwise all of them will have the same name.
> > > > > 
> > > > > > +
> > > > > > +		if (ret) {
> > > > > > +			dev_err(dev, "Failure request doorbell IRQ\n");
> > > > > 
> > > > > "Failed to request doorbell"
> > > > > 
> > > > > > +			goto err_irq;
> > > > > 
> > > > > err_free_irq
> > > > > 
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	epf->num_msgs = num_msgs;
> > > > > 
> > > > > Newline
> > > > > 
> > > > > > +	return ret;
> > > > > > +
> > > > > > +err_irq:
> > > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > > +err_mem:
> > > > > > +	kfree(epf->msg);
> > > > > > +	epf->msg = NULL;
> > > > > > +	epf->num_msgs = 0;
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > > > > > +
> > > > > > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > > > > > +{
> > > > > > +	struct pci_epc *epc;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	epc = epf->epc;
> > > > > > +
> > > > > > +	for (i = 0; i < epf->num_msgs; i++)
> > > > > > +		free_irq(epf->virq_base + i, epf);
> > > > > > +
> > > > > > +	platform_msi_domain_free_irqs(&epc->dev);
> > > > > > +	kfree(epf->msg);
> > > > > > +	epf->msg = NULL;
> > > > > > +	epf->num_msgs = 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > > > > > +
> > > > > >  static void pci_epf_remove_cfs(struct pci_epf_driver *driver)
> > > > > >  {
> > > > > >  	struct config_group *group, *tmp;
> > > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > > index b8441db2fa52..e187e3ee48d2 100644
> > > > > > --- a/include/linux/pci-epf.h
> > > > > > +++ b/include/linux/pci-epf.h
> > > > > > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> > > > > >  struct pci_epc_event_ops {
> > > > > >  	int (*core_init)(struct pci_epf *epf);
> > > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > > >  };
> > > > > >  
> > > > > >  /**
> > > > > > @@ -173,6 +174,9 @@ struct pci_epf {
> > > > > >  	unsigned long		vfunction_num_map;
> > > > > >  	struct list_head	pci_vepf;
> > > > > >  	const struct pci_epc_event_ops *event_ops;
> > > > > > +	struct msi_msg *msg;
> > > > > > +	u16 num_msgs;
> > > > > > +	int virq_base;
> > > > > >  };
> > > > > >  
> > > > > >  /**
> > > > > > @@ -216,4 +220,16 @@ 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);
> > > > > >  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> > > > > > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > > > > > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > > > > > +
> > > > > > +static inline struct msi_msg *epf_get_msg(struct pci_epf *epf)
> > > > > > +{
> > > > > > +	return epf->msg;
> > > > > > +}
> > > > > > +
> > > > > > +static inline u16 epf_get_msg_num(struct pci_epf *epf)
> > > > > > +{
> > > > > > +	return epf->num_msgs;
> > > > > > +}
> > > > > 
> > > > > I don't see a need for these two functions as they are doing just dereferences.
> > > > > 
> > > > > - Mani
> > > > > 
> > > > > >  #endif /* __LINUX_PCI_EPF_H */
> > > > > > -- 
> > > > > > 2.34.1
> > > > > > 
> > > > > 
> > > > > -- 
> > > > > மணிவண்ணன் சதாசிவம்
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Wed, Sep 06, 2023 at 08:22:27PM +0530, Manivannan Sadhasivam wrote:
> > > 
> > > Can't you assign it in the EPF driver itself? I do not want any OF reference in
> > > the EPF core since it has no OF support.
> > 
> > If that, Each EPF driver need do duplicate work. 
> > 
> 
> Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> It is the responsibility of the EPF drivers as they depend on OF for platform
> support.

EPF driver still not depend on OF. such pci-epf-test, which was probed by
configfs.

Frank 

> 
> - Mani
> 
> > IMS will support per-device MSI domain, then we can implement customized
> > MSI irq allocated. But so far, it is simplest solution, we can update it
> > after IMS implementation at kernel. Only one place need be changed.
> > 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Manivannan Sadhasivam]

On Wed, Sep 06, 2023 at 11:00:12AM -0400, Frank Li wrote:
> On Wed, Sep 06, 2023 at 08:22:27PM +0530, Manivannan Sadhasivam wrote:
> > > > 
> > > > Can't you assign it in the EPF driver itself? I do not want any OF reference in
> > > > the EPF core since it has no OF support.
> > > 
> > > If that, Each EPF driver need do duplicate work. 
> > > 
> > 
> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> > It is the responsibility of the EPF drivers as they depend on OF for platform
> > support.
> 
> EPF driver still not depend on OF. such pci-epf-test, which was probed by
> configfs.
> 

Hmm, yeah. Then it should be part of the EPC driver.

Sorry for the confusion.

- Mani

> Frank 
> 
> > 
> > - Mani
> > 
> > > IMS will support per-device MSI domain, then we can implement customized
> > > MSI irq allocated. But so far, it is simplest solution, we can update it
> > > after IMS implementation at kernel. Only one place need be changed.
> > > 
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Wed, Sep 06, 2023 at 08:57:04PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Sep 06, 2023 at 11:00:12AM -0400, Frank Li wrote:
> > On Wed, Sep 06, 2023 at 08:22:27PM +0530, Manivannan Sadhasivam wrote:
> > > > > 
> > > > > Can't you assign it in the EPF driver itself? I do not want any OF reference in
> > > > > the EPF core since it has no OF support.
> > > > 
> > > > If that, Each EPF driver need do duplicate work. 
> > > > 
> > > 
> > > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> > > It is the responsibility of the EPF drivers as they depend on OF for platform
> > > support.
> > 
> > EPF driver still not depend on OF. such pci-epf-test, which was probed by
> > configfs.
> > 
> 
> Hmm, yeah. Then it should be part of the EPC driver.
> 
> Sorry for the confusion.

EPC can't get epf->msg data. It is too long time. I can't remember the
detail.

Let try to move to epc again. 

> 
> - Mani
> 
> > Frank 
> > 
> > > 
> > > - Mani
> > > 
> > > > IMS will support per-device MSI domain, then we can implement customized
> > > > MSI irq allocated. But so far, it is simplest solution, we can update it
> > > > after IMS implementation at kernel. Only one place need be changed.
> > > > 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

[PATCH 2/3] misc: pci_endpoint_test: Add doorbell test case

[Frank Li]

Reused flags as capability register in pci_endpoint_test struct to
support older driver versions. Save capability flags to 'cap' field
of struct pci_endpoint_test to prevent reading non-existent address.

Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR,
PCIE_ENDPOINT_TEST_DB_DATA.

Write data from PCI_ENDPOINT_TEST_DB_DATA to address from
PCI_ENDPOINT_TEST_DB_ADDR to trigger doorbell and wait for remote
endpoint feedback.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/misc/pci_endpoint_test.c | 41 ++++++++++++++++++++++++++++++++
 include/uapi/linux/pcitest.h     |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index ed4d0ef5e5c3..3320a3334594 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -52,6 +52,7 @@
 #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 PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0x0c
 #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
@@ -66,7 +67,12 @@
 #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_ADDR		0x34
+#define PCI_ENDPOINT_TEST_DB_DATA		0x38
+
 #define FLAG_USE_DMA				BIT(0)
+#define FLAG_SUPPORT_DOORBELL			BIT(1)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
@@ -102,6 +108,7 @@ enum pci_barno {
 	BAR_3,
 	BAR_4,
 	BAR_5,
+	NO_BAR = -1,
 };
 
 struct pci_endpoint_test {
@@ -118,6 +125,7 @@ struct pci_endpoint_test {
 	enum pci_barno test_reg_bar;
 	size_t alignment;
 	const char *name;
+	u32 cap;
 };
 
 struct pci_endpoint_test_data {
@@ -713,6 +721,35 @@ 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)
+{
+	enum pci_barno bar;
+	u32 data;
+	u32 addr;
+
+	if (!(test->cap & FLAG_SUPPORT_DOORBELL))
+		return false;
+
+	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
+	if (bar == NO_BAR)
+		return false;
+
+	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
+	addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_ADDR);
+	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
+	pci_endpoint_test_bar_writel(test, bar, addr, data);
+
+	wait_for_completion(&test->irq_raised);
+
+	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+	if (data & STATUS_DOORBELL_SUCCESS)
+		return true;
+
+	return false;
+}
+
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 				    unsigned long arg)
 {
@@ -760,6 +797,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:
@@ -887,6 +927,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	misc_device->parent = &pdev->dev;
 	misc_device->fops = &pci_endpoint_test_fops;
 
+	test->cap = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_FLAGS);
 	err = misc_register(misc_device);
 	if (err) {
 		dev_err(dev, "Failed to register device\n");
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index f9c1af8d141b..479ca1aa3ae0 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -20,6 +20,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

Re: [PATCH 2/3] misc: pci_endpoint_test: Add doorbell test case

[Manivannan Sadhasivam]

On Wed, Apr 26, 2023 at 04:34:35PM -0400, Frank Li wrote:
> Reused flags as capability register in pci_endpoint_test struct to
> support older driver versions. Save capability flags to 'cap' field
> of struct pci_endpoint_test to prevent reading non-existent address.
> 

This won't work, please see below.

> Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR,
> PCIE_ENDPOINT_TEST_DB_DATA.
> 
> Write data from PCI_ENDPOINT_TEST_DB_DATA to address from
> PCI_ENDPOINT_TEST_DB_ADDR to trigger doorbell and wait for remote
> endpoint feedback.

"wait for endpoint response"

> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 41 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/pcitest.h     |  1 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index ed4d0ef5e5c3..3320a3334594 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -52,6 +52,7 @@
>  #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 PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0x0c
>  #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
> @@ -66,7 +67,12 @@
>  #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_ADDR		0x34
> +#define PCI_ENDPOINT_TEST_DB_DATA		0x38
> +
>  #define FLAG_USE_DMA				BIT(0)
> +#define FLAG_SUPPORT_DOORBELL			BIT(1)
>  
>  #define PCI_DEVICE_ID_TI_AM654			0xb00c
>  #define PCI_DEVICE_ID_TI_J7200			0xb00f
> @@ -102,6 +108,7 @@ enum pci_barno {
>  	BAR_3,
>  	BAR_4,
>  	BAR_5,
> +	NO_BAR = -1,
>  };
>  
>  struct pci_endpoint_test {
> @@ -118,6 +125,7 @@ struct pci_endpoint_test {
>  	enum pci_barno test_reg_bar;
>  	size_t alignment;
>  	const char *name;
> +	u32 cap;
>  };
>  
>  struct pci_endpoint_test_data {
> @@ -713,6 +721,35 @@ 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)
> +{
> +	enum pci_barno bar;
> +	u32 data;
> +	u32 addr;
> +
> +	if (!(test->cap & FLAG_SUPPORT_DOORBELL))
> +		return false;
> +
> +	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> +	if (bar == NO_BAR)
> +		return false;

Is this possible?

> +
> +	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
> +	addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_ADDR);
> +	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> +
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
> +	pci_endpoint_test_bar_writel(test, bar, addr, data);

From patch 1, I understood that EP supports multiple doorbell. But you are not
making use of it here?

> +
> +	wait_for_completion(&test->irq_raised);
> +

No timeout?

> +	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> +	if (data & STATUS_DOORBELL_SUCCESS)

Please use a separate variable.

> +		return true;
> +
> +	return false;
> +}
> +
>  static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  				    unsigned long arg)
>  {
> @@ -760,6 +797,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:
> @@ -887,6 +927,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  	misc_device->parent = &pdev->dev;
>  	misc_device->fops = &pci_endpoint_test_fops;
>  
> +	test->cap = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_FLAGS);

This register will be overwritten by this driver during (copy,read,write) tests.
So this logic will not work.

- Mani

>  	err = misc_register(misc_device);
>  	if (err) {
>  		dev_err(dev, "Failed to register device\n");
> diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
> index f9c1af8d141b..479ca1aa3ae0 100644
> --- a/include/uapi/linux/pcitest.h
> +++ b/include/uapi/linux/pcitest.h
> @@ -20,6 +20,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 3/3] tools: PCI: Add 'B' option for test doorbell

[Frank Li]

Add doorbell test support.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 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 441b54234635..215d0aa8a09f 100644
--- a/tools/pci/pcitest.c
+++ b/tools/pci/pcitest.c
@@ -36,6 +36,7 @@ struct pci_test {
 	bool		copy;
 	unsigned long	size;
 	bool		use_dma;
+	bool		doorbell;
 };
 
 static int run_test(struct pci_test *test)
@@ -149,6 +150,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, "Push doorbell\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 */
@@ -174,7 +184,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;
@@ -224,6 +234,9 @@ int main(int argc, char **argv)
 	case 'd':
 		test->use_dma = true;
 		continue;
+	case 'B':
+		test->doorbell = true;
+		continue;
 	case 'h':
 	default:
 usage:
@@ -243,6 +256,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 0/3] Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

> 
> This patches add new API to pci-epf-core, so any EP driver can use it.
> 
> The key point is comments from Thomas Gleixner, who suggest use new
> PCI/IMS. But arm platform change still not be merged yet.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm
> 
> So I still use existed method implement RC to EP doorbell.
> 
> If Thomas Gleixner want to continue work on devmsi-v2-arm, I can help test
> and update this patch.
> 

Ping?

> Frank Li (3):
>   PCI: endpoint: Add RC-to-EP doorbell support using platform MSI
>     controller
>   misc: pci_endpoint_test: Add doorbell test case
>   tools: PCI: Add 'B' option for test doorbell
> 
>  drivers/misc/pci_endpoint_test.c    |  41 +++++++++++
>  drivers/pci/endpoint/pci-epf-core.c | 109
> ++++++++++++++++++++++++++++
>  include/linux/pci-epf.h             |  16 ++++
>  include/uapi/linux/pcitest.h        |   1 +
>  tools/pci/pcitest.c                 |  16 +++-
>  5 files changed, 182 insertions(+), 1 deletion(-)
> 
> --
> 2.34.1

Re: [PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

On Fri, May 12, 2023 at 02:45:12PM +0000, Frank Li wrote:
> > 
> > This patches add new API to pci-epf-core, so any EP driver can use it.
> > 
> > The key point is comments from Thomas Gleixner, who suggest use new
> > PCI/IMS. But arm platform change still not be merged yet.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm
> > 
> > So I still use existed method implement RC to EP doorbell.
> > 
> > If Thomas Gleixner want to continue work on devmsi-v2-arm, I can help test
> > and update this patch.
> > 
> 
> Ping?

Ping? 

> 
> > Frank Li (3):
> >   PCI: endpoint: Add RC-to-EP doorbell support using platform MSI
> >     controller
> >   misc: pci_endpoint_test: Add doorbell test case
> >   tools: PCI: Add 'B' option for test doorbell
> > 
> >  drivers/misc/pci_endpoint_test.c    |  41 +++++++++++
> >  drivers/pci/endpoint/pci-epf-core.c | 109
> > ++++++++++++++++++++++++++++
> >  include/linux/pci-epf.h             |  16 ++++
> >  include/uapi/linux/pcitest.h        |   1 +
> >  tools/pci/pcitest.c                 |  16 +++-
> >  5 files changed, 182 insertions(+), 1 deletion(-)
> > 
> > --
> > 2.34.1
> 

Re: [PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

On Mon, Jun 12, 2023 at 12:17:25PM -0400, Frank Li wrote:
> On Fri, May 12, 2023 at 02:45:12PM +0000, Frank Li wrote:
> > > 
> > > This patches add new API to pci-epf-core, so any EP driver can use it.
> > > 
> > > The key point is comments from Thomas Gleixner, who suggest use new
> > > PCI/IMS. But arm platform change still not be merged yet.
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm
> > > 
> > > So I still use existed method implement RC to EP doorbell.
> > > 
> > > If Thomas Gleixner want to continue work on devmsi-v2-arm, I can help test
> > > and update this patch.
> > > 
> > 
> > Ping?
> 
> Ping? 

ping? 

> 
> > 
> > > Frank Li (3):
> > >   PCI: endpoint: Add RC-to-EP doorbell support using platform MSI
> > >     controller
> > >   misc: pci_endpoint_test: Add doorbell test case
> > >   tools: PCI: Add 'B' option for test doorbell
> > > 
> > >  drivers/misc/pci_endpoint_test.c    |  41 +++++++++++
> > >  drivers/pci/endpoint/pci-epf-core.c | 109
> > > ++++++++++++++++++++++++++++
> > >  include/linux/pci-epf.h             |  16 ++++
> > >  include/uapi/linux/pcitest.h        |   1 +
> > >  tools/pci/pcitest.c                 |  16 +++-
> > >  5 files changed, 182 insertions(+), 1 deletion(-)
> > > 
> > > --
> > > 2.34.1
> > 

Re: [PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

On Mon, Jul 17, 2023 at 10:06:39AM -0400, Frank Li wrote:
> On Mon, Jun 12, 2023 at 12:17:25PM -0400, Frank Li wrote:
> > On Fri, May 12, 2023 at 02:45:12PM +0000, Frank Li wrote:
> > > > 
> > > > This patches add new API to pci-epf-core, so any EP driver can use it.
> > > > 
> > > > The key point is comments from Thomas Gleixner, who suggest use new
> > > > PCI/IMS. But arm platform change still not be merged yet.
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm
> > > > 
> > > > So I still use existed method implement RC to EP doorbell.
> > > > 
> > > > If Thomas Gleixner want to continue work on devmsi-v2-arm, I can help test
> > > > and update this patch.
> > > > 
> > > 
> > > Ping?
> > 
> > Ping? 
> 
> ping? 

@Mani
     Do you have chance to review these patches? It provide a common
method with GIC ITS to implement notification from RC to EP.

Frank

> 
> > 
> > > 
> > > > Frank Li (3):
> > > >   PCI: endpoint: Add RC-to-EP doorbell support using platform MSI
> > > >     controller
> > > >   misc: pci_endpoint_test: Add doorbell test case
> > > >   tools: PCI: Add 'B' option for test doorbell
> > > > 
> > > >  drivers/misc/pci_endpoint_test.c    |  41 +++++++++++
> > > >  drivers/pci/endpoint/pci-epf-core.c | 109
> > > > ++++++++++++++++++++++++++++
> > > >  include/linux/pci-epf.h             |  16 ++++
> > > >  include/uapi/linux/pcitest.h        |   1 +
> > > >  tools/pci/pcitest.c                 |  16 +++-
> > > >  5 files changed, 182 insertions(+), 1 deletion(-)
> > > > 
> > > > --
> > > > 2.34.1
> > > 

Re: [PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[Manivannan Sadhasivam]

On Thu, Aug 24, 2023 at 03:01:30PM -0400, Frank Li wrote:
> On Mon, Jul 17, 2023 at 10:06:39AM -0400, Frank Li wrote:
> > On Mon, Jun 12, 2023 at 12:17:25PM -0400, Frank Li wrote:
> > > On Fri, May 12, 2023 at 02:45:12PM +0000, Frank Li wrote:
> > > > > 
> > > > > This patches add new API to pci-epf-core, so any EP driver can use it.
> > > > > 
> > > > > The key point is comments from Thomas Gleixner, who suggest use new
> > > > > PCI/IMS. But arm platform change still not be merged yet.
> > > > > 
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm
> > > > > 
> > > > > So I still use existed method implement RC to EP doorbell.
> > > > > 
> > > > > If Thomas Gleixner want to continue work on devmsi-v2-arm, I can help test
> > > > > and update this patch.
> > > > > 
> > > > 
> > > > Ping?
> > > 
> > > Ping? 
> > 
> > ping? 
> 
> @Mani
>      Do you have chance to review these patches? It provide a common
> method with GIC ITS to implement notification from RC to EP.
> 

Sorry for the delay. I was wating for a review from Thomas. But since this
series hasn't caught his attention, I'll provide my review next week.

- Mani

> Frank
> 
> > 
> > > 
> > > > 
> > > > > Frank Li (3):
> > > > >   PCI: endpoint: Add RC-to-EP doorbell support using platform MSI
> > > > >     controller
> > > > >   misc: pci_endpoint_test: Add doorbell test case
> > > > >   tools: PCI: Add 'B' option for test doorbell
> > > > > 
> > > > >  drivers/misc/pci_endpoint_test.c    |  41 +++++++++++
> > > > >  drivers/pci/endpoint/pci-epf-core.c | 109
> > > > > ++++++++++++++++++++++++++++
> > > > >  include/linux/pci-epf.h             |  16 ++++
> > > > >  include/uapi/linux/pcitest.h        |   1 +
> > > > >  tools/pci/pcitest.c                 |  16 +++-
> > > > >  5 files changed, 182 insertions(+), 1 deletion(-)
> > > > > 
> > > > > --
> > > > > 2.34.1
> > > > 

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

Re: [PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[Li Chen]

Hi Frank,

Frank Li <Frank.Li@nxp.com> writes:

> drivers/misc/pci_endpoint_test.c | 41 +++++++++++
> drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> include/linux/pci-epf.h | 16 ++++
> include/uapi/linux/pcitest.h | 1 +
> tools/pci/pcitest.c | 16 +++-
> 5 files changed, 182 insertions(+), 1 deletion(-)

It seems that you forgot to add changes to drivers/pci/endpoint/functions/pci-epf-test.c.

Regards,
Li

Re: [PATCH 0/3] Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

On Wed, Aug 30, 2023 at 03:36:05PM +0800, Li Chen wrote:
> 
> Hi Frank,
> 
> Frank Li <Frank.Li@nxp.com> writes:
> 
> > drivers/misc/pci_endpoint_test.c | 41 +++++++++++
> > drivers/pci/endpoint/pci-epf-core.c | 109 ++++++++++++++++++++++++++++
> > include/linux/pci-epf.h | 16 ++++
> > include/uapi/linux/pcitest.h | 1 +
> > tools/pci/pcitest.c | 16 +++-
> > 5 files changed, 182 insertions(+), 1 deletion(-)
> 
> It seems that you forgot to add changes to drivers/pci/endpoint/functions/pci-epf-test.c.

Yes, let us wait for mani's feedback and Update together.

Frank

> 
> Regards,
> Li