* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:01 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-21 10:01 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Masami Hiramatsu, Jassi Brar, Masahiro Yamada,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap, devicetree,
linux-kernel, David S. Miller, Rob Herring, Greg Kroah-Hartman,
Mark Rutland, linux-arm-kernel
UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
to provide additional features that are not covered by GIC. The main
purpose is to provide logic inverter to support low level and falling
edge trigger type for interrupt lines from on-board devices.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Use readl/write_relaxed instead of readl/writel
- Move val = 0 for code symmetry
- .alloc() hook only supports nr_irqs==1 case
- Move struct irq_chip to static data because this driver expects
only one device instance in the system
- Match the interrupt number to SPI of GIC to make DT binding clearer
(register offset starts from 0x4)
.../socionext,uniphier-aidet.txt | 32 +++
MAINTAINERS | 1 +
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
5 files changed, 288 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
new file mode 100644
index 000000000000..48e71d3ac2ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
@@ -0,0 +1,32 @@
+UniPhier AIDET
+
+UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
+Interrupt Controller). GIC itself can handle only high level and rising edge
+interrupts. The AIDET provides logic inverter to support low level and falling
+edge interrupts.
+
+Required properties:
+- compatible: Should be one of the following:
+ "socionext,uniphier-ld4-aidet" - for LD4 SoC
+ "socionext,uniphier-pro4-aidet" - for Pro4 SoC
+ "socionext,uniphier-sld8-aidet" - for sLD8 SoC
+ "socionext,uniphier-pro5-aidet" - for Pro5 SoC
+ "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
+ "socionext,uniphier-ld11-aidet" - for LD11 SoC
+ "socionext,uniphier-ld20-aidet" - for LD20 SoC
+ "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
+- reg: Specifies offset and length of the register set for the device.
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
+ source. The value should be 2. The first cell defines the interrupt number
+ (corresponds to the SPI interrupt number of GIC). The second cell specifies
+ the trigger type as defined in interrupts.txt in this directory.
+
+Example:
+
+ aidet: aidet@5fc20000 {
+ compatible = "socionext,uniphier-pro4-aidet";
+ reg = <0x5fc20000 0x200>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1c3feffb1c1c..7b84f047b3fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1993,6 +1993,7 @@ F: arch/arm64/boot/dts/socionext/
F: drivers/bus/uniphier-system-bus.c
F: drivers/clk/uniphier/
F: drivers/i2c/busses/i2c-uniphier*
+F: drivers/irqchip/irq-uniphier-aidet.c
F: drivers/pinctrl/uniphier/
F: drivers/reset/reset-uniphier.c
F: drivers/tty/serial/8250/8250_uniphier.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f1fd5f44d1d4..d1f43cb92e4d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -306,3 +306,11 @@ config QCOM_IRQ_COMBINER
help
Say yes here to add support for the IRQ combiner devices embedded
in Qualcomm Technologies chips.
+
+config IRQ_UNIPHIER_AIDET
+ bool "UniPhier AIDET support" if COMPILE_TEST
+ depends on ARCH_UNIPHIER || COMPILE_TEST
+ default ARCH_UNIPHIER
+ select IRQ_DOMAIN_HIERARCHY
+ help
+ Support for the UniPhier AIDET (ARM Interrupt Detector).
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e88d856cc09c..2c630574986f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
+obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
new file mode 100644
index 000000000000..418427f0d8e5
--- /dev/null
+++ b/drivers/irqchip/irq-uniphier-aidet.c
@@ -0,0 +1,246 @@
+/*
+ * Driver for UniPhier AIDET (ARM Interrupt Detector)
+ *
+ * Copyright (C) 2017 Socionext Inc.
+ * Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define UNIPHIER_AIDET_NR_IRQS 256
+
+#define UNIPHIER_AIDET_DETCONF 0x04 /* inverter register base */
+
+struct uniphier_aidet_priv {
+ struct irq_domain *domain;
+ void __iomem *reg_base;
+ spinlock_t lock;
+ u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
+};
+
+static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
+ unsigned int reg, u32 mask, u32 val)
+{
+ unsigned long flags;
+ u32 tmp;
+
+ spin_lock_irqsave(&priv->lock, flags);
+ tmp = readl_relaxed(priv->reg_base + reg);
+ tmp &= ~mask;
+ tmp |= mask & val;
+ writel_relaxed(tmp, priv->reg_base + reg);
+ spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
+ unsigned long index, unsigned int val)
+{
+ unsigned int reg;
+ u32 mask;
+
+ reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
+ mask = BIT(index % 32);
+
+ uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
+}
+
+static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct uniphier_aidet_priv *priv = data->chip_data;
+ unsigned int val;
+
+ /* enable inverter for active-low triggers */
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_LEVEL_HIGH:
+ val = 0;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ val = 1;
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ val = 1;
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ uniphier_aidet_detconf_update(priv, data->hwirq, val);
+
+ return irq_chip_set_type_parent(data, type);
+}
+
+static struct irq_chip uniphier_aidet_irq_chip = {
+ .name = "AIDET",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = uniphier_aidet_irq_set_type,
+};
+
+static int uniphier_aidet_domain_translate(struct irq_domain *domain,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (WARN_ON(fwspec->param_count < 2))
+ return -EINVAL;
+
+ *out_hwirq = fwspec->param[0];
+ *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+ return 0;
+}
+
+static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret;
+
+ if (nr_irqs != 1)
+ return -EINVAL;
+
+ ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
+ return -ENXIO;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ &uniphier_aidet_irq_chip,
+ domain->host_data);
+ if (ret)
+ return ret;
+
+ /* parent is GIC */
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0] = 0; /* SPI */
+ parent_fwspec.param[1] = hwirq;
+ parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+}
+
+static const struct irq_domain_ops uniphier_aidet_domain_ops = {
+ .alloc = uniphier_aidet_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+ .translate = uniphier_aidet_domain_translate,
+};
+
+static int uniphier_aidet_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *parent_np;
+ struct irq_domain *parent_domain;
+ struct uniphier_aidet_priv *priv;
+ struct resource *res;
+
+ parent_np = of_irq_find_parent(dev->of_node);
+ if (!parent_np)
+ return -ENXIO;
+
+ parent_domain = irq_find_host(parent_np);
+ of_node_put(parent_np);
+ if (!parent_domain)
+ return -EPROBE_DEFER;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->reg_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->reg_base))
+ return PTR_ERR(priv->reg_base);
+
+ spin_lock_init(&priv->lock);
+
+ priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
+ UNIPHIER_AIDET_NR_IRQS,
+ dev->of_node,
+ &uniphier_aidet_domain_ops,
+ priv);
+ if (!priv->domain)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
+{
+ struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+ priv->saved_vals[i] = readl_relaxed(
+ priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+ return 0;
+}
+
+static int __maybe_unused uniphier_aidet_resume(struct device *dev)
+{
+ struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+ writel_relaxed(priv->saved_vals[i],
+ priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+ return 0;
+}
+
+static const struct dev_pm_ops uniphier_aidet_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
+ uniphier_aidet_resume)
+};
+
+static const struct of_device_id uniphier_aidet_match[] = {
+ { .compatible = "socionext,uniphier-ld4-aidet" },
+ { .compatible = "socionext,uniphier-pro4-aidet" },
+ { .compatible = "socionext,uniphier-sld8-aidet" },
+ { .compatible = "socionext,uniphier-pro5-aidet" },
+ { .compatible = "socionext,uniphier-pxs2-aidet" },
+ { .compatible = "socionext,uniphier-ld11-aidet" },
+ { .compatible = "socionext,uniphier-ld20-aidet" },
+ { .compatible = "socionext,uniphier-pxs3-aidet" },
+ { /* sentinel */ }
+};
+
+static struct platform_driver uniphier_aidet_driver = {
+ .probe = uniphier_aidet_probe,
+ .driver = {
+ .name = "uniphier-aidet",
+ .of_match_table = uniphier_aidet_match,
+ .pm = &uniphier_aidet_pm_ops,
+ },
+};
+builtin_platform_driver(uniphier_aidet_driver);
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:01 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-21 10:01 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Mark Rutland, devicetree, Masahiro Yamada, Greg Kroah-Hartman,
Randy Dunlap, linux-kernel, Jassi Brar, Rob Herring,
Hans Verkuil, Masami Hiramatsu, Mauro Carvalho Chehab,
David S. Miller, linux-arm-kernel
UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
to provide additional features that are not covered by GIC. The main
purpose is to provide logic inverter to support low level and falling
edge trigger type for interrupt lines from on-board devices.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Use readl/write_relaxed instead of readl/writel
- Move val = 0 for code symmetry
- .alloc() hook only supports nr_irqs==1 case
- Move struct irq_chip to static data because this driver expects
only one device instance in the system
- Match the interrupt number to SPI of GIC to make DT binding clearer
(register offset starts from 0x4)
.../socionext,uniphier-aidet.txt | 32 +++
MAINTAINERS | 1 +
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
5 files changed, 288 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
new file mode 100644
index 000000000000..48e71d3ac2ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
@@ -0,0 +1,32 @@
+UniPhier AIDET
+
+UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
+Interrupt Controller). GIC itself can handle only high level and rising edge
+interrupts. The AIDET provides logic inverter to support low level and falling
+edge interrupts.
+
+Required properties:
+- compatible: Should be one of the following:
+ "socionext,uniphier-ld4-aidet" - for LD4 SoC
+ "socionext,uniphier-pro4-aidet" - for Pro4 SoC
+ "socionext,uniphier-sld8-aidet" - for sLD8 SoC
+ "socionext,uniphier-pro5-aidet" - for Pro5 SoC
+ "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
+ "socionext,uniphier-ld11-aidet" - for LD11 SoC
+ "socionext,uniphier-ld20-aidet" - for LD20 SoC
+ "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
+- reg: Specifies offset and length of the register set for the device.
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
+ source. The value should be 2. The first cell defines the interrupt number
+ (corresponds to the SPI interrupt number of GIC). The second cell specifies
+ the trigger type as defined in interrupts.txt in this directory.
+
+Example:
+
+ aidet: aidet@5fc20000 {
+ compatible = "socionext,uniphier-pro4-aidet";
+ reg = <0x5fc20000 0x200>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1c3feffb1c1c..7b84f047b3fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1993,6 +1993,7 @@ F: arch/arm64/boot/dts/socionext/
F: drivers/bus/uniphier-system-bus.c
F: drivers/clk/uniphier/
F: drivers/i2c/busses/i2c-uniphier*
+F: drivers/irqchip/irq-uniphier-aidet.c
F: drivers/pinctrl/uniphier/
F: drivers/reset/reset-uniphier.c
F: drivers/tty/serial/8250/8250_uniphier.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f1fd5f44d1d4..d1f43cb92e4d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -306,3 +306,11 @@ config QCOM_IRQ_COMBINER
help
Say yes here to add support for the IRQ combiner devices embedded
in Qualcomm Technologies chips.
+
+config IRQ_UNIPHIER_AIDET
+ bool "UniPhier AIDET support" if COMPILE_TEST
+ depends on ARCH_UNIPHIER || COMPILE_TEST
+ default ARCH_UNIPHIER
+ select IRQ_DOMAIN_HIERARCHY
+ help
+ Support for the UniPhier AIDET (ARM Interrupt Detector).
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e88d856cc09c..2c630574986f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
+obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
new file mode 100644
index 000000000000..418427f0d8e5
--- /dev/null
+++ b/drivers/irqchip/irq-uniphier-aidet.c
@@ -0,0 +1,246 @@
+/*
+ * Driver for UniPhier AIDET (ARM Interrupt Detector)
+ *
+ * Copyright (C) 2017 Socionext Inc.
+ * Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define UNIPHIER_AIDET_NR_IRQS 256
+
+#define UNIPHIER_AIDET_DETCONF 0x04 /* inverter register base */
+
+struct uniphier_aidet_priv {
+ struct irq_domain *domain;
+ void __iomem *reg_base;
+ spinlock_t lock;
+ u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
+};
+
+static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
+ unsigned int reg, u32 mask, u32 val)
+{
+ unsigned long flags;
+ u32 tmp;
+
+ spin_lock_irqsave(&priv->lock, flags);
+ tmp = readl_relaxed(priv->reg_base + reg);
+ tmp &= ~mask;
+ tmp |= mask & val;
+ writel_relaxed(tmp, priv->reg_base + reg);
+ spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
+ unsigned long index, unsigned int val)
+{
+ unsigned int reg;
+ u32 mask;
+
+ reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
+ mask = BIT(index % 32);
+
+ uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
+}
+
+static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct uniphier_aidet_priv *priv = data->chip_data;
+ unsigned int val;
+
+ /* enable inverter for active-low triggers */
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_LEVEL_HIGH:
+ val = 0;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ val = 1;
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ val = 1;
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ uniphier_aidet_detconf_update(priv, data->hwirq, val);
+
+ return irq_chip_set_type_parent(data, type);
+}
+
+static struct irq_chip uniphier_aidet_irq_chip = {
+ .name = "AIDET",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = uniphier_aidet_irq_set_type,
+};
+
+static int uniphier_aidet_domain_translate(struct irq_domain *domain,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (WARN_ON(fwspec->param_count < 2))
+ return -EINVAL;
+
+ *out_hwirq = fwspec->param[0];
+ *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+ return 0;
+}
+
+static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret;
+
+ if (nr_irqs != 1)
+ return -EINVAL;
+
+ ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
+ return -ENXIO;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ &uniphier_aidet_irq_chip,
+ domain->host_data);
+ if (ret)
+ return ret;
+
+ /* parent is GIC */
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0] = 0; /* SPI */
+ parent_fwspec.param[1] = hwirq;
+ parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+}
+
+static const struct irq_domain_ops uniphier_aidet_domain_ops = {
+ .alloc = uniphier_aidet_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+ .translate = uniphier_aidet_domain_translate,
+};
+
+static int uniphier_aidet_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *parent_np;
+ struct irq_domain *parent_domain;
+ struct uniphier_aidet_priv *priv;
+ struct resource *res;
+
+ parent_np = of_irq_find_parent(dev->of_node);
+ if (!parent_np)
+ return -ENXIO;
+
+ parent_domain = irq_find_host(parent_np);
+ of_node_put(parent_np);
+ if (!parent_domain)
+ return -EPROBE_DEFER;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->reg_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->reg_base))
+ return PTR_ERR(priv->reg_base);
+
+ spin_lock_init(&priv->lock);
+
+ priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
+ UNIPHIER_AIDET_NR_IRQS,
+ dev->of_node,
+ &uniphier_aidet_domain_ops,
+ priv);
+ if (!priv->domain)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
+{
+ struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+ priv->saved_vals[i] = readl_relaxed(
+ priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+ return 0;
+}
+
+static int __maybe_unused uniphier_aidet_resume(struct device *dev)
+{
+ struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+ writel_relaxed(priv->saved_vals[i],
+ priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+ return 0;
+}
+
+static const struct dev_pm_ops uniphier_aidet_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
+ uniphier_aidet_resume)
+};
+
+static const struct of_device_id uniphier_aidet_match[] = {
+ { .compatible = "socionext,uniphier-ld4-aidet" },
+ { .compatible = "socionext,uniphier-pro4-aidet" },
+ { .compatible = "socionext,uniphier-sld8-aidet" },
+ { .compatible = "socionext,uniphier-pro5-aidet" },
+ { .compatible = "socionext,uniphier-pxs2-aidet" },
+ { .compatible = "socionext,uniphier-ld11-aidet" },
+ { .compatible = "socionext,uniphier-ld20-aidet" },
+ { .compatible = "socionext,uniphier-pxs3-aidet" },
+ { /* sentinel */ }
+};
+
+static struct platform_driver uniphier_aidet_driver = {
+ .probe = uniphier_aidet_probe,
+ .driver = {
+ .name = "uniphier-aidet",
+ .of_match_table = uniphier_aidet_match,
+ .pm = &uniphier_aidet_pm_ops,
+ },
+};
+builtin_platform_driver(uniphier_aidet_driver);
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:01 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-21 10:01 UTC (permalink / raw)
To: linux-arm-kernel
UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
to provide additional features that are not covered by GIC. The main
purpose is to provide logic inverter to support low level and falling
edge trigger type for interrupt lines from on-board devices.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Use readl/write_relaxed instead of readl/writel
- Move val = 0 for code symmetry
- .alloc() hook only supports nr_irqs==1 case
- Move struct irq_chip to static data because this driver expects
only one device instance in the system
- Match the interrupt number to SPI of GIC to make DT binding clearer
(register offset starts from 0x4)
.../socionext,uniphier-aidet.txt | 32 +++
MAINTAINERS | 1 +
drivers/irqchip/Kconfig | 8 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
5 files changed, 288 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
new file mode 100644
index 000000000000..48e71d3ac2ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
@@ -0,0 +1,32 @@
+UniPhier AIDET
+
+UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
+Interrupt Controller). GIC itself can handle only high level and rising edge
+interrupts. The AIDET provides logic inverter to support low level and falling
+edge interrupts.
+
+Required properties:
+- compatible: Should be one of the following:
+ "socionext,uniphier-ld4-aidet" - for LD4 SoC
+ "socionext,uniphier-pro4-aidet" - for Pro4 SoC
+ "socionext,uniphier-sld8-aidet" - for sLD8 SoC
+ "socionext,uniphier-pro5-aidet" - for Pro5 SoC
+ "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
+ "socionext,uniphier-ld11-aidet" - for LD11 SoC
+ "socionext,uniphier-ld20-aidet" - for LD20 SoC
+ "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
+- reg: Specifies offset and length of the register set for the device.
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
+ source. The value should be 2. The first cell defines the interrupt number
+ (corresponds to the SPI interrupt number of GIC). The second cell specifies
+ the trigger type as defined in interrupts.txt in this directory.
+
+Example:
+
+ aidet: aidet at 5fc20000 {
+ compatible = "socionext,uniphier-pro4-aidet";
+ reg = <0x5fc20000 0x200>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1c3feffb1c1c..7b84f047b3fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1993,6 +1993,7 @@ F: arch/arm64/boot/dts/socionext/
F: drivers/bus/uniphier-system-bus.c
F: drivers/clk/uniphier/
F: drivers/i2c/busses/i2c-uniphier*
+F: drivers/irqchip/irq-uniphier-aidet.c
F: drivers/pinctrl/uniphier/
F: drivers/reset/reset-uniphier.c
F: drivers/tty/serial/8250/8250_uniphier.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f1fd5f44d1d4..d1f43cb92e4d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -306,3 +306,11 @@ config QCOM_IRQ_COMBINER
help
Say yes here to add support for the IRQ combiner devices embedded
in Qualcomm Technologies chips.
+
+config IRQ_UNIPHIER_AIDET
+ bool "UniPhier AIDET support" if COMPILE_TEST
+ depends on ARCH_UNIPHIER || COMPILE_TEST
+ default ARCH_UNIPHIER
+ select IRQ_DOMAIN_HIERARCHY
+ help
+ Support for the UniPhier AIDET (ARM Interrupt Detector).
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e88d856cc09c..2c630574986f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
+obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
new file mode 100644
index 000000000000..418427f0d8e5
--- /dev/null
+++ b/drivers/irqchip/irq-uniphier-aidet.c
@@ -0,0 +1,246 @@
+/*
+ * Driver for UniPhier AIDET (ARM Interrupt Detector)
+ *
+ * Copyright (C) 2017 Socionext Inc.
+ * Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define UNIPHIER_AIDET_NR_IRQS 256
+
+#define UNIPHIER_AIDET_DETCONF 0x04 /* inverter register base */
+
+struct uniphier_aidet_priv {
+ struct irq_domain *domain;
+ void __iomem *reg_base;
+ spinlock_t lock;
+ u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
+};
+
+static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
+ unsigned int reg, u32 mask, u32 val)
+{
+ unsigned long flags;
+ u32 tmp;
+
+ spin_lock_irqsave(&priv->lock, flags);
+ tmp = readl_relaxed(priv->reg_base + reg);
+ tmp &= ~mask;
+ tmp |= mask & val;
+ writel_relaxed(tmp, priv->reg_base + reg);
+ spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
+ unsigned long index, unsigned int val)
+{
+ unsigned int reg;
+ u32 mask;
+
+ reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
+ mask = BIT(index % 32);
+
+ uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
+}
+
+static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct uniphier_aidet_priv *priv = data->chip_data;
+ unsigned int val;
+
+ /* enable inverter for active-low triggers */
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_LEVEL_HIGH:
+ val = 0;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ val = 1;
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ val = 1;
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ uniphier_aidet_detconf_update(priv, data->hwirq, val);
+
+ return irq_chip_set_type_parent(data, type);
+}
+
+static struct irq_chip uniphier_aidet_irq_chip = {
+ .name = "AIDET",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = uniphier_aidet_irq_set_type,
+};
+
+static int uniphier_aidet_domain_translate(struct irq_domain *domain,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (WARN_ON(fwspec->param_count < 2))
+ return -EINVAL;
+
+ *out_hwirq = fwspec->param[0];
+ *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+ return 0;
+}
+
+static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret;
+
+ if (nr_irqs != 1)
+ return -EINVAL;
+
+ ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
+ return -ENXIO;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ &uniphier_aidet_irq_chip,
+ domain->host_data);
+ if (ret)
+ return ret;
+
+ /* parent is GIC */
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0] = 0; /* SPI */
+ parent_fwspec.param[1] = hwirq;
+ parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+}
+
+static const struct irq_domain_ops uniphier_aidet_domain_ops = {
+ .alloc = uniphier_aidet_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+ .translate = uniphier_aidet_domain_translate,
+};
+
+static int uniphier_aidet_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *parent_np;
+ struct irq_domain *parent_domain;
+ struct uniphier_aidet_priv *priv;
+ struct resource *res;
+
+ parent_np = of_irq_find_parent(dev->of_node);
+ if (!parent_np)
+ return -ENXIO;
+
+ parent_domain = irq_find_host(parent_np);
+ of_node_put(parent_np);
+ if (!parent_domain)
+ return -EPROBE_DEFER;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->reg_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->reg_base))
+ return PTR_ERR(priv->reg_base);
+
+ spin_lock_init(&priv->lock);
+
+ priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
+ UNIPHIER_AIDET_NR_IRQS,
+ dev->of_node,
+ &uniphier_aidet_domain_ops,
+ priv);
+ if (!priv->domain)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
+{
+ struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+ priv->saved_vals[i] = readl_relaxed(
+ priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+ return 0;
+}
+
+static int __maybe_unused uniphier_aidet_resume(struct device *dev)
+{
+ struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+ writel_relaxed(priv->saved_vals[i],
+ priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+ return 0;
+}
+
+static const struct dev_pm_ops uniphier_aidet_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
+ uniphier_aidet_resume)
+};
+
+static const struct of_device_id uniphier_aidet_match[] = {
+ { .compatible = "socionext,uniphier-ld4-aidet" },
+ { .compatible = "socionext,uniphier-pro4-aidet" },
+ { .compatible = "socionext,uniphier-sld8-aidet" },
+ { .compatible = "socionext,uniphier-pro5-aidet" },
+ { .compatible = "socionext,uniphier-pxs2-aidet" },
+ { .compatible = "socionext,uniphier-ld11-aidet" },
+ { .compatible = "socionext,uniphier-ld20-aidet" },
+ { .compatible = "socionext,uniphier-pxs3-aidet" },
+ { /* sentinel */ }
+};
+
+static struct platform_driver uniphier_aidet_driver = {
+ .probe = uniphier_aidet_probe,
+ .driver = {
+ .name = "uniphier-aidet",
+ .of_match_table = uniphier_aidet_match,
+ .pm = &uniphier_aidet_pm_ops,
+ },
+};
+builtin_platform_driver(uniphier_aidet_driver);
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:25 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-21 10:25 UTC (permalink / raw)
To: Masahiro Yamada, Thomas Gleixner, Jason Cooper
Cc: Masami Hiramatsu, Jassi Brar, Mauro Carvalho Chehab,
Hans Verkuil, Randy Dunlap, devicetree, linux-kernel,
David S. Miller, Rob Herring, Greg Kroah-Hartman, Mark Rutland,
linux-arm-kernel
On 21/08/17 11:01, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
> to provide additional features that are not covered by GIC. The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2:
> - Use readl/write_relaxed instead of readl/writel
> - Move val = 0 for code symmetry
> - .alloc() hook only supports nr_irqs==1 case
> - Move struct irq_chip to static data because this driver expects
> only one device instance in the system
> - Match the interrupt number to SPI of GIC to make DT binding clearer
> (register offset starts from 0x4)
>
> .../socionext,uniphier-aidet.txt | 32 +++
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
> 5 files changed, 288 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> new file mode 100644
> index 000000000000..48e71d3ac2ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> @@ -0,0 +1,32 @@
> +UniPhier AIDET
> +
> +UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
> +Interrupt Controller). GIC itself can handle only high level and rising edge
> +interrupts. The AIDET provides logic inverter to support low level and falling
> +edge interrupts.
> +
> +Required properties:
> +- compatible: Should be one of the following:
> + "socionext,uniphier-ld4-aidet" - for LD4 SoC
> + "socionext,uniphier-pro4-aidet" - for Pro4 SoC
> + "socionext,uniphier-sld8-aidet" - for sLD8 SoC
> + "socionext,uniphier-pro5-aidet" - for Pro5 SoC
> + "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
> + "socionext,uniphier-ld11-aidet" - for LD11 SoC
> + "socionext,uniphier-ld20-aidet" - for LD20 SoC
> + "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
> +- reg: Specifies offset and length of the register set for the device.
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
> + source. The value should be 2. The first cell defines the interrupt number
> + (corresponds to the SPI interrupt number of GIC). The second cell specifies
> + the trigger type as defined in interrupts.txt in this directory.
> +
> +Example:
> +
> + aidet: aidet@5fc20000 {
> + compatible = "socionext,uniphier-pro4-aidet";
> + reg = <0x5fc20000 0x200>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1c3feffb1c1c..7b84f047b3fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1993,6 +1993,7 @@ F: arch/arm64/boot/dts/socionext/
> F: drivers/bus/uniphier-system-bus.c
> F: drivers/clk/uniphier/
> F: drivers/i2c/busses/i2c-uniphier*
> +F: drivers/irqchip/irq-uniphier-aidet.c
> F: drivers/pinctrl/uniphier/
> F: drivers/reset/reset-uniphier.c
> F: drivers/tty/serial/8250/8250_uniphier.c
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index f1fd5f44d1d4..d1f43cb92e4d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -306,3 +306,11 @@ config QCOM_IRQ_COMBINER
> help
> Say yes here to add support for the IRQ combiner devices embedded
> in Qualcomm Technologies chips.
> +
> +config IRQ_UNIPHIER_AIDET
> + bool "UniPhier AIDET support" if COMPILE_TEST
> + depends on ARCH_UNIPHIER || COMPILE_TEST
> + default ARCH_UNIPHIER
> + select IRQ_DOMAIN_HIERARCHY
> + help
> + Support for the UniPhier AIDET (ARM Interrupt Detector).
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856cc09c..2c630574986f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
> obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
> +obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
> new file mode 100644
> index 000000000000..418427f0d8e5
> --- /dev/null
> +++ b/drivers/irqchip/irq-uniphier-aidet.c
> @@ -0,0 +1,246 @@
> +/*
> + * Driver for UniPhier AIDET (ARM Interrupt Detector)
> + *
> + * Copyright (C) 2017 Socionext Inc.
> + * Author: Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define UNIPHIER_AIDET_NR_IRQS 256
> +
> +#define UNIPHIER_AIDET_DETCONF 0x04 /* inverter register base */
> +
> +struct uniphier_aidet_priv {
> + struct irq_domain *domain;
> + void __iomem *reg_base;
> + spinlock_t lock;
> + u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
> +};
> +
> +static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
> + unsigned int reg, u32 mask, u32 val)
> +{
> + unsigned long flags;
> + u32 tmp;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + tmp = readl_relaxed(priv->reg_base + reg);
> + tmp &= ~mask;
> + tmp |= mask & val;
> + writel_relaxed(tmp, priv->reg_base + reg);
> + spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
> + unsigned long index, unsigned int val)
> +{
> + unsigned int reg;
> + u32 mask;
> +
> + reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
> + mask = BIT(index % 32);
> +
> + uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
> +}
> +
> +static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> + struct uniphier_aidet_priv *priv = data->chip_data;
> + unsigned int val;
> +
> + /* enable inverter for active-low triggers */
> + switch (type) {
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_LEVEL_HIGH:
> + val = 0;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + val = 1;
> + type = IRQ_TYPE_EDGE_RISING;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + val = 1;
> + type = IRQ_TYPE_LEVEL_HIGH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + uniphier_aidet_detconf_update(priv, data->hwirq, val);
> +
> + return irq_chip_set_type_parent(data, type);
> +}
> +
> +static struct irq_chip uniphier_aidet_irq_chip = {
> + .name = "AIDET",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_type = uniphier_aidet_irq_set_type,
Is this irqchip only used in a uniprocessor system? If not, how is the
interrupt affinity managed without a irq_set_affinity callback?
> +};
> +
> +static int uniphier_aidet_domain_translate(struct irq_domain *domain,
> + struct irq_fwspec *fwspec,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (WARN_ON(fwspec->param_count < 2))
> + return -EINVAL;
> +
> + *out_hwirq = fwspec->param[0];
> + *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> + return 0;
> +}
> +
> +static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs,
> + void *arg)
> +{
> + struct irq_fwspec parent_fwspec;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> + int ret;
> +
> + if (nr_irqs != 1)
> + return -EINVAL;
> +
> + ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
> + return -ENXIO;
> +
> + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> + &uniphier_aidet_irq_chip,
> + domain->host_data);
> + if (ret)
> + return ret;
> +
> + /* parent is GIC */
> + parent_fwspec.fwnode = domain->parent->fwnode;
> + parent_fwspec.param_count = 3;
> + parent_fwspec.param[0] = 0; /* SPI */
> + parent_fwspec.param[1] = hwirq;
> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
Why defer it to later? You already have the right information in "type",
so you might as well provide it immediately.
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops uniphier_aidet_domain_ops = {
> + .alloc = uniphier_aidet_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> + .translate = uniphier_aidet_domain_translate,
> +};
> +
> +static int uniphier_aidet_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *parent_np;
> + struct irq_domain *parent_domain;
> + struct uniphier_aidet_priv *priv;
> + struct resource *res;
> +
> + parent_np = of_irq_find_parent(dev->of_node);
> + if (!parent_np)
> + return -ENXIO;
> +
> + parent_domain = irq_find_host(parent_np);
> + of_node_put(parent_np);
> + if (!parent_domain)
> + return -EPROBE_DEFER;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->reg_base))
> + return PTR_ERR(priv->reg_base);
> +
> + spin_lock_init(&priv->lock);
> +
> + priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
> + UNIPHIER_AIDET_NR_IRQS,
> + dev->of_node,
> + &uniphier_aidet_domain_ops,
> + priv);
Nit: please use irq_domain_create_hierarchy. This does the exact same
thing, but is consistent with the use of fwnode instead of mixing
of_node and fwnode in this code.
> + if (!priv->domain)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
> +{
> + struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> + priv->saved_vals[i] = readl_relaxed(
> + priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_resume(struct device *dev)
> +{
> + struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> + writel_relaxed(priv->saved_vals[i],
> + priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops uniphier_aidet_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
> + uniphier_aidet_resume)
> +};
> +
> +static const struct of_device_id uniphier_aidet_match[] = {
> + { .compatible = "socionext,uniphier-ld4-aidet" },
> + { .compatible = "socionext,uniphier-pro4-aidet" },
> + { .compatible = "socionext,uniphier-sld8-aidet" },
> + { .compatible = "socionext,uniphier-pro5-aidet" },
> + { .compatible = "socionext,uniphier-pxs2-aidet" },
> + { .compatible = "socionext,uniphier-ld11-aidet" },
> + { .compatible = "socionext,uniphier-ld20-aidet" },
> + { .compatible = "socionext,uniphier-pxs3-aidet" },
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver uniphier_aidet_driver = {
> + .probe = uniphier_aidet_probe,
> + .driver = {
> + .name = "uniphier-aidet",
> + .of_match_table = uniphier_aidet_match,
> + .pm = &uniphier_aidet_pm_ops,
> + },
> +};
> +builtin_platform_driver(uniphier_aidet_driver);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:25 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-21 10:25 UTC (permalink / raw)
To: Masahiro Yamada, Thomas Gleixner, Jason Cooper
Cc: Masami Hiramatsu, Jassi Brar, Mauro Carvalho Chehab,
Hans Verkuil, Randy Dunlap, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
Rob Herring, Greg Kroah-Hartman, Mark Rutland,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 21/08/17 11:01, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
> to provide additional features that are not covered by GIC. The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> ---
>
> Changes in v2:
> - Use readl/write_relaxed instead of readl/writel
> - Move val = 0 for code symmetry
> - .alloc() hook only supports nr_irqs==1 case
> - Move struct irq_chip to static data because this driver expects
> only one device instance in the system
> - Match the interrupt number to SPI of GIC to make DT binding clearer
> (register offset starts from 0x4)
>
> .../socionext,uniphier-aidet.txt | 32 +++
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
> 5 files changed, 288 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> new file mode 100644
> index 000000000000..48e71d3ac2ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> @@ -0,0 +1,32 @@
> +UniPhier AIDET
> +
> +UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
> +Interrupt Controller). GIC itself can handle only high level and rising edge
> +interrupts. The AIDET provides logic inverter to support low level and falling
> +edge interrupts.
> +
> +Required properties:
> +- compatible: Should be one of the following:
> + "socionext,uniphier-ld4-aidet" - for LD4 SoC
> + "socionext,uniphier-pro4-aidet" - for Pro4 SoC
> + "socionext,uniphier-sld8-aidet" - for sLD8 SoC
> + "socionext,uniphier-pro5-aidet" - for Pro5 SoC
> + "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
> + "socionext,uniphier-ld11-aidet" - for LD11 SoC
> + "socionext,uniphier-ld20-aidet" - for LD20 SoC
> + "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
> +- reg: Specifies offset and length of the register set for the device.
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
> + source. The value should be 2. The first cell defines the interrupt number
> + (corresponds to the SPI interrupt number of GIC). The second cell specifies
> + the trigger type as defined in interrupts.txt in this directory.
> +
> +Example:
> +
> + aidet: aidet@5fc20000 {
> + compatible = "socionext,uniphier-pro4-aidet";
> + reg = <0x5fc20000 0x200>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1c3feffb1c1c..7b84f047b3fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1993,6 +1993,7 @@ F: arch/arm64/boot/dts/socionext/
> F: drivers/bus/uniphier-system-bus.c
> F: drivers/clk/uniphier/
> F: drivers/i2c/busses/i2c-uniphier*
> +F: drivers/irqchip/irq-uniphier-aidet.c
> F: drivers/pinctrl/uniphier/
> F: drivers/reset/reset-uniphier.c
> F: drivers/tty/serial/8250/8250_uniphier.c
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index f1fd5f44d1d4..d1f43cb92e4d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -306,3 +306,11 @@ config QCOM_IRQ_COMBINER
> help
> Say yes here to add support for the IRQ combiner devices embedded
> in Qualcomm Technologies chips.
> +
> +config IRQ_UNIPHIER_AIDET
> + bool "UniPhier AIDET support" if COMPILE_TEST
> + depends on ARCH_UNIPHIER || COMPILE_TEST
> + default ARCH_UNIPHIER
> + select IRQ_DOMAIN_HIERARCHY
> + help
> + Support for the UniPhier AIDET (ARM Interrupt Detector).
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856cc09c..2c630574986f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
> obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
> +obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
> new file mode 100644
> index 000000000000..418427f0d8e5
> --- /dev/null
> +++ b/drivers/irqchip/irq-uniphier-aidet.c
> @@ -0,0 +1,246 @@
> +/*
> + * Driver for UniPhier AIDET (ARM Interrupt Detector)
> + *
> + * Copyright (C) 2017 Socionext Inc.
> + * Author: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define UNIPHIER_AIDET_NR_IRQS 256
> +
> +#define UNIPHIER_AIDET_DETCONF 0x04 /* inverter register base */
> +
> +struct uniphier_aidet_priv {
> + struct irq_domain *domain;
> + void __iomem *reg_base;
> + spinlock_t lock;
> + u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
> +};
> +
> +static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
> + unsigned int reg, u32 mask, u32 val)
> +{
> + unsigned long flags;
> + u32 tmp;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + tmp = readl_relaxed(priv->reg_base + reg);
> + tmp &= ~mask;
> + tmp |= mask & val;
> + writel_relaxed(tmp, priv->reg_base + reg);
> + spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
> + unsigned long index, unsigned int val)
> +{
> + unsigned int reg;
> + u32 mask;
> +
> + reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
> + mask = BIT(index % 32);
> +
> + uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
> +}
> +
> +static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> + struct uniphier_aidet_priv *priv = data->chip_data;
> + unsigned int val;
> +
> + /* enable inverter for active-low triggers */
> + switch (type) {
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_LEVEL_HIGH:
> + val = 0;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + val = 1;
> + type = IRQ_TYPE_EDGE_RISING;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + val = 1;
> + type = IRQ_TYPE_LEVEL_HIGH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + uniphier_aidet_detconf_update(priv, data->hwirq, val);
> +
> + return irq_chip_set_type_parent(data, type);
> +}
> +
> +static struct irq_chip uniphier_aidet_irq_chip = {
> + .name = "AIDET",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_type = uniphier_aidet_irq_set_type,
Is this irqchip only used in a uniprocessor system? If not, how is the
interrupt affinity managed without a irq_set_affinity callback?
> +};
> +
> +static int uniphier_aidet_domain_translate(struct irq_domain *domain,
> + struct irq_fwspec *fwspec,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (WARN_ON(fwspec->param_count < 2))
> + return -EINVAL;
> +
> + *out_hwirq = fwspec->param[0];
> + *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> + return 0;
> +}
> +
> +static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs,
> + void *arg)
> +{
> + struct irq_fwspec parent_fwspec;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> + int ret;
> +
> + if (nr_irqs != 1)
> + return -EINVAL;
> +
> + ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
> + return -ENXIO;
> +
> + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> + &uniphier_aidet_irq_chip,
> + domain->host_data);
> + if (ret)
> + return ret;
> +
> + /* parent is GIC */
> + parent_fwspec.fwnode = domain->parent->fwnode;
> + parent_fwspec.param_count = 3;
> + parent_fwspec.param[0] = 0; /* SPI */
> + parent_fwspec.param[1] = hwirq;
> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
Why defer it to later? You already have the right information in "type",
so you might as well provide it immediately.
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops uniphier_aidet_domain_ops = {
> + .alloc = uniphier_aidet_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> + .translate = uniphier_aidet_domain_translate,
> +};
> +
> +static int uniphier_aidet_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *parent_np;
> + struct irq_domain *parent_domain;
> + struct uniphier_aidet_priv *priv;
> + struct resource *res;
> +
> + parent_np = of_irq_find_parent(dev->of_node);
> + if (!parent_np)
> + return -ENXIO;
> +
> + parent_domain = irq_find_host(parent_np);
> + of_node_put(parent_np);
> + if (!parent_domain)
> + return -EPROBE_DEFER;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->reg_base))
> + return PTR_ERR(priv->reg_base);
> +
> + spin_lock_init(&priv->lock);
> +
> + priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
> + UNIPHIER_AIDET_NR_IRQS,
> + dev->of_node,
> + &uniphier_aidet_domain_ops,
> + priv);
Nit: please use irq_domain_create_hierarchy. This does the exact same
thing, but is consistent with the use of fwnode instead of mixing
of_node and fwnode in this code.
> + if (!priv->domain)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
> +{
> + struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> + priv->saved_vals[i] = readl_relaxed(
> + priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_resume(struct device *dev)
> +{
> + struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> + writel_relaxed(priv->saved_vals[i],
> + priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops uniphier_aidet_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
> + uniphier_aidet_resume)
> +};
> +
> +static const struct of_device_id uniphier_aidet_match[] = {
> + { .compatible = "socionext,uniphier-ld4-aidet" },
> + { .compatible = "socionext,uniphier-pro4-aidet" },
> + { .compatible = "socionext,uniphier-sld8-aidet" },
> + { .compatible = "socionext,uniphier-pro5-aidet" },
> + { .compatible = "socionext,uniphier-pxs2-aidet" },
> + { .compatible = "socionext,uniphier-ld11-aidet" },
> + { .compatible = "socionext,uniphier-ld20-aidet" },
> + { .compatible = "socionext,uniphier-pxs3-aidet" },
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver uniphier_aidet_driver = {
> + .probe = uniphier_aidet_probe,
> + .driver = {
> + .name = "uniphier-aidet",
> + .of_match_table = uniphier_aidet_match,
> + .pm = &uniphier_aidet_pm_ops,
> + },
> +};
> +builtin_platform_driver(uniphier_aidet_driver);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:25 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-21 10:25 UTC (permalink / raw)
To: linux-arm-kernel
On 21/08/17 11:01, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
> to provide additional features that are not covered by GIC. The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2:
> - Use readl/write_relaxed instead of readl/writel
> - Move val = 0 for code symmetry
> - .alloc() hook only supports nr_irqs==1 case
> - Move struct irq_chip to static data because this driver expects
> only one device instance in the system
> - Match the interrupt number to SPI of GIC to make DT binding clearer
> (register offset starts from 0x4)
>
> .../socionext,uniphier-aidet.txt | 32 +++
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
> 5 files changed, 288 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> new file mode 100644
> index 000000000000..48e71d3ac2ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> @@ -0,0 +1,32 @@
> +UniPhier AIDET
> +
> +UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
> +Interrupt Controller). GIC itself can handle only high level and rising edge
> +interrupts. The AIDET provides logic inverter to support low level and falling
> +edge interrupts.
> +
> +Required properties:
> +- compatible: Should be one of the following:
> + "socionext,uniphier-ld4-aidet" - for LD4 SoC
> + "socionext,uniphier-pro4-aidet" - for Pro4 SoC
> + "socionext,uniphier-sld8-aidet" - for sLD8 SoC
> + "socionext,uniphier-pro5-aidet" - for Pro5 SoC
> + "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
> + "socionext,uniphier-ld11-aidet" - for LD11 SoC
> + "socionext,uniphier-ld20-aidet" - for LD20 SoC
> + "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
> +- reg: Specifies offset and length of the register set for the device.
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
> + source. The value should be 2. The first cell defines the interrupt number
> + (corresponds to the SPI interrupt number of GIC). The second cell specifies
> + the trigger type as defined in interrupts.txt in this directory.
> +
> +Example:
> +
> + aidet: aidet at 5fc20000 {
> + compatible = "socionext,uniphier-pro4-aidet";
> + reg = <0x5fc20000 0x200>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1c3feffb1c1c..7b84f047b3fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1993,6 +1993,7 @@ F: arch/arm64/boot/dts/socionext/
> F: drivers/bus/uniphier-system-bus.c
> F: drivers/clk/uniphier/
> F: drivers/i2c/busses/i2c-uniphier*
> +F: drivers/irqchip/irq-uniphier-aidet.c
> F: drivers/pinctrl/uniphier/
> F: drivers/reset/reset-uniphier.c
> F: drivers/tty/serial/8250/8250_uniphier.c
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index f1fd5f44d1d4..d1f43cb92e4d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -306,3 +306,11 @@ config QCOM_IRQ_COMBINER
> help
> Say yes here to add support for the IRQ combiner devices embedded
> in Qualcomm Technologies chips.
> +
> +config IRQ_UNIPHIER_AIDET
> + bool "UniPhier AIDET support" if COMPILE_TEST
> + depends on ARCH_UNIPHIER || COMPILE_TEST
> + default ARCH_UNIPHIER
> + select IRQ_DOMAIN_HIERARCHY
> + help
> + Support for the UniPhier AIDET (ARM Interrupt Detector).
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856cc09c..2c630574986f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
> obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
> +obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
> new file mode 100644
> index 000000000000..418427f0d8e5
> --- /dev/null
> +++ b/drivers/irqchip/irq-uniphier-aidet.c
> @@ -0,0 +1,246 @@
> +/*
> + * Driver for UniPhier AIDET (ARM Interrupt Detector)
> + *
> + * Copyright (C) 2017 Socionext Inc.
> + * Author: Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define UNIPHIER_AIDET_NR_IRQS 256
> +
> +#define UNIPHIER_AIDET_DETCONF 0x04 /* inverter register base */
> +
> +struct uniphier_aidet_priv {
> + struct irq_domain *domain;
> + void __iomem *reg_base;
> + spinlock_t lock;
> + u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
> +};
> +
> +static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
> + unsigned int reg, u32 mask, u32 val)
> +{
> + unsigned long flags;
> + u32 tmp;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + tmp = readl_relaxed(priv->reg_base + reg);
> + tmp &= ~mask;
> + tmp |= mask & val;
> + writel_relaxed(tmp, priv->reg_base + reg);
> + spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
> + unsigned long index, unsigned int val)
> +{
> + unsigned int reg;
> + u32 mask;
> +
> + reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
> + mask = BIT(index % 32);
> +
> + uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
> +}
> +
> +static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> + struct uniphier_aidet_priv *priv = data->chip_data;
> + unsigned int val;
> +
> + /* enable inverter for active-low triggers */
> + switch (type) {
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_LEVEL_HIGH:
> + val = 0;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + val = 1;
> + type = IRQ_TYPE_EDGE_RISING;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + val = 1;
> + type = IRQ_TYPE_LEVEL_HIGH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + uniphier_aidet_detconf_update(priv, data->hwirq, val);
> +
> + return irq_chip_set_type_parent(data, type);
> +}
> +
> +static struct irq_chip uniphier_aidet_irq_chip = {
> + .name = "AIDET",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_type = uniphier_aidet_irq_set_type,
Is this irqchip only used in a uniprocessor system? If not, how is the
interrupt affinity managed without a irq_set_affinity callback?
> +};
> +
> +static int uniphier_aidet_domain_translate(struct irq_domain *domain,
> + struct irq_fwspec *fwspec,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (WARN_ON(fwspec->param_count < 2))
> + return -EINVAL;
> +
> + *out_hwirq = fwspec->param[0];
> + *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> + return 0;
> +}
> +
> +static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs,
> + void *arg)
> +{
> + struct irq_fwspec parent_fwspec;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> + int ret;
> +
> + if (nr_irqs != 1)
> + return -EINVAL;
> +
> + ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
> + return -ENXIO;
> +
> + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> + &uniphier_aidet_irq_chip,
> + domain->host_data);
> + if (ret)
> + return ret;
> +
> + /* parent is GIC */
> + parent_fwspec.fwnode = domain->parent->fwnode;
> + parent_fwspec.param_count = 3;
> + parent_fwspec.param[0] = 0; /* SPI */
> + parent_fwspec.param[1] = hwirq;
> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
Why defer it to later? You already have the right information in "type",
so you might as well provide it immediately.
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops uniphier_aidet_domain_ops = {
> + .alloc = uniphier_aidet_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> + .translate = uniphier_aidet_domain_translate,
> +};
> +
> +static int uniphier_aidet_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *parent_np;
> + struct irq_domain *parent_domain;
> + struct uniphier_aidet_priv *priv;
> + struct resource *res;
> +
> + parent_np = of_irq_find_parent(dev->of_node);
> + if (!parent_np)
> + return -ENXIO;
> +
> + parent_domain = irq_find_host(parent_np);
> + of_node_put(parent_np);
> + if (!parent_domain)
> + return -EPROBE_DEFER;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->reg_base))
> + return PTR_ERR(priv->reg_base);
> +
> + spin_lock_init(&priv->lock);
> +
> + priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
> + UNIPHIER_AIDET_NR_IRQS,
> + dev->of_node,
> + &uniphier_aidet_domain_ops,
> + priv);
Nit: please use irq_domain_create_hierarchy. This does the exact same
thing, but is consistent with the use of fwnode instead of mixing
of_node and fwnode in this code.
> + if (!priv->domain)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
> +{
> + struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> + priv->saved_vals[i] = readl_relaxed(
> + priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_resume(struct device *dev)
> +{
> + struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> + writel_relaxed(priv->saved_vals[i],
> + priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops uniphier_aidet_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
> + uniphier_aidet_resume)
> +};
> +
> +static const struct of_device_id uniphier_aidet_match[] = {
> + { .compatible = "socionext,uniphier-ld4-aidet" },
> + { .compatible = "socionext,uniphier-pro4-aidet" },
> + { .compatible = "socionext,uniphier-sld8-aidet" },
> + { .compatible = "socionext,uniphier-pro5-aidet" },
> + { .compatible = "socionext,uniphier-pxs2-aidet" },
> + { .compatible = "socionext,uniphier-ld11-aidet" },
> + { .compatible = "socionext,uniphier-ld20-aidet" },
> + { .compatible = "socionext,uniphier-pxs3-aidet" },
> + { /* sentinel */ }
> +};
> +
> +static struct platform_driver uniphier_aidet_driver = {
> + .probe = uniphier_aidet_probe,
> + .driver = {
> + .name = "uniphier-aidet",
> + .of_match_table = uniphier_aidet_match,
> + .pm = &uniphier_aidet_pm_ops,
> + },
> +};
> +builtin_platform_driver(uniphier_aidet_driver);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
2017-08-21 10:25 ` Marc Zyngier
@ 2017-08-21 10:54 ` Masahiro Yamada
-1 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-21 10:54 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jason Cooper, Masami Hiramatsu, Jassi Brar,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap, devicetree,
Linux Kernel Mailing List, David S. Miller, Rob Herring,
Greg Kroah-Hartman, Mark Rutland, linux-arm-kernel
Hi Mark,
2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 21/08/17 11:01, Masahiro Yamada wrote:
>> +static struct irq_chip uniphier_aidet_irq_chip = {
>> + .name = "AIDET",
>> + .irq_mask = irq_chip_mask_parent,
>> + .irq_unmask = irq_chip_unmask_parent,
>> + .irq_eoi = irq_chip_eoi_parent,
>> + .irq_set_type = uniphier_aidet_irq_set_type,
>
> Is this irqchip only used in a uniprocessor system? If not, how is the
> interrupt affinity managed without a irq_set_affinity callback?
I missed that.
I will set irq_chip_set_affinity_parent.
>> + /* parent is GIC */
>> + parent_fwspec.fwnode = domain->parent->fwnode;
>> + parent_fwspec.param_count = 3;
>> + parent_fwspec.param[0] = 0; /* SPI */
>> + parent_fwspec.param[1] = hwirq;
>> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
>
> Why defer it to later? You already have the right information in "type",
> so you might as well provide it immediately.
Because .irq_set_type() will set it up
before the IRQ is really in use.
If we look gic_irq_domain_alloc() implementation,
it does not care "type".
gic_set_type() will manipulate hardware registers.
Having said that, it shouldn't hurt to set type here.
>> +
>> + priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
>> + UNIPHIER_AIDET_NR_IRQS,
>> + dev->of_node,
>> + &uniphier_aidet_domain_ops,
>> + priv);
>
> Nit: please use irq_domain_create_hierarchy. This does the exact same
> thing, but is consistent with the use of fwnode instead of mixing
> of_node and fwnode in this code.
>
I will do so.
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 10:54 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-21 10:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi Mark,
2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 21/08/17 11:01, Masahiro Yamada wrote:
>> +static struct irq_chip uniphier_aidet_irq_chip = {
>> + .name = "AIDET",
>> + .irq_mask = irq_chip_mask_parent,
>> + .irq_unmask = irq_chip_unmask_parent,
>> + .irq_eoi = irq_chip_eoi_parent,
>> + .irq_set_type = uniphier_aidet_irq_set_type,
>
> Is this irqchip only used in a uniprocessor system? If not, how is the
> interrupt affinity managed without a irq_set_affinity callback?
I missed that.
I will set irq_chip_set_affinity_parent.
>> + /* parent is GIC */
>> + parent_fwspec.fwnode = domain->parent->fwnode;
>> + parent_fwspec.param_count = 3;
>> + parent_fwspec.param[0] = 0; /* SPI */
>> + parent_fwspec.param[1] = hwirq;
>> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
>
> Why defer it to later? You already have the right information in "type",
> so you might as well provide it immediately.
Because .irq_set_type() will set it up
before the IRQ is really in use.
If we look gic_irq_domain_alloc() implementation,
it does not care "type".
gic_set_type() will manipulate hardware registers.
Having said that, it shouldn't hurt to set type here.
>> +
>> + priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
>> + UNIPHIER_AIDET_NR_IRQS,
>> + dev->of_node,
>> + &uniphier_aidet_domain_ops,
>> + priv);
>
> Nit: please use irq_domain_create_hierarchy. This does the exact same
> thing, but is consistent with the use of fwnode instead of mixing
> of_node and fwnode in this code.
>
I will do so.
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 11:53 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-21 11:53 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Thomas Gleixner, Jason Cooper, Masami Hiramatsu, Jassi Brar,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap, devicetree,
Linux Kernel Mailing List, David S. Miller, Rob Herring,
Greg Kroah-Hartman, Mark Rutland, linux-arm-kernel
On 21/08/17 11:54, Masahiro Yamada wrote:
>>> + /* parent is GIC */
>>> + parent_fwspec.fwnode = domain->parent->fwnode;
>>> + parent_fwspec.param_count = 3;
>>> + parent_fwspec.param[0] = 0; /* SPI */
>>> + parent_fwspec.param[1] = hwirq;
>>> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
>>
>> Why defer it to later? You already have the right information in "type",
>> so you might as well provide it immediately.
>
>
> Because .irq_set_type() will set it up
> before the IRQ is really in use.
>
>
> If we look gic_irq_domain_alloc() implementation,
> it does not care "type".
>
> gic_set_type() will manipulate hardware registers.
But that's out of the scope of this driver. Whatever the GIC driver does
(or doesn't), you should pass it the right information.
> Having said that, it shouldn't hurt to set type here.
Exactly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 11:53 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-21 11:53 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Thomas Gleixner, Jason Cooper, Masami Hiramatsu, Jassi Brar,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List,
David S. Miller, Rob Herring, Greg Kroah-Hartman, Mark Rutland,
linux-arm-kernel
On 21/08/17 11:54, Masahiro Yamada wrote:
>>> + /* parent is GIC */
>>> + parent_fwspec.fwnode = domain->parent->fwnode;
>>> + parent_fwspec.param_count = 3;
>>> + parent_fwspec.param[0] = 0; /* SPI */
>>> + parent_fwspec.param[1] = hwirq;
>>> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
>>
>> Why defer it to later? You already have the right information in "type",
>> so you might as well provide it immediately.
>
>
> Because .irq_set_type() will set it up
> before the IRQ is really in use.
>
>
> If we look gic_irq_domain_alloc() implementation,
> it does not care "type".
>
> gic_set_type() will manipulate hardware registers.
But that's out of the scope of this driver. Whatever the GIC driver does
(or doesn't), you should pass it the right information.
> Having said that, it shouldn't hurt to set type here.
Exactly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-21 11:53 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-21 11:53 UTC (permalink / raw)
To: linux-arm-kernel
On 21/08/17 11:54, Masahiro Yamada wrote:
>>> + /* parent is GIC */
>>> + parent_fwspec.fwnode = domain->parent->fwnode;
>>> + parent_fwspec.param_count = 3;
>>> + parent_fwspec.param[0] = 0; /* SPI */
>>> + parent_fwspec.param[1] = hwirq;
>>> + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; /* properly set later */
>>
>> Why defer it to later? You already have the right information in "type",
>> so you might as well provide it immediately.
>
>
> Because .irq_set_type() will set it up
> before the IRQ is really in use.
>
>
> If we look gic_irq_domain_alloc() implementation,
> it does not care "type".
>
> gic_set_type() will manipulate hardware registers.
But that's out of the scope of this driver. Whatever the GIC driver does
(or doesn't), you should pass it the right information.
> Having said that, it shouldn't hurt to set type here.
Exactly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
2017-08-21 10:25 ` Marc Zyngier
@ 2017-08-22 2:03 ` Masahiro Yamada
-1 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-22 2:03 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jason Cooper, Masami Hiramatsu, Jassi Brar,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap, devicetree,
Linux Kernel Mailing List, David S. Miller, Rob Herring,
Greg Kroah-Hartman, Mark Rutland, linux-arm-kernel
Hi Mark,
2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>> +static struct irq_chip uniphier_aidet_irq_chip = {
>> + .name = "AIDET",
>> + .irq_mask = irq_chip_mask_parent,
>> + .irq_unmask = irq_chip_unmask_parent,
>> + .irq_eoi = irq_chip_eoi_parent,
>> + .irq_set_type = uniphier_aidet_irq_set_type,
>
> Is this irqchip only used in a uniprocessor system? If not, how is the
> interrupt affinity managed without a irq_set_affinity callback?
>
After consideration, some questions popped up.
We can set other hooks, for example, .irq_{enable,disable} if we like.
.irq_enable = irq_chip_enable_parent,
.irq_disable = irq_chip_disable_parent,
I know the parent (GIC) implements unmask/mask instead of enable/disable,
but this is also out of the scope of this driver.
I am not familiar with the difference between unmask/mask and enable/disable.
IIUC, the difference is that
if enable/disable hooks are missing, IRQs are masked lazily.
If a child irqchip implemented enable/disable,
IRQs would be masked immediately. So, in irq-domain hierarchy,
a child irqchip need to have a good insight about its parent
which is be better, unmask/mask or enable/disable.
> Nit: please use irq_domain_create_hierarchy.
I'd like to know your intention about your commit
2a5e9a072da6469a37d1f0b1577416f51223c280
Is that mean, irq_domain_add_hierarchy will be deprecated
some time in the future?
If I grep under drivers/irqchip/,
most drivers are currently using irq_domain_add_hierarchy(),
and this provides a shorter form for DT-based drivers.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-22 2:03 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-22 2:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Mark,
2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>> +static struct irq_chip uniphier_aidet_irq_chip = {
>> + .name = "AIDET",
>> + .irq_mask = irq_chip_mask_parent,
>> + .irq_unmask = irq_chip_unmask_parent,
>> + .irq_eoi = irq_chip_eoi_parent,
>> + .irq_set_type = uniphier_aidet_irq_set_type,
>
> Is this irqchip only used in a uniprocessor system? If not, how is the
> interrupt affinity managed without a irq_set_affinity callback?
>
After consideration, some questions popped up.
We can set other hooks, for example, .irq_{enable,disable} if we like.
.irq_enable = irq_chip_enable_parent,
.irq_disable = irq_chip_disable_parent,
I know the parent (GIC) implements unmask/mask instead of enable/disable,
but this is also out of the scope of this driver.
I am not familiar with the difference between unmask/mask and enable/disable.
IIUC, the difference is that
if enable/disable hooks are missing, IRQs are masked lazily.
If a child irqchip implemented enable/disable,
IRQs would be masked immediately. So, in irq-domain hierarchy,
a child irqchip need to have a good insight about its parent
which is be better, unmask/mask or enable/disable.
> Nit: please use irq_domain_create_hierarchy.
I'd like to know your intention about your commit
2a5e9a072da6469a37d1f0b1577416f51223c280
Is that mean, irq_domain_add_hierarchy will be deprecated
some time in the future?
If I grep under drivers/irqchip/,
most drivers are currently using irq_domain_add_hierarchy(),
and this provides a shorter form for DT-based drivers.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
2017-08-22 2:03 ` Masahiro Yamada
@ 2017-08-22 8:20 ` Marc Zyngier
-1 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-22 8:20 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Thomas Gleixner, Jason Cooper, Masami Hiramatsu, Jassi Brar,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap, devicetree,
Linux Kernel Mailing List, David S. Miller, Rob Herring,
Greg Kroah-Hartman, Mark Rutland, linux-arm-kernel
On 22/08/17 03:03, Masahiro Yamada wrote:
> Hi Mark,
>
>
> 2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>
>>> +static struct irq_chip uniphier_aidet_irq_chip = {
>>> + .name = "AIDET",
>>> + .irq_mask = irq_chip_mask_parent,
>>> + .irq_unmask = irq_chip_unmask_parent,
>>> + .irq_eoi = irq_chip_eoi_parent,
>>> + .irq_set_type = uniphier_aidet_irq_set_type,
>>
>> Is this irqchip only used in a uniprocessor system? If not, how is the
>> interrupt affinity managed without a irq_set_affinity callback?
>>
>
>
> After consideration, some questions popped up.
>
>
>
> We can set other hooks, for example, .irq_{enable,disable} if we like.
>
> .irq_enable = irq_chip_enable_parent,
> .irq_disable = irq_chip_disable_parent,
>
>
> I know the parent (GIC) implements unmask/mask instead of enable/disable,
> but this is also out of the scope of this driver.
>
> I am not familiar with the difference between unmask/mask and enable/disable.
> IIUC, the difference is that
> if enable/disable hooks are missing, IRQs are masked lazily.
That's a rather good thing. Disabling interrupts lazily is a net
performance gain when you you have to repeatedly mask/unmask interrupts.
> If a child irqchip implemented enable/disable,
> IRQs would be masked immediately. So, in irq-domain hierarchy,
> a child irqchip need to have a good insight about its parent
> which is be better, unmask/mask or enable/disable.
Not necessarily. irq_chip_enable_parent will call unmask if enable is
not implemented in the parent. But you'll loose the benefit of lazy
masking of interrupts routed through this controller.
There are many other things that are *much* worse, like the need to
implement a irq_eoi callback even if the irqchip has no such concept.
>> Nit: please use irq_domain_create_hierarchy.
>
> I'd like to know your intention about your commit
> 2a5e9a072da6469a37d1f0b1577416f51223c280
>
> Is that mean, irq_domain_add_hierarchy will be deprecated
> some time in the future?
That's the intent, as we're moving towards a firmware-agnostic
irq_domain layer. Think of it as a deprecated interface.
> If I grep under drivers/irqchip/,
> most drivers are currently using irq_domain_add_hierarchy(),
> and this provides a shorter form for DT-based drivers.
Yes, we have a lot of legacy, and I don't always catch new additions.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-22 8:20 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-22 8:20 UTC (permalink / raw)
To: linux-arm-kernel
On 22/08/17 03:03, Masahiro Yamada wrote:
> Hi Mark,
>
>
> 2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>
>>> +static struct irq_chip uniphier_aidet_irq_chip = {
>>> + .name = "AIDET",
>>> + .irq_mask = irq_chip_mask_parent,
>>> + .irq_unmask = irq_chip_unmask_parent,
>>> + .irq_eoi = irq_chip_eoi_parent,
>>> + .irq_set_type = uniphier_aidet_irq_set_type,
>>
>> Is this irqchip only used in a uniprocessor system? If not, how is the
>> interrupt affinity managed without a irq_set_affinity callback?
>>
>
>
> After consideration, some questions popped up.
>
>
>
> We can set other hooks, for example, .irq_{enable,disable} if we like.
>
> .irq_enable = irq_chip_enable_parent,
> .irq_disable = irq_chip_disable_parent,
>
>
> I know the parent (GIC) implements unmask/mask instead of enable/disable,
> but this is also out of the scope of this driver.
>
> I am not familiar with the difference between unmask/mask and enable/disable.
> IIUC, the difference is that
> if enable/disable hooks are missing, IRQs are masked lazily.
That's a rather good thing. Disabling interrupts lazily is a net
performance gain when you you have to repeatedly mask/unmask interrupts.
> If a child irqchip implemented enable/disable,
> IRQs would be masked immediately. So, in irq-domain hierarchy,
> a child irqchip need to have a good insight about its parent
> which is be better, unmask/mask or enable/disable.
Not necessarily. irq_chip_enable_parent will call unmask if enable is
not implemented in the parent. But you'll loose the benefit of lazy
masking of interrupts routed through this controller.
There are many other things that are *much* worse, like the need to
implement a irq_eoi callback even if the irqchip has no such concept.
>> Nit: please use irq_domain_create_hierarchy.
>
> I'd like to know your intention about your commit
> 2a5e9a072da6469a37d1f0b1577416f51223c280
>
> Is that mean, irq_domain_add_hierarchy will be deprecated
> some time in the future?
That's the intent, as we're moving towards a firmware-agnostic
irq_domain layer. Think of it as a deprecated interface.
> If I grep under drivers/irqchip/,
> most drivers are currently using irq_domain_add_hierarchy(),
> and this provides a shorter form for DT-based drivers.
Yes, we have a lot of legacy, and I don't always catch new additions.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-23 0:40 ` Rob Herring
0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2017-08-23 0:40 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland,
devicetree, Greg Kroah-Hartman, Randy Dunlap, linux-kernel,
Jassi Brar, Hans Verkuil, Masami Hiramatsu,
Mauro Carvalho Chehab, David S. Miller, linux-arm-kernel
On Mon, Aug 21, 2017 at 07:01:03PM +0900, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
> to provide additional features that are not covered by GIC. The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2:
> - Use readl/write_relaxed instead of readl/writel
> - Move val = 0 for code symmetry
> - .alloc() hook only supports nr_irqs==1 case
> - Move struct irq_chip to static data because this driver expects
> only one device instance in the system
> - Match the interrupt number to SPI of GIC to make DT binding clearer
> (register offset starts from 0x4)
>
> .../socionext,uniphier-aidet.txt | 32 +++
Acked-by: Rob Herring <robh@kernel.org>
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
> 5 files changed, 288 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-23 0:40 ` Rob Herring
0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2017-08-23 0:40 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman,
Randy Dunlap, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jassi Brar,
Hans Verkuil, Masami Hiramatsu, Mauro Carvalho Chehab,
David S. Miller,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Mon, Aug 21, 2017 at 07:01:03PM +0900, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
> to provide additional features that are not covered by GIC. The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> ---
>
> Changes in v2:
> - Use readl/write_relaxed instead of readl/writel
> - Move val = 0 for code symmetry
> - .alloc() hook only supports nr_irqs==1 case
> - Move struct irq_chip to static data because this driver expects
> only one device instance in the system
> - Match the interrupt number to SPI of GIC to make DT binding clearer
> (register offset starts from 0x4)
>
> .../socionext,uniphier-aidet.txt | 32 +++
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
> 5 files changed, 288 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-23 0:40 ` Rob Herring
0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2017-08-23 0:40 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Aug 21, 2017 at 07:01:03PM +0900, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector). This is intended
> to provide additional features that are not covered by GIC. The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2:
> - Use readl/write_relaxed instead of readl/writel
> - Move val = 0 for code symmetry
> - .alloc() hook only supports nr_irqs==1 case
> - Move struct irq_chip to static data because this driver expects
> only one device instance in the system
> - Match the interrupt number to SPI of GIC to make DT binding clearer
> (register offset starts from 0x4)
>
> .../socionext,uniphier-aidet.txt | 32 +++
Acked-by: Rob Herring <robh@kernel.org>
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-uniphier-aidet.c | 246 +++++++++++++++++++++
> 5 files changed, 288 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
2017-08-22 8:20 ` Marc Zyngier
@ 2017-08-23 1:30 ` Masahiro Yamada
-1 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-23 1:30 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jason Cooper, Masami Hiramatsu, Jassi Brar,
Mauro Carvalho Chehab, Hans Verkuil, Randy Dunlap, devicetree,
Linux Kernel Mailing List, David S. Miller, Rob Herring,
Greg Kroah-Hartman, Mark Rutland, linux-arm-kernel
Hi Marc,
2017-08-22 17:20 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 22/08/17 03:03, Masahiro Yamada wrote:
>> Hi Mark,
>>
>>
>> 2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>>
>>>> +static struct irq_chip uniphier_aidet_irq_chip = {
>>>> + .name = "AIDET",
>>>> + .irq_mask = irq_chip_mask_parent,
>>>> + .irq_unmask = irq_chip_unmask_parent,
>>>> + .irq_eoi = irq_chip_eoi_parent,
>>>> + .irq_set_type = uniphier_aidet_irq_set_type,
>>>
>>> Is this irqchip only used in a uniprocessor system? If not, how is the
>>> interrupt affinity managed without a irq_set_affinity callback?
>>>
>>
>>
>> After consideration, some questions popped up.
>>
>>
>>
>> We can set other hooks, for example, .irq_{enable,disable} if we like.
>>
>> .irq_enable = irq_chip_enable_parent,
>> .irq_disable = irq_chip_disable_parent,
>>
>>
>> I know the parent (GIC) implements unmask/mask instead of enable/disable,
>> but this is also out of the scope of this driver.
>>
>> I am not familiar with the difference between unmask/mask and enable/disable.
>> IIUC, the difference is that
>> if enable/disable hooks are missing, IRQs are masked lazily.
>
> That's a rather good thing. Disabling interrupts lazily is a net
> performance gain when you you have to repeatedly mask/unmask interrupts.
>
>> If a child irqchip implemented enable/disable,
>> IRQs would be masked immediately. So, in irq-domain hierarchy,
>> a child irqchip need to have a good insight about its parent
>> which is be better, unmask/mask or enable/disable.
>
> Not necessarily. irq_chip_enable_parent will call unmask if enable is
> not implemented in the parent. But you'll loose the benefit of lazy
> masking of interrupts routed through this controller.
Right.
I will not add .irq_{enable/disable} to keep the benefit of lazy masking.
> There are many other things that are *much* worse, like the need to
> implement a irq_eoi callback even if the irqchip has no such concept.
Right. I noticed irq_eoi is mandatory when I tested my driver.
I notice handle_percpu_irq() and handle_percpu_devid_irq()
check the NULL pointer dereference like
if (chip->irq_eoi)
chip->irq_eoi(&desc->irq_data);
But, other flow handlers do not this.
>>> Nit: please use irq_domain_create_hierarchy.
>>
>> I'd like to know your intention about your commit
>> 2a5e9a072da6469a37d1f0b1577416f51223c280
>>
>> Is that mean, irq_domain_add_hierarchy will be deprecated
>> some time in the future?
>
> That's the intent, as we're moving towards a firmware-agnostic
> irq_domain layer. Think of it as a deprecated interface.
>
>> If I grep under drivers/irqchip/,
>> most drivers are currently using irq_domain_add_hierarchy(),
>> and this provides a shorter form for DT-based drivers.
> Yes, we have a lot of legacy, and I don't always catch new additions.
I see.
I hope all drivers will be converted and irq_domain_add_hierarchy() will be
deleted.
Having many variants with slight difference is confusing to driver developers.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
@ 2017-08-23 1:30 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2017-08-23 1:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi Marc,
2017-08-22 17:20 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 22/08/17 03:03, Masahiro Yamada wrote:
>> Hi Mark,
>>
>>
>> 2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>>
>>>> +static struct irq_chip uniphier_aidet_irq_chip = {
>>>> + .name = "AIDET",
>>>> + .irq_mask = irq_chip_mask_parent,
>>>> + .irq_unmask = irq_chip_unmask_parent,
>>>> + .irq_eoi = irq_chip_eoi_parent,
>>>> + .irq_set_type = uniphier_aidet_irq_set_type,
>>>
>>> Is this irqchip only used in a uniprocessor system? If not, how is the
>>> interrupt affinity managed without a irq_set_affinity callback?
>>>
>>
>>
>> After consideration, some questions popped up.
>>
>>
>>
>> We can set other hooks, for example, .irq_{enable,disable} if we like.
>>
>> .irq_enable = irq_chip_enable_parent,
>> .irq_disable = irq_chip_disable_parent,
>>
>>
>> I know the parent (GIC) implements unmask/mask instead of enable/disable,
>> but this is also out of the scope of this driver.
>>
>> I am not familiar with the difference between unmask/mask and enable/disable.
>> IIUC, the difference is that
>> if enable/disable hooks are missing, IRQs are masked lazily.
>
> That's a rather good thing. Disabling interrupts lazily is a net
> performance gain when you you have to repeatedly mask/unmask interrupts.
>
>> If a child irqchip implemented enable/disable,
>> IRQs would be masked immediately. So, in irq-domain hierarchy,
>> a child irqchip need to have a good insight about its parent
>> which is be better, unmask/mask or enable/disable.
>
> Not necessarily. irq_chip_enable_parent will call unmask if enable is
> not implemented in the parent. But you'll loose the benefit of lazy
> masking of interrupts routed through this controller.
Right.
I will not add .irq_{enable/disable} to keep the benefit of lazy masking.
> There are many other things that are *much* worse, like the need to
> implement a irq_eoi callback even if the irqchip has no such concept.
Right. I noticed irq_eoi is mandatory when I tested my driver.
I notice handle_percpu_irq() and handle_percpu_devid_irq()
check the NULL pointer dereference like
if (chip->irq_eoi)
chip->irq_eoi(&desc->irq_data);
But, other flow handlers do not this.
>>> Nit: please use irq_domain_create_hierarchy.
>>
>> I'd like to know your intention about your commit
>> 2a5e9a072da6469a37d1f0b1577416f51223c280
>>
>> Is that mean, irq_domain_add_hierarchy will be deprecated
>> some time in the future?
>
> That's the intent, as we're moving towards a firmware-agnostic
> irq_domain layer. Think of it as a deprecated interface.
>
>> If I grep under drivers/irqchip/,
>> most drivers are currently using irq_domain_add_hierarchy(),
>> and this provides a shorter form for DT-based drivers.
> Yes, we have a lot of legacy, and I don't always catch new additions.
I see.
I hope all drivers will be converted and irq_domain_add_hierarchy() will be
deleted.
Having many variants with slight difference is confusing to driver developers.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver
2017-08-23 1:30 ` Masahiro Yamada
(?)
@ 2017-08-23 7:06 ` Marc Zyngier
-1 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2017-08-23 7:06 UTC (permalink / raw)
To: linux-arm-kernel
On 2017-08-23 02:30, Masahiro Yamada wrote:
> Hi Marc,
>
> 2017-08-22 17:20 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>> On 22/08/17 03:03, Masahiro Yamada wrote:
>>> Hi Mark,
>>>
>>>
>>> 2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>>>
>>>>> +static struct irq_chip uniphier_aidet_irq_chip = {
>>>>> + .name = "AIDET",
>>>>> + .irq_mask = irq_chip_mask_parent,
>>>>> + .irq_unmask = irq_chip_unmask_parent,
>>>>> + .irq_eoi = irq_chip_eoi_parent,
>>>>> + .irq_set_type = uniphier_aidet_irq_set_type,
>>>>
>>>> Is this irqchip only used in a uniprocessor system? If not, how is
>>>> the
>>>> interrupt affinity managed without a irq_set_affinity callback?
>>>>
>>>
>>>
>>> After consideration, some questions popped up.
>>>
>>>
>>>
>>> We can set other hooks, for example, .irq_{enable,disable} if we
>>> like.
>>>
>>> .irq_enable = irq_chip_enable_parent,
>>> .irq_disable = irq_chip_disable_parent,
>>>
>>>
>>> I know the parent (GIC) implements unmask/mask instead of
>>> enable/disable,
>>> but this is also out of the scope of this driver.
>>>
>>> I am not familiar with the difference between unmask/mask and
>>> enable/disable.
>>> IIUC, the difference is that
>>> if enable/disable hooks are missing, IRQs are masked lazily.
>>
>> That's a rather good thing. Disabling interrupts lazily is a net
>> performance gain when you you have to repeatedly mask/unmask
>> interrupts.
>>
>>> If a child irqchip implemented enable/disable,
>>> IRQs would be masked immediately. So, in irq-domain hierarchy,
>>> a child irqchip need to have a good insight about its parent
>>> which is be better, unmask/mask or enable/disable.
>>
>> Not necessarily. irq_chip_enable_parent will call unmask if enable
>> is
>> not implemented in the parent. But you'll loose the benefit of lazy
>> masking of interrupts routed through this controller.
>
> Right.
> I will not add .irq_{enable/disable} to keep the benefit of lazy
> masking.
>
>
>
>
>> There are many other things that are *much* worse, like the need to
>> implement a irq_eoi callback even if the irqchip has no such
>> concept.
>
> Right. I noticed irq_eoi is mandatory when I tested my driver.
>
> I notice handle_percpu_irq() and handle_percpu_devid_irq()
> check the NULL pointer dereference like
>
> if (chip->irq_eoi)
> chip->irq_eoi(&desc->irq_data);
>
> But, other flow handlers do not this.
>
>
>
>
>>>> Nit: please use irq_domain_create_hierarchy.
>>>
>>> I'd like to know your intention about your commit
>>> 2a5e9a072da6469a37d1f0b1577416f51223c280
>>>
>>> Is that mean, irq_domain_add_hierarchy will be deprecated
>>> some time in the future?
>>
>> That's the intent, as we're moving towards a firmware-agnostic
>> irq_domain layer. Think of it as a deprecated interface.
>>
>>> If I grep under drivers/irqchip/,
>>> most drivers are currently using irq_domain_add_hierarchy(),
>>> and this provides a shorter form for DT-based drivers.
>> Yes, we have a lot of legacy, and I don't always catch new
>> additions.
>
> I see.
>
> I hope all drivers will be converted and irq_domain_add_hierarchy()
> will be
> deleted.
>
> Having many variants with slight difference is confusing to driver
> developers.
Feel free to submit patches removing all of the irq_domain_add_*().
With about 300 uses covering most non-x86 architectures, that will be
interesting to watch. I seriously doubt there is any value in that much
churn, and simply documenting the fact that this is irq_domain_add* is
not the preferred API would be just as efficient.
M.
--
Fast, cheap, reliable. Pick two.
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-08-23 7:06 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-21 10:01 [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver Masahiro Yamada
2017-08-21 10:01 ` Masahiro Yamada
2017-08-21 10:01 ` Masahiro Yamada
2017-08-21 10:25 ` Marc Zyngier
2017-08-21 10:25 ` Marc Zyngier
2017-08-21 10:25 ` Marc Zyngier
2017-08-21 10:54 ` Masahiro Yamada
2017-08-21 10:54 ` Masahiro Yamada
2017-08-21 11:53 ` Marc Zyngier
2017-08-21 11:53 ` Marc Zyngier
2017-08-21 11:53 ` Marc Zyngier
2017-08-22 2:03 ` Masahiro Yamada
2017-08-22 2:03 ` Masahiro Yamada
2017-08-22 8:20 ` Marc Zyngier
2017-08-22 8:20 ` Marc Zyngier
2017-08-23 1:30 ` Masahiro Yamada
2017-08-23 1:30 ` Masahiro Yamada
2017-08-23 7:06 ` Marc Zyngier
2017-08-23 0:40 ` Rob Herring
2017-08-23 0:40 ` Rob Herring
2017-08-23 0:40 ` Rob Herring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.