[PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[Aleix Roca Nonell]

This driver adds support for the RTD1296 and RTD1295 interrupt
controller (intc). It is based on both the BPI-SINOVOIP project and
Andreas Färber's previous attempt to submit a similar driver.

There is currently no publicly available datasheet on this SoC and the
exact behaviour of the registers controlling the intc remain uncertain.

This driver controls two intcs: "iso" and "misc". Each intc has its own
Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR).
However, not all "misc" intc irqs have the same offsets for both ISR and
IER. For this reason an ISR to IER offsets table is defined.

The driver catches the IER value to reduce accesses to the table inside the
interrupt handler. Actually, the driver stores the ISR offsets of currently
enabled interrupts in a variable.

Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com>
---
 drivers/irqchip/Makefile      |   1 +
 drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++
 2 files changed, 372 insertions(+)
 create mode 100644 drivers/irqchip/irq-rtd129x.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 606a003a0000..0689c3956250 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
 obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
 obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
+obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd129x.o
diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c
new file mode 100644
index 000000000000..76358ca50f10
--- /dev/null
+++ b/drivers/irqchip/irq-rtd129x.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+#include <linux/irqchip.h>
+#include <linux/bits.h>
+#include <linux/irqchip/chained_irq.h>
+
+#define RTD129X_INTC_NR_IRQS 32
+#define DEV_NAME "RTD1296_INTC"
+
+/*
+ * This interrupt controller (hereinafter intc) driver controls two intcs: "iso"
+ * and "misc". Each intc has its own Interrupt Enable Register (IER) and
+ * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the
+ * same offsets for both ISR and IER. For this reason an ISR to IER offsets
+ * table is defined. Also, to reduce accesses to this table in the interrupt
+ * handler, the driver stores the ISR offsets of currently enabled interrupts in
+ * a variable.
+ */
+
+enum misc_int_en {
+	MISC_INT_FAIL		= 0xFF,
+	MISC_INT_RVD		= 0xFE,
+	MISC_INT_EN_FAN		= 29,
+	MISC_INT_EN_I2C3	= 28,
+	MISC_INT_EN_GSPI	= 27,
+	MISC_INT_EN_I2C2	= 26,
+	MISC_INT_EN_SC0		= 24,
+	MISC_INT_EN_LSADC1	= 22,
+	MISC_INT_EN_LSADC0	= 21,
+	MISC_INT_EN_GPIODA	= 20,
+	MISC_INT_EN_GPIOA	= 19,
+	MISC_INT_EN_I2C4	= 15,
+	MISC_INT_EN_I2C5	= 14,
+	MISC_INT_EN_RTC_DATA	= 12,
+	MISC_INT_EN_RTC_HOUR	= 11,
+	MISC_INT_EN_RTC_MIN	= 10,
+	MISC_INT_EN_UR2		= 7,
+	MISC_INT_EN_UR2_TO	= 6,
+	MISC_INT_EN_UR1_TO	= 5,
+	MISC_INT_EN_UR1		= 3,
+};
+
+enum iso_int_en {
+	ISO_INT_FAIL		= 0xFF,
+	ISO_INT_RVD		= 0xFE,
+	ISO_INT_EN_I2C1_REQ	= 31,
+	ISO_INT_EN_GPHY_AV	= 30,
+	ISO_INT_EN_GPHY_DV	= 29,
+	ISO_INT_EN_GPIODA	= 20,
+	ISO_INT_EN_GPIOA	= 19,
+	ISO_INT_EN_RTC_ALARM	= 13,
+	ISO_INT_EN_RTC_HSEC	= 12,
+	ISO_INT_EN_I2C1		= 11,
+	ISO_INT_EN_I2C0		= 8,
+	ISO_INT_EN_IRDA		= 5,
+	ISO_INT_EN_UR0		= 2,
+};
+
+unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = {
+	MISC_INT_FAIL,		/* Bit0 */
+	MISC_INT_FAIL,		/* Bit1 */
+	MISC_INT_RVD,		/* Bit2 */
+	MISC_INT_EN_UR1,	/* Bit3 */
+	MISC_INT_FAIL,		/* Bit4 */
+	MISC_INT_EN_UR1_TO,	/* Bit5 */
+	MISC_INT_RVD,		/* Bit6 */
+	MISC_INT_RVD,		/* Bit7 */
+	MISC_INT_EN_UR2,	/* Bit8 */
+	MISC_INT_RVD,		/* Bit9 */
+	MISC_INT_EN_RTC_MIN,	/* Bit10 */
+	MISC_INT_EN_RTC_HOUR,	/* Bit11 */
+	MISC_INT_EN_RTC_DATA,	/* Bit12 */
+	MISC_INT_EN_UR2_TO,	/* Bit13 */
+	MISC_INT_EN_I2C5,	/* Bit14 */
+	MISC_INT_EN_I2C4,	/* Bit15 */
+	MISC_INT_FAIL,		/* Bit16 */
+	MISC_INT_FAIL,		/* Bit17 */
+	MISC_INT_FAIL,		/* Bit18 */
+	MISC_INT_EN_GPIOA,	/* Bit19 */
+	MISC_INT_EN_GPIODA,	/* Bit20 */
+	MISC_INT_EN_LSADC0,	/* Bit21 */
+	MISC_INT_EN_LSADC1,	/* Bit22 */
+	MISC_INT_EN_I2C3,	/* Bit23 */
+	MISC_INT_EN_SC0,	/* Bit24 */
+	MISC_INT_FAIL,		/* Bit25 */
+	MISC_INT_EN_I2C2,	/* Bit26 */
+	MISC_INT_EN_GSPI,	/* Bit27 */
+	MISC_INT_FAIL,		/* Bit28 */
+	MISC_INT_EN_FAN,	/* Bit29 */
+	MISC_INT_FAIL,		/* Bit30 */
+	MISC_INT_FAIL		/* Bit31 */
+};
+
+unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = {
+	ISO_INT_FAIL,		/* Bit0 */
+	ISO_INT_RVD,		/* Bit1 */
+	ISO_INT_EN_UR0,		/* Bit2 */
+	ISO_INT_FAIL,		/* Bit3 */
+	ISO_INT_FAIL,		/* Bit4 */
+	ISO_INT_EN_IRDA,	/* Bit5 */
+	ISO_INT_FAIL,		/* Bit6 */
+	ISO_INT_RVD,		/* Bit7 */
+	ISO_INT_EN_I2C0,	/* Bit8 */
+	ISO_INT_RVD,		/* Bit9 */
+	ISO_INT_FAIL,		/* Bit10 */
+	ISO_INT_EN_I2C1,	/* Bit11 */
+	ISO_INT_EN_RTC_HSEC,	/* Bit12 */
+	ISO_INT_EN_RTC_ALARM,	/* Bit13 */
+	ISO_INT_FAIL,		/* Bit14 */
+	ISO_INT_FAIL,		/* Bit15 */
+	ISO_INT_FAIL,		/* Bit16 */
+	ISO_INT_FAIL,		/* Bit17 */
+	ISO_INT_FAIL,		/* Bit18 */
+	ISO_INT_EN_GPIOA,	/* Bit19 */
+	ISO_INT_EN_GPIODA,	/* Bit20 */
+	ISO_INT_RVD,		/* Bit21 */
+	ISO_INT_RVD,		/* Bit22 */
+	ISO_INT_RVD,		/* Bit23 */
+	ISO_INT_RVD,		/* Bit24 */
+	ISO_INT_FAIL,		/* Bit25 */
+	ISO_INT_FAIL,		/* Bit26 */
+	ISO_INT_FAIL,		/* Bit27 */
+	ISO_INT_FAIL,		/* Bit28 */
+	ISO_INT_EN_GPHY_DV,	/* Bit29 */
+	ISO_INT_EN_GPHY_AV,	/* Bit30 */
+	ISO_INT_EN_I2C1_REQ	/* Bit31 */
+};
+
+struct rtd129x_intc_data {
+	void __iomem		*unmask;
+	void __iomem		*isr;
+	void __iomem		*ier;
+	u32			ier_cached;
+	u32			isr_en;
+	raw_spinlock_t		lock;
+	unsigned int		parent_irq;
+	const unsigned char	*en_map;
+};
+
+static struct irq_domain *rtd129x_intc_domain;
+
+static void rtd129x_intc_irq_handle(struct irq_desc *desc)
+{
+	struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int local_irq;
+	u32 status;
+	int i;
+
+	chained_irq_enter(chip, desc);
+
+	raw_spin_lock(&priv->lock);
+	status = readl_relaxed(priv->isr);
+	status &= priv->isr_en;
+	raw_spin_unlock(&priv->lock);
+
+	while (status) {
+		i = __ffs(status);
+		status &= ~BIT(i);
+
+		local_irq = irq_find_mapping(rtd129x_intc_domain, i);
+		if (likely(local_irq)) {
+			if (!generic_handle_irq(local_irq))
+				writel_relaxed(BIT(i), priv->isr);
+		} else {
+			handle_bad_irq(desc);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void rtd129x_intc_mask(struct irq_data *data)
+{
+	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
+
+	writel_relaxed(BIT(data->hwirq), priv->isr);
+}
+
+static void rtd129x_intc_unmask(struct irq_data *data)
+{
+	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
+
+	writel_relaxed(BIT(data->hwirq), priv->unmask);
+}
+
+static void rtd129x_intc_enable(struct irq_data *data)
+{
+	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u8 en_offset;
+
+	en_offset = priv->en_map[data->hwirq];
+
+	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
+		raw_spin_lock_irqsave(&priv->lock, flags);
+
+		priv->isr_en |= BIT(data->hwirq);
+		priv->ier_cached |= BIT(en_offset);
+		writel_relaxed(priv->ier_cached, priv->ier);
+
+		raw_spin_unlock_irqrestore(&priv->lock, flags);
+	} else if (en_offset == MISC_INT_FAIL) {
+		pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq);
+	}
+}
+
+static void rtd129x_intc_disable(struct irq_data *data)
+{
+	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u8 en_offset;
+
+	en_offset = priv->en_map[data->hwirq];
+
+	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
+		raw_spin_lock_irqsave(&priv->lock, flags);
+
+		priv->isr_en &= ~BIT(data->hwirq);
+		priv->ier_cached &= ~BIT(en_offset);
+		writel_relaxed(priv->ier_cached, priv->ier);
+
+		raw_spin_unlock_irqrestore(&priv->lock, flags);
+	} else if (en_offset == MISC_INT_FAIL) {
+		pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq);
+	}
+}
+
+static struct irq_chip rtd129x_intc_chip = {
+	.name		= DEV_NAME,
+	.irq_mask	= rtd129x_intc_mask,
+	.irq_unmask	= rtd129x_intc_unmask,
+	.irq_enable	= rtd129x_intc_enable,
+	.irq_disable	= rtd129x_intc_disable,
+};
+
+static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq,
+			    irq_hw_number_t hw_irq)
+{
+	struct rtd129x_intc_data *priv = d->host_data;
+
+	irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq);
+	irq_set_chip_data(virq, priv);
+	irq_set_probe(virq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops rtd129x_intc_domain_ops = {
+	.xlate			= irq_domain_xlate_onecell,
+	.map			= rtd129x_intc_map,
+};
+
+static const struct of_device_id rtd129x_intc_matches[] = {
+	{ .compatible = "realtek,rtd129x-intc-misc",
+		.data = rtd129x_intc_enable_map_misc
+	},
+	{ .compatible = "realtek,rtd129x-intc-iso",
+		.data = rtd129x_intc_enable_map_iso
+	},
+	{ }
+};
+
+static int rtd129x_intc_of_init(struct device_node *node,
+				struct device_node *parent)
+{
+	struct rtd129x_intc_data *priv;
+	const struct of_device_id *match;
+	u32 isr_tmp, ier_tmp, ier_bit;
+	int ret, i;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	raw_spin_lock_init(&priv->lock);
+
+	priv->isr = of_iomap(node, 0);
+	if (!priv->isr) {
+		pr_err("unable to obtain status reg iomap address\n");
+		ret = -ENOMEM;
+		goto free_priv;
+	}
+
+	priv->ier = of_iomap(node, 1);
+	if (!priv->ier) {
+		pr_err("unable to obtain enable reg iomap address\n");
+		ret = -ENOMEM;
+		goto iounmap_status;
+	}
+
+	priv->unmask = of_iomap(node, 2);
+	if (!priv->unmask) {
+		pr_err("unable to obtain unmask reg iomap address\n");
+		ret = -ENOMEM;
+		goto iounmap_enable;
+	}
+
+	priv->parent_irq = irq_of_parse_and_map(node, 0);
+	if (!priv->parent_irq) {
+		pr_err("failed to map parent interrupt %d\n", priv->parent_irq);
+		ret = -EINVAL;
+		goto iounmap_all;
+	}
+
+	match = of_match_node(rtd129x_intc_matches, node);
+	if (!match) {
+		pr_err("failed to find matching node\n");
+		ret = -ENODEV;
+		goto iounmap_all;
+	}
+	priv->en_map = match->data;
+
+	// initialize enabled irq's map to its matching status bit in isr by
+	// inverse walking the enable to status offsets map. Only needed for
+	// misc
+	priv->ier_cached = readl_relaxed(priv->ier);
+	if (priv->en_map == rtd129x_intc_enable_map_misc) {
+		ier_tmp = priv->ier_cached;
+		isr_tmp = 0;
+		while (ier_tmp) {
+			ier_bit = __ffs(ier_tmp);
+			ier_tmp &= ~BIT(ier_bit);
+			for (i = 0; i < RTD129X_INTC_NR_IRQS; i++)
+				if (priv->en_map[i] == ier_bit)
+					isr_tmp |= BIT(i);
+		}
+		priv->isr_en = isr_tmp;
+	} else {
+		priv->isr_en = priv->ier_cached;
+	}
+
+	rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS,
+						    &rtd129x_intc_domain_ops,
+						    priv);
+	if (!rtd129x_intc_domain) {
+		pr_err("failed to create irq domain\n");
+		ret = -ENOMEM;
+		goto iounmap_all;
+	}
+
+	irq_set_chained_handler_and_data(priv->parent_irq,
+					 rtd129x_intc_irq_handle, priv);
+
+	return 0;
+
+iounmap_all:
+	iounmap(priv->unmask);
+iounmap_enable:
+	iounmap(priv->ier);
+iounmap_status:
+	iounmap(priv->isr);
+free_priv:
+	kfree(priv);
+err:
+	return ret;
+}
+
+IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc",
+		rtd129x_intc_of_init);
+IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso",
+		rtd129x_intc_of_init);
-- 
2.21.0

Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[Andreas Färber]

