[PATCH 1/3] irqchip: imx mu worked as msi controller

[PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

MU support generate irq by write data to a register.
This patch make mu worked as msi controller.
So MU can do doorbell by using standard msi api.

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

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5e4e50122777d..4599471d880c0 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -470,6 +470,13 @@ config IMX_INTMUX
 	help
 	  Support for the i.MX INTMUX interrupt multiplexer.
 
+config IMX_MU_MSI
+	bool "i.MX MU work as MSI controller"
+	default y if ARCH_MXC
+	select IRQ_DOMAIN
+	help
+	  MU work as MSI controller to do general doorbell
+
 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..f7193a6c1245e
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,490 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NXP MU worked 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/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/spinlock.h>
+#include <linux/dma-iommu.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
+
+
+#define IMX_MU_CHANS            4
+
+enum imx_mu_chan_type {
+	IMX_MU_TYPE_TX,         /* Tx */
+	IMX_MU_TYPE_RX,         /* Rx */
+	IMX_MU_TYPE_TXDB,       /* Tx doorbell */
+	IMX_MU_TYPE_RXDB,       /* Rx doorbell */
+};
+
+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,
+};
+
+enum imx_mu_type {
+	IMX_MU_V1,
+	IMX_MU_V2,
+	IMX_MU_V2_S4 = BIT(15),
+};
+
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+#define IMX_MU_xSR_RFn(type, x) (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[4];         /* Status Registers */
+	u32     xCR[4];         /* Control Registers */
+};
+
+struct imx_mu_msi {
+	spinlock_t		lock;
+	struct platform_device	*pdev;
+	struct irq_domain	*parent;
+	struct irq_domain	*msi_domain;
+	void __iomem		*regs;
+	phys_addr_t		msiir_addr;
+	struct imx_mu_dcfg	*cfg;
+	u32			msir_num;
+	struct imx_mu_msir	*msir;
+	u32			irqs_num;
+	unsigned long		used;
+	u32			gic_irq;
+	struct clk              *clk;
+	struct device		*pd_a;
+	struct device		*pd_b;
+	struct device_link	*pd_link_a;
+	struct device_link	*pd_link_b;
+};
+
+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;
+
+	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]);
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	return val;
+}
+
+static void imx_mu_msi_mask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
+
+	pci_msi_mask_irq(data);
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
+}
+
+static void imx_mu_msi_unmask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
+
+	pci_msi_unmask_irq(data);
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+	.name = "MU-MSI",
+	.irq_mask       = imx_mu_msi_mask_irq,
+	.irq_unmask     = imx_mu_msi_unmask_irq,
+};
+
+static struct msi_domain_ops its_pmsi_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
+		   MSI_FLAG_USE_DEF_CHIP_OPS |
+		   MSI_FLAG_PCI_MSIX),
+	.ops	= &its_pmsi_ops,
+	.chip	= &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
+	msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data->hwirq);
+	msg->data = data->hwirq;
+
+	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
+}
+
+static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+
+{
+	return IRQ_SET_MASK_OK;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+	.name			= "MU",
+	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,
+	.irq_set_affinity = imx_mu_msi_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;
+	msi_alloc_info_t *info = args;
+	int pos, err = 0;
+
+	pm_runtime_get_sync(&msi_data->pdev->dev);
+
+	WARN_ON(nr_irqs != 1);
+
+	spin_lock(&msi_data->lock);
+	pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
+	if (pos < msi_data->irqs_num)
+		__set_bit(pos, &msi_data->used);
+	else
+		err = -ENOSPC;
+	spin_unlock(&msi_data->lock);
+
+	if (err)
+		return err;
+
+	err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr + pos * 4);
+	if (err)
+		return err;
+
+	irq_domain_set_info(domain, virq, pos,
+			    &imx_mu_msi_parent_chip, msi_data,
+			    handle_simple_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);
+	int pos;
+
+	pos = d->hwirq;
+	if (pos < 0 || pos >= msi_data->irqs_num) {
+		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
+		return;
+	}
+
+	spin_lock(&msi_data->lock);
+	__clear_bit(pos, &msi_data->used);
+	spin_unlock(&msi_data->lock);
+
+	pm_runtime_put(&msi_data->pdev->dev);
+}
+
+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);
+	u32 status;
+	int i;
+
+	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
+			imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
+			generic_handle_domain_irq(msi_data->parent, i);
+		}
+	}
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
+{
+	/* Initialize MSI domain parent */
+	msi_data->parent = irq_domain_add_linear(NULL,
+						 msi_data->irqs_num,
+						 &imx_mu_msi_domain_ops,
+						 msi_data);
+	if (!msi_data->parent) {
+		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	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);
+
+	if (!msi_data->msi_domain) {
+		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
+		irq_domain_remove(msi_data->parent);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
+{
+	if (msi_data->gic_irq > 0)
+		irq_set_chained_handler_and_data(msi_data->gic_irq, NULL, NULL);
+
+	return 0;
+}
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.xTR    = 0x0,
+	.xRR    = 0x10,
+	.xSR    = {0x20, 0x20, 0x20, 0x20},
+	.xCR    = {0x24, 0x24, 0x24, 0x24},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+	.xTR    = 0x20,
+	.xRR    = 0x40,
+	.xSR    = {0x60, 0x60, 0x60, 0x60},
+	.xCR    = {0x64, 0x64, 0x64, 0x64},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
+	.type   = IMX_MU_V2,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {0xC, 0x118, 0x124, 0x12C},
+	.xCR    = {0x110, 0x114, 0x120, 0x128},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
+	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {0xC, 0x118, 0x124, 0x12C},
+	.xCR    = {0x110, 0x114, 0x120, 0x128},
+};
+
+static const struct of_device_id imx_mu_msi_ids[] = {
+	{ .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
+	{ .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
+	{ .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
+	{ .compatible = "fsl,imx8ulp-mu-msi-s4", .data = &imx_mu_cfg_imx8ulp_s4 },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
+
+static int imx_mu_msi_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct imx_mu_msi *msi_data, *priv;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	match = of_match_device(imx_mu_msi_ids, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
+
+	msi_data->cfg = (struct imx_mu_dcfg *) match->data;
+
+	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "a");
+	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, "b");
+	if (!res)
+		return -EIO;
+
+	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
+
+	msi_data->pdev = pdev;
+	msi_data->irqs_num = IMX_MU_CHANS;
+
+	msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
+	if (msi_data->gic_irq <= 0)
+		return -ENODEV;
+
+	platform_set_drvdata(pdev, msi_data);
+
+	msi_data->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(msi_data->clk)) {
+		if (PTR_ERR(msi_data->clk) != -ENOENT)
+			return PTR_ERR(msi_data->clk);
+
+		msi_data->clk = NULL;
+	}
+
+	ret = clk_prepare_enable(msi_data->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
+	if (IS_ERR(priv->pd_a))
+		return PTR_ERR(priv->pd_a);
+
+	priv->pd_link_a = device_link_add(dev, priv->pd_a,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!priv->pd_link_a) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		return -EINVAL;
+	}
+
+	priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
+	if (IS_ERR(priv->pd_b))
+		return PTR_ERR(priv->pd_b);
+
+	priv->pd_link_b = device_link_add(dev, priv->pd_b,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!priv->pd_link_b) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		return -EINVAL;
+	}
+
+	ret = imx_mu_msi_domains_init(msi_data);
+	if (ret)
+		return ret;
+
+	irq_set_chained_handler_and_data(msi_data->gic_irq,
+					 imx_mu_msi_irq_handler,
+					 msi_data);
+
+	pm_runtime_enable(dev);
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		goto disable_runtime_pm;
+	}
+
+	ret = pm_runtime_put_sync(dev);
+	if (ret < 0)
+		goto disable_runtime_pm;
+
+	clk_disable_unprepare(msi_data->clk);
+
+	return 0;
+
+disable_runtime_pm:
+	pm_runtime_disable(dev);
+	clk_disable_unprepare(msi_data->clk);
+
+	return ret;
+}
+
+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 imx_mu_msi_remove(struct platform_device *pdev)
+{
+	struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
+
+	imx_mu_msi_teardown_hwirq(msi_data);
+
+	irq_domain_remove(msi_data->msi_domain);
+	irq_domain_remove(msi_data->parent);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver imx_mu_msi_driver = {
+	.driver = {
+		.name = "imx-mu-msi",
+		.of_match_table = imx_mu_msi_ids,
+		.pm = &imx_mu_pm_ops,
+	},
+	.probe = imx_mu_msi_probe,
+	.remove = imx_mu_msi_remove,
+};
+
+module_platform_driver(imx_mu_msi_driver);
+
+MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
+MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

MU support generate irq by write data to a register.
This patch make mu worked as msi controller.
So MU can do doorbell by using standard msi api.

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

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5e4e50122777d..4599471d880c0 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -470,6 +470,13 @@ config IMX_INTMUX
 	help
 	  Support for the i.MX INTMUX interrupt multiplexer.
 
+config IMX_MU_MSI
+	bool "i.MX MU work as MSI controller"
+	default y if ARCH_MXC
+	select IRQ_DOMAIN
+	help
+	  MU work as MSI controller to do general doorbell
+
 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..f7193a6c1245e
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,490 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NXP MU worked 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/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/spinlock.h>
+#include <linux/dma-iommu.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
+
+
+#define IMX_MU_CHANS            4
+
+enum imx_mu_chan_type {
+	IMX_MU_TYPE_TX,         /* Tx */
+	IMX_MU_TYPE_RX,         /* Rx */
+	IMX_MU_TYPE_TXDB,       /* Tx doorbell */
+	IMX_MU_TYPE_RXDB,       /* Rx doorbell */
+};
+
+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,
+};
+
+enum imx_mu_type {
+	IMX_MU_V1,
+	IMX_MU_V2,
+	IMX_MU_V2_S4 = BIT(15),
+};
+
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+#define IMX_MU_xSR_RFn(type, x) (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[4];         /* Status Registers */
+	u32     xCR[4];         /* Control Registers */
+};
+
+struct imx_mu_msi {
+	spinlock_t		lock;
+	struct platform_device	*pdev;
+	struct irq_domain	*parent;
+	struct irq_domain	*msi_domain;
+	void __iomem		*regs;
+	phys_addr_t		msiir_addr;
+	struct imx_mu_dcfg	*cfg;
+	u32			msir_num;
+	struct imx_mu_msir	*msir;
+	u32			irqs_num;
+	unsigned long		used;
+	u32			gic_irq;
+	struct clk              *clk;
+	struct device		*pd_a;
+	struct device		*pd_b;
+	struct device_link	*pd_link_a;
+	struct device_link	*pd_link_b;
+};
+
+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;
+
+	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]);
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	return val;
+}
+
+static void imx_mu_msi_mask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
+
+	pci_msi_mask_irq(data);
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
+}
+
+static void imx_mu_msi_unmask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
+
+	pci_msi_unmask_irq(data);
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+	.name = "MU-MSI",
+	.irq_mask       = imx_mu_msi_mask_irq,
+	.irq_unmask     = imx_mu_msi_unmask_irq,
+};
+
+static struct msi_domain_ops its_pmsi_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
+		   MSI_FLAG_USE_DEF_CHIP_OPS |
+		   MSI_FLAG_PCI_MSIX),
+	.ops	= &its_pmsi_ops,
+	.chip	= &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
+	msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data->hwirq);
+	msg->data = data->hwirq;
+
+	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
+}
+
+static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+
+{
+	return IRQ_SET_MASK_OK;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+	.name			= "MU",
+	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,
+	.irq_set_affinity = imx_mu_msi_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;
+	msi_alloc_info_t *info = args;
+	int pos, err = 0;
+
+	pm_runtime_get_sync(&msi_data->pdev->dev);
+
+	WARN_ON(nr_irqs != 1);
+
+	spin_lock(&msi_data->lock);
+	pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
+	if (pos < msi_data->irqs_num)
+		__set_bit(pos, &msi_data->used);
+	else
+		err = -ENOSPC;
+	spin_unlock(&msi_data->lock);
+
+	if (err)
+		return err;
+
+	err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr + pos * 4);
+	if (err)
+		return err;
+
+	irq_domain_set_info(domain, virq, pos,
+			    &imx_mu_msi_parent_chip, msi_data,
+			    handle_simple_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);
+	int pos;
+
+	pos = d->hwirq;
+	if (pos < 0 || pos >= msi_data->irqs_num) {
+		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
+		return;
+	}
+
+	spin_lock(&msi_data->lock);
+	__clear_bit(pos, &msi_data->used);
+	spin_unlock(&msi_data->lock);
+
+	pm_runtime_put(&msi_data->pdev->dev);
+}
+
+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);
+	u32 status;
+	int i;
+
+	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
+			imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
+			generic_handle_domain_irq(msi_data->parent, i);
+		}
+	}
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
+{
+	/* Initialize MSI domain parent */
+	msi_data->parent = irq_domain_add_linear(NULL,
+						 msi_data->irqs_num,
+						 &imx_mu_msi_domain_ops,
+						 msi_data);
+	if (!msi_data->parent) {
+		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	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);
+
+	if (!msi_data->msi_domain) {
+		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
+		irq_domain_remove(msi_data->parent);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
+{
+	if (msi_data->gic_irq > 0)
+		irq_set_chained_handler_and_data(msi_data->gic_irq, NULL, NULL);
+
+	return 0;
+}
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.xTR    = 0x0,
+	.xRR    = 0x10,
+	.xSR    = {0x20, 0x20, 0x20, 0x20},
+	.xCR    = {0x24, 0x24, 0x24, 0x24},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+	.xTR    = 0x20,
+	.xRR    = 0x40,
+	.xSR    = {0x60, 0x60, 0x60, 0x60},
+	.xCR    = {0x64, 0x64, 0x64, 0x64},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
+	.type   = IMX_MU_V2,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {0xC, 0x118, 0x124, 0x12C},
+	.xCR    = {0x110, 0x114, 0x120, 0x128},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
+	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {0xC, 0x118, 0x124, 0x12C},
+	.xCR    = {0x110, 0x114, 0x120, 0x128},
+};
+
+static const struct of_device_id imx_mu_msi_ids[] = {
+	{ .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
+	{ .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
+	{ .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
+	{ .compatible = "fsl,imx8ulp-mu-msi-s4", .data = &imx_mu_cfg_imx8ulp_s4 },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
+
+static int imx_mu_msi_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct imx_mu_msi *msi_data, *priv;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	match = of_match_device(imx_mu_msi_ids, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
+
+	msi_data->cfg = (struct imx_mu_dcfg *) match->data;
+
+	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "a");
+	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, "b");
+	if (!res)
+		return -EIO;
+
+	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
+
+	msi_data->pdev = pdev;
+	msi_data->irqs_num = IMX_MU_CHANS;
+
+	msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
+	if (msi_data->gic_irq <= 0)
+		return -ENODEV;
+
+	platform_set_drvdata(pdev, msi_data);
+
+	msi_data->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(msi_data->clk)) {
+		if (PTR_ERR(msi_data->clk) != -ENOENT)
+			return PTR_ERR(msi_data->clk);
+
+		msi_data->clk = NULL;
+	}
+
+	ret = clk_prepare_enable(msi_data->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
+	if (IS_ERR(priv->pd_a))
+		return PTR_ERR(priv->pd_a);
+
+	priv->pd_link_a = device_link_add(dev, priv->pd_a,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!priv->pd_link_a) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		return -EINVAL;
+	}
+
+	priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
+	if (IS_ERR(priv->pd_b))
+		return PTR_ERR(priv->pd_b);
+
+	priv->pd_link_b = device_link_add(dev, priv->pd_b,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!priv->pd_link_b) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		return -EINVAL;
+	}
+
+	ret = imx_mu_msi_domains_init(msi_data);
+	if (ret)
+		return ret;
+
+	irq_set_chained_handler_and_data(msi_data->gic_irq,
+					 imx_mu_msi_irq_handler,
+					 msi_data);
+
+	pm_runtime_enable(dev);
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		goto disable_runtime_pm;
+	}
+
+	ret = pm_runtime_put_sync(dev);
+	if (ret < 0)
+		goto disable_runtime_pm;
+
+	clk_disable_unprepare(msi_data->clk);
+
+	return 0;
+
+disable_runtime_pm:
+	pm_runtime_disable(dev);
+	clk_disable_unprepare(msi_data->clk);
+
+	return ret;
+}
+
+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 imx_mu_msi_remove(struct platform_device *pdev)
+{
+	struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
+
+	imx_mu_msi_teardown_hwirq(msi_data);
+
+	irq_domain_remove(msi_data->msi_domain);
+	irq_domain_remove(msi_data->parent);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver imx_mu_msi_driver = {
+	.driver = {
+		.name = "imx-mu-msi",
+		.of_match_table = imx_mu_msi_ids,
+		.pm = &imx_mu_pm_ops,
+	},
+	.probe = imx_mu_msi_probe,
+	.remove = imx_mu_msi_remove,
+};
+
+module_platform_driver(imx_mu_msi_driver);
+
+MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
+MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1


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

[PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Frank Li]

imx mu support generate irq by write a register.
provide msi controller support so other driver
can use it by standard msi interface.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
 1 file changed, 94 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..b4ac583f60227
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
@@ -0,0 +1,94 @@
+# 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: NXP i.MX Messaging Unit (MU)
+
+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 to signal the other processor 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-facing, Processor B-facing).
+
+  MU can work as msi interrupt controller to do doorbell
+
+properties:
+  compatible:
+    oneOf:
+      - const: fsl,imx6sx-mu-msi
+      - const: fsl,imx7ulp-mu-msi
+      - const: fsl,imx8ulp-mu-msi
+      - const: fsl,imx8-mu-msi
+      - const: fsl,imx8ulp-mu-msi-s4
+      - items:
+          - const: fsl,imx8ulp-mu-msi
+      - items:
+          - enum:
+              - fsl,imx7s-mu-msi
+              - fsl,imx8mq-mu-msi
+              - fsl,imx8mm-mu-msi
+              - fsl,imx8mn-mu-msi
+              - fsl,imx8mp-mu-msi
+              - fsl,imx8qm-mu-msi
+              - fsl,imx8qxp-mu-msi
+          - const: fsl,imx6sx-mu-msi
+      - description: MU work as msi controller
+        items:
+          - enum:
+              - fsl,imx8qm-mu-msi
+              - fsl,imx8qxp-mu-msi
+          - const: fsl,imx6sx-mu-msi
+  reg:
+    maxItems: 2
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: tx
+      - const: rx
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - msi-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    lsio_mu12: msi@5d270000 {
+               compatible = "fsl,imx6sx-mu-msi-db";
+               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";
+    };
-- 
2.35.1

[PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Frank Li]

imx mu support generate irq by write a register.
provide msi controller support so other driver
can use it by standard msi interface.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
 1 file changed, 94 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..b4ac583f60227
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
@@ -0,0 +1,94 @@
+# 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: NXP i.MX Messaging Unit (MU)
+
+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 to signal the other processor 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-facing, Processor B-facing).
+
+  MU can work as msi interrupt controller to do doorbell
+
+properties:
+  compatible:
+    oneOf:
+      - const: fsl,imx6sx-mu-msi
+      - const: fsl,imx7ulp-mu-msi
+      - const: fsl,imx8ulp-mu-msi
+      - const: fsl,imx8-mu-msi
+      - const: fsl,imx8ulp-mu-msi-s4
+      - items:
+          - const: fsl,imx8ulp-mu-msi
+      - items:
+          - enum:
+              - fsl,imx7s-mu-msi
+              - fsl,imx8mq-mu-msi
+              - fsl,imx8mm-mu-msi
+              - fsl,imx8mn-mu-msi
+              - fsl,imx8mp-mu-msi
+              - fsl,imx8qm-mu-msi
+              - fsl,imx8qxp-mu-msi
+          - const: fsl,imx6sx-mu-msi
+      - description: MU work as msi controller
+        items:
+          - enum:
+              - fsl,imx8qm-mu-msi
+              - fsl,imx8qxp-mu-msi
+          - const: fsl,imx6sx-mu-msi
+  reg:
+    maxItems: 2
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: tx
+      - const: rx
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - msi-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    lsio_mu12: msi@5d270000 {
+               compatible = "fsl,imx6sx-mu-msi-db";
+               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";
+    };
-- 
2.35.1


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

[PATCH 3/3] pcie: endpoint: pci-epf-vntb: add endpoint msi support

[Frank Li]

This patch add msi support for ntb endpoint(EP) side.
EP side driver query if system have msi controller.
Setup doorbell address according to struct msi_msg.

So PCIe host can write this doorbell address to EP
side's irq.

If no msi controller exist, failback software polling.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

pci-epf-vntb.c is on ntb next branch
git://github.com/jonmason/ntb.git

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 134 +++++++++++++++---
 1 file changed, 112 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 1466dd1904175..dcaebcda4d7ad 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;
 
@@ -143,6 +144,8 @@ struct epf_ntb {
 	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)
@@ -253,7 +256,7 @@ 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++) {
+	for (i = 1; i < ntb->db_count && !ntb->epf_db_phy; i++) {
 		if (readl(ntb->epf_db + i * 4)) {
 			if (readl(ntb->epf_db + i * 4))
 				ntb->db |= 1 << (i - 1);
@@ -454,11 +457,9 @@ 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;
-
 	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] = 4 * i;
 	}
 
 	return 0;
@@ -509,6 +510,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 = 4 * 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
@@ -520,35 +543,33 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	struct device *dev = &ntb->epf->dev;
 	int ret;
 	struct pci_epf_bar *epf_bar;
-	void __iomem *mw_addr;
+	void __iomem *mw_addr = NULL;
 	enum pci_barno barno;
-	size_t size = 4 * ntb->db_count;
+	size_t size;
 
 	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);
+	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_phy) {
+		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;
+		}
+	} else {
+		epf_bar->phys_addr = ntb->epf_db_phy;
+		epf_bar->barno = barno;
+		epf_bar->size = size;
 	}
 
 	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");
@@ -704,6 +725,74 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 	return 0;
 }
 
+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 == 0)
+		ntb->epf_db_phy = round_down(addr, size);
+
+	reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phy;
+}
+
+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;
+}
+
+static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
+{
+	struct irq_domain *domain;
+	struct device *dev = &ntb->epf->dev;
+	int ret;
+	int i;
+	int virq;
+
+	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_info(dev, "Can't allocate MSI, failure back to poll mode\n");
+		return;
+	}
+
+	dev_info(dev, "vntb use 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,
+			       "ntb", ntb);
+
+		if (ret)
+			dev_err(dev, "request irq failure\n");
+
+		if (!i)
+			ntb->msi_virqbase = virq;
+	}
+}
+
 /**
  * epf_ntb_epc_init() - Initialize NTB interface
  * @ntb: NTB device that facilitates communication between HOST and vHOST2
@@ -1299,14 +1388,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

[PATCH 3/3] pcie: endpoint: pci-epf-vntb: add endpoint msi support

[Frank Li]

This patch add msi support for ntb endpoint(EP) side.
EP side driver query if system have msi controller.
Setup doorbell address according to struct msi_msg.

So PCIe host can write this doorbell address to EP
side's irq.

If no msi controller exist, failback software polling.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

pci-epf-vntb.c is on ntb next branch
git://github.com/jonmason/ntb.git

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 134 +++++++++++++++---
 1 file changed, 112 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 1466dd1904175..dcaebcda4d7ad 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;
 
@@ -143,6 +144,8 @@ struct epf_ntb {
 	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)
@@ -253,7 +256,7 @@ 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++) {
+	for (i = 1; i < ntb->db_count && !ntb->epf_db_phy; i++) {
 		if (readl(ntb->epf_db + i * 4)) {
 			if (readl(ntb->epf_db + i * 4))
 				ntb->db |= 1 << (i - 1);
@@ -454,11 +457,9 @@ 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;
-
 	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] = 4 * i;
 	}
 
 	return 0;
@@ -509,6 +510,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 = 4 * 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
@@ -520,35 +543,33 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	struct device *dev = &ntb->epf->dev;
 	int ret;
 	struct pci_epf_bar *epf_bar;
-	void __iomem *mw_addr;
+	void __iomem *mw_addr = NULL;
 	enum pci_barno barno;
-	size_t size = 4 * ntb->db_count;
+	size_t size;
 
 	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);
+	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_phy) {
+		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;
+		}
+	} else {
+		epf_bar->phys_addr = ntb->epf_db_phy;
+		epf_bar->barno = barno;
+		epf_bar->size = size;
 	}
 
 	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");
@@ -704,6 +725,74 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 	return 0;
 }
 
+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 == 0)
+		ntb->epf_db_phy = round_down(addr, size);
+
+	reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phy;
+}
+
+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;
+}
+
+static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
+{
+	struct irq_domain *domain;
+	struct device *dev = &ntb->epf->dev;
+	int ret;
+	int i;
+	int virq;
+
+	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_info(dev, "Can't allocate MSI, failure back to poll mode\n");
+		return;
+	}
+
+	dev_info(dev, "vntb use 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,
+			       "ntb", ntb);
+
+		if (ret)
+			dev_err(dev, "request irq failure\n");
+
+		if (!i)
+			ntb->msi_virqbase = virq;
+	}
+}
+
 /**
  * epf_ntb_epc_init() - Initialize NTB interface
  * @ntb: NTB device that facilitates communication between HOST and vHOST2
@@ -1299,14 +1388,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


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

Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Thu, 07 Jul 2022 22:02:36 +0100,
Frank Li <Frank.Li@nxp.com> wrote:
> 
> MU support generate irq by write data to a register.
> This patch make mu worked as msi controller.
> So MU can do doorbell by using standard msi api.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Where is the cover letter for this? How am I supposed to guess what
this series is about? The rule is simple: you send a series with two
patches or more, you include a cover letter describing your changes,
what they provide, and how they interact with each other. Here, all I
see is a bunch of seemingly unrelated changes.

I could spend the next 2 hours trying to reverse engineer it, but it
is probably a lot quicker for you to write an actual cover letter and
send it for everyone's benefit.

Thanks,

	M.

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

Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Thu, 07 Jul 2022 22:02:36 +0100,
Frank Li <Frank.Li@nxp.com> wrote:
> 
> MU support generate irq by write data to a register.
> This patch make mu worked as msi controller.
> So MU can do doorbell by using standard msi api.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Where is the cover letter for this? How am I supposed to guess what
this series is about? The rule is simple: you send a series with two
patches or more, you include a cover letter describing your changes,
what they provide, and how they interact with each other. Here, all I
see is a bunch of seemingly unrelated changes.

I could spend the next 2 hours trying to reverse engineer it, but it
is probably a lot quicker for you to write an actual cover letter and
send it for everyone's benefit.

Thanks,

	M.

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

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

Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Thu, 07 Jul 2022 22:02:36 +0100,
Frank Li <Frank.Li@nxp.com> wrote:
> 
> MU support generate irq by write data to a register.
> This patch make mu worked as msi controller.
> So MU can do doorbell by using standard msi api.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/irqchip/Kconfig          |   7 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-mu-msi.c | 490 +++++++++++++++++++++++++++++++
>  3 files changed, 498 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5e4e50122777d..4599471d880c0 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -470,6 +470,13 @@ config IMX_INTMUX
>  	help
>  	  Support for the i.MX INTMUX interrupt multiplexer.
>  
> +config IMX_MU_MSI
> +	bool "i.MX MU work as MSI controller"
> +	default y if ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  MU work as MSI controller to do general doorbell
> +
>  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..f7193a6c1245e
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP MU worked 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_domain.h>
> +
> +
> +#define IMX_MU_CHANS            4
> +
> +enum imx_mu_chan_type {
> +	IMX_MU_TYPE_TX,         /* Tx */
> +	IMX_MU_TYPE_RX,         /* Rx */
> +	IMX_MU_TYPE_TXDB,       /* Tx doorbell */
> +	IMX_MU_TYPE_RXDB,       /* Rx doorbell */

What does any of this even mean for MSIs?

> +};
> +
> +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,
> +};
> +
> +enum imx_mu_type {
> +	IMX_MU_V1,
> +	IMX_MU_V2,
> +	IMX_MU_V2_S4 = BIT(15),

If the bit assignment is significant, make it so for all members of
this enum.

> +};
> +
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +#define IMX_MU_xSR_RFn(type, x) (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[4];         /* Status Registers */
> +	u32     xCR[4];         /* Control Registers */
> +};
> +
> +struct imx_mu_msi {
> +	spinlock_t		lock;
> +	struct platform_device	*pdev;
> +	struct irq_domain	*parent;
> +	struct irq_domain	*msi_domain;
> +	void __iomem		*regs;
> +	phys_addr_t		msiir_addr;
> +	struct imx_mu_dcfg	*cfg;
> +	u32			msir_num;
> +	struct imx_mu_msir	*msir;
> +	u32			irqs_num;
> +	unsigned long		used;
> +	u32			gic_irq;
> +	struct clk              *clk;
> +	struct device		*pd_a;
> +	struct device		*pd_b;
> +	struct device_link	*pd_link_a;
> +	struct device_link	*pd_link_b;
> +};
> +
> +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;
> +
> +	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]);
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> +	return val;
> +}
> +
> +static void imx_mu_msi_mask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
> +
> +	pci_msi_mask_irq(data);

What is this? Below, you create a platform MSI domain. Either you
support PCI, and you create a PCI/MSI domain (and the above may make
sense), or you are doing platform MSI, and the above is non-sense.

> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
> +}
> +
> +static void imx_mu_msi_unmask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
> +
> +	pci_msi_unmask_irq(data);
> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
> +}
> +
> +static struct irq_chip imx_mu_msi_irq_chip = {
> +	.name = "MU-MSI",
> +	.irq_mask       = imx_mu_msi_mask_irq,
> +	.irq_unmask     = imx_mu_msi_unmask_irq,
> +};
> +
> +static struct msi_domain_ops its_pmsi_ops = {
> +};
> +
> +static struct msi_domain_info imx_mu_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
> +		   MSI_FLAG_USE_DEF_CHIP_OPS |
> +		   MSI_FLAG_PCI_MSIX),
> +	.ops	= &its_pmsi_ops,
> +	.chip	= &imx_mu_msi_irq_chip,
> +};
> +
> +static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> +	msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data->hwirq);
> +	msg->data = data->hwirq;
> +
> +	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
> +}
> +
> +static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
> +				   const struct cpumask *mask, bool force)
> +
> +{
> +	return IRQ_SET_MASK_OK;
> +}
> +
> +static struct irq_chip imx_mu_msi_parent_chip = {
> +	.name			= "MU",
> +	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,
> +	.irq_set_affinity = imx_mu_msi_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;
> +	msi_alloc_info_t *info = args;
> +	int pos, err = 0;
> +
> +	pm_runtime_get_sync(&msi_data->pdev->dev);

The core code already deals with runtime PM. What prevents it from
working, other than the fact you don't populate the device in the
top-level domain?

> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	spin_lock(&msi_data->lock);
> +	pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
> +	if (pos < msi_data->irqs_num)
> +		__set_bit(pos, &msi_data->used);
> +	else
> +		err = -ENOSPC;
> +	spin_unlock(&msi_data->lock);
> +
> +	if (err)
> +		return err;
> +
> +	err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr + pos * 4);
> +	if (err)
> +		return err;
> +
> +	irq_domain_set_info(domain, virq, pos,
> +			    &imx_mu_msi_parent_chip, msi_data,
> +			    handle_simple_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);
> +	int pos;
> +
> +	pos = d->hwirq;
> +	if (pos < 0 || pos >= msi_data->irqs_num) {
> +		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> +		return;
> +	}

How can this happen?

> +
> +	spin_lock(&msi_data->lock);
> +	__clear_bit(pos, &msi_data->used);
> +	spin_unlock(&msi_data->lock);
> +
> +	pm_runtime_put(&msi_data->pdev->dev);
> +}
> +
> +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);
> +	u32 status;
> +	int i;
> +
> +	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +	for (i = 0; i < IMX_MU_CHANS; i++) {
> +		if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
> +			imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
> +			generic_handle_domain_irq(msi_data->parent, i);
> +		}
> +	}
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> +{
> +	/* Initialize MSI domain parent */
> +	msi_data->parent = irq_domain_add_linear(NULL,

NAK. Don't create anonymous domains.

> +						 msi_data->irqs_num,
> +						 &imx_mu_msi_domain_ops,
> +						 msi_data);
> +	if (!msi_data->parent) {
> +		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	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);
> +
> +	if (!msi_data->msi_domain) {
> +		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> +		irq_domain_remove(msi_data->parent);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
> +{
> +	if (msi_data->gic_irq > 0)
> +		irq_set_chained_handler_and_data(msi_data->gic_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> +	.xTR    = 0x0,
> +	.xRR    = 0x10,
> +	.xSR    = {0x20, 0x20, 0x20, 0x20},
> +	.xCR    = {0x24, 0x24, 0x24, 0x24},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> +	.xTR    = 0x20,
> +	.xRR    = 0x40,
> +	.xSR    = {0x60, 0x60, 0x60, 0x60},
> +	.xCR    = {0x64, 0x64, 0x64, 0x64},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> +	.type   = IMX_MU_V2,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> +	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};

What are these? We really don't need more magic numbers.

> +
> +static const struct of_device_id imx_mu_msi_ids[] = {
> +	{ .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
> +	{ .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
> +	{ .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
> +	{ .compatible = "fsl,imx8ulp-mu-msi-s4", .data = &imx_mu_cfg_imx8ulp_s4 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
> +
> +static int imx_mu_msi_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct imx_mu_msi *msi_data, *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +
> +	match = of_match_device(imx_mu_msi_ids, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> +	if (!msi_data)
> +		return -ENOMEM;
> +
> +	msi_data->cfg = (struct imx_mu_dcfg *) match->data;
> +
> +	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "a");
> +	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, "b");
> +	if (!res)
> +		return -EIO;
> +
> +	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> +
> +	msi_data->pdev = pdev;
> +	msi_data->irqs_num = IMX_MU_CHANS;

If that's hardcoded, why do we need an extra variable? I also question
the usefulness of this driver if the HW can only deal with *4* MSIs...
This looks a bit like a joke.

> +
> +	msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
> +	if (msi_data->gic_irq <= 0)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, msi_data);
> +
> +	msi_data->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(msi_data->clk)) {
> +		if (PTR_ERR(msi_data->clk) != -ENOENT)
> +			return PTR_ERR(msi_data->clk);
> +
> +		msi_data->clk = NULL;
> +	}
> +
> +	ret = clk_prepare_enable(msi_data->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> +	if (IS_ERR(priv->pd_a))
> +		return PTR_ERR(priv->pd_a);
> +
> +	priv->pd_link_a = device_link_add(dev, priv->pd_a,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!priv->pd_link_a) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
> +	if (IS_ERR(priv->pd_b))
> +		return PTR_ERR(priv->pd_b);
> +
> +	priv->pd_link_b = device_link_add(dev, priv->pd_b,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!priv->pd_link_b) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = imx_mu_msi_domains_init(msi_data);
> +	if (ret)
> +		return ret;
> +
> +	irq_set_chained_handler_and_data(msi_data->gic_irq,
> +					 imx_mu_msi_irq_handler,
> +					 msi_data);
> +
> +	pm_runtime_enable(dev);
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(dev);
> +		goto disable_runtime_pm;
> +	}
> +
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0)
> +		goto disable_runtime_pm;
> +
> +	clk_disable_unprepare(msi_data->clk);
> +
> +	return 0;
> +
> +disable_runtime_pm:
> +	pm_runtime_disable(dev);
> +	clk_disable_unprepare(msi_data->clk);
> +
> +	return ret;
> +}
> +
> +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 imx_mu_msi_remove(struct platform_device *pdev)
> +{
> +	struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> +
> +	imx_mu_msi_teardown_hwirq(msi_data);
> +
> +	irq_domain_remove(msi_data->msi_domain);
> +	irq_domain_remove(msi_data->parent);

How do you ensure that no device is still holding interrupts? Let me
give you a hint: you can't. So removing an interrupt controller module
should not be possible.

> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx_mu_msi_driver = {
> +	.driver = {
> +		.name = "imx-mu-msi",
> +		.of_match_table = imx_mu_msi_ids,
> +		.pm = &imx_mu_pm_ops,
> +	},
> +	.probe = imx_mu_msi_probe,
> +	.remove = imx_mu_msi_remove,
> +};
> +
> +module_platform_driver(imx_mu_msi_driver);

Please use the standard probing methods (IRQCHIP_PLATFORM_DRIVER_BEGIN
and co).

> +
> +MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
> +MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller driver");
> +MODULE_LICENSE("GPL");

I have the ugly feeling that this driver really isn't about MSIs, but
is just a way to sneak some terrible abstraction into the kernel... I
guess we'll eventually find out. In the meantime, this driver needs
fixing.

	M.

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

Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Thu, 07 Jul 2022 22:02:36 +0100,
Frank Li <Frank.Li@nxp.com> wrote:
> 
> MU support generate irq by write data to a register.
> This patch make mu worked as msi controller.
> So MU can do doorbell by using standard msi api.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/irqchip/Kconfig          |   7 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-mu-msi.c | 490 +++++++++++++++++++++++++++++++
>  3 files changed, 498 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5e4e50122777d..4599471d880c0 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -470,6 +470,13 @@ config IMX_INTMUX
>  	help
>  	  Support for the i.MX INTMUX interrupt multiplexer.
>  
> +config IMX_MU_MSI
> +	bool "i.MX MU work as MSI controller"
> +	default y if ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  MU work as MSI controller to do general doorbell
> +
>  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..f7193a6c1245e
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP MU worked 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_domain.h>
> +
> +
> +#define IMX_MU_CHANS            4
> +
> +enum imx_mu_chan_type {
> +	IMX_MU_TYPE_TX,         /* Tx */
> +	IMX_MU_TYPE_RX,         /* Rx */
> +	IMX_MU_TYPE_TXDB,       /* Tx doorbell */
> +	IMX_MU_TYPE_RXDB,       /* Rx doorbell */

What does any of this even mean for MSIs?

> +};
> +
> +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,
> +};
> +
> +enum imx_mu_type {
> +	IMX_MU_V1,
> +	IMX_MU_V2,
> +	IMX_MU_V2_S4 = BIT(15),

If the bit assignment is significant, make it so for all members of
this enum.

> +};
> +
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +#define IMX_MU_xSR_RFn(type, x) (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[4];         /* Status Registers */
> +	u32     xCR[4];         /* Control Registers */
> +};
> +
> +struct imx_mu_msi {
> +	spinlock_t		lock;
> +	struct platform_device	*pdev;
> +	struct irq_domain	*parent;
> +	struct irq_domain	*msi_domain;
> +	void __iomem		*regs;
> +	phys_addr_t		msiir_addr;
> +	struct imx_mu_dcfg	*cfg;
> +	u32			msir_num;
> +	struct imx_mu_msir	*msir;
> +	u32			irqs_num;
> +	unsigned long		used;
> +	u32			gic_irq;
> +	struct clk              *clk;
> +	struct device		*pd_a;
> +	struct device		*pd_b;
> +	struct device_link	*pd_link_a;
> +	struct device_link	*pd_link_b;
> +};
> +
> +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;
> +
> +	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]);
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> +	return val;
> +}
> +
> +static void imx_mu_msi_mask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
> +
> +	pci_msi_mask_irq(data);

What is this? Below, you create a platform MSI domain. Either you
support PCI, and you create a PCI/MSI domain (and the above may make
sense), or you are doing platform MSI, and the above is non-sense.

> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
> +}
> +
> +static void imx_mu_msi_unmask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
> +
> +	pci_msi_unmask_irq(data);
> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
> +}
> +
> +static struct irq_chip imx_mu_msi_irq_chip = {
> +	.name = "MU-MSI",
> +	.irq_mask       = imx_mu_msi_mask_irq,
> +	.irq_unmask     = imx_mu_msi_unmask_irq,
> +};
> +
> +static struct msi_domain_ops its_pmsi_ops = {
> +};
> +
> +static struct msi_domain_info imx_mu_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
> +		   MSI_FLAG_USE_DEF_CHIP_OPS |
> +		   MSI_FLAG_PCI_MSIX),
> +	.ops	= &its_pmsi_ops,
> +	.chip	= &imx_mu_msi_irq_chip,
> +};
> +
> +static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> +	msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data->hwirq);
> +	msg->data = data->hwirq;
> +
> +	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
> +}
> +
> +static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
> +				   const struct cpumask *mask, bool force)
> +
> +{
> +	return IRQ_SET_MASK_OK;
> +}
> +
> +static struct irq_chip imx_mu_msi_parent_chip = {
> +	.name			= "MU",
> +	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,
> +	.irq_set_affinity = imx_mu_msi_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;
> +	msi_alloc_info_t *info = args;
> +	int pos, err = 0;
> +
> +	pm_runtime_get_sync(&msi_data->pdev->dev);

The core code already deals with runtime PM. What prevents it from
working, other than the fact you don't populate the device in the
top-level domain?

> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	spin_lock(&msi_data->lock);
> +	pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
> +	if (pos < msi_data->irqs_num)
> +		__set_bit(pos, &msi_data->used);
> +	else
> +		err = -ENOSPC;
> +	spin_unlock(&msi_data->lock);
> +
> +	if (err)
> +		return err;
> +
> +	err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr + pos * 4);
> +	if (err)
> +		return err;
> +
> +	irq_domain_set_info(domain, virq, pos,
> +			    &imx_mu_msi_parent_chip, msi_data,
> +			    handle_simple_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);
> +	int pos;
> +
> +	pos = d->hwirq;
> +	if (pos < 0 || pos >= msi_data->irqs_num) {
> +		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> +		return;
> +	}

How can this happen?

