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

Commit Message

Frank Li Aug. 12, 2022, 9:52 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 | 443 +++++++++++++++++++++++++++++++
 3 files changed, 451 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-mu-msi.c

Comments

Marc Zyngier Aug. 13, 2022, 9:19 a.m. UTC | #1

Frank,

The patch title needs work:

"irqchip: Add IMX MU MSI controller driver"

On Fri, 12 Aug 2022 22:52:40 +0100,
Frank Li <[e-mail]> wrote:
> 
> MU support generate irq by write data to a register.

"The MU block found in a number of Freescale/NXP SoCs supports
generating IRQs by writing data to a register."

> This patch make mu worked as msi controller.

Please see Documentation/process/submitting-patches.rst, and the
requirement to avoid wordings such as "This patch".

> So MU can do doorbell by using standard msi api.

"This enables the MU block to be used as a MSI controller, by
leveraging the platform-MSI API"

>
> Signed-off-by: Frank Li <[e-mail]>
> ---
>  drivers/irqchip/Kconfig          |   7 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-mu-msi.c | 443 +++++++++++++++++++++++++++++++
>  3 files changed, 451 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5e4e50122777d..4599471d880c0 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -470,6 +470,13 @@ config IMX_INTMUX
>  	help
>  	  Support for the i.MX INTMUX interrupt multiplexer.
>  
> +config IMX_MU_MSI
> +	bool "i.MX MU work as MSI controller"
> +	default y if ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  MU work as MSI controller to do general doorbell
> +
>  config LS1X_IRQ
>  	bool "Loongson-1 Interrupt Controller"
>  	depends on MACH_LOONGSON32
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 5d8e21d3dc6d8..870423746c783 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
>  obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
> +obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
>  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-msi.c
> new file mode 100644
> index 0000000000000..bb111412d598f
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP MU worked as MSI controller

Freescale? Or NXP? Please make up your mind.

> + *
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <[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(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +
> +struct imx_mu_dcfg {
> +	enum imx_mu_type type;
> +	u32     xTR;            /* Transmit Register0 */
> +	u32     xRR;            /* Receive Register0 */
> +	u32     xSR[4];         /* Status Registers */
> +	u32     xCR[4];         /* Control Registers */
> +};
> +
> +struct imx_mu_msi {
> +	spinlock_t			lock;
> +	struct platform_device		*pdev;

This pointer isn't useful. It is only used in
imx_mu_msi_domains_init(), which could take it as a parameter.

> +	struct irq_domain		*parent;

This pointer isn't useful. It is only used in the same function, and
could well be a local variable.

> +	struct irq_domain		*msi_domain;
> +	void __iomem			*regs;
> +	phys_addr_t			msiir_addr;
> +	const struct imx_mu_dcfg	*cfg;
> +	unsigned long			used;
> +	int				gic_irq;

This variable is only used in a single function.

> +	struct clk			*clk;
> +	struct device			*pd_a;
> +	struct device			*pd_b;
> +	struct device_link		*pd_link_a;
> +	struct device_link		*pd_link_b;

Same thing. All this pd_* stuff is *never* used outside of a single
function.

> +};
> +
> +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> +{
> +	iowrite32(val, msi_data->regs + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> +{
> +	return ioread32(msi_data->regs + offs);
> +}
> +
> +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&msi_data->lock, flags);

This needs to be a raw spinlock.

> +	val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> +	val &= ~clr;
> +	val |= set;
> +	imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> +	return val;
> +}
> +
> +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> +}
> +
> +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> +}
> +
> +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> +{
> +        struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +        imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> +}
> +
> +static struct irq_chip imx_mu_msi_irq_chip = {
> +	.name = "MU-MSI",
> +	.irq_ack = irq_chip_ack_parent,

Crucially, no irq_write_msi_msg callback. So we happily inherit
platform_msi_write_msg() and use the per descriptor write_msg()
callback. Who sets this? Nobody.

So I suspect you're hiding it somewhere else, and I really want to see
this code. I really don't see a good reason why it should be anywhere
else.

> +};
> +
> +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> +};
> +
> +static struct msi_domain_info imx_mu_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> +	.ops	= &imx_mu_msi_irq_ops,
> +	.chip	= &imx_mu_msi_irq_chip,
> +};
> +
> +static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +	u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> +
> +	msg->address_hi = upper_32_bits(addr);
> +	msg->address_lo = lower_32_bits(addr);
> +	msg->data = data->hwirq;
> +}
> +
> +static struct irq_chip imx_mu_msi_parent_chip = {
> +	.name		= "MU",
> +	.irq_mask	= imx_mu_msi_parent_mask_irq,
> +	.irq_unmask	= imx_mu_msi_parent_unmask_irq,
> +	.irq_ack	= imx_mu_msi_parent_ack_irq,
> +	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,

Please be consistent in the naming.

> +};
> +
> +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> +					unsigned int virq,
> +					unsigned int nr_irqs,
> +					void *args)
> +{
> +	struct imx_mu_msi *msi_data = domain->host_data;
> +	unsigned long flags;
> +	int pos, err = 0;
> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	spin_lock_irqsave(&msi_data->lock, flags);
> +	pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> +	if (pos < IMX_MU_CHANS)
> +		__set_bit(pos, &msi_data->used);
> +	else
> +		err = -ENOSPC;
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> +	if (err)
> +		return err;
> +
> +	irq_domain_set_info(domain, virq, pos,
> +			    &imx_mu_msi_parent_chip, msi_data,
> +			    handle_edge_irq, NULL, NULL);
> +	return 0;
> +}
> +
> +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&msi_data->lock, flags);
> +	__clear_bit(d->hwirq, &msi_data->used);
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +}
> +
> +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> +	.alloc	= imx_mu_msi_domain_irq_alloc,
> +	.free	= imx_mu_msi_domain_irq_free,
> +};
> +
> +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> +{
> +	struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> +	u32 status;
> +	int i;
> +
> +	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +	for (i = 0; i < IMX_MU_CHANS; i++) {
> +		if (status & IMX_MU_xSR_RFn(msi_data, i)) {
> +			generic_handle_domain_irq(msi_data->msi_domain, i);
> +		}
> +	}
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);