Am 07.07.19 um 15:22 schrieb Aleix Roca Nonell:
> This driver adds support for the RTD1296 and RTD1295 interrupt
> controller (intc). It is based on both the BPI-SINOVOIP project and
> Andreas Färber's previous attempt to submit a similar driver.

Doing that without my Signed-off-by and Copyright is certainly not okay.
It is also lacking a clear description of what you changed from my last
submission or the post-submission GitHub version adressing review
comments, which broke.

Regards,
Andreas

-- 
SUSE Linux GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[Aleix Roca Nonell]

On Sun, Jul 07, 2019 at 03:27:16PM +0200, Andreas Färber wrote:
> Am 07.07.19 um 15:22 schrieb Aleix Roca Nonell:
> > This driver adds support for the RTD1296 and RTD1295 interrupt
> > controller (intc). It is based on both the BPI-SINOVOIP project and
> > Andreas Färber's previous attempt to submit a similar driver.
> 
> Doing that without my Signed-off-by and Copyright is certainly not okay.
> It is also lacking a clear description of what you changed from my last
> submission or the post-submission GitHub version adressing review
> comments, which broke.

I'm really sorry about that, because I rewrote the code (almost) from
scratch (given that I wasn't aware of this previous attempt when I
started working with it) I was not sure if it was necessary. I will
address this in the next version of the patch series.

Thank you and sorry for the inconvenience.

> 
> Regards,
> Andreas
> 
> -- 
> SUSE Linux GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG Nürnberg)

Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[Marc Zyngier]

On 07/07/2019 14:22, Aleix Roca Nonell wrote:
> This driver adds support for the RTD1296 and RTD1295 interrupt
> controller (intc). It is based on both the BPI-SINOVOIP project and
> Andreas Färber's previous attempt to submit a similar driver.
> 
> There is currently no publicly available datasheet on this SoC and the
> exact behaviour of the registers controlling the intc remain uncertain.
> 
> This driver controls two intcs: "iso" and "misc". Each intc has its own
> Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR).

Register

> However, not all "misc" intc irqs have the same offsets for both ISR and
> IER. For this reason an ISR to IER offsets table is defined.
> 
> The driver catches the IER value to reduce accesses to the table inside the
> interrupt handler. Actually, the driver stores the ISR offsets of currently
> enabled interrupts in a variable.
> 
> Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com>

I expect Andreas and you to sort the attribution issue. I'm certainly
not going to take this in if things are unclear.

