Subject: Re: [PATCH] irqchip/ls-scfg-msi: update Layerscape SCFG MSI driver
To: Minghuan Lian <Minghuan.Lian@nxp.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
References: <1477399162-22881-1-git-send-email-Minghuan.Lian@nxp.com>
Cc: Rob Herring <robh@kernel.org>, Jason Cooper <jason@lakedaemon.net>,
        Roy Zang <roy.zang@nxp.com>, Mingkai Hu <mingkai.hu@nxp.com>,
        Stuart Yoder <stuart.yoder@nxp.com>, Yang-Leo Li <leoyang.li@nxp.com>,
        Scott Wood <scott.wood@nxp.com>
From: Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <3b0b6b6a-b6ac-b252-8578-0122b081f0fd@arm.com>
Date: Tue, 25 Oct 2016 14:11:00 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Icedove/45.2.0
MIME-Version: 1.0
In-Reply-To: <1477399162-22881-1-git-send-email-Minghuan.Lian@nxp.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 17423
Lines: 605

On 25/10/16 13:39, Minghuan Lian wrote:
> 1. The patch uses soc_device_match() to match the SoC family
> and revision instead of DTS compatible, because compatible cannot
> describe the SoC revision information.
> 2. The patch provides a new method to support Layerscape
> SCFG MSI. It tries to assign a dedicated MSIR to every core.
> When changing a MSI interrupt affinity, the MSI message
> data will be changed to refer to a new MSIR that has
> been associated with the core.
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
> The patch depends on https://patchwork.kernel.org/patch/9342915/

What is the status of this patch? This is the first time I hear about
it, and I can't find it in -rc2.

