[v3,2/4] irqchip: imx mu worked as msi controller

Commit Message

Frank Li July 20, 2022, 9:30 p.m. UTC

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 <[e-mail]>
---
 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

Comments

Marc Zyngier July 21, 2022, 7:57 a.m. UTC | #1

On Wed, 20 Jul 2022 22:30:34 +0100,
Frank Li <[e-mail]> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
> + * Copyright 2022 NXP
> + *	Frank Li <[e-mail]>
> + *	Peng Fan <[e-mail]>
> + *
> + * 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?

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

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

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

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

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

> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> +{
> +	/* Initialize MSI domain parent */
> +	msi_data->parent = irq_domain_add_linear(dev_of_node(&msi_data->pdev->dev),
> +						 IMX_MU_CHANS,
> +						 &imx_mu_msi_domain_ops,
> +						 msi_data);

Use irq_domain_create_linear() instead.

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

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?

> +
> +	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;
> +	}
> +
> +	irq_domain_set_pm_device(msi_data->parent, &msi_data->pdev->dev);

I really wonder why you both implementing a MSI domain if you are
always bypassing it for absolutely everything... This completely
nullifies the effect of this call, as no interrupt request will ever
trigger the PM.

> +
> +	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);
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(dev);
> +		goto disable_runtime_pm;
> +	}
> +
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0)
> +		goto disable_runtime_pm;
> +
> +	clk_disable_unprepare(msi_data->clk);

Why do you need to do all this dance?