> ---
>  drivers/irqchip/Makefile      |   1 +
>  drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++
>  2 files changed, 372 insertions(+)
>  create mode 100644 drivers/irqchip/irq-rtd129x.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 606a003a0000..0689c3956250 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
>  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>  obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
> +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd129x.o
> diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c
> new file mode 100644
> index 000000000000..76358ca50f10
> --- /dev/null
> +++ b/drivers/irqchip/irq-rtd129x.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/irqchip.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/spinlock.h>
> +#include <linux/irqchip.h>
> +#include <linux/bits.h>
> +#include <linux/irqchip/chained_irq.h>
> +
> +#define RTD129X_INTC_NR_IRQS 32
> +#define DEV_NAME "RTD1296_INTC"
> +
> +/*
> + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso"
> + * and "misc". Each intc has its own Interrupt Enable Register (IER) and
> + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the
> + * same offsets for both ISR and IER. For this reason an ISR to IER offsets
> + * table is defined. Also, to reduce accesses to this table in the interrupt
> + * handler, the driver stores the ISR offsets of currently enabled interrupts in
> + * a variable.
> + */
> +
> +enum misc_int_en {
> +	MISC_INT_FAIL		= 0xFF,
> +	MISC_INT_RVD		= 0xFE,
> +	MISC_INT_EN_FAN		= 29,
> +	MISC_INT_EN_I2C3	= 28,
> +	MISC_INT_EN_GSPI	= 27,
> +	MISC_INT_EN_I2C2	= 26,
> +	MISC_INT_EN_SC0		= 24,
> +	MISC_INT_EN_LSADC1	= 22,
> +	MISC_INT_EN_LSADC0	= 21,
> +	MISC_INT_EN_GPIODA	= 20,
> +	MISC_INT_EN_GPIOA	= 19,
> +	MISC_INT_EN_I2C4	= 15,
> +	MISC_INT_EN_I2C5	= 14,
> +	MISC_INT_EN_RTC_DATA	= 12,
> +	MISC_INT_EN_RTC_HOUR	= 11,
> +	MISC_INT_EN_RTC_MIN	= 10,
> +	MISC_INT_EN_UR2		= 7,
> +	MISC_INT_EN_UR2_TO	= 6,
> +	MISC_INT_EN_UR1_TO	= 5,
> +	MISC_INT_EN_UR1		= 3,
> +};
> +
> +enum iso_int_en {
> +	ISO_INT_FAIL		= 0xFF,
> +	ISO_INT_RVD		= 0xFE,
> +	ISO_INT_EN_I2C1_REQ	= 31,
> +	ISO_INT_EN_GPHY_AV	= 30,
> +	ISO_INT_EN_GPHY_DV	= 29,
> +	ISO_INT_EN_GPIODA	= 20,
> +	ISO_INT_EN_GPIOA	= 19,
> +	ISO_INT_EN_RTC_ALARM	= 13,
> +	ISO_INT_EN_RTC_HSEC	= 12,
> +	ISO_INT_EN_I2C1		= 11,
> +	ISO_INT_EN_I2C0		= 8,
> +	ISO_INT_EN_IRDA		= 5,
> +	ISO_INT_EN_UR0		= 2,
> +};
> +
> +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = {
> +	MISC_INT_FAIL,		/* Bit0 */
> +	MISC_INT_FAIL,		/* Bit1 */
> +	MISC_INT_RVD,		/* Bit2 */
> +	MISC_INT_EN_UR1,	/* Bit3 */
> +	MISC_INT_FAIL,		/* Bit4 */
> +	MISC_INT_EN_UR1_TO,	/* Bit5 */
> +	MISC_INT_RVD,		/* Bit6 */
> +	MISC_INT_RVD,		/* Bit7 */
> +	MISC_INT_EN_UR2,	/* Bit8 */
> +	MISC_INT_RVD,		/* Bit9 */
> +	MISC_INT_EN_RTC_MIN,	/* Bit10 */
> +	MISC_INT_EN_RTC_HOUR,	/* Bit11 */
> +	MISC_INT_EN_RTC_DATA,	/* Bit12 */
> +	MISC_INT_EN_UR2_TO,	/* Bit13 */
> +	MISC_INT_EN_I2C5,	/* Bit14 */
> +	MISC_INT_EN_I2C4,	/* Bit15 */
> +	MISC_INT_FAIL,		/* Bit16 */
> +	MISC_INT_FAIL,		/* Bit17 */
> +	MISC_INT_FAIL,		/* Bit18 */
> +	MISC_INT_EN_GPIOA,	/* Bit19 */
> +	MISC_INT_EN_GPIODA,	/* Bit20 */
> +	MISC_INT_EN_LSADC0,	/* Bit21 */
> +	MISC_INT_EN_LSADC1,	/* Bit22 */
> +	MISC_INT_EN_I2C3,	/* Bit23 */
> +	MISC_INT_EN_SC0,	/* Bit24 */
> +	MISC_INT_FAIL,		/* Bit25 */
> +	MISC_INT_EN_I2C2,	/* Bit26 */
> +	MISC_INT_EN_GSPI,	/* Bit27 */
> +	MISC_INT_FAIL,		/* Bit28 */
> +	MISC_INT_EN_FAN,	/* Bit29 */
> +	MISC_INT_FAIL,		/* Bit30 */
> +	MISC_INT_FAIL		/* Bit31 */
> +};
> +
> +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = {
> +	ISO_INT_FAIL,		/* Bit0 */
> +	ISO_INT_RVD,		/* Bit1 */
> +	ISO_INT_EN_UR0,		/* Bit2 */
> +	ISO_INT_FAIL,		/* Bit3 */
> +	ISO_INT_FAIL,		/* Bit4 */
> +	ISO_INT_EN_IRDA,	/* Bit5 */
> +	ISO_INT_FAIL,		/* Bit6 */
> +	ISO_INT_RVD,		/* Bit7 */
> +	ISO_INT_EN_I2C0,	/* Bit8 */
> +	ISO_INT_RVD,		/* Bit9 */
> +	ISO_INT_FAIL,		/* Bit10 */
> +	ISO_INT_EN_I2C1,	/* Bit11 */
> +	ISO_INT_EN_RTC_HSEC,	/* Bit12 */
> +	ISO_INT_EN_RTC_ALARM,	/* Bit13 */
> +	ISO_INT_FAIL,		/* Bit14 */
> +	ISO_INT_FAIL,		/* Bit15 */
> +	ISO_INT_FAIL,		/* Bit16 */
> +	ISO_INT_FAIL,		/* Bit17 */
> +	ISO_INT_FAIL,		/* Bit18 */
> +	ISO_INT_EN_GPIOA,	/* Bit19 */
> +	ISO_INT_EN_GPIODA,	/* Bit20 */
> +	ISO_INT_RVD,		/* Bit21 */
> +	ISO_INT_RVD,		/* Bit22 */
> +	ISO_INT_RVD,		/* Bit23 */
> +	ISO_INT_RVD,		/* Bit24 */
> +	ISO_INT_FAIL,		/* Bit25 */
> +	ISO_INT_FAIL,		/* Bit26 */
> +	ISO_INT_FAIL,		/* Bit27 */
> +	ISO_INT_FAIL,		/* Bit28 */
> +	ISO_INT_EN_GPHY_DV,	/* Bit29 */
> +	ISO_INT_EN_GPHY_AV,	/* Bit30 */
> +	ISO_INT_EN_I2C1_REQ	/* Bit31 */
> +};
> +
> +struct rtd129x_intc_data {
> +	void __iomem		*unmask;
> +	void __iomem		*isr;
> +	void __iomem		*ier;
> +	u32			ier_cached;
> +	u32			isr_en;
> +	raw_spinlock_t		lock;
> +	unsigned int		parent_irq;
> +	const unsigned char	*en_map;
> +};
> +
> +static struct irq_domain *rtd129x_intc_domain;
> +
> +static void rtd129x_intc_irq_handle(struct irq_desc *desc)
> +{
> +	struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int local_irq;
> +	u32 status;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	raw_spin_lock(&priv->lock);
> +	status = readl_relaxed(priv->isr);
> +	status &= priv->isr_en;
> +	raw_spin_unlock(&priv->lock);

What is this lock protecting? isr_en?

> +
> +	while (status) {
> +		i = __ffs(status);
> +		status &= ~BIT(i);
> +
> +		local_irq = irq_find_mapping(rtd129x_intc_domain, i);
> +		if (likely(local_irq)) {
> +			if (!generic_handle_irq(local_irq))
> +				writel_relaxed(BIT(i), priv->isr);

What are the write semantics of the ISR register? Hot bit clear? How
does it work since mask() does the same thing? Clearly, something is
wrong here.

> +		} else {
> +			handle_bad_irq(desc);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void rtd129x_intc_mask(struct irq_data *data)
> +{
> +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> +
> +	writel_relaxed(BIT(data->hwirq), priv->isr);
> +}
> +
> +static void rtd129x_intc_unmask(struct irq_data *data)
> +{
> +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> +
> +	writel_relaxed(BIT(data->hwirq), priv->unmask);

What effect does this have on the isr register? The whole mask/unmask
thing seems to be pretty dodgy...

> +}
> +
> +static void rtd129x_intc_enable(struct irq_data *data)
> +{
> +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u8 en_offset;
> +
> +	en_offset = priv->en_map[data->hwirq];
> +
> +	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> +		raw_spin_lock_irqsave(&priv->lock, flags);
> +
> +		priv->isr_en |= BIT(data->hwirq);
> +		priv->ier_cached |= BIT(en_offset);
> +		writel_relaxed(priv->ier_cached, priv->ier);
> +
> +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> +	} else if (en_offset == MISC_INT_FAIL) {
> +		pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> +	}
> +}
> +
> +static void rtd129x_intc_disable(struct irq_data *data)
> +{
> +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u8 en_offset;
> +
> +	en_offset = priv->en_map[data->hwirq];
> +
> +	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> +		raw_spin_lock_irqsave(&priv->lock, flags);
> +
> +		priv->isr_en &= ~BIT(data->hwirq);
> +		priv->ier_cached &= ~BIT(en_offset);
> +		writel_relaxed(priv->ier_cached, priv->ier);
> +
> +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> +	} else if (en_offset == MISC_INT_FAIL) {
> +		pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> +	}
> +}

So here's a thought: Why do we need all of this? If mask/unmask do their
job correctly, we could just enable all interrupts in one go (just a
32bit write) at probe time, and leave all interrupts masked until they
are in use. You could then drop all these silly tables that don't bring
much...

> +
> +static struct irq_chip rtd129x_intc_chip = {
> +	.name		= DEV_NAME,
> +	.irq_mask	= rtd129x_intc_mask,
> +	.irq_unmask	= rtd129x_intc_unmask,
> +	.irq_enable	= rtd129x_intc_enable,
> +	.irq_disable	= rtd129x_intc_disable,
> +};
> +
> +static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq,
> +			    irq_hw_number_t hw_irq)
> +{
> +	struct rtd129x_intc_data *priv = d->host_data;
> +
> +	irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq);
> +	irq_set_chip_data(virq, priv);
> +	irq_set_probe(virq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops rtd129x_intc_domain_ops = {
> +	.xlate			= irq_domain_xlate_onecell,
> +	.map			= rtd129x_intc_map,
> +};
> +
> +static const struct of_device_id rtd129x_intc_matches[] = {
> +	{ .compatible = "realtek,rtd129x-intc-misc",
> +		.data = rtd129x_intc_enable_map_misc
> +	},
> +	{ .compatible = "realtek,rtd129x-intc-iso",
> +		.data = rtd129x_intc_enable_map_iso
> +	},
> +	{ }
> +};
> +
> +static int rtd129x_intc_of_init(struct device_node *node,
> +				struct device_node *parent)
> +{
> +	struct rtd129x_intc_data *priv;
> +	const struct of_device_id *match;
> +	u32 isr_tmp, ier_tmp, ier_bit;
> +	int ret, i;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	raw_spin_lock_init(&priv->lock);
> +
> +	priv->isr = of_iomap(node, 0);
> +	if (!priv->isr) {
> +		pr_err("unable to obtain status reg iomap address\n");
> +		ret = -ENOMEM;
> +		goto free_priv;
> +	}
> +
> +	priv->ier = of_iomap(node, 1);
> +	if (!priv->ier) {
> +		pr_err("unable to obtain enable reg iomap address\n");
> +		ret = -ENOMEM;
> +		goto iounmap_status;
> +	}
> +
> +	priv->unmask = of_iomap(node, 2);
> +	if (!priv->unmask) {
> +		pr_err("unable to obtain unmask reg iomap address\n");
> +		ret = -ENOMEM;
> +		goto iounmap_enable;
> +	}
> +
> +	priv->parent_irq = irq_of_parse_and_map(node, 0);
> +	if (!priv->parent_irq) {
> +		pr_err("failed to map parent interrupt %d\n", priv->parent_irq);
> +		ret = -EINVAL;
> +		goto iounmap_all;
> +	}
> +
> +	match = of_match_node(rtd129x_intc_matches, node);
> +	if (!match) {
> +		pr_err("failed to find matching node\n");
> +		ret = -ENODEV;
> +		goto iounmap_all;
> +	}
> +	priv->en_map = match->data;
> +
> +	// initialize enabled irq's map to its matching status bit in isr by
> +	// inverse walking the enable to status offsets map. Only needed for
> +	// misc

Why do we need any of this? The kernel is supposed to start from a clean
slate, not to inherit whatever has been set before, unless there is a
very compelling reason.

> +	priv->ier_cached = readl_relaxed(priv->ier);
> +	if (priv->en_map == rtd129x_intc_enable_map_misc) {
> +		ier_tmp = priv->ier_cached;
> +		isr_tmp = 0;
> +		while (ier_tmp) {
> +			ier_bit = __ffs(ier_tmp);
> +			ier_tmp &= ~BIT(ier_bit);
> +			for (i = 0; i < RTD129X_INTC_NR_IRQS; i++)
> +				if (priv->en_map[i] == ier_bit)
> +					isr_tmp |= BIT(i);
> +		}
> +		priv->isr_en = isr_tmp;
> +	} else {
> +		priv->isr_en = priv->ier_cached;
> +	}
> +
> +	rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS,
> +						    &rtd129x_intc_domain_ops,
> +						    priv);
> +	if (!rtd129x_intc_domain) {
> +		pr_err("failed to create irq domain\n");
> +		ret = -ENOMEM;
> +		goto iounmap_all;
> +	}
> +
> +	irq_set_chained_handler_and_data(priv->parent_irq,
> +					 rtd129x_intc_irq_handle, priv);
> +
> +	return 0;
> +
> +iounmap_all:
> +	iounmap(priv->unmask);
> +iounmap_enable:
> +	iounmap(priv->ier);
> +iounmap_status:
> +	iounmap(priv->isr);
> +free_priv:
> +	kfree(priv);
> +err:
> +	return ret;
> +}
> +
> +IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc",
> +		rtd129x_intc_of_init);
> +IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso",
> +		rtd129x_intc_of_init);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[Aleix Roca Nonell]