Do yourself a favour, and compute irq_desc_get_chip(desc) once, just
like for most irqchips.

> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> +{
> +	struct fwnode_handle *fwnodes = of_node_to_fwnode(dev_of_node(&msi_data->pdev->dev));

How about dev_fwnode()?

> +
> +	/* Initialize MSI domain parent */
> +	msi_data->parent = irq_domain_create_linear(fwnodes,
> +						    IMX_MU_CHANS,
> +						    &imx_mu_msi_domain_ops,
> +						    msi_data);

Consider setting the bus_token attribute for this domain to something
that isn't the default, as it otherwise clashes with the following
creation.

> +	if (!msi_data->parent) {
> +		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	msi_data->msi_domain = platform_msi_create_irq_domain(
> +				of_node_to_fwnode(msi_data->pdev->dev.of_node),

Why aren't you using the 'fwnodes' variable here?

> +				&imx_mu_msi_domain_info,
> +				msi_data->parent);
> +
> +	if (!msi_data->msi_domain) {
> +		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> +		irq_domain_remove(msi_data->parent);
> +		return -ENOMEM;
> +	}
> +
> +	/* clean irq_set_affinity again because it is chained irq */
> +	imx_mu_msi_irq_chip.irq_set_affinity = NULL;

NAK. The way to do this is to provide a callback that returns -EINVAL,
not to try and adjust things after the facts.

> +
> +	irq_domain_set_pm_device(msi_data->msi_domain, &msi_data->pdev->dev);
> +
> +	return 0;
> +}
> +
> +/* Register offset of different version MU IP */
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> +	.xTR    = 0x0,
> +	.xRR    = 0x10,
> +	.xSR    = {0x20, 0x20, 0x20, 0x20},
> +	.xCR    = {0x24, 0x24, 0x24, 0x24},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> +	.xTR    = 0x20,
> +	.xRR    = 0x40,
> +	.xSR    = {0x60, 0x60, 0x60, 0x60},
> +	.xCR    = {0x64, 0x64, 0x64, 0x64},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> +	.type   = IMX_MU_V2,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> +
> +	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static int __init imx_mu_of_init(struct device_node *dn,
> +				 struct device_node *parent,
> +				 const struct imx_mu_dcfg *cfg)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(dn);
> +	struct imx_mu_msi *msi_data, *priv;
> +	struct resource *res;
> +	struct device *dev;
> +	int ret;
> +
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	dev = &pdev->dev;
> +
> +	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> +	if (!msi_data)
> +		return -ENOMEM;
> +
> +	msi_data->cfg = cfg;
> +
> +	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "a");
> +	if (IS_ERR(msi_data->regs)) {
> +		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> +		return PTR_ERR(msi_data->regs);
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "b");
> +	if (!res)
> +		return -EIO;
> +
> +	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> +
> +	msi_data->pdev = pdev;
> +
> +	msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
> +	if (msi_data->gic_irq <= 0)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, msi_data);
> +
> +	msi_data->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(msi_data->clk)) {
> +		if (PTR_ERR(msi_data->clk) != -ENOENT)
> +			return PTR_ERR(msi_data->clk);
> +
> +		msi_data->clk = NULL;
> +	}
> +
> +	ret = clk_prepare_enable(msi_data->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");

I'm sorry, but you'll have to come up with something slightly more
descriptive than "a" or "b". At least add a qualifier to it. Same
thing for the DT by the way.

> +	if (IS_ERR(priv->pd_a))
> +		return PTR_ERR(priv->pd_a);
> +
> +	priv->pd_link_a = device_link_add(dev, priv->pd_a,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!priv->pd_link_a) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
> +	if (IS_ERR(priv->pd_b))
> +		return PTR_ERR(priv->pd_b);
> +
> +	priv->pd_link_b = device_link_add(dev, priv->pd_b,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!priv->pd_link_b) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = imx_mu_msi_domains_init(msi_data);
> +	if (ret)
> +		return ret;

How about the clocks, the links, and everything else that has been
allocated, enabled?

> +
> +	irq_set_chained_handler_and_data(msi_data->gic_irq,
> +					 imx_mu_msi_irq_handler,
> +					 msi_data);
> +
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
> +{
> +	struct imx_mu_msi *priv = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
> +{
> +	struct imx_mu_msi *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		dev_err(dev, "failed to enable clock\n");
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops imx_mu_pm_ops = {
> +	SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
> +			   imx_mu_runtime_resume, NULL)
> +};
> +
> +static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
> +					 struct device_node *parent)
> +{
> +	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
> +}
> +
> +static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
> +					struct device_node *parent)
> +{
> +	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
> +}
> +
> +static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
> +					 struct device_node *parent)
> +{
> +	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
> +}
> +
> +static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
> +					    struct device_node *parent)
> +{
> +	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
> +}
> +
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
> +IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
> +IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
> +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
> +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
> +IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm = &imx_mu_pm_ops)
> +
> +
> +MODULE_AUTHOR("Frank Li <[e-mail]>");
> +MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");

