On Thu, Nov 24, 2022 at 06:03:40PM +0000, Frank Li wrote:

quoted

-----Original Message-----
From: Manivannan Sadhasivam <redacted>
Sent: Thursday, November 24, 2022 3:00 AM
To: Frank Li <frank.li@nxp.com>
Cc: lpieralisi@kernel.org; Aisheng Dong <aisheng.dong@nxp.com>;
bhelgaas@google.com; devicetree@vger.kernel.org; festevam@gmail.com;
imx@lists.linux.dev; jdmason@kudzu.us; kernel@pengutronix.de;
kishon@ti.com; krzysztof.kozlowski+dt@linaro.org; kw@linux.com; linux-
arm-kernel@lists.infradead.org; dl-linux-imx [off-list ref]; linux-
kernel@vger.kernel.org; linux-pci@vger.kernel.org;
lorenzo.pieralisi@arm.com; lznuaa@gmail.com; maz@kernel.org;
ntb@lists.linux.dev; Peng Fan [off-list ref]; robh+dt@kernel.org;
s.hauer@pengutronix.de; shawnguo@kernel.org; tglx@linutronix.de
Subject: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using platform
MSI as doorbell

Caution: EXT Email

On Thu, Nov 24, 2022 at 12:50:36AM -0500, Frank Li wrote:

quoted

┌────────────┐   ┌───────────────

────────────────────┐   ┌─────────
───────┐

quoted

│            │   │                                   │   │                │
│            │   │ PCI Endpoint                      │   │ PCI Host       │
│            │   │                                   │   │                │
│            │◄──┤ 1.platform_msi_domain_alloc_irqs()│   │

│

quoted

│            │   │                                   │   │                │
│ MSI        ├──►│ 2.write_msi_msg()                 ├──►├─BAR<n>

│

quoted

│ Controller │   │   update doorbell register address│   │                │
│            │   │   for BAR                         │   │                │
│            │   │                                   │   │ 3. Write BAR<n>│
│            │◄──┼──────────────────────

─────────────┼───┤                │

quoted

│            │   │                                   │   │                │
│            ├──►│ 4.Irq Handle                      │   │                │
│            │   │                                   │   │                │
│            │   │                                   │   │                │
└────────────┘   └───────────────

────────────────────┘   └─────────
───────┘

quoted

There are at least couple of BAR regions used in this patch but they were not
mentioned in the above diagram.

This patch just affected one BAR regions.  Do you like "BAR[DB]"?

Do you want to me draw other BARs, which used by this function?

It'd be good to just mention DB BAR.

quoted

The subject should be:

"PCI: endpoint: pci-epf-vntb: Use EP MSI controller to handle DB from host"

quoted

Using platform MSI interrupt controller as endpoint(EP)'s doorbell.

Above line is not needed.

quoted

The memory assigned for BAR region by the PCI host is mapped to the

Which BAR? (BAR 1 aka. DB BAR)? There are multiple BAR regions exposed by
this function driver.

quoted

message address of platform msi interrupt controller in PCI Endpoint.

s/msi/MSI. Also, use either Endpoint or EP, pick one but not both.

quoted

Such that, whenever the PCI host writes to the BAR region, it will
trigger an IRQ in the EP.

Basic working follow as

"work flow is"?

quoted

1. EP function driver call platform_msi_domain_alloc_irqs() alloc a

pci-epf-vntb function driver calls platform_msi_domain_alloc_irqs() to
allocate
MSI's from the platform MSI controller.

quoted

MSI irq from MSI controller with call back function write_msi_msg();
2. write_msg_msg will config BAR and map to address defined in msi_msg;

The epf_ntb_write_msi_msg() passed as a callback will write the offset of the
MSI controller's MSI address dedicated for each MSI to the doorbell register
db_offset and also writes the MSI data to db_data register in the CTRL BAR
region.

quoted

3. Host side trigger an IRQ at Endpoint by write to BAR region.

Finally, the host can trigger doorbell by reading the offset of the doorbell
from db_offset register and writing the data read from db_data register in
CTRL
BAR region to the computed address in the DB BAR region.

quoted

Add MSI doorbell support for pci-epf-vntb. Query if system has an MSI
controller. Set up doorbell address according to struct msi_msg.

So PCI host can write this doorbell address to trigger EP side's IRQ.

If no MSI controller exists, fall back to software polling.

"Add doorbell support to pci-epf-vntb function driver making use of the
platform
MSI controller. If the MSI controller is not available, fallback to the polling
method."

Also, please move this paragraph to the beginning of the description.

quoted

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 146 +++++++++++++++---
 1 file changed, 125 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c

b/drivers/pci/endpoint/functions/pci-epf-vntb.c

quoted

index 0d744975f815..f770a068e58c 100644

--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

@@ -44,6 +44,7 @@
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
 #include <linux/ntb.h>
+#include <linux/msi.h>

 static struct workqueue_struct *kpcintb_workqueue;

@@ -137,11 +138,14 @@ struct epf_ntb {
      struct epf_ntb_ctrl *reg;

      u32 *epf_db;
+     phys_addr_t epf_db_phys;

      phys_addr_t vpci_mw_phy[MAX_MW];
      void __iomem *vpci_mw_addr[MAX_MW];

      struct delayed_work cmd_handler;
+
+     int msi_virqbase;
 };

You should add kernel doc comments for this struct in a separate patch. It
will
help in understanding the driver better.

quoted

 #define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb,

group)

quoted

@@ -256,11 +260,13 @@ static void epf_ntb_cmd_handler(struct

work_struct *work)

quoted

      ntb = container_of(work, struct epf_ntb, cmd_handler.work);

-     for (i = 1; i < ntb->db_count; i++) {
-             if (ntb->epf_db[i]) {
-                     ntb->db |= 1 << (i - 1);
-                     ntb_db_event(&ntb->ntb, i);
-                     ntb->epf_db[i] = 0;

A comment here stating that polling is implemented would be better.

quoted

+     if (!ntb->epf_db_phys) {
+             for (i = 1; i < ntb->db_count; i++) {
+                     if (ntb->epf_db[i]) {
+                             ntb->db |= 1 << (i - 1);
+                             ntb_db_event(&ntb->ntb, i);
+                             ntb->epf_db[i] = 0;
+                     }
              }
      }

@@ -518,6 +524,28 @@ static int epf_ntb_configure_interrupt(struct

epf_ntb *ntb)

quoted

      return 0;
 }

+static int epf_ntb_db_size(struct epf_ntb *ntb)
+{
+     const struct pci_epc_features *epc_features;
+     size_t size = sizeof(u32) * ntb->db_count;
+     u32 align;
+
+     epc_features = pci_epc_get_features(ntb->epf->epc,
+                                         ntb->epf->func_no,
+                                         ntb->epf->vfunc_no);
+     align = epc_features->align;
+
+     if (size < 128)

Shouldn't this be (size > 128)?

 
This is one coming from pci-epf-ntb.c.
Not sure there are some EP hardware have such limitation.
 

I'm not sure if that is correct though. drivers/ntb/hw/epf/ntb_hw_epf.c sets
the upper limit to 32 (NTB_EPF_MAX_DB_COUNT + 1) DBs, in that case the size
cannot go beyond 128.

Thanks,
Mani
-- 
மணிவண்ணன் சதாசிவம்

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel