[v9,2/4] irqchip: Add IMX MU MSI controller driver

Commit Message

Frank Li Sept. 7, 2022, 3:48 a.m. UTC

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

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

Signed-off-by: Frank Li <[e-mail]>
---
 drivers/irqchip/Kconfig          |   9 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-imx-mu-msi.c | 451 +++++++++++++++++++++++++++++++
 3 files changed, 461 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-mu-msi.c

Comments

kernel test robot Sept. 8, 2022, 12:03 a.m. UTC | #1

Hi Frank,

I love your patch! Yet something to improve:

[auto build test ERROR on jonmason-ntb/ntb-next]
[also build test ERROR on robh/for-next linus/master v6.0-rc4 next-20220907]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
base:   https://github.com/jonmason/ntb ntb-next
config: s390-randconfig-s033-20220907 (https://download.01.org/0day-ci/archive/20220908/[e-mail]/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
        git checkout c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function `imx_mu_of_init':
>> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to `devm_platform_ioremap_resource_byname'


vim +316 drivers/irqchip/irq-imx-mu-msi.c

   288	
   289	static int __init imx_mu_of_init(struct device_node *dn,
   290					 struct device_node *parent,
   291					 const struct imx_mu_dcfg *cfg
   292					)
   293	{
   294		struct platform_device *pdev = of_find_device_by_node(dn);
   295		struct device_link *pd_link_a;
   296		struct device_link *pd_link_b;
   297		struct imx_mu_msi *msi_data;
   298		struct resource *res;
   299		struct device *pd_a;
   300		struct device *pd_b;
   301		struct device *dev;
   302		int ret;
   303		int irq;
   304	
   305		if (!pdev)
   306			return -ENODEV;
   307	
   308		dev = &pdev->dev;
   309	
   310		msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
   311		if (!msi_data)
   312			return -ENOMEM;
   313	
   314		msi_data->cfg = cfg;
   315	
 > 316		msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
   317		if (IS_ERR(msi_data->regs)) {
   318			dev_err(&pdev->dev, "failed to initialize 'regs'\n");
   319			return PTR_ERR(msi_data->regs);
   320		}
   321	
   322		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
   323		if (!res)
   324			return -EIO;
   325	
   326		msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
   327	
   328		irq = platform_get_irq(pdev, 0);
   329		if (irq <= 0)
   330			return -ENODEV;
   331	
   332		platform_set_drvdata(pdev, msi_data);
   333	
   334		msi_data->clk = devm_clk_get(dev, NULL);
   335		if (IS_ERR(msi_data->clk)) {
   336			if (PTR_ERR(msi_data->clk) != -ENOENT)
   337				return PTR_ERR(msi_data->clk);
   338	
   339			msi_data->clk = NULL;
   340		}
   341	
   342		pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
   343		if (IS_ERR(pd_a))
   344			return PTR_ERR(pd_a);
   345	
   346		pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
   347		if (IS_ERR(pd_b))
   348			return PTR_ERR(pd_b);
   349	
   350		pd_link_a = device_link_add(dev, pd_a,
   351				DL_FLAG_STATELESS |
   352				DL_FLAG_PM_RUNTIME |
   353				DL_FLAG_RPM_ACTIVE);
   354	
   355		if (!pd_link_a) {
   356			dev_err(dev, "Failed to add device_link to mu a.\n");
   357			goto err_pd_a;
   358		}
   359	
   360		pd_link_b = device_link_add(dev, pd_b,
   361				DL_FLAG_STATELESS |
   362				DL_FLAG_PM_RUNTIME |
   363				DL_FLAG_RPM_ACTIVE);
   364	
   365	
   366		if (!pd_link_b) {
   367			dev_err(dev, "Failed to add device_link to mu a.\n");
   368			goto err_pd_b;
   369		}
   370	
   371		ret = imx_mu_msi_domains_init(msi_data, dev);
   372		if (ret)
   373			goto err_dm_init;
   374	
   375		irq_set_chained_handler_and_data(irq,
   376						 imx_mu_msi_irq_handler,
   377						 msi_data);
   378	
   379		pm_runtime_enable(dev);
   380	
   381		return 0;
   382	
   383	err_dm_init:
   384		device_link_remove(dev,	pd_b);
   385	err_pd_b:
   386		device_link_remove(dev, pd_a);
   387	err_pd_a:
   388		return -EINVAL;
   389	}
   390

Marc Zyngier Sept. 8, 2022, 7:02 a.m. UTC | #2

Frank,

On Thu, 08 Sep 2022 01:03:27 +0100,
kernel test robot <[e-mail]> wrote:
> 
> Hi Frank,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on jonmason-ntb/ntb-next]
> [also build test ERROR on robh/for-next linus/master v6.0-rc4 next-20220907]
> [cannot apply to tip/irq/core]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
> base:   https://github.com/jonmason/ntb ntb-next
> config: s390-randconfig-s033-20220907 (https://download.01.org/0day-ci/archive/20220908/[e-mail]/config)
> compiler: s390-linux-gcc (GCC) 12.1.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # apt-get install sparse
>         # sparse version: v0.6.4-39-gce1a6720-dirty
>         # https://github.com/intel-lab-lkp/linux/commit/c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
>         git checkout c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=s390 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[e-mail]>
> 
> All errors (new ones prefixed by >>):
> 
>    s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function `imx_mu_of_init':
> >> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to `devm_platform_ioremap_resource_byname'

This is about the 4th time this breakage gets reported. You keep
reposting this series without addressing it. What is it going to take
for you to finally fix it? Clearly, I'm not going to bother taking a
series that has pending build breakages.

	M.

Marc Zyngier Sept. 8, 2022, 7:39 a.m. UTC | #3

On Wed, 07 Sep 2022 04:48:54 +0100,
Frank Li <[e-mail]> wrote:
> 
> The MU block found in a number of Freescale/NXP SoCs supports generating
> IRQs by writing data to a register
> 
> This enables the MU block to be used as a MSI controller, by leveraging
> the platform-MSI API

Missing full stop after each sentence.

> 
> Signed-off-by: Frank Li <[e-mail]>
> ---
>  drivers/irqchip/Kconfig          |   9 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-imx-mu-msi.c | 451 +++++++++++++++++++++++++++++++
>  3 files changed, 461 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5e4e50122777d..e04c6521dce55 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -470,6 +470,15 @@ config IMX_INTMUX
>  	help
>  	  Support for the i.MX INTMUX interrupt multiplexer.
>  
> +config IMX_MU_MSI
> +	bool "i.MX MU work as MSI controller"

Why bool? Doesn't it also work as a module?

> +	default y if ARCH_MXC

Why would this be selected by default?

> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	select GENERIC_MSI_IRQ_DOMAIN
> +	help
> +	  MU work as MSI controller to do general doorbell

I'm not sure this is that generic. It really is limited to CPU-to-CPU
interrupts.

> +
>  config LS1X_IRQ
>  	bool "Loongson-1 Interrupt Controller"
>  	depends on MACH_LOONGSON32
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 5d8e21d3dc6d8..870423746c783 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
>  obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
> +obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
>  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-msi.c
> new file mode 100644
> index 0000000000000..82b55f6d87266
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,451 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Freescale MU worked as MSI controller

s/worked/used/

> + *
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <[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>

Keep this list in alphabetical order.

> +
> +
> +#define IMX_MU_CHANS            4
> +
> +enum imx_mu_xcr {
> +	IMX_MU_GIER,
> +	IMX_MU_GCR,
> +	IMX_MU_TCR,
> +	IMX_MU_RCR,
> +	IMX_MU_xCR_MAX,

What is this last enum used for?

> +};
> +
> +enum imx_mu_xsr {
> +	IMX_MU_SR,
> +	IMX_MU_GSR,
> +	IMX_MU_TSR,
> +	IMX_MU_RSR,
> +};
> +
> +enum imx_mu_type {
> +	IMX_MU_V1 = BIT(0),

This is never used. Why?

> +	IMX_MU_V2 = BIT(1),
> +	IMX_MU_V2_S4 = BIT(15),

Same thing.

> +};
> +
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +
> +struct imx_mu_dcfg {
> +	enum imx_mu_type type;
> +	u32     xTR;            /* Transmit Register0 */
> +	u32     xRR;            /* Receive Register0 */
> +	u32     xSR[4];         /* Status Registers */
> +	u32     xCR[4];         /* Control Registers */
> +};
> +
> +struct imx_mu_msi {
> +	spinlock_t			lock;
> +	raw_spinlock_t			reglock;

Why two locks? Isn't one enough to protect both MSI allocation (which
happens once in a blue moon) and register access?

Also, where are these locks initialised?

> +	struct irq_domain		*msi_domain;
> +	void __iomem			*regs;
> +	phys_addr_t			msiir_addr;
> +	const struct imx_mu_dcfg	*cfg;
> +	unsigned long			used;
> +	struct clk			*clk;
> +};
> +
> +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> +{
> +	iowrite32(val, msi_data->regs + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> +{
> +	return ioread32(msi_data->regs + offs);
> +}
> +
> +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&msi_data->reglock, flags);
> +	val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> +	val &= ~clr;
> +	val |= set;
> +	imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> +	raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> +
> +	return val;
> +}
> +
> +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> +}
> +
> +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> +}
> +
> +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> +	imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> +}
> +
> +static struct irq_chip imx_mu_msi_irq_chip = {
> +	.name = "MU-MSI",
> +	.irq_ack = irq_chip_ack_parent,
> +};
> +
> +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> +};
> +
> +static struct msi_domain_info imx_mu_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> +	.ops	= &imx_mu_msi_irq_ops,
> +	.chip	= &imx_mu_msi_irq_chip,
> +};
> +
> +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> +					  struct msi_msg *msg)
> +{
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +	u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> +
> +	msg->address_hi = upper_32_bits(addr);
> +	msg->address_lo = lower_32_bits(addr);
> +	msg->data = data->hwirq;
> +}
> +
> +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> +				   const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static struct irq_chip imx_mu_msi_parent_chip = {
> +	.name		= "MU",
> +	.irq_mask	= imx_mu_msi_parent_mask_irq,
> +	.irq_unmask	= imx_mu_msi_parent_unmask_irq,
> +	.irq_ack	= imx_mu_msi_parent_ack_irq,
> +	.irq_compose_msi_msg	= imx_mu_msi_parent_compose_msg,
> +	.irq_set_affinity = imx_mu_msi_parent_set_affinity,
> +};
> +
> +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> +					unsigned int virq,
> +					unsigned int nr_irqs,
> +					void *args)
> +{
> +	struct imx_mu_msi *msi_data = domain->host_data;
> +	unsigned long flags;
> +	int pos, err = 0;
> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	spin_lock_irqsave(&msi_data->lock, flags);
> +	pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> +	if (pos < IMX_MU_CHANS)
> +		__set_bit(pos, &msi_data->used);
> +	else
> +		err = -ENOSPC;
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> +	if (err)
> +		return err;
> +
> +	irq_domain_set_info(domain, virq, pos,
> +			    &imx_mu_msi_parent_chip, msi_data,
> +			    handle_edge_irq, NULL, NULL);
> +	return 0;
> +}
> +
> +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> +	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&msi_data->lock, flags);
> +	__clear_bit(d->hwirq, &msi_data->used);
> +	spin_unlock_irqrestore(&msi_data->lock, flags);
> +}
> +
> +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> +	.alloc	= imx_mu_msi_domain_irq_alloc,
> +	.free	= imx_mu_msi_domain_irq_free,
> +};
> +
> +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> +{
> +	struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	u32 status;
> +	int i;
> +
> +	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> +
> +	chained_irq_enter(chip, desc);
> +	for (i = 0; i < IMX_MU_CHANS; i++) {
> +		if (status & IMX_MU_xSR_RFn(msi_data, i))
> +			generic_handle_domain_irq(msi_data->msi_domain, i);
> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct device *dev)
> +{
> +	struct fwnode_handle *fwnodes = dev_fwnode(dev);
> +	struct irq_domain *parent;
> +
> +	/* Initialize MSI domain parent */
> +	parent = irq_domain_create_linear(fwnodes,
> +					    IMX_MU_CHANS,
> +					    &imx_mu_msi_domain_ops,
> +					    msi_data);
> +	if (!parent) {
> +		dev_err(dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> +
> +	msi_data->msi_domain = platform_msi_create_irq_domain(
> +				fwnodes,
> +				&imx_mu_msi_domain_info,
> +				parent);

nit: move the first argument after the opening bracket (longer lines
are fine).

> +
> +	if (!msi_data->msi_domain) {
> +		dev_err(dev, "failed to create MSI domain\n");
> +		irq_domain_remove(parent);
> +		return -ENOMEM;
> +	}
> +
> +	irq_domain_set_pm_device(msi_data->msi_domain, dev);
> +
> +	return 0;
> +}
> +
> +/* Register offset of different version MU IP */
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {

Why doesn't this have a type?

> +	.xTR    = 0x0,
> +	.xRR    = 0x10,
> +	.xSR    = {0x20, 0x20, 0x20, 0x20},

Since you defined enums for all the register offsets, please be
consistent and use them everywhere:

	.xSR = {
		[IMX_MU_SR]	= 0x20,
		[IMX_MU_GSR]	= 0x20,
		[...]
	},

> +	.xCR    = {0x24, 0x24, 0x24, 0x24},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> +	.xTR    = 0x20,
> +	.xRR    = 0x40,
> +	.xSR    = {0x60, 0x60, 0x60, 0x60},
> +	.xCR    = {0x64, 0x64, 0x64, 0x64},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> +	.type   = IMX_MU_V2,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> +
> +	.type   = IMX_MU_V2 | IMX_MU_V2_S4,
> +	.xTR    = 0x200,
> +	.xRR    = 0x280,
> +	.xSR    = {0xC, 0x118, 0x124, 0x12C},
> +	.xCR    = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static int __init imx_mu_of_init(struct device_node *dn,
> +				 struct device_node *parent,
> +				 const struct imx_mu_dcfg *cfg
> +				)

Move closing bracket after 'cfg'.

> +{
> +	struct platform_device *pdev = of_find_device_by_node(dn);
> +	struct device_link *pd_link_a;
> +	struct device_link *pd_link_b;
> +	struct imx_mu_msi *msi_data;
> +	struct resource *res;
> +	struct device *pd_a;
> +	struct device *pd_b;
> +	struct device *dev;
> +	int ret;
> +	int irq;
> +
> +	if (!pdev)
> +		return -ENODEV;

How can that happen?

> +
> +	dev = &pdev->dev;
> +
> +	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> +	if (!msi_data)
> +		return -ENOMEM;
> +
> +	msi_data->cfg = cfg;
> +
> +	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
> +	if (IS_ERR(msi_data->regs)) {
> +		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> +		return PTR_ERR(msi_data->regs);
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
> +	if (!res)
> +		return -EIO;
> +
> +	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, msi_data);
> +
> +	msi_data->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(msi_data->clk)) {
> +		if (PTR_ERR(msi_data->clk) != -ENOENT)
> +			return PTR_ERR(msi_data->clk);
> +
> +		msi_data->clk = NULL;

Why is it acceptable to continue with no clock?

> +	}
> +
> +	pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> +	if (IS_ERR(pd_a))
> +		return PTR_ERR(pd_a);
> +
> +	pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> +	if (IS_ERR(pd_b))
> +		return PTR_ERR(pd_b);
> +
> +	pd_link_a = device_link_add(dev, pd_a,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +	if (!pd_link_a) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		goto err_pd_a;
> +	}
> +
> +	pd_link_b = device_link_add(dev, pd_b,
> +			DL_FLAG_STATELESS |
> +			DL_FLAG_PM_RUNTIME |
> +			DL_FLAG_RPM_ACTIVE);
> +
> +
> +	if (!pd_link_b) {
> +		dev_err(dev, "Failed to add device_link to mu a.\n");
> +		goto err_pd_b;
> +	}
> +
> +	ret = imx_mu_msi_domains_init(msi_data, dev);
> +	if (ret)
> +		goto err_dm_init;
> +
> +	irq_set_chained_handler_and_data(irq,
> +					 imx_mu_msi_irq_handler,
> +					 msi_data);
> +
> +	pm_runtime_enable(dev);

Shouldn't you enable the device PM before registering the chained
handler?

	M.

Frank Li Sept. 8, 2022, 2:23 p.m. UTC | #4

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [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]; [e-mail];
> [e-mail]
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
> 
> Caution: EXT Email
> 
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
> 
> Missing full stop after each sentence.

[Frank Li] Do you means missed "."?
 

> 
> >
> > Signed-off-by: Frank Li <[e-mail]>
> > ---
> >  drivers/irqchip/Kconfig          |   9 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> >  3 files changed, 461 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> >       help
> >         Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > +     bool "i.MX MU work as MSI controller"
> 
> Why bool? Doesn't it also work as a module?

[Frank Li] I remember you said that irq-chip can't be removed. 
So I am not sure why need build as module.  

> 
> > +     default y if ARCH_MXC
> 
> Why would this be selected by default?
> 
> > +     select IRQ_DOMAIN
> > +     select IRQ_DOMAIN_HIERARCHY
> > +     select GENERIC_MSI_IRQ_DOMAIN
> > +     help
> > +       MU work as MSI controller to do general doorbell
> 
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.
> 
> > +
> >  config LS1X_IRQ
> >       bool "Loongson-1 Interrupt Controller"
> >       depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)            += irq-riscv-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)            += irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)           += irq-imx-irqsteer.o
> >  obj-$(CONFIG_IMX_INTMUX)             += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI)             += irq-imx-mu-msi.o
> >  obj-$(CONFIG_MADERA_IRQ)             += irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)                       += irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)    += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
> 
> s/worked/used/
> 
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <[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>
> 
> Keep this list in alphabetical order.
> 
> > +
> > +
> > +#define IMX_MU_CHANS            4
> > +
> > +enum imx_mu_xcr {
> > +     IMX_MU_GIER,
> > +     IMX_MU_GCR,
> > +     IMX_MU_TCR,
> > +     IMX_MU_RCR,
> > +     IMX_MU_xCR_MAX,
> 
> What is this last enum used for?
> 
> > +};
> > +
> > +enum imx_mu_xsr {
> > +     IMX_MU_SR,
> > +     IMX_MU_GSR,
> > +     IMX_MU_TSR,
> > +     IMX_MU_RSR,
> > +};
> > +
> > +enum imx_mu_type {
> > +     IMX_MU_V1 = BIT(0),
> 
> This is never used. Why?
> 
> > +     IMX_MU_V2 = BIT(1),
> > +     IMX_MU_V2_S4 = BIT(15),
> 
> Same thing.
> 
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > +     enum imx_mu_type type;
> > +     u32     xTR;            /* Transmit Register0 */
> > +     u32     xRR;            /* Receive Register0 */
> > +     u32     xSR[4];         /* Status Registers */
> > +     u32     xCR[4];         /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > +     spinlock_t                      lock;
> > +     raw_spinlock_t                  reglock;
> 
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?

[Frank Li] Previously your comment, ask me to use raw_spinlock for read\write register access. 
I don't think raw_spinlock is good for MSI allocation. 

> 
> Also, where are these locks initialised?
> 

[Frank Li] struct imx_mu_msi is fill zero when allocated.
Does it still need additional initialization for spinlock?

> > +     struct irq_domain               *msi_domain;
> > +     void __iomem                    *regs;
> > +     phys_addr_t                     msiir_addr;
> > +     const struct imx_mu_dcfg        *cfg;
> > +     unsigned long                   used;
> > +     struct clk                      *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > +     iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > +     return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > +     val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > +     val &= ~clr;
> > +     val |= set;
> > +     imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > +     raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > +     return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > +     .name = "MU-MSI",
> > +     .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > +     .ops    = &imx_mu_msi_irq_ops,
> > +     .chip   = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > +                                       struct msi_msg *msg)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +     u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > +     msg->address_hi = upper_32_bits(addr);
> > +     msg->address_lo = lower_32_bits(addr);
> > +     msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > +                                const struct cpumask *mask, bool force)
> > +{
> > +     return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > +     .name           = "MU",
> > +     .irq_mask       = imx_mu_msi_parent_mask_irq,
> > +     .irq_unmask     = imx_mu_msi_parent_unmask_irq,
> > +     .irq_ack        = imx_mu_msi_parent_ack_irq,
> > +     .irq_compose_msi_msg    = imx_mu_msi_parent_compose_msg,
> > +     .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > +                                     unsigned int virq,
> > +                                     unsigned int nr_irqs,
> > +                                     void *args)
> > +{
> > +     struct imx_mu_msi *msi_data = domain->host_data;
> > +     unsigned long flags;
> > +     int pos, err = 0;
> > +
> > +     WARN_ON(nr_irqs != 1);
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > +     if (pos < IMX_MU_CHANS)
> > +             __set_bit(pos, &msi_data->used);
> > +     else
> > +             err = -ENOSPC;
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     irq_domain_set_info(domain, virq, pos,
> > +                         &imx_mu_msi_parent_chip, msi_data,
> > +                         handle_edge_irq, NULL, NULL);
> > +     return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > +                                    unsigned int virq, unsigned int nr_irqs)
> > +{
> > +     struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     __clear_bit(d->hwirq, &msi_data->used);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > +     .alloc  = imx_mu_msi_domain_irq_alloc,
> > +     .free   = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > +     struct irq_chip *chip = irq_desc_get_chip(desc);
> > +     u32 status;
> > +     int i;
> > +
> > +     status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > +     chained_irq_enter(chip, desc);
> > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > +             if (status & IMX_MU_xSR_RFn(msi_data, i))
> > +                     generic_handle_domain_irq(msi_data->msi_domain, i);
> > +     }
> > +     chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > +     struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > +     struct irq_domain *parent;
> > +
> > +     /* Initialize MSI domain parent */
> > +     parent = irq_domain_create_linear(fwnodes,
> > +                                         IMX_MU_CHANS,
> > +                                         &imx_mu_msi_domain_ops,
> > +                                         msi_data);
> > +     if (!parent) {
> > +             dev_err(dev, "failed to create IRQ domain\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > +     msi_data->msi_domain = platform_msi_create_irq_domain(
> > +                             fwnodes,
> > +                             &imx_mu_msi_domain_info,
> > +                             parent);
> 
> nit: move the first argument after the opening bracket (longer lines
> are fine).
> 
> > +
> > +     if (!msi_data->msi_domain) {
> > +             dev_err(dev, "failed to create MSI domain\n");
> > +             irq_domain_remove(parent);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> 
> Why doesn't this have a type?
> 
> > +     .xTR    = 0x0,
> > +     .xRR    = 0x10,
> > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> 
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
> 
>         .xSR = {
>                 [IMX_MU_SR]     = 0x20,
>                 [IMX_MU_GSR]    = 0x20,
>                 [...]
>         },
> 
> > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +     .xTR    = 0x20,
> > +     .xRR    = 0x40,
> > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > +     .type   = IMX_MU_V2,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > +                              struct device_node *parent,
> > +                              const struct imx_mu_dcfg *cfg
> > +                             )
> 
> Move closing bracket after 'cfg'.
> 
> > +{
> > +     struct platform_device *pdev = of_find_device_by_node(dn);
> > +     struct device_link *pd_link_a;
> > +     struct device_link *pd_link_b;
> > +     struct imx_mu_msi *msi_data;
> > +     struct resource *res;
> > +     struct device *pd_a;
> > +     struct device *pd_b;
> > +     struct device *dev;
> > +     int ret;
> > +     int irq;
> > +
> > +     if (!pdev)
> > +             return -ENODEV;
> 
> How can that happen?
> 
[Frank Li] Not sure, many driver check as it. 

> > +
> > +     dev = &pdev->dev;
> > +
> > +     msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> > +     if (!msi_data)
> > +             return -ENOMEM;
> > +
> > +     msi_data->cfg = cfg;
> > +
> > +     msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > +     if (IS_ERR(msi_data->regs)) {
> > +             dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > +             return PTR_ERR(msi_data->regs);
> > +     }
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > +     if (!res)
> > +             return -EIO;
> > +
> > +     msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq <= 0)
> > +             return -ENODEV;
> > +
> > +     platform_set_drvdata(pdev, msi_data);
> > +
> > +     msi_data->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(msi_data->clk)) {
> > +             if (PTR_ERR(msi_data->clk) != -ENOENT)
> > +                     return PTR_ERR(msi_data->clk);
> > +
> > +             msi_data->clk = NULL;
> 
> Why is it acceptable to continue with no clock?
> 
> > +     }
> > +
> > +     pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > +     if (IS_ERR(pd_a))
> > +             return PTR_ERR(pd_a);
> > +
> > +     pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > +     if (IS_ERR(pd_b))
> > +             return PTR_ERR(pd_b);
> > +
> > +     pd_link_a = device_link_add(dev, pd_a,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!pd_link_a) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             goto err_pd_a;
> > +     }
> > +
> > +     pd_link_b = device_link_add(dev, pd_b,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > +     if (!pd_link_b) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             goto err_pd_b;
> > +     }
> > +
> > +     ret = imx_mu_msi_domains_init(msi_data, dev);
> > +     if (ret)
> > +             goto err_dm_init;
> > +
> > +     irq_set_chained_handler_and_data(irq,
> > +                                      imx_mu_msi_irq_handler,
> > +                                      msi_data);
> > +
> > +     pm_runtime_enable(dev);
> 
> Shouldn't you enable the device PM before registering the chained
> handler?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Frank Li Sept. 8, 2022, 2:26 p.m. UTC | #5

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, September 8, 2022 2:02 AM
> To: kernel test robot <[e-mail]>; Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; kbuild-
> [e-mail]; [e-mail]; [e-mail];
> [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]; [e-mail];
> [e-mail]
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
> 
> Caution: EXT Email
> 
> Frank,
> 
> On Thu, 08 Sep 2022 01:03:27 +0100,
> kernel test robot <[e-mail]> wrote:
> >
> > Hi Frank,
> >
> > I love your patch! Yet something to improve:
> >
> > [auto build test ERROR on jonmason-ntb/ntb-next]
> > [also build test ERROR on robh/for-next linus/master v6.0-rc4 next-
> 20220907]
> > [cannot apply to tip/irq/core]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit-
> scm.com%2Fdocs%2Fgit-format-
> patch%23_base_tree_information&amp;data=05%7C01%7CFrank.Li%40nxp.c
> om%7Cc409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFpbGZsb
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%7C3000%7C%7C%7C&amp;sdata=LgRkCWQ%2BX0GLT5qUaLvIS9SuYMB
> 6tX%2FerPNl3KwW7Tc%3D&amp;reserved=0]
> >
> > url:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fintel-lab-lkp%2Flinux%2Fcommits%2FFrank-Li%2FPCI-EP-driver-
> support-MSI-doorbell-from-host%2F20220907-
> 115114&amp;data=05%7C01%7CFrank.Li%40nxp.com%7Cc409668bc0994f1d
> f11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 37982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7
> C&amp;sdata=hpU6XQmibxCGo0S8J7VuPuZDhe6OwCzR92ld9UvODYw%3D&
> amp;reserved=0
> > base:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fjonmason%2Fntb&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C
> c409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&amp;sdata=3y7%2BH6wcN%2FxcV8swP3QR0lUihWhqw
> qjlTtgUAVAPVg8%3D&amp;reserved=0 ntb-next
> > config: s390-randconfig-s033-20220907
> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdow
> nload.01.org%2F0day-
> ci%2Farchive%2F20220908%2F202209080757.hQMfrrfm-
> lkp%40intel.com%2Fconfig&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C
> c409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&amp;sdata=lFaWJd2d3ob06d3qNilgFovocFU%2FN4Goz
> 7jBTXLvCss%3D&amp;reserved=0)
> > compiler: s390-linux-gcc (GCC) 12.1.0
> > reproduce:
> >         wget
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fraw.gi
> thubusercontent.com%2Fintel%2Flkp-
> tests%2Fmaster%2Fsbin%2Fmake.cross&amp;data=05%7C01%7CFrank.Li%40
> nxp.com%7Cc409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=8C2pQ57ym0gNi0wHXT6KEj8%
> 2BAgMQKph8UhK6GPo%2BNIc%3D&amp;reserved=0 -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # apt-get install sparse
> >         # sparse version: v0.6.4-39-gce1a6720-dirty
> >         #
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fintel-lab-
> lkp%2Flinux%2Fcommit%2Fc1f079e633c10b4f2f1f3c8f52e447d13fda8ddb&a
> mp;data=05%7C01%7CFrank.Li%40nxp.com%7Cc409668bc0994f1df11708da9
> 1681c6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379821735
> 98672297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sda
> ta=oBYu8TZJ15AxtGO2IdtGJdE80fYIJTwF4RlkBkeO6hA%3D&amp;reserved=0
> >         git remote add linux-review
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fintel-lab-
> lkp%2Flinux&amp;data=05%7C01%7CFrank.Li%40nxp.com%7Cc409668bc099
> 4f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C637982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&amp;sdata=AQn3FPjP13rnvvkA7TxRs3gpfd26siSoKM%2B%2Fwzw2J94
> %3D&amp;reserved=0
> >         git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-
> doorbell-from-host/20220907-115114
> >         git checkout c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0
> make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir
> ARCH=s390 SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <[e-mail]>
> >
> > All errors (new ones prefixed by >>):
> >
> >    s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function
> `imx_mu_of_init':
> > >> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to
> `devm_platform_ioremap_resource_byname'
> 
> This is about the 4th time this breakage gets reported. You keep
> reposting this series without addressing it. What is it going to take
> for you to finally fix it? Clearly, I'm not going to bother taking a
> series that has pending build breakages.

[Frank Li] I also frustrate it now.  Robot use random config and can't 
Report all problems once.  Recently update to gcc 12.x.  Build broken
Happen at other place at my environment.  

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

Marc Zyngier Sept. 8, 2022, 2:35 p.m. UTC | #6

On Thu, 08 Sep 2022 15:26:50 +0100,
Frank Li <[e-mail]> wrote:
> 
> > >    s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function
> > `imx_mu_of_init':
> > > >> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to
> > `devm_platform_ioremap_resource_byname'
> > 
> > This is about the 4th time this breakage gets reported. You keep
> > reposting this series without addressing it. What is it going to take
> > for you to finally fix it? Clearly, I'm not going to bother taking a
> > series that has pending build breakages.
> 
> [Frank Li] I also frustrate it now.  Robot use random config and can't 
> Report all problems once.  Recently update to gcc 12.x.  Build broken
> Happen at other place at my environment.  

Well, that's your job to address them. Honestly, cross-compiling for a
few extra architectures isn't that hard.

	M.

Marc Zyngier Sept. 8, 2022, 2:51 p.m. UTC | #7

On Thu, 08 Sep 2022 15:23:53 +0100,
Frank Li <[e-mail]> wrote:
> 
> >
> > On Wed, 07 Sep 2022 04:48:54 +0100,
> > Frank Li <[e-mail]> wrote:
> > >
> > > The MU block found in a number of Freescale/NXP SoCs supports
> > generating
> > > IRQs by writing data to a register
> > >
> > > This enables the MU block to be used as a MSI controller, by leveraging
> > > the platform-MSI API
> > 
> > Missing full stop after each sentence.
> 
> [Frank Li] Do you means missed "."?

Yes.

> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 5e4e50122777d..e04c6521dce55 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > >       help
> > >         Support for the i.MX INTMUX interrupt multiplexer.
> > >
> > > +config IMX_MU_MSI
> > > +     bool "i.MX MU work as MSI controller"
> > 
> > Why bool? Doesn't it also work as a module?
> 
> [Frank Li] I remember you said that irq-chip can't be removed. 
> So I am not sure why need build as module.

Not being removed doesn't mean it cannot be built as a module and
loaded on demand. Why should I be forced to have this driver built-in
if my kernel is used on a variety of systems, only one of them having
this device?

> > > +
> > > +struct imx_mu_msi {
> > > +     spinlock_t                      lock;
> > > +     raw_spinlock_t                  reglock;
> > 
> > Why two locks? Isn't one enough to protect both MSI allocation (which
> > happens once in a blue moon) and register access?
> 
> [Frank Li] Previously your comment, ask me to use raw_spinlock for
> read\write register access.  I don't think raw_spinlock is good for
> MSI allocation.

Why wouldn't it be good enough? I'd really like to know.

> 
> > 
> > Also, where are these locks initialised?
> > 
> 
> [Frank Li] struct imx_mu_msi is fill zero when allocated.
> Does it still need additional initialization for spinlock?

Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK?  Maybe you should
try it.

> > > +     if (!pdev)
> > > +             return -ENODEV;
> > 
> > How can that happen?
> > 
> [Frank Li] Not sure, many driver check as it. 

And? Just because someone does something pointless, you have to
imitate them?

	M.

Frank Li Sept. 8, 2022, 3:35 p.m. UTC | #8

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, September 8, 2022 9:52 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [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]; [e-mail];
> [e-mail]
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
> 
> Caution: EXT Email
> 
> On Thu, 08 Sep 2022 15:23:53 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > >
> > > On Wed, 07 Sep 2022 04:48:54 +0100,
> > > Frank Li <[e-mail]> wrote:
> > > >
> > > > The MU block found in a number of Freescale/NXP SoCs supports
> > > generating
> > > > IRQs by writing data to a register
> > > >
> > > > This enables the MU block to be used as a MSI controller, by leveraging
> > > > the platform-MSI API
> > >
> > > Missing full stop after each sentence.
> >
> > [Frank Li] Do you means missed "."?
> 
> Yes.
> 
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index 5e4e50122777d..e04c6521dce55 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > > >       help
> > > >         Support for the i.MX INTMUX interrupt multiplexer.
> > > >
> > > > +config IMX_MU_MSI
> > > > +     bool "i.MX MU work as MSI controller"
> > >
> > > Why bool? Doesn't it also work as a module?
> >
> > [Frank Li] I remember you said that irq-chip can't be removed.
> > So I am not sure why need build as module.
> 
> Not being removed doesn't mean it cannot be built as a module and
> loaded on demand. Why should I be forced to have this driver built-in
> if my kernel is used on a variety of systems, only one of them having
> this device?
> 
> > > > +
> > > > +struct imx_mu_msi {
> > > > +     spinlock_t                      lock;
> > > > +     raw_spinlock_t                  reglock;
> > >
> > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > happens once in a blue moon) and register access?
> >
> > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > read\write register access.  I don't think raw_spinlock is good for
> > MSI allocation.
> 
> Why wouldn't it be good enough? I'd really like to know.[Frank Li] '