Hi Mark and everyone! Sorry for the large delay, I'm doing this in my
free time, which is not that abundant. In this mail, I'm focusing only
on the largest change mentioned by Mark. I will answer the rest later.

On Mon, Jul 08, 2019 at 10:36:14AM +0100, Marc Zyngier wrote:
> On 07/07/2019 14:22, Aleix Roca Nonell wrote:
> > This driver adds support for the RTD1296 and RTD1295 interrupt
> > controller (intc). It is based on both the BPI-SINOVOIP project and
> > Andreas Färber's previous attempt to submit a similar driver.
> > 
> > There is currently no publicly available datasheet on this SoC and the
> > exact behaviour of the registers controlling the intc remain uncertain.
> > 
> > This driver controls two intcs: "iso" and "misc". Each intc has its own
> > Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR).
> 
> Register
> 
> > However, not all "misc" intc irqs have the same offsets for both ISR and
> > IER. For this reason an ISR to IER offsets table is defined.
> > 
> > The driver catches the IER value to reduce accesses to the table inside the
> > interrupt handler. Actually, the driver stores the ISR offsets of currently
> > enabled interrupts in a variable.
> > 
> > Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com>
> 
> I expect Andreas and you to sort the attribution issue. I'm certainly
> not going to take this in if things are unclear.
> 
> > ---
> >  drivers/irqchip/Makefile      |   1 +
> >  drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 372 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-rtd129x.c
> > 
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 606a003a0000..0689c3956250 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> >  obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
> > +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd129x.o
> > diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c
> > new file mode 100644
> > index 000000000000..76358ca50f10
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-rtd129x.c
> > @@ -0,0 +1,371 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/irqchip.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/io.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/bits.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +
> > +#define RTD129X_INTC_NR_IRQS 32
> > +#define DEV_NAME "RTD1296_INTC"
> > +
> > +/*
> > + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso"
> > + * and "misc". Each intc has its own Interrupt Enable Register (IER) and
> > + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the
> > + * same offsets for both ISR and IER. For this reason an ISR to IER offsets
> > + * table is defined. Also, to reduce accesses to this table in the interrupt
> > + * handler, the driver stores the ISR offsets of currently enabled interrupts in
> > + * a variable.
> > + */
> > +
> > +enum misc_int_en {
> > +	MISC_INT_FAIL		= 0xFF,
> > +	MISC_INT_RVD		= 0xFE,
> > +	MISC_INT_EN_FAN		= 29,
> > +	MISC_INT_EN_I2C3	= 28,
> > +	MISC_INT_EN_GSPI	= 27,
> > +	MISC_INT_EN_I2C2	= 26,
> > +	MISC_INT_EN_SC0		= 24,
> > +	MISC_INT_EN_LSADC1	= 22,
> > +	MISC_INT_EN_LSADC0	= 21,
> > +	MISC_INT_EN_GPIODA	= 20,
> > +	MISC_INT_EN_GPIOA	= 19,
> > +	MISC_INT_EN_I2C4	= 15,
> > +	MISC_INT_EN_I2C5	= 14,
> > +	MISC_INT_EN_RTC_DATA	= 12,
> > +	MISC_INT_EN_RTC_HOUR	= 11,
> > +	MISC_INT_EN_RTC_MIN	= 10,
> > +	MISC_INT_EN_UR2		= 7,
> > +	MISC_INT_EN_UR2_TO	= 6,
> > +	MISC_INT_EN_UR1_TO	= 5,
> > +	MISC_INT_EN_UR1		= 3,
> > +};
> > +
> > +enum iso_int_en {
> > +	ISO_INT_FAIL		= 0xFF,
> > +	ISO_INT_RVD		= 0xFE,
> > +	ISO_INT_EN_I2C1_REQ	= 31,
> > +	ISO_INT_EN_GPHY_AV	= 30,
> > +	ISO_INT_EN_GPHY_DV	= 29,
> > +	ISO_INT_EN_GPIODA	= 20,
> > +	ISO_INT_EN_GPIOA	= 19,
> > +	ISO_INT_EN_RTC_ALARM	= 13,
> > +	ISO_INT_EN_RTC_HSEC	= 12,
> > +	ISO_INT_EN_I2C1		= 11,
> > +	ISO_INT_EN_I2C0		= 8,
> > +	ISO_INT_EN_IRDA		= 5,
> > +	ISO_INT_EN_UR0		= 2,
> > +};
> > +
> > +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = {
> > +	MISC_INT_FAIL,		/* Bit0 */
> > +	MISC_INT_FAIL,		/* Bit1 */
> > +	MISC_INT_RVD,		/* Bit2 */
> > +	MISC_INT_EN_UR1,	/* Bit3 */
> > +	MISC_INT_FAIL,		/* Bit4 */
> > +	MISC_INT_EN_UR1_TO,	/* Bit5 */
> > +	MISC_INT_RVD,		/* Bit6 */
> > +	MISC_INT_RVD,		/* Bit7 */
> > +	MISC_INT_EN_UR2,	/* Bit8 */
> > +	MISC_INT_RVD,		/* Bit9 */
> > +	MISC_INT_EN_RTC_MIN,	/* Bit10 */
> > +	MISC_INT_EN_RTC_HOUR,	/* Bit11 */
> > +	MISC_INT_EN_RTC_DATA,	/* Bit12 */
> > +	MISC_INT_EN_UR2_TO,	/* Bit13 */
> > +	MISC_INT_EN_I2C5,	/* Bit14 */
> > +	MISC_INT_EN_I2C4,	/* Bit15 */
> > +	MISC_INT_FAIL,		/* Bit16 */
> > +	MISC_INT_FAIL,		/* Bit17 */
> > +	MISC_INT_FAIL,		/* Bit18 */
> > +	MISC_INT_EN_GPIOA,	/* Bit19 */
> > +	MISC_INT_EN_GPIODA,	/* Bit20 */
> > +	MISC_INT_EN_LSADC0,	/* Bit21 */
> > +	MISC_INT_EN_LSADC1,	/* Bit22 */
> > +	MISC_INT_EN_I2C3,	/* Bit23 */
> > +	MISC_INT_EN_SC0,	/* Bit24 */
> > +	MISC_INT_FAIL,		/* Bit25 */
> > +	MISC_INT_EN_I2C2,	/* Bit26 */
> > +	MISC_INT_EN_GSPI,	/* Bit27 */
> > +	MISC_INT_FAIL,		/* Bit28 */
> > +	MISC_INT_EN_FAN,	/* Bit29 */
> > +	MISC_INT_FAIL,		/* Bit30 */
> > +	MISC_INT_FAIL		/* Bit31 */
> > +};
> > +
> > +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = {
> > +	ISO_INT_FAIL,		/* Bit0 */
> > +	ISO_INT_RVD,		/* Bit1 */
> > +	ISO_INT_EN_UR0,		/* Bit2 */
> > +	ISO_INT_FAIL,		/* Bit3 */
> > +	ISO_INT_FAIL,		/* Bit4 */
> > +	ISO_INT_EN_IRDA,	/* Bit5 */
> > +	ISO_INT_FAIL,		/* Bit6 */
> > +	ISO_INT_RVD,		/* Bit7 */
> > +	ISO_INT_EN_I2C0,	/* Bit8 */
> > +	ISO_INT_RVD,		/* Bit9 */
> > +	ISO_INT_FAIL,		/* Bit10 */
> > +	ISO_INT_EN_I2C1,	/* Bit11 */
> > +	ISO_INT_EN_RTC_HSEC,	/* Bit12 */
> > +	ISO_INT_EN_RTC_ALARM,	/* Bit13 */
> > +	ISO_INT_FAIL,		/* Bit14 */
> > +	ISO_INT_FAIL,		/* Bit15 */
> > +	ISO_INT_FAIL,		/* Bit16 */
> > +	ISO_INT_FAIL,		/* Bit17 */
> > +	ISO_INT_FAIL,		/* Bit18 */
> > +	ISO_INT_EN_GPIOA,	/* Bit19 */
> > +	ISO_INT_EN_GPIODA,	/* Bit20 */
> > +	ISO_INT_RVD,		/* Bit21 */
> > +	ISO_INT_RVD,		/* Bit22 */
> > +	ISO_INT_RVD,		/* Bit23 */
> > +	ISO_INT_RVD,		/* Bit24 */
> > +	ISO_INT_FAIL,		/* Bit25 */
> > +	ISO_INT_FAIL,		/* Bit26 */
> > +	ISO_INT_FAIL,		/* Bit27 */
> > +	ISO_INT_FAIL,		/* Bit28 */
> > +	ISO_INT_EN_GPHY_DV,	/* Bit29 */
> > +	ISO_INT_EN_GPHY_AV,	/* Bit30 */
> > +	ISO_INT_EN_I2C1_REQ	/* Bit31 */
> > +};
> > +
> > +struct rtd129x_intc_data {
> > +	void __iomem		*unmask;
> > +	void __iomem		*isr;
> > +	void __iomem		*ier;
> > +	u32			ier_cached;
> > +	u32			isr_en;
> > +	raw_spinlock_t		lock;
> > +	unsigned int		parent_irq;
> > +	const unsigned char	*en_map;
> > +};
> > +
> > +static struct irq_domain *rtd129x_intc_domain;
> > +
> > +static void rtd129x_intc_irq_handle(struct irq_desc *desc)
> > +{
> > +	struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	unsigned int local_irq;
> > +	u32 status;
> > +	int i;
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	raw_spin_lock(&priv->lock);
> > +	status = readl_relaxed(priv->isr);
> > +	status &= priv->isr_en;
> > +	raw_spin_unlock(&priv->lock);
> 
> What is this lock protecting? isr_en?
> 
> > +
> > +	while (status) {
> > +		i = __ffs(status);
> > +		status &= ~BIT(i);
> > +
> > +		local_irq = irq_find_mapping(rtd129x_intc_domain, i);
> > +		if (likely(local_irq)) {
> > +			if (!generic_handle_irq(local_irq))
> > +				writel_relaxed(BIT(i), priv->isr);
> 
> What are the write semantics of the ISR register? Hot bit clear? How
> does it work since mask() does the same thing? Clearly, something is
> wrong here.

Sorry but I have not been able to found the definition of "hot bit
clear", could you explain it? Anyways, you were right, apparently the
mask/unmask code were doing nothing useful. More on this below.

> 
> > +		} else {
> > +			handle_bad_irq(desc);
> > +		}
> > +	}
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void rtd129x_intc_mask(struct irq_data *data)
> > +{
> > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > +
> > +	writel_relaxed(BIT(data->hwirq), priv->isr);
> > +}
> > +
> > +static void rtd129x_intc_unmask(struct irq_data *data)
> > +{
> > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > +
> > +	writel_relaxed(BIT(data->hwirq), priv->unmask);
> 
> What effect does this have on the isr register? The whole mask/unmask
> thing seems to be pretty dodgy...
> 
> > +}
> > +
> > +static void rtd129x_intc_enable(struct irq_data *data)
> > +{
> > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > +	unsigned long flags;
> > +	u8 en_offset;
> > +
> > +	en_offset = priv->en_map[data->hwirq];
> > +
> > +	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> > +		raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > +		priv->isr_en |= BIT(data->hwirq);
> > +		priv->ier_cached |= BIT(en_offset);
> > +		writel_relaxed(priv->ier_cached, priv->ier);
> > +
> > +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> > +	} else if (en_offset == MISC_INT_FAIL) {
> > +		pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> > +	}
> > +}
> > +
> > +static void rtd129x_intc_disable(struct irq_data *data)
> > +{
> > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > +	unsigned long flags;
> > +	u8 en_offset;
> > +
> > +	en_offset = priv->en_map[data->hwirq];
> > +
> > +	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> > +		raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > +		priv->isr_en &= ~BIT(data->hwirq);
> > +		priv->ier_cached &= ~BIT(en_offset);
> > +		writel_relaxed(priv->ier_cached, priv->ier);
> > +
> > +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> > +	} else if (en_offset == MISC_INT_FAIL) {
> > +		pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> > +	}
> > +}
> 
> So here's a thought: Why do we need all of this? If mask/unmask do their
> job correctly, we could just enable all interrupts in one go (just a
> 32bit write) at probe time, and leave all interrupts masked until they
> are in use. You could then drop all these silly tables that don't bring
> much...

