mbox series

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

Message ID 20230426203436.1277307-1-Frank.Li@nxp.com
Headers show
Series Add RC-to-EP doorbell with platform MSI controller | expand

Message

Frank Li April 26, 2023, 8:34 p.m. UTC
┌────────────┐   ┌───────────────────────────────────┐   ┌────────────────┐
│            │   │                                   │   │                │
│            │   │ 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(-)

Comments

Frank Li May 12, 2023, 2:45 p.m. UTC | #1
> 
> 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
Frank Li July 17, 2023, 2:06 p.m. UTC | #2
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
> >
Manivannan Sadhasivam Aug. 25, 2023, 8:34 a.m. UTC | #3
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
> > > >
Li Chen Aug. 30, 2023, 7:36 a.m. UTC | #4
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
Frank Li Aug. 30, 2023, 6:27 p.m. UTC | #5
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
Manivannan Sadhasivam Sept. 2, 2023, 4:53 a.m. UTC | #6
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
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam Sept. 6, 2023, 12:26 p.m. UTC | #7
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
> > > > 
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
Frank Li Sept. 6, 2023, 2:33 p.m. UTC | #8
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
> > > > > 
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam Sept. 6, 2023, 3:27 p.m. UTC | #9
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.
> > > 
> > மணிவண்ணன் சதாசிவம்