> +
> +	return 0;
> +
> +disable_runtime_pm:
> +	pm_runtime_disable(dev);
> +	clk_disable_unprepare(msi_data->clk);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
> +{
> +	struct imx_mu_msi *priv = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
> +{
> +	struct imx_mu_msi *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		dev_err(dev, "failed to enable clock\n");
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops imx_mu_pm_ops = {
> +	SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
> +			   imx_mu_runtime_resume, NULL)
> +};
> +
> +static int __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 <[e-mail]>");
> +MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
> +MODULE_LICENSE("GPL");

	M.

Frank Li July 21, 2022, 3:22 p.m. UTC | #2

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, July 21, 2022 2:57 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; dl-linux-imx <linux-
> [e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> 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 <[e-mail]> 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 <[e-mail]>
> > ---
> >  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
> <[e-mail]>
> > + * Copyright 2022 NXP
> > + *   Frank Li <[e-mail]>
> > + *   Peng Fan <[e-mail]>
> > + *
> > + * 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.

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

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

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

> 
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> > +{
> > +     /* Initialize MSI domain parent */
> > +     msi_data->parent = irq_domain_add_linear(dev_of_node(&msi_data-
> >pdev->dev),
> > +                                              IMX_MU_CHANS,
> > +                                              &imx_mu_msi_domain_ops,
> > +                                              msi_data);
> 
> Use irq_domain_create_linear() instead.
> 
> > +     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);
> 
> 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?
> 
> > +
> > +     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;
> > +     }
> > +
> > +     irq_domain_set_pm_device(msi_data->parent, &msi_data->pdev-
> >dev);
> 
> I really wonder why you both implementing a MSI domain if you are
> always bypassing it for absolutely everything... This completely
> nullifies the effect of this call, as no interrupt request will ever
> trigger the PM.
> 
> > +
> > +     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);
> > +
> > +     ret = pm_runtime_get_sync(dev);
> > +     if (ret < 0) {
> > +             pm_runtime_put_noidle(dev);
> > +             goto disable_runtime_pm;
> > +     }
> > +
> > +     ret = pm_runtime_put_sync(dev);
> > +     if (ret < 0)
> > +             goto disable_runtime_pm;
> > +
> > +     clk_disable_unprepare(msi_data->clk);
> 
> Why do you need to do all this dance?
> 
> > +
> > +     return 0;
> > +
> > +disable_runtime_pm:
> > +     pm_runtime_disable(dev);
> > +     clk_disable_unprepare(msi_data->clk);
> > +
> > +     return ret;
> > +}
> > +
> > +static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
> > +{
> > +     struct imx_mu_msi *priv = dev_get_drvdata(dev);
> > +
> > +     clk_disable_unprepare(priv->clk);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
> > +{
> > +     struct imx_mu_msi *priv = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(priv->clk);
> > +     if (ret)
> > +             dev_err(dev, "failed to enable clock\n");
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct dev_pm_ops imx_mu_pm_ops = {
> > +     SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
> > +                        imx_mu_runtime_resume, NULL)
> > +};
> > +
> > +static int __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 <[e-mail]>");
> > +MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
> > +MODULE_LICENSE("GPL");
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier July 21, 2022, 3:28 p.m. UTC | #3

On Thu, 21 Jul 2022 16:22:08 +0100,
Frank Li <[e-mail]> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <[e-mail]>
> > Sent: Thursday, July 21, 2022 2:57 AM
> > To: Frank Li <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-arm-
> > [e-mail]; [e-mail]; Peng Fan
> > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > [e-mail]>; [e-mail]; [e-mail];
> > [e-mail]
> > 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 <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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
> > <[e-mail]>
> > > + * Copyright 2022 NXP
> > > + *   Frank Li <[e-mail]>
> > > + *   Peng Fan <[e-mail]>
> > > + *
> > > + * 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?????

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

Frank Li July 21, 2022, 3:35 p.m. UTC | #4

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, July 21, 2022 10:28 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; dl-linux-imx <linux-
> [e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> 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 <[e-mail]> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <[e-mail]>
> > > Sent: Thursday, July 21, 2022 2:57 AM
> > > To: Frank Li <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; linux-arm-
> > > [e-mail]; [e-mail]; Peng Fan
> > > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > > [e-mail]>; [e-mail]; [e-mail];
> > > [e-mail]
> > > 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 <[e-mail]> 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 <[e-mail]>
> > > > ---
> > > >  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
> > > <[e-mail]>
> > > > + * Copyright 2022 NXP
> > > > + *   Frank Li <[e-mail]>
> > > > + *   Peng Fan <[e-mail]>
> > > > + *
> > > > + * 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.

Marc Zyngier July 22, 2022, 7:33 a.m. UTC | #5

On Thu, 21 Jul 2022 16:22:08 +0100,
Frank Li <[e-mail]> wrote:
> 
> > > +     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.

I really wonder whether you are taking me for a ride, or whether you
are completely misunderstanding what this IOMMU business is about.

This is a *CPU* writing to the device to generate an MSI. CPU
transactions cannot be translated by an IOMMU as they are not
(surprise!) IO devices. They are in control of their own translation,
contrary to devices that generate MSIs.

So what sort of translation do you expect this to setup? What StreamID
is getting used by the DMA framework? There is no answer to these
questions because they don't make any sense. None of it makes any
sense.

At best, you are simply copy-pasting things from various drivers
without understanding what they are all about. I suggest you stop
doing that and make use of your time working out the problem rather
than wasting the reviewers'.

	M.

Frank Li July 22, 2022, 4:12 p.m. UTC | #6

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Friday, July 22, 2022 2:33 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; Peng Fan <[e-mail]>; Aisheng Dong
> <[e-mail]>; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> 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 <[e-mail]> wrote:
> >
> > > > +     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.
> 
> I really wonder whether you are taking me for a ride, or whether you
> are completely misunderstanding what this IOMMU business is about.
> 
> This is a *CPU* writing to the device to generate an MSI. CPU
> transactions cannot be translated by an IOMMU as they are not
> (surprise!) IO devices. They are in control of their own translation,
> contrary to devices that generate MSIs.

[Frank Li] I think that It is not *CPU* writing to the device to generate an MSI. 
It is another bus master to write to generate IRQ.  According to my understand, 
Bus master (such as PCI EP devices) write to an address to trigger irq, OR
DMA setup DMA write descriptor at last one.  Any address should be translate by
IOMMU if it is enabled.  If not,  this is empty function call.  

> 
> So what sort of translation do you expect this to setup? What StreamID
> is getting used by the DMA framework? There is no answer to these
> questions because they don't make any sense. None of it makes any
> sense.
> 
> At best, you are simply copy-pasting things from various drivers
> without understanding what they are all about.
 
[Frank Li] it is first time to write irq chip driver, so I have to base on or reference
Other drivers to start development.  Of course, there are some miss understand
During development.  From functional call name, it make sense although I don't know
All detail behind that.  

Any bus master access memory need go through IOMMU if enabled.  

I have not tested it at IOMMU enabled platform yet.  Some maintainer require
Added similar function call if a feature enabled when I do other upstream work.  
So I kept as refer driver did. Maybe refer driver code is too old.  

If there are problem, I can delete it.  So far, it don't impact my test platform. 
I can add fix patch if met problem at IOMMU enabled platform.

> I suggest you stop
> doing that and make use of your time working out the problem rather
> than wasting the reviewers'.

Thank you for your review and great comments.  This driver function worked. 
I know I made some stupid problem, it should be fixed before sent out.

I also want to make sure the overall idea worked firstly.  So far no-one
Use MSI as doorbell from PCIe-EP function drivers yet.  

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

Frank Li July 26, 2022, 9:48 p.m. UTC | #7

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, July 21, 2022 10:28 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; dl-linux-imx <linux-
> [e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> 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 <[e-mail]> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <[e-mail]>
> > > Sent: Thursday, July 21, 2022 2:57 AM
> > > To: Frank Li <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; linux-arm-
> > > [e-mail]; [e-mail]; Peng Fan
> > > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > > [e-mail]>; [e-mail]; [e-mail];
> > > [e-mail]
> > > 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 <[e-mail]> 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 <[e-mail]>
> > > > ---
> > > >  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
> > > <[e-mail]>
> > > > + * Copyright 2022 NXP
> > > > + *   Frank Li <[e-mail]>
> > > > + *   Peng Fan <[e-mail]>
> > > > + *
> > > > + * 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?????
> 
> >
> > >
> > > > +
> > > > +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.

[Frank Li] Do you means that should be msi_data->msi_domain instead of msi_data->parent? 

> > >
> > > > +             }
> > > > +     }
> > > > +     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.

[Frank Li] After debug,  msi_domain_set_affinity() have not did null check for (parent->chip->irq_set_affinity). 
I think impact by using dummy set_affinity is minimized.  

int msi_domain_set_affinity(struct irq_data *irq_data,	
			    const struct cpumask *mask, bool force)
{
	struct irq_data *parent = irq_data->parent_data;
	struct msi_msg msg[2] = { [1] = { }, };
	int ret;

	ret = parent->chip->irq_set_affinity(parent, mask, force);
	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
		msi_check_level(irq_data->domain, msg);
		irq_chip_write_msi_msg(irq_data, msg);
	}

	return ret;
}
 

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

