dt/bindings: Add bindings for Layerscape SCFG MSI

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 <[email protected]>

---

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

drivers/irqchip/Kconfig +5 -0

index fb50911..0f2a3c3 100644
@@ -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

drivers/irqchip/Makefile +1 -0

index 18caacb..37e12de 100644
@@ -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

drivers/irqchip/irq-ls-scfg-msi.c new file +36 -0

new file mode 100644
index 0000000..f57a72c
@@ -0,0 +1,244 @@
+/*
+ * NXP SCFG MSI(-X) support
+ *
+ * Copyright (C) 2016 NXP Semiconductor.
+ *
+ * Author: Minghuan Lian <[email protected]>
+ *
+ * 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;

drivers/irqchip/irq-ls-scfg-msi.c new file +15 -0

+
+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;

drivers/irqchip/irq-ls-scfg-msi.c new file +54 -0

+	.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,
+};

drivers/irqchip/irq-ls-scfg-msi.c new file +50 -0

+	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);
+	}

drivers/irqchip/irq-ls-scfg-msi.c new file +39 -0

+	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,

-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier at arm.com]
Sent: Thursday, February 18, 2016 1:30 AM
To: Minghuan Lian <[email protected]>;
linux-arm-kernel at lists.infradead.org
Cc: Thomas Gleixner <[email protected]>; Jason Cooper <[email protected]>; Roy Zang <[email protected]>; Mingkai Hu <[email protected]>; Stuart Yoder <[email protected]>; Yang-Leo Li <[email protected]>; linux-kernel at 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.

Quoted 91 lines · Minghuan Lian

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 <[email protected]>

---

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

drivers/irqchip/Kconfig index renamed from drivers/irqchip/Kconfig +0 -0

fb50911..0f2a3c3 100644

drivers/irqchip/Kconfig +5 -0

@@ -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

drivers/irqchip/Makefile index renamed from drivers/irqchip/Makefile +0 -0

18caacb..37e12de 100644

drivers/irqchip/Makefile +1 -0

@@ -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

drivers/irqchip/irq-ls-scfg-msi.c new file +39 -0

new file mode 100644
index 0000000..f57a72c
@@ -0,0 +1,244 @@
+/*
+ * NXP SCFG MSI(-X) support
+ *
+ * Copyright (C) 2016 NXP Semiconductor.
+ *
+ * Author: Minghuan Lian <[email protected]>
+ *
+ * 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 = {

drivers/irqchip/irq-ls-scfg-msi.c line 46

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -0

+	.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.

Quoted 18 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +17 -0

+
+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,

drivers/irqchip/irq-ls-scfg-msi.c line 71

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -0

+				    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.

Quoted 56 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +56 -0

+	.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);

drivers/irqchip/irq-ls-scfg-msi.c line 135

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -0

+	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?

Quoted 52 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +51 -0

+	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) {

drivers/irqchip/irq-ls-scfg-msi.c line 195

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -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?

Quoted 40 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +38 -0

+	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,

drivers/irqchip/irq-ls-scfg-msi.c line 244

Quoted 10 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +10 -0

+	},
+	.probe = ls_scfg_msi_probe,
+	.remove = ls_scfg_msi_remove,
+};
+
+module_platform_driver(ls_scfg_msi_driver);
+
+MODULE_AUTHOR("Minghuan Lian <[email protected]>");
+MODULE_DESCRIPTION("NXP Layerscape SCFG MSI controller driver");
+MODULE_LICENSE("GPL v2");

Thanks,

	M.

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

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

Quoted 19 lines

Quoted 19 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +19 -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) {

Quoted 8 lines

drivers/irqchip/irq-ls-scfg-msi.c line 195

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -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

-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier at arm.com]
Sent: Monday, February 29, 2016 6:14 PM
To: Minghuan Lian <[email protected]>;
linux-arm-kernel at lists.infradead.org
Cc: Thomas Gleixner <[email protected]>; Jason Cooper <[email protected]>; Roy Zang <[email protected]>; Mingkai Hu <[email protected]>; Stuart Yoder <[email protected]>; Yang-Leo Li <[email protected]>; linux-kernel at vger.kernel.org
Subject: Re: [PATCH 2/2 v3] irqchip/Layerscape: Add SCFG MSI controller support

Quoted 9 lines · 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

[...]

Quoted 6 lines

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -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);

Quoted 21 lines

Quoted 13 lines

Quoted 13 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +13 -0

+	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) {

Quoted 8 lines

drivers/irqchip/irq-ls-scfg-msi.c line 195

Quoted 6 lines

drivers/irqchip/irq-ls-scfg-msi.c new file +6 -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?

Quoted 4 lines

[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...

Some Layerscape SoCs use a simple MSI controller implementation. It contains only two SCFG register to trigger and describe a group 32 MSI interrupts. The patch adds bindings to describe the controller.

Signed-off-by Minghuan Lian <[email protected]>

---
 .../interrupt-controller/fsl,ls-scfg-msi.txt       | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt

Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt new file +1 -0

new file mode 100644
index 0000000..0c41151
@@ -0,0 +1,23 @@
+* Freescale Layerscape SCFG PCIe MSI controller