> 
>  drivers/irqchip/irq-ls-scfg-msi.c | 444 +++++++++++++++++++++++++++++++-------
>  1 file changed, 367 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
> index 02cca74c..0245d8a 100644
> --- a/drivers/irqchip/irq-ls-scfg-msi.c
> +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> @@ -10,6 +10,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/bitmap.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/msi.h>
> @@ -17,23 +18,91 @@
>  #include <linux/irq.h>
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/spinlock.h>
> +#include <linux/sys_soc.h>
>  
> -#define MSI_MAX_IRQS	32
> -#define MSI_IBS_SHIFT	3
> -#define MSIR		4
> +#define LS_MSIR_NUM_MAX		4 /* MSIIR can index 4 MSI registers */
> +#define IRQS_32_PER_MSIR	32
> +#define IRQS_8_PER_MSIR		8
> +
> +#define MSIR_OFFSET(idx)	((idx) * 0x4)
> +
> +enum msi_affinity_flag {
> +	MSI_GROUP_AFFINITY_FLAG,
> +	MSI_AFFINITY_FLAG
> +};
> +
> +struct ls_scfg_msi;
> +struct ls_scfg_msi_ctrl;
> +
> +struct ls_scfg_msi_cfg {
> +	u32 ibs_shift; /* Shift of interrupt bit select */
> +	u32 msir_irqs; /* The irq number per MSIR */
> +	u32 msir_base; /* The base address of MSIR */
> +};
> +
> +struct ls_scfg_msir {
> +	struct ls_scfg_msi_ctrl	*ctrl;
> +	void __iomem		*addr;
> +	int			index;
> +	int			virq;
> +};
> +
> +struct ls_scfg_msi_ctrl {
> +	struct list_head		list;
> +	struct ls_scfg_msi		*msi_data;
> +	void __iomem			*regs;
> +	phys_addr_t			msiir_addr;
> +	enum msi_affinity_flag		flag;
> +	int				irq_base;
> +	spinlock_t			lock;
> +	struct ls_scfg_msir		*msir;
> +	unsigned long			*bm;
> +};
>  
>  struct ls_scfg_msi {
> -	spinlock_t		lock;
> -	struct platform_device	*pdev;
> -	struct irq_domain	*parent;
> -	struct irq_domain	*msi_domain;
> -	void __iomem		*regs;
> -	phys_addr_t		msiir_addr;
> -	int			irq;
> -	DECLARE_BITMAP(used, MSI_MAX_IRQS);
> +	struct platform_device		*pdev;
> +	struct irq_domain		*parent;
> +	struct irq_domain		*msi_domain;
> +	struct list_head		ctrl_list;
> +	const struct ls_scfg_msi_cfg	*cfg;
> +	u32				cpu_num;
> +};
> +
> +static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
> +	.ibs_shift = 3,
> +	.msir_irqs = IRQS_32_PER_MSIR,
> +	.msir_base = 0x4,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1043_rev11_msi_cfg = {
> +	.ibs_shift = 2,
> +	.msir_irqs = IRQS_8_PER_MSIR,
> +	.msir_base = 0x10,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
> +	.ibs_shift = 2,
> +	.msir_irqs = IRQS_32_PER_MSIR,
> +	.msir_base = 0x4,
> +};
> +
> +static struct soc_device_attribute soc_msi_matches[] = {
> +	{ .family = "QorIQ LS1021A",
> +	  .data = &ls1021_msi_cfg },
> +	{ .family = "QorIQ LS1012A",
> +	  .data = &ls1021_msi_cfg },
> +	{ .family = "QorIQ LS1043A", .revision = "1.0",
> +	  .data = &ls1021_msi_cfg },
> +	{ .family = "QorIQ LS1043A", .revision = "1.1",
> +	  .data = &ls1043_rev11_msi_cfg },
> +	{ .family = "QorIQ LS1046A",
> +	  .data = &ls1046_msi_cfg },
> +	{ },

Right. So after spending about 5 years getting rid of board files, they
are now back, just spread over a zillion drivers? Why isn't that
described in DT?

>  };
>  
>  static struct irq_chip ls_scfg_msi_irq_chip = {
> @@ -49,19 +118,53 @@ struct ls_scfg_msi {
>  	.chip	= &ls_scfg_msi_irq_chip,
>  };
>  
> +static int ctrl_num;
> +
> +static irqreturn_t (*ls_scfg_msi_irq_handler)(int irq, void *arg);
> +
>  static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
>  {
> -	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
> +	struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(data);
> +	u32 ibs, srs;
>  
> -	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> -	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
> -	msg->data = data->hwirq << MSI_IBS_SHIFT;
> +	msg->address_hi = upper_32_bits(ctrl->msiir_addr);
> +	msg->address_lo = lower_32_bits(ctrl->msiir_addr);
> +
> +	ibs = data->hwirq - ctrl->irq_base;
> +
> +	srs = cpumask_first(irq_data_get_affinity_mask(data));
> +	if (srs >= ctrl->msi_data->cpu_num)
> +		srs = 0;
> +
> +	msg->data = ibs << ctrl->msi_data->cfg->ibs_shift | srs;
> +
> +	pr_debug("%s: ibs %d srs %d address0x%x-0x%x data 0x%x\n",
> +		 __func__, ibs, srs, msg->address_hi,
> +		 msg->address_lo, msg->data);
>  }
>  
> -static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
> -				    const struct cpumask *mask, bool force)
> +static int ls_scfg_msi_set_affinity(struct irq_data *data,
> +				const struct cpumask *mask, bool force)
>  {
> -	return -EINVAL;
> +	struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(data);
> +	u32 cpu;
> +
> +	if (!force)
> +		cpu = cpumask_any_and(mask, cpu_online_mask);
> +	else
> +		cpu = cpumask_first(mask);
> +
> +	if (cpu >= ctrl->msi_data->cpu_num)
> +		return -EINVAL;
> +
> +	if (ctrl->msir[cpu].virq <= 0) {
> +		pr_warn("cannot bind the irq to cpu%d\n", cpu);
> +		return -EINVAL;
> +	}
> +
> +	cpumask_copy(irq_data_get_affinity_mask(data), mask);
> +
> +	return IRQ_SET_MASK_OK_NOCOPY;
>  }
>  
>  static struct irq_chip ls_scfg_msi_parent_chip = {
> @@ -76,44 +179,57 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
>  					void *args)
>  {
>  	struct ls_scfg_msi *msi_data = domain->host_data;
> -	int pos, err = 0;
> +	static struct list_head *current_entry;
> +	struct ls_scfg_msi_ctrl *ctrl;
> +	int i, hwirq = -ENOMEM;
> +
> +	if (!current_entry || current_entry->next == &msi_data->ctrl_list)
> +		current_entry = &msi_data->ctrl_list;
> +
> +	list_for_each_entry(ctrl, current_entry, list) {
> +		spin_lock(&ctrl->lock);
> +		hwirq = bitmap_find_free_region(ctrl->bm,
> +						msi_data->cfg->msir_irqs,
> +						order_base_2(nr_irqs));
> +		spin_unlock(&ctrl->lock);
> +
> +		if (hwirq >= 0)
> +			break;
> +	}
>  
> -	WARN_ON(nr_irqs != 1);
> +	if (hwirq < 0)
> +		return hwirq;
>  
> -	spin_lock(&msi_data->lock);
> -	pos = find_first_zero_bit(msi_data->used, MSI_MAX_IRQS);
> -	if (pos < MSI_MAX_IRQS)
> -		__set_bit(pos, msi_data->used);
> -	else
> -		err = -ENOSPC;
> -	spin_unlock(&msi_data->lock);
> +	hwirq = hwirq + ctrl->irq_base;
>  
> -	if (err)
> -		return err;
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_info(domain, virq + i, hwirq + i,
> +				    &ls_scfg_msi_parent_chip, ctrl,
> +				    handle_simple_irq, NULL, NULL);
>  
> -	irq_domain_set_info(domain, virq, pos,
> -			    &ls_scfg_msi_parent_chip, msi_data,
> -			    handle_simple_irq, NULL, NULL);
> +	current_entry = &ctrl->list;
>  
>  	return 0;
>  }
>  
>  static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
> -				   unsigned int virq, unsigned int nr_irqs)
> +					unsigned int virq,
> +					unsigned int nr_irqs)
>  {
>  	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> -	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d);
> +	struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(d);
>  	int pos;
>  
> -	pos = d->hwirq;
> -	if (pos < 0 || pos >= MSI_MAX_IRQS) {
> -		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> +	pos = d->hwirq - ctrl->irq_base;
> +
> +	if (pos < 0 || pos >= ctrl->msi_data->cfg->msir_irqs) {
> +		pr_err("Failed to teardown msi. Invalid hwirq %d\n", pos);
>  		return;
>  	}
>  
> -	spin_lock(&msi_data->lock);
> -	__clear_bit(pos, msi_data->used);
> -	spin_unlock(&msi_data->lock);
> +	spin_lock(&ctrl->lock);
> +	bitmap_release_region(ctrl->bm, pos, order_base_2(nr_irqs));
> +	spin_unlock(&ctrl->lock);
>  }
>  
>  static const struct irq_domain_ops ls_scfg_msi_domain_ops = {
> @@ -121,29 +237,198 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
>  	.free	= ls_scfg_msi_domain_irq_free,
>  };
>  
> -static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
> +static irqreturn_t ls_scfg_msi_irqs32_handler(int irq, void *arg)
>  {
> -	struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
> +	struct ls_scfg_msir *msir = arg;
> +	struct ls_scfg_msi_ctrl *ctrl = msir->ctrl;
> +	struct ls_scfg_msi *msi_data = ctrl->msi_data;
>  	unsigned long val;
> -	int pos, virq;
> +	int pos = 0, hwirq, virq;
> +	irqreturn_t ret = IRQ_NONE;
>  
> -	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +	val = ioread32be(msir->addr);
>  
> -	val = ioread32be(msi_data->regs + MSIR);
> -	for_each_set_bit(pos, &val, MSI_MAX_IRQS) {
> -		virq = irq_find_mapping(msi_data->parent, (31 - pos));
> -		if (virq)
> +	for_each_set_bit(pos, &val, IRQS_32_PER_MSIR) {
> +		hwirq = (IRQS_32_PER_MSIR - 1 - pos) + ctrl->irq_base;
> +		virq = irq_find_mapping(msi_data->parent, hwirq);
> +		if (virq) {
>  			generic_handle_irq(virq);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
> +	return ret;
> +}

NAK. This is not an interrupt handler. This is a flow handler.

> +
> +static irqreturn_t ls_scfg_msi_irqs8_handler(int irq, void *arg)
> +{
> +	struct ls_scfg_msir *msir = arg;
> +	struct ls_scfg_msi_ctrl *ctrl = msir->ctrl;
> +	struct ls_scfg_msi *msi_data = ctrl->msi_data;
> +	unsigned long val;
> +	int pos = 0, hwirq, virq;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	val = ioread32be(msir->addr);
> +	val = (val << (msir->index * 8)) & 0xff000000;
> +
> +	for_each_set_bit(pos, &val, IRQS_32_PER_MSIR) {
> +		hwirq = (IRQS_32_PER_MSIR - 1 - pos) + ctrl->irq_base;
> +		virq = irq_find_mapping(msi_data->parent, hwirq);
> +		if (virq) {
> +			generic_handle_irq(virq);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void ls_scfg_msi_cascade(struct irq_desc *desc)
> +{
> +	struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +	chained_irq_enter(chip, desc);
> +	ls_scfg_msi_irq_handler(desc->irq_data.irq, msir);
> +	chained_irq_exit(chip, desc);
> +}

NAK.

> +
> +static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi_ctrl *ctrl,
> +				   struct device_node *node,
> +				   int index)
> +{
> +	struct ls_scfg_msir *msir = &ctrl->msir[index];
> +	int ret;
> +
> +	msir->virq = of_irq_get(node, index);
> +	if (msir->virq <= 0)
> +		return -ENODEV;
> +
> +	msir->index = index;
> +	msir->ctrl = ctrl;
> +	msir->addr = ctrl->regs + ctrl->msi_data->cfg->msir_base +
> +		     MSIR_OFFSET(msir->index);
> +
> +	if (ctrl->flag == MSI_GROUP_AFFINITY_FLAG) {
> +		ret = request_irq(msir->virq, ls_scfg_msi_irq_handler,
> +				  IRQF_NO_THREAD, "MSI-GROUP", msir);
> +		if (ret) {
> +			pr_err("failed to request irq %d\n", msir->virq);
> +			msir->virq = 0;
> +			return -ENODEV;
> +		}
> +	} else {
> +		irq_set_chained_handler(msir->virq, ls_scfg_msi_cascade);
> +		irq_set_handler_data(msir->virq, msir);
> +		irq_set_affinity(msir->virq, get_cpu_mask(index));
> +	}
> +
> +	return 0;
> +}
> +
> +static void ls_scfg_msi_ctrl_remove(struct ls_scfg_msi_ctrl *ctrl)
> +{
> +	struct ls_scfg_msir *msir;
> +	int i;
> +
> +	if (!ctrl)
> +		return;
> +
> +	if (ctrl->msir) {
> +		for (i = 0; i < ctrl->msi_data->cpu_num; i++) {
> +			msir = &ctrl->msir[i];
> +
> +			if (msir->virq <= 0)
> +				continue;
> +
> +			if (ctrl->flag == MSI_GROUP_AFFINITY_FLAG)
> +				free_irq(msir->virq, msir);
> +			else
> +				irq_set_chained_handler_and_data(msir->virq,
> +								 NULL, NULL);
> +		}
> +
> +		kfree(ctrl->msir);
>  	}
>  
> -	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +	if (ctrl->regs)
> +		iounmap(ctrl->regs);
> +
> +	kfree(ctrl->bm);
> +	kfree(ctrl);
> +}
> +
> +static int ls_scfg_msi_ctrl_probe(struct device_node *node,
> +				  struct ls_scfg_msi *msi_data)
> +{
> +	struct ls_scfg_msi_ctrl *ctrl;
> +	struct resource res;
> +	int err, irqs, i;
> +
> +	err = of_address_to_resource(node, 0, &res);
> +	if (err) {
> +		pr_warn("%s: no regs\n", node->full_name);
> +		return -ENXIO;
> +	}
> +
> +	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return  -ENOMEM;
> +
> +	ctrl->msi_data = msi_data;
> +	ctrl->msiir_addr = res.start;
> +	spin_lock_init(&ctrl->lock);
> +
> +	ctrl->regs = ioremap(res.start, resource_size(&res));
> +	if (!ctrl->regs) {
> +		pr_err("%s: unable to map registers\n", node->full_name);
> +		err = -ENOMEM;
> +		goto _err;
> +	}
> +
> +	ctrl->msir = kcalloc(msi_data->cpu_num, sizeof(struct ls_scfg_msir),
> +			     GFP_KERNEL);
> +	if (!ctrl->msir) {
> +		err = -ENOMEM;
> +		goto _err;
> +	}
> +
> +	ctrl->bm = kcalloc(BITS_TO_LONGS(msi_data->cfg->msir_irqs),
> +			   sizeof(long), GFP_KERNEL);
> +	if (!ctrl->bm) {
> +		err = -ENOMEM;
> +		goto _err;
> +	}
> +
> +	ctrl->irq_base = msi_data->cfg->msir_irqs * ctrl_num;
> +	ctrl_num++;
> +
> +	irqs = of_irq_count(node);
> +	if (irqs >= msi_data->cpu_num)
> +		ctrl->flag = MSI_AFFINITY_FLAG;
> +	else
> +		ctrl->flag = MSI_GROUP_AFFINITY_FLAG;
> +
> +	for (i = 0; i < msi_data->cpu_num; i++)
> +		ls_scfg_msi_setup_hwirq(ctrl, node, i);
> +
> +	list_add_tail(&ctrl->list, &msi_data->ctrl_list);
> +
> +	return 0;
> +
> +_err:
> +	ls_scfg_msi_ctrl_remove(ctrl);
> +	pr_err("MSI: failed probing %s (%d)\n", node->full_name, err);
> +	return err;
>  }
>  
>  static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
>  {
>  	/* Initialize MSI domain parent */
>  	msi_data->parent = irq_domain_add_linear(NULL,
> -						 MSI_MAX_IRQS,
> +						 msi_data->cfg->msir_irqs *
> +						 ctrl_num,
>  						 &ls_scfg_msi_domain_ops,
>  						 msi_data);
>  	if (!msi_data->parent) {
> @@ -167,51 +452,57 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
>  static int ls_scfg_msi_probe(struct platform_device *pdev)
>  {
>  	struct ls_scfg_msi *msi_data;
> -	struct resource *res;
> -	int ret;
> +	const struct soc_device_attribute *match;
> +	struct device_node *child;
>  
>  	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
>  	if (!msi_data)
>  		return -ENOMEM;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(msi_data->regs)) {
> -		dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> -		return PTR_ERR(msi_data->regs);
> -	}
> -	msi_data->msiir_addr = res->start;
> -
> -	msi_data->irq = platform_get_irq(pdev, 0);
> -	if (msi_data->irq <= 0) {
> -		dev_err(&pdev->dev, "failed to get MSI irq\n");
> -		return -ENODEV;
> -	}
> +	INIT_LIST_HEAD(&msi_data->ctrl_list);
>  
>  	msi_data->pdev = pdev;
> -	spin_lock_init(&msi_data->lock);
> +	msi_data->cpu_num = num_possible_cpus();
> +
> +	match = soc_device_match(soc_msi_matches);
> +	if (match)
> +		msi_data->cfg = match->data;
> +	else
> +		msi_data->cfg = &ls1046_msi_cfg;
> +
> +	if (msi_data->cfg->msir_irqs == IRQS_8_PER_MSIR)
> +		ls_scfg_msi_irq_handler = ls_scfg_msi_irqs8_handler;
> +	else
> +		ls_scfg_msi_irq_handler = ls_scfg_msi_irqs32_handler;
>  
> -	ret = ls_scfg_msi_domains_init(msi_data);
> -	if (ret)
> -		return ret;
> +	for_each_child_of_node(msi_data->pdev->dev.of_node, child)
> +		ls_scfg_msi_ctrl_probe(child, msi_data);
>  
> -	irq_set_chained_handler_and_data(msi_data->irq,
> -					 ls_scfg_msi_irq_handler,
> -					 msi_data);
> +	ls_scfg_msi_domains_init(msi_data);
>  
>  	platform_set_drvdata(pdev, msi_data);
>  
> +	dev_info(&pdev->dev, "irqs:%dx%d ibs_shift:%d msir_base:0x%x\n",
> +		 msi_data->cfg->msir_irqs, ctrl_num,
> +		 msi_data->cfg->ibs_shift, msi_data->cfg->msir_base);
> +
>  	return 0;
>  }
>  
>  static int ls_scfg_msi_remove(struct platform_device *pdev)
>  {
>  	struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
> +	struct ls_scfg_msi_ctrl *ctrl, *temp;
>  
> -	irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
> +	list_for_each_entry_safe(ctrl, temp, &msi_data->ctrl_list, list) {
> +		list_move_tail(&ctrl->list, &msi_data->ctrl_list);
> +		ls_scfg_msi_ctrl_remove(ctrl);
> +	}
>  
> -	irq_domain_remove(msi_data->msi_domain);
> -	irq_domain_remove(msi_data->parent);
> +	if (msi_data->msi_domain)
> +		irq_domain_remove(msi_data->msi_domain);
> +	if (msi_data->parent)
> +		irq_domain_remove(msi_data->parent);
>  
>  	platform_set_drvdata(pdev, NULL);
>  
> @@ -219,8 +510,7 @@ static int ls_scfg_msi_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id ls_scfg_msi_id[] = {
> -	{ .compatible = "fsl,1s1021a-msi", },
> -	{ .compatible = "fsl,1s1043a-msi", },
> +	{ .compatible = "fsl,ls-scfg-msi" },

And now you've broken all the previous DTs that previously existed.
What's the plan?

>  	{},
>  };
>  
> 

Overall, I hate this patch. It is broken from an irqchip PoV, it creates
a new range of board files, just even less maintainable, it breaks
compatibility with previous DTs.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...