Marc Zyngier July 27, 2022, 8:02 a.m. UTC | #8

On Tue, 26 Jul 2022 22:48:32 +0100,
Frank Li <[e-mail]> wrote:
> 
> > > > > +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.
> 
> [Frank Li] Do you means that should be msi_data->msi_domain instead
> of msi_data->parent?

Indeed. you must *not* bypass the hierarchy, and the top level of the
hierarchy has to implement whatever is required by the interrupt flow.

> 
> > > >
> > > > > +             }
> > > > > +     }
> > > > > +     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.
> 
> [Frank Li] After debug,  msi_domain_set_affinity() have not did null check for (parent->chip->irq_set_affinity). 
> I think impact by using dummy set_affinity is minimized.  
> 
> int msi_domain_set_affinity(struct irq_data *irq_data,	
> 			    const struct cpumask *mask, bool force)
> {
> 	struct irq_data *parent = irq_data->parent_data;
> 	struct msi_msg msg[2] = { [1] = { }, };
> 	int ret;
> 
> 	ret = parent->chip->irq_set_affinity(parent, mask, force);
> 	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> 		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
> 		msi_check_level(irq_data->domain, msg);
> 		irq_chip_write_msi_msg(irq_data, msg);
> 	}
> 
> 	return ret;
> }

No. Changing the affinity of an interrupt must not affect the affinity
of another. Given that this is a chained handler, you *cannot* satisfy
this requirement. So you can't change the affinity at all.

	N,