The idea of dropping all those tables look really good to me, that
would greatly simplify the code! I have been trying to mask all
interrupts on the probe function using the ISR register but while
doing so, I realized that it does not work. Writing to ISR does not
mask interrupts, apparently it only acknowledges them once they have
been triggered. On the scarse available documentation of this Soc I
cannot find a mask-like register. It seems interrupts are managed with
an ISR and an IER register. So it should be posible to use the enable
register to maks/unmask instead. These do work. However, that would
mean that we have to keep those ugly tables.

Nonetheless we might still be able to do something else. Please,
correct me if I'm wrong, but do we really need to mask/unamsk in this
scenario? This is the devised board layout:

           +------+       +----------+       +---------+
           |      |       |          |       |         |
           | UART |-------|2  INTC   |-------|c  GIC   |
           |      |  +----|1         |  +----|b        |
           +------+  | +--|0         |  | +--|a        |
                     | |  |          |  | |  |         |
                     | |  +----------+  | |  +---------+
                     |                  |

Once the UART generates an interrupt it passes through the line 2 of
the custom realtek interrupt contoller before reaching the GIC's line
"c". On the INTC interrupt handler, we call chained_irq_enter/exit
to mask/unmask the GIC's "c" line. Because all of this realtek INTC
interrupt lines (2,1,0,...) are muxed on the GIC's line "c", this
means that while on the INTC interrupt handler it is not possible to
send further interrupts on the CPU. Given that interrupts are masked
on the GIC, it seems safe to just remove INTC's mask/unmask functions.