> +
> +	spin_lock(&msi_data->lock);
> +	__clear_bit(pos, &msi_data->used);
> +	spin_unlock(&msi_data->lock);
> +
> +	pm_runtime_put(&msi_data->pdev->dev);
> +}
> +
> +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);
> +	u32 status;
> +	int i;
> +
> +	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +	for (i = 0; i < IMX_MU_CHANS; i++) {
> +		if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
> +			imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
> +			generic_handle_domain_irq(msi_data->parent, i);
> +		}
> +	}
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> +{
> +	/* Initialize MSI domain parent */
> +	msi_data->parent = irq_domain_add_linear(NULL,

NAK. Don't create anonymous domains.

> +						 msi_data->irqs_num,
> +						 &imx_mu_msi_domain_ops,
> +						 msi_data);
> +	if (!msi_data->parent) {
> +		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	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);
> +
> +	if (!msi_data->msi_domain) {
> +		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> +		irq_domain_remove(msi_data->parent);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
> +{
> +	if (msi_data->gic_irq > 0)
> +		irq_set_chained_handler_and_data(msi_data->gic_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> +	.xTR    = 0x0,
> +	.xRR    = 0x10,
> +	.xSR    = {0x20, 0x20, 0x20, 0x20},
> +	.xCR    = {0x24, 0x24, 0x24, 0x24},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> +	.xTR    = 0x20,
> +	.xRR    = 0x40,
> +	.xSR    = {0x60, 0x60, 0x60, 0x60},
> +	.xCR    = {0x64, 0x64, 0x64, 0x64},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> +	.type   = IMX_MU_V2,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> +	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};

What are these? We really don't need more magic numbers.

> +
> +static const struct of_device_id imx_mu_msi_ids[] = {
> +	{ .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
> +	{ .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
> +	{ .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
> +	{ .compatible = "fsl,imx8ulp-mu-msi-s4", .data = &imx_mu_cfg_imx8ulp_s4 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
> +
> +static int imx_mu_msi_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct imx_mu_msi *msi_data, *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +
> +	match = of_match_device(imx_mu_msi_ids, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> +	if (!msi_data)
> +		return -ENOMEM;
> +
> +	msi_data->cfg = (struct imx_mu_dcfg *) match->data;
> +
> +	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "a");
> +	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, "b");
> +	if (!res)
> +		return -EIO;
> +
> +	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> +
> +	msi_data->pdev = pdev;
> +	msi_data->irqs_num = IMX_MU_CHANS;

If that's hardcoded, why do we need an extra variable? I also question
the usefulness of this driver if the HW can only deal with *4* MSIs...
This looks a bit like a joke.

> +
> +	msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
> +	if (msi_data->gic_irq <= 0)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, msi_data);
> +
> +	msi_data->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(msi_data->clk)) {
> +		if (PTR_ERR(msi_data->clk) != -ENOENT)
> +			return PTR_ERR(msi_data->clk);
> +
> +		msi_data->clk = NULL;
> +	}
> +
> +	ret = clk_prepare_enable(msi_data->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> +	if (IS_ERR(priv->pd_a))
> +		return PTR_ERR(priv->pd_a);
> +
> +	priv->pd_link_a = device_link_add(dev, priv->pd_a,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!priv->pd_link_a) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
> +	if (IS_ERR(priv->pd_b))
> +		return PTR_ERR(priv->pd_b);
> +
> +	priv->pd_link_b = device_link_add(dev, priv->pd_b,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!priv->pd_link_b) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = imx_mu_msi_domains_init(msi_data);
> +	if (ret)
> +		return ret;
> +
> +	irq_set_chained_handler_and_data(msi_data->gic_irq,
> +					 imx_mu_msi_irq_handler,
> +					 msi_data);
> +
> +	pm_runtime_enable(dev);
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(dev);
> +		goto disable_runtime_pm;
> +	}
> +
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0)
> +		goto disable_runtime_pm;
> +
> +	clk_disable_unprepare(msi_data->clk);
> +
> +	return 0;
> +
> +disable_runtime_pm:
> +	pm_runtime_disable(dev);
> +	clk_disable_unprepare(msi_data->clk);
> +
> +	return ret;
> +}
> +
> +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 imx_mu_msi_remove(struct platform_device *pdev)
> +{
> +	struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> +
> +	imx_mu_msi_teardown_hwirq(msi_data);
> +
> +	irq_domain_remove(msi_data->msi_domain);
> +	irq_domain_remove(msi_data->parent);

How do you ensure that no device is still holding interrupts? Let me
give you a hint: you can't. So removing an interrupt controller module
should not be possible.

> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx_mu_msi_driver = {
> +	.driver = {
> +		.name = "imx-mu-msi",
> +		.of_match_table = imx_mu_msi_ids,
> +		.pm = &imx_mu_pm_ops,
> +	},
> +	.probe = imx_mu_msi_probe,
> +	.remove = imx_mu_msi_remove,
> +};
> +
> +module_platform_driver(imx_mu_msi_driver);

Please use the standard probing methods (IRQCHIP_PLATFORM_DRIVER_BEGIN
and co).

> +
> +MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
> +MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller driver");
> +MODULE_LICENSE("GPL");

I have the ugly feeling that this driver really isn't about MSIs, but
is just a way to sneak some terrible abstraction into the kernel... I
guess we'll eventually find out. In the meantime, this driver needs
fixing.

	M.

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

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

RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Friday, July 8, 2022 3:59 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: 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
> Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> 
> Caution: EXT Email
> 
> On Thu, 07 Jul 2022 22:02:36 +0100,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > MU support generate irq by write data to a register.
> > This patch make mu worked as msi controller.
> > So MU can do doorbell by using standard msi api.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/irqchip/Kconfig          |   7 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-mu-msi.c | 490
> +++++++++++++++++++++++++++++++
> >  3 files changed, 498 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..4599471d880c0 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,13 @@ config IMX_INTMUX
> >       help
> >         Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > +     bool "i.MX MU work as MSI controller"
> > +     default y if ARCH_MXC
> > +     select IRQ_DOMAIN
> > +     help
> > +       MU work as MSI controller to do general doorbell
> > +
> >  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..f7193a6c1245e
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,490 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * NXP MU worked 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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/dma-iommu.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pm_domain.h>
> > +
> > +
> > +#define IMX_MU_CHANS            4
> > +
> > +enum imx_mu_chan_type {
> > +     IMX_MU_TYPE_TX,         /* Tx */
> > +     IMX_MU_TYPE_RX,         /* Rx */
> > +     IMX_MU_TYPE_TXDB,       /* Tx doorbell */
> > +     IMX_MU_TYPE_RXDB,       /* Rx doorbell */
> 
> What does any of this even mean for MSIs?

Sorry, I forget deleted it.

> 
> > +};
> > +
> > +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,
> > +};
> > +
> > +enum imx_mu_type {
> > +     IMX_MU_V1,
> > +     IMX_MU_V2,
> > +     IMX_MU_V2_S4 = BIT(15),
> 
> If the bit assignment is significant, make it so for all members of
> this enum.

[Frank Li] Yes, you are right.

> 
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24
> + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(type, x) (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[4];         /* Status Registers */
> > +     u32     xCR[4];         /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > +     spinlock_t              lock;
> > +     struct platform_device  *pdev;
> > +     struct irq_domain       *parent;
> > +     struct irq_domain       *msi_domain;
> > +     void __iomem            *regs;
> > +     phys_addr_t             msiir_addr;
> > +     struct imx_mu_dcfg      *cfg;
> > +     u32                     msir_num;
> > +     struct imx_mu_msir      *msir;
> > +     u32                     irqs_num;
> > +     unsigned long           used;
> > +     u32                     gic_irq;
> > +     struct clk              *clk;
> > +     struct device           *pd_a;
> > +     struct device           *pd_b;
> > +     struct device_link      *pd_link_a;
> > +     struct device_link      *pd_link_b;
> > +};
> > +
> > +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;
> > +
> > +     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]);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     return val;
> > +}
> > +
> > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> >parent_data);
> > +
> > +     pci_msi_mask_irq(data);
> 
> What is this? Below, you create a platform MSI domain. Either you
> support PCI, and you create a PCI/MSI domain (and the above may make
> sense), or you are doing platform MSI, and the above is non-sense.

[Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq()  

> 
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_unmask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> >parent_data);
> > +
> > +     pci_msi_unmask_irq(data);
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > +     .name = "MU-MSI",
> > +     .irq_mask       = imx_mu_msi_mask_irq,
> > +     .irq_unmask     = imx_mu_msi_unmask_irq,
> > +};
> > +
> > +static struct msi_domain_ops its_pmsi_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> > +                MSI_FLAG_USE_DEF_CHIP_OPS |
> > +                MSI_FLAG_PCI_MSIX),
> > +     .ops    = &its_pmsi_ops,
> > +     .chip   = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_compose_msg(struct irq_data *data, struct
> msi_msg *msg)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> > +     msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data-
> >hwirq);
> > +     msg->data = data->hwirq;
> > +
> > +     iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
> > +}
> > +
> > +static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
> > +                                const struct cpumask *mask, bool force)
> > +
> > +{
> > +     return IRQ_SET_MASK_OK;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > +     .name                   = "MU",
> > +     .irq_compose_msi_msg    = imx_mu_msi_compose_msg,
> > +     .irq_set_affinity = imx_mu_msi_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;
> > +     msi_alloc_info_t *info = args;
> > +     int pos, err = 0;
> > +
> > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> 
> The core code already deals with runtime PM. What prevents it from
> working, other than the fact you don't populate the device in the
> top-level domain?

[Frank Li]  Do you means power domain or irq domain?

> 
> > +
> > +     WARN_ON(nr_irqs != 1);
> > +
> > +     spin_lock(&msi_data->lock);
> > +     pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
> > +     if (pos < msi_data->irqs_num)
> > +             __set_bit(pos, &msi_data->used);
> > +     else
> > +             err = -ENOSPC;
> > +     spin_unlock(&msi_data->lock);
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr +
> pos * 4);
> > +     if (err)
> > +             return err;
> > +
> > +     irq_domain_set_info(domain, virq, pos,
> > +                         &imx_mu_msi_parent_chip, msi_data,
> > +                         handle_simple_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);
> > +     int pos;
> > +
> > +     pos = d->hwirq;
> > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > +             return;
> > +     }
> 
> How can this happen?

I just copy from irq-ls-scfg-msi.c
It should be impossible happen if everything work as expected. 

> 
> > +
> > +     spin_lock(&msi_data->lock);
> > +     __clear_bit(pos, &msi_data->used);
> > +     spin_unlock(&msi_data->lock);
> > +
> > +     pm_runtime_put(&msi_data->pdev->dev);
> > +}
> > +
> > +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);
> > +     u32 status;
> > +     int i;
> > +
> > +     status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > +     chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > +             if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
> > +                     imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
> > +                     generic_handle_domain_irq(msi_data->parent, i);
> > +             }
> > +     }
> > +     chained_irq_exit(irq_desc_get_chip(desc), desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> > +{
> > +     /* Initialize MSI domain parent */
> > +     msi_data->parent = irq_domain_add_linear(NULL,
> 
> NAK. Don't create anonymous domains.
[Frank Li] will fixed at next version.

> 
> > +                                              msi_data->irqs_num,
> > +                                              &imx_mu_msi_domain_ops,
> > +                                              msi_data);
> > +     if (!msi_data->parent) {
> > +             dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     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);
> > +
> > +     if (!msi_data->msi_domain) {
> > +             dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> > +             irq_domain_remove(msi_data->parent);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
> > +{
> > +     if (msi_data->gic_irq > 0)
> > +             irq_set_chained_handler_and_data(msi_data->gic_irq, NULL,
> NULL);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > +     .xTR    = 0x0,
> > +     .xRR    = 0x10,
> > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +     .xTR    = 0x20,
> > +     .xRR    = 0x40,
> > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > +     .type   = IMX_MU_V2,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> 
> What are these? We really don't need more magic numbers.

It is register offset.  The difference version MU hardware's register map is difference. 

> 
> > +
> > +static const struct of_device_id imx_mu_msi_ids[] = {
> > +     { .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
> > +     { .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
> > +     { .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
> > +     { .compatible = "fsl,imx8ulp-mu-msi-s4", .data =
> &imx_mu_cfg_imx8ulp_s4 },
> > +     { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
> > +
> > +static int imx_mu_msi_probe(struct platform_device *pdev)
> > +{
> > +     const struct of_device_id *match;
> > +     struct imx_mu_msi *msi_data, *priv;
> > +     struct device *dev = &pdev->dev;
> > +     struct resource *res;
> > +     int ret;
> > +
> > +     match = of_match_device(imx_mu_msi_ids, &pdev->dev);
> > +     if (!match)
> > +             return -ENODEV;
> > +
> > +     priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> > +     if (!msi_data)
> > +             return -ENOMEM;
> > +
> > +     msi_data->cfg = (struct imx_mu_dcfg *) match->data;
> > +
> > +     msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "a");
> > +     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, "b");
> > +     if (!res)
> > +             return -EIO;
> > +
> > +     msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > +     msi_data->pdev = pdev;
> > +     msi_data->irqs_num = IMX_MU_CHANS;
> 
> If that's hardcoded, why do we need an extra variable? I also question
> the usefulness of this driver if the HW can only deal with *4* MSIs...
> This looks a bit like a joke.

MU don't really MSI controller.  Each MU have 4 channel.  
I.MX have several MU units.  

PCI EP driver need an address as doorbell,  so PCI RC side can write
This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
Have not ITS support yet now.  So we can use MU as simple MSI controller.

So PCE EP side driver can use standard MSI method to do doorbell for
GIC-ITS,  MU and other msi controller without any modification. 

I will add such information at cover letter next version.

> 
> > +
> > +     msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
> > +     if (msi_data->gic_irq <= 0)
> > +             return -ENODEV;
> > +
> > +     platform_set_drvdata(pdev, msi_data);
> > +
> > +     msi_data->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(msi_data->clk)) {
> > +             if (PTR_ERR(msi_data->clk) != -ENOENT)
> > +                     return PTR_ERR(msi_data->clk);
> > +
> > +             msi_data->clk = NULL;
> > +     }
> > +
> > +     ret = clk_prepare_enable(msi_data->clk);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to enable clock\n");
> > +             return ret;
> > +     }
> > +
> > +     priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> > +     if (IS_ERR(priv->pd_a))
> > +             return PTR_ERR(priv->pd_a);
> > +
> > +     priv->pd_link_a = device_link_add(dev, priv->pd_a,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!priv->pd_link_a) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
> > +     if (IS_ERR(priv->pd_b))
> > +             return PTR_ERR(priv->pd_b);
> > +
> > +     priv->pd_link_b = device_link_add(dev, priv->pd_b,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!priv->pd_link_b) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = imx_mu_msi_domains_init(msi_data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     irq_set_chained_handler_and_data(msi_data->gic_irq,
> > +                                      imx_mu_msi_irq_handler,
> > +                                      msi_data);
> > +
> > +     pm_runtime_enable(dev);
> > +
> > +     ret = pm_runtime_get_sync(dev);
> > +     if (ret < 0) {
> > +             pm_runtime_put_noidle(dev);
> > +             goto disable_runtime_pm;
> > +     }
> > +
> > +     ret = pm_runtime_put_sync(dev);
> > +     if (ret < 0)
> > +             goto disable_runtime_pm;
> > +
> > +     clk_disable_unprepare(msi_data->clk);
> > +
> > +     return 0;
> > +
> > +disable_runtime_pm:
> > +     pm_runtime_disable(dev);
> > +     clk_disable_unprepare(msi_data->clk);
> > +
> > +     return ret;
> > +}
> > +
> > +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 imx_mu_msi_remove(struct platform_device *pdev)
> > +{
> > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > +
> > +     imx_mu_msi_teardown_hwirq(msi_data);
> > +
> > +     irq_domain_remove(msi_data->msi_domain);
> > +     irq_domain_remove(msi_data->parent);
> 
> How do you ensure that no device is still holding interrupts? Let me
> give you a hint: you can't. So removing an interrupt controller module
> should not be possible.

[Frank Li] I agree. But there are many *_remove under irqchip. 

> 
> > +
> > +     platform_set_drvdata(pdev, NULL);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver imx_mu_msi_driver = {
> > +     .driver = {
> > +             .name = "imx-mu-msi",
> > +             .of_match_table = imx_mu_msi_ids,
> > +             .pm = &imx_mu_pm_ops,
> > +     },
> > +     .probe = imx_mu_msi_probe,
> > +     .remove = imx_mu_msi_remove,
> > +};
> > +
> > +module_platform_driver(imx_mu_msi_driver);
> 
> Please use the standard probing methods
> (IRQCHIP_PLATFORM_DRIVER_BEGIN
> and co).
> 
> > +
> > +MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller
> driver");
> > +MODULE_LICENSE("GPL");
> 
> I have the ugly feeling that this driver really isn't about MSIs, but
> is just a way to sneak some terrible abstraction into the kernel... I
> guess we'll eventually find out. In the meantime, this driver needs
> fixing.
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Friday, July 8, 2022 3:59 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: 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
> Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> 
> Caution: EXT Email
> 
> On Thu, 07 Jul 2022 22:02:36 +0100,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > MU support generate irq by write data to a register.
> > This patch make mu worked as msi controller.
> > So MU can do doorbell by using standard msi api.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/irqchip/Kconfig          |   7 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-mu-msi.c | 490
> +++++++++++++++++++++++++++++++
> >  3 files changed, 498 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..4599471d880c0 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,13 @@ config IMX_INTMUX
> >       help
> >         Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > +     bool "i.MX MU work as MSI controller"
> > +     default y if ARCH_MXC
> > +     select IRQ_DOMAIN
> > +     help
> > +       MU work as MSI controller to do general doorbell
> > +
> >  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..f7193a6c1245e
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,490 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * NXP MU worked 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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/dma-iommu.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pm_domain.h>
> > +
> > +
> > +#define IMX_MU_CHANS            4
> > +
> > +enum imx_mu_chan_type {
> > +     IMX_MU_TYPE_TX,         /* Tx */
> > +     IMX_MU_TYPE_RX,         /* Rx */
> > +     IMX_MU_TYPE_TXDB,       /* Tx doorbell */
> > +     IMX_MU_TYPE_RXDB,       /* Rx doorbell */
> 
> What does any of this even mean for MSIs?

Sorry, I forget deleted it.

> 
> > +};
> > +
> > +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,
> > +};
> > +
> > +enum imx_mu_type {
> > +     IMX_MU_V1,
> > +     IMX_MU_V2,
> > +     IMX_MU_V2_S4 = BIT(15),
> 
> If the bit assignment is significant, make it so for all members of
> this enum.

[Frank Li] Yes, you are right.

> 
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24
> + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(type, x) (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[4];         /* Status Registers */
> > +     u32     xCR[4];         /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > +     spinlock_t              lock;
> > +     struct platform_device  *pdev;
> > +     struct irq_domain       *parent;
> > +     struct irq_domain       *msi_domain;
> > +     void __iomem            *regs;
> > +     phys_addr_t             msiir_addr;
> > +     struct imx_mu_dcfg      *cfg;
> > +     u32                     msir_num;
> > +     struct imx_mu_msir      *msir;
> > +     u32                     irqs_num;
> > +     unsigned long           used;
> > +     u32                     gic_irq;
> > +     struct clk              *clk;
> > +     struct device           *pd_a;
> > +     struct device           *pd_b;
> > +     struct device_link      *pd_link_a;
> > +     struct device_link      *pd_link_b;
> > +};
> > +
> > +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;
> > +
> > +     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]);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     return val;
> > +}
> > +
> > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> >parent_data);
> > +
> > +     pci_msi_mask_irq(data);
> 
> What is this? Below, you create a platform MSI domain. Either you
> support PCI, and you create a PCI/MSI domain (and the above may make
> sense), or you are doing platform MSI, and the above is non-sense.

[Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq()  

> 
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_unmask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> >parent_data);
> > +
> > +     pci_msi_unmask_irq(data);
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > +     .name = "MU-MSI",
> > +     .irq_mask       = imx_mu_msi_mask_irq,
> > +     .irq_unmask     = imx_mu_msi_unmask_irq,
> > +};
> > +
> > +static struct msi_domain_ops its_pmsi_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> > +                MSI_FLAG_USE_DEF_CHIP_OPS |
> > +                MSI_FLAG_PCI_MSIX),
> > +     .ops    = &its_pmsi_ops,
> > +     .chip   = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_compose_msg(struct irq_data *data, struct
> msi_msg *msg)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> > +     msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data-
> >hwirq);
> > +     msg->data = data->hwirq;
> > +
> > +     iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
> > +}
> > +
> > +static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
> > +                                const struct cpumask *mask, bool force)
> > +
> > +{
> > +     return IRQ_SET_MASK_OK;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > +     .name                   = "MU",
> > +     .irq_compose_msi_msg    = imx_mu_msi_compose_msg,
> > +     .irq_set_affinity = imx_mu_msi_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;
> > +     msi_alloc_info_t *info = args;
> > +     int pos, err = 0;
> > +
> > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> 
> The core code already deals with runtime PM. What prevents it from
> working, other than the fact you don't populate the device in the
> top-level domain?

[Frank Li]  Do you means power domain or irq domain?

> 
> > +
> > +     WARN_ON(nr_irqs != 1);
> > +
> > +     spin_lock(&msi_data->lock);
> > +     pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
> > +     if (pos < msi_data->irqs_num)
> > +             __set_bit(pos, &msi_data->used);
> > +     else
> > +             err = -ENOSPC;
> > +     spin_unlock(&msi_data->lock);
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr +
> pos * 4);
> > +     if (err)
> > +             return err;
> > +
> > +     irq_domain_set_info(domain, virq, pos,
> > +                         &imx_mu_msi_parent_chip, msi_data,
> > +                         handle_simple_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);
> > +     int pos;
> > +
> > +     pos = d->hwirq;
> > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > +             return;
> > +     }
> 
> How can this happen?

