LinuxLists.cc - [PATCH v10 0/4] irqchip:support mbigen interrupt controller

[permalink] [raw]

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

From: Ma Jun <[email protected]>

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 <[email protected]>
---
Documentation/devicetree/bindings/arm/mbigen.txt | 74 ++++++++++++++++++++++
1 files changed, 74 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..3eaa678
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mbigen.txt
@@ -0,0 +1,74 @@
+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: Specifies the MSI controller this mbigen use.
+ For more detail information,please refer to the generic msi-parent binding in
+ Documentation/devicetree/bindings/interrupt-controller/msi.txt.
+
+- 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 hardware pin number of the interrupt.This number is local to
+ each mbigen chip and in the range from 0 to the maximum interrupts number
+ of the mbigen.
+
+ 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.
+ For the specific information of each cell in this property,please refer to
+ the "interrupt-cells" description mentioned above.
+
+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

2015-12-17 12:06:38

[permalink] [raw]

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

From: Ma Jun <[email protected]>

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

Reviewed-by: Marc Zyngier <[email protected]>
Signed-off-by: Ma Jun <[email protected]>
---
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 <[email protected]>
+ * Author: Yun Wu <[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.
+ *
+ * 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 <[email protected]>");
+MODULE_AUTHOR("Yun Wu <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Hisilicon MBI Generator driver");
--
1.7.1

2015-12-17 12:14:45

[permalink] [raw]

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

From: Ma Jun <[email protected]>

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 <[email protected]>
---
drivers/irqchip/irq-mbigen.c | 138 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 9f036c2..98865b1 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -16,13 +16,39 @@
* 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
+
+/* The maximum IRQ pin number of mbigen chip(start from 0) */
+#define MAXIMUM_IRQ_PIN_NUM 1407
+
+/**
+ * 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 +60,107 @@ 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);
+
+ /* The address of doorbell is encoded in mbigen register by default
+ * So,we don't need to program the doorbell address at here
+ */
+ 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;
+
+ if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
+ (fwspec->param[0] < RESERVED_IRQ_PER_MBIGEN_CHIP))
+ return -EINVAL;
+ else
+ *hwirq = fwspec->param[0];
+
+ /* If there is no valid irq type, just use the default type */
+ if ((fwspec->param[1] == IRQ_TYPE_EDGE_RISING) ||
+ (fwspec->param[1] == IRQ_TYPE_LEVEL_HIGH))
+ *type = fwspec->param[1];
+ else
+ *type = IRQ_TYPE_NONE;
+
+ 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,8 +173,23 @@ static int mbigen_device_probe(struct platform_device *pdev)
if (IS_ERR(mgn_chip->base))
return PTR_ERR(mgn_chip->base);

+ if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) {
+ dev_err(&pdev->dev, "No num-msis property\n");
+ return -EINVAL;
+ }
+
+ 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);

+ dev_info(&pdev->dev, "Probing ok, allocated %d MSIs\n", num_msis);
+
return 0;
}

--
1.7.1

2015-12-17 12:02:04

[permalink] [raw]

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

From: Ma Jun <[email protected]>

Add the interrupt controller chip operation functions of mbigen chip.