Therefore, the only work that this INTC handler would needs to do is
to acknowledge the interrupt by writing to the ISR, which it could be
done in the respective irq_ack callback of struct irq_chip instead of
in the handler body.

I have implemented this solution and it seems to work. What do you
think? I'm missing something crucial?

> 
> > +
> > +static struct irq_chip rtd129x_intc_chip = {
> > +	.name		= DEV_NAME,
> > +	.irq_mask	= rtd129x_intc_mask,
> > +	.irq_unmask	= rtd129x_intc_unmask,
> > +	.irq_enable	= rtd129x_intc_enable,
> > +	.irq_disable	= rtd129x_intc_disable,
> > +};
> > +
> > +static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq,
> > +			    irq_hw_number_t hw_irq)
> > +{
> > +	struct rtd129x_intc_data *priv = d->host_data;
> > +
> > +	irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq);
> > +	irq_set_chip_data(virq, priv);
> > +	irq_set_probe(virq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops rtd129x_intc_domain_ops = {
> > +	.xlate			= irq_domain_xlate_onecell,
> > +	.map			= rtd129x_intc_map,
> > +};
> > +
> > +static const struct of_device_id rtd129x_intc_matches[] = {
> > +	{ .compatible = "realtek,rtd129x-intc-misc",
> > +		.data = rtd129x_intc_enable_map_misc
> > +	},
> > +	{ .compatible = "realtek,rtd129x-intc-iso",
> > +		.data = rtd129x_intc_enable_map_iso
> > +	},
> > +	{ }
> > +};
> > +
> > +static int rtd129x_intc_of_init(struct device_node *node,
> > +				struct device_node *parent)
> > +{
> > +	struct rtd129x_intc_data *priv;
> > +	const struct of_device_id *match;
> > +	u32 isr_tmp, ier_tmp, ier_bit;
> > +	int ret, i;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	raw_spin_lock_init(&priv->lock);
> > +
> > +	priv->isr = of_iomap(node, 0);
> > +	if (!priv->isr) {
> > +		pr_err("unable to obtain status reg iomap address\n");
> > +		ret = -ENOMEM;
> > +		goto free_priv;
> > +	}
> > +
> > +	priv->ier = of_iomap(node, 1);
> > +	if (!priv->ier) {
> > +		pr_err("unable to obtain enable reg iomap address\n");
> > +		ret = -ENOMEM;
> > +		goto iounmap_status;
> > +	}
> > +
> > +	priv->unmask = of_iomap(node, 2);
> > +	if (!priv->unmask) {
> > +		pr_err("unable to obtain unmask reg iomap address\n");
> > +		ret = -ENOMEM;
> > +		goto iounmap_enable;
> > +	}
> > +
> > +	priv->parent_irq = irq_of_parse_and_map(node, 0);
> > +	if (!priv->parent_irq) {
> > +		pr_err("failed to map parent interrupt %d\n", priv->parent_irq);
> > +		ret = -EINVAL;
> > +		goto iounmap_all;
> > +	}
> > +
> > +	match = of_match_node(rtd129x_intc_matches, node);
> > +	if (!match) {
> > +		pr_err("failed to find matching node\n");
> > +		ret = -ENODEV;
> > +		goto iounmap_all;
> > +	}
> > +	priv->en_map = match->data;
> > +
> > +	// initialize enabled irq's map to its matching status bit in isr by
> > +	// inverse walking the enable to status offsets map. Only needed for
> > +	// misc
> 
> Why do we need any of this? The kernel is supposed to start from a clean
> slate, not to inherit whatever has been set before, unless there is a
> very compelling reason.
> 
> > +	priv->ier_cached = readl_relaxed(priv->ier);
> > +	if (priv->en_map == rtd129x_intc_enable_map_misc) {
> > +		ier_tmp = priv->ier_cached;
> > +		isr_tmp = 0;
> > +		while (ier_tmp) {
> > +			ier_bit = __ffs(ier_tmp);
> > +			ier_tmp &= ~BIT(ier_bit);
> > +			for (i = 0; i < RTD129X_INTC_NR_IRQS; i++)
> > +				if (priv->en_map[i] == ier_bit)
> > +					isr_tmp |= BIT(i);
> > +		}
> > +		priv->isr_en = isr_tmp;
> > +	} else {
> > +		priv->isr_en = priv->ier_cached;
> > +	}
> > +
> > +	rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS,
> > +						    &rtd129x_intc_domain_ops,
> > +						    priv);
> > +	if (!rtd129x_intc_domain) {
> > +		pr_err("failed to create irq domain\n");
> > +		ret = -ENOMEM;
> > +		goto iounmap_all;
> > +	}
> > +
> > +	irq_set_chained_handler_and_data(priv->parent_irq,
> > +					 rtd129x_intc_irq_handle, priv);
> > +
> > +	return 0;
> > +
> > +iounmap_all:
> > +	iounmap(priv->unmask);
> > +iounmap_enable:
> > +	iounmap(priv->ier);
> > +iounmap_status:
> > +	iounmap(priv->isr);
> > +free_priv:
> > +	kfree(priv);
> > +err:
> > +	return ret;
> > +}
> > +
> > +IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc",
> > +		rtd129x_intc_of_init);
> > +IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso",
> > +		rtd129x_intc_of_init);
> > 
> 
> Thanks,