I just copy from irq-ls-scfg-msi.c
It should be impossible happen if everything work as expected. 

> 
> > +
> > +     spin_lock(&msi_data->lock);
> > +     __clear_bit(pos, &msi_data->used);
> > +     spin_unlock(&msi_data->lock);
> > +
> > +     pm_runtime_put(&msi_data->pdev->dev);
> > +}
> > +
> > +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);
> > +     u32 status;
> > +     int i;
> > +
> > +     status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > +     chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > +             if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
> > +                     imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
> > +                     generic_handle_domain_irq(msi_data->parent, i);
> > +             }
> > +     }
> > +     chained_irq_exit(irq_desc_get_chip(desc), desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> > +{
> > +     /* Initialize MSI domain parent */
> > +     msi_data->parent = irq_domain_add_linear(NULL,
> 
> NAK. Don't create anonymous domains.
[Frank Li] will fixed at next version.

> 
> > +                                              msi_data->irqs_num,
> > +                                              &imx_mu_msi_domain_ops,
> > +                                              msi_data);
> > +     if (!msi_data->parent) {
> > +             dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     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);
> > +
> > +     if (!msi_data->msi_domain) {
> > +             dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> > +             irq_domain_remove(msi_data->parent);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
> > +{
> > +     if (msi_data->gic_irq > 0)
> > +             irq_set_chained_handler_and_data(msi_data->gic_irq, NULL,
> NULL);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > +     .xTR    = 0x0,
> > +     .xRR    = 0x10,
> > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +     .xTR    = 0x20,
> > +     .xRR    = 0x40,
> > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > +     .type   = IMX_MU_V2,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> 
> What are these? We really don't need more magic numbers.

It is register offset.  The difference version MU hardware's register map is difference. 

> 
> > +
> > +static const struct of_device_id imx_mu_msi_ids[] = {
> > +     { .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
> > +     { .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
> > +     { .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
> > +     { .compatible = "fsl,imx8ulp-mu-msi-s4", .data =
> &imx_mu_cfg_imx8ulp_s4 },
> > +     { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
> > +
> > +static int imx_mu_msi_probe(struct platform_device *pdev)
> > +{
> > +     const struct of_device_id *match;
> > +     struct imx_mu_msi *msi_data, *priv;
> > +     struct device *dev = &pdev->dev;
> > +     struct resource *res;
> > +     int ret;
> > +
> > +     match = of_match_device(imx_mu_msi_ids, &pdev->dev);
> > +     if (!match)
> > +             return -ENODEV;
> > +
> > +     priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> > +     if (!msi_data)
> > +             return -ENOMEM;
> > +
> > +     msi_data->cfg = (struct imx_mu_dcfg *) match->data;
> > +
> > +     msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "a");
> > +     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, "b");
> > +     if (!res)
> > +             return -EIO;
> > +
> > +     msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > +     msi_data->pdev = pdev;
> > +     msi_data->irqs_num = IMX_MU_CHANS;
> 
> If that's hardcoded, why do we need an extra variable? I also question
> the usefulness of this driver if the HW can only deal with *4* MSIs...
> This looks a bit like a joke.

MU don't really MSI controller.  Each MU have 4 channel.  
I.MX have several MU units.  

PCI EP driver need an address as doorbell,  so PCI RC side can write
This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
Have not ITS support yet now.  So we can use MU as simple MSI controller.

So PCE EP side driver can use standard MSI method to do doorbell for
GIC-ITS,  MU and other msi controller without any modification. 

I will add such information at cover letter next version.

> 
> > +
> > +     msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
> > +     if (msi_data->gic_irq <= 0)
> > +             return -ENODEV;
> > +
> > +     platform_set_drvdata(pdev, msi_data);
> > +
> > +     msi_data->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(msi_data->clk)) {
> > +             if (PTR_ERR(msi_data->clk) != -ENOENT)
> > +                     return PTR_ERR(msi_data->clk);
> > +
> > +             msi_data->clk = NULL;
> > +     }
> > +
> > +     ret = clk_prepare_enable(msi_data->clk);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to enable clock\n");
> > +             return ret;
> > +     }
> > +
> > +     priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> > +     if (IS_ERR(priv->pd_a))
> > +             return PTR_ERR(priv->pd_a);
> > +
> > +     priv->pd_link_a = device_link_add(dev, priv->pd_a,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!priv->pd_link_a) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
> > +     if (IS_ERR(priv->pd_b))
> > +             return PTR_ERR(priv->pd_b);
> > +
> > +     priv->pd_link_b = device_link_add(dev, priv->pd_b,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!priv->pd_link_b) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = imx_mu_msi_domains_init(msi_data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     irq_set_chained_handler_and_data(msi_data->gic_irq,
> > +                                      imx_mu_msi_irq_handler,
> > +                                      msi_data);
> > +
> > +     pm_runtime_enable(dev);
> > +
> > +     ret = pm_runtime_get_sync(dev);
> > +     if (ret < 0) {
> > +             pm_runtime_put_noidle(dev);
> > +             goto disable_runtime_pm;
> > +     }
> > +
> > +     ret = pm_runtime_put_sync(dev);
> > +     if (ret < 0)
> > +             goto disable_runtime_pm;
> > +
> > +     clk_disable_unprepare(msi_data->clk);
> > +
> > +     return 0;
> > +
> > +disable_runtime_pm:
> > +     pm_runtime_disable(dev);
> > +     clk_disable_unprepare(msi_data->clk);
> > +
> > +     return ret;
> > +}
> > +
> > +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 imx_mu_msi_remove(struct platform_device *pdev)
> > +{
> > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > +
> > +     imx_mu_msi_teardown_hwirq(msi_data);
> > +
> > +     irq_domain_remove(msi_data->msi_domain);
> > +     irq_domain_remove(msi_data->parent);
> 
> How do you ensure that no device is still holding interrupts? Let me
> give you a hint: you can't. So removing an interrupt controller module
> should not be possible.

[Frank Li] I agree. But there are many *_remove under irqchip. 

> 
> > +
> > +     platform_set_drvdata(pdev, NULL);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver imx_mu_msi_driver = {
> > +     .driver = {
> > +             .name = "imx-mu-msi",
> > +             .of_match_table = imx_mu_msi_ids,
> > +             .pm = &imx_mu_pm_ops,
> > +     },
> > +     .probe = imx_mu_msi_probe,
> > +     .remove = imx_mu_msi_remove,
> > +};
> > +
> > +module_platform_driver(imx_mu_msi_driver);
> 
> Please use the standard probing methods
> (IRQCHIP_PLATFORM_DRIVER_BEGIN
> and co).
> 
> > +
> > +MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller
> driver");
> > +MODULE_LICENSE("GPL");
> 
> I have the ugly feeling that this driver really isn't about MSIs, but
> is just a way to sneak some terrible abstraction into the kernel... I
> guess we'll eventually find out. In the meantime, this driver needs
> fixing.
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

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

Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Rob Herring]

On Thu, 07 Jul 2022 16:02:37 -0500, Frank Li wrote:
> imx mu support generate irq by write a register.
> provide msi controller support so other driver
> can use it by standard msi interface.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.example.dts:31.41-42 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:383: Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1404: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Rob Herring]

On Thu, 07 Jul 2022 16:02:37 -0500, Frank Li wrote:
> imx mu support generate irq by write a register.
> provide msi controller support so other driver
> can use it by standard msi interface.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.example.dts:31.41-42 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:383: Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1404: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


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

Re: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Fri, 08 Jul 2022 17:26:33 +0100,
Frank Li <frank.li@nxp.com> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Friday, July 8, 2022 3:59 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: 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
> > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> > 
> > Caution: EXT Email
> > 
> > On Thu, 07 Jul 2022 22:02:36 +0100,
> > Frank Li <Frank.Li@nxp.com> wrote:
> > >
> > > MU support generate irq by write data to a register.
> > > This patch make mu worked as msi controller.
> > > So MU can do doorbell by using standard msi api.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/irqchip/Kconfig          |   7 +
> > >  drivers/irqchip/Makefile         |   1 +
> > >  drivers/irqchip/irq-imx-mu-msi.c | 490
> > +++++++++++++++++++++++++++++++
> > >  3 files changed, 498 insertions(+)
> > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > >

[...]

> > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > +{
> > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > >parent_data);
> > > +
> > > +     pci_msi_mask_irq(data);
> > 
> > What is this? Below, you create a platform MSI domain. Either you
> > support PCI, and you create a PCI/MSI domain (and the above may make
> > sense), or you are doing platform MSI, and the above is non-sense.
> 
> [Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq()  

OK, hold that thought and see below.

> > > +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;
> > > +     msi_alloc_info_t *info = args;
> > > +     int pos, err = 0;
> > > +
> > > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> > 
> > The core code already deals with runtime PM. What prevents it from
> > working, other than the fact you don't populate the device in the
> > top-level domain?
> 
> [Frank Li]  Do you means power domain or irq domain?

IRQ domain. See irq_domain_set_pm_device() and how PM is used on
interrupt request.

[...]

> > > +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);
> > > +     int pos;
> > > +
> > > +     pos = d->hwirq;
> > > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > > +             return;
> > > +     }
> > 
> > How can this happen?
> 
> I just copy from irq-ls-scfg-msi.c

I wish you didn't do that.

> It should be impossible happen if everything work as expected. 

Then it should go.

[...]

> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > +     .xTR    = 0x0,
> > > +     .xRR    = 0x10,
> > > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > > +};
> > > +
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > > +     .xTR    = 0x20,
> > > +     .xRR    = 0x40,
> > > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > > +};
> > > +
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > > +     .type   = IMX_MU_V2,
> > > +     .xTR    = 0x200,
> > > +     .xRR    = 0x280,
> > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > +};
> > > +
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > > +     .xTR    = 0x200,
> > > +     .xRR    = 0x280,
> > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > +};
> > 
> > What are these? We really don't need more magic numbers.
> 
> It is register offset.  The difference version MU hardware's
> register map is difference.

Then please document what this is, what the various registers are, and
correctly set type everywhere.

[...]

