This controller appears on Actions Semi Owl family SoC's S500, S700 and
S900 and provides support for 3 external interrupt controllers through
dedicated SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low). Each line can also be masked independently.
This is based on the patch series submitted by Parthiban Nallathambi:
https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Parthiban Nallathambi <[email protected]>
Signed-off-by: Saravanan Sekar <[email protected]>
[cristi: optimized DT, various fixes/cleanups/improvements]
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 318 +++++++++++++++++++++++++++++++++
2 files changed, 319 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 133f9c45744a..b8eb5b8b766d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o
obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o
obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index 000000000000..29b7ffc40ac7
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu <[email protected]>
+ *
+ * Author: Parthiban Nallathambi <[email protected]>
+ * Author: Saravanan Sekar <[email protected]>
+ * Author: Cristian Ciocaltea <[email protected]>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define NUM_SIRQ 3
+
+#define INTC_EXTCTL_PENDING BIT(0)
+#define INTC_EXTCTL_CLK_SEL BIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#define INTC_EXTCTL_TYPE_MASK GENMASK(7, 6)
+#define INTC_EXTCTL_TYPE_HIGH 0
+#define INTC_EXTCTL_TYPE_LOW BIT(6)
+#define INTC_EXTCTL_TYPE_RISING BIT(7)
+#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
+
+/* S900 SIRQ1 & SIRQ2 control register offsets, relative to SIRQ0 */
+#define INTC_EXTCTL1 0x0328
+#define INTC_EXTCTL2 0x032c
+
+struct owl_sirq_params {
+ /* INTC_EXTCTL reg shared for all three SIRQ lines */
+ bool reg_shared;
+ /* INTC_EXTCTL reg offsets relative to controller base address */
+ u16 reg_offset[NUM_SIRQ];
+};
+
+struct owl_sirq_chip_data {
+ const struct owl_sirq_params *params;
+ void __iomem *base;
+ raw_spinlock_t lock;
+ u32 ext_irqs[NUM_SIRQ];
+ u8 trigger;
+};
+
+/* S500 and S700 SoCs */
+static const struct owl_sirq_params owl_sirq_s500_params = {
+ .reg_shared = true,
+ .reg_offset = { 0, 0, 0 },
+};
+
+/* S900 SoC */
+static const struct owl_sirq_params owl_sirq_s900_params = {
+ .reg_shared = false,
+ .reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
+};
+
+static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32 index)
+{
+ u32 val;
+
+ val = readl_relaxed(data->base + data->params->reg_offset[index]);
+ if (data->params->reg_shared)
+ val = (val >> (2 - index) * 8) & 0xff;
+
+ return val;
+}
+
+static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
+ u32 extctl, u32 index)
+{
+ u32 val;
+
+ if (data->params->reg_shared) {
+ val = readl_relaxed(data->base + data->params->reg_offset[index]);
+ val &= ~(0xff << (2 - index) * 8);
+ extctl &= 0xff;
+ extctl = (extctl << (2 - index) * 8) | val;
+ }
+
+ writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
+}
+
+static void owl_sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
+ u32 clear, u32 set, u32 index)
+{
+ unsigned long flags;
+ u32 val;
+
+ raw_spin_lock_irqsave(&d->lock, flags);
+ val = owl_sirq_read_extctl(d, index);
+ val &= ~clear;
+ val |= set;
+ owl_sirq_write_extctl(d, val, index);
+ raw_spin_unlock_irqrestore(&d->lock, flags);
+}
+
+static void owl_sirq_eoi(struct irq_data *data)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+
+ /*
+ * Software must clear external interrupt pending, when interrupt type
+ * is edge triggered, so we need per SIRQ based clearing.
+ */
+ if (chip_data->trigger & (1 << data->hwirq))
+ owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_PENDING,
+ data->hwirq);
+
+ irq_chip_eoi_parent(data);
+}
+
+static void owl_sirq_mask(struct irq_data *data)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+
+ owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_EN, 0, data->hwirq);
+ irq_chip_mask_parent(data);
+}
+
+static void owl_sirq_unmask(struct irq_data *data)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+
+ owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_EN, data->hwirq);
+ irq_chip_unmask_parent(data);
+}
+
+/*
+ * GIC does not handle falling edge or active low, hence SIRQ shall be
+ * programmed to convert falling edge to rising edge signal and active
+ * low to active high signal.
+ */
+static int owl_sirq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct owl_sirq_chip_data *chip_data = irq_data_get_irq_chip_data(data);
+ u32 sirq_type;
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ sirq_type = INTC_EXTCTL_TYPE_LOW;
+ chip_data->trigger &= ~(1 << data->hwirq);
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ sirq_type = INTC_EXTCTL_TYPE_HIGH;
+ chip_data->trigger &= ~(1 << data->hwirq);
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ sirq_type = INTC_EXTCTL_TYPE_FALLING;
+ chip_data->trigger |= 1 << data->hwirq;
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_EDGE_RISING:
+ sirq_type = INTC_EXTCTL_TYPE_RISING;
+ chip_data->trigger |= 1 << data->hwirq;
+ break;
+ default:
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
+ owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_TYPE_MASK, sirq_type,
+ data->hwirq);
+
+ return irq_chip_set_type_parent(data, type);
+}
+
+static struct irq_chip owl_sirq_chip = {
+ .name = "owl-sirq",
+ .irq_mask = owl_sirq_mask,
+ .irq_unmask = owl_sirq_unmask,
+ .irq_eoi = owl_sirq_eoi,
+ .irq_set_type = owl_sirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+};
+
+static int owl_sirq_domain_translate(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ unsigned long *hwirq,
+ unsigned int *type)
+{
+ if (!is_of_node(fwspec->fwnode))
+ return -EINVAL;
+
+ if (fwspec->param_count != 2 || fwspec->param[0] >= NUM_SIRQ)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[0];
+ *type = fwspec->param[1];
+
+ return 0;
+}
+
+static int owl_sirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct owl_sirq_chip_data *chip_data = domain->host_data;
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret;
+
+ if (WARN_ON(nr_irqs != 1))
+ return -EINVAL;
+
+ ret = owl_sirq_domain_translate(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_LEVEL_HIGH:
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &owl_sirq_chip,
+ chip_data);
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0] = 0; /* SPI */
+ parent_fwspec.param[1] = chip_data->ext_irqs[hwirq];
+ parent_fwspec.param[2] = type;
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+}
+
+static const struct irq_domain_ops owl_sirq_domain_ops = {
+ .translate = owl_sirq_domain_translate,
+ .alloc = owl_sirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static const struct of_device_id owl_sirq_of_match[] = {
+ { .compatible = "actions,s500-sirq", .data = &owl_sirq_s500_params },
+ { .compatible = "actions,s700-sirq", .data = &owl_sirq_s500_params },
+ { .compatible = "actions,s900-sirq", .data = &owl_sirq_s900_params },
+ { /* sentinel */ }
+};
+
+static int __init owl_sirq_of_init(struct device_node *node,
+ struct device_node *parent)
+{
+ const struct of_device_id *match;
+ struct irq_domain *domain, *parent_domain;
+ struct owl_sirq_chip_data *chip_data;
+ int ret, i;
+
+ parent_domain = irq_find_host(parent);
+ if (!parent_domain) {
+ pr_err("%pOF: failed to find sirq parent domain\n", node);
+ return -ENXIO;
+ }
+
+ chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+ if (!chip_data)
+ return -ENOMEM;
+
+ match = of_match_node(owl_sirq_of_match, node);
+ if (!match) {
+ pr_warn("owl-sirq: assuming S500/S700 compatible controller\n");
+ chip_data->params = &owl_sirq_s500_params;
+ } else {
+ chip_data->params = match->data;
+ }
+
+ raw_spin_lock_init(&chip_data->lock);
+
+ chip_data->base = of_iomap(node, 0);
+ if (!chip_data->base) {
+ pr_err("%pOF: failed to map sirq registers\n", node);
+ ret = -ENXIO;
+ goto out_free;
+ }
+
+ ret = of_property_read_variable_u32_array(node, "actions,ext-interrupts",
+ chip_data->ext_irqs,
+ NUM_SIRQ, NUM_SIRQ);
+ if (ret < NUM_SIRQ) {
+ pr_err("%pOF: failed to read sirq interrupts\n", node);
+ goto out_unmap;
+ }
+
+ /* Set 24MHz external interrupt clock freq */
+ for (i = 0; i < NUM_SIRQ; i++)
+ owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_CLK_SEL, i);
+
+ domain = irq_domain_add_hierarchy(parent_domain, 0, NUM_SIRQ, node,
+ &owl_sirq_domain_ops, chip_data);
+ if (!domain) {
+ pr_err("%pOF: failed to add domain\n", node);
+ ret = -ENOMEM;
+ goto out_unmap;
+ }
+
+ return 0;
+
+out_unmap:
+ iounmap(chip_data->base);
+out_free:
+ kfree(chip_data);
+
+ return ret;
+}
+
+IRQCHIP_DECLARE(owl_sirq, "actions,owl-sirq", owl_sirq_of_init);
--
2.28.0
On 2020-08-16 12:33, Cristian Ciocaltea wrote:
> This controller appears on Actions Semi Owl family SoC's S500, S700 and
> S900 and provides support for 3 external interrupt controllers through
Is that really 3 interrupt controllers? Or merely 3 interrupt lines?
> dedicated SIRQ pins.
>
> Each line can be independently configured as interrupt and triggers
> on either of the edges (raising or falling) or either of the levels
> (high or low). Each line can also be masked independently.
>
> This is based on the patch series submitted by Parthiban Nallathambi:
> https://lore.kernel.org/lkml/[email protected]/
>
> Signed-off-by: Parthiban Nallathambi <[email protected]>
> Signed-off-by: Saravanan Sekar <[email protected]>
> [cristi: optimized DT, various fixes/cleanups/improvements]
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 318 +++++++++++++++++++++++++++++++++
> 2 files changed, 319 insertions(+)
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 133f9c45744a..b8eb5b8b766d 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
> obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o
> obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o
> obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o
> diff --git a/drivers/irqchip/irq-owl-sirq.c
> b/drivers/irqchip/irq-owl-sirq.c
> new file mode 100644
> index 000000000000..29b7ffc40ac7
> --- /dev/null
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> + *
> + * Copyright (C) 2014 Actions Semi Inc.
> + * David Liu <[email protected]>
> + *
> + * Author: Parthiban Nallathambi <[email protected]>
> + * Author: Saravanan Sekar <[email protected]>
> + * Author: Cristian Ciocaltea <[email protected]>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#define NUM_SIRQ 3
> +
> +#define INTC_EXTCTL_PENDING BIT(0)
> +#define INTC_EXTCTL_CLK_SEL BIT(4)
> +#define INTC_EXTCTL_EN BIT(5)
> +#define INTC_EXTCTL_TYPE_MASK GENMASK(7, 6)
> +#define INTC_EXTCTL_TYPE_HIGH 0
> +#define INTC_EXTCTL_TYPE_LOW BIT(6)
> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> +#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
> +
> +/* S900 SIRQ1 & SIRQ2 control register offsets, relative to SIRQ0 */
> +#define INTC_EXTCTL1 0x0328
> +#define INTC_EXTCTL2 0x032c
> +
> +struct owl_sirq_params {
> + /* INTC_EXTCTL reg shared for all three SIRQ lines */
> + bool reg_shared;
> + /* INTC_EXTCTL reg offsets relative to controller base address */
> + u16 reg_offset[NUM_SIRQ];
> +};
> +
> +struct owl_sirq_chip_data {
> + const struct owl_sirq_params *params;
> + void __iomem *base;
> + raw_spinlock_t lock;
> + u32 ext_irqs[NUM_SIRQ];
> + u8 trigger;
Nit: Please align data structure members vertically:
struct owl_sirq_chip_data {
const struct owl_sirq_params *params;
void __iomem *base;
raw_spinlock_t lock;
u32 ext_irqs[NUM_SIRQ];
u8 trigger;
};
> +};
> +
> +/* S500 and S700 SoCs */
> +static const struct owl_sirq_params owl_sirq_s500_params = {
> + .reg_shared = true,
> + .reg_offset = { 0, 0, 0 },
> +};
> +
> +/* S900 SoC */
> +static const struct owl_sirq_params owl_sirq_s900_params = {
> + .reg_shared = false,
> + .reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
0 *is* an offset, right? Why doesn't it have a name too?
> +};
> +
> +static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32
> index)
> +{
> + u32 val;
> +
> + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> + if (data->params->reg_shared)
> + val = (val >> (2 - index) * 8) & 0xff;
base = (2 - index) * 8;
val = FIELD_GET(GENMASK(base + 7, base), val);
> +
> + return val;
> +}
> +
> +static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
> + u32 extctl, u32 index)
> +{
> + u32 val;
> +
> + if (data->params->reg_shared) {
> + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> + val &= ~(0xff << (2 - index) * 8);
> + extctl &= 0xff;
> + extctl = (extctl << (2 - index) * 8) | val;
Please make use of FIELD_PREP, FIELD_GET and GENMASK.
> + }
> +
> + writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
> +}
> +
> +static void owl_sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
> + u32 clear, u32 set, u32 index)
> +{
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&d->lock, flags);
> + val = owl_sirq_read_extctl(d, index);
> + val &= ~clear;
> + val |= set;
> + owl_sirq_write_extctl(d, val, index);
> + raw_spin_unlock_irqrestore(&d->lock, flags);
> +}
> +
> +static void owl_sirq_eoi(struct irq_data *data)
> +{
> + struct owl_sirq_chip_data *chip_data =
> irq_data_get_irq_chip_data(data);
> +
> + /*
> + * Software must clear external interrupt pending, when interrupt
> type
> + * is edge triggered, so we need per SIRQ based clearing.
> + */
> + if (chip_data->trigger & (1 << data->hwirq))
BIT(d->hwirq)
But it also begs the question: we already have all the trigger
information
in the irqdesc. Why do you need some additional bookkeeping?
> + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_PENDING,
> + data->hwirq);
> +
> + irq_chip_eoi_parent(data);
> +}
> +
> +static void owl_sirq_mask(struct irq_data *data)
> +{
> + struct owl_sirq_chip_data *chip_data =
> irq_data_get_irq_chip_data(data);
> +
> + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_EN, 0, data->hwirq);
> + irq_chip_mask_parent(data);
> +}
> +
> +static void owl_sirq_unmask(struct irq_data *data)
> +{
> + struct owl_sirq_chip_data *chip_data =
> irq_data_get_irq_chip_data(data);
> +
> + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_EN, data->hwirq);
> + irq_chip_unmask_parent(data);
> +}
> +
> +/*
> + * GIC does not handle falling edge or active low, hence SIRQ shall be
> + * programmed to convert falling edge to rising edge signal and active
> + * low to active high signal.
> + */
> +static int owl_sirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + struct owl_sirq_chip_data *chip_data =
> irq_data_get_irq_chip_data(data);
> + u32 sirq_type;
> +
> + switch (type) {
> + case IRQ_TYPE_LEVEL_LOW:
> + sirq_type = INTC_EXTCTL_TYPE_LOW;
> + chip_data->trigger &= ~(1 << data->hwirq);
> + type = IRQ_TYPE_LEVEL_HIGH;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + sirq_type = INTC_EXTCTL_TYPE_HIGH;
> + chip_data->trigger &= ~(1 << data->hwirq);
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + sirq_type = INTC_EXTCTL_TYPE_FALLING;
> + chip_data->trigger |= 1 << data->hwirq;
> + type = IRQ_TYPE_EDGE_RISING;
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + sirq_type = INTC_EXTCTL_TYPE_RISING;
> + chip_data->trigger |= 1 << data->hwirq;
> + break;
> + default:
> + WARN_ON(1);
No need for this WARN_ON(), the core kernel is loud enough.
> + return -EINVAL;
> + }
> +
> + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_TYPE_MASK,
> sirq_type,
> + data->hwirq);
> +
> + return irq_chip_set_type_parent(data, type);
> +}
> +
> +static struct irq_chip owl_sirq_chip = {
> + .name = "owl-sirq",
> + .irq_mask = owl_sirq_mask,
> + .irq_unmask = owl_sirq_unmask,
> + .irq_eoi = owl_sirq_eoi,
> + .irq_set_type = owl_sirq_set_type,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
How about irq_set_affinity? Or does it only exist on UP systems?
> +};
> +
> +static int owl_sirq_domain_translate(struct irq_domain *d,
> + struct irq_fwspec *fwspec,
> + unsigned long *hwirq,
> + unsigned int *type)
> +{
> + if (!is_of_node(fwspec->fwnode))
> + return -EINVAL;
> +
> + if (fwspec->param_count != 2 || fwspec->param[0] >= NUM_SIRQ)
> + return -EINVAL;
> +
> + *hwirq = fwspec->param[0];
> + *type = fwspec->param[1];
> +
> + return 0;
> +}
> +
> +static int owl_sirq_domain_alloc(struct irq_domain *domain, unsigned
> int virq,
> + unsigned int nr_irqs, void *data)
> +{
> + struct owl_sirq_chip_data *chip_data = domain->host_data;
> + struct irq_fwspec *fwspec = data;
> + struct irq_fwspec parent_fwspec;
> + irq_hw_number_t hwirq;
> + unsigned int type;
> + int ret;
> +
> + if (WARN_ON(nr_irqs != 1))
> + return -EINVAL;
> +
> + ret = owl_sirq_domain_translate(domain, fwspec, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + switch (type) {
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_LEVEL_HIGH:
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + type = IRQ_TYPE_EDGE_RISING;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + type = IRQ_TYPE_LEVEL_HIGH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &owl_sirq_chip,
> + chip_data);
> +
> + parent_fwspec.fwnode = domain->parent->fwnode;
> + parent_fwspec.param_count = 3;
> + parent_fwspec.param[0] = 0; /* SPI */
> + parent_fwspec.param[1] = chip_data->ext_irqs[hwirq];
> + parent_fwspec.param[2] = type;
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops owl_sirq_domain_ops = {
> + .translate = owl_sirq_domain_translate,
> + .alloc = owl_sirq_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> +};
> +
> +static const struct of_device_id owl_sirq_of_match[] = {
> + { .compatible = "actions,s500-sirq", .data = &owl_sirq_s500_params },
> + { .compatible = "actions,s700-sirq", .data = &owl_sirq_s500_params },
> + { .compatible = "actions,s900-sirq", .data = &owl_sirq_s900_params },
> + { /* sentinel */ }
> +};
> +
> +static int __init owl_sirq_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + const struct of_device_id *match;
> + struct irq_domain *domain, *parent_domain;
> + struct owl_sirq_chip_data *chip_data;
> + int ret, i;
> +
> + parent_domain = irq_find_host(parent);
> + if (!parent_domain) {
> + pr_err("%pOF: failed to find sirq parent domain\n", node);
> + return -ENXIO;
> + }
> +
> + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> + if (!chip_data)
> + return -ENOMEM;
> +
> + match = of_match_node(owl_sirq_of_match, node);
> + if (!match) {
> + pr_warn("owl-sirq: assuming S500/S700 compatible controller\n");
> + chip_data->params = &owl_sirq_s500_params;
> + } else {
> + chip_data->params = match->data;
> + }
> +
> + raw_spin_lock_init(&chip_data->lock);
> +
> + chip_data->base = of_iomap(node, 0);
> + if (!chip_data->base) {
> + pr_err("%pOF: failed to map sirq registers\n", node);
> + ret = -ENXIO;
> + goto out_free;
> + }
> +
> + ret = of_property_read_variable_u32_array(node,
> "actions,ext-interrupts",
> + chip_data->ext_irqs,
> + NUM_SIRQ, NUM_SIRQ);
> + if (ret < NUM_SIRQ) {
> + pr_err("%pOF: failed to read sirq interrupts\n", node);
> + goto out_unmap;
> + }
> +
> + /* Set 24MHz external interrupt clock freq */
> + for (i = 0; i < NUM_SIRQ; i++)
> + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_CLK_SEL, i);
> +
> + domain = irq_domain_add_hierarchy(parent_domain, 0, NUM_SIRQ, node,
> + &owl_sirq_domain_ops, chip_data);
> + if (!domain) {
> + pr_err("%pOF: failed to add domain\n", node);
> + ret = -ENOMEM;
> + goto out_unmap;
> + }
> +
> + return 0;
> +
> +out_unmap:
> + iounmap(chip_data->base);
> +out_free:
> + kfree(chip_data);
> +
> + return ret;
> +}
> +
> +IRQCHIP_DECLARE(owl_sirq, "actions,owl-sirq", owl_sirq_of_init);
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Hi Marc,
Thanks for your quick and detailed review!
On Mon, Aug 17, 2020 at 02:52:06PM +0100, Marc Zyngier wrote:
> On 2020-08-16 12:33, Cristian Ciocaltea wrote:
> > This controller appears on Actions Semi Owl family SoC's S500, S700 and
> > S900 and provides support for 3 external interrupt controllers through
>
> Is that really 3 interrupt controllers? Or merely 3 interrupt lines?
This is mostly a leftover statement from the previous patch revision.
I will change it to something like:
"This interrupt controller is found in the Actions Semi Owl SoCs (S500,
S700 and S900) and provides support for handling up to 3 external
interrupt lines."
> > dedicated SIRQ pins.
> >
> > Each line can be independently configured as interrupt and triggers
> > on either of the edges (raising or falling) or either of the levels
> > (high or low). Each line can also be masked independently.
> >
> > This is based on the patch series submitted by Parthiban Nallathambi:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > Signed-off-by: Parthiban Nallathambi <[email protected]>
> > Signed-off-by: Saravanan Sekar <[email protected]>
> > [cristi: optimized DT, various fixes/cleanups/improvements]
> > Signed-off-by: Cristian Ciocaltea <[email protected]>
> > ---
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-owl-sirq.c | 318 +++++++++++++++++++++++++++++++++
> > 2 files changed, 319 insertions(+)
> > create mode 100644 drivers/irqchip/irq-owl-sirq.c
> >
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 133f9c45744a..b8eb5b8b766d 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
> > obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> > +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> > obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o
> > obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o
> > obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o
> > diff --git a/drivers/irqchip/irq-owl-sirq.c
> > b/drivers/irqchip/irq-owl-sirq.c
> > new file mode 100644
> > index 000000000000..29b7ffc40ac7
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-owl-sirq.c
> > @@ -0,0 +1,318 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> > + *
> > + * Copyright (C) 2014 Actions Semi Inc.
> > + * David Liu <[email protected]>
> > + *
> > + * Author: Parthiban Nallathambi <[email protected]>
> > + * Author: Saravanan Sekar <[email protected]>
> > + * Author: Cristian Ciocaltea <[email protected]>
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +
> > +#define NUM_SIRQ 3
> > +
> > +#define INTC_EXTCTL_PENDING BIT(0)
> > +#define INTC_EXTCTL_CLK_SEL BIT(4)
> > +#define INTC_EXTCTL_EN BIT(5)
> > +#define INTC_EXTCTL_TYPE_MASK GENMASK(7, 6)
> > +#define INTC_EXTCTL_TYPE_HIGH 0
> > +#define INTC_EXTCTL_TYPE_LOW BIT(6)
> > +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> > +#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
> > +
> > +/* S900 SIRQ1 & SIRQ2 control register offsets, relative to SIRQ0 */
> > +#define INTC_EXTCTL1 0x0328
> > +#define INTC_EXTCTL2 0x032c
> > +
> > +struct owl_sirq_params {
> > + /* INTC_EXTCTL reg shared for all three SIRQ lines */
> > + bool reg_shared;
> > + /* INTC_EXTCTL reg offsets relative to controller base address */
> > + u16 reg_offset[NUM_SIRQ];
> > +};
> > +
> > +struct owl_sirq_chip_data {
> > + const struct owl_sirq_params *params;
> > + void __iomem *base;
> > + raw_spinlock_t lock;
> > + u32 ext_irqs[NUM_SIRQ];
> > + u8 trigger;
>
> Nit: Please align data structure members vertically:
>
> struct owl_sirq_chip_data {
> const struct owl_sirq_params *params;
> void __iomem *base;
> raw_spinlock_t lock;
> u32 ext_irqs[NUM_SIRQ];
> u8 trigger;
> };
Done.
> > +};
> > +
> > +/* S500 and S700 SoCs */
> > +static const struct owl_sirq_params owl_sirq_s500_params = {
> > + .reg_shared = true,
> > + .reg_offset = { 0, 0, 0 },
> > +};
> > +
> > +/* S900 SoC */
> > +static const struct owl_sirq_params owl_sirq_s900_params = {
> > + .reg_shared = false,
> > + .reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
>
> 0 *is* an offset, right? Why doesn't it have a name too?
Right, I updated the defines section:
/* S900 SIRQ control register offsets, relative to controller base address */
#define INTC_EXTCTL0 0x0000
#define INTC_EXTCTL1 0x0328
[...]
The controller base address points to SIRQ0 control register, so this
offset is always 0, but I totally agree we should have a name for it.
> > +};
> > +
> > +static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32
> > index)
> > +{
> > + u32 val;
> > +
> > + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> > + if (data->params->reg_shared)
> > + val = (val >> (2 - index) * 8) & 0xff;
>
> base = (2 - index) * 8;
> val = FIELD_GET(GENMASK(base + 7, base), val);
Unfortunately the context doesn't allow using FIELD_GET:
./include/linux/compiler.h:392:38: error: call to ‘__compiletime_assert_159’
declared with attribute error: FIELD_GET: mask is not constant
> > +
> > + return val;
> > +}
> > +
> > +static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
> > + u32 extctl, u32 index)
> > +{
> > + u32 val;
> > +
> > + if (data->params->reg_shared) {
> > + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> > + val &= ~(0xff << (2 - index) * 8);
> > + extctl &= 0xff;
> > + extctl = (extctl << (2 - index) * 8) | val;
>
> Please make use of FIELD_PREP, FIELD_GET and GENMASK.
I'm not sure how to deal with the mask constness restriction, except
using static defines and if/else statements, which is not quite elegant.
> > + }
> > +
> > + writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
> > +}
> > +
> > +static void owl_sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
> > + u32 clear, u32 set, u32 index)
> > +{
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&d->lock, flags);
> > + val = owl_sirq_read_extctl(d, index);
> > + val &= ~clear;
> > + val |= set;
> > + owl_sirq_write_extctl(d, val, index);
> > + raw_spin_unlock_irqrestore(&d->lock, flags);
> > +}
> > +
> > +static void owl_sirq_eoi(struct irq_data *data)
> > +{
> > + struct owl_sirq_chip_data *chip_data =
> > irq_data_get_irq_chip_data(data);
> > +
> > + /*
> > + * Software must clear external interrupt pending, when interrupt type
> > + * is edge triggered, so we need per SIRQ based clearing.
> > + */
> > + if (chip_data->trigger & (1 << data->hwirq))
>
> BIT(d->hwirq)
>
> But it also begs the question: we already have all the trigger information
> in the irqdesc. Why do you need some additional bookkeeping?
That's another leftover from the original work. Thanks for pointing
this out, I missed it. I dropped the redundant handling of the IRQ
trigger information and just replaced this with:
if (!irqd_is_level_type(data))
> > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_PENDING,
> > + data->hwirq);
> > +
> > + irq_chip_eoi_parent(data);
> > +}
> > +
> > +static void owl_sirq_mask(struct irq_data *data)
> > +{
> > + struct owl_sirq_chip_data *chip_data =
> > irq_data_get_irq_chip_data(data);
> > +
> > + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_EN, 0, data->hwirq);
> > + irq_chip_mask_parent(data);
> > +}
> > +
> > +static void owl_sirq_unmask(struct irq_data *data)
> > +{
> > + struct owl_sirq_chip_data *chip_data =
> > irq_data_get_irq_chip_data(data);
> > +
> > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_EN, data->hwirq);
> > + irq_chip_unmask_parent(data);
> > +}
> > +
> > +/*
> > + * GIC does not handle falling edge or active low, hence SIRQ shall be
> > + * programmed to convert falling edge to rising edge signal and active
> > + * low to active high signal.
> > + */
> > +static int owl_sirq_set_type(struct irq_data *data, unsigned int type)
> > +{
> > + struct owl_sirq_chip_data *chip_data =
> > irq_data_get_irq_chip_data(data);
> > + u32 sirq_type;
> > +
> > + switch (type) {
> > + case IRQ_TYPE_LEVEL_LOW:
> > + sirq_type = INTC_EXTCTL_TYPE_LOW;
> > + chip_data->trigger &= ~(1 << data->hwirq);
> > + type = IRQ_TYPE_LEVEL_HIGH;
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + sirq_type = INTC_EXTCTL_TYPE_HIGH;
> > + chip_data->trigger &= ~(1 << data->hwirq);
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + sirq_type = INTC_EXTCTL_TYPE_FALLING;
> > + chip_data->trigger |= 1 << data->hwirq;
> > + type = IRQ_TYPE_EDGE_RISING;
> > + break;
> > + case IRQ_TYPE_EDGE_RISING:
> > + sirq_type = INTC_EXTCTL_TYPE_RISING;
> > + chip_data->trigger |= 1 << data->hwirq;
> > + break;
> > + default:
> > + WARN_ON(1);
>
> No need for this WARN_ON(), the core kernel is loud enough.
Done.
> > + return -EINVAL;
> > + }
> > +
> > + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_TYPE_MASK, sirq_type,
> > + data->hwirq);
> > +
> > + return irq_chip_set_type_parent(data, type);
> > +}
> > +
> > +static struct irq_chip owl_sirq_chip = {
> > + .name = "owl-sirq",
> > + .irq_mask = owl_sirq_mask,
> > + .irq_unmask = owl_sirq_unmask,
> > + .irq_eoi = owl_sirq_eoi,
> > + .irq_set_type = owl_sirq_set_type,
> > + .irq_retrigger = irq_chip_retrigger_hierarchy,
>
> How about irq_set_affinity? Or does it only exist on UP systems?
I have just added:
#ifdef CONFIG_SMP
.irq_set_affinity = irq_chip_set_affinity_parent,
#endif
> > +};
> > +
> > +static int owl_sirq_domain_translate(struct irq_domain *d,
> > + struct irq_fwspec *fwspec,
> > + unsigned long *hwirq,
> > + unsigned int *type)
> > +{
> > + if (!is_of_node(fwspec->fwnode))
> > + return -EINVAL;
> > +
> > + if (fwspec->param_count != 2 || fwspec->param[0] >= NUM_SIRQ)
> > + return -EINVAL;
> > +
> > + *hwirq = fwspec->param[0];
> > + *type = fwspec->param[1];
> > +
> > + return 0;
> > +}
> > +
> > +static int owl_sirq_domain_alloc(struct irq_domain *domain, unsigned
> > int virq,
> > + unsigned int nr_irqs, void *data)
> > +{
> > + struct owl_sirq_chip_data *chip_data = domain->host_data;
> > + struct irq_fwspec *fwspec = data;
> > + struct irq_fwspec parent_fwspec;
> > + irq_hw_number_t hwirq;
> > + unsigned int type;
> > + int ret;
> > +
> > + if (WARN_ON(nr_irqs != 1))
> > + return -EINVAL;
> > +
> > + ret = owl_sirq_domain_translate(domain, fwspec, &hwirq, &type);
> > + if (ret)
> > + return ret;
> > +
> > + switch (type) {
> > + case IRQ_TYPE_EDGE_RISING:
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + type = IRQ_TYPE_EDGE_RISING;
> > + break;
> > + case IRQ_TYPE_LEVEL_LOW:
> > + type = IRQ_TYPE_LEVEL_HIGH;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &owl_sirq_chip,
> > + chip_data);
> > +
> > + parent_fwspec.fwnode = domain->parent->fwnode;
> > + parent_fwspec.param_count = 3;
> > + parent_fwspec.param[0] = 0; /* SPI */
> > + parent_fwspec.param[1] = chip_data->ext_irqs[hwirq];
> > + parent_fwspec.param[2] = type;
> > +
> > + return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> > +}
> > +
> > +static const struct irq_domain_ops owl_sirq_domain_ops = {
> > + .translate = owl_sirq_domain_translate,
> > + .alloc = owl_sirq_domain_alloc,
> > + .free = irq_domain_free_irqs_common,
> > +};
> > +
> > +static const struct of_device_id owl_sirq_of_match[] = {
> > + { .compatible = "actions,s500-sirq", .data = &owl_sirq_s500_params },
> > + { .compatible = "actions,s700-sirq", .data = &owl_sirq_s500_params },
> > + { .compatible = "actions,s900-sirq", .data = &owl_sirq_s900_params },
> > + { /* sentinel */ }
> > +};
> > +
> > +static int __init owl_sirq_of_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + const struct of_device_id *match;
> > + struct irq_domain *domain, *parent_domain;
> > + struct owl_sirq_chip_data *chip_data;
> > + int ret, i;
> > +
> > + parent_domain = irq_find_host(parent);
> > + if (!parent_domain) {
> > + pr_err("%pOF: failed to find sirq parent domain\n", node);
> > + return -ENXIO;
> > + }
> > +
> > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> > + if (!chip_data)
> > + return -ENOMEM;
> > +
> > + match = of_match_node(owl_sirq_of_match, node);
> > + if (!match) {
> > + pr_warn("owl-sirq: assuming S500/S700 compatible controller\n");
> > + chip_data->params = &owl_sirq_s500_params;
> > + } else {
> > + chip_data->params = match->data;
> > + }
> > +
> > + raw_spin_lock_init(&chip_data->lock);
> > +
> > + chip_data->base = of_iomap(node, 0);
> > + if (!chip_data->base) {
> > + pr_err("%pOF: failed to map sirq registers\n", node);
> > + ret = -ENXIO;
> > + goto out_free;
> > + }
> > +
> > + ret = of_property_read_variable_u32_array(node,
> > "actions,ext-interrupts",
> > + chip_data->ext_irqs,
> > + NUM_SIRQ, NUM_SIRQ);
> > + if (ret < NUM_SIRQ) {
> > + pr_err("%pOF: failed to read sirq interrupts\n", node);
> > + goto out_unmap;
> > + }
> > +
> > + /* Set 24MHz external interrupt clock freq */
> > + for (i = 0; i < NUM_SIRQ; i++)
> > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_CLK_SEL, i);
> > +
> > + domain = irq_domain_add_hierarchy(parent_domain, 0, NUM_SIRQ, node,
> > + &owl_sirq_domain_ops, chip_data);
> > + if (!domain) {
> > + pr_err("%pOF: failed to add domain\n", node);
> > + ret = -ENOMEM;
> > + goto out_unmap;
> > + }
> > +
> > + return 0;
> > +
> > +out_unmap:
> > + iounmap(chip_data->base);
> > +out_free:
> > + kfree(chip_data);
> > +
> > + return ret;
> > +}
> > +
> > +IRQCHIP_DECLARE(owl_sirq, "actions,owl-sirq", owl_sirq_of_init);
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
Thanks again,
Cristi
On Tue, 18 Aug 2020 18:42:41 +0100,
Cristian Ciocaltea <[email protected]> wrote:
>
> Hi Marc,
>
> Thanks for your quick and detailed review!
>
> On Mon, Aug 17, 2020 at 02:52:06PM +0100, Marc Zyngier wrote:
> > On 2020-08-16 12:33, Cristian Ciocaltea wrote:
> > > This controller appears on Actions Semi Owl family SoC's S500, S700 and
> > > S900 and provides support for 3 external interrupt controllers through
> >
> > Is that really 3 interrupt controllers? Or merely 3 interrupt lines?
>
> This is mostly a leftover statement from the previous patch revision.
> I will change it to something like:
>
> "This interrupt controller is found in the Actions Semi Owl SoCs (S500,
> S700 and S900) and provides support for handling up to 3 external
> interrupt lines."
Looks good.
>
> > > dedicated SIRQ pins.
> > >
> > > Each line can be independently configured as interrupt and triggers
> > > on either of the edges (raising or falling) or either of the levels
> > > (high or low). Each line can also be masked independently.
> > >
> > > This is based on the patch series submitted by Parthiban Nallathambi:
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Signed-off-by: Parthiban Nallathambi <[email protected]>
> > > Signed-off-by: Saravanan Sekar <[email protected]>
> > > [cristi: optimized DT, various fixes/cleanups/improvements]
> > > Signed-off-by: Cristian Ciocaltea <[email protected]>
> > > ---
> > > drivers/irqchip/Makefile | 1 +
> > > drivers/irqchip/irq-owl-sirq.c | 318 +++++++++++++++++++++++++++++++++
> > > 2 files changed, 319 insertions(+)
> > > create mode 100644 drivers/irqchip/irq-owl-sirq.c
> > >
> > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > index 133f9c45744a..b8eb5b8b766d 100644
> > > --- a/drivers/irqchip/Makefile
> > > +++ b/drivers/irqchip/Makefile
> > > @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
> > > obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> > > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> > > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> > > +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> > > obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o
> > > obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o
> > > obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o
> > > diff --git a/drivers/irqchip/irq-owl-sirq.c
> > > b/drivers/irqchip/irq-owl-sirq.c
> > > new file mode 100644
> > > index 000000000000..29b7ffc40ac7
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-owl-sirq.c
> > > @@ -0,0 +1,318 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> > > + *
> > > + * Copyright (C) 2014 Actions Semi Inc.
> > > + * David Liu <[email protected]>
> > > + *
> > > + * Author: Parthiban Nallathambi <[email protected]>
> > > + * Author: Saravanan Sekar <[email protected]>
> > > + * Author: Cristian Ciocaltea <[email protected]>
> > > + */
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irqchip.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +
> > > +#define NUM_SIRQ 3
> > > +
> > > +#define INTC_EXTCTL_PENDING BIT(0)
> > > +#define INTC_EXTCTL_CLK_SEL BIT(4)
> > > +#define INTC_EXTCTL_EN BIT(5)
> > > +#define INTC_EXTCTL_TYPE_MASK GENMASK(7, 6)
> > > +#define INTC_EXTCTL_TYPE_HIGH 0
> > > +#define INTC_EXTCTL_TYPE_LOW BIT(6)
> > > +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> > > +#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
> > > +
> > > +/* S900 SIRQ1 & SIRQ2 control register offsets, relative to SIRQ0 */
> > > +#define INTC_EXTCTL1 0x0328
> > > +#define INTC_EXTCTL2 0x032c
> > > +
> > > +struct owl_sirq_params {
> > > + /* INTC_EXTCTL reg shared for all three SIRQ lines */
> > > + bool reg_shared;
> > > + /* INTC_EXTCTL reg offsets relative to controller base address */
> > > + u16 reg_offset[NUM_SIRQ];
> > > +};
> > > +
> > > +struct owl_sirq_chip_data {
> > > + const struct owl_sirq_params *params;
> > > + void __iomem *base;
> > > + raw_spinlock_t lock;
> > > + u32 ext_irqs[NUM_SIRQ];
> > > + u8 trigger;
> >
> > Nit: Please align data structure members vertically:
> >
> > struct owl_sirq_chip_data {
> > const struct owl_sirq_params *params;
> > void __iomem *base;
> > raw_spinlock_t lock;
> > u32 ext_irqs[NUM_SIRQ];
> > u8 trigger;
> > };
>
> Done.
>
> > > +};
> > > +
> > > +/* S500 and S700 SoCs */
> > > +static const struct owl_sirq_params owl_sirq_s500_params = {
> > > + .reg_shared = true,
> > > + .reg_offset = { 0, 0, 0 },
> > > +};
> > > +
> > > +/* S900 SoC */
> > > +static const struct owl_sirq_params owl_sirq_s900_params = {
> > > + .reg_shared = false,
> > > + .reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
> >
> > 0 *is* an offset, right? Why doesn't it have a name too?
>
> Right, I updated the defines section:
>
> /* S900 SIRQ control register offsets, relative to controller base address */
> #define INTC_EXTCTL0 0x0000
> #define INTC_EXTCTL1 0x0328
> [...]
>
> The controller base address points to SIRQ0 control register, so this
> offset is always 0, but I totally agree we should have a name for it.
>
> > > +};
> > > +
> > > +static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32
> > > index)
> > > +{
> > > + u32 val;
> > > +
> > > + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> > > + if (data->params->reg_shared)
> > > + val = (val >> (2 - index) * 8) & 0xff;
> >
> > base = (2 - index) * 8;
> > val = FIELD_GET(GENMASK(base + 7, base), val);
>
> Unfortunately the context doesn't allow using FIELD_GET:
>
> ./include/linux/compiler.h:392:38: error: call to ‘__compiletime_assert_159’
> declared with attribute error: FIELD_GET: mask is not constant
Bah. Turning the whole thing into compile-time values would actually
be more readable, so how about this:
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
index 29b7ffc40ac7..b771acbda7d5 100644
--- a/drivers/irqchip/irq-owl-sirq.c
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -57,13 +57,39 @@ static const struct owl_sirq_params owl_sirq_s900_params = {
.reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
};
+static u32 owl_field_get(u32 val, int index)
+{
+ switch(index) {
+ case 0:
+ return FIELD_GET(GENMASK(23, 16), val);
+ case 1:
+ return FIELD_GET(GENMASK(15, 8), val);
+ case 2:
+ default:
+ return FIELD_GET(GENMASK(7, 0), val);
+ }
+}
+
+static u32 owl_field_prep(u32 val, int index)
+{
+ switch(index) {
+ case 0:
+ return FIELD_PREP(GENMASK(23, 16), val);
+ case 1:
+ return FIELD_PREP(GENMASK(15, 8), val);
+ case 2:
+ default:
+ return FIELD_PREP(GENMASK(7, 0), val);
+ }
+}
+
static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32 index)
{
u32 val;
val = readl_relaxed(data->base + data->params->reg_offset[index]);
if (data->params->reg_shared)
- val = (val >> (2 - index) * 8) & 0xff;
+ val = owl_field_get(val, index);
return val;
}
@@ -75,9 +101,8 @@ static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
if (data->params->reg_shared) {
val = readl_relaxed(data->base + data->params->reg_offset[index]);
- val &= ~(0xff << (2 - index) * 8);
- extctl &= 0xff;
- extctl = (extctl << (2 - index) * 8) | val;
+ val &= ~owl_field_prep(0xff, index);
+ extctl = owl_field_prep(extctl, index) | val;
}
writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
Yes, this is a bit more code, but it *is* readable. Bonus points if
you add proper defines for the masks.
>
> > > +
> > > + return val;
> > > +}
> > > +
> > > +static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
> > > + u32 extctl, u32 index)
> > > +{
> > > + u32 val;
> > > +
> > > + if (data->params->reg_shared) {
> > > + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> > > + val &= ~(0xff << (2 - index) * 8);
> > > + extctl &= 0xff;
> > > + extctl = (extctl << (2 - index) * 8) | val;
> >
> > Please make use of FIELD_PREP, FIELD_GET and GENMASK.
>
> I'm not sure how to deal with the mask constness restriction, except
> using static defines and if/else statements, which is not quite
> elegant.
See above.
>
> > > + }
> > > +
> > > + writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
> > > +}
> > > +
> > > +static void owl_sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
> > > + u32 clear, u32 set, u32 index)
> > > +{
> > > + unsigned long flags;
> > > + u32 val;
> > > +
> > > + raw_spin_lock_irqsave(&d->lock, flags);
> > > + val = owl_sirq_read_extctl(d, index);
> > > + val &= ~clear;
> > > + val |= set;
> > > + owl_sirq_write_extctl(d, val, index);
> > > + raw_spin_unlock_irqrestore(&d->lock, flags);
> > > +}
> > > +
> > > +static void owl_sirq_eoi(struct irq_data *data)
> > > +{
> > > + struct owl_sirq_chip_data *chip_data =
> > > irq_data_get_irq_chip_data(data);
> > > +
> > > + /*
> > > + * Software must clear external interrupt pending, when interrupt type
> > > + * is edge triggered, so we need per SIRQ based clearing.
> > > + */
> > > + if (chip_data->trigger & (1 << data->hwirq))
> >
> > BIT(d->hwirq)
> >
> > But it also begs the question: we already have all the trigger information
> > in the irqdesc. Why do you need some additional bookkeeping?
>
> That's another leftover from the original work. Thanks for pointing
> this out, I missed it. I dropped the redundant handling of the IRQ
> trigger information and just replaced this with:
>
> if (!irqd_is_level_type(data))
Yup.
>
> > > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_PENDING,
> > > + data->hwirq);
> > > +
> > > + irq_chip_eoi_parent(data);
> > > +}
> > > +
> > > +static void owl_sirq_mask(struct irq_data *data)
> > > +{
> > > + struct owl_sirq_chip_data *chip_data =
> > > irq_data_get_irq_chip_data(data);
> > > +
> > > + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_EN, 0, data->hwirq);
> > > + irq_chip_mask_parent(data);
> > > +}
> > > +
> > > +static void owl_sirq_unmask(struct irq_data *data)
> > > +{
> > > + struct owl_sirq_chip_data *chip_data =
> > > irq_data_get_irq_chip_data(data);
> > > +
> > > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_EN, data->hwirq);
> > > + irq_chip_unmask_parent(data);
> > > +}
> > > +
> > > +/*
> > > + * GIC does not handle falling edge or active low, hence SIRQ shall be
> > > + * programmed to convert falling edge to rising edge signal and active
> > > + * low to active high signal.
> > > + */
> > > +static int owl_sirq_set_type(struct irq_data *data, unsigned int type)
> > > +{
> > > + struct owl_sirq_chip_data *chip_data =
> > > irq_data_get_irq_chip_data(data);
> > > + u32 sirq_type;
> > > +
> > > + switch (type) {
> > > + case IRQ_TYPE_LEVEL_LOW:
> > > + sirq_type = INTC_EXTCTL_TYPE_LOW;
> > > + chip_data->trigger &= ~(1 << data->hwirq);
> > > + type = IRQ_TYPE_LEVEL_HIGH;
> > > + break;
> > > + case IRQ_TYPE_LEVEL_HIGH:
> > > + sirq_type = INTC_EXTCTL_TYPE_HIGH;
> > > + chip_data->trigger &= ~(1 << data->hwirq);
> > > + break;
> > > + case IRQ_TYPE_EDGE_FALLING:
> > > + sirq_type = INTC_EXTCTL_TYPE_FALLING;
> > > + chip_data->trigger |= 1 << data->hwirq;
> > > + type = IRQ_TYPE_EDGE_RISING;
> > > + break;
> > > + case IRQ_TYPE_EDGE_RISING:
> > > + sirq_type = INTC_EXTCTL_TYPE_RISING;
> > > + chip_data->trigger |= 1 << data->hwirq;
> > > + break;
> > > + default:
> > > + WARN_ON(1);
> >
> > No need for this WARN_ON(), the core kernel is loud enough.
>
> Done.
>
> > > + return -EINVAL;
> > > + }
> > > +
> > > + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_TYPE_MASK, sirq_type,
> > > + data->hwirq);
> > > +
> > > + return irq_chip_set_type_parent(data, type);
> > > +}
> > > +
> > > +static struct irq_chip owl_sirq_chip = {
> > > + .name = "owl-sirq",
> > > + .irq_mask = owl_sirq_mask,
> > > + .irq_unmask = owl_sirq_unmask,
> > > + .irq_eoi = owl_sirq_eoi,
> > > + .irq_set_type = owl_sirq_set_type,
> > > + .irq_retrigger = irq_chip_retrigger_hierarchy,
> >
> > How about irq_set_affinity? Or does it only exist on UP systems?
>
> I have just added:
>
> #ifdef CONFIG_SMP
> .irq_set_affinity = irq_chip_set_affinity_parent,
> #endif
Looks OK.
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
On Tue, Aug 18, 2020 at 09:48:41PM +0100, Marc Zyngier wrote:
> On Tue, 18 Aug 2020 18:42:41 +0100,
> Cristian Ciocaltea <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > Thanks for your quick and detailed review!
> >
> > On Mon, Aug 17, 2020 at 02:52:06PM +0100, Marc Zyngier wrote:
> > > On 2020-08-16 12:33, Cristian Ciocaltea wrote:
> > > > This controller appears on Actions Semi Owl family SoC's S500, S700 and
> > > > S900 and provides support for 3 external interrupt controllers through
> > >
> > > Is that really 3 interrupt controllers? Or merely 3 interrupt lines?
> >
> > This is mostly a leftover statement from the previous patch revision.
> > I will change it to something like:
> >
> > "This interrupt controller is found in the Actions Semi Owl SoCs (S500,
> > S700 and S900) and provides support for handling up to 3 external
> > interrupt lines."
>
> Looks good.
>
> >
> > > > dedicated SIRQ pins.
> > > >
> > > > Each line can be independently configured as interrupt and triggers
> > > > on either of the edges (raising or falling) or either of the levels
> > > > (high or low). Each line can also be masked independently.
> > > >
> > > > This is based on the patch series submitted by Parthiban Nallathambi:
> > > > https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > Signed-off-by: Parthiban Nallathambi <[email protected]>
> > > > Signed-off-by: Saravanan Sekar <[email protected]>
> > > > [cristi: optimized DT, various fixes/cleanups/improvements]
> > > > Signed-off-by: Cristian Ciocaltea <[email protected]>
> > > > ---
> > > > drivers/irqchip/Makefile | 1 +
> > > > drivers/irqchip/irq-owl-sirq.c | 318 +++++++++++++++++++++++++++++++++
> > > > 2 files changed, 319 insertions(+)
> > > > create mode 100644 drivers/irqchip/irq-owl-sirq.c
> > > >
> > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > > index 133f9c45744a..b8eb5b8b766d 100644
> > > > --- a/drivers/irqchip/Makefile
> > > > +++ b/drivers/irqchip/Makefile
> > > > @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-cpu.o
> > > > obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> > > > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> > > > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> > > > +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> > > > obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o
> > > > obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o
> > > > obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o
> > > > diff --git a/drivers/irqchip/irq-owl-sirq.c
> > > > b/drivers/irqchip/irq-owl-sirq.c
> > > > new file mode 100644
> > > > index 000000000000..29b7ffc40ac7
> > > > --- /dev/null
> > > > +++ b/drivers/irqchip/irq-owl-sirq.c
> > > > @@ -0,0 +1,318 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> > > > + *
> > > > + * Copyright (C) 2014 Actions Semi Inc.
> > > > + * David Liu <[email protected]>
> > > > + *
> > > > + * Author: Parthiban Nallathambi <[email protected]>
> > > > + * Author: Saravanan Sekar <[email protected]>
> > > > + * Author: Cristian Ciocaltea <[email protected]>
> > > > + */
> > > > +
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/irqchip.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_irq.h>
> > > > +
> > > > +#define NUM_SIRQ 3
> > > > +
> > > > +#define INTC_EXTCTL_PENDING BIT(0)
> > > > +#define INTC_EXTCTL_CLK_SEL BIT(4)
> > > > +#define INTC_EXTCTL_EN BIT(5)
> > > > +#define INTC_EXTCTL_TYPE_MASK GENMASK(7, 6)
> > > > +#define INTC_EXTCTL_TYPE_HIGH 0
> > > > +#define INTC_EXTCTL_TYPE_LOW BIT(6)
> > > > +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> > > > +#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
> > > > +
> > > > +/* S900 SIRQ1 & SIRQ2 control register offsets, relative to SIRQ0 */
> > > > +#define INTC_EXTCTL1 0x0328
> > > > +#define INTC_EXTCTL2 0x032c
> > > > +
> > > > +struct owl_sirq_params {
> > > > + /* INTC_EXTCTL reg shared for all three SIRQ lines */
> > > > + bool reg_shared;
> > > > + /* INTC_EXTCTL reg offsets relative to controller base address */
> > > > + u16 reg_offset[NUM_SIRQ];
> > > > +};
> > > > +
> > > > +struct owl_sirq_chip_data {
> > > > + const struct owl_sirq_params *params;
> > > > + void __iomem *base;
> > > > + raw_spinlock_t lock;
> > > > + u32 ext_irqs[NUM_SIRQ];
> > > > + u8 trigger;
> > >
> > > Nit: Please align data structure members vertically:
> > >
> > > struct owl_sirq_chip_data {
> > > const struct owl_sirq_params *params;
> > > void __iomem *base;
> > > raw_spinlock_t lock;
> > > u32 ext_irqs[NUM_SIRQ];
> > > u8 trigger;
> > > };
> >
> > Done.
> >
> > > > +};
> > > > +
> > > > +/* S500 and S700 SoCs */
> > > > +static const struct owl_sirq_params owl_sirq_s500_params = {
> > > > + .reg_shared = true,
> > > > + .reg_offset = { 0, 0, 0 },
> > > > +};
> > > > +
> > > > +/* S900 SoC */
> > > > +static const struct owl_sirq_params owl_sirq_s900_params = {
> > > > + .reg_shared = false,
> > > > + .reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
> > >
> > > 0 *is* an offset, right? Why doesn't it have a name too?
> >
> > Right, I updated the defines section:
> >
> > /* S900 SIRQ control register offsets, relative to controller base address */
> > #define INTC_EXTCTL0 0x0000
> > #define INTC_EXTCTL1 0x0328
> > [...]
> >
> > The controller base address points to SIRQ0 control register, so this
> > offset is always 0, but I totally agree we should have a name for it.
> >
> > > > +};
> > > > +
> > > > +static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32
> > > > index)
> > > > +{
> > > > + u32 val;
> > > > +
> > > > + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> > > > + if (data->params->reg_shared)
> > > > + val = (val >> (2 - index) * 8) & 0xff;
> > >
> > > base = (2 - index) * 8;
> > > val = FIELD_GET(GENMASK(base + 7, base), val);
> >
> > Unfortunately the context doesn't allow using FIELD_GET:
> >
> > ./include/linux/compiler.h:392:38: error: call to ‘__compiletime_assert_159’
> > declared with attribute error: FIELD_GET: mask is not constant
>
> Bah. Turning the whole thing into compile-time values would actually
> be more readable, so how about this:
>
> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
> index 29b7ffc40ac7..b771acbda7d5 100644
> --- a/drivers/irqchip/irq-owl-sirq.c
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -57,13 +57,39 @@ static const struct owl_sirq_params owl_sirq_s900_params = {
> .reg_offset = { 0, INTC_EXTCTL1, INTC_EXTCTL2 },
> };
>
> +static u32 owl_field_get(u32 val, int index)
> +{
> + switch(index) {
> + case 0:
> + return FIELD_GET(GENMASK(23, 16), val);
> + case 1:
> + return FIELD_GET(GENMASK(15, 8), val);
> + case 2:
> + default:
> + return FIELD_GET(GENMASK(7, 0), val);
> + }
> +}
> +
> +static u32 owl_field_prep(u32 val, int index)
> +{
> + switch(index) {
> + case 0:
> + return FIELD_PREP(GENMASK(23, 16), val);
> + case 1:
> + return FIELD_PREP(GENMASK(15, 8), val);
> + case 2:
> + default:
> + return FIELD_PREP(GENMASK(7, 0), val);
> + }
> +}
> +
> static u32 owl_sirq_read_extctl(struct owl_sirq_chip_data *data, u32 index)
> {
> u32 val;
>
> val = readl_relaxed(data->base + data->params->reg_offset[index]);
> if (data->params->reg_shared)
> - val = (val >> (2 - index) * 8) & 0xff;
> + val = owl_field_get(val, index);
>
> return val;
> }
> @@ -75,9 +101,8 @@ static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
>
> if (data->params->reg_shared) {
> val = readl_relaxed(data->base + data->params->reg_offset[index]);
> - val &= ~(0xff << (2 - index) * 8);
> - extctl &= 0xff;
> - extctl = (extctl << (2 - index) * 8) | val;
> + val &= ~owl_field_prep(0xff, index);
> + extctl = owl_field_prep(extctl, index) | val;
> }
>
> writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
>
> Yes, this is a bit more code, but it *is* readable. Bonus points if
> you add proper defines for the masks.
That's great, thanks a lot!
I have submitted a new revision:
https://lore.kernel.org/lkml/[email protected]/
Regards,
Cristi
> >
> > > > +
> > > > + return val;
> > > > +}
> > > > +
> > > > +static void owl_sirq_write_extctl(struct owl_sirq_chip_data *data,
> > > > + u32 extctl, u32 index)
> > > > +{
> > > > + u32 val;
> > > > +
> > > > + if (data->params->reg_shared) {
> > > > + val = readl_relaxed(data->base + data->params->reg_offset[index]);
> > > > + val &= ~(0xff << (2 - index) * 8);
> > > > + extctl &= 0xff;
> > > > + extctl = (extctl << (2 - index) * 8) | val;
> > >
> > > Please make use of FIELD_PREP, FIELD_GET and GENMASK.
> >
> > I'm not sure how to deal with the mask constness restriction, except
> > using static defines and if/else statements, which is not quite
> > elegant.
>
> See above.
>
> >
> > > > + }
> > > > +
> > > > + writel_relaxed(extctl, data->base + data->params->reg_offset[index]);
> > > > +}
> > > > +
> > > > +static void owl_sirq_clear_set_extctl(struct owl_sirq_chip_data *d,
> > > > + u32 clear, u32 set, u32 index)
> > > > +{
> > > > + unsigned long flags;
> > > > + u32 val;
> > > > +
> > > > + raw_spin_lock_irqsave(&d->lock, flags);
> > > > + val = owl_sirq_read_extctl(d, index);
> > > > + val &= ~clear;
> > > > + val |= set;
> > > > + owl_sirq_write_extctl(d, val, index);
> > > > + raw_spin_unlock_irqrestore(&d->lock, flags);
> > > > +}
> > > > +
> > > > +static void owl_sirq_eoi(struct irq_data *data)
> > > > +{
> > > > + struct owl_sirq_chip_data *chip_data =
> > > > irq_data_get_irq_chip_data(data);
> > > > +
> > > > + /*
> > > > + * Software must clear external interrupt pending, when interrupt type
> > > > + * is edge triggered, so we need per SIRQ based clearing.
> > > > + */
> > > > + if (chip_data->trigger & (1 << data->hwirq))
> > >
> > > BIT(d->hwirq)
> > >
> > > But it also begs the question: we already have all the trigger information
> > > in the irqdesc. Why do you need some additional bookkeeping?
> >
> > That's another leftover from the original work. Thanks for pointing
> > this out, I missed it. I dropped the redundant handling of the IRQ
> > trigger information and just replaced this with:
> >
> > if (!irqd_is_level_type(data))
>
> Yup.
>
> >
> > > > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_PENDING,
> > > > + data->hwirq);
> > > > +
> > > > + irq_chip_eoi_parent(data);
> > > > +}
> > > > +
> > > > +static void owl_sirq_mask(struct irq_data *data)
> > > > +{
> > > > + struct owl_sirq_chip_data *chip_data =
> > > > irq_data_get_irq_chip_data(data);
> > > > +
> > > > + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_EN, 0, data->hwirq);
> > > > + irq_chip_mask_parent(data);
> > > > +}
> > > > +
> > > > +static void owl_sirq_unmask(struct irq_data *data)
> > > > +{
> > > > + struct owl_sirq_chip_data *chip_data =
> > > > irq_data_get_irq_chip_data(data);
> > > > +
> > > > + owl_sirq_clear_set_extctl(chip_data, 0, INTC_EXTCTL_EN, data->hwirq);
> > > > + irq_chip_unmask_parent(data);
> > > > +}
> > > > +
> > > > +/*
> > > > + * GIC does not handle falling edge or active low, hence SIRQ shall be
> > > > + * programmed to convert falling edge to rising edge signal and active
> > > > + * low to active high signal.
> > > > + */
> > > > +static int owl_sirq_set_type(struct irq_data *data, unsigned int type)
> > > > +{
> > > > + struct owl_sirq_chip_data *chip_data =
> > > > irq_data_get_irq_chip_data(data);
> > > > + u32 sirq_type;
> > > > +
> > > > + switch (type) {
> > > > + case IRQ_TYPE_LEVEL_LOW:
> > > > + sirq_type = INTC_EXTCTL_TYPE_LOW;
> > > > + chip_data->trigger &= ~(1 << data->hwirq);
> > > > + type = IRQ_TYPE_LEVEL_HIGH;
> > > > + break;
> > > > + case IRQ_TYPE_LEVEL_HIGH:
> > > > + sirq_type = INTC_EXTCTL_TYPE_HIGH;
> > > > + chip_data->trigger &= ~(1 << data->hwirq);
> > > > + break;
> > > > + case IRQ_TYPE_EDGE_FALLING:
> > > > + sirq_type = INTC_EXTCTL_TYPE_FALLING;
> > > > + chip_data->trigger |= 1 << data->hwirq;
> > > > + type = IRQ_TYPE_EDGE_RISING;
> > > > + break;
> > > > + case IRQ_TYPE_EDGE_RISING:
> > > > + sirq_type = INTC_EXTCTL_TYPE_RISING;
> > > > + chip_data->trigger |= 1 << data->hwirq;
> > > > + break;
> > > > + default:
> > > > + WARN_ON(1);
> > >
> > > No need for this WARN_ON(), the core kernel is loud enough.
> >
> > Done.
> >
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + owl_sirq_clear_set_extctl(chip_data, INTC_EXTCTL_TYPE_MASK, sirq_type,
> > > > + data->hwirq);
> > > > +
> > > > + return irq_chip_set_type_parent(data, type);
> > > > +}
> > > > +
> > > > +static struct irq_chip owl_sirq_chip = {
> > > > + .name = "owl-sirq",
> > > > + .irq_mask = owl_sirq_mask,
> > > > + .irq_unmask = owl_sirq_unmask,
> > > > + .irq_eoi = owl_sirq_eoi,
> > > > + .irq_set_type = owl_sirq_set_type,
> > > > + .irq_retrigger = irq_chip_retrigger_hierarchy,
> > >
> > > How about irq_set_affinity? Or does it only exist on UP systems?
> >
> > I have just added:
> >
> > #ifdef CONFIG_SMP
> > .irq_set_affinity = irq_chip_set_affinity_parent,
> > #endif
>
> Looks OK.
>
> Cheers,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.