Please come up with a better description. Something like
"Freescale MU MSI controller driver"

> +MODULE_LICENSE("GPL");

Thanks,

	M.

Marc Zyngier Aug. 13, 2022, 11:57 a.m. UTC | #2

On Fri, 12 Aug 2022 22:52:40 +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]>

May I add that it wouldn't hurt if you checked what addresses you send
your patches to? For example, '[e-mail]' doesn't exist
(maybe you meant linux-kernel@...).

I will not take patches that haven't been posted to LKML (and I
shouldn't have reviewed it the first place).

Thanks,

	M.

Frank Li Aug. 14, 2022, 2:53 a.m. UTC | #3

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Saturday, August 13, 2022 6: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]; linux-
> [e-mail]; Peng Fan <[e-mail]>; Aisheng Dong
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Subject: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
> 
> Caution: EXT Email
> 
> On Fri, 12 Aug 2022 22:52:40 +0100,
> Frank Li <[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]>
> 
> May I add that it wouldn't hurt if you checked what addresses you send
> your patches to? For example, '[e-mail]' doesn't exist
> (maybe you meant linux-kernel@...).

[Frank Li] Thanks. Strang, My email system have not report error.  

> 
> I will not take patches that haven't been posted to LKML (and I
> shouldn't have reviewed it the first place).
> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Frank Li Aug. 14, 2022, 3:12 a.m. UTC | #4

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Saturday, August 13, 2022 4:19 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]; linux-arm-
> [e-mail]; [e-mail]; Peng Fan
> <[e-mail]>; Aisheng Dong <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; dl-linux-
> imx <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
> 
> Caution: EXT Email
> 
> Frank,
> 
> The patch title needs work:
> 
> "irqchip: Add IMX MU MSI controller driver"
> 
> On Fri, 12 Aug 2022 22:52:40 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > MU support generate irq by write data to a register.
> 
> "The MU block found in a number of Freescale/NXP SoCs supports
> generating IRQs by writing data to a register."
> 
> > This patch make mu worked as msi controller.
> 
> Please see Documentation/process/submitting-patches.rst, and the
> requirement to avoid wordings such as "This patch".
> 
> > So MU can do doorbell by using standard msi api.
> 
> "This enables the MU block to be used as a MSI controller, by
> leveraging the platform-MSI API"
> 
> >
> > Signed-off-by: Frank Li <[e-mail]>
> > ---
> >  drivers/irqchip/Kconfig          |   7 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-mu-msi.c | 443
> +++++++++++++++++++++++++++++++
> >  3 files changed, 451 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..4599471d880c0 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,13 @@ config IMX_INTMUX
> >       help
> >         Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > +     bool "i.MX MU work as MSI controller"
> > +     default y if ARCH_MXC
> > +     select IRQ_DOMAIN
> > +     help
> > +       MU work as MSI controller to do general doorbell
> > +
> >  config LS1X_IRQ
> >       bool "Loongson-1 Interrupt Controller"
> >       depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)            += irq-riscv-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)            += irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)           += irq-imx-irqsteer.o
> >  obj-$(CONFIG_IMX_INTMUX)             += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI)             += irq-imx-mu-msi.o
> >  obj-$(CONFIG_MADERA_IRQ)             += irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)                       += irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)    += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..bb111412d598f
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,443 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * NXP MU worked as MSI controller
> 
> Freescale? Or NXP? Please make up your mind.