> > If that's hardcoded, why do we need an extra variable? I also question
> > the usefulness of this driver if the HW can only deal with *4* MSIs...
> > This looks a bit like a joke.
> 
> MU don't really MSI controller.  Each MU have 4 channel.  
> I.MX have several MU units.

Then is it really useful to model that as a MSI controller? This
smells of a mailbox controller to me instead. And I really worry that
this device doesn't correctly preserve the ordering between a device
doing DMA and generating an interrupt to indicate completion of the
DMA transaction... Does this block offers such a guarantee?

> PCI EP driver need an address as doorbell,  so PCI RC side can write
> This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
> Have not ITS support yet now.  So we can use MU as simple MSI controller.

Is that an integrated EP on the same SoC? Or are you talking of two
SoCs connected over PCIe? Also, you explicitly said that this was
*not* a PCI/MSI controller. So what is this all about?

[...]

> > > +static int imx_mu_msi_remove(struct platform_device *pdev)
> > > +{
> > > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > > +
> > > +     imx_mu_msi_teardown_hwirq(msi_data);
> > > +
> > > +     irq_domain_remove(msi_data->msi_domain);
> > > +     irq_domain_remove(msi_data->parent);
> > 
> > How do you ensure that no device is still holding interrupts? Let me
> > give you a hint: you can't. So removing an interrupt controller module
> > should not be possible.
> 
> [Frank Li] I agree. But there are many *_remove under irqchip.

That doesn't make it right.

Thanks,

	M.

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

Re: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Marc Zyngier]

On Fri, 08 Jul 2022 17:26:33 +0100,
Frank Li <frank.li@nxp.com> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Friday, July 8, 2022 3:59 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: 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
> > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> > 
> > Caution: EXT Email
> > 
> > On Thu, 07 Jul 2022 22:02:36 +0100,
> > Frank Li <Frank.Li@nxp.com> wrote:
> > >
> > > MU support generate irq by write data to a register.
> > > This patch make mu worked as msi controller.
> > > So MU can do doorbell by using standard msi api.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/irqchip/Kconfig          |   7 +
> > >  drivers/irqchip/Makefile         |   1 +
> > >  drivers/irqchip/irq-imx-mu-msi.c | 490
> > +++++++++++++++++++++++++++++++
> > >  3 files changed, 498 insertions(+)
> > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > >

[...]

> > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > +{
> > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > >parent_data);
> > > +
> > > +     pci_msi_mask_irq(data);
> > 
> > What is this? Below, you create a platform MSI domain. Either you
> > support PCI, and you create a PCI/MSI domain (and the above may make
> > sense), or you are doing platform MSI, and the above is non-sense.
> 
> [Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq()  

OK, hold that thought and see below.

> > > +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;
> > > +     msi_alloc_info_t *info = args;
> > > +     int pos, err = 0;
> > > +
> > > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> > 
> > The core code already deals with runtime PM. What prevents it from
> > working, other than the fact you don't populate the device in the
> > top-level domain?
> 
> [Frank Li]  Do you means power domain or irq domain?

IRQ domain. See irq_domain_set_pm_device() and how PM is used on
interrupt request.

[...]

> > > +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);
> > > +     int pos;
> > > +
> > > +     pos = d->hwirq;
> > > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > > +             return;
> > > +     }
> > 
> > How can this happen?
> 
> I just copy from irq-ls-scfg-msi.c

I wish you didn't do that.

> It should be impossible happen if everything work as expected. 

Then it should go.

[...]

> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > +     .xTR    = 0x0,
> > > +     .xRR    = 0x10,
> > > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > > +};
> > > +
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > > +     .xTR    = 0x20,
> > > +     .xRR    = 0x40,
> > > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > > +};
> > > +
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > > +     .type   = IMX_MU_V2,
> > > +     .xTR    = 0x200,
> > > +     .xRR    = 0x280,
> > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > +};
> > > +
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > > +     .xTR    = 0x200,
> > > +     .xRR    = 0x280,
> > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > +};
> > 
> > What are these? We really don't need more magic numbers.
> 
> It is register offset.  The difference version MU hardware's
> register map is difference.

Then please document what this is, what the various registers are, and
correctly set type everywhere.

[...]

> > If that's hardcoded, why do we need an extra variable? I also question
> > the usefulness of this driver if the HW can only deal with *4* MSIs...
> > This looks a bit like a joke.
> 
> MU don't really MSI controller.  Each MU have 4 channel.  
> I.MX have several MU units.

Then is it really useful to model that as a MSI controller? This
smells of a mailbox controller to me instead. And I really worry that
this device doesn't correctly preserve the ordering between a device
doing DMA and generating an interrupt to indicate completion of the
DMA transaction... Does this block offers such a guarantee?

> PCI EP driver need an address as doorbell,  so PCI RC side can write
> This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
> Have not ITS support yet now.  So we can use MU as simple MSI controller.

Is that an integrated EP on the same SoC? Or are you talking of two
SoCs connected over PCIe? Also, you explicitly said that this was
*not* a PCI/MSI controller. So what is this all about?

[...]

> > > +static int imx_mu_msi_remove(struct platform_device *pdev)
> > > +{
> > > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > > +
> > > +     imx_mu_msi_teardown_hwirq(msi_data);
> > > +
> > > +     irq_domain_remove(msi_data->msi_domain);
> > > +     irq_domain_remove(msi_data->parent);
> > 
> > How do you ensure that no device is still holding interrupts? Let me
> > give you a hint: you can't. So removing an interrupt controller module
> > should not be possible.
> 
> [Frank Li] I agree. But there are many *_remove under irqchip.

That doesn't make it right.

Thanks,

	M.

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

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

RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Saturday, July 9, 2022 3:16 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: 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
> Subject: Re: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> 
> Caution: EXT Email
> 
> On Fri, 08 Jul 2022 17:26:33 +0100,
> Frank Li <frank.li@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <maz@kernel.org>
> > > Sent: Friday, July 8, 2022 3:59 AM
> > > To: Frank Li <frank.li@nxp.com>
> > > Cc: 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
> > > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> > >
> > > Caution: EXT Email
> > >
> > > On Thu, 07 Jul 2022 22:02:36 +0100,
> > > Frank Li <Frank.Li@nxp.com> wrote:
> > > >
> > > > MU support generate irq by write data to a register.
> > > > This patch make mu worked as msi controller.
> > > > So MU can do doorbell by using standard msi api.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  drivers/irqchip/Kconfig          |   7 +
> > > >  drivers/irqchip/Makefile         |   1 +
> > > >  drivers/irqchip/irq-imx-mu-msi.c | 490
> > > +++++++++++++++++++++++++++++++
> > > >  3 files changed, 498 insertions(+)
> > > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > > >
> 
> [...]
> 
> > > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > > >parent_data);
> > > > +
> > > > +     pci_msi_mask_irq(data);
> > >
> > > What is this? Below, you create a platform MSI domain. Either you
> > > support PCI, and you create a PCI/MSI domain (and the above may make
> > > sense), or you are doing platform MSI, and the above is non-sense.
> >
> > [Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq()
> 
> OK, hold that thought and see below.
> 
> > > > +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;
> > > > +     msi_alloc_info_t *info = args;
> > > > +     int pos, err = 0;
> > > > +
> > > > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> > >
> > > The core code already deals with runtime PM. What prevents it from
> > > working, other than the fact you don't populate the device in the
> > > top-level domain?
> >
> > [Frank Li]  Do you means power domain or irq domain?
> 
> IRQ domain. See irq_domain_set_pm_device() and how PM is used on
> interrupt request.
> 
> [...]
> 
> > > > +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);
> > > > +     int pos;
> > > > +
> > > > +     pos = d->hwirq;
> > > > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > > > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > > > +             return;
> > > > +     }
> > >
> > > How can this happen?
> >
> > I just copy from irq-ls-scfg-msi.c
> 
> I wish you didn't do that.
> 
> > It should be impossible happen if everything work as expected.
> 
> Then it should go.
> 
> [...]
> 
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > > +     .xTR    = 0x0,
> > > > +     .xRR    = 0x10,
> > > > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > > > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > > > +};
> > > > +
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > > > +     .xTR    = 0x20,
> > > > +     .xRR    = 0x40,
> > > > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > > > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > > > +};
> > > > +
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > > > +     .type   = IMX_MU_V2,
> > > > +     .xTR    = 0x200,
> > > > +     .xRR    = 0x280,
> > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > +};
> > > > +
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > > > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > > > +     .xTR    = 0x200,
> > > > +     .xRR    = 0x280,
> > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > +};
> > >
> > > What are these? We really don't need more magic numbers.
> >
> > It is register offset.  The difference version MU hardware's
> > register map is difference.
> 
> Then please document what this is, what the various registers are, and
> correctly set type everywhere.
> 
> [...]
> 
> > > If that's hardcoded, why do we need an extra variable? I also question
> > > the usefulness of this driver if the HW can only deal with *4* MSIs...
> > > This looks a bit like a joke.
> >
> > MU don't really MSI controller.  Each MU have 4 channel.
> > I.MX have several MU units.
> 
> Then is it really useful to model that as a MSI controller? This
> smells of a mailbox controller to me instead. 

MU already have driver as mailbox controller. But mailbox interface can't
Match PCI-EP requirement.  

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

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.

> And I really worry that
> this device doesn't correctly preserve the ordering between a device
> doing DMA and generating an interrupt to indicate completion of the
> DMA transaction... Does this block offers such a guarantee?

According to PCI memory model,  the sequence of write is the ordered.
PCI host write data to DDR, then write data to MSI (MU)'s register are ordered. 

PCI->AXI bridge will guarantee the order, otherwise it will block memory model.

> 
> > PCI EP driver need an address as doorbell,  so PCI RC side can write
> > This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
> > Have not ITS support yet now.  So we can use MU as simple MSI controller.
> 
> Is that an integrated EP on the same SoC? Or are you talking of two
> SoCs connected over PCIe? 

Two Socs connected over PCIe.

> Also, you explicitly said that this was
> *not* a PCI/MSI controller. So what is this all about?

I think real MSI controller works as
1.  Write data to address to trigger irq.   Data is  DID | irq_number. 
2.  MSI controller can distribute difference irq to difference core. 
3.  There are status bit, which match irq_number.   If DID| <0>  and DID | <1> write
To address at same time,  both <0> an <1> irq can be handled.
4. other features ..

MU is not designed as MSI controller at hardware. 
But if we limit irq_number to 1,  MU can work as MSI controller although only provide 4 irq numbers.

For NTB using case, it is enough.  Because i.MX have not gic-its,  I have to use MU as MSI controller.
So PCIe EP can use  it to improve transfer latency.  Without it,  PCI ep driver have to using polling to check
Status, delay will be bigger than 5ms. With msi,  transfer delay will < 1ms. 

I will put all background information at cover letter at next version. 

> 
> [...]
> 
> > > > +static int imx_mu_msi_remove(struct platform_device *pdev)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > > > +
> > > > +     imx_mu_msi_teardown_hwirq(msi_data);
> > > > +
> > > > +     irq_domain_remove(msi_data->msi_domain);
> > > > +     irq_domain_remove(msi_data->parent);
> > >
> > > How do you ensure that no device is still holding interrupts? Let me
> > > give you a hint: you can't. So removing an interrupt controller module
> > > should not be possible.
> >
> > [Frank Li] I agree. But there are many *_remove under irqchip.
> 
> That doesn't make it right.
> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Saturday, July 9, 2022 3:16 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: 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
> Subject: Re: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> 
> Caution: EXT Email
> 
> On Fri, 08 Jul 2022 17:26:33 +0100,
> Frank Li <frank.li@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <maz@kernel.org>
> > > Sent: Friday, July 8, 2022 3:59 AM
> > > To: Frank Li <frank.li@nxp.com>
> > > Cc: 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
> > > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> > >
> > > Caution: EXT Email
> > >
> > > On Thu, 07 Jul 2022 22:02:36 +0100,
> > > Frank Li <Frank.Li@nxp.com> wrote:
> > > >
> > > > MU support generate irq by write data to a register.
> > > > This patch make mu worked as msi controller.
> > > > So MU can do doorbell by using standard msi api.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  drivers/irqchip/Kconfig          |   7 +
> > > >  drivers/irqchip/Makefile         |   1 +
> > > >  drivers/irqchip/irq-imx-mu-msi.c | 490
> > > +++++++++++++++++++++++++++++++
> > > >  3 files changed, 498 insertions(+)
> > > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > > >
> 
> [...]
> 
> > > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > > >parent_data);
> > > > +
> > > > +     pci_msi_mask_irq(data);
> > >
> > > What is this? Below, you create a platform MSI domain. Either you
> > > support PCI, and you create a PCI/MSI domain (and the above may make
> > > sense), or you are doing platform MSI, and the above is non-sense.
> >
> > [Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq()
> 
> OK, hold that thought and see below.
> 
> > > > +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;
> > > > +     msi_alloc_info_t *info = args;
> > > > +     int pos, err = 0;
> > > > +
> > > > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> > >
> > > The core code already deals with runtime PM. What prevents it from
> > > working, other than the fact you don't populate the device in the
> > > top-level domain?
> >
> > [Frank Li]  Do you means power domain or irq domain?
> 
> IRQ domain. See irq_domain_set_pm_device() and how PM is used on
> interrupt request.
> 
> [...]
> 
> > > > +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);
> > > > +     int pos;
> > > > +
> > > > +     pos = d->hwirq;
> > > > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > > > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > > > +             return;
> > > > +     }
> > >
> > > How can this happen?
> >
> > I just copy from irq-ls-scfg-msi.c
> 
> I wish you didn't do that.
> 
> > It should be impossible happen if everything work as expected.
> 
> Then it should go.
> 
> [...]
> 
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > > +     .xTR    = 0x0,
> > > > +     .xRR    = 0x10,
> > > > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > > > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > > > +};
> > > > +
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > > > +     .xTR    = 0x20,
> > > > +     .xRR    = 0x40,
> > > > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > > > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > > > +};
> > > > +
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > > > +     .type   = IMX_MU_V2,
> > > > +     .xTR    = 0x200,
> > > > +     .xRR    = 0x280,
> > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > +};
> > > > +
> > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > > > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > > > +     .xTR    = 0x200,
> > > > +     .xRR    = 0x280,
> > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > +};
> > >
> > > What are these? We really don't need more magic numbers.
> >
> > It is register offset.  The difference version MU hardware's
> > register map is difference.
> 
> Then please document what this is, what the various registers are, and
> correctly set type everywhere.
> 
> [...]
> 
> > > If that's hardcoded, why do we need an extra variable? I also question
> > > the usefulness of this driver if the HW can only deal with *4* MSIs...
> > > This looks a bit like a joke.
> >
> > MU don't really MSI controller.  Each MU have 4 channel.
> > I.MX have several MU units.
> 
> Then is it really useful to model that as a MSI controller? This
> smells of a mailbox controller to me instead. 

MU already have driver as mailbox controller. But mailbox interface can't
Match PCI-EP requirement.  

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

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.

