Message-ID: <87r12wkmkm.wl-maz@kernel.org>
Date:   Fri, 08 Jul 2022 09:58:33 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Frank Li <Frank.Li@....com>
Cc:     tglx@...utronix.de, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, kw@...ux.com, bhelgaas@...gle.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
        peng.fan@....com, aisheng.dong@....com, jdmason@...zu.us,
        kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
        kishon@...com, lorenzo.pieralisi@....com, ntb@...ts.linux.dev
Subject: Re: [PATCH 1/3] irqchip: imx mu worked as msi controller

On Thu, 07 Jul 2022 22:02:36 +0100,
Frank Li <Frank.Li@....com> wrote:
> 
> MU support generate irq by write data to a register.
> This patch make mu worked as msi controller.
> So MU can do doorbell by using standard msi api.
> 
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
>  drivers/irqchip/Kconfig          |   7 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-mu-msi.c | 490 +++++++++++++++++++++++++++++++
>  3 files changed, 498 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..f7193a6c1245e
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP MU worked as MSI controller
> + *
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@...gutronix.de>
> + * Copyright 2022 NXP
> + *	Frank Li <Frank.Li@....com>
> + *	Peng Fan <peng.fan@....com>
> + *
> + * Based on drivers/mailbox/imx-mailbox.c
> + */
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/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_chan_type {
> +	IMX_MU_TYPE_TX,         /* Tx */
> +	IMX_MU_TYPE_RX,         /* Rx */
> +	IMX_MU_TYPE_TXDB,       /* Tx doorbell */
> +	IMX_MU_TYPE_RXDB,       /* Rx doorbell */

What does any of this even mean for MSIs?

> +};
> +
> +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,
> +	IMX_MU_V2,
> +	IMX_MU_V2_S4 = BIT(15),

If the bit assignment is significant, make it so for all members of
this enum.

> +};
> +
> +/* 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;
> +	struct imx_mu_dcfg	*cfg;
> +	u32			msir_num;
> +	struct imx_mu_msir	*msir;
> +	u32			irqs_num;
> +	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);
> +
> +	pci_msi_mask_irq(data);

What is this? Below, you create a platform MSI domain. Either you
support PCI, and you create a PCI/MSI domain (and the above may make
sense), or you are doing platform MSI, and the above is non-sense.

> +	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);
> +
> +	pci_msi_unmask_irq(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;
> +
> +	pm_runtime_get_sync(&msi_data->pdev->dev);

The core code already deals with runtime PM. What prevents it from
working, other than the fact you don't populate the device in the
top-level domain?

> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	spin_lock(&msi_data->lock);
> +	pos = find_first_zero_bit(&msi_data->used, msi_data->irqs_num);
> +	if (pos < msi_data->irqs_num)
> +		__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);
> +	int pos;
> +
> +	pos = d->hwirq;
> +	if (pos < 0 || pos >= msi_data->irqs_num) {
> +		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> +		return;
> +	}

How can this happen?

> +
> +	spin_lock(&msi_data->lock);
> +	__clear_bit(pos, &msi_data->used);
> +	spin_unlock(&msi_data->lock);
> +
> +	pm_runtime_put(&msi_data->pdev->dev);
> +}
> +
> +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(NULL,

NAK. Don't create anonymous domains.

> +						 msi_data->irqs_num,
> +						 &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;
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_mu_msi_teardown_hwirq(struct imx_mu_msi *msi_data)
> +{
> +	if (msi_data->gic_irq > 0)
> +		irq_set_chained_handler_and_data(msi_data->gic_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +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},
> +};

What are these? We really don't need more magic numbers.

> +
> +static const struct of_device_id imx_mu_msi_ids[] = {
> +	{ .compatible = "fsl,imx7ulp-mu-msi", .data = &imx_mu_cfg_imx7ulp },
> +	{ .compatible = "fsl,imx6sx-mu-msi", .data = &imx_mu_cfg_imx6sx },
> +	{ .compatible = "fsl,imx8ulp-mu-msi", .data = &imx_mu_cfg_imx8ulp },
> +	{ .compatible = "fsl,imx8ulp-mu-msi-s4", .data = &imx_mu_cfg_imx8ulp_s4 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, imx_mu_msi_ids);
> +
> +static int imx_mu_msi_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct imx_mu_msi *msi_data, *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +
> +	match = of_match_device(imx_mu_msi_ids, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	priv = msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> +	if (!msi_data)
> +		return -ENOMEM;
> +
> +	msi_data->cfg = (struct imx_mu_dcfg *) match->data;
> +
> +	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->irqs_num = IMX_MU_CHANS;

If that's hardcoded, why do we need an extra variable? I also question
the usefulness of this driver if the HW can only deal with *4* MSIs...
This looks a bit like a joke.

> +
> +	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 imx_mu_msi_remove(struct platform_device *pdev)
> +{
> +	struct imx_mu_msi *msi_data = platform_get_drvdata(pdev);
> +
> +	imx_mu_msi_teardown_hwirq(msi_data);
> +
> +	irq_domain_remove(msi_data->msi_domain);
> +	irq_domain_remove(msi_data->parent);

How do you ensure that no device is still holding interrupts? Let me
give you a hint: you can't. So removing an interrupt controller module
should not be possible.

> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx_mu_msi_driver = {
> +	.driver = {
> +		.name = "imx-mu-msi",
> +		.of_match_table = imx_mu_msi_ids,
> +		.pm = &imx_mu_pm_ops,
> +	},
> +	.probe = imx_mu_msi_probe,
> +	.remove = imx_mu_msi_remove,
> +};
> +
> +module_platform_driver(imx_mu_msi_driver);

Please use the standard probing methods (IRQCHIP_PLATFORM_DRIVER_BEGIN
and co).

> +
> +MODULE_AUTHOR("Frank Li <Frank.Li@....com>");
> +MODULE_DESCRIPTION("Freescale Layerscape SCFG MSI controller driver");
> +MODULE_LICENSE("GPL");

I have the ugly feeling that this driver really isn't about MSIs, but
is just a way to sneak some terrible abstraction into the kernel... I
guess we'll eventually find out. In the meantime, this driver needs
fixing.

	M.

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

Powered by blists - more mailing lists