Frank Li July 27, 2022, 3:23 p.m. UTC | #9

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Wednesday, July 27, 2022 3:03 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; dl-linux-imx <linux-
> [e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> controller
> 
> Caution: EXT Email
> 
> On Tue, 26 Jul 2022 22:48:32 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > > > > > +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.
> >
> > [Frank Li] Do you means that should be msi_data->msi_domain instead
> > of msi_data->parent?
> 
> Indeed. you must *not* bypass the hierarchy, and the top level of the
> hierarchy has to implement whatever is required by the interrupt flow.	
> 

[Frank Li] I see, just want to confirm msi_data->msi_domain should be correct here?
It should be leaf of irq hierarchy tree. 

> >
> > > > >
> > > > > > +             }
> > > > > > +     }
> > > > > > +     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.
> >
> > [Frank Li] After debug,  msi_domain_set_affinity() have not did null check
> for (parent->chip->irq_set_affinity).
> > I think impact by using dummy set_affinity is minimized.
> >
> > int msi_domain_set_affinity(struct irq_data *irq_data,
> >                           const struct cpumask *mask, bool force)
> > {
> >       struct irq_data *parent = irq_data->parent_data;
> >       struct msi_msg msg[2] = { [1] = { }, };
> >       int ret;
> >
> >       ret = parent->chip->irq_set_affinity(parent, mask, force);
> >       if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> >               BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
> >               msi_check_level(irq_data->domain, msg);
> >               irq_chip_write_msi_msg(irq_data, msg);
> >       }
> >
> >       return ret;
> > }
> 
> No. Changing the affinity of an interrupt must not affect the affinity
> of another. Given that this is a chained handler, you *cannot* satisfy
> this requirement. So you can't change the affinity at all.
> 

[Frank Li] I understand affinity can't be changed. 
But system use set affinity to write msi msg. 

The call stack as
[   25.508229]  epf_ntb_write_msi_msg+0x78/0x90 
[   25.512512]  platform_msi_write_msg+0x2c/0x38
[   25.516882]  msi_domain_set_affinity+0xb0/0xc0 
[   25.521330]  irq_do_set_affinity+0x174/0x220
[   25.525604]  irq_setup_affinity+0xe0/0x188
[   25.529713]  irq_startup+0x88/0x160
[   25.533214]  __setup_irq+0x6c8/0x768

I have not found good place to hook a function to write msi msg. 

int irq_startup(struct irq_desc *desc, bool resend, bool force)
{
        struct irq_data *d = irq_desc_get_irq_data(desc);
        struct cpumask *aff = irq_data_get_affinity_mask(d);
        int ret = 0;

        desc->depth = 0;

        if (irqd_is_started(d)) {
                irq_enable(desc);
        } else {
                switch (__irq_startup_managed(desc, aff, force)) {
                case IRQ_STARTUP_NORMAL:
                        if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)
                                irq_setup_affinity(desc);
                        ret = __irq_startup(desc);
                        if (!(d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP))
                                irq_setup_affinity(desc);
                        break;
                case IRQ_STARTUP_MANAGED:
                        irq_do_set_affinity(d, aff, false);
                        ret = __irq_startup(desc);
                        break;
                case IRQ_STARTUP_ABORT:
                        irqd_set_managed_shutdown(d);
                        return 0;
                }
        }
        if (resend)
                check_irq_resend(desc, false);

        return ret;
}

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

Marc Zyngier July 27, 2022, 3:34 p.m. UTC | #10