[Frank Li] NXP and freescale is the same thing. 
It is mux used at many place. 
 
> 
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <[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(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > +     enum imx_mu_type type;
> > +     u32     xTR;            /* Transmit Register0 */
> > +     u32     xRR;            /* Receive Register0 */
> > +     u32     xSR[4];         /* Status Registers */
> > +     u32     xCR[4];         /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > +     spinlock_t                      lock;
> > +     struct platform_device          *pdev;
> 
> This pointer isn't useful. It is only used in
> imx_mu_msi_domains_init(), which could take it as a parameter.
> 
> > +     struct irq_domain               *parent;
> 
> This pointer isn't useful. It is only used in the same function, and
> could well be a local variable.
> 
> > +     struct irq_domain               *msi_domain;
> > +     void __iomem                    *regs;
> > +     phys_addr_t                     msiir_addr;
> > +     const struct imx_mu_dcfg        *cfg;
> > +     unsigned long                   used;
> > +     int                             gic_irq;
> 
> This variable is only used in a single function.
> 
> > +     struct clk                      *clk;
> > +     struct device                   *pd_a;
> > +     struct device                   *pd_b;
> > +     struct device_link              *pd_link_a;
> > +     struct device_link              *pd_link_b;
> 
> Same thing. All this pd_* stuff is *never* used outside of a single
> function.
> 
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > +     iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > +     return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> 
> This needs to be a raw spinlock.
> 
> > +     val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > +     val &= ~clr;
> > +     val |= set;
> > +     imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > +        struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +        imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > +     .name = "MU-MSI",
> > +     .irq_ack = irq_chip_ack_parent,
> 
> Crucially, no irq_write_msi_msg callback. So we happily inherit
> platform_msi_write_msg() and use the per descriptor write_msg()
> callback. Who sets this? Nobody.

[Frank Li] when set flag MSI_FLAG_USE_DEF_CHIP_OPS, 
 irq_write_msi_msg callback will be set at function platform_msi_update_chip_ops();

> 
> So I suspect you're hiding it somewhere else, and I really want to see
> this code. I really don't see a good reason why it should be anywhere
> else.
> 
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > +     .ops    = &imx_mu_msi_irq_ops,
> > +     .chip   = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_compose_msg(struct irq_data *data, struct
> msi_msg *msg)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +     u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > +     msg->address_hi = upper_32_bits(addr);
> > +     msg->address_lo = lower_32_bits(addr);
> > +     msg->data = data->hwirq;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > +     .name           = "MU",
> > +     .irq_mask       = imx_mu_msi_parent_mask_irq,
> > +     .irq_unmask     = imx_mu_msi_parent_unmask_irq,
> > +     .irq_ack        = imx_mu_msi_parent_ack_irq,
> > +     .irq_compose_msi_msg    = imx_mu_msi_compose_msg,
> 
> Please be consistent in the naming.
> 
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > +                                     unsigned int virq,
> > +                                     unsigned int nr_irqs,
> > +                                     void *args)
> > +{
> > +     struct imx_mu_msi *msi_data = domain->host_data;
> > +     unsigned long flags;
> > +     int pos, err = 0;
> > +
> > +     WARN_ON(nr_irqs != 1);
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > +     if (pos < IMX_MU_CHANS)
> > +             __set_bit(pos, &msi_data->used);
> > +     else
> > +             err = -ENOSPC;
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     irq_domain_set_info(domain, virq, pos,
> > +                         &imx_mu_msi_parent_chip, msi_data,
> > +                         handle_edge_irq, NULL, NULL);
> > +     return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > +                                    unsigned int virq, unsigned int nr_irqs)
> > +{
> > +     struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     __clear_bit(d->hwirq, &msi_data->used);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > +     .alloc  = imx_mu_msi_domain_irq_alloc,
> > +     .free   = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > +     u32 status;
> > +     int i;
> > +
> > +     status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > +     chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > +             if (status & IMX_MU_xSR_RFn(msi_data, i)) {
> > +                     generic_handle_domain_irq(msi_data->msi_domain, i);
> > +             }
> > +     }
> > +     chained_irq_exit(irq_desc_get_chip(desc), desc);
> 
> Do yourself a favour, and compute irq_desc_get_chip(desc) once, just
> like for most irqchips.
> 
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
> > +{
> > +     struct fwnode_handle *fwnodes =
> of_node_to_fwnode(dev_of_node(&msi_data->pdev->dev));
> 
> How about dev_fwnode()?
> 
> > +
> > +     /* Initialize MSI domain parent */
> > +     msi_data->parent = irq_domain_create_linear(fwnodes,
> > +                                                 IMX_MU_CHANS,
> > +                                                 &imx_mu_msi_domain_ops,
> > +                                                 msi_data);
> 
> Consider setting the bus_token attribute for this domain to something
> that isn't the default, as it otherwise clashes with the following
> creation.

[Frank Li] Any suggestion? Which bus_token is good? 

> 
> > +     if (!msi_data->parent) {
> > +             dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     msi_data->msi_domain = platform_msi_create_irq_domain(
> > +                             of_node_to_fwnode(msi_data->pdev->dev.of_node),
> 
> Why aren't you using the 'fwnodes' variable here?
> 
> > +                             &imx_mu_msi_domain_info,
> > +                             msi_data->parent);
> > +
> > +     if (!msi_data->msi_domain) {
> > +             dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> > +             irq_domain_remove(msi_data->parent);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     /* clean irq_set_affinity again because it is chained irq */
> > +     imx_mu_msi_irq_chip.irq_set_affinity = NULL;
> 
> NAK. The way to do this is to provide a callback that returns -EINVAL,
> not to try and adjust things after the facts.
> 
> > +
> > +     irq_domain_set_pm_device(msi_data->msi_domain, &msi_data-
> >pdev->dev);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > +     .xTR    = 0x0,
> > +     .xRR    = 0x10,
> > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +     .xTR    = 0x20,
> > +     .xRR    = 0x40,
> > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > +     .type   = IMX_MU_V2,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > +                              struct device_node *parent,
> > +                              const struct imx_mu_dcfg *cfg)
> > +{
> > +     struct platform_device *pdev = of_find_device_by_node(dn);
> > +     struct imx_mu_msi *msi_data, *priv;
> > +     struct resource *res;
> > +     struct device *dev;
> > +     int ret;
> > +
> > +     if (!pdev)
> > +             return -ENODEV;
> > +
> > +     dev = &pdev->dev;
> > +
> > +     priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> > +     if (!msi_data)
> > +             return -ENOMEM;
> > +
> > +     msi_data->cfg = cfg;
> > +
> > +     msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "a");
> > +     if (IS_ERR(msi_data->regs)) {
> > +             dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > +             return PTR_ERR(msi_data->regs);
> > +     }
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "b");
> > +     if (!res)
> > +             return -EIO;
> > +
> > +     msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > +     msi_data->pdev = pdev;
> > +
> > +     msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
> > +     if (msi_data->gic_irq <= 0)
> > +             return -ENODEV;
> > +
> > +     platform_set_drvdata(pdev, msi_data);
> > +
> > +     msi_data->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(msi_data->clk)) {
> > +             if (PTR_ERR(msi_data->clk) != -ENOENT)
> > +                     return PTR_ERR(msi_data->clk);
> > +
> > +             msi_data->clk = NULL;
> > +     }
> > +
> > +     ret = clk_prepare_enable(msi_data->clk);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to enable clock\n");
> > +             return ret;
> > +     }
> > +
> > +     priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> 
> I'm sorry, but you'll have to come up with something slightly more
> descriptive than "a" or "b". At least add a qualifier to it. Same
> thing for the DT by the way.