[Frank Li] According to my understand, raw_spinlock skip some lockdep
/debug feature to get better performance, which should be used when
Frequently call, such as irq handle\polling thread. 

Spinlock have DEBUG feature to check wrong use lock.  Allocate MSI generally
only is call once when driver probe.  

The basic principle,  lock should be used only when necessary.  Access reg and
Allocate msi is totally independence events.

For this case, it is not big detail.  

1.  change spin_lock to raw_spin_lock at allocate msi function.

2.  kept define spinlock_t lock;
     In register modify function using
	Raw_spin_lock(spinlock_check(lock), flags).

> 
> >
> > >
> > > Also, where are these locks initialised?
> > >
> >
> > [Frank Li] struct imx_mu_msi is fill zero when allocated.
> > Does it still need additional initialization for spinlock?
> 
> Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK?  Maybe you
> should
> try it.
> 
> > > > +     if (!pdev)
> > > > +             return -ENODEV;
> > >
> > > How can that happen?
> > >
> > [Frank Li] Not sure, many driver check as it.
> 
> And? Just because someone does something pointless, you have to
> imitate them?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier Sept. 9, 2022, 12:07 p.m. UTC | #9

On Thu, 08 Sep 2022 16:35:20 +0100,
Frank Li <[e-mail]> wrote:

> > > > > +struct imx_mu_msi {
> > > > > +     spinlock_t                      lock;
> > > > > +     raw_spinlock_t                  reglock;
> > > >
> > > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > > happens once in a blue moon) and register access?
> > >
> > > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > > read\write register access.  I don't think raw_spinlock is good for
> > > MSI allocation.
> > 
> > Why wouldn't it be good enough? I'd really like to know.[Frank Li] '
> 
> [Frank Li] According to my understand, raw_spinlock skip some lockdep
> /debug feature to get better performance, which should be used when
> Frequently call, such as irq handle\polling thread.

I'm afraid you are terribly misguided. They both have the same debug
features because they are both using the same core implementation, and
the only difference is whether this is preemptible for RT purposes or
not.

> Spinlock have DEBUG feature to check wrong use lock.  Allocate MSI generally
> only is call once when driver probe.

Again, you should really read the code and the documentation and stop
making things up.

> 
> The basic principle,  lock should be used only when necessary.  Access reg and
> Allocate msi is totally independence events.

Independent events that do not occur simultaneously. So no harm in
sharing the same lock.

	M.

Frank Li Sept. 9, 2022, 2:52 p.m. UTC | #10

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [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]; [e-mail];
> [e-mail]
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
> 
> Caution: EXT Email
> 
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
> 
> Missing full stop after each sentence.
> 
> >
> > Signed-off-by: Frank Li <[e-mail]>
> > ---
> >  drivers/irqchip/Kconfig          |   9 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> >  3 files changed, 461 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> >       help
> >         Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > +     bool "i.MX MU work as MSI controller"
> 
> Why bool? Doesn't it also work as a module?
> 
> > +     default y if ARCH_MXC
> 
> Why would this be selected by default?
> 
> > +     select IRQ_DOMAIN
> > +     select IRQ_DOMAIN_HIERARCHY
> > +     select GENERIC_MSI_IRQ_DOMAIN
> > +     help
> > +       MU work as MSI controller to do general doorbell
> 
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.

[Frank Li] I think the only limitation is only 4 irq numbers. 
The principle CPU to CPU irq is the same as MSI. 

What's  your preferred help description?