On Wed, 27 Jul 2022 16:23:26 +0100,
Frank Li <[e-mail]> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <[e-mail]>
> > Sent: Wednesday, July 27, 2022 3:03 AM
> > To: Frank Li <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-arm-
> > [e-mail]; [e-mail]; Peng Fan
> > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > [e-mail]>; [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> > controller
> > 
> > Caution: EXT Email
> > 
> > On Tue, 26 Jul 2022 22:48:32 +0100,
> > Frank Li <[e-mail]> wrote:
> > >
> > > > > > > +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.
> > >
> > > [Frank Li] Do you means that should be msi_data->msi_domain instead
> > > of msi_data->parent?
> > 
> > Indeed. you must *not* bypass the hierarchy, and the top level of the
> > hierarchy has to implement whatever is required by the interrupt flow.	
> > 
> 
> [Frank Li] I see, just want to confirm msi_data->msi_domain should
> be correct here?  It should be leaf of irq hierarchy tree.

Yes.

> 
> > >
> > > > > >
> > > > > > > +             }
> > > > > > > +     }
> > > > > > > +     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.
> > >
> > > [Frank Li] After debug,  msi_domain_set_affinity() have not did null check
> > for (parent->chip->irq_set_affinity).
> > > I think impact by using dummy set_affinity is minimized.
> > >
> > > int msi_domain_set_affinity(struct irq_data *irq_data,
> > >                           const struct cpumask *mask, bool force)
> > > {
> > >       struct irq_data *parent = irq_data->parent_data;
> > >       struct msi_msg msg[2] = { [1] = { }, };
> > >       int ret;
> > >
> > >       ret = parent->chip->irq_set_affinity(parent, mask, force);
> > >       if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> > >               BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
> > >               msi_check_level(irq_data->domain, msg);
> > >               irq_chip_write_msi_msg(irq_data, msg);
> > >       }
> > >
> > >       return ret;
> > > }
> > 
> > No. Changing the affinity of an interrupt must not affect the affinity
> > of another. Given that this is a chained handler, you *cannot* satisfy
> > this requirement. So you can't change the affinity at all.
> > 
> 
> [Frank Li] I understand affinity can't be changed. 
> But system use set affinity to write msi msg. 
> 
> The call stack as
> [   25.508229]  epf_ntb_write_msi_msg+0x78/0x90 
> [   25.512512]  platform_msi_write_msg+0x2c/0x38
> [   25.516882]  msi_domain_set_affinity+0xb0/0xc0 
> [   25.521330]  irq_do_set_affinity+0x174/0x220
> [   25.525604]  irq_setup_affinity+0xe0/0x188
> [   25.529713]  irq_startup+0x88/0x160
> [   25.533214]  __setup_irq+0x6c8/0x768
> 
> I have not found good place to hook a function to write msi msg.

It is called at MSI activation time (msi_domain_activate).

	M.

Frank Li July 27, 2022, 6:29 p.m. UTC | #11

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Wednesday, July 27, 2022 10:35 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; dl-linux-imx <linux-
> [e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> controller
> 
> Caution: EXT Email
> 
> On Wed, 27 Jul 2022 16:23:26 +0100,
> Frank Li <[e-mail]> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <[e-mail]>
> > > Sent: Wednesday, July 27, 2022 3:03 AM
> > > To: Frank Li <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; linux-arm-
> > > [e-mail]; [e-mail]; Peng Fan
> > > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > > [e-mail]>; [e-mail]; [e-mail];
> > > [e-mail]
> > > Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> > > controller
> > >
> > > Caution: EXT Email
> > >
> > > On Tue, 26 Jul 2022 22:48:32 +0100,
> > > Frank Li <[e-mail]> wrote:
> > > >
> > > > > > > > +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.
> > > >
> > > > [Frank Li] Do you means that should be msi_data->msi_domain instead
> > > > of msi_data->parent?
> > >
> > > Indeed. you must *not* bypass the hierarchy, and the top level of the
> > > hierarchy has to implement whatever is required by the interrupt flow.
> > >
> >
> > [Frank Li] I see, just want to confirm msi_data->msi_domain should
> > be correct here?  It should be leaf of irq hierarchy tree.
> 
> Yes.
> 
> >
> > > >
> > > > > > >
> > > > > > > > +             }
> > > > > > > > +     }
> > > > > > > > +     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.
> > > >
> > > > [Frank Li] After debug,  msi_domain_set_affinity() have not did null
> check
> > > for (parent->chip->irq_set_affinity).
> > > > I think impact by using dummy set_affinity is minimized.
> > > >
> > > > int msi_domain_set_affinity(struct irq_data *irq_data,
> > > >                           const struct cpumask *mask, bool force)
> > > > {
> > > >       struct irq_data *parent = irq_data->parent_data;
> > > >       struct msi_msg msg[2] = { [1] = { }, };
> > > >       int ret;
> > > >
> > > >       ret = parent->chip->irq_set_affinity(parent, mask, force);
> > > >       if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> > > >               BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
> > > >               msi_check_level(irq_data->domain, msg);
> > > >               irq_chip_write_msi_msg(irq_data, msg);
> > > >       }
> > > >
> > > >       return ret;
> > > > }
> > >
> > > No. Changing the affinity of an interrupt must not affect the affinity
> > > of another. Given that this is a chained handler, you *cannot* satisfy
> > > this requirement. So you can't change the affinity at all.
> > >
> >
> > [Frank Li] I understand affinity can't be changed.
> > But system use set affinity to write msi msg.
> >
> > The call stack as
> > [   25.508229]  epf_ntb_write_msi_msg+0x78/0x90
> > [   25.512512]  platform_msi_write_msg+0x2c/0x38
> > [   25.516882]  msi_domain_set_affinity+0xb0/0xc0
> > [   25.521330]  irq_do_set_affinity+0x174/0x220
> > [   25.525604]  irq_setup_affinity+0xe0/0x188
> > [   25.529713]  irq_startup+0x88/0x160
> > [   25.533214]  __setup_irq+0x6c8/0x768
> >
> > I have not found good place to hook a function to write msi msg.
> 
> It is called at MSI activation time (msi_domain_activate).