> And I really worry that
> this device doesn't correctly preserve the ordering between a device
> doing DMA and generating an interrupt to indicate completion of the
> DMA transaction... Does this block offers such a guarantee?

According to PCI memory model,  the sequence of write is the ordered.
PCI host write data to DDR, then write data to MSI (MU)'s register are ordered. 

PCI->AXI bridge will guarantee the order, otherwise it will block memory model.

> 
> > PCI EP driver need an address as doorbell,  so PCI RC side can write
> > This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
> > Have not ITS support yet now.  So we can use MU as simple MSI controller.
> 
> Is that an integrated EP on the same SoC? Or are you talking of two
> SoCs connected over PCIe? 

Two Socs connected over PCIe.

> Also, you explicitly said that this was
> *not* a PCI/MSI controller. So what is this all about?

I think real MSI controller works as
1.  Write data to address to trigger irq.   Data is  DID | irq_number. 
2.  MSI controller can distribute difference irq to difference core. 
3.  There are status bit, which match irq_number.   If DID| <0>  and DID | <1> write
To address at same time,  both <0> an <1> irq can be handled.
4. other features ..

MU is not designed as MSI controller at hardware. 
But if we limit irq_number to 1,  MU can work as MSI controller although only provide 4 irq numbers.

For NTB using case, it is enough.  Because i.MX have not gic-its,  I have to use MU as MSI controller.
So PCIe EP can use  it to improve transfer latency.  Without it,  PCI ep driver have to using polling to check
Status, delay will be bigger than 5ms. With msi,  transfer delay will < 1ms. 

I will put all background information at cover letter at next version. 

> 
> [...]
> 
> > > > +static int imx_mu_msi_remove(struct platform_device *pdev)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > > > +
> > > > +     imx_mu_msi_teardown_hwirq(msi_data);
> > > > +
> > > > +     irq_domain_remove(msi_data->msi_domain);
> > > > +     irq_domain_remove(msi_data->parent);
> > >
> > > How do you ensure that no device is still holding interrupts? Let me
> > > give you a hint: you can't. So removing an interrupt controller module
> > > should not be possible.
> >
> > [Frank Li] I agree. But there are many *_remove under irqchip.
> 
> That doesn't make it right.
> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Krzysztof Kozlowski]

On 07/07/2022 23:02, Frank Li wrote:
> imx mu support generate irq by write a register.
> provide msi controller support so other driver
> can use it by standard msi interface.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
>  1 file changed, 94 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..b4ac583f60227
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
> @@ -0,0 +1,94 @@
> +# 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: NXP i.MX Messaging Unit (MU)
> +
> +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 to signal the other processor 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-facing, Processor B-facing).
> +
> +  MU can work as msi interrupt controller to do doorbell
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: fsl,imx6sx-mu-msi
> +      - const: fsl,imx7ulp-mu-msi
> +      - const: fsl,imx8ulp-mu-msi
> +      - const: fsl,imx8-mu-msi
> +      - const: fsl,imx8ulp-mu-msi-s4

Use enum

> +      - items:
> +          - const: fsl,imx8ulp-mu-msi

Single item... why?

> +      - items:
> +          - enum:
> +              - fsl,imx7s-mu-msi
> +              - fsl,imx8mq-mu-msi
> +              - fsl,imx8mm-mu-msi
> +              - fsl,imx8mn-mu-msi
> +              - fsl,imx8mp-mu-msi
> +              - fsl,imx8qm-mu-msi

Why qm is here not compatible with qxp? It's already mentioned in
section below.

> +              - fsl,imx8qxp-mu-msi
> +          - const: fsl,imx6sx-mu-msi
> +      - description: MU work as msi controller
> +        items:
> +          - enum:
> +              - fsl,imx8qm-mu-msi
> +              - fsl,imx8qxp-mu-msi
> +          - const: fsl,imx6sx-mu-msi
> +  reg:
> +    maxItems: 2
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2

Instead describe the items.

> +
> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  clocks:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - msi-controller

How this end up here?

Aren't you missing allOf with a reference to msi-controller?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    lsio_mu12: msi@5d270000 {
> +               compatible = "fsl,imx6sx-mu-msi-db";

???

> +               msi-controller;
> +               interrupt-controller;

??? How this appeared here

Also fix your indentation like in example-schema.

> +               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>;

Please do not send untested bindings. It's a waste of our time.

Really two items here? You just said only one is allowed.

> +               power-domain-names = "a", "b";

Sorry, this patch looks really poor. a/b is not a descriptive name and
they are not allowed by your own bindings. Please perform some internal
reviews...

> +    };


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Krzysztof Kozlowski]

On 07/07/2022 23:02, Frank Li wrote:
> imx mu support generate irq by write a register.
> provide msi controller support so other driver
> can use it by standard msi interface.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
>  1 file changed, 94 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..b4ac583f60227
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
> @@ -0,0 +1,94 @@
> +# 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: NXP i.MX Messaging Unit (MU)
> +
> +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 to signal the other processor 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-facing, Processor B-facing).
> +
> +  MU can work as msi interrupt controller to do doorbell
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: fsl,imx6sx-mu-msi
> +      - const: fsl,imx7ulp-mu-msi
> +      - const: fsl,imx8ulp-mu-msi
> +      - const: fsl,imx8-mu-msi
> +      - const: fsl,imx8ulp-mu-msi-s4

Use enum

> +      - items:
> +          - const: fsl,imx8ulp-mu-msi

Single item... why?

> +      - items:
> +          - enum:
> +              - fsl,imx7s-mu-msi
> +              - fsl,imx8mq-mu-msi
> +              - fsl,imx8mm-mu-msi
> +              - fsl,imx8mn-mu-msi
> +              - fsl,imx8mp-mu-msi
> +              - fsl,imx8qm-mu-msi

Why qm is here not compatible with qxp? It's already mentioned in
section below.

> +              - fsl,imx8qxp-mu-msi
> +          - const: fsl,imx6sx-mu-msi
> +      - description: MU work as msi controller
> +        items:
> +          - enum:
> +              - fsl,imx8qm-mu-msi
> +              - fsl,imx8qxp-mu-msi
> +          - const: fsl,imx6sx-mu-msi
> +  reg:
> +    maxItems: 2
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2

Instead describe the items.

> +
> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  clocks:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - msi-controller

How this end up here?

Aren't you missing allOf with a reference to msi-controller?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    lsio_mu12: msi@5d270000 {
> +               compatible = "fsl,imx6sx-mu-msi-db";

???

> +               msi-controller;
> +               interrupt-controller;

??? How this appeared here

Also fix your indentation like in example-schema.

> +               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>;

Please do not send untested bindings. It's a waste of our time.

Really two items here? You just said only one is allowed.

> +               power-domain-names = "a", "b";

Sorry, this patch looks really poor. a/b is not a descriptive name and
they are not allowed by your own bindings. Please perform some internal
reviews...

> +    };


Best regards,
Krzysztof

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

RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

> -----Original Message-----
> From: Frank Li
> Sent: Saturday, July 9, 2022 10:23 AM
> To: Marc Zyngier <maz@kernel.org>
> Cc: 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
> Subject: RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Saturday, July 9, 2022 3:16 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: 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
> > Subject: Re: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> >
> > Caution: EXT Email
> >
> > On Fri, 08 Jul 2022 17:26:33 +0100,
> > Frank Li <frank.li@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marc Zyngier <maz@kernel.org>
> > > > Sent: Friday, July 8, 2022 3:59 AM
> > > > To: Frank Li <frank.li@nxp.com>
> > > > Cc: 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
> > > > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> > > >
> > > > Caution: EXT Email
> > > >
> > > > On Thu, 07 Jul 2022 22:02:36 +0100,
> > > > Frank Li <Frank.Li@nxp.com> wrote:
> > > > >
> > > > > MU support generate irq by write data to a register.
> > > > > This patch make mu worked as msi controller.
> > > > > So MU can do doorbell by using standard msi api.
> > > > >
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >  drivers/irqchip/Kconfig          |   7 +
> > > > >  drivers/irqchip/Makefile         |   1 +
> > > > >  drivers/irqchip/irq-imx-mu-msi.c | 490
> > > > +++++++++++++++++++++++++++++++
> > > > >  3 files changed, 498 insertions(+)
> > > > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > > > >
> >
> > [...]
> >
> > > > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > > > +{
> > > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > > > >parent_data);
> > > > > +
> > > > > +     pci_msi_mask_irq(data);
> > > >
> > > > What is this? Below, you create a platform MSI domain. Either you
> > > > support PCI, and you create a PCI/MSI domain (and the above may
> make
> > > > sense), or you are doing platform MSI, and the above is non-sense.
> > >
> > > [Frank Li] You are right. This work as platform msi. Needn't call
> pci_msi_irq()
> >
> > OK, hold that thought and see below.
> >
> > > > > +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;
> > > > > +     msi_alloc_info_t *info = args;
> > > > > +     int pos, err = 0;
> > > > > +
> > > > > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> > > >
> > > > The core code already deals with runtime PM. What prevents it from
> > > > working, other than the fact you don't populate the device in the
> > > > top-level domain?
> > >
> > > [Frank Li]  Do you means power domain or irq domain?
> >
> > IRQ domain. See irq_domain_set_pm_device() and how PM is used on
> > interrupt request.
> >
> > [...]
> >
> > > > > +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);
> > > > > +     int pos;
> > > > > +
> > > > > +     pos = d->hwirq;
> > > > > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > > > > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > > > > +             return;
> > > > > +     }
> > > >
> > > > How can this happen?
> > >
> > > I just copy from irq-ls-scfg-msi.c
> >
> > I wish you didn't do that.
> >
> > > It should be impossible happen if everything work as expected.
> >
> > Then it should go.
> >
> > [...]
> >
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > > > +     .xTR    = 0x0,
> > > > > +     .xRR    = 0x10,
> > > > > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > > > > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > > > > +};
> > > > > +
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > > > > +     .xTR    = 0x20,
> > > > > +     .xRR    = 0x40,
> > > > > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > > > > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > > > > +};
> > > > > +
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > > > > +     .type   = IMX_MU_V2,
> > > > > +     .xTR    = 0x200,
> > > > > +     .xRR    = 0x280,
> > > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > > +};
> > > > > +
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > > > > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > > > > +     .xTR    = 0x200,
> > > > > +     .xRR    = 0x280,
> > > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > > +};
> > > >
> > > > What are these? We really don't need more magic numbers.
> > >
> > > It is register offset.  The difference version MU hardware's
> > > register map is difference.
> >
> > Then please document what this is, what the various registers are, and
> > correctly set type everywhere.
> >
> > [...]
> >
> > > > If that's hardcoded, why do we need an extra variable? I also question
> > > > the usefulness of this driver if the HW can only deal with *4* MSIs...
> > > > This looks a bit like a joke.
> > >
> > > MU don't really MSI controller.  Each MU have 4 channel.
> > > I.MX have several MU units.
> >
> > Then is it really useful to model that as a MSI controller? This
> > smells of a mailbox controller to me instead.
> 
> MU already have driver as mailbox controller. But mailbox interface can't
> Match PCI-EP requirement.
> 
> ┌───────┐          ┌──────────┐
> │       │          │          │
> │       │          │ PCI Host │
> │       │          │          │
> │       │          │          │
> │       │          │ Bar0     │
> │ PCI   │          │ Bar1     │
> │ Func  │          │ Bar2     │
> │       │          │ Bar3     │
> │       │          │ Bar4     │
> │       ├─────────►│          │
> └───────┘ MSI      └──────────┘
> 
> 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.
> 
> > And I really worry that
> > this device doesn't correctly preserve the ordering between a device
> > doing DMA and generating an interrupt to indicate completion of the
> > DMA transaction... Does this block offers such a guarantee?
> 
> According to PCI memory model,  the sequence of write is the ordered.
> PCI host write data to DDR, then write data to MSI (MU)'s register are
> ordered.
> 
> PCI->AXI bridge will guarantee the order, otherwise it will block memory
> model.
> 
> >
> > > PCI EP driver need an address as doorbell,  so PCI RC side can write
> > > This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
> > > Have not ITS support yet now.  So we can use MU as simple MSI
> controller.
> >
> > Is that an integrated EP on the same SoC? Or are you talking of two
> > SoCs connected over PCIe?
> 
> Two Socs connected over PCIe.
> 
> > Also, you explicitly said that this was
> > *not* a PCI/MSI controller. So what is this all about?
> 
> I think real MSI controller works as
> 1.  Write data to address to trigger irq.   Data is  DID | irq_number.
> 2.  MSI controller can distribute difference irq to difference core.
> 3.  There are status bit, which match irq_number.   If DID| <0>  and DID | <1>
> write
> To address at same time,  both <0> an <1> irq can be handled.
> 4. other features ..
> 
> MU is not designed as MSI controller at hardware.
> But if we limit irq_number to 1,  MU can work as MSI controller although only
> provide 4 irq numbers.
> 
> For NTB using case, it is enough.  Because i.MX have not gic-its,  I have to use
> MU as MSI controller.
> So PCIe EP can use  it to improve transfer latency.  Without it,  PCI ep driver
> have to using polling to check
> Status, delay will be bigger than 5ms. With msi,  transfer delay will < 1ms.
> 
> I will put all background information at cover letter at next version.

Marc:  Do you agree on the overall design?  If yes, I will respin patches. 

Best regards
Frank Li

> 
> >
> > [...]
> >
> > > > > +static int imx_mu_msi_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > > > > +
> > > > > +     imx_mu_msi_teardown_hwirq(msi_data);
> > > > > +
> > > > > +     irq_domain_remove(msi_data->msi_domain);
> > > > > +     irq_domain_remove(msi_data->parent);
> > > >
> > > > How do you ensure that no device is still holding interrupts? Let me
> > > > give you a hint: you can't. So removing an interrupt controller module
> > > > should not be possible.
> > >
> > > [Frank Li] I agree. But there are many *_remove under irqchip.
> >
> > That doesn't make it right.
> >
> > Thanks,
> >
> >         M.
> >
> > --
> > Without deviation from the norm, progress is not possible.

RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

[Frank Li]