[Frank Li] MU spec using  term "A side" and "B side".  So I think "a" and "b"
is enough.  

Or do you think "a-side" is better?  

> 
> > +     if (IS_ERR(priv->pd_a))
> > +             return PTR_ERR(priv->pd_a);
> > +
> > +     priv->pd_link_a = device_link_add(dev, priv->pd_a,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!priv->pd_link_a) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
> > +     if (IS_ERR(priv->pd_b))
> > +             return PTR_ERR(priv->pd_b);
> > +
> > +     priv->pd_link_b = device_link_add(dev, priv->pd_b,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!priv->pd_link_b) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = imx_mu_msi_domains_init(msi_data);
> > +     if (ret)
> > +             return ret;
> 
> How about the clocks, the links, and everything else that has been
> allocated, enabled?
> 
> > +
> > +     irq_set_chained_handler_and_data(msi_data->gic_irq,
> > +                                      imx_mu_msi_irq_handler,
> > +                                      msi_data);
> > +
> > +     pm_runtime_enable(dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
> > +{
> > +     struct imx_mu_msi *priv = dev_get_drvdata(dev);
> > +
> > +     clk_disable_unprepare(priv->clk);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
> > +{
> > +     struct imx_mu_msi *priv = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(priv->clk);
> > +     if (ret)
> > +             dev_err(dev, "failed to enable clock\n");
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct dev_pm_ops imx_mu_pm_ops = {
> > +     SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
> > +                        imx_mu_runtime_resume, NULL)
> > +};
> > +
> > +static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
> > +                                      struct device_node *parent)
> > +{
> > +     return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
> > +}
> > +
> > +static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
> > +                                     struct device_node *parent)
> > +{
> > +     return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
> > +}
> > +
> > +static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
> > +                                      struct device_node *parent)
> > +{
> > +     return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
> > +}
> > +
> > +static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
> > +                                         struct device_node *parent)
> > +{
> > +     return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
> > +}
> > +
> > +IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
> > +IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
> > +IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
> > +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
> > +IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
> > +IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm =
> &imx_mu_pm_ops)
> > +
> > +
> > +MODULE_AUTHOR("Frank Li <[e-mail]>");
> > +MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
> 
> Please come up with a better description. Something like
> "Freescale MU MSI controller driver"
> 
> > +MODULE_LICENSE("GPL");
> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier Aug. 15, 2022, 8:58 a.m. UTC | #5