Another issue:   platform_msi_write_msg() is static function at platform-msi.c. 
It access a local structure struct platform_msi_priv_data.

If I use MSI_FLAG_USE_DEF_CHIP_OPS flags,  both msi_domain_set_affinity and msi_domain_set_affinity. 
will be set at chip. So it will NULL point error happen if I don't set affinity function.

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

Frank Li July 27, 2022, 6:58 p.m. UTC | #12

> -----Original Message-----
> From: Frank Li
> Sent: Wednesday, July 27, 2022 1:30 PM
> To: Marc Zyngier <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; dl-linux-imx <linux-
> [e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> Subject: RE: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> controller
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier <[e-mail]>
> > Sent: Wednesday, July 27, 2022 10:35 AM
> > To: Frank Li <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-arm-
> > [e-mail]; [e-mail]; Peng Fan
> > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > [e-mail]>; [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> > controller
> >
> > Caution: EXT Email
> >
> > On Wed, 27 Jul 2022 16:23:26 +0100,
> > Frank Li <[e-mail]> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marc Zyngier <[e-mail]>
> > > > Sent: Wednesday, July 27, 2022 3:03 AM
> > > > To: Frank Li <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; linux-arm-
> > > > [e-mail]; [e-mail]; Peng Fan
> > > > <[e-mail]>; Aisheng Dong <[e-mail]>;
> > > > [e-mail]; [e-mail]; dl-linux-imx <linux-
> > > > [e-mail]>; [e-mail]; [e-mail];
> > > > [e-mail]
> > > > Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> > > > controller
> > > >
> > > > Caution: EXT Email
> > > >
> > > > On Tue, 26 Jul 2022 22:48:32 +0100,
> > > > Frank Li <[e-mail]> wrote:
> > > > >
> > > > > > > > > +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.
> > > > >
> > > > > [Frank Li] Do you means that should be msi_data->msi_domain
> instead
> > > > > of msi_data->parent?
> > > >
> > > > Indeed. you must *not* bypass the hierarchy, and the top level of the
> > > > hierarchy has to implement whatever is required by the interrupt flow.
> > > >
> > >
> > > [Frank Li] I see, just want to confirm msi_data->msi_domain should
> > > be correct here?  It should be leaf of irq hierarchy tree.
> >
> > Yes.
> >
> > >
> > > > >
> > > > > > > >
> > > > > > > > > +             }
> > > > > > > > > +     }
> > > > > > > > > +     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.
> > > > >
> > > > > [Frank Li] After debug,  msi_domain_set_affinity() have not did null
> > check
> > > > for (parent->chip->irq_set_affinity).
> > > > > I think impact by using dummy set_affinity is minimized.
> > > > >
> > > > > int msi_domain_set_affinity(struct irq_data *irq_data,
> > > > >                           const struct cpumask *mask, bool force)
> > > > > {
> > > > >       struct irq_data *parent = irq_data->parent_data;
> > > > >       struct msi_msg msg[2] = { [1] = { }, };
> > > > >       int ret;
> > > > >
> > > > >       ret = parent->chip->irq_set_affinity(parent, mask, force);
> > > > >       if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
> > > > >               BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
> > > > >               msi_check_level(irq_data->domain, msg);
> > > > >               irq_chip_write_msi_msg(irq_data, msg);
> > > > >       }
> > > > >
> > > > >       return ret;
> > > > > }
> > > >
> > > > No. Changing the affinity of an interrupt must not affect the affinity
> > > > of another. Given that this is a chained handler, you *cannot* satisfy
> > > > this requirement. So you can't change the affinity at all.
> > > >
> > >
> > > [Frank Li] I understand affinity can't be changed.
> > > But system use set affinity to write msi msg.
> > >
> > > The call stack as
> > > [   25.508229]  epf_ntb_write_msi_msg+0x78/0x90
> > > [   25.512512]  platform_msi_write_msg+0x2c/0x38
> > > [   25.516882]  msi_domain_set_affinity+0xb0/0xc0
> > > [   25.521330]  irq_do_set_affinity+0x174/0x220
> > > [   25.525604]  irq_setup_affinity+0xe0/0x188
> > > [   25.529713]  irq_startup+0x88/0x160
> > > [   25.533214]  __setup_irq+0x6c8/0x768
> > >
> > > I have not found good place to hook a function to write msi msg.
> >
> > It is called at MSI activation time (msi_domain_activate).
> 
> Another issue:   platform_msi_write_msg() is static function at platform-msi.c.
> It access a local structure struct platform_msi_priv_data.
> 
> If I use MSI_FLAG_USE_DEF_CHIP_OPS flags,  both msi_domain_set_affinity
> and msi_domain_set_affinity.
> will be set at chip. So it will NULL point error happen if I don't set affinity
> function.
> 