> 
> > +
> >  config LS1X_IRQ
> >       bool "Loongson-1 Interrupt Controller"
> >       depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)            += irq-riscv-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)            += irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)           += irq-imx-irqsteer.o
> >  obj-$(CONFIG_IMX_INTMUX)             += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI)             += irq-imx-mu-msi.o
> >  obj-$(CONFIG_MADERA_IRQ)             += irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)                       += irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)    += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
> 
> s/worked/used/
> 
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <[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>
> 
> Keep this list in alphabetical order.
> 
> > +
> > +
> > +#define IMX_MU_CHANS            4
> > +
> > +enum imx_mu_xcr {
> > +     IMX_MU_GIER,
> > +     IMX_MU_GCR,
> > +     IMX_MU_TCR,
> > +     IMX_MU_RCR,
> > +     IMX_MU_xCR_MAX,
> 
> What is this last enum used for?
> 
> > +};
> > +
> > +enum imx_mu_xsr {
> > +     IMX_MU_SR,
> > +     IMX_MU_GSR,
> > +     IMX_MU_TSR,
> > +     IMX_MU_RSR,
> > +};
> > +
> > +enum imx_mu_type {
> > +     IMX_MU_V1 = BIT(0),
> 
> This is never used. Why?
> 
> > +     IMX_MU_V2 = BIT(1),
> > +     IMX_MU_V2_S4 = BIT(15),
> 
> Same thing.
> 
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > +     enum imx_mu_type type;
> > +     u32     xTR;            /* Transmit Register0 */
> > +     u32     xRR;            /* Receive Register0 */
> > +     u32     xSR[4];         /* Status Registers */
> > +     u32     xCR[4];         /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > +     spinlock_t                      lock;
> > +     raw_spinlock_t                  reglock;
> 
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?
> 
> Also, where are these locks initialised?
> 
> > +     struct irq_domain               *msi_domain;
> > +     void __iomem                    *regs;
> > +     phys_addr_t                     msiir_addr;
> > +     const struct imx_mu_dcfg        *cfg;
> > +     unsigned long                   used;
> > +     struct clk                      *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > +     iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > +     return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > +     val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > +     val &= ~clr;
> > +     val |= set;
> > +     imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > +     raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > +     return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > +     .name = "MU-MSI",
> > +     .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > +     .ops    = &imx_mu_msi_irq_ops,
> > +     .chip   = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > +                                       struct msi_msg *msg)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +     u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > +     msg->address_hi = upper_32_bits(addr);
> > +     msg->address_lo = lower_32_bits(addr);
> > +     msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > +                                const struct cpumask *mask, bool force)
> > +{
> > +     return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > +     .name           = "MU",
> > +     .irq_mask       = imx_mu_msi_parent_mask_irq,
> > +     .irq_unmask     = imx_mu_msi_parent_unmask_irq,
> > +     .irq_ack        = imx_mu_msi_parent_ack_irq,
> > +     .irq_compose_msi_msg    = imx_mu_msi_parent_compose_msg,
> > +     .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > +                                     unsigned int virq,
> > +                                     unsigned int nr_irqs,
> > +                                     void *args)
> > +{
> > +     struct imx_mu_msi *msi_data = domain->host_data;
> > +     unsigned long flags;
> > +     int pos, err = 0;
> > +
> > +     WARN_ON(nr_irqs != 1);
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > +     if (pos < IMX_MU_CHANS)
> > +             __set_bit(pos, &msi_data->used);
> > +     else
> > +             err = -ENOSPC;
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     irq_domain_set_info(domain, virq, pos,
> > +                         &imx_mu_msi_parent_chip, msi_data,
> > +                         handle_edge_irq, NULL, NULL);
> > +     return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > +                                    unsigned int virq, unsigned int nr_irqs)
> > +{
> > +     struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     __clear_bit(d->hwirq, &msi_data->used);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > +     .alloc  = imx_mu_msi_domain_irq_alloc,
> > +     .free   = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > +     struct irq_chip *chip = irq_desc_get_chip(desc);
> > +     u32 status;
> > +     int i;
> > +
> > +     status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > +     chained_irq_enter(chip, desc);
> > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > +             if (status & IMX_MU_xSR_RFn(msi_data, i))
> > +                     generic_handle_domain_irq(msi_data->msi_domain, i);
> > +     }
> > +     chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > +     struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > +     struct irq_domain *parent;
> > +
> > +     /* Initialize MSI domain parent */
> > +     parent = irq_domain_create_linear(fwnodes,
> > +                                         IMX_MU_CHANS,
> > +                                         &imx_mu_msi_domain_ops,
> > +                                         msi_data);
> > +     if (!parent) {
> > +             dev_err(dev, "failed to create IRQ domain\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > +     msi_data->msi_domain = platform_msi_create_irq_domain(
> > +                             fwnodes,
> > +                             &imx_mu_msi_domain_info,
> > +                             parent);
> 
> nit: move the first argument after the opening bracket (longer lines
> are fine).
> 
> > +
> > +     if (!msi_data->msi_domain) {
> > +             dev_err(dev, "failed to create MSI domain\n");
> > +             irq_domain_remove(parent);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> 
> Why doesn't this have a type?
> 
> > +     .xTR    = 0x0,
> > +     .xRR    = 0x10,
> > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> 
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
> 
>         .xSR = {
>                 [IMX_MU_SR]     = 0x20,
>                 [IMX_MU_GSR]    = 0x20,
>                 [...]
>         },
> 
> > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +     .xTR    = 0x20,
> > +     .xRR    = 0x40,
> > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > +     .type   = IMX_MU_V2,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > +                              struct device_node *parent,
> > +                              const struct imx_mu_dcfg *cfg
> > +                             )
> 
> Move closing bracket after 'cfg'.
> 
> > +{
> > +     struct platform_device *pdev = of_find_device_by_node(dn);
> > +     struct device_link *pd_link_a;
> > +     struct device_link *pd_link_b;
> > +     struct imx_mu_msi *msi_data;
> > +     struct resource *res;
> > +     struct device *pd_a;
> > +     struct device *pd_b;
> > +     struct device *dev;
> > +     int ret;
> > +     int irq;
> > +
> > +     if (!pdev)
> > +             return -ENODEV;
> 
> How can that happen?
> 
> > +
> > +     dev = &pdev->dev;
> > +
> > +     msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> > +     if (!msi_data)
> > +             return -ENOMEM;
> > +
> > +     msi_data->cfg = cfg;
> > +
> > +     msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > +     if (IS_ERR(msi_data->regs)) {
> > +             dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > +             return PTR_ERR(msi_data->regs);
> > +     }
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > +     if (!res)
> > +             return -EIO;
> > +
> > +     msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq <= 0)
> > +             return -ENODEV;
> > +
> > +     platform_set_drvdata(pdev, msi_data);
> > +
> > +     msi_data->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(msi_data->clk)) {
> > +             if (PTR_ERR(msi_data->clk) != -ENOENT)
> > +                     return PTR_ERR(msi_data->clk);
> > +
> > +             msi_data->clk = NULL;
> 
> Why is it acceptable to continue with no clock?
> 
> > +     }
> > +
> > +     pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > +     if (IS_ERR(pd_a))
> > +             return PTR_ERR(pd_a);
> > +
> > +     pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > +     if (IS_ERR(pd_b))
> > +             return PTR_ERR(pd_b);
> > +
> > +     pd_link_a = device_link_add(dev, pd_a,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!pd_link_a) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             goto err_pd_a;
> > +     }
> > +
> > +     pd_link_b = device_link_add(dev, pd_b,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > +     if (!pd_link_b) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             goto err_pd_b;
> > +     }
> > +
> > +     ret = imx_mu_msi_domains_init(msi_data, dev);
> > +     if (ret)
> > +             goto err_dm_init;
> > +
> > +     irq_set_chained_handler_and_data(irq,
> > +                                      imx_mu_msi_irq_handler,
> > +                                      msi_data);
> > +
> > +     pm_runtime_enable(dev);
> 
> Shouldn't you enable the device PM before registering the chained
> handler?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Frank Li Sept. 9, 2022, 2:59 p.m. UTC | #11

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Friday, September 9, 2022 7:08 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [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]; [e-mail];
> [e-mail]
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
> 
> Caution: EXT Email
> 
> On Thu, 08 Sep 2022 16:35:20 +0100,
> Frank Li <[e-mail]> wrote:
> 
> > > > > > +struct imx_mu_msi {
> > > > > > +     spinlock_t                      lock;
> > > > > > +     raw_spinlock_t                  reglock;
> > > > >
> > > > > Why two locks? Isn't one enough to protect both MSI allocation
> (which
> > > > > happens once in a blue moon) and register access?
> > > >
> > > > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > > > read\write register access.  I don't think raw_spinlock is good for
> > > > MSI allocation.
> > >
> > > Why wouldn't it be good enough? I'd really like to know.[Frank Li] '
> >
> > [Frank Li] According to my understand, raw_spinlock skip some lockdep
> > /debug feature to get better performance, which should be used when
> > Frequently call, such as irq handle\polling thread.
> 
> I'm afraid you are terribly misguided. They both have the same debug
> features because they are both using the same core implementation, and
> the only difference is whether this is preemptible for RT purposes or
> not.
> 
> > Spinlock have DEBUG feature to check wrong use lock.  Allocate MSI
> generally
> > only is call once when driver probe.
> 
> Again, you should really read the code and the documentation and stop
> making things up.

[Frank Li] Thanks. You give me the correct direction. Some stackoverflow's
Doc was misleaded.  I double checked spin_lock implementation.  PREEMPT_RT
Kernel map spin_lock to rt_mutex.

I am curious  why exist spin_lock_irqsave and raw_spin_lock_irqsave before
PREEMTP_RT merge into kernel tree. 

> 
> >
> > The basic principle,  lock should be used only when necessary.  Access reg
> and
> > Allocate msi is totally independence events.
> 
> Independent events that do not occur simultaneously. So no harm in
> sharing the same lock.
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier Sept. 10, 2022, 2:35 p.m. UTC | #12

On Fri, 09 Sep 2022 15:59:01 +0100,
Frank Li <[e-mail]> wrote:
> 
> > >
> > > [Frank Li] According to my understand, raw_spinlock skip some lockdep
> > > /debug feature to get better performance, which should be used when
> > > Frequently call, such as irq handle\polling thread.
> > 
> > I'm afraid you are terribly misguided. They both have the same debug
> > features because they are both using the same core implementation, and
> > the only difference is whether this is preemptible for RT purposes or
> > not.
> > 
> > > Spinlock have DEBUG feature to check wrong use lock.  Allocate MSI
> > generally
> > > only is call once when driver probe.
> > 
> > Again, you should really read the code and the documentation and stop
> > making things up.
> 
> [Frank Li] Thanks. You give me the correct direction. Some stackoverflow's
> Doc was misleaded.  I double checked spin_lock implementation.  PREEMPT_RT
> Kernel map spin_lock to rt_mutex.
> 
> I am curious  why exist spin_lock_irqsave and raw_spin_lock_irqsave before
> PREEMTP_RT merge into kernel tree. 

Because the RT merge has been going on for 10 years or so, long before
CONFIG_RT was merged. Also, a mutex has a spin lock at its core, and
it makes sense to have a single primitive for all these lock types.

	M.

Marc Zyngier Sept. 10, 2022, 2:40 p.m. UTC | #13

On Fri, 09 Sep 2022 15:52:45 +0100,
Frank Li <[e-mail]> wrote:
> 
> > > +     select IRQ_DOMAIN
> > > +     select IRQ_DOMAIN_HIERARCHY
> > > +     select GENERIC_MSI_IRQ_DOMAIN
> > > +     help
> > > +       MU work as MSI controller to do general doorbell
> > 
> > I'm not sure this is that generic. It really is limited to CPU-to-CPU
> > interrupts.
> 
> [Frank Li] I think the only limitation is only 4 irq numbers. 
> The principle CPU to CPU irq is the same as MSI.

Not quite. Normal MSIs are device-to-CPU. CPU-to-CPU are normally
IPIs, and this one falls in the middle.

> What's  your preferred help description?

<quote>
	Provide a driver for the MU block used as a CPU-to-CPU MSI
	controller. This requires a specially crafted DT to make use
	of this driver.

	If unsure, say N.
</quote>

	M.

Frank Li Sept. 12, 2022, 3:53 p.m. UTC | #14

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, September 8, 2022 9:52 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [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]; [e-mail];
> [e-mail]
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
> 
> Caution: EXT Email
> 
> On Thu, 08 Sep 2022 15:23:53 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > >
> > > On Wed, 07 Sep 2022 04:48:54 +0100,
> > > Frank Li <[e-mail]> wrote:
> > > >
> > > > The MU block found in a number of Freescale/NXP SoCs supports
> > > generating
> > > > IRQs by writing data to a register
> > > >
> > > > This enables the MU block to be used as a MSI controller, by leveraging
> > > > the platform-MSI API
> > >
> > > Missing full stop after each sentence.
> >
> > [Frank Li] Do you means missed "."?
> 
> Yes.
> 
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index 5e4e50122777d..e04c6521dce55 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > > >       help
> > > >         Support for the i.MX INTMUX interrupt multiplexer.
> > > >
> > > > +config IMX_MU_MSI
> > > > +     bool "i.MX MU work as MSI controller"
> > >
> > > Why bool? Doesn't it also work as a module?
> >
> > [Frank Li] I remember you said that irq-chip can't be removed.
> > So I am not sure why need build as module.
> 
> Not being removed doesn't mean it cannot be built as a module and
> loaded on demand. Why should I be forced to have this driver built-in
> if my kernel is used on a variety of systems, only one of them having
> this device?

[Frank Li]  A problem,  platform_msi_create_irq_domain have NOT export to let module
Call it.   https://elixir.bootlin.com/linux/latest/source/drivers/base/platform-msi.c#L122

Do you want to me add EXPORT_SYMBOL_GPL for it  OR keep "bool" here? 
	
> 
> > > > +
> > > > +struct imx_mu_msi {
> > > > +     spinlock_t                      lock;
> > > > +     raw_spinlock_t                  reglock;
> > >
> > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > happens once in a blue moon) and register access?
> >
> > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > read\write register access.  I don't think raw_spinlock is good for
> > MSI allocation.
> 
> Why wouldn't it be good enough? I'd really like to know.
> 
> >
> > >
> > > Also, where are these locks initialised?
> > >
> >
> > [Frank Li] struct imx_mu_msi is fill zero when allocated.
> > Does it still need additional initialization for spinlock?
> 
> Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK?  Maybe you
> should
> try it.
> 
> > > > +     if (!pdev)
> > > > +             return -ENODEV;
> > >
> > > How can that happen?
> > >
> > [Frank Li] Not sure, many driver check as it.
> 
> And? Just because someone does something pointless, you have to
> imitate them?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Frank Li Sept. 12, 2022, 4:17 p.m. UTC | #15

> -----Original Message-----
> From: Marc Zyngier <[e-mail]>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [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]; [e-mail];
> [e-mail]
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
> 
> Caution: EXT Email
> 
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <[e-mail]> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
> 
> Missing full stop after each sentence.
> 
> >
> > Signed-off-by: Frank Li <[e-mail]>
> > ---
> >  drivers/irqchip/Kconfig          |   9 +
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> >  3 files changed, 461 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> >       help
> >         Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > +     bool "i.MX MU work as MSI controller"
> 
> Why bool? Doesn't it also work as a module?
> 
> > +     default y if ARCH_MXC
> 
> Why would this be selected by default?
> 
> > +     select IRQ_DOMAIN
> > +     select IRQ_DOMAIN_HIERARCHY
> > +     select GENERIC_MSI_IRQ_DOMAIN
> > +     help
> > +       MU work as MSI controller to do general doorbell
> 
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.
> 
> > +
> >  config LS1X_IRQ
> >       bool "Loongson-1 Interrupt Controller"
> >       depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)            += irq-riscv-intc.o
> >  obj-$(CONFIG_SIFIVE_PLIC)            += irq-sifive-plic.o
> >  obj-$(CONFIG_IMX_IRQSTEER)           += irq-imx-irqsteer.o
> >  obj-$(CONFIG_IMX_INTMUX)             += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI)             += irq-imx-mu-msi.o
> >  obj-$(CONFIG_MADERA_IRQ)             += irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)                       += irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)    += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
> 
> s/worked/used/
> 
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <[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>
> 
> Keep this list in alphabetical order.
> 
> > +
> > +
> > +#define IMX_MU_CHANS            4
> > +
> > +enum imx_mu_xcr {
> > +     IMX_MU_GIER,
> > +     IMX_MU_GCR,
> > +     IMX_MU_TCR,
> > +     IMX_MU_RCR,
> > +     IMX_MU_xCR_MAX,
> 
> What is this last enum used for?

[Frank Li] I will replace  4 in "u32     xCR[4]; " with 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),
> 
> This is never used. Why?
> 
> > +     IMX_MU_V2 = BIT(1),
> > +     IMX_MU_V2_S4 = BIT(15),
> 
> Same thing.
> 
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > +     enum imx_mu_type type;
> > +     u32     xTR;            /* Transmit Register0 */
> > +     u32     xRR;            /* Receive Register0 */
> > +     u32     xSR[4];         /* Status Registers */
> > +     u32     xCR[4];         /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > +     spinlock_t                      lock;
> > +     raw_spinlock_t                  reglock;
> 
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?
> 
> Also, where are these locks initialised?
> 
> > +     struct irq_domain               *msi_domain;
> > +     void __iomem                    *regs;
> > +     phys_addr_t                     msiir_addr;
> > +     const struct imx_mu_dcfg        *cfg;
> > +     unsigned long                   used;
> > +     struct clk                      *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > +     iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > +     return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > +     val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > +     val &= ~clr;
> > +     val |= set;
> > +     imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > +     raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > +     return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > +     imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > +     .name = "MU-MSI",
> > +     .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > +     .ops    = &imx_mu_msi_irq_ops,
> > +     .chip   = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > +                                       struct msi_msg *msg)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +     u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > +     msg->address_hi = upper_32_bits(addr);
> > +     msg->address_lo = lower_32_bits(addr);
> > +     msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > +                                const struct cpumask *mask, bool force)
> > +{
> > +     return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > +     .name           = "MU",
> > +     .irq_mask       = imx_mu_msi_parent_mask_irq,
> > +     .irq_unmask     = imx_mu_msi_parent_unmask_irq,
> > +     .irq_ack        = imx_mu_msi_parent_ack_irq,
> > +     .irq_compose_msi_msg    = imx_mu_msi_parent_compose_msg,
> > +     .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > +                                     unsigned int virq,
> > +                                     unsigned int nr_irqs,
> > +                                     void *args)
> > +{
> > +     struct imx_mu_msi *msi_data = domain->host_data;
> > +     unsigned long flags;
> > +     int pos, err = 0;
> > +
> > +     WARN_ON(nr_irqs != 1);
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > +     if (pos < IMX_MU_CHANS)
> > +             __set_bit(pos, &msi_data->used);
> > +     else
> > +             err = -ENOSPC;
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     irq_domain_set_info(domain, virq, pos,
> > +                         &imx_mu_msi_parent_chip, msi_data,
> > +                         handle_edge_irq, NULL, NULL);
> > +     return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > +                                    unsigned int virq, unsigned int nr_irqs)
> > +{
> > +     struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&msi_data->lock, flags);
> > +     __clear_bit(d->hwirq, &msi_data->used);
> > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > +     .alloc  = imx_mu_msi_domain_irq_alloc,
> > +     .free   = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > +     struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > +     struct irq_chip *chip = irq_desc_get_chip(desc);
> > +     u32 status;
> > +     int i;
> > +
> > +     status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > +     chained_irq_enter(chip, desc);
> > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > +             if (status & IMX_MU_xSR_RFn(msi_data, i))
> > +                     generic_handle_domain_irq(msi_data->msi_domain, i);
> > +     }
> > +     chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > +     struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > +     struct irq_domain *parent;
> > +
> > +     /* Initialize MSI domain parent */
> > +     parent = irq_domain_create_linear(fwnodes,
> > +                                         IMX_MU_CHANS,
> > +                                         &imx_mu_msi_domain_ops,
> > +                                         msi_data);
> > +     if (!parent) {
> > +             dev_err(dev, "failed to create IRQ domain\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > +     msi_data->msi_domain = platform_msi_create_irq_domain(
> > +                             fwnodes,
> > +                             &imx_mu_msi_domain_info,
> > +                             parent);
> 
> nit: move the first argument after the opening bracket (longer lines
> are fine).
> 
> > +
> > +     if (!msi_data->msi_domain) {
> > +             dev_err(dev, "failed to create MSI domain\n");
> > +             irq_domain_remove(parent);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> 
> Why doesn't this have a type?
> 
> > +     .xTR    = 0x0,
> > +     .xRR    = 0x10,
> > +     .xSR    = {0x20, 0x20, 0x20, 0x20},
> 
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
> 
>         .xSR = {
>                 [IMX_MU_SR]     = 0x20,
>                 [IMX_MU_GSR]    = 0x20,
>                 [...]
>         },
> 
> > +     .xCR    = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +     .xTR    = 0x20,
> > +     .xRR    = 0x40,
> > +     .xSR    = {0x60, 0x60, 0x60, 0x60},
> > +     .xCR    = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > +     .type   = IMX_MU_V2,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > +     .type   = IMX_MU_V2 | IMX_MU_V2_S4,
> > +     .xTR    = 0x200,
> > +     .xRR    = 0x280,
> > +     .xSR    = {0xC, 0x118, 0x124, 0x12C},
> > +     .xCR    = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > +                              struct device_node *parent,
> > +                              const struct imx_mu_dcfg *cfg
> > +                             )
> 
> Move closing bracket after 'cfg'.
> 
> > +{
> > +     struct platform_device *pdev = of_find_device_by_node(dn);
> > +     struct device_link *pd_link_a;
> > +     struct device_link *pd_link_b;
> > +     struct imx_mu_msi *msi_data;
> > +     struct resource *res;
> > +     struct device *pd_a;
> > +     struct device *pd_b;
> > +     struct device *dev;
> > +     int ret;
> > +     int irq;
> > +
> > +     if (!pdev)
> > +             return -ENODEV;
> 
> How can that happen?
> 
> > +
> > +     dev = &pdev->dev;
> > +
> > +     msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> > +     if (!msi_data)
> > +             return -ENOMEM;
> > +
> > +     msi_data->cfg = cfg;
> > +
> > +     msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > +     if (IS_ERR(msi_data->regs)) {
> > +             dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > +             return PTR_ERR(msi_data->regs);
> > +     }
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > +     if (!res)
> > +             return -EIO;
> > +
> > +     msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq <= 0)
> > +             return -ENODEV;
> > +
> > +     platform_set_drvdata(pdev, msi_data);
> > +
> > +     msi_data->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(msi_data->clk)) {
> > +             if (PTR_ERR(msi_data->clk) != -ENOENT)
> > +                     return PTR_ERR(msi_data->clk);
> > +
> > +             msi_data->clk = NULL;
> 
> Why is it acceptable to continue with no clock?
> 
> > +     }
> > +
> > +     pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > +     if (IS_ERR(pd_a))
> > +             return PTR_ERR(pd_a);
> > +
> > +     pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > +     if (IS_ERR(pd_b))
> > +             return PTR_ERR(pd_b);
> > +
> > +     pd_link_a = device_link_add(dev, pd_a,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +     if (!pd_link_a) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             goto err_pd_a;
> > +     }
> > +
> > +     pd_link_b = device_link_add(dev, pd_b,
> > +                     DL_FLAG_STATELESS |
> > +                     DL_FLAG_PM_RUNTIME |
> > +                     DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > +     if (!pd_link_b) {
> > +             dev_err(dev, "Failed to add device_link to mu a.\n");
> > +             goto err_pd_b;
> > +     }
> > +
> > +     ret = imx_mu_msi_domains_init(msi_data, dev);
> > +     if (ret)
> > +             goto err_dm_init;
> > +
> > +     irq_set_chained_handler_and_data(irq,
> > +                                      imx_mu_msi_irq_handler,
> > +                                      msi_data);
> > +
> > +     pm_runtime_enable(dev);
> 
> Shouldn't you enable the device PM before registering the chained
> handler?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier Sept. 13, 2022, 5:44 p.m. UTC | #16

On Mon, 12 Sep 2022 16:53:40 +0100,
Frank Li <[e-mail]> wrote:
> 
> > > [Frank Li] I remember you said that irq-chip can't be removed.
> > > So I am not sure why need build as module.
> > 
> > Not being removed doesn't mean it cannot be built as a module and
> > loaded on demand. Why should I be forced to have this driver built-in
> > if my kernel is used on a variety of systems, only one of them having
> > this device?
> 
> [Frank Li] A problem, platform_msi_create_irq_domain have NOT export
> to let module Call it.
> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform-msi.c#L122
> 
> Do you want to me add EXPORT_SYMBOL_GPL for it  OR keep "bool" here? 

Please add a patch exporting the missing symbols, and make the think
modular.

	M.

Patch

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5e4e50122777d..e04c6521dce55 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -470,6 +470,15 @@  config IMX_INTMUX
 	help
 	  Support for the i.MX INTMUX interrupt multiplexer.
 
+config IMX_MU_MSI
+	bool "i.MX MU work as MSI controller"
+	default y if ARCH_MXC
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	select GENERIC_MSI_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..82b55f6d87266
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,451 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Freescale MU worked as MSI controller
+ *
+ * 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;
+	raw_spinlock_t			reglock;
+	struct irq_domain		*msi_domain;
+	void __iomem			*regs;
+	phys_addr_t			msiir_addr;
+	const struct imx_mu_dcfg	*cfg;
+	unsigned long			used;
+	struct clk			*clk;
+};
+
+static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
+{
+	iowrite32(val, msi_data->regs + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
+{
+	return ioread32(msi_data->regs + offs);
+}
+
+static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
+{
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&msi_data->reglock, flags);
+	val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
+	raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
+
+	return val;
+}
+
+static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
+}
+
+static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
+}
+
+static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+	imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+	.name = "MU-MSI",
+	.irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_ops imx_mu_msi_irq_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.ops	= &imx_mu_msi_irq_ops,
+	.chip	= &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
+					  struct msi_msg *msg)
+{
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+	u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
+
+	msg->address_hi = upper_32_bits(addr);
+	msg->address_lo = lower_32_bits(addr);
+	msg->data = data->hwirq;
+}
+
+static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+	.name		= "MU",
+	.irq_mask	= imx_mu_msi_parent_mask_irq,
+	.irq_unmask	= imx_mu_msi_parent_unmask_irq,
+	.irq_ack	= imx_mu_msi_parent_ack_irq,
+	.irq_compose_msi_msg	= imx_mu_msi_parent_compose_msg,
+	.irq_set_affinity = imx_mu_msi_parent_set_affinity,
+};
+
+static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs,
+					void *args)
+{
+	struct imx_mu_msi *msi_data = domain->host_data;
+	unsigned long flags;
+	int pos, err = 0;
+
+	WARN_ON(nr_irqs != 1);
+
+	spin_lock_irqsave(&msi_data->lock, flags);
+	pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
+	if (pos < IMX_MU_CHANS)
+		__set_bit(pos, &msi_data->used);
+	else
+		err = -ENOSPC;
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+
+	if (err)
+		return err;
+
+	irq_domain_set_info(domain, virq, pos,
+			    &imx_mu_msi_parent_chip, msi_data,
+			    handle_edge_irq, NULL, NULL);
+	return 0;
+}
+
+static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&msi_data->lock, flags);
+	__clear_bit(d->hwirq, &msi_data->used);
+	spin_unlock_irqrestore(&msi_data->lock, flags);
+}
+
+static const struct irq_domain_ops imx_mu_msi_domain_ops = {
+	.alloc	= imx_mu_msi_domain_irq_alloc,
+	.free	= imx_mu_msi_domain_irq_free,
+};
+
+static void imx_mu_msi_irq_handler(struct irq_desc *desc)
+{
+	struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 status;
+	int i;
+
+	status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
+
+	chained_irq_enter(chip, desc);
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		if (status & IMX_MU_xSR_RFn(msi_data, i))
+			generic_handle_domain_irq(msi_data->msi_domain, i);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct device *dev)
+{
+	struct fwnode_handle *fwnodes = dev_fwnode(dev);
+	struct irq_domain *parent;
+
+	/* Initialize MSI domain parent */
+	parent = irq_domain_create_linear(fwnodes,
+					    IMX_MU_CHANS,
+					    &imx_mu_msi_domain_ops,
+					    msi_data);
+	if (!parent) {
+		dev_err(dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
+
+	msi_data->msi_domain = platform_msi_create_irq_domain(
+				fwnodes,
+				&imx_mu_msi_domain_info,
+				parent);
+
+	if (!msi_data->msi_domain) {
+		dev_err(dev, "failed to create MSI domain\n");
+		irq_domain_remove(parent);
+		return -ENOMEM;
+	}
+
+	irq_domain_set_pm_device(msi_data->msi_domain, dev);
+
+	return 0;
+}
+
+/* Register offset of different version MU IP */
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.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 device_link *pd_link_a;
+	struct device_link *pd_link_b;
+	struct imx_mu_msi *msi_data;
+	struct resource *res;
+	struct device *pd_a;
+	struct device *pd_b;
+	struct device *dev;
+	int ret;
+	int irq;
+
+	if (!pdev)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+
+	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
+
+	msi_data->cfg = cfg;
+
+	msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
+	if (IS_ERR(msi_data->regs)) {
+		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
+		return PTR_ERR(msi_data->regs);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
+	if (!res)
+		return -EIO;
+
+	msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return -ENODEV;
+
+	platform_set_drvdata(pdev, msi_data);
+
+	msi_data->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(msi_data->clk)) {
+		if (PTR_ERR(msi_data->clk) != -ENOENT)
+			return PTR_ERR(msi_data->clk);
+
+		msi_data->clk = NULL;
+	}
+
+	pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
+	if (IS_ERR(pd_a))
+		return PTR_ERR(pd_a);
+
+	pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
+	if (IS_ERR(pd_b))
+		return PTR_ERR(pd_b);
+
+	pd_link_a = device_link_add(dev, pd_a,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+	if (!pd_link_a) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		goto err_pd_a;
+	}
+
+	pd_link_b = device_link_add(dev, pd_b,
+			DL_FLAG_STATELESS |
+			DL_FLAG_PM_RUNTIME |
+			DL_FLAG_RPM_ACTIVE);
+
+
+	if (!pd_link_b) {
+		dev_err(dev, "Failed to add device_link to mu a.\n");
+		goto err_pd_b;
+	}
+
+	ret = imx_mu_msi_domains_init(msi_data, dev);
+	if (ret)
+		goto err_dm_init;
+
+	irq_set_chained_handler_and_data(irq,
+					 imx_mu_msi_irq_handler,
+					 msi_data);
+
+	pm_runtime_enable(dev);
+
+	return 0;
+
+err_dm_init:
+	device_link_remove(dev,	pd_b);
+err_pd_b:
+	device_link_remove(dev, pd_a);
+err_pd_a:
+	return -EINVAL;
+}
+
+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 MSI controller driver");
+MODULE_LICENSE("GPL");