* [PATCH v4 0/4] PCI EP driver support MSI doorbell from host
@ 2022-08-12 21:52 Frank Li
2022-08-12 21:52 ` [PATCH v4 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Frank Li @ 2022-08-12 21:52 UTC (permalink / raw
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas
Cc: kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa
┌───────┐ ┌──────────┐
│ │ │ │
┌─────────────┐ │ │ │ PCI Host │
│ MSI │◄┐ │ │ │ │
│ Controller │ │ │ │ │ │
└─────────────┘ └─┼───────┼──────────┼─Bar0 │
│ PCI │ │ Bar1 │
│ Func │ │ Bar2 │
│ │ │ Bar3 │
│ │ │ Bar4 │
│ ├─────────►│ │
└───────┘ └──────────┘
Many PCI controllers provided Endpoint functions.
Generally PCI endpoint is hardware, which is not running a rich OS,
like linux.
But Linux also supports endpoint functions. PCI Host write BAR<n> space
like write to memory. The EP side can't know memory changed by the Host
driver.
PCI Spec has not defined a standard method to do that. Only define
MSI(x) to let EP notified RC status change.
The basic idea is to trigger an IRQ when PCI RC writes to a memory
address. That's what MSI controller provided. EP drivers just need to
request a platform MSI interrupt, struct MSI_msg *msg will pass down a
memory address and data. EP driver will map such memory address to
one of PCI BAR<n>. Host just writes such an address to trigger EP side
IRQ.
If system have gic-its, only need update PCI EP side driver. But i.MX
have not chip support gic-its yet. So we have to use MU to simulate a
MSI controller. Although only 4 MSI IRQs are simulated, it matched
vntb(pci-epf-vntb) network requirement.
After enable MSI, ping delay reduce < 1ms from ~8ms
IRQchip: imx mu worked as MSI controller:
let imx mu worked as MSI controllers. Although IP is not design
as MSI controller, we still can use it if limited IRQ number to 4.
pcie: endpoint: pci-epf-vntb: add endpoint MSI support
Based on ntb-next branch. https://github.com/jonmason/ntb/commits/ntb-next
Using MSI as door bell registers
This patch is totally independent on previous on. It can be
applied to ntb-next seperately.
i.MX EP function driver is upstreaming by Richard Zhu.
Some dts change missed at this patches. below is reference dts change
--- a/arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
@@ -160,5 +160,6 @@ pcieb_ep: pcie_ep@5f010000 {
num-ib-windows = <6>;
num-ob-windows = <6>;
status = "disabled";
+ MSI-parent = <&lsio_mu12>;
};
--- a/arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi
@@ -172,6 +172,19 @@ lsio_mu6: mailbox@5d210000 {
status = "disabled";
};
+ lsio_mu12: mailbox@5d270000 {
+ compatible = "fsl,imx6sx-mu-MSI";
+ msi-controller;
+ interrupt-controller;
+ reg = <0x5d270000 0x10000>, /* A side */
+ <0x5d300000 0x10000>; /* B side */
+ reg-names = "a", "b";
+ interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
+ power-domains = <&pd IMX_SC_R_MU_12A>,
+ <&pd IMX_SC_R_MU_12B>;
+ power-domain-names = "a", "b";
+ };
+
Change Log
- Change from v3 to v4
Fixed dt-binding document according to Krzysztof Kozlowski's feedback
Fixed irqchip-imx-mu-worked-as-msi-controller according to Marc Zyngier's
comments.
There are still two important points, which I am not sure.
1. clean irq_set_affinity after platform_msi_create_irq_domain.
Some function, like platform_msi_write_msg() is static.
so I have to set MSI_FLAG_USE_DEF_CHIP_OPS flags, which will
set irq_set_affinity to default one.
2. about comments
> + msi_data->msi_domain = platform_msi_create_irq_domain(
> + of_node_to_fwnode(msi_data->pdev->dev.of_node),
> + &imx_mu_msi_domain_info,
> + msi_data->parent);
"And you don't get an error due to the fact that you use the same
fwnode for both domains without overriding the domain bus token?"
I did not understand yet.
Fixed static check warning, reported by Dan Carpenter
pcie: endpoint: pci-epf-vntb: add endpoint MSI support
- Change from v2 to v3
Fixed dt-binding docment check failure
Fixed typo a cover letter.
Change according Bjorn's comments at patch
pcie: endpoint: pci-epf-vntb: add endpoint MSI support
- from V1 to V2
Fixed fsl,mu-msi.yaml's problem
Fixed irq-imx-mu-msi.c problem according Marc Zyngier's feeback
Added a new patch to allow pass down .pm by IRQCHIP_PLATFORM_DRIVER_END
--
2.35.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END
2022-08-12 21:52 [PATCH v4 0/4] PCI EP driver support MSI doorbell from host Frank Li
@ 2022-08-12 21:52 ` Frank Li
2022-08-12 21:52 ` [PATCH v4 2/4] irqchip: imx mu worked as msi controller Frank Li
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Frank Li @ 2022-08-12 21:52 UTC (permalink / raw
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas
Cc: kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa
IRQCHIP_PLATFORM_DRIVER_* compilation define platform_driver
for irqchip. But can't set .pm field of platform_driver.
Added variadic macros to set .pm field or other field if need.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
include/linux/irqchip.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index 3a091d0710ae1..d5e6024cb2a8c 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -44,7 +44,8 @@ static const struct of_device_id drv_name##_irqchip_match_table[] = {
#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, \
.data = typecheck_irq_init_cb(fn), },
-#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \
+
+#define IRQCHIP_PLATFORM_DRIVER_END(drv_name, ...) \
{}, \
}; \
MODULE_DEVICE_TABLE(of, drv_name##_irqchip_match_table); \
@@ -56,6 +57,7 @@ static struct platform_driver drv_name##_driver = { \
.owner = THIS_MODULE, \
.of_match_table = drv_name##_irqchip_match_table, \
.suppress_bind_attrs = true, \
+ __VA_ARGS__ \
}, \
}; \
builtin_platform_driver(drv_name##_driver)
--
2.35.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/4] irqchip: imx mu worked as msi controller
2022-08-12 21:52 [PATCH v4 0/4] PCI EP driver support MSI doorbell from host Frank Li
2022-08-12 21:52 ` [PATCH v4 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
@ 2022-08-12 21:52 ` Frank Li
2022-08-13 9:19 ` Marc Zyngier
2022-08-13 11:57 ` Marc Zyngier
2022-08-12 21:52 ` [PATCH v4 3/4] dt-bindings: irqchip: imx mu work " Frank Li
2022-08-12 21:52 ` [PATCH v4 4/4] pcie: endpoint: pci-epf-vntb: add endpoint MSI support Frank Li
3 siblings, 2 replies; 11+ messages in thread
From: Frank Li @ 2022-08-12 21:52 UTC (permalink / raw
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas
Cc: kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa
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 | 443 +++++++++++++++++++++++++++++++
3 files changed, 451 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..bb111412d598f
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,443 @@
+// 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/irqchip.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_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 = BIT(0),
+ IMX_MU_V2 = BIT(1),
+ IMX_MU_V2_S4 = BIT(15),
+};
+
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+
+struct imx_mu_dcfg {
+ enum imx_mu_type type;
+ u32 xTR; /* Transmit Register0 */
+ u32 xRR; /* Receive Register0 */
+ u32 xSR[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;
+ const struct imx_mu_dcfg *cfg;
+ unsigned long used;
+ int 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_parent_mask_irq(struct irq_data *data)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+ imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
+}
+
+static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+ imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
+}
+
+static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+ imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+ .name = "MU-MSI",
+ .irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_ops imx_mu_msi_irq_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+ .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+ .ops = &imx_mu_msi_irq_ops,
+ .chip = &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+ u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
+
+ msg->address_hi = upper_32_bits(addr);
+ msg->address_lo = lower_32_bits(addr);
+ msg->data = data->hwirq;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+ .name = "MU",
+ .irq_mask = imx_mu_msi_parent_mask_irq,
+ .irq_unmask = imx_mu_msi_parent_unmask_irq,
+ .irq_ack = imx_mu_msi_parent_ack_irq,
+ .irq_compose_msi_msg = imx_mu_msi_compose_msg,
+};
+
+static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
+ unsigned int virq,
+ unsigned int nr_irqs,
+ void *args)
+{
+ struct imx_mu_msi *msi_data = domain->host_data;
+ unsigned long flags;
+ int pos, err = 0;
+
+ WARN_ON(nr_irqs != 1);
+
+ spin_lock_irqsave(&msi_data->lock, flags);
+ pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
+ if (pos < IMX_MU_CHANS)
+ __set_bit(pos, &msi_data->used);
+ else
+ err = -ENOSPC;
+ spin_unlock_irqrestore(&msi_data->lock, flags);
+
+ if (err)
+ return err;
+
+ irq_domain_set_info(domain, virq, pos,
+ &imx_mu_msi_parent_chip, msi_data,
+ handle_edge_irq, NULL, NULL);
+ return 0;
+}
+
+static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+
+ spin_lock_irqsave(&msi_data->lock, flags);
+ __clear_bit(d->hwirq, &msi_data->used);
+ spin_unlock_irqrestore(&msi_data->lock, flags);
+}
+
+static const struct irq_domain_ops imx_mu_msi_domain_ops = {
+ .alloc = imx_mu_msi_domain_irq_alloc,
+ .free = imx_mu_msi_domain_irq_free,
+};
+
+static void imx_mu_msi_irq_handler(struct irq_desc *desc)
+{
+ struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
+ 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, i)) {
+ generic_handle_domain_irq(msi_data->msi_domain, i);
+ }
+ }
+ chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
+{
+ struct fwnode_handle *fwnodes = of_node_to_fwnode(dev_of_node(&msi_data->pdev->dev));
+
+ /* Initialize MSI domain parent */
+ msi_data->parent = irq_domain_create_linear(fwnodes,
+ IMX_MU_CHANS,
+ &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;
+ }
+
+ /* clean irq_set_affinity again because it is chained irq */
+ imx_mu_msi_irq_chip.irq_set_affinity = NULL;
+
+ irq_domain_set_pm_device(msi_data->msi_domain, &msi_data->pdev->dev);
+
+ return 0;
+}
+
+/* Register offset of different version MU IP */
+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 int __init imx_mu_of_init(struct device_node *dn,
+ struct device_node *parent,
+ const struct imx_mu_dcfg *cfg)
+{
+ struct platform_device *pdev = of_find_device_by_node(dn);
+ struct imx_mu_msi *msi_data, *priv;
+ struct resource *res;
+ struct device *dev;
+ int ret;
+
+ if (!pdev)
+ return -ENODEV;
+
+ dev = &pdev->dev;
+
+ priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+ if (!msi_data)
+ return -ENOMEM;
+
+ msi_data->cfg = cfg;
+
+ msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "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->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);
+
+ return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
+{
+ struct imx_mu_msi *priv = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
+{
+ struct imx_mu_msi *priv = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ dev_err(dev, "failed to enable clock\n");
+
+ return ret;
+}
+
+static const struct dev_pm_ops imx_mu_pm_ops = {
+ SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
+ imx_mu_runtime_resume, NULL)
+};
+
+static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
+}
+
+static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
+}
+
+static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
+}
+
+static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
+IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
+IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm = &imx_mu_pm_ops)
+
+
+MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
+MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
+MODULE_LICENSE("GPL");
--
2.35.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 3/4] dt-bindings: irqchip: imx mu work as msi controller
2022-08-12 21:52 [PATCH v4 0/4] PCI EP driver support MSI doorbell from host Frank Li
2022-08-12 21:52 ` [PATCH v4 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
2022-08-12 21:52 ` [PATCH v4 2/4] irqchip: imx mu worked as msi controller Frank Li
@ 2022-08-12 21:52 ` Frank Li
2022-08-14 20:41 ` Rob Herring
2022-08-12 21:52 ` [PATCH v4 4/4] pcie: endpoint: pci-epf-vntb: add endpoint MSI support Frank Li
3 siblings, 1 reply; 11+ messages in thread
From: Frank Li @ 2022-08-12 21:52 UTC (permalink / raw
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas
Cc: kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa
I.MX mu support generate irq by write a register. Provide msi controller
support so other driver such as PCI EP can use it by standard msi
interface as doorbell.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
.../interrupt-controller/fsl,mu-msi.yaml | 93 +++++++++++++++++++
1 file changed, 93 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..f60fa8b686879
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
@@ -0,0 +1,93 @@
+# 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) work as msi controller
+
+maintainers:
+ - Frank Li <Frank.Li@nxp.com>
+
+description: |
+ The Messaging Unit module enables two processors within the SoC to
+ communicate and coordinate by passing messages (e.g. data, status
+ and control) through the MU interface. The MU also provides the ability
+ for one processor (A side) to signal the other processor (B side) using
+ interrupts.
+
+ Because the MU manages the messaging between processors, the MU uses
+ different clocks (from each side of the different peripheral buses).
+ Therefore, the MU must synchronize the accesses from one side to the
+ other. The MU accomplishes synchronization using two sets of matching
+ registers (Processor A-facing, Processor B-facing).
+
+ MU can work as msi interrupt controller to do doorbell
+
+allOf:
+ - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx6sx-mu-msi
+ - fsl,imx7ulp-mu-msi
+ - fsl,imx8ulp-mu-msi
+ - fsl,imx8ulp-mu-msi-s4
+
+ reg:
+ items:
+ - description: a side register base address
+ - description: b side register base address
+
+ reg-names:
+ items:
+ - const: a
+ - const: b
+
+ interrupts:
+ description: a side interrupt number.
+
+ clocks:
+ maxItems: 1
+
+ power-domains:
+ items:
+ - description: a side power domain
+ - description: b side power domain
+
+ power-domain-names:
+ items:
+ - const: a
+ - const: b
+
+ interrupt-controller: true
+
+ msi-controller: true
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - msi-controller
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/firmware/imx/rsrc.h>
+
+ lsio_mu12: msi-controller@5d270000 {
+ compatible = "fsl,imx6sx-mu-msi";
+ msi-controller;
+ interrupt-controller;
+ reg = <0x5d270000 0x10000>, /* A side */
+ <0x5d300000 0x10000>; /* B side */
+ reg-names = "a", "b";
+ interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
+ power-domains = <&pd IMX_SC_R_MU_12A>,
+ <&pd IMX_SC_R_MU_12B>;
+ power-domain-names = "a", "b";
+ };
--
2.35.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 4/4] pcie: endpoint: pci-epf-vntb: add endpoint MSI support
2022-08-12 21:52 [PATCH v4 0/4] PCI EP driver support MSI doorbell from host Frank Li
` (2 preceding siblings ...)
2022-08-12 21:52 ` [PATCH v4 3/4] dt-bindings: irqchip: imx mu work " Frank Li
@ 2022-08-12 21:52 ` Frank Li
3 siblings, 0 replies; 11+ messages in thread
From: Frank Li @ 2022-08-12 21:52 UTC (permalink / raw
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas
Cc: kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa
┌───────┐ ┌──────────┐
│ │ │ │
┌─────────────┐ │ │ │ PCI Host │
│ MSI │◄┐ │ │ │ │
│ Controller │ │ │ │ │ │
└─────────────┘ └─┼───────┼──────────┼─BAR0 │
│ PCI │ │ BAR1 │
│ Func │ │ BAR2 │
│ │ │ BAR3 │
│ │ │ BAR4 │
│ ├─────────►│ │
└───────┘ └──────────┘
Linux 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.
Add MSI support for pci-epf-vntb. pci-epf-vntb driver query if system
have MSI controller. Setup doorbell address according to struct msi_msg.
So PCIe host can write this doorbell address to triger EP side's irq.
If no MSI controller exist, fall back to software polling.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
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..ad4f7ec8a39fc 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 device *dev = &ntb->epf->dev;
+ struct irq_domain *domain;
+ int virq;
+ int ret;
+ int i;
+
+ domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
+ if (!domain)
+ return;
+
+ dev_set_msi_domain(dev, domain);
+
+ if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
+ ntb->db_count,
+ epf_ntb_write_msi_msg)) {
+ dev_info(dev, "Can't allocate MSI, fall 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, "devm_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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
2022-08-12 21:52 ` [PATCH v4 2/4] irqchip: imx mu worked as msi controller Frank Li
@ 2022-08-13 9:19 ` Marc Zyngier
2022-08-14 3:12 ` [EXT] " Frank Li
2022-08-13 11:57 ` Marc Zyngier
1 sibling, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2022-08-13 9:19 UTC (permalink / raw
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, kernel, devicetree, linux-arm-kernel, linux-pci,
peng.fan, aisheng.dong, jdmason, kernel, festevam, linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa
Frank,
The patch title needs work:
"irqchip: Add IMX MU MSI controller driver"
On Fri, 12 Aug 2022 22:52:40 +0100,
Frank Li <Frank.Li@nxp.com> wrote:
>
> MU support generate irq by write data to a register.
"The MU block found in a number of Freescale/NXP SoCs supports
generating IRQs by writing data to a register."
> This patch make mu worked as msi controller.
Please see Documentation/process/submitting-patches.rst, and the
requirement to avoid wordings such as "This patch".
> So MU can do doorbell by using standard msi api.
"This enables the MU block to be used as a MSI controller, by
leveraging the platform-MSI API"
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/irqchip/Kconfig | 7 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-imx-mu-msi.c | 443 +++++++++++++++++++++++++++++++
> 3 files changed, 451 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..bb111412d598f
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP MU worked as MSI controller
Freescale? Or NXP? Please make up your mind.
> + *
> + * 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/irqchip.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_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 = BIT(0),
> + IMX_MU_V2 = BIT(1),
> + IMX_MU_V2_S4 = BIT(15),
> +};
> +
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +
> +struct imx_mu_dcfg {
> + enum imx_mu_type type;
> + u32 xTR; /* Transmit Register0 */
> + u32 xRR; /* Receive Register0 */
> + u32 xSR[4]; /* Status Registers */
> + u32 xCR[4]; /* Control Registers */
> +};
> +
> +struct imx_mu_msi {
> + spinlock_t lock;
> + struct platform_device *pdev;
This pointer isn't useful. It is only used in
imx_mu_msi_domains_init(), which could take it as a parameter.
> + struct irq_domain *parent;
This pointer isn't useful. It is only used in the same function, and
could well be a local variable.
> + struct irq_domain *msi_domain;
> + void __iomem *regs;
> + phys_addr_t msiir_addr;
> + const struct imx_mu_dcfg *cfg;
> + unsigned long used;
> + int gic_irq;
This variable is only used in a single function.
> + struct clk *clk;
> + struct device *pd_a;
> + struct device *pd_b;
> + struct device_link *pd_link_a;
> + struct device_link *pd_link_b;
Same thing. All this pd_* stuff is *never* used outside of a single
function.
> +};
> +
> +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);
This needs to be a raw spinlock.
> + 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_parent_mask_irq(struct irq_data *data)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> +}
> +
> +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> +}
> +
> +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> +}
> +
> +static struct irq_chip imx_mu_msi_irq_chip = {
> + .name = "MU-MSI",
> + .irq_ack = irq_chip_ack_parent,
Crucially, no irq_write_msi_msg callback. So we happily inherit
platform_msi_write_msg() and use the per descriptor write_msg()
callback. Who sets this? Nobody.
So I suspect you're hiding it somewhere else, and I really want to see
this code. I really don't see a good reason why it should be anywhere
else.
> +};
> +
> +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> +};
> +
> +static struct msi_domain_info imx_mu_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> + .ops = &imx_mu_msi_irq_ops,
> + .chip = &imx_mu_msi_irq_chip,
> +};
> +
> +static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> +
> + msg->address_hi = upper_32_bits(addr);
> + msg->address_lo = lower_32_bits(addr);
> + msg->data = data->hwirq;
> +}
> +
> +static struct irq_chip imx_mu_msi_parent_chip = {
> + .name = "MU",
> + .irq_mask = imx_mu_msi_parent_mask_irq,
> + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> + .irq_ack = imx_mu_msi_parent_ack_irq,
> + .irq_compose_msi_msg = imx_mu_msi_compose_msg,
Please be consistent in the naming.
> +};
> +
> +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> + unsigned int virq,
> + unsigned int nr_irqs,
> + void *args)
> +{
> + struct imx_mu_msi *msi_data = domain->host_data;
> + unsigned long flags;
> + int pos, err = 0;
> +
> + WARN_ON(nr_irqs != 1);
> +
> + spin_lock_irqsave(&msi_data->lock, flags);
> + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> + if (pos < IMX_MU_CHANS)
> + __set_bit(pos, &msi_data->used);
> + else
> + err = -ENOSPC;
> + spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> + if (err)
> + return err;
> +
> + irq_domain_set_info(domain, virq, pos,
> + &imx_mu_msi_parent_chip, msi_data,
> + handle_edge_irq, NULL, NULL);
> + return 0;
> +}
> +
> +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&msi_data->lock, flags);
> + __clear_bit(d->hwirq, &msi_data->used);
> + spin_unlock_irqrestore(&msi_data->lock, flags);
> +}
> +
> +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> + .alloc = imx_mu_msi_domain_irq_alloc,
> + .free = imx_mu_msi_domain_irq_free,
> +};
> +
> +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> +{
> + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> + 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, i)) {
> + generic_handle_domain_irq(msi_data->msi_domain, i);
> + }
> + }
> + chained_irq_exit(irq_desc_get_chip(desc), desc);
Do yourself a favour, and compute irq_desc_get_chip(desc) once, just
like for most irqchips.
> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> +{
> + struct fwnode_handle *fwnodes = of_node_to_fwnode(dev_of_node(&msi_data->pdev->dev));
How about dev_fwnode()?
> +
> + /* Initialize MSI domain parent */
> + msi_data->parent = irq_domain_create_linear(fwnodes,
> + IMX_MU_CHANS,
> + &imx_mu_msi_domain_ops,
> + msi_data);
Consider setting the bus_token attribute for this domain to something
that isn't the default, as it otherwise clashes with the following
creation.
> + 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),
Why aren't you using the 'fwnodes' variable here?
> + &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;
> + }
> +
> + /* clean irq_set_affinity again because it is chained irq */
> + imx_mu_msi_irq_chip.irq_set_affinity = NULL;
NAK. The way to do this is to provide a callback that returns -EINVAL,
not to try and adjust things after the facts.
> +
> + irq_domain_set_pm_device(msi_data->msi_domain, &msi_data->pdev->dev);
> +
> + return 0;
> +}
> +
> +/* Register offset of different version MU IP */
> +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 int __init imx_mu_of_init(struct device_node *dn,
> + struct device_node *parent,
> + const struct imx_mu_dcfg *cfg)
> +{
> + struct platform_device *pdev = of_find_device_by_node(dn);
> + struct imx_mu_msi *msi_data, *priv;
> + struct resource *res;
> + struct device *dev;
> + int ret;
> +
> + if (!pdev)
> + return -ENODEV;
> +
> + dev = &pdev->dev;
> +
> + priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> + if (!msi_data)
> + return -ENOMEM;
> +
> + msi_data->cfg = cfg;
> +
> + msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "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->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");
I'm sorry, but you'll have to come up with something slightly more
descriptive than "a" or "b". At least add a qualifier to it. Same
thing for the DT by the way.
> + 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;
How about the clocks, the links, and everything else that has been
allocated, enabled?
> +
> + irq_set_chained_handler_and_data(msi_data->gic_irq,
> + imx_mu_msi_irq_handler,
> + msi_data);
> +
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
> +{
> + struct imx_mu_msi *priv = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(priv->clk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
> +{
> + struct imx_mu_msi *priv = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(priv->clk);
> + if (ret)
> + dev_err(dev, "failed to enable clock\n");
> +
> + return ret;
> +}
> +
> +static const struct dev_pm_ops imx_mu_pm_ops = {
> + SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
> + imx_mu_runtime_resume, NULL)
> +};
> +
> +static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
> + struct device_node *parent)
> +{
> + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
> +}
> +
> +static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
> + struct device_node *parent)
> +{
> + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
> +}
> +
> +static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
> + struct device_node *parent)
> +{
> + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
> +}
> +
> +static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
> + struct device_node *parent)
> +{
> + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
> +}
> +
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
> +IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
> +IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
> +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
> +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
> +IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm = &imx_mu_pm_ops)
> +
> +
> +MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
> +MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
Please come up with a better description. Something like
"Freescale MU MSI controller driver"
> +MODULE_LICENSE("GPL");
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
2022-08-12 21:52 ` [PATCH v4 2/4] irqchip: imx mu worked as msi controller Frank Li
2022-08-13 9:19 ` Marc Zyngier
@ 2022-08-13 11:57 ` Marc Zyngier
2022-08-14 2:53 ` [EXT] " Frank Li
1 sibling, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2022-08-13 11:57 UTC (permalink / raw
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa
On Fri, 12 Aug 2022 22:52:40 +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>
May I add that it wouldn't hurt if you checked what addresses you send
your patches to? For example, 'kernel@vger.kernel.org' doesn't exist
(maybe you meant linux-kernel@...).
I will not take patches that haven't been posted to LKML (and I
shouldn't have reviewed it the first place).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
2022-08-13 11:57 ` Marc Zyngier
@ 2022-08-14 2:53 ` Frank Li
0 siblings, 0 replies; 11+ messages in thread
From: Frank Li @ 2022-08-14 2:53 UTC (permalink / raw
To: Marc Zyngier
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,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-pci@vger.kernel.org, Peng Fan, Aisheng Dong,
jdmason@kudzu.us, kernel@pengutronix.de, festevam@gmail.com,
dl-linux-imx, kishon@ti.com, lorenzo.pieralisi@arm.com,
ntb@lists.linux.dev, lznuaa@gmail.com
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Saturday, August 13, 2022 6:57 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;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> pci@vger.kernel.org; Peng Fan <peng.fan@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; jdmason@kudzu.us; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; kishon@ti.com;
> lorenzo.pieralisi@arm.com; ntb@lists.linux.dev; lznuaa@gmail.com
> Subject: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
>
> Caution: EXT Email
>
> On Fri, 12 Aug 2022 22:52:40 +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>
>
> May I add that it wouldn't hurt if you checked what addresses you send
> your patches to? For example, 'kernel@vger.kernel.org' doesn't exist
> (maybe you meant linux-kernel@...).
[Frank Li] Thanks. Strang, My email system have not report error.
>
> I will not take patches that haven't been posted to LKML (and I
> shouldn't have reviewed it the first place).
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
2022-08-13 9:19 ` Marc Zyngier
@ 2022-08-14 3:12 ` Frank Li
2022-08-15 8:58 ` Marc Zyngier
0 siblings, 1 reply; 11+ messages in thread
From: Frank Li @ 2022-08-14 3:12 UTC (permalink / raw
To: Marc Zyngier
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,
kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
Peng Fan, Aisheng Dong, jdmason@kudzu.us, kernel@pengutronix.de,
festevam@gmail.com, dl-linux-imx, kishon@ti.com,
lorenzo.pieralisi@arm.com, ntb@lists.linux.dev, lznuaa@gmail.com
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Saturday, August 13, 2022 4:19 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;
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com
> Subject: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
>
> Caution: EXT Email
>
> Frank,
>
> The patch title needs work:
>
> "irqchip: Add IMX MU MSI controller driver"
>
> On Fri, 12 Aug 2022 22:52:40 +0100,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > MU support generate irq by write data to a register.
>
> "The MU block found in a number of Freescale/NXP SoCs supports
> generating IRQs by writing data to a register."
>
> > This patch make mu worked as msi controller.
>
> Please see Documentation/process/submitting-patches.rst, and the
> requirement to avoid wordings such as "This patch".
>
> > So MU can do doorbell by using standard msi api.
>
> "This enables the MU block to be used as a MSI controller, by
> leveraging the platform-MSI API"
>
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > drivers/irqchip/Kconfig | 7 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-imx-mu-msi.c | 443
> +++++++++++++++++++++++++++++++
> > 3 files changed, 451 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..bb111412d598f
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,443 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * NXP MU worked as MSI controller
>
> Freescale? Or NXP? Please make up your mind.
[Frank Li] NXP and freescale is the same thing.
It is mux used at many place.
>
> > + *
> > + * 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/irqchip.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_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 = BIT(0),
> > + IMX_MU_V2 = BIT(1),
> > + IMX_MU_V2_S4 = BIT(15),
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > + enum imx_mu_type type;
> > + u32 xTR; /* Transmit Register0 */
> > + u32 xRR; /* Receive Register0 */
> > + u32 xSR[4]; /* Status Registers */
> > + u32 xCR[4]; /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > + spinlock_t lock;
> > + struct platform_device *pdev;
>
> This pointer isn't useful. It is only used in
> imx_mu_msi_domains_init(), which could take it as a parameter.
>
> > + struct irq_domain *parent;
>
> This pointer isn't useful. It is only used in the same function, and
> could well be a local variable.
>
> > + struct irq_domain *msi_domain;
> > + void __iomem *regs;
> > + phys_addr_t msiir_addr;
> > + const struct imx_mu_dcfg *cfg;
> > + unsigned long used;
> > + int gic_irq;
>
> This variable is only used in a single function.
>
> > + struct clk *clk;
> > + struct device *pd_a;
> > + struct device *pd_b;
> > + struct device_link *pd_link_a;
> > + struct device_link *pd_link_b;
>
> Same thing. All this pd_* stuff is *never* used outside of a single
> function.
>
> > +};
> > +
> > +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);
>
> This needs to be a raw spinlock.
>
> > + 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_parent_mask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > + .name = "MU-MSI",
> > + .irq_ack = irq_chip_ack_parent,
>
> Crucially, no irq_write_msi_msg callback. So we happily inherit
> platform_msi_write_msg() and use the per descriptor write_msg()
> callback. Who sets this? Nobody.
[Frank Li] when set flag MSI_FLAG_USE_DEF_CHIP_OPS,
irq_write_msi_msg callback will be set at function platform_msi_update_chip_ops();
>
> So I suspect you're hiding it somewhere else, and I really want to see
> this code. I really don't see a good reason why it should be anywhere
> else.
>
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > + .ops = &imx_mu_msi_irq_ops,
> > + .chip = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_compose_msg(struct irq_data *data, struct
> msi_msg *msg)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > + msg->address_hi = upper_32_bits(addr);
> > + msg->address_lo = lower_32_bits(addr);
> > + msg->data = data->hwirq;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > + .name = "MU",
> > + .irq_mask = imx_mu_msi_parent_mask_irq,
> > + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> > + .irq_ack = imx_mu_msi_parent_ack_irq,
> > + .irq_compose_msi_msg = imx_mu_msi_compose_msg,
>
> Please be consistent in the naming.
>
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > + unsigned int virq,
> > + unsigned int nr_irqs,
> > + void *args)
> > +{
> > + struct imx_mu_msi *msi_data = domain->host_data;
> > + unsigned long flags;
> > + int pos, err = 0;
> > +
> > + WARN_ON(nr_irqs != 1);
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > + if (pos < IMX_MU_CHANS)
> > + __set_bit(pos, &msi_data->used);
> > + else
> > + err = -ENOSPC;
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > + if (err)
> > + return err;
> > +
> > + irq_domain_set_info(domain, virq, pos,
> > + &imx_mu_msi_parent_chip, msi_data,
> > + handle_edge_irq, NULL, NULL);
> > + return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > + unsigned int virq, unsigned int nr_irqs)
> > +{
> > + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + __clear_bit(d->hwirq, &msi_data->used);
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > + .alloc = imx_mu_msi_domain_irq_alloc,
> > + .free = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > + 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, i)) {
> > + generic_handle_domain_irq(msi_data->msi_domain, i);
> > + }
> > + }
> > + chained_irq_exit(irq_desc_get_chip(desc), desc);
>
> Do yourself a favour, and compute irq_desc_get_chip(desc) once, just
> like for most irqchips.
>
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> > +{
> > + struct fwnode_handle *fwnodes =
> of_node_to_fwnode(dev_of_node(&msi_data->pdev->dev));
>
> How about dev_fwnode()?
>
> > +
> > + /* Initialize MSI domain parent */
> > + msi_data->parent = irq_domain_create_linear(fwnodes,
> > + IMX_MU_CHANS,
> > + &imx_mu_msi_domain_ops,
> > + msi_data);
>
> Consider setting the bus_token attribute for this domain to something
> that isn't the default, as it otherwise clashes with the following
> creation.
[Frank Li] Any suggestion? Which bus_token is good?
>
> > + 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),
>
> Why aren't you using the 'fwnodes' variable here?
>
> > + &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;
> > + }
> > +
> > + /* clean irq_set_affinity again because it is chained irq */
> > + imx_mu_msi_irq_chip.irq_set_affinity = NULL;
>
> NAK. The way to do this is to provide a callback that returns -EINVAL,
> not to try and adjust things after the facts.
>
> > +
> > + irq_domain_set_pm_device(msi_data->msi_domain, &msi_data-
> >pdev->dev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +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 int __init imx_mu_of_init(struct device_node *dn,
> > + struct device_node *parent,
> > + const struct imx_mu_dcfg *cfg)
> > +{
> > + struct platform_device *pdev = of_find_device_by_node(dn);
> > + struct imx_mu_msi *msi_data, *priv;
> > + struct resource *res;
> > + struct device *dev;
> > + int ret;
> > +
> > + if (!pdev)
> > + return -ENODEV;
> > +
> > + dev = &pdev->dev;
> > +
> > + priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> > + if (!msi_data)
> > + return -ENOMEM;
> > +
> > + msi_data->cfg = cfg;
> > +
> > + msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "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->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");
>
> I'm sorry, but you'll have to come up with something slightly more
> descriptive than "a" or "b". At least add a qualifier to it. Same
> thing for the DT by the way.
[Frank Li] MU spec using term "A side" and "B side". So I think "a" and "b"
is enough.
Or do you think "a-side" is better?
>
> > + 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;
>
> How about the clocks, the links, and everything else that has been
> allocated, enabled?
>
> > +
> > + irq_set_chained_handler_and_data(msi_data->gic_irq,
> > + imx_mu_msi_irq_handler,
> > + msi_data);
> > +
> > + pm_runtime_enable(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
> > +{
> > + struct imx_mu_msi *priv = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(priv->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
> > +{
> > + struct imx_mu_msi *priv = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret)
> > + dev_err(dev, "failed to enable clock\n");
> > +
> > + return ret;
> > +}
> > +
> > +static const struct dev_pm_ops imx_mu_pm_ops = {
> > + SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
> > + imx_mu_runtime_resume, NULL)
> > +};
> > +
> > +static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
> > + struct device_node *parent)
> > +{
> > + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
> > +}
> > +
> > +static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
> > + struct device_node *parent)
> > +{
> > + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
> > +}
> > +
> > +static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
> > + struct device_node *parent)
> > +{
> > + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
> > +}
> > +
> > +static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
> > + struct device_node *parent)
> > +{
> > + return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
> > +}
> > +
> > +IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
> > +IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
> > +IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
> > +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
> > +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
> > +IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm =
> &imx_mu_pm_ops)
> > +
> > +
> > +MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
>
> Please come up with a better description. Something like
> "Freescale MU MSI controller driver"
>
> > +MODULE_LICENSE("GPL");
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/4] dt-bindings: irqchip: imx mu work as msi controller
2022-08-12 21:52 ` [PATCH v4 3/4] dt-bindings: irqchip: imx mu work " Frank Li
@ 2022-08-14 20:41 ` Rob Herring
0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2022-08-14 20:41 UTC (permalink / raw
To: Frank Li
Cc: maz, tglx, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, kernel, devicetree, linux-arm-kernel, linux-pci,
peng.fan, aisheng.dong, jdmason, kernel, festevam, linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa
On Fri, Aug 12, 2022 at 04:52:41PM -0500, Frank Li wrote:
> I.MX mu support generate irq by write a register. Provide msi controller
> support so other driver such as PCI EP can use it by standard msi
> interface as doorbell.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> .../interrupt-controller/fsl,mu-msi.yaml | 93 +++++++++++++++++++
> 1 file changed, 93 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..f60fa8b686879
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
> @@ -0,0 +1,93 @@
> +# 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) work as msi controller
> +
> +maintainers:
> + - Frank Li <Frank.Li@nxp.com>
> +
> +description: |
> + The Messaging Unit module enables two processors within the SoC to
> + communicate and coordinate by passing messages (e.g. data, status
> + and control) through the MU interface. The MU also provides the ability
> + for one processor (A side) to signal the other processor (B side) using
> + interrupts.
> +
> + Because the MU manages the messaging between processors, the MU uses
> + different clocks (from each side of the different peripheral buses).
> + Therefore, the MU must synchronize the accesses from one side to the
> + other. The MU accomplishes synchronization using two sets of matching
> + registers (Processor A-facing, Processor B-facing).
> +
> + MU can work as msi interrupt controller to do doorbell
> +
> +allOf:
> + - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - fsl,imx6sx-mu-msi
> + - fsl,imx7ulp-mu-msi
> + - fsl,imx8ulp-mu-msi
> + - fsl,imx8ulp-mu-msi-s4
> +
> + reg:
> + items:
> + - description: a side register base address
> + - description: b side register base address
> +
> + reg-names:
> + items:
> + - const: a
> + - const: b
> +
> + interrupts:
> + description: a side interrupt number.
How many?
> +
> + clocks:
> + maxItems: 1
> +
> + power-domains:
> + items:
> + - description: a side power domain
> + - description: b side power domain
> +
> + power-domain-names:
> + items:
> + - const: a
> + - const: b
> +
> + interrupt-controller: true
> +
> + msi-controller: true
#msi-cells?
(Missing is treated as 0, but new bindings should be explicit)
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - interrupt-controller
> + - msi-controller
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/firmware/imx/rsrc.h>
> +
> + lsio_mu12: msi-controller@5d270000 {
Drop unused labels.
> + compatible = "fsl,imx6sx-mu-msi";
> + msi-controller;
> + interrupt-controller;
> + reg = <0x5d270000 0x10000>, /* A side */
> + <0x5d300000 0x10000>; /* B side */
> + reg-names = "a", "b";
> + interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
> + power-domains = <&pd IMX_SC_R_MU_12A>,
> + <&pd IMX_SC_R_MU_12B>;
> + power-domain-names = "a", "b";
> + };
> --
> 2.35.1
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
2022-08-14 3:12 ` [EXT] " Frank Li
@ 2022-08-15 8:58 ` Marc Zyngier
0 siblings, 0 replies; 11+ messages in thread
From: Marc Zyngier @ 2022-08-15 8:58 UTC (permalink / raw
To: Frank Li
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,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-pci@vger.kernel.org, Peng Fan, Aisheng Dong,
jdmason@kudzu.us, kernel@pengutronix.de, festevam@gmail.com,
dl-linux-imx, kishon@ti.com, lorenzo.pieralisi@arm.com,
ntb@lists.linux.dev, lznuaa@gmail.com
On Sun, 14 Aug 2022 04:12:01 +0100,
Frank Li <frank.li@nxp.com> wrote:
> > > new file mode 100644
> > > index 0000000000000..bb111412d598f
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > > @@ -0,0 +1,443 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * NXP MU worked as MSI controller
> >
> > Freescale? Or NXP? Please make up your mind.
>
>
> [Frank Li] NXP and freescale is the same thing.
> It is mux used at many place.
Pick one, and stick to it. Having two names for the same thing is
pointlessly confusing.
> > > +static struct irq_chip imx_mu_msi_irq_chip = {
> > > + .name = "MU-MSI",
> > > + .irq_ack = irq_chip_ack_parent,
> >
> > Crucially, no irq_write_msi_msg callback. So we happily inherit
> > platform_msi_write_msg() and use the per descriptor write_msg()
> > callback. Who sets this? Nobody.
>
> [Frank Li] when set flag MSI_FLAG_USE_DEF_CHIP_OPS,
> irq_write_msi_msg callback will be set at function platform_msi_update_chip_ops();
That wasn't my question. But never mind, I found the call to
platform_msi_domain_alloc_irqs() in patch #4.
> > > +
> > > + /* Initialize MSI domain parent */
> > > + msi_data->parent = irq_domain_create_linear(fwnodes,
> > > + IMX_MU_CHANS,
> > > + &imx_mu_msi_domain_ops,
> > > + msi_data);
> >
> > Consider setting the bus_token attribute for this domain to something
> > that isn't the default, as it otherwise clashes with the following
> > creation.
>
> [Frank Li] Any suggestion? Which bus_token is good?
DOMAIN_BUS_NEXUS is what other drivers use.
> > > + priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> >
> > I'm sorry, but you'll have to come up with something slightly more
> > descriptive than "a" or "b". At least add a qualifier to it. Same
> > thing for the DT by the way.
>
> [Frank Li] MU spec using term "A side" and "B side". So I think "a" and "b"
> is enough.
No, it really isn't.
>
> Or do you think "a-side" is better?
No, I would like something fully descriptive. The DT actually has
"Processor A-facing", which seems like a reasonable description.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-08-15 8:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-12 21:52 [PATCH v4 0/4] PCI EP driver support MSI doorbell from host Frank Li
2022-08-12 21:52 ` [PATCH v4 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
2022-08-12 21:52 ` [PATCH v4 2/4] irqchip: imx mu worked as msi controller Frank Li
2022-08-13 9:19 ` Marc Zyngier
2022-08-14 3:12 ` [EXT] " Frank Li
2022-08-15 8:58 ` Marc Zyngier
2022-08-13 11:57 ` Marc Zyngier
2022-08-14 2:53 ` [EXT] " Frank Li
2022-08-12 21:52 ` [PATCH v4 3/4] dt-bindings: irqchip: imx mu work " Frank Li
2022-08-14 20:41 ` Rob Herring
2022-08-12 21:52 ` [PATCH v4 4/4] pcie: endpoint: pci-epf-vntb: add endpoint MSI support Frank Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).