On Tue, 18 Aug 2020 18:42:41 +0100,
Cristian Ciocaltea [off-list ref] wrote:

quoted

Hi Marc,

Thanks for your quick and detailed review!

On Mon, Aug 17, 2020 at 02:52:06PM +0100, Marc Zyngier wrote:

quoted

On 2020-08-16 12:33, Cristian Ciocaltea wrote:

quoted

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.

quoted

quoted

quoted

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/20181126100356.2840578-1-pn@denx.de/ (local)

Signed-off-by: Parthiban Nallathambi <redacted>
Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
[cristi: optimized DT, various fixes/cleanups/improvements]
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 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 <liuwei@actions-semi.com>
+ *
+ * Author: Parthiban Nallathambi <pn@denx.de>
+ * Author: Saravanan Sekar <sravanhome@gmail.com>
+ * Author: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
+ */
+
+#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.

quoted

quoted

+};
+
+/* 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.

quoted

quoted

+};
+
+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.