[Frank Li] look like imx_mu_msi_irq_chip.irq_set_affinity = NULL; after call
platform_msi_create_irq_domain() can resolve this problem. 

But it looks hack method.  I think imx_mu_msi_irq_chip should keep unchanged
after call create platform_msi_create_irq_domain(). 

Do you have better solution or this way should be fine?	

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

Patch

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 <[e-mail]>
+ * Copyright 2022 NXP
+ *	Frank Li <[e-mail]>
+ *	Peng Fan <[e-mail]>
+ *
+ * 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);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
+}
+
+static void imx_mu_msi_unmask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data->parent_data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+	.name = "MU-MSI",
+	.irq_mask       = imx_mu_msi_mask_irq,
+	.irq_unmask     = imx_mu_msi_unmask_irq,
+};
+
+static struct msi_domain_ops its_pmsi_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
+		   MSI_FLAG_USE_DEF_CHIP_OPS |
+		   MSI_FLAG_PCI_MSIX),
+	.ops	= &its_pmsi_ops,
+	.chip	= &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
+	msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data->hwirq);
+	msg->data = data->hwirq;
+
+	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
+}
+
+static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+
+{
+	return IRQ_SET_MASK_OK;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+	.name			= "MU",
+	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,
+	.irq_set_affinity = imx_mu_msi_set_affinity,
+};
+
+static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs,
+					void *args)
+{
+	struct imx_mu_msi *msi_data = domain->host_data;
+	msi_alloc_info_t *info = args;
+	int pos, err = 0;
+
+	WARN_ON(nr_irqs != 1);
+
+	spin_lock(&msi_data->lock);
+	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);
+	if (err)
+		return err;
+
+	irq_domain_set_info(domain, virq, pos,
+			    &imx_mu_msi_parent_chip, msi_data,
+			    handle_simple_irq, NULL, NULL);
+	return 0;
+}
+
+static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
+
+	spin_lock(&msi_data->lock);
+	__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);
+		}
+	}
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
+{
+	/* Initialize MSI domain parent */
+	msi_data->parent = irq_domain_add_linear(dev_of_node(&msi_data->pdev->dev),
+						 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;
+	}
+
+	irq_domain_set_pm_device(msi_data->parent, &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);
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		goto disable_runtime_pm;
+	}
+
+	ret = pm_runtime_put_sync(dev);
+	if (ret < 0)
+		goto disable_runtime_pm;
+
+	clk_disable_unprepare(msi_data->clk);
+
+	return 0;
+
+disable_runtime_pm:
+	pm_runtime_disable(dev);
+	clk_disable_unprepare(msi_data->clk);
+
+	return ret;
+}
+
+static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
+{
+	struct imx_mu_msi *priv = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
+{
+	struct imx_mu_msi *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		dev_err(dev, "failed to enable clock\n");
+
+	return ret;
+}
+
+static const struct dev_pm_ops imx_mu_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
+			   imx_mu_runtime_resume, NULL)
+};
+
+static int __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 <[e-mail]>");
+MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
+MODULE_LICENSE("GPL");