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@xxxxxxx>
> > > > > > ---
> > > > > > 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@xxxxxx>
> > > > > > */
> > > > > >
> > > > > > +#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
> > > > > >
> > > > >
> > > > > --
> > > > > மணிவண்ணன் சதாசிவம்
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
