> -----Original Message-----
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <frank.li@xxxxxxx>
> Cc: tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan
> <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> jdmason@xxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-
> imx <linux-imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx;
> ntb@xxxxxxxxxxxxxxx; lznuaa@xxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
>
> Caution: EXT Email
>
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <Frank.Li@xxxxxxx> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
>
> Missing full stop after each sentence.
[Frank Li] Do you means missed "."?
>
> >
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > drivers/irqchip/Kconfig | 9 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> > 3 files changed, 461 insertions(+)
> > create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > help
> > Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > + bool "i.MX MU work as MSI controller"
>
> Why bool? Doesn't it also work as a module?
[Frank Li] I remember you said that irq-chip can't be removed.
So I am not sure why need build as module.
>
> > + default y if ARCH_MXC
>
> Why would this be selected by default?
>
> > + select IRQ_DOMAIN
> > + select IRQ_DOMAIN_HIERARCHY
> > + select GENERIC_MSI_IRQ_DOMAIN
> > + help
> > + MU work as MSI controller to do general doorbell
>
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.
>
> > +
> > 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..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
>
> s/worked/used/
>
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <o.rempel@xxxxxxxxxxxxxx>
> > + * Copyright 2022 NXP
> > + * Frank Li <Frank.Li@xxxxxxx>
> > + * Peng Fan <peng.fan@xxxxxxx>
> > + *
> > + * 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>
>
> Keep this list in alphabetical order.
>
> > +
> > +
> > +#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,
>
> What is this last enum used for?
>
> > +};
> > +
> > +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),
>
> This is never used. Why?
>
> > + IMX_MU_V2 = BIT(1),
> > + IMX_MU_V2_S4 = BIT(15),
>
> Same thing.
>
> > +};
> > +
> > +/* 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;
> > + raw_spinlock_t reglock;
>
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?
[Frank Li] Previously your comment, ask me to use raw_spinlock for read\write register access.
I don't think raw_spinlock is good for MSI allocation.
>
> Also, where are these locks initialised?
>
[Frank Li] struct imx_mu_msi is fill zero when allocated.
Does it still need additional initialization for spinlock?
> > + struct irq_domain *msi_domain;
> > + void __iomem *regs;
> > + phys_addr_t msiir_addr;
> > + const struct imx_mu_dcfg *cfg;
> > + unsigned long used;
> > + struct clk *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > + iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > + return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > + val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > + val &= ~clr;
> > + val |= set;
> > + imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > + raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > + return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > + .name = "MU-MSI",
> > + .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > + .ops = &imx_mu_msi_irq_ops,
> > + .chip = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > + struct msi_msg *msg)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > + msg->address_hi = upper_32_bits(addr);
> > + msg->address_lo = lower_32_bits(addr);
> > + msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > + const struct cpumask *mask, bool force)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > + .name = "MU",
> > + .irq_mask = imx_mu_msi_parent_mask_irq,
> > + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> > + .irq_ack = imx_mu_msi_parent_ack_irq,
> > + .irq_compose_msi_msg = imx_mu_msi_parent_compose_msg,
> > + .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > + unsigned int virq,
> > + unsigned int nr_irqs,
> > + void *args)
> > +{
> > + struct imx_mu_msi *msi_data = domain->host_data;
> > + unsigned long flags;
> > + int pos, err = 0;
> > +
> > + WARN_ON(nr_irqs != 1);
> > +
> > + 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);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + u32 status;
> > + int i;
> > +
> > + status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > + chained_irq_enter(chip, desc);
> > + for (i = 0; i < IMX_MU_CHANS; i++) {
> > + if (status & IMX_MU_xSR_RFn(msi_data, i))
> > + generic_handle_domain_irq(msi_data->msi_domain, i);
> > + }
> > + chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > + struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > + struct irq_domain *parent;
> > +
> > + /* Initialize MSI domain parent */
> > + parent = irq_domain_create_linear(fwnodes,
> > + IMX_MU_CHANS,
> > + &imx_mu_msi_domain_ops,
> > + msi_data);
> > + if (!parent) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > + msi_data->msi_domain = platform_msi_create_irq_domain(
> > + fwnodes,
> > + &imx_mu_msi_domain_info,
> > + parent);
>
> nit: move the first argument after the opening bracket (longer lines
> are fine).
>
> > +
> > + if (!msi_data->msi_domain) {
> > + dev_err(dev, "failed to create MSI domain\n");
> > + irq_domain_remove(parent);
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>
> Why doesn't this have a type?
>
> > + .xTR = 0x0,
> > + .xRR = 0x10,
> > + .xSR = {0x20, 0x20, 0x20, 0x20},
>
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
>
> .xSR = {
> [IMX_MU_SR] = 0x20,
> [IMX_MU_GSR] = 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
> > + )
>
> Move closing bracket after 'cfg'.
>
> > +{
> > + struct platform_device *pdev = of_find_device_by_node(dn);
> > + struct device_link *pd_link_a;
> > + struct device_link *pd_link_b;
> > + struct imx_mu_msi *msi_data;
> > + struct resource *res;
> > + struct device *pd_a;
> > + struct device *pd_b;
> > + struct device *dev;
> > + int ret;
> > + int irq;
> > +
> > + if (!pdev)
> > + return -ENODEV;
>
> How can that happen?
>
[Frank Li] Not sure, many driver check as it.
> > +
> > + dev = &pdev->dev;
> > +
> > + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> > + if (!msi_data)
> > + return -ENOMEM;
> > +
> > + msi_data->cfg = cfg;
> > +
> > + msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > + if (IS_ERR(msi_data->regs)) {
> > + dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > + return PTR_ERR(msi_data->regs);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > + if (!res)
> > + return -EIO;
> > +
> > + msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq <= 0)
> > + return -ENODEV;
> > +
> > + platform_set_drvdata(pdev, msi_data);
> > +
> > + msi_data->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(msi_data->clk)) {
> > + if (PTR_ERR(msi_data->clk) != -ENOENT)
> > + return PTR_ERR(msi_data->clk);
> > +
> > + msi_data->clk = NULL;
>
> Why is it acceptable to continue with no clock?
>
> > + }
> > +
> > + pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > + if (IS_ERR(pd_a))
> > + return PTR_ERR(pd_a);
> > +
> > + pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > + if (IS_ERR(pd_b))
> > + return PTR_ERR(pd_b);
> > +
> > + pd_link_a = device_link_add(dev, pd_a,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > + if (!pd_link_a) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_a;
> > + }
> > +
> > + pd_link_b = device_link_add(dev, pd_b,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > + if (!pd_link_b) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_b;
> > + }
> > +
> > + ret = imx_mu_msi_domains_init(msi_data, dev);
> > + if (ret)
> > + goto err_dm_init;
> > +
> > + irq_set_chained_handler_and_data(irq,
> > + imx_mu_msi_irq_handler,
> > + msi_data);
> > +
> > + pm_runtime_enable(dev);
>
> Shouldn't you enable the device PM before registering the chained
> handler?
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