> -----Original Message-----
> From: Frank Li
> Sent: Saturday, July 9, 2022 10:23 AM
> To: Marc Zyngier <maz@kernel.org>
> Cc: 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
> Subject: RE: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Saturday, July 9, 2022 3:16 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: 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
> > Subject: Re: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> >
> > Caution: EXT Email
> >
> > On Fri, 08 Jul 2022 17:26:33 +0100,
> > Frank Li <frank.li@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marc Zyngier <maz@kernel.org>
> > > > Sent: Friday, July 8, 2022 3:59 AM
> > > > To: Frank Li <frank.li@nxp.com>
> > > > Cc: 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
> > > > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller
> > > >
> > > > Caution: EXT Email
> > > >
> > > > On Thu, 07 Jul 2022 22:02:36 +0100,
> > > > Frank Li <Frank.Li@nxp.com> wrote:
> > > > >
> > > > > MU support generate irq by write data to a register.
> > > > > This patch make mu worked as msi controller.
> > > > > So MU can do doorbell by using standard msi api.
> > > > >
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >  drivers/irqchip/Kconfig          |   7 +
> > > > >  drivers/irqchip/Makefile         |   1 +
> > > > >  drivers/irqchip/irq-imx-mu-msi.c | 490
> > > > +++++++++++++++++++++++++++++++
> > > > >  3 files changed, 498 insertions(+)
> > > > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > > > >
> >
> > [...]
> >
> > > > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > > > +{
> > > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > > > >parent_data);
> > > > > +
> > > > > +     pci_msi_mask_irq(data);
> > > >
> > > > What is this? Below, you create a platform MSI domain. Either you
> > > > support PCI, and you create a PCI/MSI domain (and the above may
> make
> > > > sense), or you are doing platform MSI, and the above is non-sense.
> > >
> > > [Frank Li] You are right. This work as platform msi. Needn't call
> pci_msi_irq()
> >
> > OK, hold that thought and see below.
> >
> > > > > +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;
> > > > > +     msi_alloc_info_t *info = args;
> > > > > +     int pos, err = 0;
> > > > > +
> > > > > +     pm_runtime_get_sync(&msi_data->pdev->dev);
> > > >
> > > > The core code already deals with runtime PM. What prevents it from
> > > > working, other than the fact you don't populate the device in the
> > > > top-level domain?
> > >
> > > [Frank Li]  Do you means power domain or irq domain?
> >
> > IRQ domain. See irq_domain_set_pm_device() and how PM is used on
> > interrupt request.
> >
> > [...]
> >
> > > > > +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);
> > > > > +     int pos;
> > > > > +
> > > > > +     pos = d->hwirq;
> > > > > +     if (pos < 0 || pos >= msi_data->irqs_num) {
> > > > > +             pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> > > > > +             return;
> > > > > +     }
> > > >
> > > > How can this happen?
> > >
> > > I just copy from irq-ls-scfg-msi.c
> >
> > I wish you didn't do that.
> >
> > > It should be impossible happen if everything work as expected.
> >
> > Then it should go.
> >
> > [...]
> >
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > > > +     .xTR    = 0x0,
> > > > > +     .xRR    = 0x10,
> > > > > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > > > > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > > > > +};
> > > > > +
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > > > > +     .xTR    = 0x20,
> > > > > +     .xRR    = 0x40,
> > > > > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > > > > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > > > > +};
> > > > > +
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > > > > +     .type   = IMX_MU_V2,
> > > > > +     .xTR    = 0x200,
> > > > > +     .xRR    = 0x280,
> > > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > > +};
> > > > > +
> > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > > > > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > > > > +     .xTR    = 0x200,
> > > > > +     .xRR    = 0x280,
> > > > > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > > > > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > > > > +};
> > > >
> > > > What are these? We really don't need more magic numbers.
> > >
> > > It is register offset.  The difference version MU hardware's
> > > register map is difference.
> >
> > Then please document what this is, what the various registers are, and
> > correctly set type everywhere.
> >
> > [...]
> >
> > > > If that's hardcoded, why do we need an extra variable? I also question
> > > > the usefulness of this driver if the HW can only deal with *4* MSIs...
> > > > This looks a bit like a joke.
> > >
> > > MU don't really MSI controller.  Each MU have 4 channel.
> > > I.MX have several MU units.
> >
> > Then is it really useful to model that as a MSI controller? This
> > smells of a mailbox controller to me instead.
> 
> MU already have driver as mailbox controller. But mailbox interface can't
> Match PCI-EP requirement.
> 
> ┌───────┐          ┌──────────┐
> │       │          │          │
> │       │          │ PCI Host │
> │       │          │          │
> │       │          │          │
> │       │          │ Bar0     │
> │ PCI   │          │ Bar1     │
> │ Func  │          │ Bar2     │
> │       │          │ Bar3     │
> │       │          │ Bar4     │
> │       ├─────────►│          │
> └───────┘ MSI      └──────────┘
> 
> 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.
> 
> > And I really worry that
> > this device doesn't correctly preserve the ordering between a device
> > doing DMA and generating an interrupt to indicate completion of the
> > DMA transaction... Does this block offers such a guarantee?
> 
> According to PCI memory model,  the sequence of write is the ordered.
> PCI host write data to DDR, then write data to MSI (MU)'s register are
> ordered.
> 
> PCI->AXI bridge will guarantee the order, otherwise it will block memory
> model.
> 
> >
> > > PCI EP driver need an address as doorbell,  so PCI RC side can write
> > > This address to trigger irq.  Ideally,  it use GIC-ITS. But our i.MX chip
> > > Have not ITS support yet now.  So we can use MU as simple MSI
> controller.
> >
> > Is that an integrated EP on the same SoC? Or are you talking of two
> > SoCs connected over PCIe?
> 
> Two Socs connected over PCIe.
> 
> > Also, you explicitly said that this was
> > *not* a PCI/MSI controller. So what is this all about?
> 
> I think real MSI controller works as
> 1.  Write data to address to trigger irq.   Data is  DID | irq_number.
> 2.  MSI controller can distribute difference irq to difference core.
> 3.  There are status bit, which match irq_number.   If DID| <0>  and DID | <1>
> write
> To address at same time,  both <0> an <1> irq can be handled.
> 4. other features ..
> 
> MU is not designed as MSI controller at hardware.
> But if we limit irq_number to 1,  MU can work as MSI controller although only
> provide 4 irq numbers.
> 
> For NTB using case, it is enough.  Because i.MX have not gic-its,  I have to use
> MU as MSI controller.
> So PCIe EP can use  it to improve transfer latency.  Without it,  PCI ep driver
> have to using polling to check
> Status, delay will be bigger than 5ms. With msi,  transfer delay will < 1ms.
> 
> I will put all background information at cover letter at next version.

Marc:  Do you agree on the overall design?  If yes, I will respin patches. 

Best regards
Frank Li

> 
> >
> > [...]
> >
> > > > > +static int imx_mu_msi_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> > > > > +
> > > > > +     imx_mu_msi_teardown_hwirq(msi_data);
> > > > > +
> > > > > +     irq_domain_remove(msi_data->msi_domain);
> > > > > +     irq_domain_remove(msi_data->parent);
> > > >
> > > > How do you ensure that no device is still holding interrupts? Let me
> > > > give you a hint: you can't. So removing an interrupt controller module
> > > > should not be possible.
> > >
> > > [Frank Li] I agree. But there are many *_remove under irqchip.
> >
> > That doesn't make it right.
> >
> > Thanks,
> >
> >         M.
> >
> > --
> > Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [EXT] Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Frank Li]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, July 12, 2022 5:26 AM
> To: Frank Li <frank.li@nxp.com>; tglx@linutronix.de; maz@kernel.org;
> 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
> Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev
> Subject: [EXT] Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi
> controller
> 
> Caution: EXT Email
> 
> On 07/07/2022 23:02, Frank Li wrote:
> > imx mu support generate irq by write a register.
> > provide msi controller support so other driver
> > can use it by standard msi interface.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
> >  1 file changed, 94 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..b4ac583f60227
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-
> msi.yaml
> > @@ -0,0 +1,94 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicet
> ree.org%2Fschemas%2Finterrupt-controller%2Ffsl%2Cmu-
> msi.yaml%23&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C1f7d0921308
> 74d4a52d808da63f0e9db%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637932183637319092%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&amp;sdata=eA4wrkb39C4NFpLcpvMdFfqEBxcikyTOGaBthf61tjI%3D&
> amp;reserved=0
> > +$schema:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicet
> ree.org%2Fmeta-
> schemas%2Fcore.yaml%23&amp;data=05%7C01%7CFrank.Li%40nxp.com%7
> C1f7d092130874d4a52d808da63f0e9db%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C637932183637319092%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&amp;sdata=ke6K1nzmgNXh%2FkyEh6UP%2F0VZ4C17
> GuhnqZTX4WeB4kY%3D&amp;reserved=0
> > +
> > +title: NXP i.MX Messaging Unit (MU)
> > +
> > +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 to signal the other processor 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-facing, Processor B-facing).
> > +
> > +  MU can work as msi interrupt controller to do doorbell
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: fsl,imx6sx-mu-msi
> > +      - const: fsl,imx7ulp-mu-msi
> > +      - const: fsl,imx8ulp-mu-msi
> > +      - const: fsl,imx8-mu-msi
> > +      - const: fsl,imx8ulp-mu-msi-s4
> 
> Use enum
> 
> > +      - items:
> > +          - const: fsl,imx8ulp-mu-msi
> 
> Single item... why?
> 
> > +      - items:
> > +          - enum:
> > +              - fsl,imx7s-mu-msi
> > +              - fsl,imx8mq-mu-msi
> > +              - fsl,imx8mm-mu-msi
> > +              - fsl,imx8mn-mu-msi
> > +              - fsl,imx8mp-mu-msi
> > +              - fsl,imx8qm-mu-msi
> 
> Why qm is here not compatible with qxp? It's already mentioned in
> section below.
> 
> > +              - fsl,imx8qxp-mu-msi
> > +          - const: fsl,imx6sx-mu-msi
> > +      - description: MU work as msi controller
> > +        items:
> > +          - enum:
> > +              - fsl,imx8qm-mu-msi
> > +              - fsl,imx8qxp-mu-msi
> > +          - const: fsl,imx6sx-mu-msi
> > +  reg:
> > +    maxItems: 2
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 2
> 
> Instead describe the items.
> 
> > +
> > +  interrupt-names:
> > +    minItems: 1
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - msi-controller
> 
> How this end up here?
> 
> Aren't you missing allOf with a reference to msi-controller?
> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    lsio_mu12: msi@5d270000 {
> > +               compatible = "fsl,imx6sx-mu-msi-db";
> 
> ???
> 
> > +               msi-controller;
> > +               interrupt-controller;
> 
> ??? How this appeared here
> 
> Also fix your indentation like in example-schema.
> 
> > +               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>;
> 
> Please do not send untested bindings. It's a waste of our time.
> 
> Really two items here? You just said only one is allowed.
> 
> > +               power-domain-names = "a", "b";
> 
> Sorry, this patch looks really poor. a/b is not a descriptive name and

[Frank Li] MU spec said it is A-side and B-side.  So I think "a" "b" is good
Enough here.  I fixed most problem at v2. 

PCI EP using MSI as door bell is quite new.  I want check if the overall idea is good
Enough to go further. 

> they are not allowed by your own bindings. Please perform some internal
> reviews...
> 
> > +    };
> 
> 
> Best regards,
> Krzysztof

RE: [EXT] Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi controller

[Frank Li]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, July 12, 2022 5:26 AM
> To: Frank Li <frank.li@nxp.com>; tglx@linutronix.de; maz@kernel.org;
> 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
> Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev
> Subject: [EXT] Re: [PATCH 2/3] dt-bindings: irqchip: imx mu work as msi
> controller
> 
> Caution: EXT Email
> 
> On 07/07/2022 23:02, Frank Li wrote:
> > imx mu support generate irq by write a register.
> > provide msi controller support so other driver
> > can use it by standard msi interface.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  .../interrupt-controller/fsl,mu-msi.yaml      | 94 +++++++++++++++++++
> >  1 file changed, 94 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..b4ac583f60227
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-
> msi.yaml
> > @@ -0,0 +1,94 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicet
> ree.org%2Fschemas%2Finterrupt-controller%2Ffsl%2Cmu-
> msi.yaml%23&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C1f7d0921308
> 74d4a52d808da63f0e9db%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637932183637319092%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&amp;sdata=eA4wrkb39C4NFpLcpvMdFfqEBxcikyTOGaBthf61tjI%3D&
> amp;reserved=0
> > +$schema:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicet
> ree.org%2Fmeta-
> schemas%2Fcore.yaml%23&amp;data=05%7C01%7CFrank.Li%40nxp.com%7
> C1f7d092130874d4a52d808da63f0e9db%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C637932183637319092%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&amp;sdata=ke6K1nzmgNXh%2FkyEh6UP%2F0VZ4C17
> GuhnqZTX4WeB4kY%3D&amp;reserved=0
> > +
> > +title: NXP i.MX Messaging Unit (MU)
> > +
> > +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 to signal the other processor 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-facing, Processor B-facing).
> > +
> > +  MU can work as msi interrupt controller to do doorbell
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: fsl,imx6sx-mu-msi
> > +      - const: fsl,imx7ulp-mu-msi
> > +      - const: fsl,imx8ulp-mu-msi
> > +      - const: fsl,imx8-mu-msi
> > +      - const: fsl,imx8ulp-mu-msi-s4
> 
> Use enum
> 
> > +      - items:
> > +          - const: fsl,imx8ulp-mu-msi
> 
> Single item... why?
> 
> > +      - items:
> > +          - enum:
> > +              - fsl,imx7s-mu-msi
> > +              - fsl,imx8mq-mu-msi
> > +              - fsl,imx8mm-mu-msi
> > +              - fsl,imx8mn-mu-msi
> > +              - fsl,imx8mp-mu-msi
> > +              - fsl,imx8qm-mu-msi
> 
> Why qm is here not compatible with qxp? It's already mentioned in
> section below.
> 
> > +              - fsl,imx8qxp-mu-msi
> > +          - const: fsl,imx6sx-mu-msi
> > +      - description: MU work as msi controller
> > +        items:
> > +          - enum:
> > +              - fsl,imx8qm-mu-msi
> > +              - fsl,imx8qxp-mu-msi
> > +          - const: fsl,imx6sx-mu-msi
> > +  reg:
> > +    maxItems: 2
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 2
> 
> Instead describe the items.
> 
> > +
> > +  interrupt-names:
> > +    minItems: 1
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - msi-controller
> 
> How this end up here?
> 
> Aren't you missing allOf with a reference to msi-controller?
> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    lsio_mu12: msi@5d270000 {
> > +               compatible = "fsl,imx6sx-mu-msi-db";
> 
> ???
> 
> > +               msi-controller;
> > +               interrupt-controller;
> 
> ??? How this appeared here
> 
> Also fix your indentation like in example-schema.
> 
> > +               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>;
> 
> Please do not send untested bindings. It's a waste of our time.
> 
> Really two items here? You just said only one is allowed.
> 
> > +               power-domain-names = "a", "b";
> 
> Sorry, this patch looks really poor. a/b is not a descriptive name and

[Frank Li] MU spec said it is A-side and B-side.  So I think "a" "b" is good
Enough here.  I fixed most problem at v2. 

PCI EP using MSI as door bell is quite new.  I want check if the overall idea is good
Enough to go further. 

> they are not allowed by your own bindings. Please perform some internal
> reviews...
> 
> > +    };
> 
> 
> Best regards,
> Krzysztof

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