RE: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using platform... | linux-arm-kernel

quoted

-----Original Message-----
From: Manivannan Sadhasivam <redacted>
Sent: Thursday, November 24, 2022 12:45 PM
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: Re: [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 06:03:40PM +0000, Frank Li wrote:

-----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:
┌────────────┐   ┌──────────────
─
────────────────────┐   ┌───────
──
───────┐
│            │   │                                   │   │                │
│            │   │ 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                      │   │                │
│            │   │                                   │   │                │
│            │   │                                   │   │                │
└────────────┘   └──────────────
─
────────────────────┘   └───────
──
───────┘
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.

The subject should be:

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

Using platform MSI interrupt controller as endpoint(EP)'s doorbell.
Above line is not needed.

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.

message address of platform msi interrupt controller in PCI Endpoint.
s/msi/MSI. Also, use either Endpoint or EP, pick one but not both.

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"?

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.

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.

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.

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.

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
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.

 #define to_epf_ntb(epf_group) container_of((epf_group), struct
epf_ntb,
group)
@@ -256,11 +260,13 @@ static void epf_ntb_cmd_handler(struct
work_struct *work)
      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.

+     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)
      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.
I don’t think so.  Please check
drivers/pci/endpoint/functions/pci-epf-ntb.c

Looks like some EP hardware have mini windows map size requirement.

I think it is not important for this patch.  

Thanks,
Mani
--
மணிவண்ணன் சதாசிவம்
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help