This series introduces support for Amazon's Annapurna Labs Fabric Interrupt
Controller.
The Amazon's Annapurna Labs FIC (Fabric Interrupt Controller) has 32
inputs/sources. This FIC may be cascaded into another FIC or connected
directly to the main CPU Interrupt Controller (e.g. GIC).
Changes since v1:
=================
- removing unused exported APIs
- updating cover letter and commit message accordingly
Talel Shenhar (2):
dt-bindings: interrupt-controller: Amazon's Annapurna Labs FIC
irqchip: al-fic: Introduce Amazon's Annapurna Labs Fabric Interrupt
Controller Driver
.../interrupt-controller/amazon,al-fic.txt | 22 ++
MAINTAINERS | 6 +
drivers/irqchip/Kconfig | 11 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-al-fic.c | 289 +++++++++++++++++++++
5 files changed, 329 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
create mode 100644 drivers/irqchip/irq-al-fic.c
--
2.7.4
Document Amazon's Annapurna Labs Fabric Interrupt Controller SoC binding.
Signed-off-by: Talel Shenhar <[email protected]>
---
.../interrupt-controller/amazon,al-fic.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
diff --git a/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
new file mode 100644
index 0000000..a2f31a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
@@ -0,0 +1,22 @@
+Amazon's Annapurna Labs Fabric Interrupt Controller
+
+Required properties:
+
+- compatible: should be "amazon,al-fic"
+- reg: physical base address and size of the registers
+- interrupt-controller: identifies the node as an interrupt controller
+- #interrupt-cells: must be 2.
+- interrupt-parent: specifies the parent interrupt controller.
+- interrupts: describes which input line in the interrupt parent, this
+ fic's output is connected to.
+
+Example:
+
+amazon_fic: amazon_fic {
+ compatible = "amazon,al-fic";
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ reg = <0x0 0xfd8a8500 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 0x0 IRQ_TYPE_LEVEL_HIGH>;
+};
--
2.7.4
The Amazon's Annapurna Labs Fabric Interrupt Controller has 32 inputs
lines. A FIC (Fabric Interrupt Controller) may be cascaded into another FIC
or directly to the main CPU Interrupt Controller (e.g. GIC).
Signed-off-by: Talel Shenhar <[email protected]>
---
MAINTAINERS | 6 +
drivers/irqchip/Kconfig | 11 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-al-fic.c | 289 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 307 insertions(+)
create mode 100644 drivers/irqchip/irq-al-fic.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f485597..b4f5255 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1209,6 +1209,12 @@ S: Maintained
F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.txt
F: drivers/irqchip/irq-vic.c
+AMAZON ANNAPURNA LABS FIC DRIVER
+M: Talel Shenhar <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
+F: drivers/irqchip/irq-al-fic.c
+
ARM SMMU DRIVERS
M: Will Deacon <[email protected]>
R: Robin Murphy <[email protected]>
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 51a5ef0..1e51f0f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -89,6 +89,17 @@ config ALPINE_MSI
select PCI_MSI
select GENERIC_IRQ_CHIP
+config AL_FIC
+ bool "Amazon's Annapurna Labs Fabric Interrupt Controller"
+ depends on OF || COMPILE_TEST
+ select GENERIC_IRQ_CHIP
+ select IRQ_DOMAIN
+ select GENERIC_IRQ_MULTI_HANDLER
+ select IRQ_DOMAIN_HIERARCHY
+ select SPARSE_IRQ
+ help
+ Support Amazon's Annapurna Labs Fabric Interrupt Controller.
+
config ATMEL_AIC_IRQ
bool
select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 794c13d..a20eba5 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_IRQCHIP) += irqchip.o
+obj-$(CONFIG_AL_FIC) += irq-al-fic.o
obj-$(CONFIG_ALPINE_MSI) += irq-alpine-msi.o
obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
obj-$(CONFIG_ATH79) += irq-ath79-misc.o
diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
new file mode 100644
index 0000000..484ef18
--- /dev/null
+++ b/drivers/irqchip/irq-al-fic.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+/* FIC Registers */
+#define AL_FIC_CAUSE 0x00
+#define AL_FIC_MASK 0x10
+#define AL_FIC_CONTROL 0x28
+
+#define CONTROL_TRIGGER_RISING BIT(3)
+#define CONTROL_MASK_MSI_X BIT(5)
+
+#define NR_FIC_IRQS 32
+
+MODULE_AUTHOR("Talel Shenhar");
+MODULE_DESCRIPTION("Amazon's Annapurna Labs Interrupt Controller Driver");
+MODULE_LICENSE("GPL v2");
+
+enum al_fic_state {
+ AL_FIC_CLEAN = 0,
+ AL_FIC_CONFIGURED_LEVEL,
+ AL_FIC_CONFIGURED_RAISING_EDGE,
+};
+
+struct al_fic {
+ void __iomem *base;
+ struct irq_domain *domain;
+ const char *name;
+ unsigned int parent_irq;
+ enum al_fic_state state;
+};
+
+static void al_fic_set_trigger(struct al_fic *fic,
+ struct irq_chip_generic *gc,
+ enum al_fic_state new_state)
+{
+ irq_flow_handler_t handler;
+ u32 control = readl(fic->base + AL_FIC_CONTROL);
+
+ if (new_state == AL_FIC_CONFIGURED_LEVEL) {
+ handler = handle_level_irq;
+ control &= ~CONTROL_TRIGGER_RISING;
+ } else {
+ handler = handle_edge_irq;
+ control |= CONTROL_TRIGGER_RISING;
+ }
+ gc->chip_types->handler = handler;
+ fic->state = new_state;
+ writel(control, fic->base + AL_FIC_CONTROL);
+}
+
+static int al_fic_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+ struct al_fic *fic = gc->private;
+ enum al_fic_state new_state;
+ int ret = 0;
+
+ irq_gc_lock(gc);
+
+ if (!(flow_type & IRQ_TYPE_LEVEL_HIGH) &&
+ !(flow_type & IRQ_TYPE_EDGE_RISING)) {
+ pr_err("fic doesn't support flow type %d\n", flow_type);
+ ret = -EPERM;
+ goto err;
+ }
+
+ new_state = (flow_type & IRQ_TYPE_LEVEL_HIGH) ?
+ AL_FIC_CONFIGURED_LEVEL : AL_FIC_CONFIGURED_RAISING_EDGE;
+
+ /* A given FIC instance can be either all level or all edge triggered.
+ * This is generally fixed depending on what pieces of HW it's wired up
+ * to.
+ *
+ * We configure it based on the sensitivity of the first source
+ * being setup, and reject any subsequent attempt at configuring it in a
+ * different way.
+ */
+ if (fic->state == AL_FIC_CLEAN) {
+ al_fic_set_trigger(fic, gc, new_state);
+ } else if (fic->state != new_state) {
+ pr_err("fic %s state already configured to %d\n",
+ fic->name, fic->state);
+ ret = -EPERM;
+ goto err;
+ }
+
+err:
+ irq_gc_unlock(gc);
+
+ return ret;
+}
+
+static void al_fic_irq_handler(struct irq_desc *desc)
+{
+ struct al_fic *fic = irq_desc_get_handler_data(desc);
+ struct irq_domain *domain = fic->domain;
+ struct irq_chip *irqchip = irq_desc_get_chip(desc);
+ struct irq_chip_generic *gc = irq_get_domain_generic_chip(domain, 0);
+ unsigned long pending;
+ unsigned int irq;
+ u32 hwirq;
+
+ chained_irq_enter(irqchip, desc);
+
+ pending = readl(fic->base + AL_FIC_CAUSE);
+ pending &= ~gc->mask_cache;
+
+ for_each_set_bit(hwirq, &pending, domain->revmap_size) {
+ irq = irq_find_mapping(domain, hwirq);
+ generic_handle_irq(irq);
+ }
+
+ chained_irq_exit(irqchip, desc);
+}
+
+static int al_fic_register(struct device_node *node,
+ struct al_fic *fic)
+{
+ struct irq_chip_generic *gc;
+ int ret;
+
+ fic->domain = irq_domain_add_linear(node,
+ NR_FIC_IRQS,
+ &irq_generic_chip_ops,
+ fic);
+ if (!fic->domain) {
+ pr_err("fail to add irq domain\n");
+ return -ENOMEM;
+ }
+
+ ret = irq_alloc_domain_generic_chips(fic->domain,
+ NR_FIC_IRQS,
+ 1, fic->name,
+ handle_level_irq,
+ 0, 0, IRQ_GC_INIT_MASK_CACHE);
+ if (ret) {
+ pr_err("fail to allocate generic chip (%d)\n", ret);
+ goto err_domain_remove;
+ }
+
+ gc = irq_get_domain_generic_chip(fic->domain, 0);
+ gc->reg_base = fic->base;
+ gc->chip_types->regs.mask = AL_FIC_MASK;
+ gc->chip_types->regs.ack = AL_FIC_CAUSE;
+ gc->chip_types->chip.irq_mask = irq_gc_mask_set_bit;
+ gc->chip_types->chip.irq_unmask = irq_gc_mask_clr_bit;
+ gc->chip_types->chip.irq_ack = irq_gc_ack_clr_bit;
+ gc->chip_types->chip.irq_set_type = al_fic_irq_set_type;
+ gc->chip_types->chip.flags = IRQCHIP_SKIP_SET_WAKE;
+ gc->private = fic;
+
+ irq_set_chained_handler_and_data(fic->parent_irq,
+ al_fic_irq_handler,
+ fic);
+ return 0;
+
+err_domain_remove:
+ irq_domain_remove(fic->domain);
+
+ return ret;
+}
+
+static void al_fic_hw_init(struct al_fic *fic)
+{
+ u32 control = CONTROL_MASK_MSI_X;
+
+ /* mask out all interrupts */
+ writel(0xFFFFFFFF, fic->base + AL_FIC_MASK);
+
+ /* clear any pending interrupt */
+ writel(0, fic->base + AL_FIC_CAUSE);
+
+ writel(control, fic->base + AL_FIC_CONTROL);
+}
+
+/**
+ * al_fic_wire_init() - initialize and configure fic in wire mode
+ * @of_node: optional pointer to interrupt controller's device tree node.
+ * @base: mmio to fic register
+ * @name: name of the fic
+ * @parent_irq: interrupt of parent
+ *
+ * This API will configure the fic hardware to to work in wire mode.
+ * In wire mode, fic hardware is generating a wire ("wired") interrupt.
+ * Interrupt can be generated based on positive edge or level - configuration is
+ * to be determined based on connected hardware to this fic.
+ *
+ * Returns fic context that allows the user to obtain the irq_domain by using
+ * al_fic_wire_get_domain().
+ */
+static struct al_fic *al_fic_wire_init(struct device_node *node,
+ void __iomem *base,
+ const char *name,
+ unsigned int parent_irq)
+{
+ struct al_fic *fic;
+ int ret;
+
+ if (!base)
+ return ERR_PTR(-EINVAL);
+
+ fic = kzalloc(sizeof(*fic), GFP_KERNEL);
+ if (!fic)
+ return ERR_PTR(-ENOMEM);
+
+ fic->base = base;
+ fic->parent_irq = parent_irq;
+ fic->name = (name ?: "al-fic-wire");
+
+ al_fic_hw_init(fic);
+
+ ret = al_fic_register(node, fic);
+ if (ret) {
+ pr_err("fail to register irqchip\n");
+ goto err_free;
+ }
+
+ pr_debug("%s initialized successfully in Legacy mode (parent-irq=%u)\n",
+ fic->name, parent_irq);
+
+ return fic;
+
+err_free:
+ kfree(fic);
+ return ERR_PTR(ret);
+}
+
+static int __init al_fic_init_dt(struct device_node *node,
+ struct device_node *parent)
+{
+ int ret;
+ void __iomem *base;
+ unsigned int parent_irq;
+ struct al_fic *fic;
+
+ if (!parent) {
+ pr_err("%s: unsupported - device require a parent\n",
+ node->name);
+ return -EINVAL;
+ }
+
+ base = of_iomap(node, 0);
+ if (!base) {
+ pr_err("%s: fail to map memory\n", node->name);
+ return -ENOMEM;
+ }
+
+ parent_irq = irq_of_parse_and_map(node, 0);
+ if (!parent_irq) {
+ pr_err("%s: fail to map irq\n", node->name);
+ ret = -EINVAL;
+ goto err_unmap;
+ }
+
+ fic = al_fic_wire_init(node,
+ base,
+ node->name,
+ parent_irq);
+ if (IS_ERR(fic)) {
+ pr_err("%s: fail to initialize irqchip (%lu)\n",
+ node->name,
+ PTR_ERR(fic));
+ ret = PTR_ERR(fic);
+ goto err_irq_dispose;
+ }
+
+ return 0;
+
+err_irq_dispose:
+ irq_dispose_mapping(parent_irq);
+err_unmap:
+ iounmap(base);
+
+ return ret;
+}
+
+IRQCHIP_DECLARE(al_fic, "amazon,al-fic", al_fic_init_dt);
--
2.7.4
On Wed, Jun 05, 2019 at 01:52:00PM +0300, Talel Shenhar wrote:
> Document Amazon's Annapurna Labs Fabric Interrupt Controller SoC binding.
>
> Signed-off-by: Talel Shenhar <[email protected]>
> ---
> .../interrupt-controller/amazon,al-fic.txt | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
> new file mode 100644
> index 0000000..a2f31a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
> @@ -0,0 +1,22 @@
> +Amazon's Annapurna Labs Fabric Interrupt Controller
> +
> +Required properties:
> +
> +- compatible: should be "amazon,al-fic"
> +- reg: physical base address and size of the registers
> +- interrupt-controller: identifies the node as an interrupt controller
> +- #interrupt-cells: must be 2.
> +- interrupt-parent: specifies the parent interrupt controller.
> +- interrupts: describes which input line in the interrupt parent, this
> + fic's output is connected to.
> +
> +Example:
> +
> +amazon_fic: amazon_fic {
above must be:
amazon_fic: interrupt-controller@0xfd8a8500 {
--
Regards,
Sudeep
Talel,
On 05/06/2019 11:52, Talel Shenhar wrote:
> The Amazon's Annapurna Labs Fabric Interrupt Controller has 32 inputs
> lines. A FIC (Fabric Interrupt Controller) may be cascaded into another FIC
Really? :-(
> or directly to the main CPU Interrupt Controller (e.g. GIC).
>
> Signed-off-by: Talel Shenhar <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/irqchip/Kconfig | 11 ++
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-al-fic.c | 289 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 307 insertions(+)
> create mode 100644 drivers/irqchip/irq-al-fic.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f485597..b4f5255 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1209,6 +1209,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/interrupt-controller/arm,vic.txt
> F: drivers/irqchip/irq-vic.c
>
> +AMAZON ANNAPURNA LABS FIC DRIVER
> +M: Talel Shenhar <[email protected]>
> +S: Maintained
> +F: Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
> +F: drivers/irqchip/irq-al-fic.c
> +
> ARM SMMU DRIVERS
> M: Will Deacon <[email protected]>
> R: Robin Murphy <[email protected]>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 51a5ef0..1e51f0f 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -89,6 +89,17 @@ config ALPINE_MSI
> select PCI_MSI
> select GENERIC_IRQ_CHIP
>
> +config AL_FIC
> + bool "Amazon's Annapurna Labs Fabric Interrupt Controller"
> + depends on OF || COMPILE_TEST
> + select GENERIC_IRQ_CHIP
> + select IRQ_DOMAIN
> + select GENERIC_IRQ_MULTI_HANDLER
> + select IRQ_DOMAIN_HIERARCHY
> + select SPARSE_IRQ
GENERIC_IRQ_MULTI_HANDLER and SPARSE_IRQ are to be selected by the
architecture only, and not the individual irqchip drivers.
IRQ_DOMAIN_HIERARCHY is bizarre as well, as this driver doesn't use
hierarchical domains at all.
> + help
> + Support Amazon's Annapurna Labs Fabric Interrupt Controller.
> +
> config ATMEL_AIC_IRQ
> bool
> select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 794c13d..a20eba5 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_IRQCHIP) += irqchip.o
>
> +obj-$(CONFIG_AL_FIC) += irq-al-fic.o
> obj-$(CONFIG_ALPINE_MSI) += irq-alpine-msi.o
> obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
> obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> new file mode 100644
> index 0000000..484ef18
> --- /dev/null
> +++ b/drivers/irqchip/irq-al-fic.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +/* FIC Registers */
> +#define AL_FIC_CAUSE 0x00
> +#define AL_FIC_MASK 0x10
> +#define AL_FIC_CONTROL 0x28
> +
> +#define CONTROL_TRIGGER_RISING BIT(3)
> +#define CONTROL_MASK_MSI_X BIT(5)
> +
> +#define NR_FIC_IRQS 32
> +
> +MODULE_AUTHOR("Talel Shenhar");
> +MODULE_DESCRIPTION("Amazon's Annapurna Labs Interrupt Controller Driver");
> +MODULE_LICENSE("GPL v2");
> +
> +enum al_fic_state {
> + AL_FIC_CLEAN = 0,
What does "CLEAN" mean in this context? Shouldn't it be "UNCONFIGURED"
instead?
> + AL_FIC_CONFIGURED_LEVEL,
> + AL_FIC_CONFIGURED_RAISING_EDGE,
s/RAISING/RISING/
> +};
> +
> +struct al_fic {
> + void __iomem *base;
> + struct irq_domain *domain;
> + const char *name;
> + unsigned int parent_irq;
> + enum al_fic_state state;
> +};
> +
> +static void al_fic_set_trigger(struct al_fic *fic,
> + struct irq_chip_generic *gc,
> + enum al_fic_state new_state)
> +{
> + irq_flow_handler_t handler;
> + u32 control = readl(fic->base + AL_FIC_CONTROL);
Please use relaxed accessors for all MMIO accesses. Nothing in this code
seem to warrant a bunch of DSBs.
> +
> + if (new_state == AL_FIC_CONFIGURED_LEVEL) {
> + handler = handle_level_irq;
> + control &= ~CONTROL_TRIGGER_RISING;
> + } else {
> + handler = handle_edge_irq;
> + control |= CONTROL_TRIGGER_RISING;
> + }
> + gc->chip_types->handler = handler;
> + fic->state = new_state;
> + writel(control, fic->base + AL_FIC_CONTROL);
> +}
> +
> +static int al_fic_irq_set_type(struct irq_data *data, unsigned int flow_type)
> +{
> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
> + struct al_fic *fic = gc->private;
> + enum al_fic_state new_state;
> + int ret = 0;
> +
> + irq_gc_lock(gc);
> +
> + if (!(flow_type & IRQ_TYPE_LEVEL_HIGH) &&
> + !(flow_type & IRQ_TYPE_EDGE_RISING)) {
And what if this gets passed EDGE_BOTH?
> + pr_err("fic doesn't support flow type %d\n", flow_type);
Drop the pr_err. Returning the error should be enough.
> + ret = -EPERM;
EPERM? What's the rational for such error code? I'd expect EINVAL.
> + goto err;
> + }
> +
> + new_state = (flow_type & IRQ_TYPE_LEVEL_HIGH) ?
> + AL_FIC_CONFIGURED_LEVEL : AL_FIC_CONFIGURED_RAISING_EDGE;
> +
> + /* A given FIC instance can be either all level or all edge triggered.
Comment format.
> + * This is generally fixed depending on what pieces of HW it's wired up
> + * to.
> + *
> + * We configure it based on the sensitivity of the first source
> + * being setup, and reject any subsequent attempt at configuring it in a
> + * different way.
Is that a reliable guess? It also strikes me that the DT binding doesn't
allow for the trigger type to be passed, meaning the individual drivers
have to request the trigger as part of their request_irq() call. I'd
rather you have a complete interrupt specifier in DT, and document the
various limitations of the HW.
> + */
> + if (fic->state == AL_FIC_CLEAN) {
> + al_fic_set_trigger(fic, gc, new_state);
> + } else if (fic->state != new_state) {
> + pr_err("fic %s state already configured to %d\n",
> + fic->name, fic->state);
> + ret = -EPERM;
Same as above.
> + goto err;
> + }
> +
> +err:
> + irq_gc_unlock(gc);
> +
> + return ret;
> +}
> +
> +static void al_fic_irq_handler(struct irq_desc *desc)
> +{
> + struct al_fic *fic = irq_desc_get_handler_data(desc);
> + struct irq_domain *domain = fic->domain;
> + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> + struct irq_chip_generic *gc = irq_get_domain_generic_chip(domain, 0);
> + unsigned long pending;
> + unsigned int irq;
> + u32 hwirq;
> +
> + chained_irq_enter(irqchip, desc);
> +
> + pending = readl(fic->base + AL_FIC_CAUSE);
> + pending &= ~gc->mask_cache;
> +
> + for_each_set_bit(hwirq, &pending, domain->revmap_size) {
I'd rather you don't rely on the internals of the irqdomain
implementation by using revmap_size. You know the exact number of
interrupts, as it is a hardcoded constant. Just use that.
> + irq = irq_find_mapping(domain, hwirq);
> + generic_handle_irq(irq);
> + }
> +
> + chained_irq_exit(irqchip, desc);
> +}
> +
> +static int al_fic_register(struct device_node *node,
> + struct al_fic *fic)
> +{
> + struct irq_chip_generic *gc;
> + int ret;
> +
> + fic->domain = irq_domain_add_linear(node,
> + NR_FIC_IRQS,
> + &irq_generic_chip_ops,
> + fic);
> + if (!fic->domain) {
> + pr_err("fail to add irq domain\n");
> + return -ENOMEM;
> + }
> +
> + ret = irq_alloc_domain_generic_chips(fic->domain,
> + NR_FIC_IRQS,
> + 1, fic->name,
> + handle_level_irq,
> + 0, 0, IRQ_GC_INIT_MASK_CACHE);
> + if (ret) {
> + pr_err("fail to allocate generic chip (%d)\n", ret);
> + goto err_domain_remove;
> + }
> +
> + gc = irq_get_domain_generic_chip(fic->domain, 0);
> + gc->reg_base = fic->base;
> + gc->chip_types->regs.mask = AL_FIC_MASK;
> + gc->chip_types->regs.ack = AL_FIC_CAUSE;
> + gc->chip_types->chip.irq_mask = irq_gc_mask_set_bit;
> + gc->chip_types->chip.irq_unmask = irq_gc_mask_clr_bit;
> + gc->chip_types->chip.irq_ack = irq_gc_ack_clr_bit;
> + gc->chip_types->chip.irq_set_type = al_fic_irq_set_type;
> + gc->chip_types->chip.flags = IRQCHIP_SKIP_SET_WAKE;
> + gc->private = fic;
> +
> + irq_set_chained_handler_and_data(fic->parent_irq,
> + al_fic_irq_handler,
> + fic);
> + return 0;
> +
> +err_domain_remove:
> + irq_domain_remove(fic->domain);
> +
> + return ret;
> +}
> +
> +static void al_fic_hw_init(struct al_fic *fic)
> +{
> + u32 control = CONTROL_MASK_MSI_X;
> +
> + /* mask out all interrupts */
> + writel(0xFFFFFFFF, fic->base + AL_FIC_MASK);> +
> + /* clear any pending interrupt */
> + writel(0, fic->base + AL_FIC_CAUSE);
> +
> + writel(control, fic->base + AL_FIC_CONTROL);
nit: You can directly write CONTROL_MASK_MSI_X there, no need for the
extra variable. And while you're at it, move this function into its caller.
> +}
> +
> +/**
> + * al_fic_wire_init() - initialize and configure fic in wire mode
> + * @of_node: optional pointer to interrupt controller's device tree node.
> + * @base: mmio to fic register
> + * @name: name of the fic
> + * @parent_irq: interrupt of parent
> + *
> + * This API will configure the fic hardware to to work in wire mode.
> + * In wire mode, fic hardware is generating a wire ("wired") interrupt.
> + * Interrupt can be generated based on positive edge or level - configuration is
> + * to be determined based on connected hardware to this fic.
> + *
> + * Returns fic context that allows the user to obtain the irq_domain by using
> + * al_fic_wire_get_domain().
This function is now thankfully gone.
> + */
> +static struct al_fic *al_fic_wire_init(struct device_node *node,
> + void __iomem *base,
> + const char *name,
> + unsigned int parent_irq)
> +{
> + struct al_fic *fic;
> + int ret;
> +
> + if (!base)
> + return ERR_PTR(-EINVAL);
How can this happen?
> +
> + fic = kzalloc(sizeof(*fic), GFP_KERNEL);
> + if (!fic)
> + return ERR_PTR(-ENOMEM);
> +
> + fic->base = base;
> + fic->parent_irq = parent_irq;
> + fic->name = (name ?: "al-fic-wire");
Under which circumstance can name be NULL?
> +
> + al_fic_hw_init(fic);
> +
> + ret = al_fic_register(node, fic);
> + if (ret) {
> + pr_err("fail to register irqchip\n");
> + goto err_free;
> + }
> +
> + pr_debug("%s initialized successfully in Legacy mode (parent-irq=%u)\n",
> + fic->name, parent_irq);
> +
> + return fic;
> +
> +err_free:
> + kfree(fic);
> + return ERR_PTR(ret);
> +}
> +
> +static int __init al_fic_init_dt(struct device_node *node,
> + struct device_node *parent)
> +{
> + int ret;
> + void __iomem *base;
> + unsigned int parent_irq;
> + struct al_fic *fic;
> +
> + if (!parent) {
> + pr_err("%s: unsupported - device require a parent\n",
> + node->name);
> + return -EINVAL;
> + }
> +
> + base = of_iomap(node, 0);
> + if (!base) {
> + pr_err("%s: fail to map memory\n", node->name);
> + return -ENOMEM;
> + }
> +
> + parent_irq = irq_of_parse_and_map(node, 0);
> + if (!parent_irq) {
> + pr_err("%s: fail to map irq\n", node->name);
> + ret = -EINVAL;
> + goto err_unmap;
> + }
> +
> + fic = al_fic_wire_init(node,
> + base,
> + node->name,
> + parent_irq);
> + if (IS_ERR(fic)) {
> + pr_err("%s: fail to initialize irqchip (%lu)\n",
> + node->name,
> + PTR_ERR(fic));
> + ret = PTR_ERR(fic);
> + goto err_irq_dispose;
> + }
> +
> + return 0;
> +
> +err_irq_dispose:
> + irq_dispose_mapping(parent_irq);
> +err_unmap:
> + iounmap(base);
> +
> + return ret;
> +}
> +
> +IRQCHIP_DECLARE(al_fic, "amazon,al-fic", al_fic_init_dt);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Wed, Jun 05, 2019 at 01:52:01PM +0300, Talel Shenhar wrote:
> --- /dev/null
> +++ b/drivers/irqchip/irq-al-fic.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
No need for kernel-doc format style here.
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
"or its affiliates"? You know the answer to this, don't keep us in
suspense. Put the proper copyright holder here please, otherwise this
is totally useless.
Well, copyright notices are technically useless anyway, but lawyers like
to cargo-cult with the best of them, so it should be correct at the
least.
thanks,
greg k-h
Thanks, will publish the fixes on v3.
On 6/5/2019 3:22 PM, Marc Zyngier wrote:
> Talel,
>
> On 05/06/2019 11:52, Talel Shenhar wrote:
>> The Amazon's Annapurna Labs Fabric Interrupt Controller has 32 inputs
>> lines. A FIC (Fabric Interrupt Controller) may be cascaded into another FIC
> Really? :-(
Cascading is used for control path events. For data path the HW is not
cascaded (and usually even configured in MSI-X instead of wire interrupts)
>
> +}
> +
> +static int al_fic_irq_set_type(struct irq_data *data, unsigned int flow_type)
> +{
> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
> + struct al_fic *fic = gc->private;
> + enum al_fic_state new_state;
> + int ret = 0;
> +
> + irq_gc_lock(gc);
> +
> + if (!(flow_type & IRQ_TYPE_LEVEL_HIGH) &&
> + !(flow_type & IRQ_TYPE_EDGE_RISING)) {
> And what if this gets passed EDGE_BOTH?
FIC only support two sensing modes, rising-edge and level.
>
>> + * This is generally fixed depending on what pieces of HW it's wired up
>> + * to.
>> + *
>> + * We configure it based on the sensitivity of the first source
>> + * being setup, and reject any subsequent attempt at configuring it in a
>> + * different way.
> Is that a reliable guess? It also strikes me that the DT binding doesn't
> allow for the trigger type to be passed, meaning the individual drivers
> have to request the trigger as part of their request_irq() call. I'd
> rather you have a complete interrupt specifier in DT, and document the
> various limitations of the HW.
Indeed we use interrupt specifier that has the level type in it
(dt-binding: "#interrupt-cells: must be 2.") which in turns causes to
this irq_set_type callback.
Part of the FICs are connected to hws that generate pulse (for those,
FIC shall be configured to rising-edge-triggered) and the others to hws
that keep the line up (for those the FIC shall be configured to
level-triggered).
>
>> + */
>> + if (fic->state == AL_FIC_CLEAN) {
>> + al_fic_set_trigger(fic, gc, new_state);
>> + } else if (fic->state != new_state) {
>> + pr_err("fic %s state already configured to %d\n",
>> + fic->name, fic->state);
>> + ret = -EPERM;
> Same as above.
Those error messages are control path messages. if we return the same
error value from here and from the previous error, how can we
differentiate between the two error cases by looking at the log?
Having informative printouts seems like a good idea for bad
configuration cases as such, wouldn't you agree?
On 6/5/2019 3:50 PM, Greg KH wrote:
> On Wed, Jun 05, 2019 at 01:52:01PM +0300, Talel Shenhar wrote:
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-al-fic.c
>> @@ -0,0 +1,289 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
> No need for kernel-doc format style here.
done
>
>> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> "or its affiliates"? You know the answer to this, don't keep us in
> suspense. Put the proper copyright holder here please, otherwise this
> is totally useless.
>
> Well, copyright notices are technically useless anyway, but lawyers like
> to cargo-cult with the best of them, so it should be correct at the
> least.
This is the format we were asked to use and have been using.
I am pinging them with your comment but I am likely not to get immediate
response so I'm publishing v3 without changing the "affiliates" for now.
>
> thanks,
>
> greg k-h
On Wed, 2019-06-05 at 14:50 +0200, Greg KH wrote:
> On Wed, Jun 05, 2019 at 01:52:01PM +0300, Talel Shenhar wrote:
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-al-fic.c
> > @@ -0,0 +1,289 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
>
> No need for kernel-doc format style here.
>
> > + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
>
> "or its affiliates"? You know the answer to this, don't keep us in
> suspense. Put the proper copyright holder here please, otherwise this
> is totally useless.
>
> Well, copyright notices are technically useless anyway, but lawyers like
> to cargo-cult with the best of them, so it should be correct at the
> least.
The "or its affiliates" form seems to be fairly common; there are
hundreds of them in the kernel already. I think it's something to do
with *not* having to manually update copyright notices to keep track of
the precise legal entities which make up a large corporation, as things
change.
Is there a particular problem with it, and an ongoing campaign to
eradicate it from the kernel? Any reason we should actually go and
spend time dealing with legal nonsense instead of just using the text
the lawyers have asked us to use?
On 05/06/2019 15:38, Shenhar, Talel wrote:
> Thanks, will publish the fixes on v3.
>
> On 6/5/2019 3:22 PM, Marc Zyngier wrote:
>> Talel,
>>
>> On 05/06/2019 11:52, Talel Shenhar wrote:
>>> The Amazon's Annapurna Labs Fabric Interrupt Controller has 32 inputs
>>> lines. A FIC (Fabric Interrupt Controller) may be cascaded into another FIC
>> Really? :-(
>
> Cascading is used for control path events. For data path the HW is not
> cascaded (and usually even configured in MSI-X instead of wire interrupts)
>
>
>>
>> +}
>> +
>> +static int al_fic_irq_set_type(struct irq_data *data, unsigned int flow_type)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
>> + struct al_fic *fic = gc->private;
>> + enum al_fic_state new_state;
>> + int ret = 0;
>> +
>> + irq_gc_lock(gc);
>> +
>> + if (!(flow_type & IRQ_TYPE_LEVEL_HIGH) &&
>> + !(flow_type & IRQ_TYPE_EDGE_RISING)) {
>> And what if this gets passed EDGE_BOTH?
>
> FIC only support two sensing modes, rising-edge and level.
Yes, I can tell. Yet, this code will let EDGE_BOTH pass through, even if
it cannot handle it.
>
>>
>>> + * This is generally fixed depending on what pieces of HW it's wired up
>>> + * to.
>>> + *
>>> + * We configure it based on the sensitivity of the first source
>>> + * being setup, and reject any subsequent attempt at configuring it in a
>>> + * different way.
>> Is that a reliable guess? It also strikes me that the DT binding doesn't
>> allow for the trigger type to be passed, meaning the individual drivers
>> have to request the trigger as part of their request_irq() call. I'd
>> rather you have a complete interrupt specifier in DT, and document the
>> various limitations of the HW.
>
> Indeed we use interrupt specifier that has the level type in it
> (dt-binding: "#interrupt-cells: must be 2.") which in turns causes to
> this irq_set_type callback.
Well, this isn't what the example in your DT binding shows.
>
> Part of the FICs are connected to hws that generate pulse (for those,
> FIC shall be configured to rising-edge-triggered) and the others to hws
> that keep the line up (for those the FIC shall be configured to
> level-triggered).
>
>>
>>> + */
>>> + if (fic->state == AL_FIC_CLEAN) {
>>> + al_fic_set_trigger(fic, gc, new_state);
>>> + } else if (fic->state != new_state) {
>>> + pr_err("fic %s state already configured to %d\n",
>>> + fic->name, fic->state);
>>> + ret = -EPERM;
>> Same as above.
>
> Those error messages are control path messages. if we return the same
> error value from here and from the previous error, how can we
> differentiate between the two error cases by looking at the log?
>
> Having informative printouts seems like a good idea for bad
> configuration cases as such, wouldn't you agree?
I completely disagree. The kernel log isn't a dumping ground for this
kind of pretty useless information. Furthermore, the irq subsystem will
also shout at you when it gets an error, so no need to add insult to injury.
If you really want to keep them around, turn them into pr_debug.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Wed, Jun 05, 2019 at 05:51:08PM +0300, Shenhar, Talel wrote:
>
> On 6/5/2019 3:50 PM, Greg KH wrote:
> > On Wed, Jun 05, 2019 at 01:52:01PM +0300, Talel Shenhar wrote:
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-al-fic.c
> > > @@ -0,0 +1,289 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/**
> > No need for kernel-doc format style here.
> done
> >
> > > + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> > "or its affiliates"? You know the answer to this, don't keep us in
> > suspense. Put the proper copyright holder here please, otherwise this
> > is totally useless.
> >
> > Well, copyright notices are technically useless anyway, but lawyers like
> > to cargo-cult with the best of them, so it should be correct at the
> > least.
>
> This is the format we were asked to use and have been using.
>
> I am pinging them with your comment but I am likely not to get immediate
> response so I'm publishing v3 without changing the "affiliates" for now.
If that is what your lawyers say to use, that's fine. Gotta love being
vague in copyright lines, what could go wrong...
good luck!
greg k-h
On Wed, Jun 05, 2019 at 01:52:00PM +0300, Talel Shenhar wrote:
> Document Amazon's Annapurna Labs Fabric Interrupt Controller SoC binding.
>
> Signed-off-by: Talel Shenhar <[email protected]>
> ---
> .../interrupt-controller/amazon,al-fic.txt | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
> new file mode 100644
> index 0000000..a2f31a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
> @@ -0,0 +1,22 @@
> +Amazon's Annapurna Labs Fabric Interrupt Controller
> +
> +Required properties:
> +
> +- compatible: should be "amazon,al-fic"
> +- reg: physical base address and size of the registers
> +- interrupt-controller: identifies the node as an interrupt controller
> +- #interrupt-cells: must be 2.
It would be great if you describe what the 2 numbers must represent here..
> +- interrupt-parent: specifies the parent interrupt controller.
> +- interrupts: describes which input line in the interrupt parent, this
> + fic's output is connected to.
> +
> +Example:
> +
> +amazon_fic: amazon_fic {
> + compatible = "amazon,al-fic";
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + reg = <0x0 0xfd8a8500 0x0 0x1000>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 0x0 IRQ_TYPE_LEVEL_HIGH>;
> +};
> --
> 2.7.4
>
--
All the best,
Eduardo Valentin
On Wed, 2019-06-05 at 13:22 +0100, Marc Zyngier wrote:
>
> > + * This is generally fixed depending on what pieces of HW it's wired up
> > + * to.
> > + *
> > + * We configure it based on the sensitivity of the first source
> > + * being setup, and reject any subsequent attempt at configuring it in a
> > + * different way.
>
> Is that a reliable guess? It also strikes me that the DT binding doesn't
> allow for the trigger type to be passed, meaning the individual drivers
> have to request the trigger as part of their request_irq() call. I'd
> rather you have a complete interrupt specifier in DT, and document the
> various limitations of the HW.
Actually the DT does, but Talel forgot to update the "example" part of
the binding patch. The description does say 2 cells.
This is the best approach imho (translation: I asked Talel to do it
this way :-) The other option which I don't like is to stick to
#interrupt-cells = 1, and have a separate property in the interrupt
controller node to indicate whether it needs to be configured as level
or edge.
These FICs are used for what is generally fixed wires inside the SoC,
so it doesn't matter much either way, but I prefer having it self
configured based on source just in case a future implementation doesn't
have the limitation of all inputs having the same trigger type.
Cheers,
Ben.
On Wed, 2019-06-05 at 16:12 +0100, Marc Zyngier wrote:
> > Those error messages are control path messages. if we return the same
> > error value from here and from the previous error, how can we
> > differentiate between the two error cases by looking at the log?
> >
> > Having informative printouts seems like a good idea for bad
> > configuration cases as such, wouldn't you agree?
>
> I completely disagree. The kernel log isn't a dumping ground for this
> kind of pretty useless information. Furthermore, the irq subsystem will
> also shout at you when it gets an error, so no need to add insult to injury.
>
> If you really want to keep them around, turn them into pr_debug.
I disagree Marc. This is a rather bad error which indicates that the device-tree
is probably incorrect (or the HW was wired in a way that cannot work).
Basically a given FIC can either be entirely level sensitive or entirely edge
sensitive. This catches cases where the DT has routed a mixed of both to the
same FIC. Definitely worth barfing loudly about rather than trying to understand
subtle odd misbehaviours of the device in the field.
Cheers,
Ben.
On Wed, 05 Jun 2019 23:06:05 +0100,
Benjamin Herrenschmidt <[email protected]> wrote:
>
> On Wed, 2019-06-05 at 16:12 +0100, Marc Zyngier wrote:
> > > Those error messages are control path messages. if we return the same
> > > error value from here and from the previous error, how can we
> > > differentiate between the two error cases by looking at the log?
> > >
> > > Having informative printouts seems like a good idea for bad
> > > configuration cases as such, wouldn't you agree?
> >
> > I completely disagree. The kernel log isn't a dumping ground for this
> > kind of pretty useless information. Furthermore, the irq subsystem will
> > also shout at you when it gets an error, so no need to add insult to injury.
> >
> > If you really want to keep them around, turn them into pr_debug.
>
> I disagree Marc. This is a rather bad error which indicates that the
> device-tree is probably incorrect (or the HW was wired in a way that
> cannot work).
But surely that's something you'll spot pretty quickly. Also, you get
a splat from the irq subsystem already, telling you that things went
wrong (see __irq_set_trigger). At that stage, you can enable debugging
and figure it out.
What I'm trying to avoid is the kernel becoming a (pretty bad)
validation tool for DTS files.
> Basically a given FIC can either be entirely level sensitive or
> entirely edge sensitive. This catches cases where the DT has routed
> a mixed of both to the same FIC. Definitely worth barfing loudly
> about rather than trying to understand subtle odd misbehaviours of
> the device in the field.
Then, in the interest of not producing incorrect DTs, could the
edge/level property be encoded in the FIC description itself, rather
than in the interrupt specifiers of the individual devices? It would
sidestep the problem altogether. You can still put the wrong one in
the FIC node, but it then becomes even more obvious what is going
on...
Thanks,
M.
--
Jazz is not dead, it just smells funny.
On Wed, 05 Jun 2019 23:02:13 +0100,
Benjamin Herrenschmidt <[email protected]> wrote:
>
> On Wed, 2019-06-05 at 13:22 +0100, Marc Zyngier wrote:
> >
> > > + * This is generally fixed depending on what pieces of HW it's wired up
> > > + * to.
> > > + *
> > > + * We configure it based on the sensitivity of the first source
> > > + * being setup, and reject any subsequent attempt at configuring it in a
> > > + * different way.
> >
> > Is that a reliable guess? It also strikes me that the DT binding doesn't
> > allow for the trigger type to be passed, meaning the individual drivers
> > have to request the trigger as part of their request_irq() call. I'd
> > rather you have a complete interrupt specifier in DT, and document the
> > various limitations of the HW.
>
> Actually the DT does, but Talel forgot to update the "example" part of
> the binding patch. The description does say 2 cells.
Yeah, I missed that and only read the example.
> This is the best approach imho (translation: I asked Talel to do it
> this way :-) The other option which I don't like is to stick to
> #interrupt-cells = 1, and have a separate property in the interrupt
> controller node to indicate whether it needs to be configured as level
> or edge.
Right, that's what I was suggesting in a separate email.
> These FICs are used for what is generally fixed wires inside the SoC,
> so it doesn't matter much either way, but I prefer having it self
> configured based on source just in case a future implementation doesn't
> have the limitation of all inputs having the same trigger type.
Fair enough. But it should be pretty easy to verify statically that
all interrupt specifiers targeting this interrupt controller have a
similar type.
RobH, is that something the yaml thing could do for us?
Thanks,
M.
--
Jazz is not dead, it just smells funny.
On 6/5/2019 6:49 PM, Eduardo Valentin wrote:
> On Wed, Jun 05, 2019 at 01:52:00PM +0300, Talel Shenhar wrote:
>> +- compatible: should be "amazon,al-fic"
>> +- reg: physical base address and size of the registers
>> +- interrupt-controller: identifies the node as an interrupt controller
>> +- #interrupt-cells: must be 2.
> It would be great if you describe what the 2 numbers must represent here..
will be included in v4
Thanks,
Talel.
On 6/5/2019 6:12 PM, Marc Zyngier wrote:
> On 05/06/2019 15:38, Shenhar, Talel wrote:
>>
>> FIC only support two sensing modes, rising-edge and level.
> Yes, I can tell. Yet, this code will let EDGE_BOTH pass through, even if
> it cannot handle it.
Will handle on v4
> Indeed we use interrupt specifier that has the level type in it
>> (dt-binding: "#interrupt-cells: must be 2.") which in turns causes to
>> this irq_set_type callback.
> Well, this isn't what the example in your DT binding shows.
Will update the example in v4
Thanks,
Talel.
On Thu, 2019-06-06 at 08:05 +0100, Marc Zyngier wrote:
>
> > I disagree Marc. This is a rather bad error which indicates that the
> > device-tree is probably incorrect (or the HW was wired in a way that
> > cannot work).
>
> But surely that's something you'll spot pretty quickly.
Not really. A level/edge mismatch isn't something you can spot that
quickly, but will cause lost interrupts on load. Since the kernel can
spot the error pretty much right away, I think that could even be a
pr_err :)
> Also, you get
> a splat from the irq subsystem already, telling you that things went
> wrong (see __irq_set_trigger). At that stage, you can enable debugging
> and figure it out.
Ah returning an error will cause such splat indeed.
> What I'm trying to avoid is the kernel becoming a (pretty bad)
> validation tool for DTS files.
Haha, yeah, I don't like it going out of its way to validate them but
that sort of very obvious sanity checking makes sense.
> > Basically a given FIC can either be entirely level sensitive or
> > entirely edge sensitive. This catches cases where the DT has routed
> > a mixed of both to the same FIC. Definitely worth barfing loudly
> > about rather than trying to understand subtle odd misbehaviours of
> > the device in the field.
>
> Then, in the interest of not producing incorrect DTs, could the
> edge/level property be encoded in the FIC description itself, rather
> than in the interrupt specifiers of the individual devices? It would
> sidestep the problem altogether. You can still put the wrong one in
> the FIC node, but it then becomes even more obvious what is going
> on...
This was Talel original approach internally in fact. I told him to put
it in the specifier instead :-) The advantage in doing it that way is
that you get the right flags in the descriptor by default iirc, so the
right value in /proc/interrupts etc... And it will continue working if
a future FIC loses that limitation.
That said, if you feel strongly about it, we can revert to putting a
global property in the FIC node itself. Let us know what you want.
Cheers,
Ben.