This patch set is applied to supprot the mbigen device.
Mbigen means Message Based Interrupt Generator.
Its a kind of interrupt controller 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 enought for so many perpherals.
Mbigen is designed to fix this problem.
Mbigen chip locates in ITS or outside of ITS.
The working flow of Mbigen shows as below:
external devices ------> MBIGEN ------->ITS
The devices connect to Mbigen chip through wire connecting way.
Mbigen detects and collectes the interrupts from the these devices.
Then, Mbigen generats the MBI interrupts by writting the ITS
Translator register.
Ma Jun (3):
Add mbigen driver to support mbigen interrupt controller
Change arm-gic-its to support the Mbigen interrupt
dt-binding:Documents the mbigen bindings
Documentation/devicetree/bindings/arm/mbigen.txt | 65 +++
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v3-its.c | 60 ++-
drivers/irqchip/irq-mbigen.c | 486 ++++++++++++++++++++++
include/linux/mbi.h | 15 +
6 files changed, 615 insertions(+), 20 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/mbigen.txt
create mode 100644 drivers/irqchip/irq-mbigen.c
create mode 100644 include/linux/mbi.h
This patch contains the mbigen interrupt controller driver.
To support Mbigen device, irq-mbigen.c and mbi.h are added.
As a kind of MSI interrupt controller, the mbigen is used as a child
domain of ITS domain just like PCI devices.
In this patch:
[1]: Create the Mbigen domain as a child domain of ITS according to the
Mbigen device node definition in dts file
[2]: Parse the interrupts of devices(connected to mbigen chip) node which defined in dts file
[3]: other operations of interrupts: mask,unmask,activate..
Signed-off-by: Ma Jun <[email protected]>
---
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-mbigen.c | 486 ++++++++++++++++++++++++++++++++++++++++++
include/linux/mbi.h | 15 ++
4 files changed, 510 insertions(+), 0 deletions(-)
create mode 100644 drivers/irqchip/irq-mbigen.c
create mode 100644 include/linux/mbi.h
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 6de62a9..bb70af7 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 MBIGEN_IRQ_DOMAIN
+ bool "Support mbigen interrupt controller"
+ default y
+ depends on ARM_GIC_V3 && ARM_GIC_V3_ITS
+ help
+ Enable the mbigen interrupt controller used on
+ Hisillicon platform.
+
config ARM_NVIC
bool
select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index dda4927..23571c1 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -23,6 +23,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
+obj-$(CONFIG_MBIGEN_IRQ_DOMAIN) += 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..25f1442
--- /dev/null
+++ b/drivers/irqchip/irq-mbigen.c
@@ -0,0 +1,486 @@
+/*
+ * Copyright (C) 2014 Hisilicon Limited, All Rights Reserved.
+ * Author: Yun Wu <[email protected]>
+ * Author: Jun Ma <[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/init.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/mbi.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include "irqchip.h"
+
+/* Irq numbers per mbigen node supported */
+#define IRQS_PER_MBIGEN_NODE 128
+/* Max mbigen node number in one chip */
+#define MG_NR (10)
+/* Max interrupts Mbigen chip supported */
+#define MG_NR_IRQS IRQS_PER_MBIGEN_NODE * (MG_NR + 1)
+
+#define DEV_SHIFT (10)
+#define COMPOSE_HWIRQ(x, y) (((x) << DEV_SHIFT) | (y))
+#define HWIRQ_OFFSET(x) ((x) & 0x3ff)
+#define GET_NODE_NUM(x) (((x) >> DEV_SHIFT) & 0xff)
+
+#define IRQ_EVENT_ID_SHIFT (12)
+
+#define IRQ_EVENT_ID_MASK (0x3ff << IRQ_EVENT_ID_SHIFT)
+
+/* mbigen node register range */
+#define MBIGEN_NODE_OFFSET 0x1000
+/* vector register offset in mbigen node */
+#define REG_MBIGEN_VEC_OFFSET 0x200
+/* interrupt type register offset */
+#define REG_MBIGEN_TYPE_OFFSET 0x0
+
+/* get the vector register addr in mbigne node
+ * x: mbigen node number
+ * y: the irq pin offset
+ */
+#define MBIGEN_NODE_ADDR_BASE(x) ((x) * MBIGEN_NODE_OFFSET)
+
+#define MBIGEN_VEC_REG_ADDR(x, y) \
+ (MBIGEN_NODE_ADDR_BASE(x) + REG_MBIGEN_VEC_OFFSET + ((y) * 4))
+
+#define MBIGEN_TYPE_REG_ADDR(x, y) \
+ (MBIGEN_NODE_ADDR_BASE(x) + REG_MBIGEN_TYPE_OFFSET + y)
+
+/**
+ * strutct mbigen_chip - mbigen chip structure descriptor
+ * usually one subsys(ex.DSA,ALG,PCIE)has one mbigen chip
+ */
+struct mbigen_chip {
+ raw_spinlock_t lock;
+ struct list_head entry;
+ struct device *dev;
+ struct device_node *node;
+ void __iomem *base;
+ struct irq_domain *domain;
+ struct list_head nodes;
+};
+
+/*
+ * mbigen_node: structure of mbigen node in a mbigen chip
+ * usually, a mbigen chip includes 8 ~ 11 mbigen nodes.
+ * The node number depends on the device number connected
+ * to this mbigen chip.
+ * @nid: the mbigen nod number
+ */
+struct mbigen_node {
+ raw_spinlock_t lock;
+ struct list_head entry;
+ struct mbigen_chip *chip;
+ unsigned int nid;
+ struct list_head nodes;
+};
+
+/* refer to the devices connected to mbigen node */
+struct mbigen_device {
+ struct list_head entry;
+ struct mbigen_node *mgn_node;
+ struct device_node *source;
+ unsigned int irq;
+ unsigned int nr_irqs;
+ unsigned int offset;
+};
+
+/**
+ * struct mbi_desc - Message Based Interrupt (MBI) descriptor
+ *
+ * @dev: the device owned the MBI
+ * @msg_id: identifier of the message group
+ * @lines: max number of interrupts supported by the message register
+ * @irq: base linux interrupt number of the MBI
+ * @nvec: number of interrupts controlled by the MBI
+ * @data: message specific data
+ */
+struct mbi_desc {
+ struct device *dev;
+ int msg_id;
+ unsigned int lines;
+ unsigned int irq;
+ unsigned int nvec;
+ void *data;
+};
+
+static LIST_HEAD(mbigen_chip_list);
+static DEFINE_SPINLOCK(mbigen_lock);
+
+static void mbigen_free_dev(struct mbigen_device *mgn_dev)
+{
+ raw_spin_lock(&mgn_dev->mgn_node->lock);
+ list_del(&mgn_dev->entry);
+ raw_spin_unlock(&mgn_dev->mgn_node->lock);
+ kfree(mgn_dev);
+}
+
+static struct mbigen_device *mbigen_create_device(struct mbigen_node *mgn_node,
+ struct device_node *node,
+ unsigned int virq,
+ unsigned int nr_irqs)
+{
+ struct mbigen_device *mgn_dev;
+
+ mgn_dev = kzalloc(sizeof(*mgn_dev), GFP_KERNEL);
+ if (!mgn_dev)
+ return NULL;
+
+ INIT_LIST_HEAD(&mgn_dev->entry);
+ mgn_dev->mgn_node = mgn_node;
+ mgn_dev->source = node;
+ mgn_dev->irq = virq;
+ mgn_dev->nr_irqs = nr_irqs;
+
+ raw_spin_lock(&mgn_node->lock);
+ list_add(&mgn_dev->entry, &mgn_node->nodes);
+ raw_spin_unlock(&mgn_node->lock);
+ return mgn_dev;
+}
+
+static struct mbigen_node *get_mbigen_node(struct mbigen_chip *chip,
+ unsigned int nid)
+{
+ struct mbigen_node *tmp, *mbigen;
+ bool found = false;
+
+ if (nid > MG_NR) {
+ pr_warn("MBIGEN: Device ID exceeds max number!!\n");
+ return NULL;
+ }
+
+ list_for_each_entry(mbigen, &chip->nodes, entry) {
+ if (mbigen->nid == nid) {
+ found = true;
+ return mbigen;
+ }
+ }
+
+ /*
+ * Stop working if no memory available, even if we could
+ * get what we want.
+ */
+ tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return NULL;
+
+ raw_spin_lock(&chip->lock);
+
+ tmp->chip = chip;
+ tmp->nid = nid;
+ raw_spin_lock_init(&tmp->lock);
+ INIT_LIST_HEAD(&tmp->entry);
+ INIT_LIST_HEAD(&tmp->nodes);
+
+ list_add(&tmp->entry, &chip->nodes);
+ mbigen = tmp;
+ raw_spin_unlock(&chip->lock);
+
+ return mbigen;
+}
+
+/**
+ * get_mbigen_node_type: get the mbigen node type
+ * @nid: the mbigen node value
+ * return 0: evnent id of interrupt connected to this node can be changed.
+ * return 1: evnent id of interrupt connected to this node cant be changed.
+ */
+static int get_mbigen_node_type(int nid)
+{
+ if (nid > MG_NR) {
+ pr_warn("MBIGEN: Device ID exceeds max number!\n");
+ return 1;
+ }
+ if ((nid == 0) || (nid == 5) || (nid > 7))
+ return 0;
+ else
+ return 1;
+}
+
+static int mbigen_write_msg(struct irq_data *d, struct msi_msg *msg)
+{
+ struct mbigen_chip *chip = d->domain->host_data;
+ void __iomem *addr;
+ u32 nid, val, offset;
+ int ret = 0;
+
+ nid = GET_NODE_NUM(d->hwirq);
+ ret = get_mbigen_node_type(nid);
+ if (ret)
+ return 0;
+
+ offset = HWIRQ_OFFSET(d->hwirq);
+
+ addr = chip->base + MBIGEN_VEC_REG_ADDR(nid, offset);
+
+ val = readl_relaxed(addr);
+
+ val &= ~IRQ_EVENT_ID_MASK;
+ val |= (msg->data << IRQ_EVENT_ID_SHIFT);
+
+ writel_relaxed(val, addr);
+ return ret;
+}
+
+/*
+ * Interrupt controller operations
+ */
+static int mbigen_set_type(struct irq_data *d, unsigned int type)
+{
+ struct mbigen_chip *chip = d->domain->host_data;
+ u32 ofst, mask;
+ u32 val, nid, hwirq;
+ void __iomem *addr;
+
+ if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
+ return -EINVAL;
+
+ nid = GET_NODE_NUM(d->hwirq);
+ hwirq = HWIRQ_OFFSET(d->hwirq);
+
+ ofst = hwirq / 32 * 4;
+ mask = 1 << (hwirq % 32);
+
+ addr = chip->base + MBIGEN_TYPE_REG_ADDR(nid, ofst);
+ raw_spin_lock(&chip->lock);
+ val = readl_relaxed(addr);
+
+ if (type == IRQ_TYPE_LEVEL_HIGH)
+ val |= mask;
+ else if (type == IRQ_TYPE_EDGE_RISING)
+ val &= ~mask;
+
+ writel_relaxed(val, addr);
+ raw_spin_unlock(&chip->lock);
+
+ return 0;
+}
+
+static void mbigen_mask_irq(struct irq_data *data)
+{
+ irq_chip_mask_parent(data);
+}
+
+static void mbigen_unmask_irq(struct irq_data *data)
+{
+ irq_chip_unmask_parent(data);
+}
+
+static int mbigen_set_affinity(struct irq_data *data,
+ const struct cpumask *mask,
+ bool force)
+{
+ int ret;
+
+ ret = irq_chip_set_affinity_parent(data, mask, force);
+ return ret;
+}
+
+static void mbigen_irq_eoi(struct irq_data *d)
+{
+ irq_chip_eoi_parent(d);
+}
+
+static struct irq_chip mbigen_irq_chip = {
+ .name = "MBIGEN-v2",
+ .irq_mask = mbigen_mask_irq,
+ .irq_unmask = mbigen_unmask_irq,
+ .irq_eoi = mbigen_irq_eoi,
+ .irq_set_affinity = mbigen_set_affinity,
+ .irq_set_type = mbigen_set_type,
+};
+
+/*
+ * Interrupt domain operations
+ */
+
+static int mbigen_domain_xlate(struct irq_domain *d,
+ struct device_node *controller,
+ const u32 *intspec, unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+
+ if (d->of_node != controller)
+ return -EINVAL;
+
+ if (intsize < 4)
+ return -EINVAL;
+
+ *out_hwirq = COMPOSE_HWIRQ(intspec[3], intspec[2]);
+
+ *out_type = intspec[4] & IRQ_TYPE_SENSE_MASK;
+
+ return 0;
+}
+
+static int mbigen_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ struct mbigen_chip *chip = domain->host_data;
+ struct of_phandle_args *irq_data = arg;
+ irq_hw_number_t hwirq;
+ u32 nid, dev_id, mbi_lines;
+ struct mbigen_node *mgn_node;
+ struct mbigen_device *mgn_dev;
+ msi_alloc_info_t out_arg;
+ int ret = 0, i;
+
+ /* OF style allocation, one interrupt at a time */
+ WARN_ON(nr_irqs != 1);
+
+ dev_id = irq_data->args[0];
+ nid = irq_data->args[3];
+ hwirq = COMPOSE_HWIRQ(nid, irq_data->args[2]);
+
+ mgn_node = get_mbigen_node(chip, nid);
+ if (!mgn_node)
+ return -ENODEV;
+
+ mgn_dev = mbigen_create_device(mgn_node, irq_data->np, virq, nr_irqs);
+ if (!mgn_dev)
+ return -ENOMEM;
+
+ mbi_lines = irq_data->args[1];
+
+ ret = its_msi_prepare(domain, dev_id, mbi_lines, &out_arg);
+ if (ret)
+ return ret;
+
+ ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &out_arg);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < nr_irqs; i++) {
+ irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+ &mbigen_irq_chip, mgn_dev);
+ }
+
+ return ret;
+}
+
+static void mbigen_domain_free(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs)
+{
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+ struct mbigen_device *mgn_dev = irq_data_get_irq_chip_data(d);
+
+ WARN_ON(virq != mgn_dev->irq);
+ WARN_ON(nr_irqs != mgn_dev->nr_irqs);
+ mbigen_free_dev(mgn_dev);
+ irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+}
+
+static void mbigen_domain_activate(struct irq_domain *domain,
+ struct irq_data *irq_data)
+{
+ struct msi_msg msg;
+
+ BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
+ mbigen_write_msg(irq_data, &msg);
+}
+
+static void mbigen_domain_deactivate(struct irq_domain *domain,
+ struct irq_data *irq_data)
+{
+ struct msi_msg msg;
+
+ memset(&msg, 0, sizeof(msg));
+ mbigen_write_msg(irq_data, &msg);
+}
+
+static struct irq_domain_ops mbigen_domain_ops = {
+ .xlate = mbigen_domain_xlate,
+ .alloc = mbigen_domain_alloc,
+ .free = mbigen_domain_free,
+ .activate = mbigen_domain_activate,
+ .deactivate = mbigen_domain_deactivate,
+};
+
+/*
+ * Early initialization as an interrupt controller
+ */
+static int __init mbigen_of_init(struct device_node *node,
+ struct device_node *parent_node)
+{
+ struct mbigen_chip *chip;
+ struct irq_domain *parent_domain;
+ int err;
+
+ parent_node = of_parse_phandle(node, "msi-parent", 0);
+
+ if (!parent_node) {
+ pr_warn("MBIGEN: no ITS node for %s\n", node->full_name);
+ return -ENXIO;
+ }
+
+ parent_domain = get_its_domain(parent_node);
+
+ if (!parent_domain) {
+ pr_warn("MBIGEN: no ITS domain for %s\n", node->full_name);
+ return -ENXIO;
+ }
+
+ chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->base = of_iomap(node, 0);
+ if (!chip->base) {
+ pr_err("%s: Registers not found.\n", node->full_name);
+ err = -ENXIO;
+ goto free_chip;
+ }
+
+ chip->domain = irq_domain_add_hierarchy(parent_domain,
+ 0, MG_NR_IRQS, node,
+ &mbigen_domain_ops, chip);
+
+ if (!chip->domain) {
+ err = -ENOMEM;
+ goto unmap_reg;
+ }
+
+ chip->node = node;
+ raw_spin_lock_init(&chip->lock);
+ INIT_LIST_HEAD(&chip->entry);
+ INIT_LIST_HEAD(&chip->nodes);
+ pr_debug("MBIGEN: %s\n", node->full_name);
+ spin_lock(&mbigen_lock);
+ list_add(&chip->entry, &mbigen_chip_list);
+ spin_unlock(&mbigen_lock);
+
+ return 0;
+
+unmap_reg:
+ iounmap(chip->base);
+free_chip:
+ kfree(chip);
+ pr_warn("MBIGEN: failed probing %s\n", node->full_name);
+ return err;
+}
+IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);
+
+MODULE_AUTHOR("Jun Ma <[email protected]>");
+MODULE_AUTHOR("Yun Wu <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Hisilicon MBI Generator driver");
diff --git a/include/linux/mbi.h b/include/linux/mbi.h
new file mode 100644
index 0000000..d3b8155
--- /dev/null
+++ b/include/linux/mbi.h
@@ -0,0 +1,15 @@
+#ifndef _LINUX_MBI_H
+#define _LINUX_MBI_H
+
+#include <linux/device.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+
+
+/* Function to parse and map message interrupts */
+extern int its_msi_prepare(struct irq_domain *domain, u32 dev_id,
+ int nvec, msi_alloc_info_t *info);
+extern struct irq_domain *get_its_domain(struct device_node *node);
+
+#endif /* _LINUX_MBI_H */
--
1.7.1
This patch is applied to support the mbigen interrupt.
As a kind of MSI interrupt controller, the mbigen is used as a child
domain of ITS domain just like PCI devices.
To support the mbigen driver, changes in its file list below:
Change in v3:
---Splitting the its_pci_msi_prepare fucntion into two functions: it_pci_msi_prepare and
its_msi_prepare. So its_msi_prepare can be called from mbigen driver to create its
device.
This change based on the code from Marc. Thanks
---Add a function get_its_domain for mbigen driver using to the get pointer of its domain.
Change in v2:
---arm-gic-v3-its.c: [1]Create the mbigen irq domain in its_init;
[2] add the irq_compose_mbigen_msg and irq_write_mbigen_msg
for mbigen using.[3]add its_mbigen_prepare function.
---irq.h: Add 'irq_compose_mbigen_msg' and 'irq_write_mbigen_msg' in
struct irq_chip for mbigen using.
---chip.c: Before the irq_ack callback, check the irq_ack first(chip.c)
Signed-off-by: Ma Jun <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 60 +++++++++++++++++++++++++------------
1 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 9687f8a..1cb8c20 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1193,49 +1193,55 @@ static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
return 0;
}
-static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
+int its_msi_prepare(struct irq_domain *domain, u32 dev_id,
int nvec, msi_alloc_info_t *info)
{
- struct pci_dev *pdev;
struct its_node *its;
struct its_device *its_dev;
- struct its_pci_alias dev_alias;
-
- if (!dev_is_pci(dev))
- return -EINVAL;
-
- pdev = to_pci_dev(dev);
- dev_alias.pdev = pdev;
- dev_alias.count = nvec;
- pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
its = domain->parent->host_data;
+ its_dev = its_find_device(its, dev_id);
- its_dev = its_find_device(its, dev_alias.dev_id);
if (its_dev) {
/*
* We already have seen this ID, probably through
* another alias (PCI bridge of some sort). No need to
* create the device.
*/
- dev_dbg(dev, "Reusing ITT for devID %x\n", dev_alias.dev_id);
+ pr_debug("Reusing ITT for devID %x\n", dev_id);
goto out;
}
- its_dev = its_create_device(its, dev_alias.dev_id, dev_alias.count);
+ its_dev = its_create_device(its, dev_id, nvec);
if (!its_dev)
return -ENOMEM;
- dev_dbg(&pdev->dev, "ITT %d entries, %d bits\n",
- dev_alias.count, ilog2(dev_alias.count));
+ pr_debug("ITT %d entries, %d bits\n", nvec, ilog2(nvec));
out:
info->scratchpad[0].ptr = its_dev;
- info->scratchpad[1].ptr = dev;
return 0;
}
+static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
+ int nvec, msi_alloc_info_t *info)
+{
+ struct pci_dev *pdev;
+ struct its_pci_alias dev_alias;
+
+ if (!dev_is_pci(dev))
+ return -EINVAL;
+
+ pdev = to_pci_dev(dev);
+ dev_alias.pdev = pdev;
+ dev_alias.count = nvec;
+
+ pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
+
+ return its_msi_prepare(domain, dev_alias.dev_id, dev_alias.count, info);
+}
+
static struct msi_domain_ops its_pci_msi_ops = {
- .msi_prepare = its_msi_prepare,
+ .msi_prepare = its_pci_msi_prepare,
};
static struct msi_domain_info its_pci_msi_domain_info = {
@@ -1280,8 +1286,10 @@ static int its_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
irq_domain_set_hwirq_and_chip(domain, virq + i,
hwirq, &its_irq_chip, its_dev);
- dev_dbg(info->scratchpad[1].ptr, "ID:%d pID:%d vID:%d\n",
- (int)(hwirq - its_dev->lpi_base), (int)hwirq, virq + i);
+
+ pr_debug("ID:%d pID:%d vID:%d\n",
+ (int)(hwirq - its_dev->lpi_base),
+ (int)hwirq, virq + i);
}
return 0;
@@ -1375,6 +1383,18 @@ static int its_force_quiescent(void __iomem *base)
}
}
+struct irq_domain *get_its_domain(struct device_node *node)
+{
+ struct its_node *its = NULL;
+
+ list_for_each_entry(its, &its_nodes, entry) {
+ if (its->msi_chip.of_node == node)
+ break;
+ }
+
+ return (its)?its->domain:NULL;
+}
+
static int its_probe(struct device_node *node, struct irq_domain *parent)
{
struct resource res;
--
1.7.1
Add the mbigen msi interrupt controller bindings document
Change in v3:
---Change the compatible string
---Change the interrupt cells definition.
Signed-off-by: Ma Jun <[email protected]>
---
Documentation/devicetree/bindings/arm/mbigen.txt | 65 ++++++++++++++++++++++
1 files changed, 65 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..cf92ef8
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mbigen.txt
@@ -0,0 +1,65 @@
+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 gnerate the inteerrupt
+by wirtting ITS register.
+
+The mbigen and devices connect to mbigen have the following properties:
+
+
+Mbigen required properties:
+-------------------------------------------
+-compatible: Should be "hisilicon,mbigen-v2"
+-msi-parent: should specified the ITS mbigen connected
+-interrupt controller: Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value is 5 now.
+
+ The 1st cell is the device id.
+ The 2nd cell is the totall interrupt number of this device
+ The 3rd cell is the hardware pin number of the interrupt.
+ This value depends on the Soc design.
+ The 4th cell is the mbigen node number. This value should refer to the
+ vendor soc specification.
+ The 5th cell is the interrupt trigger type, encoded as follows:
+ 1 = edge triggered
+ 4 = level triggered
+
+- reg: Specifies the base physical address and size of the ITS
+ registers.
+
+Examples:
+
+ mbigen_dsa: interrupt-controller@c0080000 {
+ compatible = "hisilicon,mbigen-v2";
+ msi-parent = <&its_dsa>;
+ interrupt-controller;
+ #interrupt-cells = <5>;
+ reg = <0xc0080000 0x10000>;
+ };
+
+Device connect to mbigen required properties:
+----------------------------------------------------
+-interrupt-parent: Specifies the mbigen node which device connected.
+-interrupts:specifies the interrupt source.The first cell is hwirq num, the
+ second number is trigger type.
+
+Examples:
+ smmu_dsa {
+ compatible = "arm,smmu-v3";
+ reg = <0x0 0xc0040000 0x0 0x20000>;
+ interrupt-parent = <&mbigen_dsa>;
+ interrupts = <0x40b20 3 78 6 1>,
+ <0x40b20 3 79 6 1>,
+ <0x40b20 3 80 6 1>;
+ smmu-cb-memtype = <0x0 0x1>;
+ };
+
--
1.7.1
On Mon, 6 Jul 2015, Ma Jun wrote:
> This patch contains the mbigen interrupt controller driver.
>
> To support Mbigen device, irq-mbigen.c and mbi.h are added.
>
> As a kind of MSI interrupt controller, the mbigen is used as a child
> domain of ITS domain just like PCI devices.
>
> In this patch:
> [1]: Create the Mbigen domain as a child domain of ITS according to the
> Mbigen device node definition in dts file
> [2]: Parse the interrupts of devices(connected to mbigen chip) node which defined in dts file
> [3]: other operations of interrupts: mask,unmask,activate..
This is not a proper changelog. We can see which files are added and
modified.
What's missing is a proper description of MBI and how it's connected
to ITS.
Also the patches are in the wrong order. This one uses a function
which does not exist yet plus non existant device tree entries ....
> +config MBIGEN_IRQ_DOMAIN
The config symbol should reflect the functionality
HISILICON_IRQ_MBIGEN
would be a more descriptive on, right?
> + bool "Support mbigen interrupt controller"
> + default y
Why would this be default Y? Nothing needs that stuff except hisilicon
platforms.
> + depends on ARM_GIC_V3 && ARM_GIC_V3_ITS
> + help
> + Enable the mbigen interrupt controller used on
> + Hisillicon platform.
Looks like a typo. Should that be Hisillycon perhaps?
> +/* Irq numbers per mbigen node supported */
> +#define IRQS_PER_MBIGEN_NODE 128
> +/* Max mbigen node number in one chip */
> +#define MG_NR (10)
That define sucks. You have IRQS_PER_MBIGEN_NODE above so why not
using a descriptive one for this as well?
MBIGEN_NODES_PER_CHIP or something like that?
Also please use a proper name space. XXX_MBIGEN_ / MBIGEN_XXX / MB_XX
... looks just like created by a random generator.
> +/* Max interrupts Mbigen chip supported */
> +#define MG_NR_IRQS IRQS_PER_MBIGEN_NODE * (MG_NR + 1)
Why is this NODES per SOC plus 1? That's either wrong or the whole
logic of this define chain is wrong.
> +#define DEV_SHIFT (10)
> +#define COMPOSE_HWIRQ(x, y) (((x) << DEV_SHIFT) | (y))
> +#define HWIRQ_OFFSET(x) ((x) & 0x3ff)
Magic hex constants pulled from thin air?
> +#define GET_NODE_NUM(x) (((x) >> DEV_SHIFT) & 0xff)
Can you please use consistant spacing (tabs) for everything?
> +
> +#define IRQ_EVENT_ID_SHIFT (12)
> +
> +#define IRQ_EVENT_ID_MASK (0x3ff << IRQ_EVENT_ID_SHIFT)
More magic constants without explanation.
> +/* mbigen node register range */
> +#define MBIGEN_NODE_OFFSET 0x1000
> +/* vector register offset in mbigen node */
> +#define REG_MBIGEN_VEC_OFFSET 0x200
> +/* interrupt type register offset */
> +#define REG_MBIGEN_TYPE_OFFSET 0x0
> +
> +/* get the vector register addr in mbigne node
mbigne? Is that another variant?
> + * x: mbigen node number
> + * y: the irq pin offset
> + */
> +#define MBIGEN_NODE_ADDR_BASE(x) ((x) * MBIGEN_NODE_OFFSET)
> +
> +#define MBIGEN_VEC_REG_ADDR(x, y) \
> + (MBIGEN_NODE_ADDR_BASE(x) + REG_MBIGEN_VEC_OFFSET + ((y) * 4))
> +
> +#define MBIGEN_TYPE_REG_ADDR(x, y) \
> + (MBIGEN_NODE_ADDR_BASE(x) + REG_MBIGEN_TYPE_OFFSET + y)
These macros can be implemented cleanly as readable inline functions.
> +/**
> + * strutct mbigen_chip - mbigen chip structure descriptor
Broken kerneldoc comment. Also missing the struct member documentation
> + * usually one subsys(ex.DSA,ALG,PCIE)has one mbigen chip
Please use proper sentences.
> + */
> +struct mbigen_chip {
> + raw_spinlock_t lock;
> + struct list_head entry;
> + struct device *dev;
> + struct device_node *node;
> + void __iomem *base;
> + struct irq_domain *domain;
> + struct list_head nodes;
> +};
> +
> +/*
> + * mbigen_node: structure of mbigen node in a mbigen chip
> + * usually, a mbigen chip includes 8 ~ 11 mbigen nodes.
> + * The node number depends on the device number connected
> + * to this mbigen chip.
> + * @nid: the mbigen nod number
More broken comment.
> + */
> +struct mbigen_node {
> + raw_spinlock_t lock;
> + struct list_head entry;
> + struct mbigen_chip *chip;
> + unsigned int nid;
> + struct list_head nodes;
> +};
> +
> +/* refer to the devices connected to mbigen node */
Completely useless explanation of the data structure
> +struct mbigen_device {
> + struct list_head entry;
> + struct mbigen_node *mgn_node;
> + struct device_node *source;
> + unsigned int irq;
> + unsigned int nr_irqs;
> + unsigned int offset;
> +};
> +
> +/**
> + * struct mbi_desc - Message Based Interrupt (MBI) descriptor
> + *
> + * @dev: the device owned the MBI
> + * @msg_id: identifier of the message group
> + * @lines: max number of interrupts supported by the message register
> + * @irq: base linux interrupt number of the MBI
> + * @nvec: number of interrupts controlled by the MBI
> + * @data: message specific data
> + */
> +struct mbi_desc {
> + struct device *dev;
> + int msg_id;
> + unsigned int lines;
Please align the struct members proper.
> + unsigned int irq;
> + unsigned int nvec;
> + void *data;
Why is this a void pointer if that is message specific data?
> +};
> +
> +static LIST_HEAD(mbigen_chip_list);
> +static DEFINE_SPINLOCK(mbigen_lock);
What is the lock protecting and why does it need to be a spinlock? The
code completely lacks an explanation of the locking rules and the lock
nesting rules.
> +
> +static void mbigen_free_dev(struct mbigen_device *mgn_dev)
> +{
> + raw_spin_lock(&mgn_dev->mgn_node->lock);
> + list_del(&mgn_dev->entry);
> + raw_spin_unlock(&mgn_dev->mgn_node->lock);
> + kfree(mgn_dev);
> +}
> +
> +static struct mbigen_device *mbigen_create_device(struct mbigen_node *mgn_node,
> + struct device_node *node,
> + unsigned int virq,
> + unsigned int nr_irqs)
Your source formating is based on /dev/random, right?
> +{
> + struct mbigen_device *mgn_dev;
> +
> + mgn_dev = kzalloc(sizeof(*mgn_dev), GFP_KERNEL);
> + if (!mgn_dev)
> + return NULL;
> +
> + INIT_LIST_HEAD(&mgn_dev->entry);
> + mgn_dev->mgn_node = mgn_node;
> + mgn_dev->source = node;
> + mgn_dev->irq = virq;
> + mgn_dev->nr_irqs = nr_irqs;
> +
> + raw_spin_lock(&mgn_node->lock);
> + list_add(&mgn_dev->entry, &mgn_node->nodes);
> + raw_spin_unlock(&mgn_node->lock);
> + return mgn_dev;
> +}
> +
> +static struct mbigen_node *get_mbigen_node(struct mbigen_chip *chip,
> + unsigned int nid)
> +{
> + struct mbigen_node *tmp, *mbigen;
> + bool found = false;
> +
> + if (nid > MG_NR) {
> + pr_warn("MBIGEN: Device ID exceeds max number!!\n");
> + return NULL;
> + }
> +
> + list_for_each_entry(mbigen, &chip->nodes, entry) {
> + if (mbigen->nid == nid) {
> + found = true;
> + return mbigen;
> + }
> + }
That list walk does not need locking?
> + /*
> + * Stop working if no memory available, even if we could
> + * get what we want.
> + */
> + tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
tmp is a pretty useless variable name. Why cant you use mbigen for
that? Just because it makes the code harder to read, right?
> + if (!tmp)
> + return NULL;
> +
> + raw_spin_lock(&chip->lock);
Your lock scopes are completely random. Why do you need to protect the
initialization of tmp?
> + tmp->chip = chip;
> + tmp->nid = nid;
> + raw_spin_lock_init(&tmp->lock);
> + INIT_LIST_HEAD(&tmp->entry);
> + INIT_LIST_HEAD(&tmp->nodes);
> +
> + list_add(&tmp->entry, &chip->nodes);
> + mbigen = tmp;
> + raw_spin_unlock(&chip->lock);
> +
> + return mbigen;
> +}
> +
> +/**
> + * get_mbigen_node_type: get the mbigen node type
> + * @nid: the mbigen node value
> + * return 0: evnent id of interrupt connected to this node can be changed.
> + * return 1: evnent id of interrupt connected to this node cant be changed.
> + */
> +static int get_mbigen_node_type(int nid)
> +{
> + if (nid > MG_NR) {
> + pr_warn("MBIGEN: Device ID exceeds max number!\n");
> + return 1;
> + }
> + if ((nid == 0) || (nid == 5) || (nid > 7))
> + return 0;
> + else
> + return 1;
Oh no. We do not hardcode such properties into a driver. That wants to
be in the device tree and set as a property in the node data structure.
> +static int mbigen_write_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> + struct mbigen_chip *chip = d->domain->host_data;
> + void __iomem *addr;
> + u32 nid, val, offset;
> + int ret = 0;
> +
> + nid = GET_NODE_NUM(d->hwirq);
> + ret = get_mbigen_node_type(nid);
> + if (ret)
> + return 0;
Care to explain what this does? It seems for some nodes you cannot
write the msi message. So how is that supposed to work? How is that
interrupt controlled (mask/unmask ...) ?
> + offset = HWIRQ_OFFSET(d->hwirq);
> +
> + addr = chip->base + MBIGEN_VEC_REG_ADDR(nid, offset);
> +
> + val = readl_relaxed(addr);
> +
> + val &= ~IRQ_EVENT_ID_MASK;
> + val |= (msg->data << IRQ_EVENT_ID_SHIFT);
> +
> + writel_relaxed(val, addr);
> + return ret;
> +}
> +
> +/*
> + * Interrupt controller operations
> + */
> +static int mbigen_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct mbigen_chip *chip = d->domain->host_data;
> + u32 ofst, mask;
> + u32 val, nid, hwirq;
> + void __iomem *addr;
> +
> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> + return -EINVAL;
> +
> + nid = GET_NODE_NUM(d->hwirq);
> + hwirq = HWIRQ_OFFSET(d->hwirq);
> +
> + ofst = hwirq / 32 * 4;
> + mask = 1 << (hwirq % 32);
> +
> + addr = chip->base + MBIGEN_TYPE_REG_ADDR(nid, ofst);
> + raw_spin_lock(&chip->lock);
> + val = readl_relaxed(addr);
> +
> + if (type == IRQ_TYPE_LEVEL_HIGH)
> + val |= mask;
> + else if (type == IRQ_TYPE_EDGE_RISING)
> + val &= ~mask;
> +
> + writel_relaxed(val, addr);
> + raw_spin_unlock(&chip->lock);
What is the lock protecting here? The read/write access to a per
interrupt register? Why is the per interrupt descriptor lock not
sufficient and why does the above write_msg not requited locking?
> + return 0;
> +}
> +
> +static void mbigen_mask_irq(struct irq_data *data)
> +{
> + irq_chip_mask_parent(data);
> +}
> +
> +static void mbigen_unmask_irq(struct irq_data *data)
> +{
> + irq_chip_unmask_parent(data);
> +}
> +
> +static int mbigen_set_affinity(struct irq_data *data,
> + const struct cpumask *mask,
> + bool force)
> +{
> + int ret;
> +
> + ret = irq_chip_set_affinity_parent(data, mask, force);
> + return ret;
> +}
> +
> +static void mbigen_irq_eoi(struct irq_data *d)
> +{
> + irq_chip_eoi_parent(d);
> +}
What's the purpose of these wrappers? Why is setting the chip
functions to irq_chip_*_parent not sufficient?
> +static struct irq_chip mbigen_irq_chip = {
> + .name = "MBIGEN-v2",
> + .irq_mask = mbigen_mask_irq,
> + .irq_unmask = mbigen_unmask_irq,
> + .irq_eoi = mbigen_irq_eoi,
> + .irq_set_affinity = mbigen_set_affinity,
> + .irq_set_type = mbigen_set_type,
> +};
> +static int mbigen_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + struct mbigen_chip *chip = domain->host_data;
> + struct of_phandle_args *irq_data = arg;
> + irq_hw_number_t hwirq;
> + u32 nid, dev_id, mbi_lines;
> + struct mbigen_node *mgn_node;
> + struct mbigen_device *mgn_dev;
> + msi_alloc_info_t out_arg;
> + int ret = 0, i;
> +
> + /* OF style allocation, one interrupt at a time */
-ENOPARSE
> + WARN_ON(nr_irqs != 1);
> +
> + dev_id = irq_data->args[0];
> + nid = irq_data->args[3];
> + hwirq = COMPOSE_HWIRQ(nid, irq_data->args[2]);
> +
> + mgn_node = get_mbigen_node(chip, nid);
> + if (!mgn_node)
> + return -ENODEV;
> +
> + mgn_dev = mbigen_create_device(mgn_node, irq_data->np, virq, nr_irqs);
> + if (!mgn_dev)
> + return -ENOMEM;
Leaks the node allocation.
> +
> + mbi_lines = irq_data->args[1];
> +
> + ret = its_msi_prepare(domain, dev_id, mbi_lines, &out_arg);
This looks wrong. Why do you have an explicit call for this in the
allocation function?
msi_domain_ops.msi_prepare is called from the core code and you should
provide a msi_prepare callback which does the necessary initialization
and invokes the parent domains msi_prepare callback.
> + if (ret)
> + return ret;
Leaks the node allocation and the device.
> +
> + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &out_arg);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < nr_irqs; i++) {
This loop is required because?
> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> + &mbigen_irq_chip, mgn_dev);
> + }
> +
> + return ret;
> +/*
> + * Early initialization as an interrupt controller
> + */
> +static int __init mbigen_of_init(struct device_node *node,
> + struct device_node *parent_node)
> +{
> + struct mbigen_chip *chip;
> + struct irq_domain *parent_domain;
> + int err;
> +
> + parent_node = of_parse_phandle(node, "msi-parent", 0);
Huch. parent node is an argument here. So WHY do you need to override
it with some magic parent entry in the mbigen node? Seems your
devicetree design sucks.
> diff --git a/include/linux/mbi.h b/include/linux/mbi.h
> new file mode 100644
> index 0000000..d3b8155
> --- /dev/null
> +++ b/include/linux/mbi.h
> +
> +/* Function to parse and map message interrupts */
> +extern int its_msi_prepare(struct irq_domain *domain, u32 dev_id,
> + int nvec, msi_alloc_info_t *info);
> +extern struct irq_domain *get_its_domain(struct device_node *node);
Crap in various aspects
- these functions should only be visible from drivers/irqchip/
- the header name is wrong as it does not provide any MBI
specific functionality
Thanks,
tglx
Hi Thomas:
在 2015/7/6 20:33, Thomas Gleixner 写道:
> On Mon, 6 Jul 2015, Ma Jun wrote:
>
>> +/**
>> + * get_mbigen_node_type: get the mbigen node type
>> + * @nid: the mbigen node value
>> + * return 0: evnent id of interrupt connected to this node can be changed.
>> + * return 1: evnent id of interrupt connected to this node cant be changed.
>> + */
>> +static int get_mbigen_node_type(int nid)
>> +{
>> + if (nid > MG_NR) {
>> + pr_warn("MBIGEN: Device ID exceeds max number!\n");
>> + return 1;
>> + }
>> + if ((nid == 0) || (nid == 5) || (nid > 7))
>> + return 0;
>> + else
>> + return 1;
>
> Oh no. We do not hardcode such properties into a driver. That wants to
> be in the device tree and set as a property in the node data structure.
>
Ok,I will move this to device tree
>> +static int mbigen_write_msg(struct irq_data *d, struct msi_msg *msg)
>> +{
>> + struct mbigen_chip *chip = d->domain->host_data;
>> + void __iomem *addr;
>> + u32 nid, val, offset;
>> + int ret = 0;
>> +
>> + nid = GET_NODE_NUM(d->hwirq);
>> + ret = get_mbigen_node_type(nid);
>> + if (ret)
>> + return 0;
>
> Care to explain what this does? It seems for some nodes you cannot
> write the msi message. So how is that supposed to work? How is that
> interrupt controlled (mask/unmask ...) ?
>
This function is used to write irq event id into vector register.Depends on
hardware design, write operation is permitted in some mbigen node(nid=0,5,and >7),
For other mbigen node, this register is read only.
But only vector register has this problem. Other registers are ok for read/write.
>> +
>> + ofst = hwirq / 32 * 4;
>> + mask = 1 << (hwirq % 32);
>> +
>> + addr = chip->base + MBIGEN_TYPE_REG_ADDR(nid, ofst);
>> + raw_spin_lock(&chip->lock);
>> + val = readl_relaxed(addr);
>> +
>> + if (type == IRQ_TYPE_LEVEL_HIGH)
>> + val |= mask;
>> + else if (type == IRQ_TYPE_EDGE_RISING)
>> + val &= ~mask;
>> +
>> + writel_relaxed(val, addr);
>> + raw_spin_unlock(&chip->lock);
>
> What is the lock protecting here? The read/write access to a per
> interrupt register? Why is the per interrupt descriptor lock not
> sufficient and why does the above write_msg not requited locking?
>
yes,lock protecting is not necessary here.
>> + return 0;
[...]
>> +static int mbigen_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> + unsigned int nr_irqs, void *arg)
>> +{
>> + struct mbigen_chip *chip = domain->host_data;
>> + struct of_phandle_args *irq_data = arg;
>> + irq_hw_number_t hwirq;
>> + u32 nid, dev_id, mbi_lines;
>> + struct mbigen_node *mgn_node;
>> + struct mbigen_device *mgn_dev;
>> + msi_alloc_info_t out_arg;
>> + int ret = 0, i;
>> +
>> + /* OF style allocation, one interrupt at a time */
>
> -ENOPARSE
>
what's this mean? I didn't find this definition in kernel code
>> + WARN_ON(nr_irqs != 1);
>> +
>> + dev_id = irq_data->args[0];
>> + nid = irq_data->args[3];
>> + hwirq = COMPOSE_HWIRQ(nid, irq_data->args[2]);
>> +
>> + mgn_node = get_mbigen_node(chip, nid);
>> + if (!mgn_node)
>> + return -ENODEV;
>> +
>> + mgn_dev = mbigen_create_device(mgn_node, irq_data->np, virq, nr_irqs);
>> + if (!mgn_dev)
>> + return -ENOMEM;
>
> Leaks the node allocation.
>
>> +
>> + mbi_lines = irq_data->args[1];
>> +
>> + ret = its_msi_prepare(domain, dev_id, mbi_lines, &out_arg);
>
> This looks wrong. Why do you have an explicit call for this in the
> allocation function?
>
> msi_domain_ops.msi_prepare is called from the core code and you should
> provide a msi_prepare callback which does the necessary initialization
> and invokes the parent domains msi_prepare callback.
>
According to Marc suggestion, I changed the ITS code so I can use its_msi_prepare
function in my code.
So,do you mean i should not call this function directly ?
How about make this code likes below in mbigen driver:
static struct msi_domain_ops mbigen_domain_ops = {
.msi_prepare = mbigen_domain_ops_prepare,
};
static int mbigen_domain_ops_prepare(struct irq_domain *domain, struct device *dev,
int nvec, msi_alloc_info_t *info)
{
return its_msi_prepare(domain, dev_id, count, info);
}
>> + if (ret)
>> + return ret;
>
> Leaks the node allocation and the device.
>
>> +
>> + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &out_arg);
>> + if (ret < 0)
>> + return ret;
>> +
>> + for (i = 0; i < nr_irqs; i++) {
>
> This loop is required because?
>
Although we know this value is 1, I think use loop seems better
>> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> + &mbigen_irq_chip, mgn_dev);
>> + }
>> +
>> + return ret;
>
>> +/*
>> + * Early initialization as an interrupt controller
>> + */
>> +static int __init mbigen_of_init(struct device_node *node,
>> + struct device_node *parent_node)
>> +{
>> + struct mbigen_chip *chip;
>> + struct irq_domain *parent_domain;
>> + int err;
>> +
>> + parent_node = of_parse_phandle(node, "msi-parent", 0);
>
> Huch. parent node is an argument here. So WHY do you need to override
> it with some magic parent entry in the mbigen node? Seems your
> devicetree design sucks.
Because parent_nod argument point to gic node, but the parent device node of
mbigen is its node.
I didn't find the way how to pass its node into this function as the parent_node,
would you please give me some hint?
Thanks
>
>> diff --git a/include/linux/mbi.h b/include/linux/mbi.h
>> new file mode 100644
>> index 0000000..d3b8155
>> --- /dev/null
>> +++ b/include/linux/mbi.h
>> +
>> +/* Function to parse and map message interrupts */
>> +extern int its_msi_prepare(struct irq_domain *domain, u32 dev_id,
>> + int nvec, msi_alloc_info_t *info);
>> +extern struct irq_domain *get_its_domain(struct device_node *node);
>
> Crap in various aspects
>
> - these functions should only be visible from drivers/irqchip/
>
> - the header name is wrong as it does not provide any MBI
> specific functionality
>
Maybe I can named this file as 'arm-gic-v3-its.h' and put it in
include/linux/irqchip/
Thanks
On Wed, 8 Jul 2015, majun (F) wrote:
> $B:_(J 2015/7/6 20:33, Thomas Gleixner $B<LF;(J:
> > Care to explain what this does? It seems for some nodes you cannot
> > write the msi message. So how is that supposed to work? How is that
> > interrupt controlled (mask/unmask ...) ?
> >
> This function is used to write irq event id into vector register.Depends on
> hardware design, write operation is permitted in some mbigen node(nid=0,5,and >7),
> For other mbigen node, this register is read only.
>
> But only vector register has this problem. Other registers are ok for read/write.
You still fail to explain how that works if the register is not
writeable. And the code wants a proper comment explaining it.
> >> +static int mbigen_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >> + unsigned int nr_irqs, void *arg)
> >> +{
> >> + struct mbigen_chip *chip = domain->host_data;
> >> + struct of_phandle_args *irq_data = arg;
> >> + irq_hw_number_t hwirq;
> >> + u32 nid, dev_id, mbi_lines;
> >> + struct mbigen_node *mgn_node;
> >> + struct mbigen_device *mgn_dev;
> >> + msi_alloc_info_t out_arg;
> >> + int ret = 0, i;
> >> +
> >> + /* OF style allocation, one interrupt at a time */
> >
> > -ENOPARSE
> >
> what's this mean? I didn't find this definition in kernel code
That error code does not exist at all. It's just a jargon word and
means: "Error: Cannot parse".
In other words: That comment does not make any sense to me.
> According to Marc suggestion, I changed the ITS code so I can use its_msi_prepare
> function in my code.
> So,do you mean i should not call this function directly ?
> How about make this code likes below in mbigen driver:
>
> static struct msi_domain_ops mbigen_domain_ops = {
>
> .msi_prepare = mbigen_domain_ops_prepare,
> };
>
> static int mbigen_domain_ops_prepare(struct irq_domain *domain, struct device *dev,
> int nvec, msi_alloc_info_t *info)
> {
> return its_msi_prepare(domain, dev_id, count, info);
> }
How about using the parent domain pointer and invoking the function
via the parent->msi_domain_ops?
You seem to be focussed on hacking the ITS code into submission
instead of looking at the hierarchy information and use it proper.
> >> + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &out_arg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + for (i = 0; i < nr_irqs; i++) {
> >
> > This loop is required because?
> >
> Although we know this value is 1, I think use loop seems better
Better for what? For obfuscating the code?
Either this function can handle nr_irqs > 1 or not. If it can handle
it, then the WARN_ON(nr_irqs != 1) is bogus. If it can not, then the
loop is pointless.
> >> +static int __init mbigen_of_init(struct device_node *node,
> >> + struct device_node *parent_node)
> >> +{
> >> + struct mbigen_chip *chip;
> >> + struct irq_domain *parent_domain;
> >> + int err;
> >> +
> >> + parent_node = of_parse_phandle(node, "msi-parent", 0);
> >
> > Huch. parent node is an argument here. So WHY do you need to override
> > it with some magic parent entry in the mbigen node? Seems your
> > devicetree design sucks.
> Because parent_nod argument point to gic node, but the parent device node of
> mbigen is its node.
>
> I didn't find the way how to pass its node into this function as the parent_node,
> would you please give me some hint?
I gave you a hint already:
> > .... Seems your devicetree design sucks.
In other words: If your device tree the MBI node parent is GIC, then
your device tree is not reflecting the actual hierarchy.
> > Crap in various aspects
> >
> > - these functions should only be visible from drivers/irqchip/
> >
> > - the header name is wrong as it does not provide any MBI
> > specific functionality
> >
> Maybe I can named this file as 'arm-gic-v3-its.h' and put it in
> include/linux/irqchip/
Care to read what I wrote?
> > - these functions should only be visible from drivers/irqchip/
So what's the proper place for the header? Certainly not
include/linux/....
Aside of that, please look at the per-device MSI series Marc posted
(you were cc'ed). This is going to be where we are heading and your
driver should be based on that.
Thanks,
tglx
On Mon, Jul 06, 2015 at 08:09:08AM +0100, Ma Jun wrote:
> Add the mbigen msi interrupt controller bindings document
>
> Change in v3:
> ---Change the compatible string
> ---Change the interrupt cells definition.
>
> Signed-off-by: Ma Jun <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/mbigen.txt | 65 ++++++++++++++++++++++
> 1 files changed, 65 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..cf92ef8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
> @@ -0,0 +1,65 @@
> +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 gnerate the inteerrupt
> +by wirtting ITS register.
Please run this through a spell checker to get rid of typos.
> +
> +The mbigen and devices connect to mbigen have the following properties:
> +
> +
> +Mbigen required properties:
> +-------------------------------------------
> +-compatible: Should be "hisilicon,mbigen-v2"
> +-msi-parent: should specified the ITS mbigen connected
> +-interrupt controller: Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> + interrupt source. The value is 5 now.
> +
> + The 1st cell is the device id.
Does a given mbigen block generate interrupts with different ITS device
IDs? Or does a given mbigen block have a single device ID shared by all
interrupts it generates?
> + The 2nd cell is the totall interrupt number of this device?
I don't follow. What is a "total interrupt number"?
> + The 3rd cell is the hardware pin number of the interrupt.
> + This value depends on the Soc design.
This property seems sane.
> + The 4th cell is the mbigen node number. This value should refer to the
> + vendor soc specification.
What is this, and why do you think you need it?
Surely the address of the mbigen node is a sufficient unique identifier?
> + The 5th cell is the interrupt trigger type, encoded as follows:
> + 1 = edge triggered
> + 4 = level triggered
Hmm. How are level-triggered interrupts expected to be handled by the
mbigen?
> +
> +- reg: Specifies the base physical address and size of the ITS
> + registers.
NAK. You should not describe ITS properties here given this is not the
ITS.
Perhaps you mean "the mbigen registers"?
Thanks,
Mark.
On 08/07/15 14:40, Mark Rutland wrote:
> On Mon, Jul 06, 2015 at 08:09:08AM +0100, Ma Jun wrote:
>> Add the mbigen msi interrupt controller bindings document
>>
>> Change in v3:
>> ---Change the compatible string
>> ---Change the interrupt cells definition.
>>
>> Signed-off-by: Ma Jun <[email protected]>
>> ---
>> Documentation/devicetree/bindings/arm/mbigen.txt | 65 ++++++++++++++++++++++
>> 1 files changed, 65 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..cf92ef8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/mbigen.txt
>> @@ -0,0 +1,65 @@
>> +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 gnerate the inteerrupt
>> +by wirtting ITS register.
>
> Please run this through a spell checker to get rid of typos.
>
>> +
>> +The mbigen and devices connect to mbigen have the following properties:
>> +
>> +
>> +Mbigen required properties:
>> +-------------------------------------------
>> +-compatible: Should be "hisilicon,mbigen-v2"
>> +-msi-parent: should specified the ITS mbigen connected
>> +-interrupt controller: Identifies the node as an interrupt controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> + interrupt source. The value is 5 now.
>> +
>> + The 1st cell is the device id.
>
> Does a given mbigen block generate interrupts with different ITS device
> IDs? Or does a given mbigen block have a single device ID shared by all
> interrupts it generates?
>
>> + The 2nd cell is the totall interrupt number of this device?
>
> I don't follow. What is a "total interrupt number"?
>
>> + The 3rd cell is the hardware pin number of the interrupt.
>> + This value depends on the Soc design.
>
> This property seems sane.
>
>> + The 4th cell is the mbigen node number. This value should refer to the
>> + vendor soc specification.
>
> What is this, and why do you think you need it?
>
> Surely the address of the mbigen node is a sufficient unique identifier?
>
>> + The 5th cell is the interrupt trigger type, encoded as follows:
>> + 1 = edge triggered
>> + 4 = level triggered
>
> Hmm. How are level-triggered interrupts expected to be handled by the
> mbigen?
>
>> +
>> +- reg: Specifies the base physical address and size of the ITS
>> + registers.
>
> NAK. You should not describe ITS properties here given this is not the
> ITS.
>
> Perhaps you mean "the mbigen registers"?
Also, as we all seems to be looking at different aspects of the same
problem, it would make sense to follow (and hopefully contribute) to the
topology discussion that is taking place on this thread:
http://iommu.linux-foundation.narkive.com/GZ4NREWm/master-aware-devices-and-sideband-id-data
I'd definitely expect the MBIGEN subsystem to express the DeviceID it
presents to the ITS using a common format - the ITS code itself should
be able to parse it in isolation and still do the right thing.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 08/07/15 05:21, majun (F) wrote:
> Hi Thomas:
>
> 在 2015/7/6 20:33, Thomas Gleixner 写道:
>> On Mon, 6 Jul 2015, Ma Jun wrote:
>>
>
>>> +/**
>>> + * get_mbigen_node_type: get the mbigen node type
>>> + * @nid: the mbigen node value
>>> + * return 0: evnent id of interrupt connected to this node can be changed.
>>> + * return 1: evnent id of interrupt connected to this node cant be changed.
>>> + */
>>> +static int get_mbigen_node_type(int nid)
>>> +{
>>> + if (nid > MG_NR) {
>>> + pr_warn("MBIGEN: Device ID exceeds max number!\n");
>>> + return 1;
>>> + }
>>> + if ((nid == 0) || (nid == 5) || (nid > 7))
>>> + return 0;
>>> + else
>>> + return 1;
>>
>> Oh no. We do not hardcode such properties into a driver. That wants to
>> be in the device tree and set as a property in the node data structure.
>>
> Ok,I will move this to device tree
>
>>> +static int mbigen_write_msg(struct irq_data *d, struct msi_msg *msg)
>>> +{
>>> + struct mbigen_chip *chip = d->domain->host_data;
>>> + void __iomem *addr;
>>> + u32 nid, val, offset;
>>> + int ret = 0;
>>> +
>>> + nid = GET_NODE_NUM(d->hwirq);
>>> + ret = get_mbigen_node_type(nid);
>>> + if (ret)
>>> + return 0;
>>
>> Care to explain what this does? It seems for some nodes you cannot
>> write the msi message. So how is that supposed to work? How is that
>> interrupt controlled (mask/unmask ...) ?
>>
> This function is used to write irq event id into vector register.Depends on
> hardware design, write operation is permitted in some mbigen node(nid=0,5,and >7),
> For other mbigen node, this register is read only.
So how do you expect this to work? You cannot program the event
generated by the mbigen, and the ITS has an ITT that probably doesn't
match your HW.
Best case, the interrupt is simply dropped, worse case you end up in an
interrupt storm because you can't figure out which device is screaming.
I'm a bit puzzled.
M.
--
Jazz is not dead. It just smells funny...
Hi,
Aside from all the comments Thomas had, the following aspect is worrying
me a bit:
On 06/07/15 08:09, Ma Jun wrote:
> This patch contains the mbigen interrupt controller driver.
[...]
> +static int mbigen_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct mbigen_chip *chip = d->domain->host_data;
> + u32 ofst, mask;
> + u32 val, nid, hwirq;
> + void __iomem *addr;
> +
> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> + return -EINVAL;
You seem to be supporting both edge and level triggered interrupts.
Given that the ITS is edge triggered only, I must assume you have some
code to regenerate an edge if the wired interrupt is level triggered,
and that the line level is still high when you perform the EOI...
> +
> + nid = GET_NODE_NUM(d->hwirq);
> + hwirq = HWIRQ_OFFSET(d->hwirq);
> +
> + ofst = hwirq / 32 * 4;
> + mask = 1 << (hwirq % 32);
> +
> + addr = chip->base + MBIGEN_TYPE_REG_ADDR(nid, ofst);
> + raw_spin_lock(&chip->lock);
> + val = readl_relaxed(addr);
> +
> + if (type == IRQ_TYPE_LEVEL_HIGH)
> + val |= mask;
> + else if (type == IRQ_TYPE_EDGE_RISING)
> + val &= ~mask;
> +
> + writel_relaxed(val, addr);
> + raw_spin_unlock(&chip->lock);
> +
> + return 0;
> +}
> +
> +static void mbigen_mask_irq(struct irq_data *data)
> +{
> + irq_chip_mask_parent(data);
> +}
> +
> +static void mbigen_unmask_irq(struct irq_data *data)
> +{
> + irq_chip_unmask_parent(data);
> +}
> +
> +static int mbigen_set_affinity(struct irq_data *data,
> + const struct cpumask *mask,
> + bool force)
> +{
> + int ret;
> +
> + ret = irq_chip_set_affinity_parent(data, mask, force);
> + return ret;
> +}
> +
> +static void mbigen_irq_eoi(struct irq_data *d)
> +{
> + irq_chip_eoi_parent(d);
... but this function doesn't have any code dealing with injecting an
edge on detecting a level high.
So how does it work? Either you're missing some logic here, or you don't
really support level interrupts.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
在 2015/7/8 23:16, Marc Zyngier 写道:
> On 08/07/15 05:21, majun (F) wrote:
>> Hi Thomas:
>>
[...]
>>>> +
>>>> + nid = GET_NODE_NUM(d->hwirq);
>>>> + ret = get_mbigen_node_type(nid);
>>>> + if (ret)
>>>> + return 0;
>>>
>>> Care to explain what this does? It seems for some nodes you cannot
>>> write the msi message. So how is that supposed to work? How is that
>>> interrupt controlled (mask/unmask ...) ?
>>>
>> This function is used to write irq event id into vector register.Depends on
>> hardware design, write operation is permitted in some mbigen node(nid=0,5,and >7),
>> For other mbigen node, this register is read only.
>
> So how do you expect this to work? You cannot program the event
> generated by the mbigen, and the ITS has an ITT that probably doesn't
> match your HW.
>
> Best case, the interrupt is simply dropped, worse case you end up in an
> interrupt storm because you can't figure out which device is screaming.
>
> I'm a bit puzzled.
For interrupts connect to mbigen , the interrupt trigger type, device id and
event id value are encoded in mbigen chip already.
There are two types of mbigen node within a mbigen chip.
Type1: event id valud can't be programmed.
Type2: event id value can be programmed.
For example: An device with 5 interrupts connected to Mbigen node
type 1.The default event id vlaue encoded in mbigen chip for these 5 interrupt
is from 0 to 4.
Because the event id value can't be programmed, we need to define all of
5 interrupts in dts file so that these 5 interrupt has
$B:_(B 2015/7/8 18:44, Thomas Gleixner $B<LF;(B:
> On Wed, 8 Jul 2015, majun (F) wrote:
>> $B:_(B 2015/7/6 20:33, Thomas Gleixner $B<LF;(B:
>>> Care to explain what this does? It seems for some nodes you cannot
>>> write the msi message. So how is that supposed to work? How is that
>>> interrupt controlled (mask/unmask ...) ?
>>>
>> This function is used to write irq event id into vector register.Depends on
>> hardware design, write operation is permitted in some mbigen node(nid=0,5,and >7),
>> For other mbigen node, this register is read only.
>>
>> But only vector register has this problem. Other registers are ok for read/write.
>
> You still fail to explain how that works if the register is not
> writeable. And the code wants a proper comment explaining it.
>
Actually, the interrupt trigger type, device id and event id already encoded in
mbigen chip.
Depends on hardweare design, the device id and event id value in some mbigen nodes
can't be modified, but some nodes can.
For the mbigen node which event id can't be modified, we can use the default event id
value (encoded in mbigen register).
If the event id can be programmed, we can use this function to modify the event id value.
[....]
>
> Aside of that, please look at the per-device MSI series Marc posted
> (you were cc'ed). This is going to be where we are heading and your
> driver should be based on that.
Ok, i will change mbigen code based on Marc's patch.
>
> Thanks,
>
> tglx
>
在 2015/7/8 23:30, Marc Zyngier 写道:
> Hi,
>
> Aside from all the comments Thomas had, the following aspect is worrying
> me a bit:
>
> On 06/07/15 08:09, Ma Jun wrote:
>> This patch contains the mbigen interrupt controller driver.
>
> [...]
>
>> +static int mbigen_set_type(struct irq_data *d, unsigned int type)
>> +{
>> + struct mbigen_chip *chip = d->domain->host_data;
>> + u32 ofst, mask;
>> + u32 val, nid, hwirq;
>> + void __iomem *addr;
>> +
>> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>> + return -EINVAL;
>
> You seem to be supporting both edge and level triggered interrupts.
>
> Given that the ITS is edge triggered only, I must assume you have some
> code to regenerate an edge if the wired interrupt is level triggered,
> and that the line level is still high when you perform the EOI...
>
For each interrupt, there is state machine in mbigen chip.
inactive-->pending--> active(pending & active)
The level triggered interrupt process flow list as below:
device---->mbigen---->ITS---->GIC--->CPU
[1]: device triggered interrupt A and line level changes to high
[2]: Mbigen receive interrupt A and changes the status of A to pending in mbigen(mbigen.state = pending)
[3]: Mbigen send interrupt A to ITS , the A status in mbigen will be changed to pending
& active (mbigen.state = pending & active)
[4]: ITS receive the interrupt A and send A to gic (A status in gic is pending. gic.state=pending)
[5]: CPU ack the interrupt A ( gic.state = active)
[6]: Enter interrupt handler. The interrupt line level is cleared in device irq handler.
[7]: When detects the low level on interrupt A line, mbigen change the interrupt A status
from pending & active to inactive (mbigen.state = inactive).
[8]: Send EOI . a): write register to clear the status in mbigen .
b):clear the status in gic. (gic.state = inactive)
[....]
>> +static void mbigen_irq_eoi(struct irq_data *d)
>> +{
>> + irq_chip_eoi_parent(d);
>
> ... but this function doesn't have any code dealing with injecting an
> edge on detecting a level high.
>
Yes, before irq_chip_eoi_parent is called, some code should be added to
clear the interrupt status in mbigen.
> So how does it work? Either you're missing some logic here, or you don't
> really support level interrupts.
>
> Thanks,
>
> M.
>
在 2015/7/8 21:40, Mark Rutland 写道:
> On Mon, Jul 06, 2015 at 08:09:08AM +0100, Ma Jun wrote:
[...]
>> +
>> +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 gnerate the inteerrupt
>> +by wirtting ITS register.
>
> Please run this through a spell checker to get rid of typos.
>
sorry, it's my mistake
>> +
>> +The mbigen and devices connect to mbigen have the following properties:
>> +
>> +
>> +Mbigen required properties:
>> +-------------------------------------------
>> +-compatible: Should be "hisilicon,mbigen-v2"
>> +-msi-parent: should specified the ITS mbigen connected
>> +-interrupt controller: Identifies the node as an interrupt controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> + interrupt source. The value is 5 now.
>> +
>> + The 1st cell is the device id.
>
> Does a given mbigen block generate interrupts with different ITS device
> IDs? Or does a given mbigen block have a single device ID shared by all
> interrupts it generates?
>
The mbigen chip structrue likes below:
mbigen_chip(domain)
|------------|------------------|
mbigen_node0 mbigen_node1 mbigen_node2
| | | | | |
dev1 dev2 dev3 dev4 dev5 dev6
For each mbigen chip, it contains several mbigen nodes.
For each mbigen node, it can collect interrupts from different devices.
For example, dev1 and dev2 both connect to mbigen node0.Because dev1 and dev2 are
differnt device, they have different device id.
The device id is encoded in mbigen chip and can not be changed.
>> + The 2nd cell is the totall interrupt number of this device?
>
> I don't follow. What is a "total interrupt number"?
>
It's the wired interrupt number connected to a device.
For the devices connected to mbigen node, the interrupt number varied from
1 to 100+ .So I have to specifies this value in dts.
>> + The 3rd cell is the hardware pin number of the interrupt.
>> + This value depends on the Soc design.
>
> This property seems sane.
>
>> + The 4th cell is the mbigen node number. This value should refer to the
>> + vendor soc specification.
>
> What is this, and why do you think you need it?
>
> Surely the address of the mbigen node is a sufficient unique identifier?
>
mbigen_chip(domain)
|------------|------------------|
mbigen_node0 mbigen_node1 mbigen_node2
| | | | | |
dev1 dev2 dev3 dev4 dev5 dev6
To avoid the duplicat hardware irq number problem, the Mbigen node number is defined here.
For example:
dev1 has 3 interrupts with pin number from 0 to 2
dev3 has 5 interrupts with pin number from 0 to 4
For dev3 the interrupt from 0 to 2 would be has same hardware irq number
as dev1 if we only use pin number.
Because these two devices located in same irq domain(mbigen chip),using same
hwirq number is a mistake.
In mbigen driver, I will use this value and the 3rd cell(pin number) to compose
a new hardware irq( (nid<<8) | pin number) for mbigen using.
>> + The 5th cell is the interrupt trigger type, encoded as follows:
>> + 1 = edge triggered
>> + 4 = level triggered
>
> Hmm. How are level-triggered interrupts expected to be handled by the
> mbigen?
>
For each interrupt, there is state machine in mbigen chip.
inactive-->pending--> active(pending & active)
The level triggered interrupt process flow list as below:
device---->mbigen---->ITS---->GIC--->CPU
[1]: device triggered interrupt A and line level changes to high
[2]: Mbigen receive interrupt A and changes the status of A to pending in mbigen(mbigen.state = pending)
[3]: Mbigen send interrupt A to ITS , the A status in mbigen will be changed to pending
& active (mbigen.state = pending & active)
[4]: ITS receive the interrupt A and send A to gic (A status in gic is pending. gic.state=pending)
[5]: CPU ack the interrupt A ( gic.state = active)
[6]: Enter interrupt handler. The interrupt line level is cleared in device irq handler.
[7]: When detects the low level on interrupt A line, mbigen change the interrupt A status
from pending & active to inactive (mbigen.state = inactive).
[8]: Send EOI . a): write register to clear the status in mbigen .
b):clear the status in gic. (gic.state = inactive)
>> +
>> +- reg: Specifies the base physical address and size of the ITS
>> + registers.
>
> NAK. You should not describe ITS properties here given this is not the
> ITS.
>
> Perhaps you mean "the mbigen registers"?
>
yes, it should be 'mbigen'
On 16/07/15 09:35, majun (F) wrote:
>
>
> 在 2015/7/8 23:16, Marc Zyngier 写道:
>> On 08/07/15 05:21, majun (F) wrote:
>>> Hi Thomas:
>>>
> [...]
>>>>> +
>>>>> + nid = GET_NODE_NUM(d->hwirq);
>>>>> + ret = get_mbigen_node_type(nid);
>>>>> + if (ret)
>>>>> + return 0;
>>>>
>>>> Care to explain what this does? It seems for some nodes you cannot
>>>> write the msi message. So how is that supposed to work? How is that
>>>> interrupt controlled (mask/unmask ...) ?
>>>>
>>> This function is used to write irq event id into vector register.Depends on
>>> hardware design, write operation is permitted in some mbigen node(nid=0,5,and >7),
>>> For other mbigen node, this register is read only.
>>
>> So how do you expect this to work? You cannot program the event
>> generated by the mbigen, and the ITS has an ITT that probably doesn't
>> match your HW.
>>
>> Best case, the interrupt is simply dropped, worse case you end up in an
>> interrupt storm because you can't figure out which device is screaming.
>>
>> I'm a bit puzzled.
>
> For interrupts connect to mbigen , the interrupt trigger type, device id and
> event id value are encoded in mbigen chip already.
>
> There are two types of mbigen node within a mbigen chip.
> Type1: event id valud can't be programmed.
> Type2: event id value can be programmed.
>
> For example: An device with 5 interrupts connected to Mbigen node
> type 1.The default event id vlaue encoded in mbigen chip for these 5 interrupt
> is from 0 to 4.
>
> Because the event id value can't be programmed, we need to define all of
> 5 interrupts in dts file so that these 5 interrupt has
You can define what you want in the device tree, the ITS doesn't care!
Nothing in the ITS code parses this property, and there is absolutely
zero chance that the even the ITS has allocated will actually match what
you expect.
The ITS *relies* on the principle that the evenID can be programmed,
just like any MSI controller relies on the device to be programmed with
whatever payload has been provided. If all of a sudden we have to
support HW that has its own view of the payload, what you have here will
simply not work.
M.
--
Jazz is not dead. It just smells funny...
在 2015/7/16 16:52, Marc Zyngier 写道:
> On 16/07/15 09:35, majun (F) wrote:
>>> I'm a bit puzzled.
>>
>> For interrupts connect to mbigen , the interrupt trigger type, device id and
>> event id value are encoded in mbigen chip already.
>>
>> There are two types of mbigen node within a mbigen chip.
>> Type1: event id valud can't be programmed.
>> Type2: event id value can be programmed.
>>
>> For example: An device with 5 interrupts connected to Mbigen node
>> type 1.The default event id vlaue encoded in mbigen chip for these 5 interrupt
>> is from 0 to 4.
>>
>> Because the event id value can't be programmed, we need to define all of
>> 5 interrupts in dts file so that these 5 interrupt has
>
> You can define what you want in the device tree, the ITS doesn't care!
> Nothing in the ITS code parses this property, and there is absolutely
> zero chance that the even the ITS has allocated will actually match what
> you expect.
>
> The ITS *relies* on the principle that the evenID can be programmed,
> just like any MSI controller relies on the device to be programmed with
> whatever payload has been provided. If all of a sudden we have to
> support HW that has its own view of the payload, what you have here will
> simply not work.
>
"If all of a sudden we have to
support HW that has its own view of the payload, what you have here will
simply not work."
I am not very unstand this case, would you please explain this more detail?
Thanks!
On 16/07/15 10:22, majun (F) wrote:
>
>
> 在 2015/7/16 16:52, Marc Zyngier 写道:
>> On 16/07/15 09:35, majun (F) wrote:
>
>>>> I'm a bit puzzled.
>>>
>>> For interrupts connect to mbigen , the interrupt trigger type, device id and
>>> event id value are encoded in mbigen chip already.
>>>
>>> There are two types of mbigen node within a mbigen chip.
>>> Type1: event id valud can't be programmed.
>>> Type2: event id value can be programmed.
>>>
>>> For example: An device with 5 interrupts connected to Mbigen node
>>> type 1.The default event id vlaue encoded in mbigen chip for these 5 interrupt
>>> is from 0 to 4.
>>>
>>> Because the event id value can't be programmed, we need to define all of
>>> 5 interrupts in dts file so that these 5 interrupt has
>>
>> You can define what you want in the device tree, the ITS doesn't care!
>> Nothing in the ITS code parses this property, and there is absolutely
>> zero chance that the even the ITS has allocated will actually match what
>> you expect.
>>
>> The ITS *relies* on the principle that the evenID can be programmed,
>> just like any MSI controller relies on the device to be programmed with
>> whatever payload has been provided. If all of a sudden we have to
>> support HW that has its own view of the payload, what you have here will
>> simply not work.
>>
> "If all of a sudden we have to
> support HW that has its own view of the payload, what you have here will
> simply not work."
>
> I am not very unstand this case, would you please explain this more detail?
It is the ITS driver that allocates the eventID, not the device. If your
hardware has decided that it will use event 5, while the ITS expect it
to use event 3, nothing will work. And there is no way for the ITS to
know which event your device is using (it doesn't parse your device's
device tree).
M.
--
Jazz is not dead. It just smells funny...
在 2015/7/16 17:30, Marc Zyngier 写道:
> On 16/07/15 10:22, majun (F) wrote:
>>
>>
>> 在 2015/7/16 16:52, Marc Zyngier 写道:
>>> On 16/07/15 09:35, majun (F) wrote:
>>
>>>>> I'm a bit puzzled.
>>>>
>>>> For interrupts connect to mbigen , the interrupt trigger type, device id and
>>>> event id value are encoded in mbigen chip already.
>>>>
>>>> There are two types of mbigen node within a mbigen chip.
>>>> Type1: event id valud can't be programmed.
>>>> Type2: event id value can be programmed.
>>>>
>>>> For example: An device with 5 interrupts connected to Mbigen node
>>>> type 1.The default event id vlaue encoded in mbigen chip for these 5 interrupt
>>>> is from 0 to 4.
>>>>
>>>> Because the event id value can't be programmed, we need to define all of
>>>> 5 interrupts in dts file so that these 5 interrupt has
>>>
>>> You can define what you want in the device tree, the ITS doesn't care!
>>> Nothing in the ITS code parses this property, and there is absolutely
>>> zero chance that the even the ITS has allocated will actually match what
>>> you expect.
>>>
>>> The ITS *relies* on the principle that the evenID can be programmed,
>>> just like any MSI controller relies on the device to be programmed with
>>> whatever payload has been provided. If all of a sudden we have to
>>> support HW that has its own view of the payload, what you have here will
>>> simply not work.
>>>
>> "If all of a sudden we have to
>> support HW that has its own view of the payload, what you have here will
>> simply not work."
>>
>> I am not very unstand this case, would you please explain this more detail?
>
> It is the ITS driver that allocates the eventID, not the device. If your
> hardware has decided that it will use event 5, while the ITS expect it
> to use event 3, nothing will work. And there is no way for the ITS to
> know which event your device is using (it doesn't parse your device's
> device tree).
>
Yes, I agree with you that eventID should be allocated by ITS driver.
But I want to explain the case you said above can be avoided.
For example, An device with total 5 interrupts connected to Mbigen node.
In mbigen chip, the default eventID value for each device starts from 0.
So, for these 5 interrupts the default eventID value is from 0 to 4.
Because the default eventID is fixed in mbigen chip, to make these 5 interrupt
work, the only way is define all these 5 interrupts in dts file.
When irq initializing, ITS driver will allocat LPI interrupt number for these
5 interrupts, for example : from 8192 to 8196. and the eventID value is from 0 to 4.
Now, the allocated eventID value same as the eventID value encoded in mbigen chip,
The interrupt can work.
I know this is not a good method,but for current mbigen chip,it's the only solution.
On 16/07/15 13:26, majun (F) wrote:
>
>
> 在 2015/7/16 17:30, Marc Zyngier 写道:
>> On 16/07/15 10:22, majun (F) wrote:
>>>
>>>
>>> 在 2015/7/16 16:52, Marc Zyngier 写道:
>>>> On 16/07/15 09:35, majun (F) wrote:
>>>
>>>>>> I'm a bit puzzled.
>>>>>
>>>>> For interrupts connect to mbigen , the interrupt trigger type, device id and
>>>>> event id value are encoded in mbigen chip already.
>>>>>
>>>>> There are two types of mbigen node within a mbigen chip.
>>>>> Type1: event id valud can't be programmed.
>>>>> Type2: event id value can be programmed.
>>>>>
>>>>> For example: An device with 5 interrupts connected to Mbigen node
>>>>> type 1.The default event id vlaue encoded in mbigen chip for these 5 interrupt
>>>>> is from 0 to 4.
>>>>>
>>>>> Because the event id value can't be programmed, we need to define all of
>>>>> 5 interrupts in dts file so that these 5 interrupt has
>>>>
>>>> You can define what you want in the device tree, the ITS doesn't care!
>>>> Nothing in the ITS code parses this property, and there is absolutely
>>>> zero chance that the even the ITS has allocated will actually match what
>>>> you expect.
>>>>
>>>> The ITS *relies* on the principle that the evenID can be programmed,
>>>> just like any MSI controller relies on the device to be programmed with
>>>> whatever payload has been provided. If all of a sudden we have to
>>>> support HW that has its own view of the payload, what you have here will
>>>> simply not work.
>>>>
>>> "If all of a sudden we have to
>>> support HW that has its own view of the payload, what you have here will
>>> simply not work."
>>>
>>> I am not very unstand this case, would you please explain this more detail?
>>
>> It is the ITS driver that allocates the eventID, not the device. If your
>> hardware has decided that it will use event 5, while the ITS expect it
>> to use event 3, nothing will work. And there is no way for the ITS to
>> know which event your device is using (it doesn't parse your device's
>> device tree).
>>
> Yes, I agree with you that eventID should be allocated by ITS driver.
> But I want to explain the case you said above can be avoided.
>
> For example, An device with total 5 interrupts connected to Mbigen node.
> In mbigen chip, the default eventID value for each device starts from 0.
> So, for these 5 interrupts the default eventID value is from 0 to 4.
>
> Because the default eventID is fixed in mbigen chip, to make these 5 interrupt
> work, the only way is define all these 5 interrupts in dts file.
But the ITS doesn't read these interrupts, and won't let you program the
translation either.
> When irq initializing, ITS driver will allocat LPI interrupt number for these
> 5 interrupts, for example : from 8192 to 8196. and the eventID value is from 0 to 4.
> Now, the allocated eventID value same as the eventID value encoded in mbigen chip,
> The interrupt can work.
But that's pure luck! What if I decide to change the allocation method
so that all the devices share a common ITT?
Have you really hardcoded the Linux behaviour in your hardware?
> I know this is not a good method,but for current mbigen chip,it's the only solution.
I'm sorry, I can't call this a solution.
M.
--
Jazz is not dead. It just smells funny...
> >> +The mbigen and devices connect to mbigen have the following properties:
> >> +
> >> +
> >> +Mbigen required properties:
> >> +-------------------------------------------
> >> +-compatible: Should be "hisilicon,mbigen-v2"
> >> +-msi-parent: should specified the ITS mbigen connected
> >> +-interrupt controller: Identifies the node as an interrupt controller
> >> +- #interrupt-cells : Specifies the number of cells needed to encode an
> >> + interrupt source. The value is 5 now.
> >> +
> >> + The 1st cell is the device id.
> >
> > Does a given mbigen block generate interrupts with different ITS device
> > IDs? Or does a given mbigen block have a single device ID shared by all
> > interrupts it generates?
> >
> The mbigen chip structrue likes below:
>
> mbigen_chip(domain)
> |------------|------------------|
> mbigen_node0 mbigen_node1 mbigen_node2
> | | | | | |
> dev1 dev2 dev3 dev4 dev5 dev6
>
> For each mbigen chip, it contains several mbigen nodes.
> For each mbigen node, it can collect interrupts from different devices.
>
> For example, dev1 and dev2 both connect to mbigen node0.Because dev1 and dev2 are
> differnt device, they have different device id.
>
> The device id is encoded in mbigen chip and can not be changed.
Thanks for the diagram, that clears up some of my confusion regarding
nodes.
Ok, so each device has it's own device ID. That's good. If a device has
multiple interrupt lines to the mbigen, do these always share the same
device ID?
> >> + The 2nd cell is the totall interrupt number of this device?
> >
> > I don't follow. What is a "total interrupt number"?
> >
> It's the wired interrupt number connected to a device.
> For the devices connected to mbigen node, the interrupt number varied from
> 1 to 100+ .So I have to specifies this value in dts.
>
> >> + The 3rd cell is the hardware pin number of the interrupt.
> >> + This value depends on the Soc design.
> >
> > This property seems sane.
> >
> >> + The 4th cell is the mbigen node number. This value should refer to the
> >> + vendor soc specification.
> >
> > What is this, and why do you think you need it?
> >
> > Surely the address of the mbigen node is a sufficient unique identifier?
> >
> mbigen_chip(domain)
> |------------|------------------|
> mbigen_node0 mbigen_node1 mbigen_node2
> | | | | | |
> dev1 dev2 dev3 dev4 dev5 dev6
>
> To avoid the duplicat hardware irq number problem, the Mbigen node number is defined here.
> For example:
> dev1 has 3 interrupts with pin number from 0 to 2
> dev3 has 5 interrupts with pin number from 0 to 4
> For dev3 the interrupt from 0 to 2 would be has same hardware irq number
> as dev1 if we only use pin number.
>
> Because these two devices located in same irq domain(mbigen chip),using same
> hwirq number is a mistake.
>
> In mbigen driver, I will use this value and the 3rd cell(pin number) to compose
> a new hardware irq( (nid<<8) | pin number) for mbigen using.
Ok. I now see why you need node and pin.
However, I don't see why you also need the 'total' interrupt number. Why
isn't node and pin sufficient to uniquely identify an interrupt?
Thanks,
Mark.
Hi Mark:
在 2015/7/21 0:38, Mark Rutland 写道:
>>>> +The mbigen and devices connect to mbigen have the following properties:
>> The mbigen chip structrue likes below:
>>
>> mbigen_chip(domain)
>> |------------|------------------|
>> mbigen_node0 mbigen_node1 mbigen_node2
>> | | | | | |
>> dev1 dev2 dev3 dev4 dev5 dev6
>>
>> For each mbigen chip, it contains several mbigen nodes.
>> For each mbigen node, it can collect interrupts from different devices.
>>
>> For example, dev1 and dev2 both connect to mbigen node0.Because dev1 and dev2 are
>> differnt device, they have different device id.
>>
>> The device id is encoded in mbigen chip and can not be changed.
>
> Thanks for the diagram, that clears up some of my confusion regarding
> nodes.
>
> Ok, so each device has it's own device ID. That's good. If a device has
> multiple interrupt lines to the mbigen, do these always share the same
> device ID?
>
yes, for the interrupt lines within a device, they always share the same
device ID.
>>>> + The 2nd cell is the totall interrupt number of this device?
>>>
>>> I don't follow. What is a "total interrupt number"?
>>>
>> It's the wired interrupt number connected to a device.
>> For the devices connected to mbigen node, the interrupt number varied from
>> 1 to 100+ .So I have to specifies this value in dts.
>>
>>>> + The 3rd cell is the hardware pin number of the interrupt.
>>>> + This value depends on the Soc design.
>>>
>>> This property seems sane.
>>>
>>>> + The 4th cell is the mbigen node number. This value should refer to the
>>>> + vendor soc specification.
>>>
>>> What is this, and why do you think you need it?
>>>
>>> Surely the address of the mbigen node is a sufficient unique identifier?
>>>
>> mbigen_chip(domain)
>> |------------|------------------|
>> mbigen_node0 mbigen_node1 mbigen_node2
>> | | | | | |
>> dev1 dev2 dev3 dev4 dev5 dev6
>>
>> To avoid the duplicat hardware irq number problem, the Mbigen node number is defined here.
>> For example:
>> dev1 has 3 interrupts with pin number from 0 to 2
>> dev3 has 5 interrupts with pin number from 0 to 4
>> For dev3 the interrupt from 0 to 2 would be has same hardware irq number
>> as dev1 if we only use pin number.
>>
>> Because these two devices located in same irq domain(mbigen chip),using same
>> hwirq number is a mistake.
>>
>> In mbigen driver, I will use this value and the 3rd cell(pin number) to compose
>> a new hardware irq( (nid<<8) | pin number) for mbigen using.
>
> Ok. I now see why you need node and pin.
>
> However, I don't see why you also need the 'total' interrupt number. Why
> isn't node and pin sufficient to uniquely identify an interrupt?
>
The ITS driver alloc a ITT table for each device.
The table size depends on interrupt number of the device.
So I need to specify the total interrupt number .
> Thanks,
> Mark.
>
> .
>
Hi Marc
在 2015/7/16 21:37, Marc Zyngier 写道:
[...]
>>
>> For example, An device with total 5 interrupts connected to Mbigen node.
>> In mbigen chip, the default eventID value for each device starts from 0.
>> So, for these 5 interrupts the default eventID value is from 0 to 4.
>>
>> Because the default eventID is fixed in mbigen chip, to make these 5 interrupt
>> work, the only way is define all these 5 interrupts in dts file.
>
> But the ITS doesn't read these interrupts, and won't let you program the
> translation either.
>
>> When irq initializing, ITS driver will allocat LPI interrupt number for these
>> 5 interrupts, for example : from 8192 to 8196. and the eventID value is from 0 to 4.
>> Now, the allocated eventID value same as the eventID value encoded in mbigen chip,
>> The interrupt can work.
>
> But that's pure luck! What if I decide to change the allocation method
> so that all the devices share a common ITT?
>
> Have you really hardcoded the Linux behaviour in your hardware?
>
>> I know this is not a good method,but for current mbigen chip,it's the only solution.
>
> I'm sorry, I can't call this a solution.
Yes,you are right. This is not a right way to solve the problem.
This problem will be fixed in next version chip.
>
> M.
>