Thank you very much for your time!

> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver

[Marc Zyngier]

On Mon, 12 Aug 2019 09:26:48 +0100,
Aleix Roca Nonell <kernelrocks@gmail.com> wrote:
> 
> Hi Mark and everyone! Sorry for the large delay, I'm doing this in my
> free time, which is not that abundant. In this mail, I'm focusing only
> on the largest change mentioned by Mark. I will answer the rest later.
> 
> On Mon, Jul 08, 2019 at 10:36:14AM +0100, Marc Zyngier wrote:
> > On 07/07/2019 14:22, Aleix Roca Nonell wrote:
> > > This driver adds support for the RTD1296 and RTD1295 interrupt
> > > controller (intc). It is based on both the BPI-SINOVOIP project and
> > > Andreas Färber's previous attempt to submit a similar driver.
> > > 
> > > There is currently no publicly available datasheet on this SoC and the
> > > exact behaviour of the registers controlling the intc remain uncertain.
> > > 
> > > This driver controls two intcs: "iso" and "misc". Each intc has its own
> > > Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR).
> > 
> > Register
> > 
> > > However, not all "misc" intc irqs have the same offsets for both ISR and
> > > IER. For this reason an ISR to IER offsets table is defined.
> > > 
> > > The driver catches the IER value to reduce accesses to the table inside the
> > > interrupt handler. Actually, the driver stores the ISR offsets of currently
> > > enabled interrupts in a variable.
> > > 
> > > Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com>
> > 
> > I expect Andreas and you to sort the attribution issue. I'm certainly
> > not going to take this in if things are unclear.
> > 
> > > ---
> > >  drivers/irqchip/Makefile      |   1 +
> > >  drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++
> > >  2 files changed, 372 insertions(+)
> > >  create mode 100644 drivers/irqchip/irq-rtd129x.c
> > > 
> > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > index 606a003a0000..0689c3956250 100644
> > > --- a/drivers/irqchip/Makefile
> > > +++ b/drivers/irqchip/Makefile
> > > @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> > >  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> > >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> > >  obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
> > > +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd129x.o
> > > diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c
> > > new file mode 100644
> > > index 000000000000..76358ca50f10
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-rtd129x.c
> > > @@ -0,0 +1,371 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <linux/irqchip.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/irqdomain.h>
> > > +#include <linux/io.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/irqchip.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/irqchip/chained_irq.h>
> > > +
> > > +#define RTD129X_INTC_NR_IRQS 32
> > > +#define DEV_NAME "RTD1296_INTC"
> > > +
> > > +/*
> > > + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso"
> > > + * and "misc". Each intc has its own Interrupt Enable Register (IER) and
> > > + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the
> > > + * same offsets for both ISR and IER. For this reason an ISR to IER offsets
> > > + * table is defined. Also, to reduce accesses to this table in the interrupt
> > > + * handler, the driver stores the ISR offsets of currently enabled interrupts in
> > > + * a variable.
> > > + */
> > > +
> > > +enum misc_int_en {
> > > +	MISC_INT_FAIL		= 0xFF,
> > > +	MISC_INT_RVD		= 0xFE,
> > > +	MISC_INT_EN_FAN		= 29,
> > > +	MISC_INT_EN_I2C3	= 28,
> > > +	MISC_INT_EN_GSPI	= 27,
> > > +	MISC_INT_EN_I2C2	= 26,
> > > +	MISC_INT_EN_SC0		= 24,
> > > +	MISC_INT_EN_LSADC1	= 22,
> > > +	MISC_INT_EN_LSADC0	= 21,
> > > +	MISC_INT_EN_GPIODA	= 20,
> > > +	MISC_INT_EN_GPIOA	= 19,
> > > +	MISC_INT_EN_I2C4	= 15,
> > > +	MISC_INT_EN_I2C5	= 14,
> > > +	MISC_INT_EN_RTC_DATA	= 12,
> > > +	MISC_INT_EN_RTC_HOUR	= 11,
> > > +	MISC_INT_EN_RTC_MIN	= 10,
> > > +	MISC_INT_EN_UR2		= 7,
> > > +	MISC_INT_EN_UR2_TO	= 6,
> > > +	MISC_INT_EN_UR1_TO	= 5,
> > > +	MISC_INT_EN_UR1		= 3,
> > > +};
> > > +
> > > +enum iso_int_en {
> > > +	ISO_INT_FAIL		= 0xFF,
> > > +	ISO_INT_RVD		= 0xFE,
> > > +	ISO_INT_EN_I2C1_REQ	= 31,
> > > +	ISO_INT_EN_GPHY_AV	= 30,
> > > +	ISO_INT_EN_GPHY_DV	= 29,
> > > +	ISO_INT_EN_GPIODA	= 20,
> > > +	ISO_INT_EN_GPIOA	= 19,
> > > +	ISO_INT_EN_RTC_ALARM	= 13,
> > > +	ISO_INT_EN_RTC_HSEC	= 12,
> > > +	ISO_INT_EN_I2C1		= 11,
> > > +	ISO_INT_EN_I2C0		= 8,
> > > +	ISO_INT_EN_IRDA		= 5,
> > > +	ISO_INT_EN_UR0		= 2,
> > > +};
> > > +
> > > +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = {
> > > +	MISC_INT_FAIL,		/* Bit0 */
> > > +	MISC_INT_FAIL,		/* Bit1 */
> > > +	MISC_INT_RVD,		/* Bit2 */
> > > +	MISC_INT_EN_UR1,	/* Bit3 */
> > > +	MISC_INT_FAIL,		/* Bit4 */
> > > +	MISC_INT_EN_UR1_TO,	/* Bit5 */
> > > +	MISC_INT_RVD,		/* Bit6 */
> > > +	MISC_INT_RVD,		/* Bit7 */
> > > +	MISC_INT_EN_UR2,	/* Bit8 */
> > > +	MISC_INT_RVD,		/* Bit9 */
> > > +	MISC_INT_EN_RTC_MIN,	/* Bit10 */
> > > +	MISC_INT_EN_RTC_HOUR,	/* Bit11 */
> > > +	MISC_INT_EN_RTC_DATA,	/* Bit12 */
> > > +	MISC_INT_EN_UR2_TO,	/* Bit13 */
> > > +	MISC_INT_EN_I2C5,	/* Bit14 */
> > > +	MISC_INT_EN_I2C4,	/* Bit15 */
> > > +	MISC_INT_FAIL,		/* Bit16 */
> > > +	MISC_INT_FAIL,		/* Bit17 */
> > > +	MISC_INT_FAIL,		/* Bit18 */
> > > +	MISC_INT_EN_GPIOA,	/* Bit19 */
> > > +	MISC_INT_EN_GPIODA,	/* Bit20 */
> > > +	MISC_INT_EN_LSADC0,	/* Bit21 */
> > > +	MISC_INT_EN_LSADC1,	/* Bit22 */
> > > +	MISC_INT_EN_I2C3,	/* Bit23 */
> > > +	MISC_INT_EN_SC0,	/* Bit24 */
> > > +	MISC_INT_FAIL,		/* Bit25 */
> > > +	MISC_INT_EN_I2C2,	/* Bit26 */
> > > +	MISC_INT_EN_GSPI,	/* Bit27 */
> > > +	MISC_INT_FAIL,		/* Bit28 */
> > > +	MISC_INT_EN_FAN,	/* Bit29 */
> > > +	MISC_INT_FAIL,		/* Bit30 */
> > > +	MISC_INT_FAIL		/* Bit31 */
> > > +};
> > > +
> > > +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = {
> > > +	ISO_INT_FAIL,		/* Bit0 */
> > > +	ISO_INT_RVD,		/* Bit1 */
> > > +	ISO_INT_EN_UR0,		/* Bit2 */
> > > +	ISO_INT_FAIL,		/* Bit3 */
> > > +	ISO_INT_FAIL,		/* Bit4 */
> > > +	ISO_INT_EN_IRDA,	/* Bit5 */
> > > +	ISO_INT_FAIL,		/* Bit6 */
> > > +	ISO_INT_RVD,		/* Bit7 */
> > > +	ISO_INT_EN_I2C0,	/* Bit8 */
> > > +	ISO_INT_RVD,		/* Bit9 */
> > > +	ISO_INT_FAIL,		/* Bit10 */
> > > +	ISO_INT_EN_I2C1,	/* Bit11 */
> > > +	ISO_INT_EN_RTC_HSEC,	/* Bit12 */
> > > +	ISO_INT_EN_RTC_ALARM,	/* Bit13 */
> > > +	ISO_INT_FAIL,		/* Bit14 */
> > > +	ISO_INT_FAIL,		/* Bit15 */
> > > +	ISO_INT_FAIL,		/* Bit16 */
> > > +	ISO_INT_FAIL,		/* Bit17 */
> > > +	ISO_INT_FAIL,		/* Bit18 */
> > > +	ISO_INT_EN_GPIOA,	/* Bit19 */
> > > +	ISO_INT_EN_GPIODA,	/* Bit20 */
> > > +	ISO_INT_RVD,		/* Bit21 */
> > > +	ISO_INT_RVD,		/* Bit22 */
> > > +	ISO_INT_RVD,		/* Bit23 */
> > > +	ISO_INT_RVD,		/* Bit24 */
> > > +	ISO_INT_FAIL,		/* Bit25 */
> > > +	ISO_INT_FAIL,		/* Bit26 */
> > > +	ISO_INT_FAIL,		/* Bit27 */
> > > +	ISO_INT_FAIL,		/* Bit28 */
> > > +	ISO_INT_EN_GPHY_DV,	/* Bit29 */
> > > +	ISO_INT_EN_GPHY_AV,	/* Bit30 */
> > > +	ISO_INT_EN_I2C1_REQ	/* Bit31 */
> > > +};
> > > +
> > > +struct rtd129x_intc_data {
> > > +	void __iomem		*unmask;
> > > +	void __iomem		*isr;
> > > +	void __iomem		*ier;
> > > +	u32			ier_cached;
> > > +	u32			isr_en;
> > > +	raw_spinlock_t		lock;
> > > +	unsigned int		parent_irq;
> > > +	const unsigned char	*en_map;
> > > +};
> > > +
> > > +static struct irq_domain *rtd129x_intc_domain;
> > > +
> > > +static void rtd129x_intc_irq_handle(struct irq_desc *desc)
> > > +{
> > > +	struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc);
> > > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > > +	unsigned int local_irq;
> > > +	u32 status;
> > > +	int i;
> > > +
> > > +	chained_irq_enter(chip, desc);
> > > +
> > > +	raw_spin_lock(&priv->lock);
> > > +	status = readl_relaxed(priv->isr);
> > > +	status &= priv->isr_en;
> > > +	raw_spin_unlock(&priv->lock);
> > 
> > What is this lock protecting? isr_en?
> > 
> > > +
> > > +	while (status) {
> > > +		i = __ffs(status);
> > > +		status &= ~BIT(i);
> > > +
> > > +		local_irq = irq_find_mapping(rtd129x_intc_domain, i);
> > > +		if (likely(local_irq)) {
> > > +			if (!generic_handle_irq(local_irq))
> > > +				writel_relaxed(BIT(i), priv->isr);
> > 
> > What are the write semantics of the ISR register? Hot bit clear? How
> > does it work since mask() does the same thing? Clearly, something is
> > wrong here.
> 
> Sorry but I have not been able to found the definition of "hot bit
> clear", could you explain it? Anyways, you were right, apparently the
> mask/unmask code were doing nothing useful. More on this below.

A hot-bit clear (or set) is a register where to write the bits that
you want to clear (or set), leaving alone the bits that are written as
zero. For example:

REG = 0xFFFF
clear_reg(0x1001)
REG = 0x7FFE
set_reg(0x1000)
REG = 0xFFFE

It is extremely useful for registers that need to be accessed
concurrently (the GIC uses that a lot, for example).

> 
> > 
> > > +		} else {
> > > +			handle_bad_irq(desc);
> > > +		}
> > > +	}
> > > +
> > > +	chained_irq_exit(chip, desc);
> > > +}
> > > +
> > > +static void rtd129x_intc_mask(struct irq_data *data)
> > > +{
> > > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > > +
> > > +	writel_relaxed(BIT(data->hwirq), priv->isr);
> > > +}
> > > +
> > > +static void rtd129x_intc_unmask(struct irq_data *data)
> > > +{
> > > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > > +
> > > +	writel_relaxed(BIT(data->hwirq), priv->unmask);
> > 
> > What effect does this have on the isr register? The whole mask/unmask
> > thing seems to be pretty dodgy...
> > 
> > > +}
> > > +
> > > +static void rtd129x_intc_enable(struct irq_data *data)
> > > +{
> > > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > > +	unsigned long flags;
> > > +	u8 en_offset;
> > > +
> > > +	en_offset = priv->en_map[data->hwirq];
> > > +
> > > +	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> > > +		raw_spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > +		priv->isr_en |= BIT(data->hwirq);
> > > +		priv->ier_cached |= BIT(en_offset);
> > > +		writel_relaxed(priv->ier_cached, priv->ier);
> > > +
> > > +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> > > +	} else if (en_offset == MISC_INT_FAIL) {
> > > +		pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> > > +	}
> > > +}
> > > +
> > > +static void rtd129x_intc_disable(struct irq_data *data)
> > > +{
> > > +	struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > > +	unsigned long flags;
> > > +	u8 en_offset;
> > > +
> > > +	en_offset = priv->en_map[data->hwirq];
> > > +
> > > +	if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> > > +		raw_spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > +		priv->isr_en &= ~BIT(data->hwirq);
> > > +		priv->ier_cached &= ~BIT(en_offset);
> > > +		writel_relaxed(priv->ier_cached, priv->ier);
> > > +
> > > +		raw_spin_unlock_irqrestore(&priv->lock, flags);
> > > +	} else if (en_offset == MISC_INT_FAIL) {
> > > +		pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> > > +	}
> > > +}
> > 
> > So here's a thought: Why do we need all of this? If mask/unmask do their
> > job correctly, we could just enable all interrupts in one go (just a
> > 32bit write) at probe time, and leave all interrupts masked until they
> > are in use. You could then drop all these silly tables that don't bring
> > much...
> 
> The idea of dropping all those tables look really good to me, that
> would greatly simplify the code! I have been trying to mask all
> interrupts on the probe function using the ISR register but while
> doing so, I realized that it does not work. Writing to ISR does not
> mask interrupts, apparently it only acknowledges them once they have
> been triggered. On the scarse available documentation of this Soc I
> cannot find a mask-like register. It seems interrupts are managed with
> an ISR and an IER register. So it should be posible to use the enable
> register to maks/unmask instead. These do work. However, that would
> mean that we have to keep those ugly tables.
> 
> Nonetheless we might still be able to do something else. Please,
> correct me if I'm wrong, but do we really need to mask/unamsk in this
> scenario? This is the devised board layout:
> 
>            +------+       +----------+       +---------+
>            |      |       |          |       |         |
>            | UART |-------|2  INTC   |-------|c  GIC   |
>            |      |  +----|1         |  +----|b        |
>            +------+  | +--|0         |  | +--|a        |
>                      | |  |          |  | |  |         |
>                      | |  +----------+  | |  +---------+
>                      |                  |
> 
> Once the UART generates an interrupt it passes through the line 2 of
> the custom realtek interrupt contoller before reaching the GIC's line
> "c". On the INTC interrupt handler, we call chained_irq_enter/exit
> to mask/unmask the GIC's "c" line. Because all of this realtek INTC
> interrupt lines (2,1,0,...) are muxed on the GIC's line "c", this
> means that while on the INTC interrupt handler it is not possible to
> send further interrupts on the CPU. Given that interrupts are masked
> on the GIC, it seems safe to just remove INTC's mask/unmask functions.

No, that's not true. If you cannot mask an individual interrupt at the
INTC level, it means that the only way to stop a screaming interrupt
(because the endpoint has crashed or that the kernel doesn't have a
driver for it) is to disable the interrupt at the GIC level, killing
all users of the INTC. Also, because the core code doesn't really know
that the INTC is behind the GIC, it cannot do that automatically.

So if you get into that situation, your system is dead. Believe it or
not, that's not something I want to see. An irqchip driver without a
mask callback is a lose grenade, and the pin is in your pocket.

> Therefore, the only work that this INTC handler would needs to do is
> to acknowledge the interrupt by writing to the ISR, which it could be
> done in the respective irq_ack callback of struct irq_chip instead of
> in the handler body.
> 
> I have implemented this solution and it seems to work. What do you
> think? I'm missing something crucial?

See above. Your system is terribly unsafe. Now, I'm pretty sure the
Realtek folks could help you there. Or you could start trying to
reverse engineer the thing, which shouldn't really hard (try poking at
the registers next to the ones you already have).

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.