imx.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 0/6] PCI EP driver support MSI doorbell from host
@ 2022-09-22 16:12 Frank Li
  2022-09-22 16:12 ` [PATCH v12 1/6] platform-msi: export symbol platform_msi_create_irq_domain() Frank Li
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam



                  ┌───────┐          ┌──────────┐
                  │       │          │          │
┌─────────────┐   │       │          │ PCI Host │
│ MSI         │◄┐ │       │          │          │
│ Controller  │ │ │       │          │          │
└─────────────┘ └─┼───────┼──────────┼─Bar0     │
                  │ PCI   │          │ Bar1     │
                  │ Func  │          │ Bar2     │
                  │       │          │ Bar3     │
                  │       │          │ Bar4     │
                  │       ├─────────►│          │
                  └───────┘          └──────────┘

Many PCI controllers provided Endpoint functions.
Generally PCI endpoint is hardware, which is not running a rich OS,
like linux.

But Linux also supports endpoint functions.  PCI Host write BAR<n> space
like write to memory. The EP side can't know memory changed by the Host
driver. 

PCI Spec has not defined a standard method to do that.  Only define
MSI(x) to let EP notified RC status change. 

The basic idea is to trigger an IRQ when PCI RC writes to a memory
address. That's what MSI controller provided.  EP drivers just need to
request a platform MSI interrupt, struct MSI_msg *msg will pass down a
memory address and data.  EP driver will map such memory address to
one of PCI BAR<n>.  Host just writes such an address to trigger EP side
IRQ.

If system have gic-its, only need update PCI EP side driver. But i.MX
have not chip support gic-its yet. So we have to use MU to simulate a
MSI controller. Although only 4 MSI IRQs are simulated, it matched
vntb(pci-epf-vntb) network requirement.

After enable MSI, ping delay reduce < 1ms from ~8ms

IRQchip: imx mu worked as MSI controller: 
     let imx mu worked as MSI controllers. Although IP is not design
as MSI controller, we still can use it if limited IRQ number to 4.

pcie: endpoint: pci-epf-vntb: add endpoint MSI support
	 Based on ntb-next branch. https://github.com/jonmason/ntb/commits/ntb-next
	 Using MSI as door bell registers
	 
mu-msi patches (1-4) and vntb patch(5-6) is totally independently.
These can be applied by irqchip and pci's maintainer seperatedly.

i.MX EP function driver is upstreaming by Richard Zhu.
Some dts change missed at this patches. below is reference dts change

--- a/arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
@@ -160,5 +160,6 @@ pcieb_ep: pcie_ep@5f010000 {
                num-ib-windows = <6>;
                num-ob-windows = <6>;
                status = "disabled";
+               MSI-parent = <&lsio_mu12>;
        };

--- a/arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi
@@ -172,6 +172,19 @@ lsio_mu6: mailbox@5d210000 {
                status = "disabled";
        };

+       lsio_mu12: mailbox@5d270000 {
+               compatible = "fsl,imx6sx-mu-MSI";
+               msi-controller;
+               interrupt-controller;
+               reg = <0x5d270000 0x10000>,     /* A side */
+                     <0x5d300000 0x10000>;     /* B side */
+               reg-names = "a", "b";
+               interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
+               power-domains = <&pd IMX_SC_R_MU_12A>,
+                               <&pd IMX_SC_R_MU_12B>;
+               power-domain-names = "a", "b";
+       };
+

Change Log
- Change from v11 to v12
  pcie: 
	fix typo in commit message
	change subject in commit message
- Change from v10 to v11
  irqchips: no change
  pcie:
	clean up build warning foundby kernel test robot
	clean up kernel-doc warning.
	clean up vhost VHost.
- Change from v9 to v10
  fixed build error reported by kernel test robot <lkp@intel.com>
  irqchips:
	fixed accoring to Marc Zyngier's comments
	Added new patch platform-msi: export symbol
 platform_msi_create_irq_domain()
	Using one lock for both reg and alloc msi irq
	Using predefined macro to init cfg data
   pcie: endpoint:
	fixed according to Manivannan Sadhasivam's feedback
	Added makeup patch before enable msi irq
		PCI: endpoint: makeup pci-epf-vntb.c

- Change from v8 to v9
  fix dt_bind_check error

- Change from v7 to v8
  irqchip: using name process-a-side as resource bind name
  pcie: endpoint:
     - fix build error reported by kernel test robot <lkp@intel.com>
     - rename epf_db_phy to epf_db_phys
     - rework error message
     - rework commit message
     - change ntb to vtb at apply irq.
     - kept name msi_virqbase because it is msi irq base number,
	not base address. 
		
- Change from v6 to v7
  pcie: endpoint: add endpoint MSI support
  Fine tuning commit message
  Fixed issues, reviewed by Bjorn Helgaas

- Change from v5 to v6
  Fixed build error found by kernel test robot

- Change from v4 to v5
  Fixed dt-binding document
        add msi-cell
        add interrupt max number
	update naming reg-names and power-domain-names.
  Fixed irqchip-Add-IMX-MU-MSI-controller-driver.patch
        rework commit message
        remove some field in struct imx_mu_dcfg
	error handle when link power domain failure.
	add irq_domain_update_bus_token

- Change from v3 to v4
  Fixed dt-binding document according to Krzysztof Kozlowski's feedback
  Fixed irqchip-imx-mu-worked-as-msi-controller according to Marc Zyngier's
        comments.

	There are still two important points, which I am not sure.
	1. clean irq_set_affinity after platform_msi_create_irq_domain.
	   Some function, like platform_msi_write_msg() is static.
	   so I have to set MSI_FLAG_USE_DEF_CHIP_OPS flags, which will
	   set irq_set_affinity to default one.
	2. about comments

	> +	msi_data->msi_domain = platform_msi_create_irq_domain(
	> +				of_node_to_fwnode(msi_data->pdev->dev.of_node),
	> +				&imx_mu_msi_domain_info,
	> +				msi_data->parent);

	"And you don't get an error due to the fact that you use the same
	fwnode for both domains without overriding the domain bus token?"

 	I did not understand yet. 

  Fixed static check warning, reported by Dan Carpenter
	pcie: endpoint: pci-epf-vntb: add endpoint MSI support

- Change from v2 to v3
  Fixed dt-binding docment check failure
  Fixed typo a cover letter.
  Change according Bjorn's comments at patch 
	pcie: endpoint: pci-epf-vntb: add endpoint MSI support
	 

- from V1 to V2
  Fixed fsl,mu-msi.yaml's problem
  Fixed irq-imx-mu-msi.c problem according Marc Zyngier's feeback 
  Added a new patch to allow pass down .pm by IRQCHIP_PLATFORM_DRIVER_END

Frank Li (6):
  platform-msi: export symbol platform_msi_create_irq_domain()
  irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END
  irqchip: Add IMX MU MSI controller driver
  dt-bindings: irqchip: imx mu work as msi controller
  PCI: endpoint: cleanup pci-epf-vntb.c
  PCI: endpoint: Add vNTB MSI support

 .../interrupt-controller/fsl,mu-msi.yaml      |  99 ++++
 drivers/base/platform-msi.c                   |   1 +
 drivers/irqchip/Kconfig                       |  14 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-imx-mu-msi.c              | 455 ++++++++++++++++++
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 296 ++++++++----
 include/linux/irqchip.h                       |   4 +-
 7 files changed, 786 insertions(+), 84 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
 create mode 100644 drivers/irqchip/irq-imx-mu-msi.c

-- 
2.35.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v12 1/6] platform-msi: export symbol platform_msi_create_irq_domain()
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
@ 2022-09-22 16:12 ` Frank Li
  2022-09-22 16:12 ` [PATCH v12 2/6] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam

Make platform msi irqchip driver can be built as module

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/base/platform-msi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 296ea673d6615..12b044151298b 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -138,6 +138,7 @@ struct irq_domain *platform_msi_create_irq_domain(struct fwnode_handle *fwnode,
 
 	return domain;
 }
+EXPORT_SYMBOL_GPL(platform_msi_create_irq_domain);
 
 static int platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec,
 					irq_write_msi_msg_t write_msi_msg)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v12 2/6] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
  2022-09-22 16:12 ` [PATCH v12 1/6] platform-msi: export symbol platform_msi_create_irq_domain() Frank Li
@ 2022-09-22 16:12 ` Frank Li
  2022-09-22 16:12 ` [PATCH v12 3/6] irqchip: Add IMX MU MSI controller driver Frank Li
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam

IRQCHIP_PLATFORM_DRIVER_* compilation define platform_driver
for irqchip. But can't set .pm field of platform_driver.
Added variadic macros to set .pm field or other field if need.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 include/linux/irqchip.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index 3a091d0710ae1..d5e6024cb2a8c 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -44,7 +44,8 @@ static const struct of_device_id drv_name##_irqchip_match_table[] = {
 #define IRQCHIP_MATCH(compat, fn) { .compatible = compat,		\
 				    .data = typecheck_irq_init_cb(fn), },
 