Signed-off-by: Ma Jun <[email protected]>
---
drivers/irqchip/irq-mbigen.c | 81 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 98865b1..a6856f2 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -50,6 +50,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.
@@ -72,8 +86,75 @@ 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,
+ u32 *mask, u32 *addr)
+{
+ unsigned int nid, irq_ofst, ofst;
+
+ hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
+ nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
+ irq_ofst = hwirq % IRQS_PER_MBIGEN_NODE;
+
+ *mask = 1 << (irq_ofst % 32);
+ ofst = irq_ofst / 32 * 4;
+
+ *addr = ofst + nid * MBIGEN_NODE_OFFSET
+ + REG_MBIGEN_TYPE_OFFSET;
+}
+
+static inline void get_mbigen_clear_reg(irq_hw_number_t hwirq,
+ u32 *mask, u32 *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;
+ u32 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;
+ u32 mask, addr, 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

2015-12-17 13:53:06

[permalink] [raw]

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

On Thu, Dec 17, 2015 at 07:56:34PM +0800, MaJun wrote:
> From: Ma Jun <[email protected]>
>
> 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 <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/mbigen.txt | 74 ++++++++++++++++++++++
> 1 files changed, 74 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..3eaa678
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
> @@ -0,0 +1,74 @@
> +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: Specifies the MSI controller this mbigen use.
> + For more detail information,please refer to the generic msi-parent binding in
> + Documentation/devicetree/bindings/interrupt-controller/msi.txt.
> +
> +- num-msis:Specifies the total number of interrupt this device has.

Is this the number of pins implemented? Or the number of pins that are
in use?

The latter feels like something we can derive.

> +- #interrupt-cells : Specifies the number of cells needed to encode an
> + interrupt source. The value must be 2.
> +
> + The 1st cell is hardware pin number of the interrupt.This number is local to
> + each mbigen chip and in the range from 0 to the maximum interrupts number
> + of the mbigen.

Just to check: 0 - 63 represent the "reserved" pins, yes?

Other than those questions, this looks good to me.

Thanks,
Mark.

> +
> + 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.
> + For the specific information of each cell in this property,please refer to
> + the "interrupt-cells" description mentioned above.
> +
> +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
>
>

2015-12-17 16:02:32

[permalink] [raw]

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

On 17/12/15 11:56, MaJun wrote:
> From: Ma Jun <[email protected]>
>
> 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 <[email protected]>

Reviewed-by: Marc Zyngier <[email protected]>

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

2015-12-17 16:03:35

[permalink] [raw]

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

On 17/12/15 11:56, MaJun wrote:
> From: Ma Jun <[email protected]>
>
> Add the interrupt controller chip operation functions of mbigen chip.
>
> Signed-off-by: Ma Jun <[email protected]>

Reviewed-by: Marc Zyngier <[email protected]>

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

2015-12-17 16:06:11

[permalink] [raw]

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

On 17/12/15 11:56, MaJun wrote:
> From: Ma Jun <[email protected]>
>
> This patch set adds the driver of mbigen and binding document for Hisilicon
> Mbigen chips.

[...[

Right, we're almost there. Assuming you quickly deal with the couple of
remarks Mark had, I'll apply this on top of my irq/wire-msi-bridge
branch for Thomas to merge it into 4.5.

Thanks,

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

2015-12-18 01:58:50

[permalink] [raw]

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

Hi Mark:

在 2015/12/17 21:52, Mark Rutland 写道:
> On Thu, Dec 17, 2015 at 07:56:34PM +0800, MaJun wrote:
>> From: Ma Jun <[email protected]>
[...]
>> +- 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: Specifies the MSI controller this mbigen use.
>> + For more detail information,please refer to the generic msi-parent binding in
>> + Documentation/devicetree/bindings/interrupt-controller/msi.txt.
>> +
>> +- num-msis:Specifies the total number of interrupt this device has.
>
> Is this the number of pins implemented? Or the number of pins that are
> in use?
>
> The latter feels like something we can derive.

num-msis means the total number of pins implemented.

>
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> + interrupt source. The value must be 2.
>> +
>> + The 1st cell is hardware pin number of the interrupt.This number is local to
>> + each mbigen chip and in the range from 0 to the maximum interrupts number
>> + of the mbigen.
>
> Just to check: 0 - 63 represent the "reserved" pins, yes?

Yes, you are right.

>
> Other than those questions, this looks good to me.

Do i need to post a new patch to update these two questions?

Thanks!
MaJun

>
> Thanks,
> Mark.
>
>> +
>> + 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.
>> + For the specific information of each cell in this property,please refer to
>> + the "interrupt-cells" description mentioned above.
>> +
>> +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
>>
>>
>
> .
>

2015-12-18 10:58:57

[permalink] [raw]

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

On Fri, Dec 18, 2015 at 09:58:20AM +0800, majun (F) wrote:
> Hi Mark:
>
> 在 2015/12/17 21:52, Mark Rutland 写道:
> > On Thu, Dec 17, 2015 at 07:56:34PM +0800, MaJun wrote:
> >> From: Ma Jun <[email protected]>
> [...]
> >> +- 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: Specifies the MSI controller this mbigen use.
> >> + For more detail information,please refer to the generic msi-parent binding in
> >> + Documentation/devicetree/bindings/interrupt-controller/msi.txt.
> >> +
> >> +- num-msis:Specifies the total number of interrupt this device has.
> >
> > Is this the number of pins implemented? Or the number of pins that are
> > in use?
> >
> > The latter feels like something we can derive.
>
> num-msis means the total number of pins implemented.

Ok. In that case I think it should be:

- num-pins: the total number of pins implemented in this Mbigen
instance.

(with the appropriate fixups in the driver).

With that:

Acked-by: Mark Rutland <[email protected]>

> >> +- #interrupt-cells : Specifies the number of cells needed to encode an
> >> + interrupt source. The value must be 2.
> >> +
> >> + The 1st cell is hardware pin number of the interrupt.This number is local to
> >> + each mbigen chip and in the range from 0 to the maximum interrupts number
> >> + of the mbigen.
> >
> > Just to check: 0 - 63 represent the "reserved" pins, yes?
>
> Yes, you are right.
>
> >
> > Other than those questions, this looks good to me.
>
> Do i need to post a new patch to update these two questions?

Hopefully not.

Marc, are you happy to fold in the s/num-msis/num-pins/ change?

Thanks,
Mark.

2015-12-18 11:03:12

[permalink] [raw]

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

On Thu, Dec 17, 2015 at 07:56:36PM +0800, MaJun wrote:
> From: Ma Jun <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/irqchip/irq-mbigen.c | 138 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 138 insertions(+), 0 deletions(-)

[...]

> +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;
> +
> + if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
> + (fwspec->param[0] < RESERVED_IRQ_PER_MBIGEN_CHIP))
> + return -EINVAL;
> + else
> + *hwirq = fwspec->param[0];
> +
> + /* If there is no valid irq type, just use the default type */
> + if ((fwspec->param[1] == IRQ_TYPE_EDGE_RISING) ||
> + (fwspec->param[1] == IRQ_TYPE_LEVEL_HIGH))
> + *type = fwspec->param[1];
> + else
> + *type = IRQ_TYPE_NONE;

That does not seem like a good idea. The binding requires one of two
types, and the DT is clearly wrong in the else case.

I think for the else case we should return -EINVAL.

Thanks,
Mark.

2015-12-18 11:27:00

[permalink] [raw]

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

On 18/12/15 10:58, Mark Rutland wrote:
> On Fri, Dec 18, 2015 at 09:58:20AM +0800, majun (F) wrote:
>> Hi Mark:
>>
>> 在 2015/12/17 21:52, Mark Rutland 写道:
>>> On Thu, Dec 17, 2015 at 07:56:34PM +0800, MaJun wrote:
>>>> From: Ma Jun <[email protected]>
>> [...]
>>>> +- 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: Specifies the MSI controller this mbigen use.
>>>> + For more detail information,please refer to the generic msi-parent binding in
>>>> + Documentation/devicetree/bindings/interrupt-controller/msi.txt.
>>>> +
>>>> +- num-msis:Specifies the total number of interrupt this device has.
>>>
>>> Is this the number of pins implemented? Or the number of pins that are
>>> in use?
>>>
>>> The latter feels like something we can derive.
>>
>> num-msis means the total number of pins implemented.
>
> Ok. In that case I think it should be:
>
> - num-pins: the total number of pins implemented in this Mbigen
> instance.
>
> (with the appropriate fixups in the driver).
>
> With that:
>
> Acked-by: Mark Rutland <[email protected]>
>
>>>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>>>> + interrupt source. The value must be 2.
>>>> +
>>>> + The 1st cell is hardware pin number of the interrupt.This number is local to
>>>> + each mbigen chip and in the range from 0 to the maximum interrupts number
>>>> + of the mbigen.
>>>
>>> Just to check: 0 - 63 represent the "reserved" pins, yes?
>>
>> Yes, you are right.
>>
>>>
>>> Other than those questions, this looks good to me.
>>
>> Do i need to post a new patch to update these two questions?
>
> Hopefully not.
>
> Marc, are you happy to fold in the s/num-msis/num-pins/ change?

Sure, I'll update the whole thing while merging it.

Thanks,

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

2015-12-18 11:27:42

[permalink] [raw]

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

On 18/12/15 11:02, Mark Rutland wrote:
> On Thu, Dec 17, 2015 at 07:56:36PM +0800, MaJun wrote:
>> From: Ma Jun <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> drivers/irqchip/irq-mbigen.c | 138 ++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 138 insertions(+), 0 deletions(-)
>
> [...]
>
>> +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;
>> +
>> + if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
>> + (fwspec->param[0] < RESERVED_IRQ_PER_MBIGEN_CHIP))
>> + return -EINVAL;
>> + else
>> + *hwirq = fwspec->param[0];
>> +
>> + /* If there is no valid irq type, just use the default type */
>> + if ((fwspec->param[1] == IRQ_TYPE_EDGE_RISING) ||
>> + (fwspec->param[1] == IRQ_TYPE_LEVEL_HIGH))
>> + *type = fwspec->param[1];
>> + else
>> + *type = IRQ_TYPE_NONE;
>
> That does not seem like a good idea. The binding requires one of two
> types, and the DT is clearly wrong in the else case.
>
> I think for the else case we should return -EINVAL.

Ah, nicely spotted. I'll fix that in my tree too.

Thanks,

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

2015-12-18 11:35:42

[permalink] [raw]

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

On 18/12/15 10:58, Mark Rutland wrote:
> On Fri, Dec 18, 2015 at 09:58:20AM +0800, majun (F) wrote:
>> Hi Mark:
>>
>> 在 2015/12/17 21:52, Mark Rutland 写道:
>>> On Thu, Dec 17, 2015 at 07:56:34PM +0800, MaJun wrote:
>>>> From: Ma Jun <[email protected]>
>> [...]
>>>> +- 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: Specifies the MSI controller this mbigen use.
>>>> + For more detail information,please refer to the generic msi-parent binding in
>>>> + Documentation/devicetree/bindings/interrupt-controller/msi.txt.
>>>> +
>>>> +- num-msis:Specifies the total number of interrupt this device has.
>>>
>>> Is this the number of pins implemented? Or the number of pins that are
>>> in use?
>>>
>>> The latter feels like something we can derive.
>>
>> num-msis means the total number of pins implemented.
>
> Ok. In that case I think it should be:
>
> - num-pins: the total number of pins implemented in this Mbigen
> instance.
>
> (with the appropriate fixups in the driver).
>
> With that:
>
> Acked-by: Mark Rutland <[email protected]>
>
>>>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>>>> + interrupt source. The value must be 2.
>>>> +
>>>> + The 1st cell is hardware pin number of the interrupt.This number is local to
>>>> + each mbigen chip and in the range from 0 to the maximum interrupts number
>>>> + of the mbigen.
>>>
>>> Just to check: 0 - 63 represent the "reserved" pins, yes?
>>
>> Yes, you are right.
>>
>>>
>>> Other than those questions, this looks good to me.
>>
>> Do i need to post a new patch to update these two questions?
>
> Hopefully not.
>
> Marc, are you happy to fold in the s/num-msis/num-pins/ change?

FWIW, I'm also moving this to:

Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt

in order to match the new documentation conventions.

Thanks,

M.

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

2015-12-18 11:55:05

[permalink] [raw]

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

On Fri, Dec 18, 2015 at 11:35:36AM +0000, Marc Zyngier wrote:
> On 18/12/15 10:58, Mark Rutland wrote:
> > Marc, are you happy to fold in the s/num-msis/num-pins/ change?
>
> FWIW, I'm also moving this to:
>
> Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt
>
> in order to match the new documentation conventions.

Even better! My ack stil stands.

Mark.

2015-12-18 11:58:16

[permalink] [raw]

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

On 17/12/15 11:56, MaJun wrote:
> From: Ma Jun <[email protected]>
>
> This patch set adds the driver of mbigen and binding document for Hisilicon
> Mbigen chips.

[...]

I've reworked the nits noticed by Mark and stashed this in my
irq/wire-msi-bridge branch.

Thanks,

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

2015-12-18 12:32:01

by Hanjun Guo

[permalink] [raw]

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

Hi Marc, Mark,

On 2015/12/18 19:58, Marc Zyngier wrote:
> On 17/12/15 11:56, MaJun wrote:
>> From: Ma Jun <[email protected]>
>>
>> This patch set adds the driver of mbigen and binding document for Hisilicon
>> Mbigen chips.
> [...]
>
> I've reworked the nits noticed by Mark and stashed this in my
> irq/wire-msi-bridge branch.

I'm also following this patch set and I checked your tree in irq/wire-msi-bridge
branch, especially with the "num-pins" property/driver update, I think the code
in your tree is in good shape, thanks a lot for the review!

Thanks
Hanjun

2015-12-18 14:03:52