[PATCH v9 0/4] irqchip:support mbigen interrupt controller

[PATCH v9 0/4] irqchip:support mbigen interrupt controller

[MaJun]

From: Ma Jun <majun258@huawei.com>

This patch set adds the driver of mbigen and binding document for Hisilicon
Mbigen chips.

Compared with previous version, this version changed much.

Because during the time between V3 and V4 of my patch, there are two
related patches were committed by Mr.Marc Zyngier and Mr. Mark Rutland.

First, Mr. Marc Zyngier changed MSI frame and added supporting for
platform MSI.

https://lkml.org/lkml/2015/7/28/552

Second, Mr.Mark Rutland changed Generic PCI MSI + IOMMU topology bindings

https://lkml.org/lkml/2015/7/23/558 

After V5 patch posted, Mr.Marc Zyngier posted a new patch set
"Adding core support for wire-MSI bridges"

https://lkml.org/lkml/2015/10/15/545

So, mbigen v6 patch is based on this new patch even though this patch is
still under review.

Changes in v9:
--- Fixed typo problem (dts)
--- Removed superfluous data structures
--- Changed clear register offset
Changes in v8:
--- Fixed some tiny bugs.
Changes in v7:
--- Fixed the build test error when applied patch v6 3/4
Changes in v6:
--- Re-based mbigen driver on kernel 4.3.0-rc5 and Marc's new patch
--- Change the mbigen chip node definition(dts).
--- Change the interrupt cells definition(dts).
Changes in v5:
--- Split mbigen driver patch into 2 smaller patches.
--- Change mbigen chip and mbigen device initialzing sequence.
--- Initializing mbigen device instead of mbigen chip as interrupt controller
--- Remove mbigen node from driver to make this driver more easily read.
--- Change the mbigen chip node definition(dts).
--- Change the interrupt cells definition(dts).
Changes in v4:
--- Re-based mbigen driver on kernel 4.2.0-rc2 and Marc's patch
--- Changed the binding document based on Mark's patch.

Ma Jun (4):
  dt-binding:Documents of the mbigen bindings
  irqchip: add platform device driver for mbigen device
  irqchip:create irq domain for each mbigen device
  irqchip:implement the mbigen irq chip operation functions

 Documentation/devicetree/bindings/arm/mbigen.txt |   69 ++++++
 drivers/irqchip/Kconfig                          |    8 +
 drivers/irqchip/Makefile                         |    1 +
 drivers/irqchip/irq-mbigen.c                     |  281 ++++++++++++++++++++++
 4 files changed, 359 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt
 create mode 100644 drivers/irqchip/irq-mbigen.c

[PATCH v9 1/4] dt-binding:Documents of the mbigen bindings

[MaJun]

From: Ma Jun <majun258@huawei.com>

Add the mbigen msi interrupt controller bindings document.

This patch based on Mark Rutland's patch
https://lkml.org/lkml/2015/7/23/558

Signed-off-by: Ma Jun <majun258@huawei.com>
---
 Documentation/devicetree/bindings/arm/mbigen.txt |   69 ++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt

diff --git a/Documentation/devicetree/bindings/arm/mbigen.txt b/Documentation/devicetree/bindings/arm/mbigen.txt
new file mode 100644
index 0000000..8ae59a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mbigen.txt
@@ -0,0 +1,69 @@
+Hisilicon mbigen device tree bindings.
+=======================================
+
+Mbigen means: message based interrupt generator.
+
+MBI is kind of msi interrupt only used on Non-PCI devices.
+
+To reduce the wired interrupt number connected to GIC,
+Hisilicon designed mbigen to collect and generate interrupt.
+
+
+Non-pci devices can connect to mbigen and generate the
+interrupt by writing ITS register.
+
+The mbigen chip and devices connect to mbigen have the following properties:
+
+Mbigen main node required properties:
+-------------------------------------------
+- compatible: Should be "hisilicon,mbigen-v2"
+- reg: Specifies the base physical address and size of the Mbigen
+  registers.
+- interrupt controller: Identifies the node as an interrupt controller
+- msi-parent: This property has two cells.
+	The 1st cell specifies the ITS this device connected.
+	The 2nd cell specifies the device id.
+- num-msis:Specifies the total number of interrupt this device has.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value must be 2.
+
+  The 1st cell is global hardware pin number of the interrupt.
+	This value depends on the Soc design.
+
+  The 2nd cell is the interrupt trigger type.
+	The value of this cell should be:
+	1: rising edge triggered
+	or
+	4: high level triggered	
+
+Examples:
+
+ 	mbigen_device_gmac:intc {
+			compatible = "hisilicon,mbigen-v2";
+			reg = <0x0 0xc0080000 0x0 0x10000>;
+			interrupt-controller;
+			msi-parent = <&its_dsa 0x40b1c>;
+			num-msis = <9>;
+			#interrupt-cells = <2>;
+ 	};
+
+Devices connect to mbigen required properties:
+----------------------------------------------------
+-interrupt-parent: Specifies the mbigen device node which device connected.
+-interrupts:specifies the interrupt source.
+  The 1st cell is global hardware pin number of the interrupt.
+		This value depends on the Soc design.
+  The 2nd cell is the interrupt trigger type(rising edge triggered or high
+		level triggered)
+
+Examples:
+	gmac0: ethernet@c2080000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0 0xc2080000 0 0x20000>,
+		      <0 0xc0000000 0 0x1000>;
+		interrupt-parent  = <&mbigen_device_gmac>;
+		interrupts =	<656 1>,
+				<657 1>;
+	};
+
-- 
1.7.1

[PATCH v9 2/4] irqchip: add platform device driver for mbigen device

[MaJun]

From: Ma Jun <majun258@huawei.com>

Mbigen means Message Based Interrupt Generator(MBIGEN).

Its a kind of interrupt controller that collects
the interrupts from external devices and generate msi interrupt.
Mbigen is applied to reduce the number of wire connected interrupts.

As the peripherals increasing, the interrupts lines needed is
increasing much, especially on the Arm64 server SOC.

Therefore, the interrupt pin in GIC is not enough to cover so
many peripherals.

Mbigen is designed to fix this problem.

Mbigen chip locates in ITS or outside of ITS.

Mbigen chip hardware structure shows as below:

		mbigen chip
|---------------------|-------------------|
mgn_node0	  mgn_node1		mgn_node2
 |		 |-------|		|-------|------|
dev1		dev1    dev2		dev1   dev3   dev4

Each mbigen chip contains several mbigen nodes.

External devices can connect to mbigen node through wire connecting way.

Because a mbigen node only can support 128 interrupt maximum, depends
on the interrupt lines number of devices, a device can connects to one
more mbigen nodes.

Also, several different devices can connect to a same mbigen node.

When devices triggered interrupt,mbigen chip detects and collects
the interrupts and generates the MBI interrupts by writing the ITS
Translator register.

To simplify mbigen driver,I used a new conception--mbigen device.
Each mbigen device is initialized as a platform device.

Mbigen device presents the parts(register, pin definition etc.) in
mbigen chip corresponding to a peripheral device.

So from software view, the structure likes below

	            mbigen chip
     |---------------------|-----------------|
mbigen device1       mbigen device2  mbigen device3
      |                   |                |
     dev1                dev2             dev3
Signed-off-by: Ma Jun <majun258@huawei.com>
---
 drivers/irqchip/Kconfig      |    8 ++++
 drivers/irqchip/Makefile     |    1 +
 drivers/irqchip/irq-mbigen.c |   78 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 0 deletions(-)
 create mode 100644 drivers/irqchip/irq-mbigen.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e..b205e15 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
 	bool
 	select PCI_MSI_IRQ_DOMAIN
 
+config HISILICON_IRQ_MBIGEN
+	bool "Support mbigen interrupt controller"
+	default n
+	depends on ARM_GIC_V3 && ARM_GIC_V3_ITS && GENERIC_MSI_IRQ_DOMAIN
+	help
+	 Enable the mbigen interrupt controller used on
+	 Hisilicon platform.
+
 config ARM_NVIC
 	bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f..cd76b11 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
 obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