-#define IRQCHIP_PLATFORM_DRIVER_END(drv_name)				\
+
+#define IRQCHIP_PLATFORM_DRIVER_END(drv_name, ...)			\
 	{},								\
 };									\
 MODULE_DEVICE_TABLE(of, drv_name##_irqchip_match_table);		\
@@ -56,6 +57,7 @@ static struct platform_driver drv_name##_driver = {			\
 		.owner = THIS_MODULE,					\
 		.of_match_table = drv_name##_irqchip_match_table,	\
 		.suppress_bind_attrs = true,				\
+		__VA_ARGS__						\
 	},								\
 };									\
 builtin_platform_driver(drv_name##_driver)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v12 3/6] irqchip: Add IMX MU MSI controller driver
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
  2022-09-22 16:12 ` [PATCH v12 1/6] platform-msi: export symbol platform_msi_create_irq_domain() Frank Li
  2022-09-22 16:12 ` [PATCH v12 2/6] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
@ 2022-09-22 16:12 ` Frank Li
  2022-09-22 16:12 ` [PATCH v12 4/6] dt-bindings: irqchip: imx mu work as msi controller Frank Li
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam

The MU block found in a number of Freescale/NXP SoCs supports generating
IRQs by writing data to a register.

This enables the MU block to be used as a MSI controller, by leveraging
the platform-MSI API.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/irqchip/Kconfig          |  14 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-imx-mu-msi.c | 455 +++++++++++++++++++++++++++++++
 3 files changed, 470 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-mu-msi.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5e4e50122777d..b9adc698ef0fc 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -470,6 +470,20 @@ config IMX_INTMUX
 	help
 	  Support for the i.MX INTMUX interrupt multiplexer.
 
+config IMX_MU_MSI
+	tristate "i.MX MU used as MSI controller"
+	depends on OF && HAS_IOMEM
+	default m if ARCH_MXC
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	select GENERIC_MSI_IRQ_DOMAIN
+	help
+	  Provide a driver for the MU block used as a CPU-to-CPU MSI
+	  controller. This requires a specially crafted DT to make use
+	  of this driver.
+
+	  If unsure, say N
+
 config LS1X_IRQ
 	bool "Loongson-1 Interrupt Controller"
 	depends on MACH_LOONGSON32
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 5d8e21d3dc6d8..870423746c783 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
+obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
 obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
 obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-msi.c
new file mode 100644
index 0000000000000..4bb9456ba4820
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Freescale MU used as MSI controller
+ *
+ * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
+ * Copyright 2022 NXP
+ *	Frank Li <Frank.Li@nxp.com>
+ *	Peng Fan <peng.fan@nxp.com>
+ *
+ * Based on drivers/mailbox/imx-mailbox.c
+ */
+
+#include <linux/clk.h>
+#include <linux/dma-iommu.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
+#include <linux/spinlock.h>
+
+#define IMX_MU_CHANS            4
+
+enum imx_mu_xcr {
+	IMX_MU_GIER,
+	IMX_MU_GCR,
+	IMX_MU_TCR,
+	IMX_MU_RCR,
+	IMX_MU_xCR_MAX,
+};
+
+enum imx_mu_xsr {
+	IMX_MU_SR,
+	IMX_MU_GSR,
+	IMX_MU_TSR,
+	IMX_MU_RSR,
+	IMX_MU_xSR_MAX
+};
+
+enum imx_mu_type {
+	IMX_MU_V2 = BIT(1),
+};
+
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+
+struct imx_mu_dcfg {
+	enum imx_mu_type type;
+	u32     xTR;            /* Transmit Register0 */
+	u32     xRR;            /* Receive Register0 */
+	u32     xSR[IMX_MU_xSR_MAX];         /* Status Registers */
+	u32     xCR[IMX_MU_xCR_MAX];         /* Control Registers */
+};
+
+struct imx_mu_msi {
+	raw_spinlock_t			lock;
+	struct irq_domain		*msi_domain;
+	void __iomem			*regs;
+	phys_addr_t			msiir_addr;
+	const struct imx_mu_dcfg	*cfg;
+	unsigned long			used;
+	struct clk			*clk;
+};
+
+static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
+{
+	iowrite32(val, msi_data->regs + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
+{
+	return ioread32(msi_data->regs + offs);
+}
+
+static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
+{
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&msi_data->lock, flags);
+	val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
+	raw_spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	return val;
+}
+
+static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
+}
+
+static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
+}
+
+static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+	.name = "MU-MSI",
+	.irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_ops imx_mu_msi_irq_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.ops	= &imx_mu_msi_irq_ops,
+	.chip	= &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
+					  struct msi_msg *msg)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+	u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
+
+	msg->address_hi = upper_32_bits(addr);
+	msg->address_lo = lower_32_bits(addr);
+	msg->data = data->hwirq;
+}
+
+static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+	.name		= "MU",
+	.irq_mask	= imx_mu_msi_parent_mask_irq,
+	.irq_unmask	= imx_mu_msi_parent_unmask_irq,
+	.irq_ack	= imx_mu_msi_parent_ack_irq,
+	.irq_compose_msi_msg	= imx_mu_msi_parent_compose_msg,
+	.irq_set_affinity = imx_mu_msi_parent_set_affinity,
+};
+
+static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs,
+					void *args)
+{
+	struct imx_mu_msi *msi_data = domain->host_data;
+	unsigned long flags;
+	int pos, err = 0;
+
+	WARN_ON(nr_irqs != 1);
+
+	raw_spin_lock_irqsave(&msi_data->lock, flags);
+	pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
+	if (pos < IMX_MU_CHANS)
+		__set_bit(pos, &msi_data->used);
+	else
+		err = -ENOSPC;
+	raw_spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	if (err)
+		return err;
+
+	irq_domain_set_info(domain, virq, pos,
+			    &imx_mu_msi_parent_chip, msi_data,
+			    handle_edge_irq, NULL, NULL);
+	return 0;
+}
+
+static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&msi_data->lock, flags);
+	__clear_bit(d->hwirq, &msi_data->used);
+	raw_spin_unlock_irqrestore(&msi_data->lock, flags);
+}
+
+static const struct irq_domain_ops imx_mu_msi_domain_ops = {
+	.alloc	= imx_mu_msi_domain_irq_alloc,
+	.free	= imx_mu_msi_domain_irq_free,
+};
+
+static void imx_mu_msi_irq_handler(struct irq_desc *desc)
+{
+	struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 status;
+	int i;
+
+	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
+
+	chained_irq_enter(chip, desc);
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		if (status & IMX_MU_xSR_RFn(msi_data, i))
+			generic_handle_domain_irq(msi_data->msi_domain, i);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct device *dev)
+{
+	struct fwnode_handle *fwnodes = dev_fwnode(dev);
+	struct irq_domain *parent;
+
+	/* Initialize MSI domain parent */
+	parent = irq_domain_create_linear(fwnodes,
+					    IMX_MU_CHANS,
+					    &imx_mu_msi_domain_ops,
+					    msi_data);
+	if (!parent) {
+		dev_err(dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
+
+	msi_data->msi_domain = platform_msi_create_irq_domain(fwnodes,
+					&imx_mu_msi_domain_info,
+					parent);
+
+	if (!msi_data->msi_domain) {
+		dev_err(dev, "failed to create MSI domain\n");
+		irq_domain_remove(parent);
+		return -ENOMEM;
+	}
+
+	irq_domain_set_pm_device(msi_data->msi_domain, dev);
+
+	return 0;
+}
+
+/* Register offset of different version MU IP */
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.type	= 0,
+	.xTR    = 0x0,
+	.xRR    = 0x10,
+	.xSR    = {
+			[IMX_MU_SR]  = 0x20,
+			[IMX_MU_GSR] = 0x20,
+			[IMX_MU_TSR] = 0x20,
+			[IMX_MU_RSR] = 0x20,
+		  },
+	.xCR    = {
+			[IMX_MU_GIER] = 0x24,
+			[IMX_MU_GCR]  = 0x24,
+			[IMX_MU_TCR]  = 0x24,
+			[IMX_MU_RCR]  = 0x24,
+		  },
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+	.type	= 0,
+	.xTR    = 0x20,
+	.xRR    = 0x40,
+	.xSR    = {
+			[IMX_MU_SR]  = 0x60,
+			[IMX_MU_GSR] = 0x60,
+			[IMX_MU_TSR] = 0x60,
+			[IMX_MU_RSR] = 0x60,
+		  },
+	.xCR    = {
+			[IMX_MU_GIER] = 0x64,
+			[IMX_MU_GCR]  = 0x64,
+			[IMX_MU_TCR]  = 0x64,
+			[IMX_MU_RCR]  = 0x64,
+		  },
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
+	.type   = IMX_MU_V2,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {
+			[IMX_MU_SR]  = 0xC,
+			[IMX_MU_GSR] = 0x118,
+			[IMX_MU_GSR] = 0x124,
+			[IMX_MU_RSR] = 0x12C,
+		  },
+	.xCR    = {
+			[IMX_MU_GIER] = 0x110,
+			[IMX_MU_GCR]  = 0x114,
+			[IMX_MU_TCR]  = 0x120,
+			[IMX_MU_RCR]  = 0x128
+		  },
+};
+
+static int __init imx_mu_of_init(struct device_node *dn,
+				 struct device_node *parent,
+				 const struct imx_mu_dcfg *cfg)
+{
+	struct platform_device *pdev = of_find_device_by_node(dn);
+	struct device_link *pd_link_a;
+	struct device_link *pd_link_b;
+	struct imx_mu_msi *msi_data;
+	struct resource *res;
+	struct device *pd_a;
+	struct device *pd_b;
+	struct device *dev;
+	int ret;
+	int irq;
+
+	dev = &pdev->dev;
+
+	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
+
+	msi_data->cfg = cfg;
+
+	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
+	if (IS_ERR(msi_data->regs)) {
+		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
+		return PTR_ERR(msi_data->regs);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
+	if (!res)
+		return -EIO;
+
+	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return -ENODEV;
+
+	platform_set_drvdata(pdev, msi_data);
+
+	msi_data->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(msi_data->clk))
+		return PTR_ERR(msi_data->clk);
+
+	pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
+	if (IS_ERR(pd_a))
+		return PTR_ERR(pd_a);
+
+	pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
+	if (IS_ERR(pd_b))
+		return PTR_ERR(pd_b);
+
+	pd_link_a = device_link_add(dev, pd_a,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!pd_link_a) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		goto err_pd_a;
+	}
+
+	pd_link_b = device_link_add(dev, pd_b,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+
+	if (!pd_link_b) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		goto err_pd_b;
+	}
+
+	ret = imx_mu_msi_domains_init(msi_data, dev);
+	if (ret)
+		goto err_dm_init;
+
+	pm_runtime_enable(dev);
+
+	irq_set_chained_handler_and_data(irq,
+					 imx_mu_msi_irq_handler,
+					 msi_data);
+
+	return 0;
+
+err_dm_init:
+	device_link_remove(dev,	pd_b);
+err_pd_b:
+	device_link_remove(dev, pd_a);
+err_pd_a:
+	return -EINVAL;
+}
+
+static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
+{
+	struct imx_mu_msi *priv = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
+{
+	struct imx_mu_msi *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		dev_err(dev, "failed to enable clock\n");
+
+	return ret;
+}
+
+static const struct dev_pm_ops imx_mu_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
+			   imx_mu_runtime_resume, NULL)
+};
+
+static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
+					 struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
+}
+
+static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
+					struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
+}
+
+static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
+					 struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
+IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
+IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm = &imx_mu_pm_ops)
+
+
+MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
+MODULE_DESCRIPTION("Freescale MU MSI controller driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v12 4/6] dt-bindings: irqchip: imx mu work as msi controller
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
                   ` (2 preceding siblings ...)
  2022-09-22 16:12 ` [PATCH v12 3/6] irqchip: Add IMX MU MSI controller driver Frank Li
@ 2022-09-22 16:12 ` Frank Li
  2022-09-22 16:12 ` [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up Frank Li
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam

I.MX mu support generate irq by write a register. Provide msi controller
support so other driver such as PCI EP can use it by standard msi
interface as doorbell.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../interrupt-controller/fsl,mu-msi.yaml      | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
new file mode 100644
index 0000000000000..799ae5c3e32ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/fsl,mu-msi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale/NXP i.MX Messaging Unit (MU) work as msi controller
+
+maintainers:
+  - Frank Li <Frank.Li@nxp.com>
+
+description: |
+  The Messaging Unit module enables two processors within the SoC to
+  communicate and coordinate by passing messages (e.g. data, status
+  and control) through the MU interface. The MU also provides the ability
+  for one processor (A side) to signal the other processor (B side) using
+  interrupts.
+
+  Because the MU manages the messaging between processors, the MU uses
+  different clocks (from each side of the different peripheral buses).
+  Therefore, the MU must synchronize the accesses from one side to the
+  other. The MU accomplishes synchronization using two sets of matching
+  registers (Processor A-side, Processor B-side).
+
+  MU can work as msi interrupt controller to do doorbell
+
+allOf:
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx6sx-mu-msi
+      - fsl,imx7ulp-mu-msi
+      - fsl,imx8ulp-mu-msi
+      - fsl,imx8ulp-mu-msi-s4
+
+  reg:
+    items:
+      - description: a side register base address
+      - description: b side register base address
+
+  reg-names:
+    items:
+      - const: processor-a-side
+      - const: processor-b-side
+
+  interrupts:
+    description: a side interrupt number.
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    items:
+      - description: a side power domain
+      - description: b side power domain
+
+  power-domain-names:
+    items:
+      - const: processor-a-side
+      - const: processor-b-side
+
+  interrupt-controller: true
+
+  msi-controller: true
+
+  "#msi-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - msi-controller
+  - "#msi-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/firmware/imx/rsrc.h>
+
+    msi-controller@5d270000 {
+        compatible = "fsl,imx6sx-mu-msi";
+        msi-controller;
+        #msi-cells = <0>;
+        interrupt-controller;
+        reg = <0x5d270000 0x10000>,     /* A side */
+              <0x5d300000 0x10000>;     /* B side */
+        reg-names = "processor-a-side", "processor-b-side";
+        interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
+        power-domains = <&pd IMX_SC_R_MU_12A>,
+                        <&pd IMX_SC_R_MU_12B>;
+        power-domain-names = "processor-a-side", "processor-b-side";
+    };
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
                   ` (3 preceding siblings ...)
  2022-09-22 16:12 ` [PATCH v12 4/6] dt-bindings: irqchip: imx mu work as msi controller Frank Li
@ 2022-09-22 16:12 ` Frank Li
  2022-10-06 14:25   ` Frank Li
  2022-09-22 16:12 ` [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support Frank Li
  2022-09-28 13:33 ` [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Marc Zyngier
  6 siblings, 1 reply; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam

Remove unused field: epf_db_phy.
Remove __iomem before epf_db.
Change epf_db to u32* from void *
Remove duplicate check if (readl(ntb->epf_db + i * 4)).
Using sizeof(u32) instead of number 4 at all place.

Clean up sparse build warning:
  Using  epf_db[i] instead of readl() because epf_db is located in local
  memory and allocated by dma_alloc_coherent(). Sparse build warning when
  there are not __iomem at readl().
  Added __iomem force type convert in vntb_epf_peer_spad_read\write() and
  vntb_epf_spad_read\write(). This require strong order at read and write.

Replace pci_epc_mem_free_addr() with pci_epf_free_space() at error handle
path to match pci_epf_alloc_space().

Cleanup warning found by scripts/kernel-doc
Fix indentation of the struct epf_ntb_ctrl
Consolidate term
  host, host1 to HOST
  vhost, vHost, Vhost, VHOST2 to VHOST

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 1466dd1904175..acea753af29ed 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -11,7 +11,7 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
-/**
+/*
  * +------------+         +---------------------------------------+
  * |            |         |                                       |
  * +------------+         |                        +--------------+
@@ -99,20 +99,20 @@ enum epf_ntb_bar {
  *       NTB Driver               NTB Driver
  */
 struct epf_ntb_ctrl {
-	u32     command;
-	u32     argument;
-	u16     command_status;
-	u16     link_status;
-	u32     topology;
-	u64     addr;
-	u64     size;
-	u32     num_mws;
-	u32	reserved;
-	u32     spad_offset;
-	u32     spad_count;
-	u32	db_entry_size;
-	u32     db_data[MAX_DB_COUNT];
-	u32     db_offset[MAX_DB_COUNT];
+	u32 command;
+	u32 argument;
+	u16 command_status;
+	u16 link_status;
+	u32 topology;
+	u64 addr;
+	u64 size;
+	u32 num_mws;
+	u32 reserved;
+	u32 spad_offset;
+	u32 spad_count;
+	u32 db_entry_size;
+	u32 db_data[MAX_DB_COUNT];
+	u32 db_offset[MAX_DB_COUNT];
 } __packed;
 
 struct epf_ntb {
@@ -136,8 +136,7 @@ struct epf_ntb {
 
 	struct epf_ntb_ctrl *reg;
 
-	phys_addr_t epf_db_phy;
-	void __iomem *epf_db;
+	u32 *epf_db;
 
 	phys_addr_t vpci_mw_phy[MAX_MW];
 	void __iomem *vpci_mw_addr[MAX_MW];
@@ -156,12 +155,14 @@ static struct pci_epf_header epf_ntb_header = {
 };
 
 /**
- * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host
+ * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
  * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @link_up: true or false indicating Link is UP or Down
  *
  * Once NTB function in HOST invoke ntb_link_enable(),
- * this NTB function driver will trigger a link event to vhost.
+ * this NTB function driver will trigger a link event to VHOST.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
 {
@@ -175,9 +176,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
 }
 
 /**
- * epf_ntb_configure_mw() - Configure the Outbound Address Space for vhost
- *   to access the memory window of host
- * @ntb: NTB device that facilitates communication between host and vhost
+ * epf_ntb_configure_mw() - Configure the Outbound Address Space for VHOST
+ *   to access the memory window of HOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @mw: Index of the memory window (either 0, 1, 2 or 3)
  *
  *                          EP Outbound Window
@@ -194,7 +195,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
  * |        |              |           |
  * |        |              |           |
  * +--------+              +-----------+
- *  VHost                   PCI EP
+ *  VHOST                   PCI EP
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
 {
@@ -219,7 +222,7 @@ static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
 
 /**
  * epf_ntb_teardown_mw() - Teardown the configured OB ATU
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @mw: Index of the memory window (either 0, 1, 2 or 3)
  *
  * Teardown the configured OB ATU configured in epf_ntb_configure_mw() using
@@ -234,12 +237,12 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw)
 }
 
 /**
- * epf_ntb_cmd_handler() - Handle commands provided by the NTB Host
+ * epf_ntb_cmd_handler() - Handle commands provided by the NTB HOST
  * @work: work_struct for the epf_ntb_epc
  *
  * Workqueue function that gets invoked for the two epf_ntb_epc
  * periodically (once every 5ms) to see if it has received any commands
- * from NTB host. The host can send commands to configure doorbell or
+ * from NTB HOST. The HOST can send commands to configure doorbell or
  * configure memory window or to update link status.
  */
 static void epf_ntb_cmd_handler(struct work_struct *work)
@@ -254,12 +257,9 @@ 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 (readl(ntb->epf_db + i * 4)) {
-			if (readl(ntb->epf_db + i * 4))
-				ntb->db |= 1 << (i - 1);
-
+		if (ntb->epf_db[i]) {
 			ntb_db_event(&ntb->ntb, i);
-			writel(0, ntb->epf_db + i * 4);
+			ntb->epf_db[i] = 0;
 		}
 	}
 
@@ -321,8 +321,8 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 
 /**
  * epf_ntb_config_sspad_bar_clear() - Clear Config + Self scratchpad BAR
- * @ntb_epc: EPC associated with one of the HOST which holds peer's outbound
- *	     address.
+ * @ntb: EPC associated with one of the HOST which holds peer's outbound
+ *	 address.
  *
  * Clear BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
  * self scratchpad region (removes inbound ATU configuration). While BAR0 is
@@ -331,8 +331,10 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
  * used for self scratchpad from epf_ntb_bar[BAR_CONFIG].
  *
  * Please note the self scratchpad region and config region is combined to
- * a single region and mapped using the same BAR. Also note HOST2's peer
- * scratchpad is HOST1's self scratchpad.
+ * a single region and mapped using the same BAR. Also note VHOST's peer
+ * scratchpad is HOST's self scratchpad.
+ *
+ * Returns: void
  */
 static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
 {
@@ -347,13 +349,15 @@ static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_config_sspad_bar_set() - Set Config + Self scratchpad BAR
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
- * Map BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
+ * Map BAR0 of EP CONTROLLER which contains the VHOST's config and
  * self scratchpad region.
  *
  * Please note the self scratchpad region and config region is combined to
  * a single region and mapped using the same BAR.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
 {
@@ -380,7 +384,7 @@ static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
 /**
  * epf_ntb_config_spad_bar_free() - Free the physical memory associated with
  *   config + scratchpad region
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
 {
@@ -393,11 +397,13 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
 /**
  * epf_ntb_config_spad_bar_alloc() - Allocate memory for config + scratchpad
  *   region
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Allocate the Local Memory mentioned in the above diagram. The size of
  * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION
  * is obtained from "spad-count" configfs entry.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 {
@@ -424,7 +430,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	spad_count = ntb->spad_count;
 
 	ctrl_size = sizeof(struct epf_ntb_ctrl);
-	spad_size = 2 * spad_count * 4;
+	spad_size = 2 * spad_count * sizeof(u32);
 
 	if (!align) {
 		ctrl_size = roundup_pow_of_two(ctrl_size);
@@ -454,7 +460,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	ctrl->num_mws = ntb->num_mws;
 	ntb->spad_size = spad_size;
 
-	ctrl->db_entry_size = 4;
+	ctrl->db_entry_size = sizeof(u32);
 
 	for (i = 0; i < ntb->db_count; i++) {
 		ntb->reg->db_data[i] = 1 + i;
@@ -465,11 +471,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 }
 
 /**
- * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capaiblity
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capability
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Configure MSI/MSI-X capability for each interface with number of
  * interrupts equal to "db_count" configfs entry.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 {
@@ -511,18 +519,22 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_db_bar_init() - Configure Doorbell window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 {
 	const struct pci_epc_features *epc_features;
-	u32 align;
 	struct device *dev = &ntb->epf->dev;
-	int ret;
 	struct pci_epf_bar *epf_bar;
-	void __iomem *mw_addr;
 	enum pci_barno barno;
-	size_t size = 4 * ntb->db_count;
+	void *mw_addr;
+	size_t size;
+	u32 align;
+	int ret;
+
+	size = sizeof(u32) * ntb->db_count;
 
 	epc_features = pci_epc_get_features(ntb->epf->epc,
 					    ntb->epf->func_no,
@@ -557,14 +569,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	return ret;
 
 err_alloc_peer_mem:
-	pci_epc_mem_free_addr(ntb->epf->epc, epf_bar->phys_addr, mw_addr, epf_bar->size);
+	pci_epf_free_space(ntb->epf, mw_addr, barno, 0);
 	return -1;
 }
 
 /**
  * epf_ntb_db_bar_clear() - Clear doorbell BAR and free memory
  *   allocated in peer's outbound address space
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 {
@@ -580,8 +592,9 @@ static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_mw_bar_init() - Configure Memory window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 {
@@ -629,7 +642,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_mw_bar_clear() - Clear Memory window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb)
 {
@@ -652,7 +665,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB interfaces
  */
@@ -665,7 +678,9 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
 /**
  * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
  * constructs (scratchpad region, doorbell, memorywindow)
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 {
@@ -706,11 +721,13 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_init() - Initialize NTB interface
- * @ntb: NTB device that facilitates communication between HOST and vHOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper to initialize a particular EPC interface and start the workqueue
- * to check for commands from host. This function will write to the
+ * to check for commands from HOST. This function will write to the
  * EP controller HW for configuring it.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_epc_init(struct epf_ntb *ntb)
 {
@@ -777,7 +794,7 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_cleanup() - Cleanup all NTB interfaces
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper to cleanup all NTB interfaces.
  */
@@ -934,6 +951,8 @@ static const struct config_item_type ntb_group_type = {
  *
  * Add configfs directory specific to NTB. This directory will hold
  * NTB specific properties like db_count, spad_count, num_mws etc.,
+ *
+ * Returns: Pointer to config_group
  */
 static struct config_group *epf_ntb_add_cfs(struct pci_epf *epf,
 					    struct config_group *group)
@@ -1084,11 +1103,11 @@ static int vntb_epf_link_enable(struct ntb_dev *ntb,
 static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
-	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * 4;
+	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
 	u32 val;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
-	val = readl(base + off + ct + idx * 4);
+	val = readl(base + off + ct + idx * sizeof(u32));
 	return val;
 }
 
@@ -1096,10 +1115,10 @@ static int vntb_epf_spad_write(struct ntb_dev *ndev, int idx, u32 val)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
-	int off = ctrl->spad_offset, ct = ctrl->spad_count * 4;
-	void __iomem *base = ntb->reg;
+	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
+	void __iomem *base = (void __iomem *)ntb->reg;
 
-	writel(val, base + off + ct + idx * 4);
+	writel(val, base + off + ct + idx * sizeof(u32));
 	return 0;
 }
 
@@ -1108,10 +1127,10 @@ static u32 vntb_epf_peer_spad_read(struct ntb_dev *ndev, int pidx, int idx)
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 	u32 val;
 
-	val = readl(base + off + idx * 4);
+	val = readl(base + off + idx * sizeof(u32));
 	return val;
 }
 
@@ -1120,9 +1139,9 @@ static int vntb_epf_peer_spad_write(struct ntb_dev *ndev, int pidx, int idx, u32
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
-	writel(val, base + off + idx * 4);
+	writel(val, base + off + idx * sizeof(u32));
 	return 0;
 }
 
@@ -1275,6 +1294,8 @@ static struct pci_driver vntb_pci_driver = {
  * Invoked when a primary interface or secondary interface is bound to EPC
  * device. This function will succeed only when EPC is bound to both the
  * interfaces.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_bind(struct pci_epf *epf)
 {
@@ -1359,6 +1380,8 @@ static struct pci_epf_ops epf_ntb_ops = {
  *
  * Probe NTB function driver when endpoint function bus detects a NTB
  * endpoint function.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_probe(struct pci_epf *epf)
 {
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
                   ` (4 preceding siblings ...)
  2022-09-22 16:12 ` [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up Frank Li
@ 2022-09-22 16:12 ` Frank Li
  2022-10-07  9:15   ` Lorenzo Pieralisi
  2022-09-28 13:33 ` [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Marc Zyngier
  6 siblings, 1 reply; 13+ messages in thread
From: Frank Li @ 2022-09-22 16:12 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
	aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam

                      ┌───────┐                   ┌──────────┐
                      │       │                   │          │
    ┌─────────────┐   │ PCI   │                   │ PCI Host │
    │ MSI         │◄┐ │ EP    │                   │          │
    │ Controller  │ │ │       │ 3.MSI Write       │          │
    └────────┬────┘ └─┼───────┼───────────────────┤          │
      ▲      │        │       │                   ├─BAR_n    │
      │      └────────┼───────┼──────────────────►│          │
      │               │       │ 2.Call Back       │          │
      │               │       │   write_msi_msg() │          │
      │               │       │                   │          │
      │               └───┬───┘                   └──────────┘
      │                   │
      └───────────────────┘
      1.platform_msi_domain_alloc_irqs()

There is no defined way of raising IRQs by PCI host to the PCI endpoint.
Only define MSI/MSI-X to let EP notified RC status change.

The memory assigned for BAR region by the PCI host is mapped to the
message address of platform msi interrupt controller in PCI Endpoint.
Such that, whenever the PCI host writes to the BAR region, it will
trigger an IRQ in the Endpoint.

Basic working follow as
1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
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;
3. Host side trigger an IRQ in Endpoint by write to BAR region.

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

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

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

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index acea753af29ed..8fdeac2201e29 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;
 };
 
 #define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb, group)
@@ -256,10 +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_event(&ntb->ntb, i);
-			ntb->epf_db[i] = 0;
+	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;
+			}
 		}
 	}
 
@@ -464,7 +471,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 
 	for (i = 0; i < ntb->db_count; i++) {
 		ntb->reg->db_data[i] = 1 + i;
-		ntb->reg->db_offset[i] = 0;
+		ntb->reg->db_offset[i] = sizeof(u32) * i;
 	}
 
 	return 0;
@@ -517,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)
+		size = 128;
+
+	if (align)
+		size = ALIGN(size, align);
+	else
+		size = roundup_pow_of_two(size);
+
+	return size;
+}
+
 /**
  * epf_ntb_db_bar_init() - Configure Doorbell window BARs
  * @ntb: NTB device that facilitates communication between HOST and VHOST
@@ -540,27 +569,26 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 					    ntb->epf->func_no,
 					    ntb->epf->vfunc_no);
 	align = epc_features->align;
-
-	if (size < 128)
-		size = 128;
-
-	if (align)
-		size = ALIGN(size, align);
-	else
-		size = roundup_pow_of_two(size);
+	size = epf_ntb_db_size(ntb);
 
 	barno = ntb->epf_ntb_bar[BAR_DB];
+	epf_bar = &ntb->epf->bar[barno];
 
-	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
-	if (!mw_addr) {
-		dev_err(dev, "Failed to allocate OB address\n");
-		return -ENOMEM;
+	if (ntb->epf_db_phys) {
+		mw_addr = NULL;
+		epf_bar->phys_addr = ntb->epf_db_phys;
+		epf_bar->barno = barno;
+		epf_bar->size = size;
+	} else {
+		mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
+		if (!mw_addr) {
+			dev_err(dev, "Failed to allocate doorbell address\n");
+			return -ENOMEM;
+		}
 	}
 
 	ntb->epf_db = mw_addr;
 
-	epf_bar = &ntb->epf->bar[barno];
-
 	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, epf_bar);
 	if (ret) {
 		dev_err(dev, "Doorbell BAR set failed\n");
@@ -719,6 +747,83 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 	return 0;
 }
 
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+static void epf_ntb_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	struct epf_ntb *ntb = dev_get_drvdata(desc->dev);
+	struct epf_ntb_ctrl *reg = ntb->reg;
+	int size = epf_ntb_db_size(ntb);
+	u64 addr;
+
+	addr = msg->address_hi;
+	addr <<= 32;
+	addr |= msg->address_lo;
+
+	reg->db_data[desc->msi_index] = msg->data;
+
+	if (!desc->msi_index)
+		ntb->epf_db_phys = round_down(addr, size);
+
+	reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phys;
+}
+#endif
+
+static irqreturn_t epf_ntb_interrupt_handler(int irq, void *data)
+{
+	struct epf_ntb *ntb = data;
+	int index;
+
+	index = irq - ntb->msi_virqbase;
+	ntb->db |= 1 << (index - 1);
+	ntb_db_event(&ntb->ntb, index);
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
+{
+	struct device *dev = &ntb->epf->dev;
+	struct irq_domain *domain;
+	int virq;
+	int ret;
+	int i;
+
+	domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
+	if (!domain)
+		return;
+
+	dev_set_msi_domain(dev, domain);
+
+	if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
+		ntb->db_count,
+		epf_ntb_write_msi_msg)) {
+		dev_err(dev, "Can't allocate MSI, falling back to polling mode\n");
+		return;
+	}
+	dev_info(dev, "Using MSI as doorbell\n");
+
+	for (i = 0; i < ntb->db_count; i++) {
+		virq = msi_get_virq(dev, i);
+		ret = devm_request_irq(dev, virq,
+			       epf_ntb_interrupt_handler, 0,
+			       "pci_epf_vntb", ntb);
+
+		if (ret) {
+			dev_err(dev, "Failed to request doorbell IRQ! Falling back to polling mode");
+			ntb->epf_db_phys = 0;
+			break;
+		}
+
+		if (!i)
+			ntb->msi_virqbase = virq; /* msi start virq number */
+	}
+}
+#else
+static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
+{
+}
+#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
 /**
  * epf_ntb_epc_init() - Initialize NTB interface
  * @ntb: NTB device that facilitates communication between HOST and VHOST
@@ -1320,14 +1425,15 @@ static int epf_ntb_bind(struct pci_epf *epf)
 		goto err_bar_alloc;
 	}
 
+	epf_set_drvdata(epf, ntb);
+	epf_ntb_epc_msi_init(ntb);
+
 	ret = epf_ntb_epc_init(ntb);
 	if (ret) {
 		dev_err(dev, "Failed to initialize EPC\n");
 		goto err_bar_alloc;
 	}
 
-	epf_set_drvdata(epf, ntb);
-
 	pci_space[0] = (ntb->vntb_pid << 16) | ntb->vntb_vid;
 	pci_vntb_table[0].vendor = ntb->vntb_vid;
 	pci_vntb_table[0].device = ntb->vntb_pid;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v11 0/6] PCI EP driver support MSI doorbell from host
  2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
                   ` (5 preceding siblings ...)
  2022-09-22 16:12 ` [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support Frank Li
@ 2022-09-28 13:33 ` Marc Zyngier
  6 siblings, 0 replies; 13+ messages in thread
From: Marc Zyngier @ 2022-09-28 13:33 UTC (permalink / raw)
  To: Frank Li
  Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
	bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
	peng.fan, aisheng.dong, jdmason, kernel, festevam, linux-imx,
	kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
	manivannan.sadhasivam

On Thu, 22 Sep 2022 12:12:40 -0400,
Frank Li <Frank.Li@nxp.com> wrote:
> 
> 
> 
>                   ┌───────┐          ┌──────────┐
>                   │       │          │          │
> ┌─────────────┐   │       │          │ PCI Host │
> │ MSI         │◄┐ │       │          │          │
> │ Controller  │ │ │       │          │          │
> └─────────────┘ └─┼───────┼──────────┼─Bar0     │
>                   │ PCI   │          │ Bar1     │
>                   │ Func  │          │ Bar2     │
>                   │       │          │ Bar3     │
>                   │       │          │ Bar4     │
>                   │       ├─────────►│          │
>                   └───────┘          └──────────┘
> 
> Many PCI controllers provided Endpoint functions.
> Generally PCI endpoint is hardware, which is not running a rich OS,
> like linux.
> 
> But Linux also supports endpoint functions.  PCI Host write BAR<n> space
> like write to memory. The EP side can't know memory changed by the Host
> driver. 
> 
> PCI Spec has not defined a standard method to do that.  Only define
> MSI(x) to let EP notified RC status change. 

[...]

FWIW, I have queued the first 4 patches of this series into -next. If
there is a need for these patches to be pulled by another subsystem, I
have pushed out a stable branch at [1].

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/fsl-mu-msi

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up
  2022-09-22 16:12 ` [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up Frank Li
@ 2022-10-06 14:25   ` Frank Li
  2022-10-07  8:43     ` Lorenzo Pieralisi
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Li @ 2022-10-06 14:25 UTC (permalink / raw)
  To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas, Bjorn Helgaas
  Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, Peng Fan,
	Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx, kishon,
	lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam



> -----Original Message-----
> From: Frank Li
> Sent: Thursday, September 22, 2022 11:14 AM
> To: maz@kernel.org; tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up
> 

[Frank Li] @Bjorn Helgaas, ping
Patches[1-4] was already picked  by irqchip. 
I think patch[5-6] should go through pci subsystem. 
Any additional comments?


> Remove unused field: epf_db_phy.
> Remove __iomem before epf_db.
> Change epf_db to u32* from void *
> Remove duplicate check if (readl(ntb->epf_db + i * 4)).
> Using sizeof(u32) instead of number 4 at all place.
> 
> Clean up sparse build warning:
>   Using  epf_db[i] instead of readl() because epf_db is located in local
>   memory and allocated by dma_alloc_coherent(). Sparse build warning
> when
>   there are not __iomem at readl().
>   Added __iomem force type convert in vntb_epf_peer_spad_read\write()
> and
>   vntb_epf_spad_read\write(). This require strong order at read and write.
> 
> Replace pci_epc_mem_free_addr() with pci_epf_free_space() at error handle
> path to match pci_epf_alloc_space().
> 
> Cleanup warning found by scripts/kernel-doc
> Fix indentation of the struct epf_ntb_ctrl
> Consolidate term
>   host, host1 to HOST
>   vhost, vHost, Vhost, VHOST2 to VHOST
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 157 ++++++++++--------
>  1 file changed, 90 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 1466dd1904175..acea753af29ed 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -11,7 +11,7 @@
>   * Author: Kishon Vijay Abraham I <kishon@ti.com>
>   */
> 
> -/**
> +/*
>   * +------------+         +---------------------------------------+
>   * |            |         |                                       |
>   * +------------+         |                        +--------------+
> @@ -99,20 +99,20 @@ enum epf_ntb_bar {
>   *       NTB Driver               NTB Driver
>   */
>  struct epf_ntb_ctrl {
> -	u32     command;
> -	u32     argument;
> -	u16     command_status;
> -	u16     link_status;
> -	u32     topology;
> -	u64     addr;
> -	u64     size;
> -	u32     num_mws;
> -	u32	reserved;
> -	u32     spad_offset;
> -	u32     spad_count;
> -	u32	db_entry_size;
> -	u32     db_data[MAX_DB_COUNT];
> -	u32     db_offset[MAX_DB_COUNT];
> +	u32 command;
> +	u32 argument;
> +	u16 command_status;
> +	u16 link_status;
> +	u32 topology;
> +	u64 addr;
> +	u64 size;
> +	u32 num_mws;
> +	u32 reserved;
> +	u32 spad_offset;
> +	u32 spad_count;
> +	u32 db_entry_size;
> +	u32 db_data[MAX_DB_COUNT];
> +	u32 db_offset[MAX_DB_COUNT];
>  } __packed;
> 
>  struct epf_ntb {
> @@ -136,8 +136,7 @@ struct epf_ntb {
> 
>  	struct epf_ntb_ctrl *reg;
> 
> -	phys_addr_t epf_db_phy;
> -	void __iomem *epf_db;
> +	u32 *epf_db;
> 
>  	phys_addr_t vpci_mw_phy[MAX_MW];
>  	void __iomem *vpci_mw_addr[MAX_MW];
> @@ -156,12 +155,14 @@ static struct pci_epf_header epf_ntb_header = {
>  };
> 
>  /**
> - * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host
> + * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
>   * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   * @link_up: true or false indicating Link is UP or Down
>   *
>   * Once NTB function in HOST invoke ntb_link_enable(),
> - * this NTB function driver will trigger a link event to vhost.
> + * this NTB function driver will trigger a link event to VHOST.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
>  {
> @@ -175,9 +176,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool
> link_up)
>  }
> 
>  /**
> - * epf_ntb_configure_mw() - Configure the Outbound Address Space for
> vhost
> - *   to access the memory window of host
> - * @ntb: NTB device that facilitates communication between host and vhost
> + * epf_ntb_configure_mw() - Configure the Outbound Address Space for
> VHOST
> + *   to access the memory window of HOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   * @mw: Index of the memory window (either 0, 1, 2 or 3)
>   *
>   *                          EP Outbound Window
> @@ -194,7 +195,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool
> link_up)
>   * |        |              |           |
>   * |        |              |           |
>   * +--------+              +-----------+
> - *  VHost                   PCI EP
> + *  VHOST                   PCI EP
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
>  {
> @@ -219,7 +222,7 @@ static int epf_ntb_configure_mw(struct epf_ntb *ntb,
> u32 mw)
> 
>  /**
>   * epf_ntb_teardown_mw() - Teardown the configured OB ATU
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   * @mw: Index of the memory window (either 0, 1, 2 or 3)
>   *
>   * Teardown the configured OB ATU configured in epf_ntb_configure_mw()
> using
> @@ -234,12 +237,12 @@ static void epf_ntb_teardown_mw(struct epf_ntb
> *ntb, u32 mw)
>  }
> 
>  /**
> - * epf_ntb_cmd_handler() - Handle commands provided by the NTB Host
> + * epf_ntb_cmd_handler() - Handle commands provided by the NTB HOST
>   * @work: work_struct for the epf_ntb_epc
>   *
>   * Workqueue function that gets invoked for the two epf_ntb_epc
>   * periodically (once every 5ms) to see if it has received any commands
> - * from NTB host. The host can send commands to configure doorbell or
> + * from NTB HOST. The HOST can send commands to configure doorbell or
>   * configure memory window or to update link status.
>   */
>  static void epf_ntb_cmd_handler(struct work_struct *work)
> @@ -254,12 +257,9 @@ 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 (readl(ntb->epf_db + i * 4)) {
> -			if (readl(ntb->epf_db + i * 4))
> -				ntb->db |= 1 << (i - 1);
> -
> +		if (ntb->epf_db[i]) {
>  			ntb_db_event(&ntb->ntb, i);
> -			writel(0, ntb->epf_db + i * 4);
> +			ntb->epf_db[i] = 0;
>  		}
>  	}
> 
> @@ -321,8 +321,8 @@ static void epf_ntb_cmd_handler(struct work_struct
> *work)
> 
>  /**
>   * epf_ntb_config_sspad_bar_clear() - Clear Config + Self scratchpad BAR
> - * @ntb_epc: EPC associated with one of the HOST which holds peer's
> outbound
> - *	     address.
> + * @ntb: EPC associated with one of the HOST which holds peer's outbound
> + *	 address.
>   *
>   * Clear BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
>   * self scratchpad region (removes inbound ATU configuration). While BAR0
> is
> @@ -331,8 +331,10 @@ static void epf_ntb_cmd_handler(struct work_struct
> *work)
>   * used for self scratchpad from epf_ntb_bar[BAR_CONFIG].
>   *
>   * Please note the self scratchpad region and config region is combined to
> - * a single region and mapped using the same BAR. Also note HOST2's peer
> - * scratchpad is HOST1's self scratchpad.
> + * a single region and mapped using the same BAR. Also note VHOST's peer
> + * scratchpad is HOST's self scratchpad.
> + *
> + * Returns: void
>   */
>  static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
>  {
> @@ -347,13 +349,15 @@ static void epf_ntb_config_sspad_bar_clear(struct
> epf_ntb *ntb)
> 
>  /**
>   * epf_ntb_config_sspad_bar_set() - Set Config + Self scratchpad BAR
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
> - * Map BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
> + * Map BAR0 of EP CONTROLLER which contains the VHOST's config and
>   * self scratchpad region.
>   *
>   * Please note the self scratchpad region and config region is combined to
>   * a single region and mapped using the same BAR.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
>  {
> @@ -380,7 +384,7 @@ static int epf_ntb_config_sspad_bar_set(struct
> epf_ntb *ntb)
>  /**
>   * epf_ntb_config_spad_bar_free() - Free the physical memory associated
> with
>   *   config + scratchpad region
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   */
>  static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
>  {
> @@ -393,11 +397,13 @@ static void epf_ntb_config_spad_bar_free(struct
> epf_ntb *ntb)
>  /**
>   * epf_ntb_config_spad_bar_alloc() - Allocate memory for config +
> scratchpad
>   *   region
> - * @ntb: NTB device that facilitates communication between HOST1 and
> HOST2
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
>   * Allocate the Local Memory mentioned in the above diagram. The size of
>   * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD
> REGION
>   * is obtained from "spad-count" configfs entry.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  {
> @@ -424,7 +430,7 @@ static int epf_ntb_config_spad_bar_alloc(struct
> epf_ntb *ntb)
>  	spad_count = ntb->spad_count;
> 
>  	ctrl_size = sizeof(struct epf_ntb_ctrl);
> -	spad_size = 2 * spad_count * 4;
> +	spad_size = 2 * spad_count * sizeof(u32);
> 
>  	if (!align) {
>  		ctrl_size = roundup_pow_of_two(ctrl_size);
> @@ -454,7 +460,7 @@ static int epf_ntb_config_spad_bar_alloc(struct
> epf_ntb *ntb)
>  	ctrl->num_mws = ntb->num_mws;
>  	ntb->spad_size = spad_size;
> 
> -	ctrl->db_entry_size = 4;
> +	ctrl->db_entry_size = sizeof(u32);
> 
>  	for (i = 0; i < ntb->db_count; i++) {
>  		ntb->reg->db_data[i] = 1 + i;
> @@ -465,11 +471,13 @@ static int epf_ntb_config_spad_bar_alloc(struct
> epf_ntb *ntb)
>  }
> 
>  /**
> - * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capaiblity
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capability
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
>   * Configure MSI/MSI-X capability for each interface with number of
>   * interrupts equal to "db_count" configfs entry.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
>  {
> @@ -511,18 +519,22 @@ static int epf_ntb_configure_interrupt(struct
> epf_ntb *ntb)
> 
>  /**
>   * epf_ntb_db_bar_init() - Configure Doorbell window BARs
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  {
>  	const struct pci_epc_features *epc_features;
> -	u32 align;
>  	struct device *dev = &ntb->epf->dev;
> -	int ret;
>  	struct pci_epf_bar *epf_bar;
> -	void __iomem *mw_addr;
>  	enum pci_barno barno;
> -	size_t size = 4 * ntb->db_count;
> +	void *mw_addr;
> +	size_t size;
> +	u32 align;
> +	int ret;
> +
> +	size = sizeof(u32) * ntb->db_count;
> 
>  	epc_features = pci_epc_get_features(ntb->epf->epc,
>  					    ntb->epf->func_no,
> @@ -557,14 +569,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  	return ret;
> 
>  err_alloc_peer_mem:
> -	pci_epc_mem_free_addr(ntb->epf->epc, epf_bar->phys_addr,
> mw_addr, epf_bar->size);
> +	pci_epf_free_space(ntb->epf, mw_addr, barno, 0);
>  	return -1;
>  }
> 
>  /**
>   * epf_ntb_db_bar_clear() - Clear doorbell BAR and free memory
>   *   allocated in peer's outbound address space
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   */
>  static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
>  {
> @@ -580,8 +592,9 @@ static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
> 
>  /**
>   * epf_ntb_mw_bar_init() - Configure Memory window BARs
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
>  {
> @@ -629,7 +642,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
> 
>  /**
>   * epf_ntb_mw_bar_clear() - Clear Memory window BARs
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   */
>  static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb)
>  {
> @@ -652,7 +665,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb
> *ntb)
> 
>  /**
>   * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
>   * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB
> interfaces
>   */
> @@ -665,7 +678,9 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
>  /**
>   * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
>   * constructs (scratchpad region, doorbell, memorywindow)
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>  {
> @@ -706,11 +721,13 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
> 
>  /**
>   * epf_ntb_epc_init() - Initialize NTB interface
> - * @ntb: NTB device that facilitates communication between HOST and
> vHOST2
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
>   * Wrapper to initialize a particular EPC interface and start the workqueue
> - * to check for commands from host. This function will write to the
> + * to check for commands from HOST. This function will write to the
>   * EP controller HW for configuring it.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_epc_init(struct epf_ntb *ntb)
>  {
> @@ -777,7 +794,7 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
> 
>  /**
>   * epf_ntb_epc_cleanup() - Cleanup all NTB interfaces
> - * @ntb: NTB device that facilitates communication between HOST1 and
> HOST2
> + * @ntb: NTB device that facilitates communication between HOST and
> VHOST
>   *
>   * Wrapper to cleanup all NTB interfaces.
>   */
> @@ -934,6 +951,8 @@ static const struct config_item_type ntb_group_type
> = {
>   *
>   * Add configfs directory specific to NTB. This directory will hold
>   * NTB specific properties like db_count, spad_count, num_mws etc.,
> + *
> + * Returns: Pointer to config_group
>   */
>  static struct config_group *epf_ntb_add_cfs(struct pci_epf *epf,
>  					    struct config_group *group)
> @@ -1084,11 +1103,11 @@ static int vntb_epf_link_enable(struct ntb_dev
> *ntb,
>  static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
>  {
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
> -	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * 4;
> +	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count *
> sizeof(u32);
>  	u32 val;
> -	void __iomem *base = ntb->reg;
> +	void __iomem *base = (void __iomem *)ntb->reg;
> 
> -	val = readl(base + off + ct + idx * 4);
> +	val = readl(base + off + ct + idx * sizeof(u32));
>  	return val;
>  }
> 
> @@ -1096,10 +1115,10 @@ static int vntb_epf_spad_write(struct ntb_dev
> *ndev, int idx, u32 val)
>  {
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
>  	struct epf_ntb_ctrl *ctrl = ntb->reg;
> -	int off = ctrl->spad_offset, ct = ctrl->spad_count * 4;
> -	void __iomem *base = ntb->reg;
> +	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> +	void __iomem *base = (void __iomem *)ntb->reg;
> 
> -	writel(val, base + off + ct + idx * 4);
> +	writel(val, base + off + ct + idx * sizeof(u32));
>  	return 0;
>  }
> 
> @@ -1108,10 +1127,10 @@ static u32 vntb_epf_peer_spad_read(struct
> ntb_dev *ndev, int pidx, int idx)
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
>  	struct epf_ntb_ctrl *ctrl = ntb->reg;
>  	int off = ctrl->spad_offset;
> -	void __iomem *base = ntb->reg;
> +	void __iomem *base = (void __iomem *)ntb->reg;
>  	u32 val;
> 
> -	val = readl(base + off + idx * 4);
> +	val = readl(base + off + idx * sizeof(u32));
>  	return val;
>  }
> 
> @@ -1120,9 +1139,9 @@ static int vntb_epf_peer_spad_write(struct
> ntb_dev *ndev, int pidx, int idx, u32
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
>  	struct epf_ntb_ctrl *ctrl = ntb->reg;
>  	int off = ctrl->spad_offset;
> -	void __iomem *base = ntb->reg;
> +	void __iomem *base = (void __iomem *)ntb->reg;
> 
> -	writel(val, base + off + idx * 4);
> +	writel(val, base + off + idx * sizeof(u32));
>  	return 0;
>  }
> 
> @@ -1275,6 +1294,8 @@ static struct pci_driver vntb_pci_driver = {
>   * Invoked when a primary interface or secondary interface is bound to EPC
>   * device. This function will succeed only when EPC is bound to both the
>   * interfaces.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_bind(struct pci_epf *epf)
>  {
> @@ -1359,6 +1380,8 @@ static struct pci_epf_ops epf_ntb_ops = {
>   *
>   * Probe NTB function driver when endpoint function bus detects a NTB
>   * endpoint function.
> + *
> + * Returns: Zero for success, or an error code in case of failure
>   */
>  static int epf_ntb_probe(struct pci_epf *epf)
>  {
> --
> 2.35.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up
  2022-10-06 14:25   ` Frank Li
@ 2022-10-07  8:43     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 13+ messages in thread
From: Lorenzo Pieralisi @ 2022-10-07  8:43 UTC (permalink / raw)
  To: Frank Li
  Cc: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas, Bjorn Helgaas, linux-kernel, devicetree,
	linux-arm-kernel, linux-pci, Peng Fan, Aisheng Dong, jdmason,
	kernel, festevam, dl-linux-imx, kishon, lorenzo.pieralisi, ntb,
	lznuaa, imx, manivannan.sadhasivam

On Thu, Oct 06, 2022 at 02:25:01PM +0000, Frank Li wrote:

[...]

> [Frank Li] @Bjorn Helgaas, ping
> Patches[1-4] was already picked  by irqchip. 
> I think patch[5-6] should go through pci subsystem. 
> Any additional comments?

Yes. Don't mix clean-ups with new features. This patch
should be split it does too many things at once, group
the clean ups in different patches and send them as
a separate series.

Lorenzo

> > Remove unused field: epf_db_phy.
> > Remove __iomem before epf_db.
> > Change epf_db to u32* from void *
> > Remove duplicate check if (readl(ntb->epf_db + i * 4)).
> > Using sizeof(u32) instead of number 4 at all place.
> > 
> > Clean up sparse build warning:
> >   Using  epf_db[i] instead of readl() because epf_db is located in local
> >   memory and allocated by dma_alloc_coherent(). Sparse build warning
> > when
> >   there are not __iomem at readl().
> >   Added __iomem force type convert in vntb_epf_peer_spad_read\write()
> > and
> >   vntb_epf_spad_read\write(). This require strong order at read and write.
> > 
> > Replace pci_epc_mem_free_addr() with pci_epf_free_space() at error handle
> > path to match pci_epf_alloc_space().
> > 
> > Cleanup warning found by scripts/kernel-doc
> > Fix indentation of the struct epf_ntb_ctrl
> > Consolidate term
> >   host, host1 to HOST
> >   vhost, vHost, Vhost, VHOST2 to VHOST
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 157 ++++++++++--------
> >  1 file changed, 90 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index 1466dd1904175..acea753af29ed 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -11,7 +11,7 @@
> >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> >   */
> > 
> > -/**
> > +/*
> >   * +------------+         +---------------------------------------+
> >   * |            |         |                                       |
> >   * +------------+         |                        +--------------+
> > @@ -99,20 +99,20 @@ enum epf_ntb_bar {
> >   *       NTB Driver               NTB Driver
> >   */
> >  struct epf_ntb_ctrl {
> > -	u32     command;
> > -	u32     argument;
> > -	u16     command_status;
> > -	u16     link_status;
> > -	u32     topology;
> > -	u64     addr;
> > -	u64     size;
> > -	u32     num_mws;
> > -	u32	reserved;
> > -	u32     spad_offset;
> > -	u32     spad_count;
> > -	u32	db_entry_size;
> > -	u32     db_data[MAX_DB_COUNT];
> > -	u32     db_offset[MAX_DB_COUNT];
> > +	u32 command;
> > +	u32 argument;
> > +	u16 command_status;
> > +	u16 link_status;
> > +	u32 topology;
> > +	u64 addr;
> > +	u64 size;
> > +	u32 num_mws;
> > +	u32 reserved;
> > +	u32 spad_offset;
> > +	u32 spad_count;
> > +	u32 db_entry_size;
> > +	u32 db_data[MAX_DB_COUNT];
> > +	u32 db_offset[MAX_DB_COUNT];
> >  } __packed;
> > 
> >  struct epf_ntb {
> > @@ -136,8 +136,7 @@ struct epf_ntb {
> > 
> >  	struct epf_ntb_ctrl *reg;
> > 
> > -	phys_addr_t epf_db_phy;
> > -	void __iomem *epf_db;
> > +	u32 *epf_db;
> > 
> >  	phys_addr_t vpci_mw_phy[MAX_MW];
> >  	void __iomem *vpci_mw_addr[MAX_MW];
> > @@ -156,12 +155,14 @@ static struct pci_epf_header epf_ntb_header = {
> >  };
> > 
> >  /**
> > - * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host
> > + * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
> >   * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   * @link_up: true or false indicating Link is UP or Down
> >   *
> >   * Once NTB function in HOST invoke ntb_link_enable(),
> > - * this NTB function driver will trigger a link event to vhost.
> > + * this NTB function driver will trigger a link event to VHOST.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
> >  {
> > @@ -175,9 +176,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool
> > link_up)
> >  }
> > 
> >  /**
> > - * epf_ntb_configure_mw() - Configure the Outbound Address Space for
> > vhost
> > - *   to access the memory window of host
> > - * @ntb: NTB device that facilitates communication between host and vhost
> > + * epf_ntb_configure_mw() - Configure the Outbound Address Space for
> > VHOST
> > + *   to access the memory window of HOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   * @mw: Index of the memory window (either 0, 1, 2 or 3)
> >   *
> >   *                          EP Outbound Window
> > @@ -194,7 +195,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool
> > link_up)
> >   * |        |              |           |
> >   * |        |              |           |
> >   * +--------+              +-----------+
> > - *  VHost                   PCI EP
> > + *  VHOST                   PCI EP
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
> >  {
> > @@ -219,7 +222,7 @@ static int epf_ntb_configure_mw(struct epf_ntb *ntb,
> > u32 mw)
> > 
> >  /**
> >   * epf_ntb_teardown_mw() - Teardown the configured OB ATU
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   * @mw: Index of the memory window (either 0, 1, 2 or 3)
> >   *
> >   * Teardown the configured OB ATU configured in epf_ntb_configure_mw()
> > using
> > @@ -234,12 +237,12 @@ static void epf_ntb_teardown_mw(struct epf_ntb
> > *ntb, u32 mw)
> >  }
> > 
> >  /**
> > - * epf_ntb_cmd_handler() - Handle commands provided by the NTB Host
> > + * epf_ntb_cmd_handler() - Handle commands provided by the NTB HOST
> >   * @work: work_struct for the epf_ntb_epc
> >   *
> >   * Workqueue function that gets invoked for the two epf_ntb_epc
> >   * periodically (once every 5ms) to see if it has received any commands
> > - * from NTB host. The host can send commands to configure doorbell or
> > + * from NTB HOST. The HOST can send commands to configure doorbell or
> >   * configure memory window or to update link status.
> >   */
> >  static void epf_ntb_cmd_handler(struct work_struct *work)
> > @@ -254,12 +257,9 @@ 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 (readl(ntb->epf_db + i * 4)) {
> > -			if (readl(ntb->epf_db + i * 4))
> > -				ntb->db |= 1 << (i - 1);
> > -
> > +		if (ntb->epf_db[i]) {
> >  			ntb_db_event(&ntb->ntb, i);
> > -			writel(0, ntb->epf_db + i * 4);
> > +			ntb->epf_db[i] = 0;
> >  		}
> >  	}
> > 
> > @@ -321,8 +321,8 @@ static void epf_ntb_cmd_handler(struct work_struct
> > *work)
> > 
> >  /**
> >   * epf_ntb_config_sspad_bar_clear() - Clear Config + Self scratchpad BAR
> > - * @ntb_epc: EPC associated with one of the HOST which holds peer's
> > outbound
> > - *	     address.
> > + * @ntb: EPC associated with one of the HOST which holds peer's outbound
> > + *	 address.
> >   *
> >   * Clear BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
> >   * self scratchpad region (removes inbound ATU configuration). While BAR0
> > is
> > @@ -331,8 +331,10 @@ static void epf_ntb_cmd_handler(struct work_struct
> > *work)
> >   * used for self scratchpad from epf_ntb_bar[BAR_CONFIG].
> >   *
> >   * Please note the self scratchpad region and config region is combined to
> > - * a single region and mapped using the same BAR. Also note HOST2's peer
> > - * scratchpad is HOST1's self scratchpad.
> > + * a single region and mapped using the same BAR. Also note VHOST's peer
> > + * scratchpad is HOST's self scratchpad.
> > + *
> > + * Returns: void
> >   */
> >  static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
> >  {
> > @@ -347,13 +349,15 @@ static void epf_ntb_config_sspad_bar_clear(struct
> > epf_ntb *ntb)
> > 
> >  /**
> >   * epf_ntb_config_sspad_bar_set() - Set Config + Self scratchpad BAR
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> > - * Map BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
> > + * Map BAR0 of EP CONTROLLER which contains the VHOST's config and
> >   * self scratchpad region.
> >   *
> >   * Please note the self scratchpad region and config region is combined to
> >   * a single region and mapped using the same BAR.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
> >  {
> > @@ -380,7 +384,7 @@ static int epf_ntb_config_sspad_bar_set(struct
> > epf_ntb *ntb)
> >  /**
> >   * epf_ntb_config_spad_bar_free() - Free the physical memory associated
> > with
> >   *   config + scratchpad region
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   */
> >  static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
> >  {
> > @@ -393,11 +397,13 @@ static void epf_ntb_config_spad_bar_free(struct
> > epf_ntb *ntb)
> >  /**
> >   * epf_ntb_config_spad_bar_alloc() - Allocate memory for config +
> > scratchpad
> >   *   region
> > - * @ntb: NTB device that facilitates communication between HOST1 and
> > HOST2
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> >   * Allocate the Local Memory mentioned in the above diagram. The size of
> >   * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD
> > REGION
> >   * is obtained from "spad-count" configfs entry.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
> >  {
> > @@ -424,7 +430,7 @@ static int epf_ntb_config_spad_bar_alloc(struct
> > epf_ntb *ntb)
> >  	spad_count = ntb->spad_count;
> > 
> >  	ctrl_size = sizeof(struct epf_ntb_ctrl);
> > -	spad_size = 2 * spad_count * 4;
> > +	spad_size = 2 * spad_count * sizeof(u32);
> > 
> >  	if (!align) {
> >  		ctrl_size = roundup_pow_of_two(ctrl_size);
> > @@ -454,7 +460,7 @@ static int epf_ntb_config_spad_bar_alloc(struct
> > epf_ntb *ntb)
> >  	ctrl->num_mws = ntb->num_mws;
> >  	ntb->spad_size = spad_size;
> > 
> > -	ctrl->db_entry_size = 4;
> > +	ctrl->db_entry_size = sizeof(u32);
> > 
> >  	for (i = 0; i < ntb->db_count; i++) {
> >  		ntb->reg->db_data[i] = 1 + i;
> > @@ -465,11 +471,13 @@ static int epf_ntb_config_spad_bar_alloc(struct
> > epf_ntb *ntb)
> >  }
> > 
> >  /**
> > - * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capaiblity
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capability
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> >   * Configure MSI/MSI-X capability for each interface with number of
> >   * interrupts equal to "db_count" configfs entry.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
> >  {
> > @@ -511,18 +519,22 @@ static int epf_ntb_configure_interrupt(struct
> > epf_ntb *ntb)
> > 
> >  /**
> >   * epf_ntb_db_bar_init() - Configure Doorbell window BARs
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
> >  {
> >  	const struct pci_epc_features *epc_features;
> > -	u32 align;
> >  	struct device *dev = &ntb->epf->dev;
> > -	int ret;
> >  	struct pci_epf_bar *epf_bar;
> > -	void __iomem *mw_addr;
> >  	enum pci_barno barno;
> > -	size_t size = 4 * ntb->db_count;
> > +	void *mw_addr;
> > +	size_t size;
> > +	u32 align;
> > +	int ret;
> > +
> > +	size = sizeof(u32) * ntb->db_count;
> > 
> >  	epc_features = pci_epc_get_features(ntb->epf->epc,
> >  					    ntb->epf->func_no,
> > @@ -557,14 +569,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
> >  	return ret;
> > 
> >  err_alloc_peer_mem:
> > -	pci_epc_mem_free_addr(ntb->epf->epc, epf_bar->phys_addr,
> > mw_addr, epf_bar->size);
> > +	pci_epf_free_space(ntb->epf, mw_addr, barno, 0);
> >  	return -1;
> >  }
> > 
> >  /**
> >   * epf_ntb_db_bar_clear() - Clear doorbell BAR and free memory
> >   *   allocated in peer's outbound address space
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   */
> >  static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
> >  {
> > @@ -580,8 +592,9 @@ static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
> > 
> >  /**
> >   * epf_ntb_mw_bar_init() - Configure Memory window BARs
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
> >  {
> > @@ -629,7 +642,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
> > 
> >  /**
> >   * epf_ntb_mw_bar_clear() - Clear Memory window BARs
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   */
> >  static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb)
> >  {
> > @@ -652,7 +665,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb
> > *ntb)
> > 
> >  /**
> >   * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> >   * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB
> > interfaces
> >   */
> > @@ -665,7 +678,9 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
> >  /**
> >   * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
> >   * constructs (scratchpad region, doorbell, memorywindow)
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
> >  {
> > @@ -706,11 +721,13 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
> > 
> >  /**
> >   * epf_ntb_epc_init() - Initialize NTB interface
> > - * @ntb: NTB device that facilitates communication between HOST and
> > vHOST2
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> >   * Wrapper to initialize a particular EPC interface and start the workqueue
> > - * to check for commands from host. This function will write to the
> > + * to check for commands from HOST. This function will write to the
> >   * EP controller HW for configuring it.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_epc_init(struct epf_ntb *ntb)
> >  {
> > @@ -777,7 +794,7 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
> > 
> >  /**
> >   * epf_ntb_epc_cleanup() - Cleanup all NTB interfaces
> > - * @ntb: NTB device that facilitates communication between HOST1 and
> > HOST2
> > + * @ntb: NTB device that facilitates communication between HOST and
> > VHOST
> >   *
> >   * Wrapper to cleanup all NTB interfaces.
> >   */
> > @@ -934,6 +951,8 @@ static const struct config_item_type ntb_group_type
> > = {
> >   *
> >   * Add configfs directory specific to NTB. This directory will hold
> >   * NTB specific properties like db_count, spad_count, num_mws etc.,
> > + *
> > + * Returns: Pointer to config_group
> >   */
> >  static struct config_group *epf_ntb_add_cfs(struct pci_epf *epf,
> >  					    struct config_group *group)
> > @@ -1084,11 +1103,11 @@ static int vntb_epf_link_enable(struct ntb_dev
> > *ntb,
> >  static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
> >  {
> >  	struct epf_ntb *ntb = ntb_ndev(ndev);
> > -	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * 4;
> > +	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count *
> > sizeof(u32);
> >  	u32 val;
> > -	void __iomem *base = ntb->reg;
> > +	void __iomem *base = (void __iomem *)ntb->reg;
> > 
> > -	val = readl(base + off + ct + idx * 4);
> > +	val = readl(base + off + ct + idx * sizeof(u32));
> >  	return val;
> >  }
> > 
> > @@ -1096,10 +1115,10 @@ static int vntb_epf_spad_write(struct ntb_dev
> > *ndev, int idx, u32 val)
> >  {
> >  	struct epf_ntb *ntb = ntb_ndev(ndev);
> >  	struct epf_ntb_ctrl *ctrl = ntb->reg;
> > -	int off = ctrl->spad_offset, ct = ctrl->spad_count * 4;
> > -	void __iomem *base = ntb->reg;
> > +	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> > +	void __iomem *base = (void __iomem *)ntb->reg;
> > 
> > -	writel(val, base + off + ct + idx * 4);
> > +	writel(val, base + off + ct + idx * sizeof(u32));
> >  	return 0;
> >  }
> > 
> > @@ -1108,10 +1127,10 @@ static u32 vntb_epf_peer_spad_read(struct
> > ntb_dev *ndev, int pidx, int idx)
> >  	struct epf_ntb *ntb = ntb_ndev(ndev);
> >  	struct epf_ntb_ctrl *ctrl = ntb->reg;
> >  	int off = ctrl->spad_offset;
> > -	void __iomem *base = ntb->reg;
> > +	void __iomem *base = (void __iomem *)ntb->reg;
> >  	u32 val;
> > 
> > -	val = readl(base + off + idx * 4);
> > +	val = readl(base + off + idx * sizeof(u32));
> >  	return val;
> >  }
> > 
> > @@ -1120,9 +1139,9 @@ static int vntb_epf_peer_spad_write(struct
> > ntb_dev *ndev, int pidx, int idx, u32
> >  	struct epf_ntb *ntb = ntb_ndev(ndev);
> >  	struct epf_ntb_ctrl *ctrl = ntb->reg;
> >  	int off = ctrl->spad_offset;
> > -	void __iomem *base = ntb->reg;
> > +	void __iomem *base = (void __iomem *)ntb->reg;
> > 
> > -	writel(val, base + off + idx * 4);
> > +	writel(val, base + off + idx * sizeof(u32));
> >  	return 0;
> >  }
> > 
> > @@ -1275,6 +1294,8 @@ static struct pci_driver vntb_pci_driver = {
> >   * Invoked when a primary interface or secondary interface is bound to EPC
> >   * device. This function will succeed only when EPC is bound to both the
> >   * interfaces.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_bind(struct pci_epf *epf)
> >  {
> > @@ -1359,6 +1380,8 @@ static struct pci_epf_ops epf_ntb_ops = {
> >   *
> >   * Probe NTB function driver when endpoint function bus detects a NTB
> >   * endpoint function.
> > + *
> > + * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_probe(struct pci_epf *epf)
> >  {
> > --
> > 2.35.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support
  2022-09-22 16:12 ` [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support Frank Li
@ 2022-10-07  9:15   ` Lorenzo Pieralisi
  2022-10-07 14:54     ` [EXT] " Frank Li
  0 siblings, 1 reply; 13+ messages in thread
From: Lorenzo Pieralisi @ 2022-10-07  9:15 UTC (permalink / raw)
  To: Frank Li
  Cc: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas, linux-kernel, devicetree, linux-arm-kernel,
	linux-pci, peng.fan, aisheng.dong, jdmason, kernel, festevam,
	linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
	manivannan.sadhasivam

On Thu, Sep 22, 2022 at 11:12:46AM -0500, Frank Li wrote:
>                       ┌───────┐                   ┌──────────┐
>                       │       │                   │          │
>     ┌─────────────┐   │ PCI   │                   │ PCI Host │
>     │ MSI         │◄┐ │ EP    │                   │          │
>     │ Controller  │ │ │       │ 3.MSI Write       │          │
>     └────────┬────┘ └─┼───────┼───────────────────┤          │
>       ▲      │        │       │                   ├─BAR_n    │
>       │      └────────┼───────┼──────────────────►│          │
>       │               │       │ 2.Call Back       │          │
>       │               │       │   write_msi_msg() │          │
>       │               │       │                   │          │
>       │               └───┬───┘                   └──────────┘
>       │                   │
>       └───────────────────┘
>       1.platform_msi_domain_alloc_irqs()
> 
> There is no defined way of raising IRQs by PCI host to the PCI endpoint.
> Only define MSI/MSI-X to let EP notified RC status change.

This picture is misleading, especially (2). IIUC all this patch is
doing is implementing an NTB DB in the EP, that's it, we should
reword the commit log as such.

We are in the merge window - it is very likely this patch should
be postponed to v6.2, I didn't notice that the IRQchip changes
went in - apologies.

> The memory assigned for BAR region by the PCI host is mapped to the
> message address of platform msi interrupt controller in PCI Endpoint.
> Such that, whenever the PCI host writes to the BAR region, it will
> trigger an IRQ in the Endpoint.
> 
> Basic working follow as
> 1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
> 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;
> 3. Host side trigger an IRQ in Endpoint by write to BAR region.
> 
> Add MSI support for pci-epf-vntb. Query if system has an MSI controller.
> Set up doorbell address according to struct msi_msg.
> 
> So PCI RC can write this doorbell address to trigger EP side's IRQ.
> 
> If no MSI controller exists, fall back to software polling.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 148 +++++++++++++++---
>  1 file changed, 127 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index acea753af29ed..8fdeac2201e29 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;
>  };
>  
>  #define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb, group)
> @@ -256,10 +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_event(&ntb->ntb, i);
> -			ntb->epf_db[i] = 0;
> +	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;
> +			}
>  		}
>  	}
>  
> @@ -464,7 +471,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>  
>  	for (i = 0; i < ntb->db_count; i++) {
>  		ntb->reg->db_data[i] = 1 + i;
> -		ntb->reg->db_offset[i] = 0;
> +		ntb->reg->db_offset[i] = sizeof(u32) * i;

Why sizeof(u32) ?

>  	}
>  
>  	return 0;
> @@ -517,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;

Same question.

> +	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)
> +		size = 128;
> +
> +	if (align)
> +		size = ALIGN(size, align);
> +	else
> +		size = roundup_pow_of_two(size);
> +
> +	return size;
> +}
> +
>  /**
>   * epf_ntb_db_bar_init() - Configure Doorbell window BARs
>   * @ntb: NTB device that facilitates communication between HOST and VHOST
> @@ -540,27 +569,26 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  					    ntb->epf->func_no,
>  					    ntb->epf->vfunc_no);
>  	align = epc_features->align;
> -
> -	if (size < 128)
> -		size = 128;
> -
> -	if (align)
> -		size = ALIGN(size, align);
> -	else
> -		size = roundup_pow_of_two(size);
> +	size = epf_ntb_db_size(ntb);
>  
>  	barno = ntb->epf_ntb_bar[BAR_DB];
> +	epf_bar = &ntb->epf->bar[barno];
>  
> -	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> -	if (!mw_addr) {
> -		dev_err(dev, "Failed to allocate OB address\n");
> -		return -ENOMEM;
> +	if (ntb->epf_db_phys) {
> +		mw_addr = NULL;
> +		epf_bar->phys_addr = ntb->epf_db_phys;
> +		epf_bar->barno = barno;
> +		epf_bar->size = size;
> +	} else {
> +		mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> +		if (!mw_addr) {
> +			dev_err(dev, "Failed to allocate doorbell address\n");
> +			return -ENOMEM;
> +		}
>  	}
>  
>  	ntb->epf_db = mw_addr;
>  
> -	epf_bar = &ntb->epf->bar[barno];
> -
>  	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, epf_bar);
>  	if (ret) {
>  		dev_err(dev, "Doorbell BAR set failed\n");
> @@ -719,6 +747,83 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +static void epf_ntb_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct epf_ntb *ntb = dev_get_drvdata(desc->dev);
> +	struct epf_ntb_ctrl *reg = ntb->reg;
> +	int size = epf_ntb_db_size(ntb);
> +	u64 addr;
> +
> +	addr = msg->address_hi;
> +	addr <<= 32;
> +	addr |= msg->address_lo;
> +
> +	reg->db_data[desc->msi_index] = msg->data;
> +
> +	if (!desc->msi_index)
> +		ntb->epf_db_phys = round_down(addr, size);
> +
> +	reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phys;
> +}
> +#endif

Can we move this hunk down into the same #ifdef guard please ?

> +static irqreturn_t epf_ntb_interrupt_handler(int irq, void *data)
> +{
> +	struct epf_ntb *ntb = data;
> +	int index;
> +
> +	index = irq - ntb->msi_virqbase;
> +	ntb->db |= 1 << (index - 1);
> +	ntb_db_event(&ntb->ntb, index);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
> +{
> +	struct device *dev = &ntb->epf->dev;
> +	struct irq_domain *domain;
> +	int virq;
> +	int ret;
> +	int i;
> +
> +	domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
> +	if (!domain)
> +		return;
> +
> +	dev_set_msi_domain(dev, domain);
> +
> +	if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
> +		ntb->db_count,
> +		epf_ntb_write_msi_msg)) {
> +		dev_err(dev, "Can't allocate MSI, falling back to polling mode\n");
> +		return;
> +	}
> +	dev_info(dev, "Using MSI as doorbell\n");

Is it really useful to print this in the kernel log ? dev_dbg seems more
suitable to me.

> +
> +	for (i = 0; i < ntb->db_count; i++) {
> +		virq = msi_get_virq(dev, i);
> +		ret = devm_request_irq(dev, virq,
> +			       epf_ntb_interrupt_handler, 0,
> +			       "pci_epf_vntb", ntb);
> +
> +		if (ret) {
> +			dev_err(dev, "Failed to request doorbell IRQ! Falling back to polling mode");
> +			ntb->epf_db_phys = 0;
> +			break;

Doesn't this require a platform_msi_domain_free_irqs() ?

Thanks,
Lorenzo

> +		}
> +
> +		if (!i)
> +			ntb->msi_virqbase = virq; /* msi start virq number */
> +	}
> +}
> +#else
> +static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
> +{
> +}
> +#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
>  /**
>   * epf_ntb_epc_init() - Initialize NTB interface
>   * @ntb: NTB device that facilitates communication between HOST and VHOST
> @@ -1320,14 +1425,15 @@ static int epf_ntb_bind(struct pci_epf *epf)
>  		goto err_bar_alloc;
>  	}
>  
> +	epf_set_drvdata(epf, ntb);
> +	epf_ntb_epc_msi_init(ntb);
> +
>  	ret = epf_ntb_epc_init(ntb);
>  	if (ret) {
>  		dev_err(dev, "Failed to initialize EPC\n");
>  		goto err_bar_alloc;
>  	}
>  
> -	epf_set_drvdata(epf, ntb);
> -
>  	pci_space[0] = (ntb->vntb_pid << 16) | ntb->vntb_vid;
>  	pci_vntb_table[0].vendor = ntb->vntb_vid;
>  	pci_vntb_table[0].device = ntb->vntb_pid;
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [EXT] Re: [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support
  2022-10-07  9:15   ` Lorenzo Pieralisi
@ 2022-10-07 14:54     ` Frank Li
  0 siblings, 0 replies; 13+ messages in thread
From: Frank Li @ 2022-10-07 14:54 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
	kw, bhelgaas, linux-kernel, devicetree, linux-arm-kernel,
	linux-pci, Peng Fan, Aisheng Dong, jdmason, kernel, festevam,
	dl-linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
	manivannan.sadhasivam



> -----Original Message-----
> From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Sent: Friday, October 7, 2022 4:16 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: maz@kernel.org; tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: [EXT] Re: [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support
> 
> Caution: EXT Email
> 
> On Thu, Sep 22, 2022 at 11:12:46AM -0500, Frank Li wrote:
> >                       ┌───────┐                   ┌──────────┐
> >                       │       │                   │          │
> >     ┌─────────────┐   │ PCI   │                   │ PCI Host │
> >     │ MSI         │◄┐ │ EP    │                   │          │
> >     │ Controller  │ │ │       │ 3.MSI Write       │          │
> >     └────────┬────┘ └─┼───────┼────
> ───────────────┤          │
> >       ▲      │        │       │                   ├─BAR_n    │
> >       │      └────────┼───────┼─────────
> ─────────►│          │
> >       │               │       │ 2.Call Back       │          │
> >       │               │       │   write_msi_msg() │          │
> >       │               │       │                   │          │
> >       │               └───┬───┘                   └──────────┘
> >       │                   │
> >       └───────────────────┘
> >       1.platform_msi_domain_alloc_irqs()
> >
> > There is no defined way of raising IRQs by PCI host to the PCI endpoint.
> > Only define MSI/MSI-X to let EP notified RC status change.
> 
> This picture is misleading, especially (2). 
[Frank Li] what's your suggestion? (2) Move into PCI-EP block 
Or totally remove picture?  

> IIUC all this patch is
> doing is implementing an NTB DB in the EP, that's it, we should
> reword the commit log as such.

[Frank Li] Previously use poll method as NTB DB. 
Now add use Platform MSI as DB.  
what you specific suggestion? 

> 
> We are in the merge window - it is very likely this patch should
> be postponed to v6.2, I didn't notice that the IRQchip changes
> went in - apologies.
> 
> > The memory assigned for BAR region by the PCI host is mapped to the
> > message address of platform msi interrupt controller in PCI Endpoint.
> > Such that, whenever the PCI host writes to the BAR region, it will
> > trigger an IRQ in the Endpoint.
> >
> > Basic working follow as
> > 1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
> > 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;
> > 3. Host side trigger an IRQ in Endpoint by write to BAR region.
> >
> > Add MSI support for pci-epf-vntb. Query if system has an MSI controller.
> > Set up doorbell address according to struct msi_msg.
> >
> > So PCI RC can write this doorbell address to trigger EP side's IRQ.
> >
> > If no MSI controller exists, fall back to software polling.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 148 +++++++++++++++---
> >  1 file changed, 127 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index acea753af29ed..8fdeac2201e29 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;
> >  };
> >
> >  #define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb,
> group)
> > @@ -256,10 +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_event(&ntb->ntb, i);
> > -                     ntb->epf_db[i] = 0;
> > +     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;
> > +                     }
> >               }
> >       }
> >
> > @@ -464,7 +471,7 @@ static int epf_ntb_config_spad_bar_alloc(struct
> epf_ntb *ntb)
> >
> >       for (i = 0; i < ntb->db_count; i++) {
> >               ntb->reg->db_data[i] = 1 + i;
> > -             ntb->reg->db_offset[i] = 0;
> > +             ntb->reg->db_offset[i] = sizeof(u32) * i;
> 
> Why sizeof(u32) ?

[Frank Li] In https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/ntb/hw/epf/ntb_hw_epf.c
Line 488:
	writel(db_data, ndev->db_reg + (db_entry_size * interrupt_num) +
	       db_offset);

We now set db_entry_size is 0. 
Directly use db_offset as DB register offset.  
Each DB register is u32.

> 
> >       }
> >
> >       return 0;
> > @@ -517,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;
> 

[Frank Li] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/ntb/hw/epf/ntb_hw_epf.c
Use u32 as DB register size.  

> Same question.
> 
> > +     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)
> > +             size = 128;
> > +
> > +     if (align)
> > +             size = ALIGN(size, align);
> > +     else
> > +             size = roundup_pow_of_two(size);
> > +
> > +     return size;
> > +}
> > +
> >  /**
> >   * epf_ntb_db_bar_init() - Configure Doorbell window BARs
> >   * @ntb: NTB device that facilitates communication between HOST and
> VHOST
> > @@ -540,27 +569,26 @@ static int epf_ntb_db_bar_init(struct epf_ntb
> *ntb)
> >                                           ntb->epf->func_no,
> >                                           ntb->epf->vfunc_no);
> >       align = epc_features->align;
> > -
> > -     if (size < 128)
> > -             size = 128;
> > -
> > -     if (align)
> > -             size = ALIGN(size, align);
> > -     else
> > -             size = roundup_pow_of_two(size);
> > +     size = epf_ntb_db_size(ntb);
> >
> >       barno = ntb->epf_ntb_bar[BAR_DB];
> > +     epf_bar = &ntb->epf->bar[barno];
> >
> > -     mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> > -     if (!mw_addr) {
> > -             dev_err(dev, "Failed to allocate OB address\n");
> > -             return -ENOMEM;
> > +     if (ntb->epf_db_phys) {
> > +             mw_addr = NULL;
> > +             epf_bar->phys_addr = ntb->epf_db_phys;
> > +             epf_bar->barno = barno;
> > +             epf_bar->size = size;
> > +     } else {
> > +             mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> > +             if (!mw_addr) {
> > +                     dev_err(dev, "Failed to allocate doorbell address\n");
> > +                     return -ENOMEM;
> > +             }
> >       }
> >
> >       ntb->epf_db = mw_addr;
> >
> > -     epf_bar = &ntb->epf->bar[barno];
> > -
> >       ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf-
> >vfunc_no, epf_bar);
> >       if (ret) {
> >               dev_err(dev, "Doorbell BAR set failed\n");
> > @@ -719,6 +747,83 @@ static int epf_ntb_init_epc_bar(struct epf_ntb
> *ntb)
> >       return 0;
> >  }
> >
> > +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> > +static void epf_ntb_write_msi_msg(struct msi_desc *desc, struct msi_msg
> *msg)
> > +{
> > +     struct epf_ntb *ntb = dev_get_drvdata(desc->dev);
> > +     struct epf_ntb_ctrl *reg = ntb->reg;
> > +     int size = epf_ntb_db_size(ntb);
> > +     u64 addr;
> > +
> > +     addr = msg->address_hi;
> > +     addr <<= 32;
> > +     addr |= msg->address_lo;
> > +
> > +     reg->db_data[desc->msi_index] = msg->data;
> > +
> > +     if (!desc->msi_index)
> > +             ntb->epf_db_phys = round_down(addr, size);
> > +
> > +     reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phys;
> > +}
> > +#endif
> 
> Can we move this hunk down into the same #ifdef guard please ?
> 
> > +static irqreturn_t epf_ntb_interrupt_handler(int irq, void *data)
> > +{
> > +     struct epf_ntb *ntb = data;
> > +     int index;
> > +
> > +     index = irq - ntb->msi_virqbase;
> > +     ntb->db |= 1 << (index - 1);
> > +     ntb_db_event(&ntb->ntb, index);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> > +static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
> > +{
> > +     struct device *dev = &ntb->epf->dev;
> > +     struct irq_domain *domain;
> > +     int virq;
> > +     int ret;
> > +     int i;
> > +
> > +     domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
> > +     if (!domain)
> > +             return;
> > +
> > +     dev_set_msi_domain(dev, domain);
> > +
> > +     if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
> > +             ntb->db_count,
> > +             epf_ntb_write_msi_msg)) {
> > +             dev_err(dev, "Can't allocate MSI, falling back to polling mode\n");
> > +             return;
> > +     }
> > +     dev_info(dev, "Using MSI as doorbell\n");
> 
> Is it really useful to print this in the kernel log ? dev_dbg seems more
> suitable to me.
> 
> > +
> > +     for (i = 0; i < ntb->db_count; i++) {
> > +             virq = msi_get_virq(dev, i);
> > +             ret = devm_request_irq(dev, virq,
> > +                            epf_ntb_interrupt_handler, 0,
> > +                            "pci_epf_vntb", ntb);
> > +
> > +             if (ret) {
> > +                     dev_err(dev, "Failed to request doorbell IRQ! Falling back to
> polling mode");
> > +                     ntb->epf_db_phys = 0;
> > +                     break;
> 
> Doesn't this require a platform_msi_domain_free_irqs() ?
[Frank Li] you are right.

> 
> Thanks,
> Lorenzo
> 
> > +             }
> > +
> > +             if (!i)
> > +                     ntb->msi_virqbase = virq; /* msi start virq number */
> > +     }
> > +}
> > +#else
> > +static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
> > +{
> > +}
> > +#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
> >  /**
> >   * epf_ntb_epc_init() - Initialize NTB interface
> >   * @ntb: NTB device that facilitates communication between HOST and
> VHOST
> > @@ -1320,14 +1425,15 @@ static int epf_ntb_bind(struct pci_epf *epf)
> >               goto err_bar_alloc;
> >       }
> >
> > +     epf_set_drvdata(epf, ntb);
> > +     epf_ntb_epc_msi_init(ntb);
> > +
> >       ret = epf_ntb_epc_init(ntb);
> >       if (ret) {
> >               dev_err(dev, "Failed to initialize EPC\n");
> >               goto err_bar_alloc;
> >       }
> >
> > -     epf_set_drvdata(epf, ntb);
> > -
> >       pci_space[0] = (ntb->vntb_pid << 16) | ntb->vntb_vid;
> >       pci_vntb_table[0].vendor = ntb->vntb_vid;
> >       pci_vntb_table[0].device = ntb->vntb_pid;
> > --
> > 2.35.1
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up
@ 2022-10-07 19:13 Frank Li
  0 siblings, 0 replies; 13+ messages in thread
From: Frank Li @ 2022-10-07 19:13 UTC (permalink / raw)
  To: imx, Jon Mason, Dave Jiang, Allen Hubbe, Kishon Vijay Abraham I,
	Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Bjorn Helgaas, open list:NTB DRIVER CORE,
	open list:PCI ENDPOINT SUBSYSTEM, open list

Remove unused field: epf_db_phy.
Remove __iomem before epf_db.
Change epf_db to u32* from void *
Remove duplicate check if (readl(ntb->epf_db + i * 4)).
Using sizeof(u32) instead of number 4 at all place.

Clean up sparse build warning:
  Using  epf_db[i] instead of readl() because epf_db is located in local
  memory and allocated by dma_alloc_coherent(). Sparse build warning when
  there are not __iomem at readl().
  Added __iomem force type convert in vntb_epf_peer_spad_read\write() and
  vntb_epf_spad_read\write(). This require strong order at read and write.

Replace pci_epc_mem_free_addr() with pci_epf_free_space() at error handle
path to match pci_epf_alloc_space().

Cleanup warning found by scripts/kernel-doc
Fix indentation of the struct epf_ntb_ctrl
Consolidate term
  host, host1 to HOST
  vhost, vHost, Vhost, VHOST2 to VHOST

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 1466dd1904175..acea753af29ed 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -11,7 +11,7 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
-/**
+/*
  * +------------+         +---------------------------------------+
  * |            |         |                                       |
  * +------------+         |                        +--------------+
@@ -99,20 +99,20 @@ enum epf_ntb_bar {
  *       NTB Driver               NTB Driver
  */
 struct epf_ntb_ctrl {
-	u32     command;
-	u32     argument;
-	u16     command_status;
-	u16     link_status;
-	u32     topology;
-	u64     addr;
-	u64     size;
-	u32     num_mws;
-	u32	reserved;
-	u32     spad_offset;
-	u32     spad_count;
-	u32	db_entry_size;
-	u32     db_data[MAX_DB_COUNT];
-	u32     db_offset[MAX_DB_COUNT];
+	u32 command;
+	u32 argument;
+	u16 command_status;
+	u16 link_status;
+	u32 topology;
+	u64 addr;
+	u64 size;
+	u32 num_mws;
+	u32 reserved;
+	u32 spad_offset;
+	u32 spad_count;
+	u32 db_entry_size;
+	u32 db_data[MAX_DB_COUNT];
+	u32 db_offset[MAX_DB_COUNT];
 } __packed;
 
 struct epf_ntb {
@@ -136,8 +136,7 @@ struct epf_ntb {
 
 	struct epf_ntb_ctrl *reg;
 
-	phys_addr_t epf_db_phy;
-	void __iomem *epf_db;
+	u32 *epf_db;
 
 	phys_addr_t vpci_mw_phy[MAX_MW];
 	void __iomem *vpci_mw_addr[MAX_MW];
@@ -156,12 +155,14 @@ static struct pci_epf_header epf_ntb_header = {
 };
 
 /**
- * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host
+ * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
  * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @link_up: true or false indicating Link is UP or Down
  *
  * Once NTB function in HOST invoke ntb_link_enable(),
- * this NTB function driver will trigger a link event to vhost.
+ * this NTB function driver will trigger a link event to VHOST.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
 {
@@ -175,9 +176,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
 }
 
 /**
- * epf_ntb_configure_mw() - Configure the Outbound Address Space for vhost
- *   to access the memory window of host
- * @ntb: NTB device that facilitates communication between host and vhost
+ * epf_ntb_configure_mw() - Configure the Outbound Address Space for VHOST
+ *   to access the memory window of HOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @mw: Index of the memory window (either 0, 1, 2 or 3)
  *
  *                          EP Outbound Window
@@ -194,7 +195,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
  * |        |              |           |
  * |        |              |           |
  * +--------+              +-----------+
- *  VHost                   PCI EP
+ *  VHOST                   PCI EP
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
 {
@@ -219,7 +222,7 @@ static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
 
 /**
  * epf_ntb_teardown_mw() - Teardown the configured OB ATU
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @mw: Index of the memory window (either 0, 1, 2 or 3)
  *
  * Teardown the configured OB ATU configured in epf_ntb_configure_mw() using
@@ -234,12 +237,12 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw)
 }
 
 /**
- * epf_ntb_cmd_handler() - Handle commands provided by the NTB Host
+ * epf_ntb_cmd_handler() - Handle commands provided by the NTB HOST
  * @work: work_struct for the epf_ntb_epc
  *
  * Workqueue function that gets invoked for the two epf_ntb_epc
  * periodically (once every 5ms) to see if it has received any commands
- * from NTB host. The host can send commands to configure doorbell or
+ * from NTB HOST. The HOST can send commands to configure doorbell or
  * configure memory window or to update link status.
  */
 static void epf_ntb_cmd_handler(struct work_struct *work)
@@ -254,12 +257,9 @@ 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 (readl(ntb->epf_db + i * 4)) {
-			if (readl(ntb->epf_db + i * 4))
-				ntb->db |= 1 << (i - 1);
-
+		if (ntb->epf_db[i]) {
 			ntb_db_event(&ntb->ntb, i);
-			writel(0, ntb->epf_db + i * 4);
+			ntb->epf_db[i] = 0;
 		}
 	}
 
@@ -321,8 +321,8 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 
 /**
  * epf_ntb_config_sspad_bar_clear() - Clear Config + Self scratchpad BAR
- * @ntb_epc: EPC associated with one of the HOST which holds peer's outbound
- *	     address.
+ * @ntb: EPC associated with one of the HOST which holds peer's outbound
+ *	 address.
  *
  * Clear BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
  * self scratchpad region (removes inbound ATU configuration). While BAR0 is
@@ -331,8 +331,10 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
  * used for self scratchpad from epf_ntb_bar[BAR_CONFIG].
  *
  * Please note the self scratchpad region and config region is combined to
- * a single region and mapped using the same BAR. Also note HOST2's peer
- * scratchpad is HOST1's self scratchpad.
+ * a single region and mapped using the same BAR. Also note VHOST's peer
+ * scratchpad is HOST's self scratchpad.
+ *
+ * Returns: void
  */
 static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
 {
@@ -347,13 +349,15 @@ static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_config_sspad_bar_set() - Set Config + Self scratchpad BAR
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
- * Map BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
+ * Map BAR0 of EP CONTROLLER which contains the VHOST's config and
  * self scratchpad region.
  *
  * Please note the self scratchpad region and config region is combined to
  * a single region and mapped using the same BAR.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
 {
@@ -380,7 +384,7 @@ static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
 /**
  * epf_ntb_config_spad_bar_free() - Free the physical memory associated with
  *   config + scratchpad region
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
 {
@@ -393,11 +397,13 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
 /**
  * epf_ntb_config_spad_bar_alloc() - Allocate memory for config + scratchpad
  *   region
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Allocate the Local Memory mentioned in the above diagram. The size of
  * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION
  * is obtained from "spad-count" configfs entry.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 {
@@ -424,7 +430,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	spad_count = ntb->spad_count;
 
 	ctrl_size = sizeof(struct epf_ntb_ctrl);
-	spad_size = 2 * spad_count * 4;
+	spad_size = 2 * spad_count * sizeof(u32);
 
 	if (!align) {
 		ctrl_size = roundup_pow_of_two(ctrl_size);
@@ -454,7 +460,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	ctrl->num_mws = ntb->num_mws;
 	ntb->spad_size = spad_size;
 
-	ctrl->db_entry_size = 4;
+	ctrl->db_entry_size = sizeof(u32);
 
 	for (i = 0; i < ntb->db_count; i++) {
 		ntb->reg->db_data[i] = 1 + i;
@@ -465,11 +471,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 }
 
 /**
- * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capaiblity
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capability
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Configure MSI/MSI-X capability for each interface with number of
  * interrupts equal to "db_count" configfs entry.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 {
@@ -511,18 +519,22 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_db_bar_init() - Configure Doorbell window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 {
 	const struct pci_epc_features *epc_features;
-	u32 align;
 	struct device *dev = &ntb->epf->dev;
-	int ret;
 	struct pci_epf_bar *epf_bar;
-	void __iomem *mw_addr;
 	enum pci_barno barno;
-	size_t size = 4 * ntb->db_count;
+	void *mw_addr;
+	size_t size;
+	u32 align;
+	int ret;
+
+	size = sizeof(u32) * ntb->db_count;
 
 	epc_features = pci_epc_get_features(ntb->epf->epc,
 					    ntb->epf->func_no,
@@ -557,14 +569,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	return ret;
 
 err_alloc_peer_mem:
-	pci_epc_mem_free_addr(ntb->epf->epc, epf_bar->phys_addr, mw_addr, epf_bar->size);
+	pci_epf_free_space(ntb->epf, mw_addr, barno, 0);
 	return -1;
 }
 
 /**
  * epf_ntb_db_bar_clear() - Clear doorbell BAR and free memory
  *   allocated in peer's outbound address space
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 {
@@ -580,8 +592,9 @@ static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_mw_bar_init() - Configure Memory window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 {
@@ -629,7 +642,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_mw_bar_clear() - Clear Memory window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb)
 {
@@ -652,7 +665,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB interfaces
  */
@@ -665,7 +678,9 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
 /**
  * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
  * constructs (scratchpad region, doorbell, memorywindow)
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 {
@@ -706,11 +721,13 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_init() - Initialize NTB interface
- * @ntb: NTB device that facilitates communication between HOST and vHOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper to initialize a particular EPC interface and start the workqueue
- * to check for commands from host. This function will write to the
+ * to check for commands from HOST. This function will write to the
  * EP controller HW for configuring it.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_epc_init(struct epf_ntb *ntb)
 {
@@ -777,7 +794,7 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_cleanup() - Cleanup all NTB interfaces
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper to cleanup all NTB interfaces.
  */
@@ -934,6 +951,8 @@ static const struct config_item_type ntb_group_type = {
  *
  * Add configfs directory specific to NTB. This directory will hold
  * NTB specific properties like db_count, spad_count, num_mws etc.,
+ *
+ * Returns: Pointer to config_group
  */
 static struct config_group *epf_ntb_add_cfs(struct pci_epf *epf,
 					    struct config_group *group)
@@ -1084,11 +1103,11 @@ static int vntb_epf_link_enable(struct ntb_dev *ntb,
 static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
-	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * 4;
+	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
 	u32 val;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
-	val = readl(base + off + ct + idx * 4);
+	val = readl(base + off + ct + idx * sizeof(u32));
 	return val;
 }
 
@@ -1096,10 +1115,10 @@ static int vntb_epf_spad_write(struct ntb_dev *ndev, int idx, u32 val)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
-	int off = ctrl->spad_offset, ct = ctrl->spad_count * 4;
-	void __iomem *base = ntb->reg;
+	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
+	void __iomem *base = (void __iomem *)ntb->reg;
 
-	writel(val, base + off + ct + idx * 4);
+	writel(val, base + off + ct + idx * sizeof(u32));
 	return 0;
 }
 
@@ -1108,10 +1127,10 @@ static u32 vntb_epf_peer_spad_read(struct ntb_dev *ndev, int pidx, int idx)
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 	u32 val;
 
-	val = readl(base + off + idx * 4);
+	val = readl(base + off + idx * sizeof(u32));
 	return val;
 }
 
@@ -1120,9 +1139,9 @@ static int vntb_epf_peer_spad_write(struct ntb_dev *ndev, int pidx, int idx, u32
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
-	writel(val, base + off + idx * 4);
+	writel(val, base + off + idx * sizeof(u32));
 	return 0;
 }
 
@@ -1275,6 +1294,8 @@ static struct pci_driver vntb_pci_driver = {
  * Invoked when a primary interface or secondary interface is bound to EPC
  * device. This function will succeed only when EPC is bound to both the
  * interfaces.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_bind(struct pci_epf *epf)
 {
@@ -1359,6 +1380,8 @@ static struct pci_epf_ops epf_ntb_ops = {
  *
  * Probe NTB function driver when endpoint function bus detects a NTB
  * endpoint function.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_probe(struct pci_epf *epf)
 {
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-10-07 19:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 16:12 [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Frank Li
2022-09-22 16:12 ` [PATCH v12 1/6] platform-msi: export symbol platform_msi_create_irq_domain() Frank Li
2022-09-22 16:12 ` [PATCH v12 2/6] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
2022-09-22 16:12 ` [PATCH v12 3/6] irqchip: Add IMX MU MSI controller driver Frank Li
2022-09-22 16:12 ` [PATCH v12 4/6] dt-bindings: irqchip: imx mu work as msi controller Frank Li
2022-09-22 16:12 ` [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up Frank Li
2022-10-06 14:25   ` Frank Li
2022-10-07  8:43     ` Lorenzo Pieralisi
2022-09-22 16:12 ` [PATCH v12 6/6] PCI: endpoint: Add vNTB MSI support Frank Li
2022-10-07  9:15   ` Lorenzo Pieralisi
2022-10-07 14:54     ` [EXT] " Frank Li
2022-09-28 13:33 ` [PATCH v11 0/6] PCI EP driver support MSI doorbell from host Marc Zyngier
2022-10-07 19:13 [PATCH v12 5/6] PCI: endpoint: pci-epf-vntb: Clean up Frank Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).