On Sun, 14 Aug 2022 04:12:01 +0100,
Frank Li <[e-mail]> wrote:

> > > new file mode 100644
> > > index 0000000000000..bb111412d598f
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > > @@ -0,0 +1,443 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * NXP MU worked as MSI controller
> > 
> > Freescale? Or NXP? Please make up your mind.
> 
> 
> [Frank Li] NXP and freescale is the same thing. 
> It is mux used at many place.

Pick one, and stick to it. Having two names for the same thing is
pointlessly confusing.

> > > +static struct irq_chip imx_mu_msi_irq_chip = {
> > > +     .name = "MU-MSI",
> > > +     .irq_ack = irq_chip_ack_parent,
> > 
> > Crucially, no irq_write_msi_msg callback. So we happily inherit
> > platform_msi_write_msg() and use the per descriptor write_msg()
> > callback. Who sets this? Nobody.
> 
> [Frank Li] when set flag MSI_FLAG_USE_DEF_CHIP_OPS, 
>  irq_write_msi_msg callback will be set at function platform_msi_update_chip_ops();

That wasn't my question. But never mind, I found the call to
platform_msi_domain_alloc_irqs() in patch #4.

> > > +
> > > +     /* Initialize MSI domain parent */
> > > +     msi_data->parent = irq_domain_create_linear(fwnodes,
> > > +                                                 IMX_MU_CHANS,
> > > +                                                 &imx_mu_msi_domain_ops,
> > > +                                                 msi_data);
> > 
> > Consider setting the bus_token attribute for this domain to something
> > that isn't the default, as it otherwise clashes with the following
> > creation.
> 
> [Frank Li] Any suggestion? Which bus_token is good?

DOMAIN_BUS_NEXUS is what other drivers use.

> > > +     priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> > 
> > I'm sorry, but you'll have to come up with something slightly more
> > descriptive than "a" or "b". At least add a qualifier to it. Same
> > thing for the DT by the way.
> 
> [Frank Li] MU spec using  term "A side" and "B side".  So I think "a" and "b"
> is enough.

No, it really isn't.

> 
> Or do you think "a-side" is better?

No, I would like something fully descriptive. The DT actually has
"Processor A-facing", which seems like a reasonable description.

	M.

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..bb111412d598f
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,443 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NXP MU worked as MSI controller
+ *
+ * Copyright (c) 2018 Pengutronix, Oleksij Rempel <[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(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+
+struct imx_mu_dcfg {
+	enum imx_mu_type type;
+	u32     xTR;            /* Transmit Register0 */
+	u32     xRR;            /* Receive Register0 */
+	u32     xSR[4];         /* Status Registers */
+	u32     xCR[4];         /* Control Registers */
+};
+
+struct imx_mu_msi {
+	spinlock_t			lock;
+	struct platform_device		*pdev;
+	struct irq_domain		*parent;
+	struct irq_domain		*msi_domain;
+	void __iomem			*regs;
+	phys_addr_t			msiir_addr;
+	const struct imx_mu_dcfg	*cfg;
+	unsigned long			used;
+	int				gic_irq;
+	struct clk			*clk;
+	struct device			*pd_a;
+	struct device			*pd_b;
+	struct device_link		*pd_link_a;
+	struct device_link		*pd_link_b;
+};
+
+static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
+{
+	iowrite32(val, msi_data->regs + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
+{
+	return ioread32(msi_data->regs + offs);
+}
+
+static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&msi_data->lock, flags);
+	val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	return val;
+}
+
+static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
+}
+
+static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
+}
+
+static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
+{
+        struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+        imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+	.name = "MU-MSI",
+	.irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_ops imx_mu_msi_irq_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.ops	= &imx_mu_msi_irq_ops,
+	.chip	= &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+	u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
+
+	msg->address_hi = upper_32_bits(addr);
+	msg->address_lo = lower_32_bits(addr);
+	msg->data = data->hwirq;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+	.name		= "MU",
+	.irq_mask	= imx_mu_msi_parent_mask_irq,
+	.irq_unmask	= imx_mu_msi_parent_unmask_irq,
+	.irq_ack	= imx_mu_msi_parent_ack_irq,
+	.irq_compose_msi_msg	= imx_mu_msi_compose_msg,
+};
+
+static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs,
+					void *args)
+{
+	struct imx_mu_msi *msi_data = domain->host_data;
+	unsigned long flags;
+	int pos, err = 0;
+
+	WARN_ON(nr_irqs != 1);
+
+	spin_lock_irqsave(&msi_data->lock, flags);
+	pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
+	if (pos < IMX_MU_CHANS)
+		__set_bit(pos, &msi_data->used);
+	else
+		err = -ENOSPC;
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	if (err)
+		return err;
+
+	irq_domain_set_info(domain, virq, pos,
+			    &imx_mu_msi_parent_chip, msi_data,
+			    handle_edge_irq, NULL, NULL);
+	return 0;
+}
+
+static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&msi_data->lock, flags);
+	__clear_bit(d->hwirq, &msi_data->used);
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+}
+
+static const struct irq_domain_ops imx_mu_msi_domain_ops = {
+	.alloc	= imx_mu_msi_domain_irq_alloc,
+	.free	= imx_mu_msi_domain_irq_free,
+};
+
+static void imx_mu_msi_irq_handler(struct irq_desc *desc)
+{
+	struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
+	u32 status;
+	int i;
+
+	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		if (status & IMX_MU_xSR_RFn(msi_data, i)) {
+			generic_handle_domain_irq(msi_data->msi_domain, i);
+		}
+	}
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data)
+{
+	struct fwnode_handle *fwnodes = of_node_to_fwnode(dev_of_node(&msi_data->pdev->dev));
+
+	/* Initialize MSI domain parent */
+	msi_data->parent = irq_domain_create_linear(fwnodes,
+						    IMX_MU_CHANS,
+						    &imx_mu_msi_domain_ops,
+						    msi_data);
+	if (!msi_data->parent) {
+		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_data->msi_domain = platform_msi_create_irq_domain(
+				of_node_to_fwnode(msi_data->pdev->dev.of_node),
+				&imx_mu_msi_domain_info,
+				msi_data->parent);
+
+	if (!msi_data->msi_domain) {
+		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
+		irq_domain_remove(msi_data->parent);
+		return -ENOMEM;
+	}
+
+	/* clean irq_set_affinity again because it is chained irq */
+	imx_mu_msi_irq_chip.irq_set_affinity = NULL;
+
+	irq_domain_set_pm_device(msi_data->msi_domain, &msi_data->pdev->dev);
+
+	return 0;
+}
+
+/* Register offset of different version MU IP */
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.xTR    = 0x0,
+	.xRR    = 0x10,
+	.xSR    = {0x20, 0x20, 0x20, 0x20},
+	.xCR    = {0x24, 0x24, 0x24, 0x24},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+	.xTR    = 0x20,
+	.xRR    = 0x40,
+	.xSR    = {0x60, 0x60, 0x60, 0x60},
+	.xCR    = {0x64, 0x64, 0x64, 0x64},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
+	.type   = IMX_MU_V2,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {0xC, 0x118, 0x124, 0x12C},
+	.xCR    = {0x110, 0x114, 0x120, 0x128},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
+
+	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
+	.xTR    = 0x200,
+	.xRR    = 0x280,
+	.xSR    = {0xC, 0x118, 0x124, 0x12C},
+	.xCR    = {0x110, 0x114, 0x120, 0x128},
+};
+
+static int __init imx_mu_of_init(struct device_node *dn,
+				 struct device_node *parent,
+				 const struct imx_mu_dcfg *cfg)
+{
+	struct platform_device *pdev = of_find_device_by_node(dn);
+	struct imx_mu_msi *msi_data, *priv;
+	struct resource *res;
+	struct device *dev;
+	int ret;
+
+	if (!pdev)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+
+	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
+
+	msi_data->cfg = cfg;
+
+	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "a");
+	if (IS_ERR(msi_data->regs)) {
+		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
+		return PTR_ERR(msi_data->regs);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "b");
+	if (!res)
+		return -EIO;
+
+	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
+
+	msi_data->pdev = pdev;
+
+	msi_data->gic_irq = platform_get_irq(msi_data->pdev, 0);
+	if (msi_data->gic_irq <= 0)
+		return -ENODEV;
+
+	platform_set_drvdata(pdev, msi_data);
+
+	msi_data->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(msi_data->clk)) {
+		if (PTR_ERR(msi_data->clk) != -ENOENT)
+			return PTR_ERR(msi_data->clk);
+
+		msi_data->clk = NULL;
+	}
+
+	ret = clk_prepare_enable(msi_data->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
+	if (IS_ERR(priv->pd_a))
+		return PTR_ERR(priv->pd_a);
+
+	priv->pd_link_a = device_link_add(dev, priv->pd_a,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!priv->pd_link_a) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		return -EINVAL;
+	}
+
+	priv->pd_b = dev_pm_domain_attach_by_name(dev, "b");
+	if (IS_ERR(priv->pd_b))
+		return PTR_ERR(priv->pd_b);
+
+	priv->pd_link_b = device_link_add(dev, priv->pd_b,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!priv->pd_link_b) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		return -EINVAL;
+	}
+
+	ret = imx_mu_msi_domains_init(msi_data);
+	if (ret)
+		return ret;
+
+	irq_set_chained_handler_and_data(msi_data->gic_irq,
+					 imx_mu_msi_irq_handler,
+					 msi_data);
+
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
+{
+	struct imx_mu_msi *priv = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
+{
+	struct imx_mu_msi *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		dev_err(dev, "failed to enable clock\n");
+
+	return ret;
+}
+
+static const struct dev_pm_ops imx_mu_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
+			   imx_mu_runtime_resume, NULL)
+};
+
+static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
+					 struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
+}
+
+static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
+					struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
+}
+
+static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
+					 struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
+}
+
+static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
+					    struct device_node *parent)
+{
+	return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
+IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
+IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm = &imx_mu_pm_ops)
+
+
+MODULE_AUTHOR("Frank Li <[e-mail]>");
+MODULE_DESCRIPTION("Freescale MU work as MSI controller driver");
+MODULE_LICENSE("GPL");