+obj-$(CONFIG_HISILICON_IRQ_MBIGEN)	+= irq-mbigen.o
 obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
 obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
 obj-$(CONFIG_ATMEL_AIC_IRQ)		+= irq-atmel-aic-common.o irq-atmel-aic.o
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
new file mode 100644
index 0000000..9f036c2
--- /dev/null
+++ b/drivers/irqchip/irq-mbigen.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2015 Hisilicon Limited, All Rights Reserved.
+ * Author: Jun Ma <majun258@huawei.com>
+ * Author: Yun Wu <wuyun.wu@huawei.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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/**
+ * struct mbigen_device - holds the information of mbigen device.
+ *
+ * @pdev:		pointer to the platform device structure of mbigen chip.
+ * @base:		mapped address of this mbigen chip.
+ */
+struct mbigen_device {
+	struct platform_device	*pdev;
+	void __iomem		*base;
+};
+
+static int mbigen_device_probe(struct platform_device *pdev)
+{
+	struct mbigen_device *mgn_chip;
+	struct resource *res;
+
+	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
+	if (!mgn_chip)
+		return -ENOMEM;
+
+	mgn_chip->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mgn_chip->base))
+		return PTR_ERR(mgn_chip->base);
+
+	platform_set_drvdata(pdev, mgn_chip);
+
+	return 0;
+}
+
+static const struct of_device_id mbigen_of_match[] = {
+	{ .compatible = "hisilicon,mbigen-v2" },
+	{ /* END */ }
+};
+MODULE_DEVICE_TABLE(of, mbigen_of_match);
+
+static struct platform_driver mbigen_platform_driver = {
+	.driver = {
+		.name		= "Hisilicon MBIGEN-V2",
+		.owner		= THIS_MODULE,
+		.of_match_table	= mbigen_of_match,
+	},
+	.probe			= mbigen_device_probe,
+};
+
+module_platform_driver(mbigen_platform_driver);
+
+MODULE_AUTHOR("Jun Ma <majun258@huawei.com>");
+MODULE_AUTHOR("Yun Wu <wuyun.wu@huawei.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Hisilicon MBI Generator driver");
-- 
1.7.1

[PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[MaJun]

From: Ma Jun <majun258@huawei.com>

For peripheral devices which connect to mbigen,mbigen is a interrupt
controller. So, we create irq domain for each mbigen device and add
mbigen irq domain into irq hierarchy structure.

Signed-off-by: Ma Jun <majun258@huawei.com>
---
 drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 119 insertions(+), 0 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 9f036c2..81ae61f 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -16,13 +16,36 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
 #include <linux/module.h>
+#include <linux/msi.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+/* Interrupt numbers per mbigen node supported */
+#define IRQS_PER_MBIGEN_NODE		128
+
+/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
+#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
+
+/**
+ * In mbigen vector register
+ * bit[21:12]:	event id value
+ * bit[11:0]:	device id
+ */
+#define IRQ_EVENT_ID_SHIFT		12
+#define IRQ_EVENT_ID_MASK		0x3ff
+
+/* register range of each mbigen node */
+#define MBIGEN_NODE_OFFSET		0x1000
+
+/* offset of vector register in mbigen node */
+#define REG_MBIGEN_VEC_OFFSET		0x200
+
 /**
  * struct mbigen_device - holds the information of mbigen device.
  *
@@ -34,10 +57,94 @@ struct mbigen_device {
 	void __iomem		*base;
 };
 
+static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
+{
+	unsigned int nid, pin;
+
+	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
+	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
+	pin = hwirq % IRQS_PER_MBIGEN_NODE;
+
+	return pin * 4 + nid * MBIGEN_NODE_OFFSET
+			+ REG_MBIGEN_VEC_OFFSET;
+}
+
+static struct irq_chip mbigen_irq_chip = {
+	.name =			"mbigen-v2",
+};
+
+static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	struct irq_data *d = irq_get_irq_data(desc->irq);
+	void __iomem *base = d->chip_data;
+	u32 val;
+
+	base += get_mbigen_vec_reg(d->hwirq);
+	val = readl_relaxed(base);
+
+	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
+	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
+
+	writel_relaxed(val, base);
+}
+
+static int mbigen_domain_translate(struct irq_domain *d,
+				    struct irq_fwspec *fwspec,
+				    unsigned long *hwirq,
+				    unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int mbigen_irq_domain_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs,
+					void *args)
+{
+	struct irq_fwspec *fwspec = args;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	struct mbigen_device *mgn_chip;
+	int i, err;
+
+	err = mbigen_domain_translate(domain, fwspec, &hwirq, &type);
+	if (err)
+		return err;
+
+	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
+	if (err)
+		return err;
+
+	mgn_chip = platform_msi_get_host_data(domain);
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+				      &mbigen_irq_chip, mgn_chip->base);
+
+	return 0;
+}
+
+static struct irq_domain_ops mbigen_domain_ops = {
+	.translate	= mbigen_domain_translate,
+	.alloc		= mbigen_irq_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
 static int mbigen_device_probe(struct platform_device *pdev)
 {
 	struct mbigen_device *mgn_chip;
 	struct resource *res;
+	struct irq_domain *domain;
+	u32 num_msis;
 
 	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
 	if (!mgn_chip)
@@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev)
 	if (IS_ERR(mgn_chip->base))
 		return PTR_ERR(mgn_chip->base);
 
+	/* If there is no "num-msis" property, assume 64... */
+	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
+		num_msis = 64;
+
+	domain = platform_msi_create_device_domain(&pdev->dev, num_msis,
+							mbigen_write_msg,
+							&mbigen_domain_ops,
+							mgn_chip);
+
+	if (!domain)
+		return -ENOMEM;
+
 	platform_set_drvdata(pdev, mgn_chip);
 
 	return 0;
-- 
1.7.1

[PATCH v9 4/4] irqchip:implement the mbigen irq chip operation functions

[MaJun]

From: Ma Jun <majun258@huawei.com>

Add the interrupt controller chip operation functions of mbigen chip.

Signed-off-by: Ma Jun <majun258@huawei.com>
---
 drivers/irqchip/irq-mbigen.c |   84 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 81ae61f..540ad05 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -47,6 +47,20 @@
 #define REG_MBIGEN_VEC_OFFSET		0x200
 
 /**
+ * offset of clear register in mbigen node
+ * This register is used to clear the status
+ * of interrupt
+ */
+#define REG_MBIGEN_CLEAR_OFFSET		0xa000
+
+/**
+ * offset of interrupt type register
+ * This register is used to configure interrupt
+ * trigger type
+ */
+#define REG_MBIGEN_TYPE_OFFSET		0x0
+
+/**
  * struct mbigen_device - holds the information of mbigen device.
  *
  * @pdev:		pointer to the platform device structure of mbigen chip.
@@ -69,8 +83,78 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
 			+ REG_MBIGEN_VEC_OFFSET;
 }
 
+static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
+					unsigned int *mask,
+					unsigned int *addr)
+{
+	unsigned int nid, pin, ofst;
+
+	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
+	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
+	pin = hwirq % IRQS_PER_MBIGEN_NODE;
+
+	*mask = 1 << (pin % 32);
+	
+	ofst = pin / 32 * 4;
+	*addr = ofst + nid * MBIGEN_NODE_OFFSET
+		+ REG_MBIGEN_TYPE_OFFSET;
+}
+
+static inline void get_mbigen_clear_reg(irq_hw_number_t hwirq,
+					unsigned int *mask,
+					unsigned int *addr)
+{
+	unsigned int ofst;
+
+	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
+	ofst = hwirq / 32 * 4;
+
+	*mask = 1 << (hwirq % 32);
+	*addr = ofst + REG_MBIGEN_CLEAR_OFFSET;
+}
+
+static void mbigen_eoi_irq(struct irq_data *data)
+{
+	void __iomem *base = data->chip_data;
+	unsigned int mask, addr;
+
+	get_mbigen_clear_reg(data->hwirq, &mask, &addr);
+
+	writel_relaxed(mask, base + addr);
+
+	irq_chip_eoi_parent(data);
+}
+
+static int mbigen_set_type(struct irq_data *data, unsigned int type)
+{
+	void __iomem *base = data->chip_data;
+	unsigned int mask, addr;
+	u32 val;
+
+	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
+		return -EINVAL;
+
+	get_mbigen_type_reg(data->hwirq, &mask, &addr);
+
+	val = readl_relaxed(base + addr);
+
+	if (type == IRQ_TYPE_LEVEL_HIGH)
+		val |= mask;
+	else
+		val &= ~mask;
+
+	writel_relaxed(val, base + addr);
+
+	return 0;
+}
+
 static struct irq_chip mbigen_irq_chip = {
 	.name =			"mbigen-v2",
+	.irq_mask =		irq_chip_mask_parent,
+	.irq_unmask =		irq_chip_unmask_parent,
+	.irq_eoi =		mbigen_eoi_irq,
+	.irq_set_type =		mbigen_set_type,
+	.irq_set_affinity =	irq_chip_set_affinity_parent,
 };
 
 static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
-- 
1.7.1

Re: [PATCH v9 1/4] dt-binding:Documents of the mbigen bindings

[Marc Zyngier]

On 23/11/15 03:15, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> Add the mbigen msi interrupt controller bindings document.
> 
> This patch based on Mark Rutland's patch
> https://lkml.org/lkml/2015/7/23/558
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---

Mark,

Do you mind having a look at this from a DT perspective?

Thanks,

	M.

>  Documentation/devicetree/bindings/arm/mbigen.txt |   69 ++++++++++++++++++++++
>  1 files changed, 69 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/mbigen.txt b/Documentation/devicetree/bindings/arm/mbigen.txt
> new file mode 100644
> index 0000000..8ae59a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
> @@ -0,0 +1,69 @@
> +Hisilicon mbigen device tree bindings.
> +=======================================
> +
> +Mbigen means: message based interrupt generator.
> +
> +MBI is kind of msi interrupt only used on Non-PCI devices.
> +
> +To reduce the wired interrupt number connected to GIC,
> +Hisilicon designed mbigen to collect and generate interrupt.
> +
> +
> +Non-pci devices can connect to mbigen and generate the
> +interrupt by writing ITS register.
> +
> +The mbigen chip and devices connect to mbigen have the following properties:
> +
> +Mbigen main node required properties:
> +-------------------------------------------
> +- compatible: Should be "hisilicon,mbigen-v2"
> +- reg: Specifies the base physical address and size of the Mbigen
> +  registers.
> +- interrupt controller: Identifies the node as an interrupt controller
> +- msi-parent: This property has two cells.
> +	The 1st cell specifies the ITS this device connected.
> +	The 2nd cell specifies the device id.
> +- num-msis:Specifies the total number of interrupt this device has.
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value must be 2.
> +
> +  The 1st cell is global hardware pin number of the interrupt.
> +	This value depends on the Soc design.
> +
> +  The 2nd cell is the interrupt trigger type.
> +	The value of this cell should be:
> +	1: rising edge triggered
> +	or
> +	4: high level triggered	
> +
> +Examples:
> +
> + 	mbigen_device_gmac:intc {
> +			compatible = "hisilicon,mbigen-v2";
> +			reg = <0x0 0xc0080000 0x0 0x10000>;
> +			interrupt-controller;
> +			msi-parent = <&its_dsa 0x40b1c>;
> +			num-msis = <9>;
> +			#interrupt-cells = <2>;
> + 	};
> +
> +Devices connect to mbigen required properties:
> +----------------------------------------------------
> +-interrupt-parent: Specifies the mbigen device node which device connected.
> +-interrupts:specifies the interrupt source.
> +  The 1st cell is global hardware pin number of the interrupt.
> +		This value depends on the Soc design.
> +  The 2nd cell is the interrupt trigger type(rising edge triggered or high
> +		level triggered)
> +
> +Examples:
> +	gmac0: ethernet@c2080000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0 0xc2080000 0 0x20000>,
> +		      <0 0xc0000000 0 0x1000>;
> +		interrupt-parent  = <&mbigen_device_gmac>;
> +		interrupts =	<656 1>,
> +				<657 1>;
> +	};
> +
> 


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

Re: [PATCH v9 2/4] irqchip: add platform device driver for mbigen device

[Marc Zyngier]

On 23/11/15 03:15, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> Mbigen means Message Based Interrupt Generator(MBIGEN).
> 
> Its a kind of interrupt controller that collects
> the interrupts from external devices and generate msi interrupt.
> Mbigen is applied to reduce the number of wire connected interrupts.
> 
> As the peripherals increasing, the interrupts lines needed is
> increasing much, especially on the Arm64 server SOC.
> 
> Therefore, the interrupt pin in GIC is not enough to cover so
> many peripherals.
> 
> Mbigen is designed to fix this problem.
> 
> Mbigen chip locates in ITS or outside of ITS.
> 
> Mbigen chip hardware structure shows as below:
> 
> 		mbigen chip
> |---------------------|-------------------|
> mgn_node0	  mgn_node1		mgn_node2
>  |		 |-------|		|-------|------|
> dev1		dev1    dev2		dev1   dev3   dev4
> 
> Each mbigen chip contains several mbigen nodes.
> 
> External devices can connect to mbigen node through wire connecting way.
> 
> Because a mbigen node only can support 128 interrupt maximum, depends
> on the interrupt lines number of devices, a device can connects to one
> more mbigen nodes.
> 
> Also, several different devices can connect to a same mbigen node.
> 
> When devices triggered interrupt,mbigen chip detects and collects
> the interrupts and generates the MBI interrupts by writing the ITS
> Translator register.
> 
> To simplify mbigen driver,I used a new conception--mbigen device.
> Each mbigen device is initialized as a platform device.
> 
> Mbigen device presents the parts(register, pin definition etc.) in
> mbigen chip corresponding to a peripheral device.
> 
> So from software view, the structure likes below
> 
> 	            mbigen chip
>      |---------------------|-----------------|
> mbigen device1       mbigen device2  mbigen device3
>       |                   |                |
>      dev1                dev2             dev3
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  drivers/irqchip/Kconfig      |    8 ++++
>  drivers/irqchip/Makefile     |    1 +
>  drivers/irqchip/irq-mbigen.c |   78 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/irqchip/irq-mbigen.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d7294e..b205e15 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -27,6 +27,14 @@ config ARM_GIC_V3_ITS
>  	bool
>  	select PCI_MSI_IRQ_DOMAIN
>  
> +config HISILICON_IRQ_MBIGEN
> +	bool "Support mbigen interrupt controller"
> +	default n
> +	depends on ARM_GIC_V3 && ARM_GIC_V3_ITS && GENERIC_MSI_IRQ_DOMAIN
> +	help
> +	 Enable the mbigen interrupt controller used on
> +	 Hisilicon platform.
> +
>  config ARM_NVIC
>  	bool
>  	select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f..cd76b11 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
> +obj-$(CONFIG_HISILICON_IRQ_MBIGEN)	+= irq-mbigen.o
>  obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
>  obj-$(CONFIG_ATMEL_AIC_IRQ)		+= irq-atmel-aic-common.o irq-atmel-aic.o
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> new file mode 100644
> index 0000000..9f036c2
> --- /dev/null
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2015 Hisilicon Limited, All Rights Reserved.
> + * Author: Jun Ma <majun258@huawei.com>
> + * Author: Yun Wu <wuyun.wu@huawei.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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/**
> + * struct mbigen_device - holds the information of mbigen device.
> + *
> + * @pdev:		pointer to the platform device structure of mbigen chip.
> + * @base:		mapped address of this mbigen chip.
> + */
> +struct mbigen_device {
> +	struct platform_device	*pdev;
> +	void __iomem		*base;
> +};
> +
> +static int mbigen_device_probe(struct platform_device *pdev)
> +{
> +	struct mbigen_device *mgn_chip;
> +	struct resource *res;
> +
> +	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
> +	if (!mgn_chip)
> +		return -ENOMEM;
> +
> +	mgn_chip->pdev = pdev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(mgn_chip->base))
> +		return PTR_ERR(mgn_chip->base);
> +
> +	platform_set_drvdata(pdev, mgn_chip);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mbigen_of_match[] = {
> +	{ .compatible = "hisilicon,mbigen-v2" },
> +	{ /* END */ }
> +};
> +MODULE_DEVICE_TABLE(of, mbigen_of_match);
> +
> +static struct platform_driver mbigen_platform_driver = {
> +	.driver = {
> +		.name		= "Hisilicon MBIGEN-V2",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= mbigen_of_match,
> +	},
> +	.probe			= mbigen_device_probe,
> +};
> +
> +module_platform_driver(mbigen_platform_driver);
> +
> +MODULE_AUTHOR("Jun Ma <majun258@huawei.com>");
> +MODULE_AUTHOR("Yun Wu <wuyun.wu@huawei.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Hisilicon MBI Generator driver");
> 

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

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

Re: [PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[Marc Zyngier]

On 23/11/15 03:15, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> For peripheral devices which connect to mbigen,mbigen is a interrupt
> controller. So, we create irq domain for each mbigen device and add
> mbigen irq domain into irq hierarchy structure.
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 119 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 9f036c2..81ae61f 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -16,13 +16,36 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> +/* Interrupt numbers per mbigen node supported */
> +#define IRQS_PER_MBIGEN_NODE		128
> +
> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
> +#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
> +
> +/**
> + * In mbigen vector register
> + * bit[21:12]:	event id value
> + * bit[11:0]:	device id
> + */
> +#define IRQ_EVENT_ID_SHIFT		12
> +#define IRQ_EVENT_ID_MASK		0x3ff
> +
> +/* register range of each mbigen node */
> +#define MBIGEN_NODE_OFFSET		0x1000
> +
> +/* offset of vector register in mbigen node */
> +#define REG_MBIGEN_VEC_OFFSET		0x200
> +
>  /**
>   * struct mbigen_device - holds the information of mbigen device.
>   *
> @@ -34,10 +57,94 @@ struct mbigen_device {
>  	void __iomem		*base;
>  };
>  
> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
> +{
> +	unsigned int nid, pin;
> +
> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
> +
> +	return pin * 4 + nid * MBIGEN_NODE_OFFSET
> +			+ REG_MBIGEN_VEC_OFFSET;
> +}
> +
> +static struct irq_chip mbigen_irq_chip = {
> +	.name =			"mbigen-v2",
> +};
> +
> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct irq_data *d = irq_get_irq_data(desc->irq);
> +	void __iomem *base = d->chip_data;
> +	u32 val;
> +
> +	base += get_mbigen_vec_reg(d->hwirq);
> +	val = readl_relaxed(base);
> +
> +	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
> +	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
> +
> +	writel_relaxed(val, base);

nit: It would be good to have a comment explaining why you do not need
to program the address of the doorbell...

> +}
> +
> +static int mbigen_domain_translate(struct irq_domain *d,
> +				    struct irq_fwspec *fwspec,
> +				    unsigned long *hwirq,
> +				    unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 2)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[0];
> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int mbigen_irq_domain_alloc(struct irq_domain *domain,
> +					unsigned int virq,
> +					unsigned int nr_irqs,
> +					void *args)
> +{
> +	struct irq_fwspec *fwspec = args;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	struct mbigen_device *mgn_chip;
> +	int i, err;
> +
> +	err = mbigen_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (err)
> +		return err;
> +
> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
> +	if (err)
> +		return err;
> +
> +	mgn_chip = platform_msi_get_host_data(domain);
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +				      &mbigen_irq_chip, mgn_chip->base);
> +
> +	return 0;
> +}
> +
> +static struct irq_domain_ops mbigen_domain_ops = {
> +	.translate	= mbigen_domain_translate,
> +	.alloc		= mbigen_irq_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
>  static int mbigen_device_probe(struct platform_device *pdev)
>  {
>  	struct mbigen_device *mgn_chip;
>  	struct resource *res;
> +	struct irq_domain *domain;
> +	u32 num_msis;
>  
>  	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
>  	if (!mgn_chip)
> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev)
>  	if (IS_ERR(mgn_chip->base))
>  		return PTR_ERR(mgn_chip->base);
>  
> +	/* If there is no "num-msis" property, assume 64... */
> +	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
> +		num_msis = 64;

nit: Is that always true? This has been lifted from my dummy example, so
I wonder if that's what you actually want to do.

> +
> +	domain = platform_msi_create_device_domain(&pdev->dev, num_msis,
> +							mbigen_write_msg,
> +							&mbigen_domain_ops,
> +							mgn_chip);
> +
> +	if (!domain)
> +		return -ENOMEM;
> +
>  	platform_set_drvdata(pdev, mgn_chip);
>  
>  	return 0;
> 

Apart for the two above point,

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

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

Re: [PATCH v9 4/4] irqchip:implement the mbigen irq chip operation functions

[Marc Zyngier]

On 23/11/15 03:15, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> Add the interrupt controller chip operation functions of mbigen chip.
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c |   84 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 84 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 81ae61f..540ad05 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -47,6 +47,20 @@
>  #define REG_MBIGEN_VEC_OFFSET		0x200
>  
>  /**
> + * offset of clear register in mbigen node
> + * This register is used to clear the status
> + * of interrupt
> + */
> +#define REG_MBIGEN_CLEAR_OFFSET		0xa000
> +
> +/**
> + * offset of interrupt type register
> + * This register is used to configure interrupt
> + * trigger type
> + */
> +#define REG_MBIGEN_TYPE_OFFSET		0x0
> +
> +/**
>   * struct mbigen_device - holds the information of mbigen device.
>   *
>   * @pdev:		pointer to the platform device structure of mbigen chip.
> @@ -69,8 +83,78 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>  			+ REG_MBIGEN_VEC_OFFSET;
>  }
>  
> +static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
> +					unsigned int *mask,
> +					unsigned int *addr)

unsigned int is the wrong type if that's something you are going to use
with readl/writel. It should be u32.

> +{
> +	unsigned int nid, pin, ofst;
> +
> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
> +
> +	*mask = 1 << (pin % 32);
> +	
> +	ofst = pin / 32 * 4;
> +	*addr = ofst + nid * MBIGEN_NODE_OFFSET
> +		+ REG_MBIGEN_TYPE_OFFSET;
> +}
> +
> +static inline void get_mbigen_clear_reg(irq_hw_number_t hwirq,
> +					unsigned int *mask,

Same here.

> +					unsigned int *addr)
> +{
> +	unsigned int ofst;
> +
> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
> +	ofst = hwirq / 32 * 4;
> +
> +	*mask = 1 << (hwirq % 32);
> +	*addr = ofst + REG_MBIGEN_CLEAR_OFFSET;
> +}
> +
> +static void mbigen_eoi_irq(struct irq_data *data)
> +{
> +	void __iomem *base = data->chip_data;
> +	unsigned int mask, addr;

same here.

> +
> +	get_mbigen_clear_reg(data->hwirq, &mask, &addr);
> +
> +	writel_relaxed(mask, base + addr);
> +
> +	irq_chip_eoi_parent(data);
> +}
> +
> +static int mbigen_set_type(struct irq_data *data, unsigned int type)
> +{
> +	void __iomem *base = data->chip_data;
> +	unsigned int mask, addr;

and here.

> +	u32 val;
> +
> +	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> +		return -EINVAL;
> +
> +	get_mbigen_type_reg(data->hwirq, &mask, &addr);
> +
> +	val = readl_relaxed(base + addr);
> +
> +	if (type == IRQ_TYPE_LEVEL_HIGH)
> +		val |= mask;
> +	else
> +		val &= ~mask;
> +
> +	writel_relaxed(val, base + addr);
> +
> +	return 0;
> +}
> +
>  static struct irq_chip mbigen_irq_chip = {
>  	.name =			"mbigen-v2",
> +	.irq_mask =		irq_chip_mask_parent,
> +	.irq_unmask =		irq_chip_unmask_parent,
> +	.irq_eoi =		mbigen_eoi_irq,
> +	.irq_set_type =		mbigen_set_type,
> +	.irq_set_affinity =	irq_chip_set_affinity_parent,
>  };
>  
>  static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> 

Thanks,

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

Re: [PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[majun]

Hi Marc:

On 2015/12/3 11:25, Marc Zyngier wrote:
> On 23/11/15 03:15, MaJun wrote:
>> From: Ma Jun <majun258@huawei.com>
>>
>> For peripheral devices which connect to mbigen,mbigen is a interrupt
>> controller. So, we create irq domain for each mbigen device and add
>> mbigen irq domain into irq hierarchy structure.
>>
>> Signed-off-by: Ma Jun <majun258@huawei.com>
>> ---
>>  drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 119 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>> index 9f036c2..81ae61f 100644
>> --- a/drivers/irqchip/irq-mbigen.c
>> +++ b/drivers/irqchip/irq-mbigen.c
>> @@ -16,13 +16,36 @@
>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip.h>
>>  #include <linux/module.h>
>> +#include <linux/msi.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  
>> +/* Interrupt numbers per mbigen node supported */
>> +#define IRQS_PER_MBIGEN_NODE		128
>> +
>> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
>> +#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
>> +
>> +/**
>> + * In mbigen vector register
>> + * bit[21:12]:	event id value
>> + * bit[11:0]:	device id
>> + */
>> +#define IRQ_EVENT_ID_SHIFT		12
>> +#define IRQ_EVENT_ID_MASK		0x3ff
>> +
>> +/* register range of each mbigen node */
>> +#define MBIGEN_NODE_OFFSET		0x1000
>> +
>> +/* offset of vector register in mbigen node */
>> +#define REG_MBIGEN_VEC_OFFSET		0x200
>> +
>>  /**
>>   * struct mbigen_device - holds the information of mbigen device.
>>   *
>> @@ -34,10 +57,94 @@ struct mbigen_device {
>>  	void __iomem		*base;
>>  };
>>  
>> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>> +{
>> +	unsigned int nid, pin;
>> +
>> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
>> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>> +
>> +	return pin * 4 + nid * MBIGEN_NODE_OFFSET
>> +			+ REG_MBIGEN_VEC_OFFSET;
>> +}
>> +
>> +static struct irq_chip mbigen_irq_chip = {
>> +	.name =			"mbigen-v2",
>> +};
>> +
>> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>> +{
>> +	struct irq_data *d = irq_get_irq_data(desc->irq);
>> +	void __iomem *base = d->chip_data;
>> +	u32 val;
>> +
>> +	base += get_mbigen_vec_reg(d->hwirq);
>> +	val = readl_relaxed(base);
>> +
>> +	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
>> +	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
>> +
>> +	writel_relaxed(val, base);
> 
> nit: It would be good to have a comment explaining why you do not need
> to program the address of the doorbell...

The address of doorbell is encoded in mbigen register by default,
So, we don't need to program the doorbell address in mbigen driver.

I'll add this comment in next version.

> 
>> +}
>> +
>> +static int mbigen_domain_translate(struct irq_domain *d,
>> +				    struct irq_fwspec *fwspec,
>> +				    unsigned long *hwirq,
>> +				    unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count != 2)
>> +			return -EINVAL;
>> +
>> +		*hwirq = fwspec->param[0];
>> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> +
>> +		return 0;
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +static int mbigen_irq_domain_alloc(struct irq_domain *domain,
>> +					unsigned int virq,
>> +					unsigned int nr_irqs,
>> +					void *args)
>> +{
>> +	struct irq_fwspec *fwspec = args;
>> +	irq_hw_number_t hwirq;
>> +	unsigned int type;
>> +	struct mbigen_device *mgn_chip;
>> +	int i, err;
>> +
>> +	err = mbigen_domain_translate(domain, fwspec, &hwirq, &type);
>> +	if (err)
>> +		return err;
>> +
>> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
>> +	if (err)
>> +		return err;
>> +
>> +	mgn_chip = platform_msi_get_host_data(domain);
>> +
>> +	for (i = 0; i < nr_irqs; i++)
>> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +				      &mbigen_irq_chip, mgn_chip->base);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct irq_domain_ops mbigen_domain_ops = {
>> +	.translate	= mbigen_domain_translate,
>> +	.alloc		= mbigen_irq_domain_alloc,
>> +	.free		= irq_domain_free_irqs_common,
>> +};
>> +
>>  static int mbigen_device_probe(struct platform_device *pdev)
>>  {
>>  	struct mbigen_device *mgn_chip;
>>  	struct resource *res;
>> +	struct irq_domain *domain;
>> +	u32 num_msis;
>>  
>>  	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
>>  	if (!mgn_chip)
>> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev)
>>  	if (IS_ERR(mgn_chip->base))
>>  		return PTR_ERR(mgn_chip->base);
>>  
>> +	/* If there is no "num-msis" property, assume 64... */
>> +	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
>> +		num_msis = 64;
> 
> nit: Is that always true? This has been lifted from my dummy example,

do you mean patch v2? I just checked your patch, this part still exits.

 so
> I wonder if that's what you actually want to do.

I think the default num_msis value should be maximum msis(256) the current
msi core supported.
How about your opinion, or I need to remove this part ?

Thanks!
Ma Jun

> 
>> +
>> +	domain = platform_msi_create_device_domain(&pdev->dev, num_msis,
>> +							mbigen_write_msg,
>> +							&mbigen_domain_ops,
>> +							mgn_chip);
>> +
>> +	if (!domain)
>> +		return -ENOMEM;
>> +
>>  	platform_set_drvdata(pdev, mgn_chip);
>>  
>>  	return 0;
>>
> 
> Apart for the two above point,
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> 	M.
> 

Re: [PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[Marc Zyngier]

On Sun, 6 Dec 2015 15:53:20 -0500
majun <majun258@huawei.com> wrote:

> Hi Marc:
> 
> On 2015/12/3 11:25, Marc Zyngier wrote:
> > On 23/11/15 03:15, MaJun wrote:
> >> From: Ma Jun <majun258@huawei.com>
> >>
> >> For peripheral devices which connect to mbigen,mbigen is a interrupt
> >> controller. So, we create irq domain for each mbigen device and add
> >> mbigen irq domain into irq hierarchy structure.
> >>
> >> Signed-off-by: Ma Jun <majun258@huawei.com>
> >> ---
> >>  drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 files changed, 119 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> >> index 9f036c2..81ae61f 100644
> >> --- a/drivers/irqchip/irq-mbigen.c
> >> +++ b/drivers/irqchip/irq-mbigen.c
> >> @@ -16,13 +16,36 @@
> >>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>   */
> >>  
> >> +#include <linux/interrupt.h>
> >> +#include <linux/irqchip.h>
> >>  #include <linux/module.h>
> >> +#include <linux/msi.h>
> >>  #include <linux/of_address.h>
> >>  #include <linux/of_irq.h>
> >>  #include <linux/of_platform.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/slab.h>
> >>  
> >> +/* Interrupt numbers per mbigen node supported */
> >> +#define IRQS_PER_MBIGEN_NODE		128
> >> +
> >> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
> >> +#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
> >> +
> >> +/**
> >> + * In mbigen vector register
> >> + * bit[21:12]:	event id value
> >> + * bit[11:0]:	device id
> >> + */
> >> +#define IRQ_EVENT_ID_SHIFT		12
> >> +#define IRQ_EVENT_ID_MASK		0x3ff
> >> +
> >> +/* register range of each mbigen node */
> >> +#define MBIGEN_NODE_OFFSET		0x1000
> >> +
> >> +/* offset of vector register in mbigen node */
> >> +#define REG_MBIGEN_VEC_OFFSET		0x200
> >> +
> >>  /**
> >>   * struct mbigen_device - holds the information of mbigen device.
> >>   *
> >> @@ -34,10 +57,94 @@ struct mbigen_device {
> >>  	void __iomem		*base;
> >>  };
> >>  
> >> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
> >> +{
> >> +	unsigned int nid, pin;
> >> +
> >> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
> >> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
> >> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
> >> +
> >> +	return pin * 4 + nid * MBIGEN_NODE_OFFSET
> >> +			+ REG_MBIGEN_VEC_OFFSET;
> >> +}
> >> +
> >> +static struct irq_chip mbigen_irq_chip = {
> >> +	.name =			"mbigen-v2",
> >> +};
> >> +
> >> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> >> +{
> >> +	struct irq_data *d = irq_get_irq_data(desc->irq);
> >> +	void __iomem *base = d->chip_data;
> >> +	u32 val;
> >> +
> >> +	base += get_mbigen_vec_reg(d->hwirq);
> >> +	val = readl_relaxed(base);
> >> +
> >> +	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
> >> +	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
> >> +
> >> +	writel_relaxed(val, base);
> > 
> > nit: It would be good to have a comment explaining why you do not need
> > to program the address of the doorbell...
> 
> The address of doorbell is encoded in mbigen register by default,
> So, we don't need to program the doorbell address in mbigen driver.
> 
> I'll add this comment in next version.
> 
> > 
> >> +}
> >> +
> >> +static int mbigen_domain_translate(struct irq_domain *d,
> >> +				    struct irq_fwspec *fwspec,
> >> +				    unsigned long *hwirq,
> >> +				    unsigned int *type)
> >> +{
> >> +	if (is_of_node(fwspec->fwnode)) {
> >> +		if (fwspec->param_count != 2)
> >> +			return -EINVAL;
> >> +
> >> +		*hwirq = fwspec->param[0];
> >> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> >> +
> >> +		return 0;
> >> +	}
> >> +	return -EINVAL;
> >> +}
> >> +
> >> +static int mbigen_irq_domain_alloc(struct irq_domain *domain,
> >> +					unsigned int virq,
> >> +					unsigned int nr_irqs,
> >> +					void *args)
> >> +{
> >> +	struct irq_fwspec *fwspec = args;
> >> +	irq_hw_number_t hwirq;
> >> +	unsigned int type;
> >> +	struct mbigen_device *mgn_chip;
> >> +	int i, err;
> >> +
> >> +	err = mbigen_domain_translate(domain, fwspec, &hwirq, &type);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	mgn_chip = platform_msi_get_host_data(domain);
> >> +
> >> +	for (i = 0; i < nr_irqs; i++)
> >> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> >> +				      &mbigen_irq_chip, mgn_chip->base);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static struct irq_domain_ops mbigen_domain_ops = {
> >> +	.translate	= mbigen_domain_translate,
> >> +	.alloc		= mbigen_irq_domain_alloc,
> >> +	.free		= irq_domain_free_irqs_common,
> >> +};
> >> +
> >>  static int mbigen_device_probe(struct platform_device *pdev)
> >>  {
> >>  	struct mbigen_device *mgn_chip;
> >>  	struct resource *res;
> >> +	struct irq_domain *domain;
> >> +	u32 num_msis;
> >>  
> >>  	mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
> >>  	if (!mgn_chip)
> >> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev)
> >>  	if (IS_ERR(mgn_chip->base))
> >>  		return PTR_ERR(mgn_chip->base);
> >>  
> >> +	/* If there is no "num-msis" property, assume 64... */
> >> +	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
> >> +		num_msis = 64;
> > 
> > nit: Is that always true? This has been lifted from my dummy example,
> 
> do you mean patch v2? I just checked your patch, this part still exits.

It does exist because this example is just a toy, and I wanted to make
it easy for people to play with it.

>  so
> > I wonder if that's what you actually want to do.
> 
> I think the default num_msis value should be maximum msis(256) the current
> msi core supported.

I don't think so. If you have a fallback mechanism, it should reflect
the default value on your *own* HW. If there is no common value that is
generally used, then you should not have a default.

> How about your opinion, or I need to remove this part ?

If you don't know what this value should really be, just drop that
part, and generate an error when the num-msis property is not present.

Thanks,

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

Re: [PATCH v9 1/4] dt-binding:Documents of the mbigen bindings

[majun]

Hi Mark：
	Do you have any comments about this patch?
Thanks!
Ma Jun

On 2015/12/3 11:21, Marc Zyngier wrote:
> On 23/11/15 03:15, MaJun wrote:
>> From: Ma Jun <majun258@huawei.com>
>>
>> Add the mbigen msi interrupt controller bindings document.
>>
>> This patch based on Mark Rutland's patch
>> https://lkml.org/lkml/2015/7/23/558
>>
>> Signed-off-by: Ma Jun <majun258@huawei.com>
>> ---
> 
> Mark,
> 
> Do you mind having a look at this from a DT perspective?
> 
> Thanks,
> 
> 	M.
> 
>>  Documentation/devicetree/bindings/arm/mbigen.txt |   69 ++++++++++++++++++++++
>>  1 files changed, 69 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/mbigen.txt b/Documentation/devicetree/bindings/arm/mbigen.txt
>> new file mode 100644
>> index 0000000..8ae59a9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
>> @@ -0,0 +1,69 @@
>> +Hisilicon mbigen device tree bindings.
>> +=======================================
>> +
>> +Mbigen means: message based interrupt generator.
>> +
>> +MBI is kind of msi interrupt only used on Non-PCI devices.
>> +
>> +To reduce the wired interrupt number connected to GIC,
>> +Hisilicon designed mbigen to collect and generate interrupt.
>> +
>> +
>> +Non-pci devices can connect to mbigen and generate the
>> +interrupt by writing ITS register.
>> +
>> +The mbigen chip and devices connect to mbigen have the following properties:
>> +
>> +Mbigen main node required properties:
>> +-------------------------------------------
>> +- compatible: Should be "hisilicon,mbigen-v2"
>> +- reg: Specifies the base physical address and size of the Mbigen
>> +  registers.
>> +- interrupt controller: Identifies the node as an interrupt controller
>> +- msi-parent: This property has two cells.
>> +	The 1st cell specifies the ITS this device connected.
>> +	The 2nd cell specifies the device id.
>> +- num-msis:Specifies the total number of interrupt this device has.
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> +  interrupt source. The value must be 2.
>> +
>> +  The 1st cell is global hardware pin number of the interrupt.
>> +	This value depends on the Soc design.
>> +
>> +  The 2nd cell is the interrupt trigger type.
>> +	The value of this cell should be:
>> +	1: rising edge triggered
>> +	or
>> +	4: high level triggered	
>> +
>> +Examples:
>> +
>> + 	mbigen_device_gmac:intc {
>> +			compatible = "hisilicon,mbigen-v2";
>> +			reg = <0x0 0xc0080000 0x0 0x10000>;
>> +			interrupt-controller;
>> +			msi-parent = <&its_dsa 0x40b1c>;
>> +			num-msis = <9>;
>> +			#interrupt-cells = <2>;
>> + 	};
>> +
>> +Devices connect to mbigen required properties:
>> +----------------------------------------------------
>> +-interrupt-parent: Specifies the mbigen device node which device connected.
>> +-interrupts:specifies the interrupt source.
>> +  The 1st cell is global hardware pin number of the interrupt.
>> +		This value depends on the Soc design.
>> +  The 2nd cell is the interrupt trigger type(rising edge triggered or high
>> +		level triggered)
>> +
>> +Examples:
>> +	gmac0: ethernet@c2080000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		reg = <0 0xc2080000 0 0x20000>,
>> +		      <0 0xc0000000 0 0x1000>;
>> +		interrupt-parent  = <&mbigen_device_gmac>;
>> +		interrupts =	<656 1>,
>> +				<657 1>;
>> +	};
>> +
>>
> 
> 

Re: [PATCH v9 1/4] dt-binding:Documents of the mbigen bindings

[Mark Rutland]

Hi,

On Mon, Nov 23, 2015 at 11:15:10AM +0800, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> Add the mbigen msi interrupt controller bindings document.
> 
> This patch based on Mark Rutland's patch
> https://lkml.org/lkml/2015/7/23/558
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  Documentation/devicetree/bindings/arm/mbigen.txt |   69 ++++++++++++++++++++++
>  1 files changed, 69 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/mbigen.txt b/Documentation/devicetree/bindings/arm/mbigen.txt
> new file mode 100644
> index 0000000..8ae59a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
> @@ -0,0 +1,69 @@
> +Hisilicon mbigen device tree bindings.
> +=======================================
> +
> +Mbigen means: message based interrupt generator.
> +
> +MBI is kind of msi interrupt only used on Non-PCI devices.
> +
> +To reduce the wired interrupt number connected to GIC,
> +Hisilicon designed mbigen to collect and generate interrupt.
> +
> +
> +Non-pci devices can connect to mbigen and generate the
> +interrupt by writing ITS register.
> +
> +The mbigen chip and devices connect to mbigen have the following properties:
> +
> +Mbigen main node required properties:
> +-------------------------------------------
> +- compatible: Should be "hisilicon,mbigen-v2"
> +- reg: Specifies the base physical address and size of the Mbigen
> +  registers.
> +- interrupt controller: Identifies the node as an interrupt controller
> +- msi-parent: This property has two cells.
> +	The 1st cell specifies the ITS this device connected.
> +	The 2nd cell specifies the device id.

This should just refer to the generic msi-parent binding in
Documentation/devicetree/bindings/interrupt-controller/msi.txt.

I assume that the driver does not try to parse this itself (i.e. it uses
generic accessors and doesn't assume anything about the meaning of these
cells).

> +- num-msis:Specifies the total number of interrupt this device has.
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value must be 2.
> +
> +  The 1st cell is global hardware pin number of the interrupt.
> +	This value depends on the Soc design.

I think a little more information is required here. Presumably the
"global hardware pin number" is actually a pin number within the
particular mbigen instance? i.e. it is local to this instance?

> +  The 2nd cell is the interrupt trigger type.
> +	The value of this cell should be:
> +	1: rising edge triggered
> +	or
> +	4: high level triggered	
> +
> +Examples:
> +
> + 	mbigen_device_gmac:intc {
> +			compatible = "hisilicon,mbigen-v2";
> +			reg = <0x0 0xc0080000 0x0 0x10000>;
> +			interrupt-controller;
> +			msi-parent = <&its_dsa 0x40b1c>;
> +			num-msis = <9>;
> +			#interrupt-cells = <2>;
> + 	};
> +
> +Devices connect to mbigen required properties:
> +----------------------------------------------------
> +-interrupt-parent: Specifies the mbigen device node which device connected.
> +-interrupts:specifies the interrupt source.
> +  The 1st cell is global hardware pin number of the interrupt.
> +		This value depends on the Soc design.
> +  The 2nd cell is the interrupt trigger type(rising edge triggered or high
> +		level triggered)

You should be able to refer to the usual interrupt bindings given you
defined the format previously when describing #interrupt-cells.

Thanks,
Mark.

Re: [PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[Mark Rutland]

On Mon, Nov 23, 2015 at 11:15:12AM +0800, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> For peripheral devices which connect to mbigen,mbigen is a interrupt
> controller. So, we create irq domain for each mbigen device and add
> mbigen irq domain into irq hierarchy structure.
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 119 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 9f036c2..81ae61f 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -16,13 +16,36 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> +/* Interrupt numbers per mbigen node supported */
> +#define IRQS_PER_MBIGEN_NODE		128
> +
> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
> +#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
> +
> +/**
> + * In mbigen vector register
> + * bit[21:12]:	event id value
> + * bit[11:0]:	device id
> + */
> +#define IRQ_EVENT_ID_SHIFT		12
> +#define IRQ_EVENT_ID_MASK		0x3ff
> +
> +/* register range of each mbigen node */
> +#define MBIGEN_NODE_OFFSET		0x1000
> +
> +/* offset of vector register in mbigen node */
> +#define REG_MBIGEN_VEC_OFFSET		0x200
> +
>  /**
>   * struct mbigen_device - holds the information of mbigen device.
>   *
> @@ -34,10 +57,94 @@ struct mbigen_device {
>  	void __iomem		*base;
>  };
>  
> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
> +{
> +	unsigned int nid, pin;
> +
> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
> +
> +	return pin * 4 + nid * MBIGEN_NODE_OFFSET
> +			+ REG_MBIGEN_VEC_OFFSET;
> +}

Ok. So your "global" pin id is "global" per mbigen chip.

I think it may make more sense to have separate nid and pin fields in
your interrupt-specifier, e.g. interrupt = <1 3 x> for nid 1, pin 3.

That's easier for someone to check against a datasheet that describes
the nid and pin rather than the global number space you've come up with,
and also makes it impossible to describe the reserved IRQs.

> +
> +static struct irq_chip mbigen_irq_chip = {
> +	.name =			"mbigen-v2",
> +};
> +
> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct irq_data *d = irq_get_irq_data(desc->irq);
> +	void __iomem *base = d->chip_data;
> +	u32 val;
> +
> +	base += get_mbigen_vec_reg(d->hwirq);
> +	val = readl_relaxed(base);
> +
> +	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
> +	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
> +
> +	writel_relaxed(val, base);
> +}
> +
> +static int mbigen_domain_translate(struct irq_domain *d,
> +				    struct irq_fwspec *fwspec,
> +				    unsigned long *hwirq,
> +				    unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 2)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[0];

You should validate the hwirq here. For instance, we never expect a
hwirq < RESERVED_IRQ_PER_MBIGEN_CHIP here.

> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;

Don't mask out bits you don't expect to be set. Validate that they
aren't set and complain if they are.

> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev)
>  	if (IS_ERR(mgn_chip->base))
>  		return PTR_ERR(mgn_chip->base);
>  
> +	/* If there is no "num-msis" property, assume 64... */
> +	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
> +		num_msis = 64;

The "num-msis" property was mandatory in the binding. We shouldn't need
the fallback.

It it is missing, print an error, and abort probing here.

Thanks,
Mark.

Re: [PATCH v9 0/4] irqchip:support mbigen interrupt controller

[Marc Zyngier]

On 23/11/15 03:15, MaJun wrote:
> From: Ma Jun <majun258@huawei.com>
> 
> This patch set adds the driver of mbigen and binding document for Hisilicon
> Mbigen chips.

[...]

Any update on this? If this is to be 4.5 material, I'd like to see the
various comments addressed shortly so that I can queue it.

Thanks,

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

Re: [PATCH v9 0/4] irqchip:support mbigen interrupt controller

[majun]

Hi Marc:
	Sorry for late response.
I just came back from a business trip from American.
I'll send a new version ASAP on tomorrow.

Thanks!
Ma Jun

On 2015/12/16 6:22, Marc Zyngier wrote:
> On 23/11/15 03:15, MaJun wrote:
>> From: Ma Jun <majun258@huawei.com>
>>
>> This patch set adds the driver of mbigen and binding document for Hisilicon
>> Mbigen chips.
> 
> [...]
> 
> Any update on this? If this is to be 4.5 material, I'd like to see the
> various comments addressed shortly so that I can queue it.
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v9 1/4] dt-binding:Documents of the mbigen bindings

[majun]

Hi Mark:

On 2015/12/11 10:26, Mark Rutland wrote:
> Hi,
> 
> On Mon, Nov 23, 2015 at 11:15:10AM +0800, MaJun wrote:
>> From: Ma Jun <majun258@huawei.com>
>>
>> Add the mbigen msi interrupt controller bindings document.
>>
>> This patch based on Mark Rutland's patch
>> https://lkml.org/lkml/2015/7/23/558
>>
>> Signed-off-by: Ma Jun <majun258@huawei.com>
>> ---
>>  Documentation/devicetree/bindings/arm/mbigen.txt |   69 ++++++++++++++++++++++
>>  1 files changed, 69 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/mbigen.txt b/Documentation/devicetree/bindings/arm/mbigen.txt
>> new file mode 100644
>> index 0000000..8ae59a9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
>> @@ -0,0 +1,69 @@
>> +Hisilicon mbigen device tree bindings.
>> +=======================================
>> +
[...]
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> +  interrupt source. The value must be 2.
>> +
>> +  The 1st cell is global hardware pin number of the interrupt.
>> +	This value depends on the Soc design.
> 
> I think a little more information is required here. Presumably the
> "global hardware pin number" is actually a pin number within the
> particular mbigen instance? i.e. it is local to this instance?
> 

Maybe "global hardware pin number" is  not an accurate definition of pin number and
makes people confused.

I will change it to "hardware pin number" to present the real pin number of
wired interrupt(from 0 to maximum interrupt number).

So, there is no global pin number or local pin number.

Thanks!
Majun


>> +  The 2nd cell is the interrupt trigger type.
>> +	The value of this cell should be:
>> +	1: rising edge triggered
>> +	or
>> +	4: high level triggered	
>> +
>> +Examples:
>> +
>> + 	mbigen_device_gmac:intc {
>> +			compatible = "hisilicon,mbigen-v2";
>> +			reg = <0x0 0xc0080000 0x0 0x10000>;
>> +			interrupt-controller;
>> +			msi-parent = <&its_dsa 0x40b1c>;
>> +			num-msis = <9>;
>> +			#interrupt-cells = <2>;
>> + 	};
>> +
>> +Devices connect to mbigen required properties:
>> +----------------------------------------------------
>> +-interrupt-parent: Specifies the mbigen device node which device connected.
>> +-interrupts:specifies the interrupt source.
>> +  The 1st cell is global hardware pin number of the interrupt.
>> +		This value depends on the Soc design.
>> +  The 2nd cell is the interrupt trigger type(rising edge triggered or high
>> +		level triggered)
> 
> You should be able to refer to the usual interrupt bindings given you
> defined the format previously when describing #interrupt-cells.
> 
> Thanks,
> Mark.
> 
> .
> 

Re: [PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[majun]

Hi Marc and Mark:

On 2015/12/11 10:42, Mark Rutland wrote:
> On Mon, Nov 23, 2015 at 11:15:12AM +0800, MaJun wrote:
>> From: Ma Jun <majun258@huawei.com>
>>
>> For peripheral devices which connect to mbigen,mbigen is a interrupt
>> controller. So, we create irq domain for each mbigen device and add
>> mbigen irq domain into irq hierarchy structure.
>>
>> Signed-off-by: Ma Jun <majun258@huawei.com>
>> ---
>>  drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 119 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>> index 9f036c2..81ae61f 100644
>> --- a/drivers/irqchip/irq-mbigen.c
>> +++ b/drivers/irqchip/irq-mbigen.c
>> @@ -16,13 +16,36 @@
>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip.h>
>>  #include <linux/module.h>
>> +#include <linux/msi.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  
>> +/* Interrupt numbers per mbigen node supported */
>> +#define IRQS_PER_MBIGEN_NODE		128
>> +
>> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
>> +#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
>> +
>> +/**
>> + * In mbigen vector register
>> + * bit[21:12]:	event id value
>> + * bit[11:0]:	device id
>> + */
>> +#define IRQ_EVENT_ID_SHIFT		12
>> +#define IRQ_EVENT_ID_MASK		0x3ff
>> +
>> +/* register range of each mbigen node */
>> +#define MBIGEN_NODE_OFFSET		0x1000
>> +
>> +/* offset of vector register in mbigen node */
>> +#define REG_MBIGEN_VEC_OFFSET		0x200
>> +
>>  /**
>>   * struct mbigen_device - holds the information of mbigen device.
>>   *
>> @@ -34,10 +57,94 @@ struct mbigen_device {
>>  	void __iomem		*base;
>>  };
>>  
>> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>> +{
>> +	unsigned int nid, pin;
>> +
>> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
>> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>> +
>> +	return pin * 4 + nid * MBIGEN_NODE_OFFSET
>> +			+ REG_MBIGEN_VEC_OFFSET;
>> +}
> 
> Ok. So your "global" pin id is "global" per mbigen chip.

right.

> 
> I think it may make more sense to have separate nid and pin fields in
> your interrupt-specifier, e.g. interrupt = <1 3 x> for nid 1, pin 3.
> 
> That's easier for someone to check against a datasheet that describes
> the nid and pin rather than the global number space you've come up with,
> and also makes it impossible to describe the reserved IRQs.

There are no nid and pin fields in our new datasheet now.
All we can see is hardware pin number.
So adding nid and pin fields makes the people more confused about using
mbigen.

Further more, "pin" is not a good variable name. I should name it as
"pin_offset" or just"offset" to present the interrupt pin offset to mbigen node.

> 
>> +
>> +static struct irq_chip mbigen_irq_chip = {
>> +	.name =			"mbigen-v2",
>> +};
>> +
>> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>> +{
>> +	struct irq_data *d = irq_get_irq_data(desc->irq);
>> +	void __iomem *base = d->chip_data;
>> +	u32 val;
>> +
>> +	base += get_mbigen_vec_reg(d->hwirq);
>> +	val = readl_relaxed(base);
>> +
>> +	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
>> +	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
>> +
>> +	writel_relaxed(val, base);
>> +}
>> +
>> +static int mbigen_domain_translate(struct irq_domain *d,
>> +				    struct irq_fwspec *fwspec,
>> +				    unsigned long *hwirq,
>> +				    unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count != 2)
>> +			return -EINVAL;
>> +
>> +		*hwirq = fwspec->param[0];
> 
> You should validate the hwirq here. For instance, we never expect a
> hwirq < RESERVED_IRQ_PER_MBIGEN_CHIP here.

Yes, I also think I need to check the hwirq input value.
The hwirq should be:
hwirq > RESERVED_IRQ_PER_MBIGEN_CHIP && hwirq < MAXIMUM_INTERRUPT_NUMBER

> 
>> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> 
> Don't mask out bits you don't expect to be set. Validate that they
> aren't set and complain if they are.
> 

I referred Marc's dummy driver when coding this function.

Marc, do you have any different comment about these two parts.


>> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev)
>>  	if (IS_ERR(mgn_chip->base))
>>  		return PTR_ERR(mgn_chip->base);
>>  
>> +	/* If there is no "num-msis" property, assume 64... */
>> +	if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0)
>> +		num_msis = 64;
> 
> The "num-msis" property was mandatory in the binding. We shouldn't need
> the fallback.
> 
> It it is missing, print an error, and abort probing here.
> 

Yes you are right.

Thanks
MaJun

> Thanks,
> Mark.
> 
> .
> 

Re: [PATCH v9 3/4] irqchip:create irq domain for each mbigen device

[Marc Zyngier]

On 16/12/15 14:57, majun wrote:
> Hi Marc and Mark:
> 
> On 2015/12/11 10:42, Mark Rutland wrote:
>> On Mon, Nov 23, 2015 at 11:15:12AM +0800, MaJun wrote:
>>> From: Ma Jun <majun258@huawei.com>
>>>
>>> For peripheral devices which connect to mbigen,mbigen is a interrupt
>>> controller. So, we create irq domain for each mbigen device and add
>>> mbigen irq domain into irq hierarchy structure.
>>>
>>> Signed-off-by: Ma Jun <majun258@huawei.com>
>>> ---
>>>  drivers/irqchip/irq-mbigen.c |  119 ++++++++++++++++++++++++++++++++++++++++++
>>>  1 files changed, 119 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>>> index 9f036c2..81ae61f 100644
>>> --- a/drivers/irqchip/irq-mbigen.c
>>> +++ b/drivers/irqchip/irq-mbigen.c
>>> @@ -16,13 +16,36 @@
>>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  
>>> +#include <linux/interrupt.h>
>>> +#include <linux/irqchip.h>
>>>  #include <linux/module.h>
>>> +#include <linux/msi.h>
>>>  #include <linux/of_address.h>
>>>  #include <linux/of_irq.h>
>>>  #include <linux/of_platform.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/slab.h>
>>>  
>>> +/* Interrupt numbers per mbigen node supported */
>>> +#define IRQS_PER_MBIGEN_NODE		128
>>> +
>>> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
>>> +#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
>>> +
>>> +/**
>>> + * In mbigen vector register
>>> + * bit[21:12]:	event id value
>>> + * bit[11:0]:	device id
>>> + */
>>> +#define IRQ_EVENT_ID_SHIFT		12
>>> +#define IRQ_EVENT_ID_MASK		0x3ff
>>> +
>>> +/* register range of each mbigen node */
>>> +#define MBIGEN_NODE_OFFSET		0x1000
>>> +
>>> +/* offset of vector register in mbigen node */
>>> +#define REG_MBIGEN_VEC_OFFSET		0x200
>>> +
>>>  /**
>>>   * struct mbigen_device - holds the information of mbigen device.
>>>   *
>>> @@ -34,10 +57,94 @@ struct mbigen_device {
>>>  	void __iomem		*base;
>>>  };
>>>  
>>> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>>> +{
>>> +	unsigned int nid, pin;
>>> +
>>> +	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
>>> +	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>>> +	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>>> +
>>> +	return pin * 4 + nid * MBIGEN_NODE_OFFSET
>>> +			+ REG_MBIGEN_VEC_OFFSET;
>>> +}
>>
>> Ok. So your "global" pin id is "global" per mbigen chip.
> 
> right.
> 
>>
>> I think it may make more sense to have separate nid and pin fields in
>> your interrupt-specifier, e.g. interrupt = <1 3 x> for nid 1, pin 3.
>>
>> That's easier for someone to check against a datasheet that describes
>> the nid and pin rather than the global number space you've come up with,
>> and also makes it impossible to describe the reserved IRQs.
> 
> There are no nid and pin fields in our new datasheet now.
> All we can see is hardware pin number.
> So adding nid and pin fields makes the people more confused about using
> mbigen.
> 
> Further more, "pin" is not a good variable name. I should name it as
> "pin_offset" or just"offset" to present the interrupt pin offset to mbigen node.
> 
>>
>>> +
>>> +static struct irq_chip mbigen_irq_chip = {
>>> +	.name =			"mbigen-v2",
>>> +};
>>> +
>>> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>>> +{
>>> +	struct irq_data *d = irq_get_irq_data(desc->irq);
>>> +	void __iomem *base = d->chip_data;
>>> +	u32 val;
>>> +
>>> +	base += get_mbigen_vec_reg(d->hwirq);
>>> +	val = readl_relaxed(base);
>>> +
>>> +	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
>>> +	val |= (msg->data << IRQ_EVENT_ID_SHIFT);
>>> +
>>> +	writel_relaxed(val, base);
>>> +}
>>> +
>>> +static int mbigen_domain_translate(struct irq_domain *d,
>>> +				    struct irq_fwspec *fwspec,
>>> +				    unsigned long *hwirq,
>>> +				    unsigned int *type)
>>> +{
>>> +	if (is_of_node(fwspec->fwnode)) {
>>> +		if (fwspec->param_count != 2)
>>> +			return -EINVAL;
>>> +
>>> +		*hwirq = fwspec->param[0];
>>
>> You should validate the hwirq here. For instance, we never expect a
>> hwirq < RESERVED_IRQ_PER_MBIGEN_CHIP here.
> 
> Yes, I also think I need to check the hwirq input value.
> The hwirq should be:
> hwirq > RESERVED_IRQ_PER_MBIGEN_CHIP && hwirq < MAXIMUM_INTERRUPT_NUMBER
> 
>>
>>> +		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>>
>> Don't mask out bits you don't expect to be set. Validate that they
>> aren't set and complain if they are.
>>
> 
> I referred Marc's dummy driver when coding this function.
> 
> Marc, do you have any different comment about these two parts.

My dummy driver was exactly that: a dummy. It was not meant to be
followed to the letter, but just an example showing how to use a new
API. And given that it didn't handle any interrupt, it really didn't
matter what it did in the translate function.

Here, I think Mark is right, and you should follow his recommendation.

Thanks,

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