[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8918a885-b803-5430-8200-6b97c8782d76@amd.com>
Date: Fri, 29 Sep 2023 15:00:16 +0530
From: Kishon Vijay Abraham I <kvijayab@....com>
To: Frank Li <Frank.Li@....com>, manivannan.sadhasivam@...aro.org
Cc: aisheng.dong@....com, bhelgaas@...gle.com,
devicetree@...r.kernel.org, festevam@...il.com,
imx@...ts.linux.dev, jdmason@...zu.us, kernel@...gutronix.de,
kishon@...nel.org, kw@...ux.com,
linux-arm-kernel@...ts.infradead.org, linux-imx@....com,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
lorenzo.pieralisi@....com, lpieralisi@...nel.org, maz@...nel.org,
s.hauer@...gutronix.de, shawnguo@...nel.org, tglx@...utronix.de
Subject: Re: [PATCH v2 1/5] PCI: endpoint: Add RC-to-EP doorbell support using
platform MSI controller
Hi Frank,
On 9/12/2023 3:39 AM, 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.
This would help avoid the polling used in current EPF drivers. Thanks!
>
> However, it currently supports only one EP physical function due to
> limitations in ARM MSI/IMS readiness.
Any such platform or architecture restrictions should not be handled in
the endpoint core layer.
>
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> drivers/pci/endpoint/pci-epf-core.c | 44 +++++++
> include/linux/pci-epc.h | 6 +
> include/linux/pci-epf.h | 7 +
> 4 files changed, 249 insertions(+)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 5a4a8b0be6262..d336a99c6a94f 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -10,6 +10,7 @@
> #include <linux/slab.h>
> #include <linux/module.h>
>
> +#include <linux/msi.h>
> #include <linux/pci-epc.h>
> #include <linux/pci-epf.h>
> #include <linux/pci-ep-cfs.h>
> @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> }
> EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
>
> +/**
> + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> + * the space.
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + * @num_msgs: the total number of doorbell messages
> + *
> + * Return: 0 success, other is failure
> + */
> +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> +{
> + int ret;
> +
> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> + return -EINVAL;
> +
> + if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> + return -EINVAL;
> +
> + if (!epc->ops->alloc_doorbell)
> + return 0;
> +
> + mutex_lock(&epc->lock);
> + ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> + mutex_unlock(&epc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> +
> +/**
> + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + *
> + * Return: 0 success, other is failure
> + */
> +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> +{
> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> + return;
> +
> + if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> + return;
> +
> + if (!epc->ops->free_doorbell)
> + return;
> +
> + mutex_lock(&epc->lock);
> + epc->ops->free_doorbell(epc, func_no, vfunc_no);
> + mutex_unlock(&epc->lock);
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> +
> +static irqreturn_t pci_epf_generic_doorbell_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;
> +}
IMO the handler should be directly implemented in the EPF drivers. There
should be one API which returns the virq and the msi_msg to the EPF
driver and the EPF driver should do request_irq.
> +
> +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> + struct pci_epc *epc = NULL;
> + struct class_dev_iter iter;
> + struct pci_epf *epf;
> + struct device *dev;
> +
> + class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> + while ((dev = class_dev_iter_next(&iter))) {
> + if (dev->parent != desc->dev)
> + continue;
Ideally the msi_desc should be associated directly with the EPF device.
> +
> + epc = to_pci_epc(dev);
> +
> + class_dev_iter_exit(&iter);
> + break;
> + }
> +
> + if (!epc)
> + return;
> +
> + /* Only support one EPF for doorbell */
> + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> +
> + if (!epf)
> + return;
If this is a platform restriction, this should be moved elsewhere.
> +
> + if (epf->msg && desc->msi_index < epf->num_msgs)
> + epf->msg[desc->msi_index] = *msg;
> +}
> +
> +
> +/**
> + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> + * controller
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + * @num_msgs: the total number of doorbell messages
> + *
> + * Remark: use this function only if EPC driver just register one EPC device.
> + *
> + * Return: 0 success, other is failure
> + */
> +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> +{
> + struct pci_epf *epf;
> + struct device *dev;
> + int virq, last;
> + int ret;
> + int i;
> +
> + if (IS_ERR_OR_NULL(epc))
> + return -EINVAL;
> +
> + /* Currently only support one func and one vfunc for doorbell */
> + if (func_no || vfunc_no)
> + return -EINVAL;
> +
> + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> + if (!epf)
> + return -EINVAL;
> +
> + dev = epc->dev.parent;
> + ret = platform_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> + if (ret) {
> + dev_err(dev, "Failed to allocate MSI\n");
> + return -ENOMEM;
> + }
The alloc_irqs should be for a EPF device IMO.
> +
> + last = -1;
> + 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_generic_doorbell_handler, 0,
> + kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> +
> + if (ret) {
> + dev_err(dev, "Failed to request doorbell\n");
> + goto err_free_irq;
> + }
> + last = i;
> + }
> +
> + return 0;
> +
> +err_free_irq:
> + for (i = 0; i < last; i++)
> + kfree(free_irq(epf->virq_base + i, epf));
> + platform_msi_domain_free_irqs(dev);
> +
> + return -EINVAL;
> +}
Thanks,
Kishon
Powered by blists - more mailing lists