Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

[Marc Zyngier]

Minguan,

Please CC LKML for any irqchip related patch - see the MAINTAINER file.

On 02/02/16 09:00, Minghuan Lian wrote:
> Some kind of NXP Layerscape SoC provides a MSI
> implementation which uses two SCFG registers MSIIR and
> MSIR to support 32 MSI interrupts for each PCIe controller.
> The patch is to support it.
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
> 
> Change log
> v3:
> 1. call of_node_to_fwnode()
> v2:
> 1. rename ls1-msi to ls-scfg-msi
> 2. remove reg-names MSIIR MSIR 
> 3. remove calling set_irq_flags()
> 
>  drivers/irqchip/Kconfig           |   5 +
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-ls-scfg-msi.c | 244 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 250 insertions(+)
>  create mode 100644 drivers/irqchip/irq-ls-scfg-msi.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index fb50911..0f2a3c3 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -218,3 +218,8 @@ config IRQ_MXS
>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
> +
> +config LS_SCFG_MSI
> +	def_bool y if SOC_LS1021A || ARCH_LAYERSCAPE
> +	depends on PCI && PCI_MSI
> +	select PCI_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 18caacb..37e12de 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
> +obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
> new file mode 100644
> index 0000000..f57a72c
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> @@ -0,0 +1,244 @@
> +/*
> + * NXP SCFG MSI(-X) support
> + *
> + * Copyright (C) 2016 NXP Semiconductor.
> + *
> + * Author: Minghuan Lian <Minghuan.Lian@nxp.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define MSI_MAX_IRQS	32
> +#define MSI_IBS_SHIFT	3
> +#define MSIR		4
> +
> +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;
> +	u32			nr_irqs;
> +	int			irq;
> +	DECLARE_BITMAP(used, MSI_MAX_IRQS);
> +};
> +
> +static struct irq_chip ls_scfg_msi_irq_chip = {
> +	.name = "MSI",
> +	.irq_enable	= pci_msi_unmask_irq,
> +	.irq_disable	= pci_msi_mask_irq,
> +	.irq_mask	= pci_msi_mask_irq,
> +	.irq_unmask	= pci_msi_unmask_irq,
> +};

You don't need all of this. mask/unmask are enough.

> +
> +static struct msi_domain_info ls_scfg_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
> +		   MSI_FLAG_USE_DEF_CHIP_OPS |
> +		   MSI_FLAG_PCI_MSIX),
> +	.chip	= &ls_scfg_msi_irq_chip,
> +};
> +
> +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);
> +
> +	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;
> +}
> +
> +static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
> +				    const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static struct irq_chip ls_scfg_msi_parent_chip = {
> +	.name			= "LS SCFG MSI",

Worth making this shorter. "SCFG" should be enough.

> +	.irq_compose_msi_msg	= ls_scfg_msi_compose_msg,
> +	.irq_set_affinity	= ls_scfg_msi_set_affinity,
> +};
> +
> +static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
> +					unsigned int virq,
> +					unsigned int nr_irqs,
> +					void *args)
> +{
> +	struct ls_scfg_msi *msi_data = domain->host_data;
> +	int pos, err = 0;
> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	spin_lock(&msi_data->lock);
> +	pos = find_first_zero_bit(msi_data->used, msi_data->nr_irqs);
> +	if (pos < msi_data->nr_irqs)
> +		__set_bit(pos, msi_data->used);
> +	else
> +		err = -ENOSPC;
> +	spin_unlock(&msi_data->lock);
> +
> +	if (err)
> +		return err;
> +
> +	irq_domain_set_info(domain, virq, pos,
> +			    &ls_scfg_msi_parent_chip, msi_data,
> +			    handle_simple_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +static void ls_scfg_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 ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d);
> +	int pos;
> +
> +	pos = d->hwirq;
> +	if (pos < 0 || pos >= msi_data->nr_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);
> +}
> +
> +static const struct irq_domain_ops ls_scfg_msi_domain_ops = {
> +	.alloc	= ls_scfg_msi_domain_irq_alloc,
> +	.free	= ls_scfg_msi_domain_irq_free,
> +};
> +
> +static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
> +{
> +	struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
> +	unsigned long val;
> +	int pos, virq;
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> +	val = ioread32be(msi_data->regs + MSIR);

Is the device guaranteed to always be BE?

> +	for_each_set_bit(pos, &val, msi_data->nr_irqs) {
> +		virq = irq_find_mapping(msi_data->parent, (31 - pos));
> +		if (virq)
> +			generic_handle_irq(virq);
> +	}
> +
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);
> +}
> +
> +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_data->nr_irqs,
> +						 &ls_scfg_msi_domain_ops,
> +						 msi_data);
> +	if (!msi_data->parent) {
> +		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	msi_data->msi_domain = pci_msi_create_irq_domain(
> +				of_node_to_fwnode(msi_data->pdev->dev.of_node),
> +				&ls_scfg_msi_domain_info,
> +				msi_data->parent);
> +	if (!msi_data->msi_domain) {
> +		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> +		irq_domain_remove(msi_data->parent);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ls_scfg_msi_probe(struct platform_device *pdev)
> +{
> +	struct ls_scfg_msi *msi_data;
> +	struct resource *res;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	msi_data->pdev = pdev;
> +	msi_data->nr_irqs = MSI_MAX_IRQS;

So this is hardcoded, always. Why do you need a nr_irqs variable at all?

> +	spin_lock_init(&msi_data->lock);
> +
> +	ret = ls_scfg_msi_domains_init(msi_data);
> +	if (ret)
> +		return ret;
> +
> +	irq_set_chained_handler_and_data(msi_data->irq,
> +					 ls_scfg_msi_irq_handler,
> +					 msi_data);
> +
> +	platform_set_drvdata(pdev, msi_data);
> +
> +	return 0;
> +}
> +
> +static int ls_scfg_msi_remove(struct platform_device *pdev)
> +{
> +	struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
> +
> +	irq_domain_remove(msi_data->msi_domain);
> +	irq_domain_remove(msi_data->parent);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ls_scfg_msi_id[] = {
> +	{ .compatible = "fsl,1s1021a-msi", },
> +	{ .compatible = "fsl,1s1043a-msi", },
> +	{},
> +};
> +
> +static struct platform_driver ls_scfg_msi_driver = {
> +	.driver = {
> +		.name = "ls-scfg-msi",
> +		.of_match_table = ls_scfg_msi_id,
> +	},
> +	.probe = ls_scfg_msi_probe,
> +	.remove = ls_scfg_msi_remove,
> +};
> +
> +module_platform_driver(ls_scfg_msi_driver);
> +
> +MODULE_AUTHOR("Minghuan Lian <Minghuan.Lian@nxp.com>");
> +MODULE_DESCRIPTION("NXP Layerscape SCFG MSI controller driver");
> +MODULE_LICENSE("GPL v2");
> 

Thanks,

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

RE: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

[Minghuan Lian]

Hi Marc,

I am sorry for the delayed response due to the Chinese Spring Festival holiday.
Thank you very much for the review.
Please see my comments inline.

Thanks,
Minghuan

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Thursday, February 18, 2016 1:30 AM
> To: Minghuan Lian <minghuan.lian@nxp.com>;
> linux-arm-kernel@lists.infradead.org
> Cc: Thomas Gleixner <tglx@linutronix.de>; 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>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller
> support
> 
> Minguan,
> 
> Please CC LKML for any irqchip related patch - see the MAINTAINER file.
> 
> On 02/02/16 09:00, Minghuan Lian wrote:
> > Some kind of NXP Layerscape SoC provides a MSI implementation which
> > uses two SCFG registers MSIIR and MSIR to support 32 MSI interrupts
> > for each PCIe controller.
> > The patch is to support it.
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> > ---
> >
> > Change log
> > v3:
> > 1. call of_node_to_fwnode()
> > v2:
> > 1. rename ls1-msi to ls-scfg-msi
> > 2. remove reg-names MSIIR MSIR
> > 3. remove calling set_irq_flags()
> >
> >  drivers/irqchip/Kconfig           |   5 +
> >  drivers/irqchip/Makefile          |   1 +
> >  drivers/irqchip/irq-ls-scfg-msi.c | 244
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 250 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-ls-scfg-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > fb50911..0f2a3c3 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -218,3 +218,8 @@ config IRQ_MXS
> >  	def_bool y if MACH_ASM9260 || ARCH_MXS
> >  	select IRQ_DOMAIN
> >  	select STMP_DEVICE
> > +
> > +config LS_SCFG_MSI
> > +	def_bool y if SOC_LS1021A || ARCH_LAYERSCAPE
> > +	depends on PCI && PCI_MSI
> > +	select PCI_MSI_IRQ_DOMAIN
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > 18caacb..37e12de 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -59,3 +59,4 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
> >  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
> >  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
> >  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
> > +obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> > diff --git a/drivers/irqchip/irq-ls-scfg-msi.c
> > b/drivers/irqchip/irq-ls-scfg-msi.c
> > new file mode 100644
> > index 0000000..f57a72c
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> > @@ -0,0 +1,244 @@
> > +/*
> > + * NXP SCFG MSI(-X) support
> > + *
> > + * Copyright (C) 2016 NXP Semiconductor.
> > + *
> > + * Author: Minghuan Lian <Minghuan.Lian@nxp.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > +#include <linux/of_pci.h> #include <linux/of_platform.h> #include
> > +<linux/spinlock.h>
> > +
> > +#define MSI_MAX_IRQS	32
> > +#define MSI_IBS_SHIFT	3
> > +#define MSIR		4
> > +
> > +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;
> > +	u32			nr_irqs;
> > +	int			irq;
> > +	DECLARE_BITMAP(used, MSI_MAX_IRQS);
> > +};
> > +
> > +static struct irq_chip ls_scfg_msi_irq_chip = {
> > +	.name = "MSI",
> > +	.irq_enable	= pci_msi_unmask_irq,
> > +	.irq_disable	= pci_msi_mask_irq,
> > +	.irq_mask	= pci_msi_mask_irq,
> > +	.irq_unmask	= pci_msi_unmask_irq,
> > +};
> 
> You don't need all of this. mask/unmask are enough.
[Lian Minghuan-B31939] ok. I will fix it.
> 
> > +
> > +static struct msi_domain_info ls_scfg_msi_domain_info = {
> > +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS |
> > +		   MSI_FLAG_USE_DEF_CHIP_OPS |
> > +		   MSI_FLAG_PCI_MSIX),
> > +	.chip	= &ls_scfg_msi_irq_chip,
> > +};
> > +
> > +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);
> > +
> > +	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; }
> > +
> > +static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
> > +				    const struct cpumask *mask, bool force) {
> > +	return -EINVAL;
> > +}
> > +
> > +static struct irq_chip ls_scfg_msi_parent_chip = {
> > +	.name			= "LS SCFG MSI",
> 
> Worth making this shorter. "SCFG" should be enough.
> 
[Lian Minghuan-B31939] ok. I will rename it to "scfg"

> > +	.irq_compose_msi_msg	= ls_scfg_msi_compose_msg,
> > +	.irq_set_affinity	= ls_scfg_msi_set_affinity,
> > +};
> > +
> > +static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
> > +					unsigned int virq,
> > +					unsigned int nr_irqs,
> > +					void *args)
> > +{
> > +	struct ls_scfg_msi *msi_data = domain->host_data;
> > +	int pos, err = 0;
> > +
> > +	WARN_ON(nr_irqs != 1);
> > +
> > +	spin_lock(&msi_data->lock);
> > +	pos = find_first_zero_bit(msi_data->used, msi_data->nr_irqs);
> > +	if (pos < msi_data->nr_irqs)
> > +		__set_bit(pos, msi_data->used);
> > +	else
> > +		err = -ENOSPC;
> > +	spin_unlock(&msi_data->lock);
> > +
> > +	if (err)
> > +		return err;
> > +
> > +	irq_domain_set_info(domain, virq, pos,
> > +			    &ls_scfg_msi_parent_chip, msi_data,
> > +			    handle_simple_irq, NULL, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static void ls_scfg_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 ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d);
> > +	int pos;
> > +
> > +	pos = d->hwirq;
> > +	if (pos < 0 || pos >= msi_data->nr_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);
> > +}
> > +
> > +static const struct irq_domain_ops ls_scfg_msi_domain_ops = {
> > +	.alloc	= ls_scfg_msi_domain_irq_alloc,
> > +	.free	= ls_scfg_msi_domain_irq_free,
> > +};
> > +
> > +static void ls_scfg_msi_irq_handler(struct irq_desc *desc) {
> > +	struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
> > +	unsigned long val;
> > +	int pos, virq;
> > +
> > +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> > +
> > +	val = ioread32be(msi_data->regs + MSIR);
> 
> Is the device guaranteed to always be BE?
[Lian Minghuan-B31939] Yes, this register on all kinds of current Layerscape SoC is big-endian format.  
> 
> > +	for_each_set_bit(pos, &val, msi_data->nr_irqs) {
> > +		virq = irq_find_mapping(msi_data->parent, (31 - pos));
> > +		if (virq)
> > +			generic_handle_irq(virq);
> > +	}
> > +
> > +	chained_irq_exit(irq_desc_get_chip(desc), desc); }
> > +
> > +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_data->nr_irqs,
> > +						 &ls_scfg_msi_domain_ops,
> > +						 msi_data);
> > +	if (!msi_data->parent) {
> > +		dev_err(&msi_data->pdev->dev, "failed to create IRQ domain\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	msi_data->msi_domain = pci_msi_create_irq_domain(
> > +				of_node_to_fwnode(msi_data->pdev->dev.of_node),
> > +				&ls_scfg_msi_domain_info,
> > +				msi_data->parent);
> > +	if (!msi_data->msi_domain) {
> > +		dev_err(&msi_data->pdev->dev, "failed to create MSI domain\n");
> > +		irq_domain_remove(msi_data->parent);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ls_scfg_msi_probe(struct platform_device *pdev) {
> > +	struct ls_scfg_msi *msi_data;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	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;
> > +	}
> > +
> > +	msi_data->pdev = pdev;
> > +	msi_data->nr_irqs = MSI_MAX_IRQS;
> 
> So this is hardcoded, always. Why do you need a nr_irqs variable at all?
[Lian Minghuan-B31939] Currently, nr_irqs is always 32, but in the future, the MSI controller may be extended to support more IRQs.
And, we may set nr_irqs the value of less than 32 to reserve some IRQs for special usage. So nr_irqs can bring flexibility
> 
> > +	spin_lock_init(&msi_data->lock);
> > +
> > +	ret = ls_scfg_msi_domains_init(msi_data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	irq_set_chained_handler_and_data(msi_data->irq,
> > +					 ls_scfg_msi_irq_handler,
> > +					 msi_data);
> > +
> > +	platform_set_drvdata(pdev, msi_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ls_scfg_msi_remove(struct platform_device *pdev) {
> > +	struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
> > +
> > +	irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
> > +
> > +	irq_domain_remove(msi_data->msi_domain);
> > +	irq_domain_remove(msi_data->parent);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ls_scfg_msi_id[] = {
> > +	{ .compatible = "fsl,1s1021a-msi", },
> > +	{ .compatible = "fsl,1s1043a-msi", },
> > +	{},
> > +};
> > +
> > +static struct platform_driver ls_scfg_msi_driver = {
> > +	.driver = {
> > +		.name = "ls-scfg-msi",
> > +		.of_match_table = ls_scfg_msi_id,
> > +	},
> > +	.probe = ls_scfg_msi_probe,
> > +	.remove = ls_scfg_msi_remove,
> > +};
> > +
> > +module_platform_driver(ls_scfg_msi_driver);
> > +
> > +MODULE_AUTHOR("Minghuan Lian <Minghuan.Lian@nxp.com>");
> > +MODULE_DESCRIPTION("NXP Layerscape SCFG MSI controller driver");
> > +MODULE_LICENSE("GPL v2");
> >
> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

[Marc Zyngier]

On 25/02/16 03:21, Minghuan Lian wrote:
> Hi Marc,
> 
> I am sorry for the delayed response due to the Chinese Spring Festival holiday.
> Thank you very much for the review.
> Please see my comments inline.
> 
> Thanks,
> Minghuan
> 

[...]

>>> +static int ls_scfg_msi_probe(struct platform_device *pdev) {
>>> +	struct ls_scfg_msi *msi_data;
>>> +	struct resource *res;
>>> +	int ret;
>>> +
>>> +	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;
>>> +	}
>>> +
>>> +	msi_data->pdev = pdev;
>>> +	msi_data->nr_irqs = MSI_MAX_IRQS;
>>
>> So this is hardcoded, always. Why do you need a nr_irqs variable at all?
> [Lian Minghuan-B31939] Currently, nr_irqs is always 32, but in the
> future, the MSI controller may be extended to support more IRQs. And,
> we may set nr_irqs the value of less than 32 to reserve some IRQs for
> special usage. So nr_irqs can bring flexibility

You have to choose: either this is configurable and you describe it in
DT, or this is not and you drop this field from the structure.

As for the "reserved interrupts", that would need to be described too.

Thanks,

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

RE: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

[Minghuan Lian]

Hi Marc,

Please see my comments inline.

Thanks,
Minghaun

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Monday, February 29, 2016 6:14 PM
> To: Minghuan Lian <minghuan.lian@nxp.com>;
> linux-arm-kernel@lists.infradead.org
> Cc: Thomas Gleixner <tglx@linutronix.de>; 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>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller
> support
> 
> On 25/02/16 03:21, Minghuan Lian wrote:
> > Hi Marc,
> >
> > I am sorry for the delayed response due to the Chinese Spring Festival holiday.
> > Thank you very much for the review.
> > Please see my comments inline.
> >
> > Thanks,
> > Minghuan
> >
> 
> [...]
> 
> >>> +static int ls_scfg_msi_probe(struct platform_device *pdev) {
> >>> +	struct ls_scfg_msi *msi_data;
> >>> +	struct resource *res;
> >>> +	int ret;
> >>> +
> >>> +	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;
> >>> +	}
> >>> +
> >>> +	msi_data->pdev = pdev;
> >>> +	msi_data->nr_irqs = MSI_MAX_IRQS;
> >>
> >> So this is hardcoded, always. Why do you need a nr_irqs variable at all?
> > [Lian Minghuan-B31939] Currently, nr_irqs is always 32, but in the
> > future, the MSI controller may be extended to support more IRQs. And,
> > we may set nr_irqs the value of less than 32 to reserve some IRQs for
> > special usage. So nr_irqs can bring flexibility
> 
> You have to choose: either this is configurable and you describe it in
> DT, or this is not and you drop this field from the structure.
> 
> As for the "reserved interrupts", that would need to be described too.
> 
[Minghuan Lian] I will drop this field in next version.
The old version of LS1021a only supports one MSI interrupt. So the driver needs to change nr_irq to 1.
Anyway, this issue has been fixed. Now, all the Layerscape SoC supports 32 MSI interrupts.

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...