RE: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller | linux-arm-kernel

quoted

-----Original Message-----
From: Marc Zyngier <maz@kernel.org>
Sent: Thursday, July 21, 2022 10:28 AM
To: Frank Li <frank.li@nxp.com>
Cc: jdmason@kudzu.us; 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
[off-list ref]; Aisheng Dong [off-list ref];
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 v3 2/4] irqchip: imx mu worked as msi
controller

Caution: EXT Email

On Thu, 21 Jul 2022 16:22:08 +0100,
Frank Li [off-list ref] wrote:


-----Original Message-----
From: Marc Zyngier <maz@kernel.org>
Sent: Thursday, July 21, 2022 2:57 AM
To: Frank Li <frank.li@nxp.com>
Cc: jdmason@kudzu.us; 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
[off-list ref]; Aisheng Dong [off-list ref];
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 v3 2/4] irqchip: imx mu worked as msi
controller
Caution: EXT Email

On Wed, 20 Jul 2022 22:30:34 +0100,
Frank Li [off-list ref] 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 | 462
+++++++++++++++++++++++++++++++
 3 files changed, 470 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-mu-msi.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5e4e50122777d..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..8277dba857759
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,462 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NXP MU worked as MSI controller
+ *
+ * Copyright (c) 2018 Pengutronix, Oleksij Rempel
[off-list ref]
+ * Copyright 2022 NXP
+ *   Frank Li [off-list ref]
+ *   Peng Fan [off-list ref]
+ *
+ * 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(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;
+     const struct imx_mu_dcfg        *cfg;
+     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);
Urgh... No. Please don't go poking into the *parent* stuff. Implement
the masking at the parent level, and use irq_chip_mask_parent() for
this level, unless you can explain why you can't do otherwise.

+
+     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));

How about making this readable and move the dereference inside the
macro?

+}
+
+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);
+
+     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 = {
+};
What does this have to do with the ITS?
Without this, it will be crashed as access 0 address.
Because the *name* of the structure has an influence on the behaviour
of the kernel?????
I understand your means now.  The name "its_pmsi_ops" is wrong. 
Not ask why empty structure here.  




+
+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),
What does PCI_MSIX mean in this context? I really wish you used
copy/paste a bit more carefully.

+     .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);
This is a horrible way of writing this. you should compute the address
first, and then apply the address split.

+     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;
+}
Err... What effect does this have if you don't do anything?
[Frank Li] Without this, it will be crashed as access 0 address.
Then you should fix that bug, because what you have here makes
absolutely no sense.



+
+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;
+
+     WARN_ON(nr_irqs != 1);
+
+     spin_lock(&msi_data->lock);
Interrupt fires after lock acquisition, handler masks the interrupt.
Deadlock.
[Frank Li] you are right, it should be spin_lock_irqsave.


+     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(&msi_data->lock);
+
+     if (err)
+             return err;
+
+     err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr
+
pos * 4);

Does this HW even have an IOMMU?
[Frank Li] we have a platform with iommu.


+     if (err)
+             return err;
+
+     irq_domain_set_info(domain, virq, pos,
+                         &imx_mu_msi_parent_chip, msi_data,
+                         handle_simple_irq, NULL, NULL);
This is an edge interrupt. Please handle it like one.
[Frank Li]  Not sure what your means?
A MSI is an edge interrupt. Use handle_edge_irq.



+     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);
+
+     spin_lock(&msi_data->lock);
Same problem.

+     __clear_bit(d->hwirq, &msi_data->used);
+     spin_unlock(&msi_data->lock);
+}
+
+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);
Why the parent? You must start at the top of the hierarchy.

+             }
+     }
+     chained_irq_exit(irq_desc_get_chip(desc), desc);
If your MSIs are a chained interrupt, why do you even provide an
affinity setting callback?
[Frank Li]  it will be crash if no affinity setting callback.
Then you have to fix your driver.

        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

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