[PATCH v2 0/3] arm64: Realtek RTD1295 IRQ mux

[PATCH v2 0/3] arm64: Realtek RTD1295 IRQ mux

[Andreas Färber]

Hello,

This series adds two IRQ muxes for the Realtek RTD1295 SoC.

There being no public source code for RTD1295, the implementation is based on
register offsets seen in the vendor DT, split up into two separate nodes,
as well as code from QNAP's rtk119x GPL code dump.

v2 does various cleanups, including limiting the serial quirk to the iso mux.

More experimental patches at:
https://github.com/afaerber/linux/commits/rtd1295-next

Have a lot of fun!

Cheers,
Andreas

v1 -> v2:
* Rebased, avoiding dependency on reset series for DT nodes
* Don't forward set_affinity to GIC (Marc)
* Added more spinlocks (Marc)
* Code cleanups
* Investigated quirk
* Fixed spinlock initialization (Andrew)

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Roc He <hepeng@zidoo.tv>
Cc: 蒋丽琴 <jiang.liqin@geniatech.com>
Cc: devicetree@vger.kernel.org
Cc: Andrew Lunn <andrew@lunn.ch>

Andreas Färber (3):
  dt-bindings: interrupt-controller: Add Realtek RTD1295
  irqchip: Add Realtek RTD1295 mux driver
  arm64: dts: realtek: Add irq mux to RTD1295

 .../interrupt-controller/realtek,rtd119x-mux.txt   |  23 +++
 arch/arm64/boot/dts/realtek/rtd1295.dtsi           |  22 +++
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-rtd119x-mux.c                  | 204 +++++++++++++++++++++
 4 files changed, 250 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
 create mode 100644 drivers/irqchip/irq-rtd119x-mux.c

-- 
2.12.3

[PATCH v2 1/3] dt-bindings: interrupt-controller: Add Realtek RTD1295

[Andreas Färber]

Add binding for Realtek RTD1295 IRQ mux.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2:
 * Dropped reference to common interrupt.txt bindings (Rob)
 
 .../interrupt-controller/realtek,rtd119x-mux.txt   | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
new file mode 100644
index 000000000000..952e5c54a5fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
@@ -0,0 +1,23 @@
+Realtek RTD119x/129x IRQ Mux Controller
+=======================================
+
+Required properties:
+
+- compatible           :  Should be one of the following:
+                          - "realtek,rtd1295-irq-mux"
+                          - "realtek,rtd1295-iso-irq-mux"
+- reg                  :  Specifies base physical address and size of the registers.
+- interrupts           :  Specifies the interrupt line which is mux'ed.
+- interrupt-controller :  Presence indicates the node as interrupt controller.
+- #interrupt-cells     :  Shall be 1. See common bindings in interrupt.txt.
+
+
+Example:
+
+	interrupt-controller@98007000 {
+		compatible = "realtek,rtd1295-iso-irq-mux";
+		reg = <0x98007000 0x100>;
+		interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+	};
-- 
2.12.3

[PATCH v2 2/3] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

This irq mux driver is derived from the RTD1295 vendor DT and assumes a
linear mapping between intr_en and intr_status registers.
Code for RTD119x indicates this may not always be the case (i2c_3).

Based in part on QNAP's arch/arm/mach-rtk119x/rtk_irq_mux.c code.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2:
 * Renamed struct fields to avoid ambiguity (Marc)
 * Refactored offset lookup to avoid per-compatible init functions
 * Inserted white lines to clarify balanced locking (Marc)
 * Dropped forwarding of set_affinity to GIC (Marc)
 * Added spinlocks for consistency (Marc)
 * Limited initialization quirk to iso mux
 * Fixed spinlock initialization (Andrew)
 
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-rtd119x-mux.c | 204 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 205 insertions(+)
 create mode 100644 drivers/irqchip/irq-rtd119x-mux.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e88d856cc09c..46202a0b7d96 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd119x-mux.o
diff --git a/drivers/irqchip/irq-rtd119x-mux.c b/drivers/irqchip/irq-rtd119x-mux.c
new file mode 100644
index 000000000000..65d22e163bef
--- /dev/null
+++ b/drivers/irqchip/irq-rtd119x-mux.c
@@ -0,0 +1,204 @@
+/*
+ * Realtek RTD129x IRQ mux
+ *
+ * Copyright (c) 2017 Andreas Färber
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+
+struct rtd119x_irq_mux_info {
+	unsigned intr_status_offset;
+	unsigned intr_en_offset;
+};
+
+struct rtd119x_irq_mux_data {
+	void __iomem *intr_status;
+	void __iomem *intr_en;
+	int irq;
+	struct irq_domain *domain;
+	spinlock_t lock;
+};
+
+static void rtd119x_mux_irq_handle(struct irq_desc *desc)
+{
+	struct rtd119x_irq_mux_data *data = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 intr_en, intr_status, status;
+	int ret;
+
+	chained_irq_enter(chip, desc);
+
+	spin_lock(&data->lock);
+	intr_en     = readl(data->intr_en);
+	intr_status = readl(data->intr_status);
+	spin_unlock(&data->lock);
+
+	status = intr_status & intr_en;
+	if (status != 0) {
+		unsigned irq = __ffs(status);
+		ret = generic_handle_irq(irq_find_mapping(data->domain, irq));
+		if (ret == 0) {
+			spin_lock(&data->lock);
+
+			intr_status = readl(data->intr_status);
+			intr_status |= BIT(irq - 1);
+			writel(intr_status, data->intr_status);
+
+			spin_unlock(&data->lock);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void rtd119x_mux_mask_irq(struct irq_data *data)
+{
+	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
+	u32 intr_status;
+
+	spin_lock(&mux_data->lock);
+
+	intr_status = readl(mux_data->intr_status);
+	intr_status |= BIT(data->hwirq);
+	writel(intr_status, mux_data->intr_status);
+
+	spin_unlock(&mux_data->lock);
+}
+
+static void rtd119x_mux_unmask_irq(struct irq_data *data)
+{
+	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
+	u32 intr_en;
+
+	spin_lock(&mux_data->lock);
+
+	intr_en = readl(mux_data->intr_en);
+	intr_en |= BIT(data->hwirq);
+	writel(intr_en, mux_data->intr_en);
+
+	spin_unlock(&mux_data->lock);
+}
+
+static int rtd119x_mux_set_affinity(struct irq_data *d,
+			const struct cpumask *mask_val, bool force)
+{
+	/* Forwarding the affinity to the parent would affect all 32 interrupts. */
+	return -EINVAL;
+}
+
+static struct irq_chip rtd119x_mux_irq_chip = {
+	.name			= "rtd119x-mux",
+	.irq_mask		= rtd119x_mux_mask_irq,
+	.irq_unmask		= rtd119x_mux_unmask_irq,
+	.irq_set_affinity	= rtd119x_mux_set_affinity,
+};
+
+static int rtd119x_mux_irq_domain_map(struct irq_domain *d,
+		unsigned int irq, irq_hw_number_t hw)
+{
+	struct rtd119x_irq_mux_data *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &rtd119x_mux_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, data);
+	irq_set_probe(irq);
+
+	return 0;
+}
+
+static struct irq_domain_ops rtd119x_mux_irq_domain_ops = {
+	.xlate	= irq_domain_xlate_onecell,
+	.map	= rtd119x_mux_irq_domain_map,
+};
+
+static const struct rtd119x_irq_mux_info rtd1295_iso_irq_mux_info = {
+	.intr_status_offset	= 0x0,
+	.intr_en_offset		= 0x40,
+};
+
+static const struct rtd119x_irq_mux_info rtd1295_irq_mux_info = {
+	.intr_status_offset	= 0xc,
+	.intr_en_offset		= 0x80,
+};
+
+static const struct of_device_id rtd1295_irq_mux_dt_matches[] = {
+	{
+		.compatible = "realtek,rtd1295-iso-irq-mux",
+		.data = &rtd1295_iso_irq_mux_info,
+	}, {
+		.compatible = "realtek,rtd1295-irq-mux",
+		.data = &rtd1295_irq_mux_info,
+	}, {
+	}
+};
+
+static int __init rtd119x_irq_mux_init(struct device_node *node,
+				       struct device_node *parent)
+{
+	struct rtd119x_irq_mux_data *data;
+	const struct of_device_id *match;
+	const struct rtd119x_irq_mux_info *info;
+	void __iomem *base;
+	u32 val;
+
+	match = of_match_node(rtd1295_irq_mux_dt_matches, node);
+	if (!match)
+		return -EINVAL;
+
+	info = match->data;
+	if (!info)
+		return -EINVAL;
+
+	base = of_iomap(node, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->intr_status = base + info->intr_status_offset;
+	data->intr_en     = base + info->intr_en_offset;
+
+	data->irq = irq_of_parse_and_map(node, 0);
+	if (data->irq <= 0) {
+		kfree(data);
+		return -EINVAL;
+	}
+
+	spin_lock_init(&data->lock);
+
+	data->domain = irq_domain_add_linear(node, 32,
+				&rtd119x_mux_irq_domain_ops, data);
+	if (!data->domain) {
+		kfree(data);
+		return -ENOMEM;
+	}
+
+	if (of_device_is_compatible(node, "realtek,rtd1295-iso-irq-mux")) {
+		const int uart0_irq = 2;
+
+		spin_lock(&data->lock);
+
+		val = readl(data->intr_en);
+		val &= ~BIT(uart0_irq);
+		writel(val, data->intr_en);
+
+		writel(BIT(uart0_irq), data->intr_status);
+
+		spin_unlock(&data->lock);
+	}
+
+	irq_set_chained_handler_and_data(data->irq, rtd119x_mux_irq_handle, data);
+
+	return 0;
+}
+IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd119x_irq_mux_init);
+IRQCHIP_DECLARE(rtd1295_mux, "realtek,rtd1295-irq-mux", rtd119x_irq_mux_init);
-- 
2.12.3

[PATCH v2 3/3] arm64: dts: realtek: Add irq mux to RTD1295

[Andreas Färber]

Update UART nodes with interrupts.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v1 -> v2:
 * Rebased
 
 arch/arm64/boot/dts/realtek/rtd1295.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/realtek/rtd1295.dtsi b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
index c8b7bb642a9a..6918373e9b5c 100644
--- a/arch/arm64/boot/dts/realtek/rtd1295.dtsi
+++ b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
@@ -93,21 +93,41 @@
 		/* Exclude up to 2 GiB of RAM */
 		ranges = <0x80000000 0x80000000 0x80000000>;
 
+		iso_irq_mux: interrupt-controller@98007000 {
+			compatible = "realtek,rtd1295-iso-irq-mux";
+			reg = <0x98007000 0x100>;
+			interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		uart0: serial@98007800 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x98007800 0x400>;
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clock-frequency = <27000000>;
+			interrupt-parent = <&iso_irq_mux>;
+			interrupts = <2>;
 			status = "disabled";
 		};
 
+		irq_mux: interrupt-controller@9801b000 {
+			compatible = "realtek,rtd1295-irq-mux";
+			reg = <0x9801b000 0x100>;
+			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		uart1: serial@9801b200 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x9801b200 0x100>;
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clock-frequency = <432000000>;
+			interrupt-parent = <&irq_mux>;
+			interrupts = <3>, <5>;
 			status = "disabled";
 		};
 
@@ -117,6 +137,8 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clock-frequency = <432000000>;
+			interrupt-parent = <&irq_mux>;
+			interrupts = <8>, <13>;
 			status = "disabled";
 		};
 
-- 
2.12.3

Re: [PATCH v2 1/3] dt-bindings: interrupt-controller: Add Realtek RTD1295

[Rob Herring]

On Mon, Aug 28, 2017 at 12:53:42PM +0200, Andreas Färber wrote:
> Add binding for Realtek RTD1295 IRQ mux.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  v1 -> v2:
>  * Dropped reference to common interrupt.txt bindings (Rob)
>  
>  .../interrupt-controller/realtek,rtd119x-mux.txt   | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/3] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 28/08/17 11:53, Andreas Färber wrote:
> This irq mux driver is derived from the RTD1295 vendor DT and assumes a
> linear mapping between intr_en and intr_status registers.
> Code for RTD119x indicates this may not always be the case (i2c_3).
> 
> Based in part on QNAP's arch/arm/mach-rtk119x/rtk_irq_mux.c code.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  v1 -> v2:
>  * Renamed struct fields to avoid ambiguity (Marc)
>  * Refactored offset lookup to avoid per-compatible init functions
>  * Inserted white lines to clarify balanced locking (Marc)
>  * Dropped forwarding of set_affinity to GIC (Marc)
>  * Added spinlocks for consistency (Marc)
>  * Limited initialization quirk to iso mux
>  * Fixed spinlock initialization (Andrew)
>  
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-rtd119x-mux.c | 204 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 205 insertions(+)
>  create mode 100644 drivers/irqchip/irq-rtd119x-mux.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856cc09c..46202a0b7d96 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd119x-mux.o
> diff --git a/drivers/irqchip/irq-rtd119x-mux.c b/drivers/irqchip/irq-rtd119x-mux.c
> new file mode 100644
> index 000000000000..65d22e163bef
> --- /dev/null
> +++ b/drivers/irqchip/irq-rtd119x-mux.c
> @@ -0,0 +1,204 @@
> +/*
> + * Realtek RTD129x IRQ mux
> + *
> + * Copyright (c) 2017 Andreas Färber
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +
> +struct rtd119x_irq_mux_info {
> +	unsigned intr_status_offset;
> +	unsigned intr_en_offset;
> +};
> +
> +struct rtd119x_irq_mux_data {
> +	void __iomem *intr_status;
> +	void __iomem *intr_en;
> +	int irq;
> +	struct irq_domain *domain;
> +	spinlock_t lock;
> +};
> +
> +static void rtd119x_mux_irq_handle(struct irq_desc *desc)
> +{
> +	struct rtd119x_irq_mux_data *data = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	u32 intr_en, intr_status, status;
> +	int ret;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	spin_lock(&data->lock);
> +	intr_en     = readl(data->intr_en);

I think that all the MMIO accessors in this file can advantageously
turned into their _relaxed version (none of them require any barrier).

> +	intr_status = readl(data->intr_status);
> +	spin_unlock(&data->lock);
> +
> +	status = intr_status & intr_en;
> +	if (status != 0) {
> +		unsigned irq = __ffs(status);
> +		ret = generic_handle_irq(irq_find_mapping(data->domain, irq));
> +		if (ret == 0) {
> +			spin_lock(&data->lock);
> +
> +			intr_status = readl(data->intr_status);
> +			intr_status |= BIT(irq - 1);
> +			writel(intr_status, data->intr_status);

This sequence feels a bit wrong: It seems to imply that writing to the
status register is a way to EOI the interrupt. But what happens to the
other bits that you've read? I fear that you are inadvertently
signalling an EOI for interrupts that you may not have handled yet.

I'd rather see something like this:

	while (status) {
		irq = __ffs(status) - 1;
		writel_relaxed(BIT(irq), data->intr_status);
		generic_handle_irq(irq_find_mapping(data->domain, irq));
		status &= ~irq;
	}

assuming I've understood how the HW works. No need for additional locking.

> +
> +			spin_unlock(&data->lock);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void rtd119x_mux_mask_irq(struct irq_data *data)
> +{
> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
> +	u32 intr_status;
> +
> +	spin_lock(&mux_data->lock);

Bang, you're dead. If you get the chained interrupt firing here on the
same CPU, it will take the lock in the above function, and everything
will grind to a halt. Use the irqsave version.

> +
> +	intr_status = readl(mux_data->intr_status);
> +	intr_status |= BIT(data->hwirq);
> +	writel(intr_status, mux_data->intr_status);

Or maybe I haven't understood how this works at all. Can you please
explain? I'd expect masking to be the opposite of unmasking, but that's
not the case...

> +
> +	spin_unlock(&mux_data->lock);
> +}
> +
> +static void rtd119x_mux_unmask_irq(struct irq_data *data)
> +{
> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
> +	u32 intr_en;
> +
> +	spin_lock(&mux_data->lock);
> +

Same here.

> +	intr_en = readl(mux_data->intr_en);
> +	intr_en |= BIT(data->hwirq);
> +	writel(intr_en, mux_data->intr_en);
> +
> +	spin_unlock(&mux_data->lock);
> +}
> +
> +static int rtd119x_mux_set_affinity(struct irq_data *d,
> +			const struct cpumask *mask_val, bool force)
> +{
> +	/* Forwarding the affinity to the parent would affect all 32 interrupts. */
> +	return -EINVAL;
> +}
> +
> +static struct irq_chip rtd119x_mux_irq_chip = {
> +	.name			= "rtd119x-mux",
> +	.irq_mask		= rtd119x_mux_mask_irq,
> +	.irq_unmask		= rtd119x_mux_unmask_irq,
> +	.irq_set_affinity	= rtd119x_mux_set_affinity,
> +};
> +
> +static int rtd119x_mux_irq_domain_map(struct irq_domain *d,
> +		unsigned int irq, irq_hw_number_t hw)
> +{
> +	struct rtd119x_irq_mux_data *data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &rtd119x_mux_irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, data);
> +	irq_set_probe(irq);
> +
> +	return 0;
> +}
> +
> +static struct irq_domain_ops rtd119x_mux_irq_domain_ops = {
> +	.xlate	= irq_domain_xlate_onecell,
> +	.map	= rtd119x_mux_irq_domain_map,
> +};
> +
> +static const struct rtd119x_irq_mux_info rtd1295_iso_irq_mux_info = {
> +	.intr_status_offset	= 0x0,
> +	.intr_en_offset		= 0x40,
> +};
> +
> +static const struct rtd119x_irq_mux_info rtd1295_irq_mux_info = {
> +	.intr_status_offset	= 0xc,
> +	.intr_en_offset		= 0x80,
> +};
> +
> +static const struct of_device_id rtd1295_irq_mux_dt_matches[] = {
> +	{
> +		.compatible = "realtek,rtd1295-iso-irq-mux",
> +		.data = &rtd1295_iso_irq_mux_info,
> +	}, {
> +		.compatible = "realtek,rtd1295-irq-mux",
> +		.data = &rtd1295_irq_mux_info,
> +	}, {
> +	}
> +};
> +
> +static int __init rtd119x_irq_mux_init(struct device_node *node,
> +				       struct device_node *parent)
> +{
> +	struct rtd119x_irq_mux_data *data;
> +	const struct of_device_id *match;
> +	const struct rtd119x_irq_mux_info *info;
> +	void __iomem *base;
> +	u32 val;
> +
> +	match = of_match_node(rtd1295_irq_mux_dt_matches, node);
> +	if (!match)
> +		return -EINVAL;
> +
> +	info = match->data;
> +	if (!info)
> +		return -EINVAL;
> +
> +	base = of_iomap(node, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->intr_status = base + info->intr_status_offset;
> +	data->intr_en     = base + info->intr_en_offset;
> +
> +	data->irq = irq_of_parse_and_map(node, 0);
> +	if (data->irq <= 0) {
> +		kfree(data);
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_init(&data->lock);
> +
> +	data->domain = irq_domain_add_linear(node, 32,
> +				&rtd119x_mux_irq_domain_ops, data);
> +	if (!data->domain) {
> +		kfree(data);
> +		return -ENOMEM;
> +	}
> +
> +	if (of_device_is_compatible(node, "realtek,rtd1295-iso-irq-mux")) {
> +		const int uart0_irq = 2;
> +
> +		spin_lock(&data->lock);
> +
> +		val = readl(data->intr_en);
> +		val &= ~BIT(uart0_irq);
> +		writel(val, data->intr_en);
> +
> +		writel(BIT(uart0_irq), data->intr_status);

Same here. Can you please explain what you're trying to do? The locking
seems a bit pointless (nobody can request the interrupt yet), and this
uart0 needs at least a comment, and maybe a description in the device-tree.

> +
> +		spin_unlock(&data->lock);
> +	}
> +
> +	irq_set_chained_handler_and_data(data->irq, rtd119x_mux_irq_handle, data);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd119x_irq_mux_init);
> +IRQCHIP_DECLARE(rtd1295_mux, "realtek,rtd1295-irq-mux", rtd119x_irq_mux_init);
> 

Thanks,

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

Re: [PATCH v2 2/3] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Am 11.10.2017 um 19:35 schrieb Marc Zyngier:
> On 28/08/17 11:53, Andreas Färber wrote:
>> This irq mux driver is derived from the RTD1295 vendor DT and assumes a
>> linear mapping between intr_en and intr_status registers.
>> Code for RTD119x indicates this may not always be the case (i2c_3).
>>
>> Based in part on QNAP's arch/arm/mach-rtk119x/rtk_irq_mux.c code.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  v1 -> v2:
>>  * Renamed struct fields to avoid ambiguity (Marc)
>>  * Refactored offset lookup to avoid per-compatible init functions
>>  * Inserted white lines to clarify balanced locking (Marc)
>>  * Dropped forwarding of set_affinity to GIC (Marc)
>>  * Added spinlocks for consistency (Marc)
>>  * Limited initialization quirk to iso mux
>>  * Fixed spinlock initialization (Andrew)
>>  
>>  drivers/irqchip/Makefile          |   1 +
>>  drivers/irqchip/irq-rtd119x-mux.c | 204 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 205 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-rtd119x-mux.c
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index e88d856cc09c..46202a0b7d96 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>> +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd119x-mux.o
>> diff --git a/drivers/irqchip/irq-rtd119x-mux.c b/drivers/irqchip/irq-rtd119x-mux.c
>> new file mode 100644
>> index 000000000000..65d22e163bef
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-rtd119x-mux.c
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Realtek RTD129x IRQ mux
>> + *
>> + * Copyright (c) 2017 Andreas Färber
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/slab.h>
>> +
>> +struct rtd119x_irq_mux_info {
>> +	unsigned intr_status_offset;
>> +	unsigned intr_en_offset;
>> +};
>> +
>> +struct rtd119x_irq_mux_data {
>> +	void __iomem *intr_status;
>> +	void __iomem *intr_en;
>> +	int irq;
>> +	struct irq_domain *domain;
>> +	spinlock_t lock;
>> +};
>> +
>> +static void rtd119x_mux_irq_handle(struct irq_desc *desc)
>> +{
>> +	struct rtd119x_irq_mux_data *data = irq_desc_get_handler_data(desc);
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	u32 intr_en, intr_status, status;
>> +	int ret;
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	spin_lock(&data->lock);
>> +	intr_en     = readl(data->intr_en);
> 
> I think that all the MMIO accessors in this file can advantageously
> turned into their _relaxed version (none of them require any barrier).

Done, works okay.

>> +	intr_status = readl(data->intr_status);
>> +	spin_unlock(&data->lock);
>> +
>> +	status = intr_status & intr_en;
>> +	if (status != 0) {
>> +		unsigned irq = __ffs(status);
>> +		ret = generic_handle_irq(irq_find_mapping(data->domain, irq));
>> +		if (ret == 0) {
>> +			spin_lock(&data->lock);
>> +
>> +			intr_status = readl(data->intr_status);
>> +			intr_status |= BIT(irq - 1);
>> +			writel(intr_status, data->intr_status);
> 
> This sequence feels a bit wrong: It seems to imply that writing to the
> status register is a way to EOI the interrupt. But what happens to the
> other bits that you've read? I fear that you are inadvertently
> signalling an EOI for interrupts that you may not have handled yet.
> 
> I'd rather see something like this:
> 
> 	while (status) {
> 		irq = __ffs(status) - 1;
> 		writel_relaxed(BIT(irq), data->intr_status);
> 		generic_handle_irq(irq_find_mapping(data->domain, irq));
> 		status &= ~irq;
> 	}
> 
> assuming I've understood how the HW works. No need for additional locking.

I've adopted a similar loop from the RTD1296 code, which differs from
the old RTD1195 code, which was like the above.

Similarly, the UART does a simple write for ack but the RTD1195's
fallback code here did the read-modify-write cycle. RTD129x removed that
fallback apparently.

I've reworked the code to use register names found in RTD1195 headers,
which will hopefully clarify things as far as there is code. I did not
find any code actually using the *_UMSK_ISR register, only *_ISR and
*_SCPU_INT_EN.

>> +
>> +			spin_unlock(&data->lock);
>> +		}
>> +	}
>> +
>> +	chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void rtd119x_mux_mask_irq(struct irq_data *data)
>> +{
>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
>> +	u32 intr_status;
>> +
>> +	spin_lock(&mux_data->lock);
> 
> Bang, you're dead. If you get the chained interrupt firing here on the
> same CPU, it will take the lock in the above function, and everything
> will grind to a halt. Use the irqsave version.

Done.

> 
>> +
>> +	intr_status = readl(mux_data->intr_status);
>> +	intr_status |= BIT(data->hwirq);
>> +	writel(intr_status, mux_data->intr_status);
> 
> Or maybe I haven't understood how this works at all. Can you please
> explain? I'd expect masking to be the opposite of unmasking, but that's
> not the case...

Again, I have no documentation, just like you, so I cannot definitively
explain their hardware. Only downstream code and experiments. I've been
testing it with serial console and I²C access to PMIC.

>> +
>> +	spin_unlock(&mux_data->lock);
>> +}
>> +
>> +static void rtd119x_mux_unmask_irq(struct irq_data *data)
>> +{
>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
>> +	u32 intr_en;
>> +
>> +	spin_lock(&mux_data->lock);
>> +
> 
> Same here.
> 
>> +	intr_en = readl(mux_data->intr_en);
>> +	intr_en |= BIT(data->hwirq);
>> +	writel(intr_en, mux_data->intr_en);
>> +
>> +	spin_unlock(&mux_data->lock);
>> +}
>> +
>> +static int rtd119x_mux_set_affinity(struct irq_data *d,
>> +			const struct cpumask *mask_val, bool force)
>> +{
>> +	/* Forwarding the affinity to the parent would affect all 32 interrupts. */
>> +	return -EINVAL;
>> +}
>> +
>> +static struct irq_chip rtd119x_mux_irq_chip = {
>> +	.name			= "rtd119x-mux",
>> +	.irq_mask		= rtd119x_mux_mask_irq,
>> +	.irq_unmask		= rtd119x_mux_unmask_irq,
>> +	.irq_set_affinity	= rtd119x_mux_set_affinity,
>> +};
>> +
>> +static int rtd119x_mux_irq_domain_map(struct irq_domain *d,
>> +		unsigned int irq, irq_hw_number_t hw)
>> +{
>> +	struct rtd119x_irq_mux_data *data = d->host_data;
>> +
>> +	irq_set_chip_and_handler(irq, &rtd119x_mux_irq_chip, handle_level_irq);
>> +	irq_set_chip_data(irq, data);
>> +	irq_set_probe(irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct irq_domain_ops rtd119x_mux_irq_domain_ops = {
>> +	.xlate	= irq_domain_xlate_onecell,
>> +	.map	= rtd119x_mux_irq_domain_map,
>> +};
>> +
>> +static const struct rtd119x_irq_mux_info rtd1295_iso_irq_mux_info = {
>> +	.intr_status_offset	= 0x0,
>> +	.intr_en_offset		= 0x40,
>> +};
>> +
>> +static const struct rtd119x_irq_mux_info rtd1295_irq_mux_info = {
>> +	.intr_status_offset	= 0xc,
>> +	.intr_en_offset		= 0x80,
>> +};
>> +
>> +static const struct of_device_id rtd1295_irq_mux_dt_matches[] = {
>> +	{
>> +		.compatible = "realtek,rtd1295-iso-irq-mux",
>> +		.data = &rtd1295_iso_irq_mux_info,
>> +	}, {
>> +		.compatible = "realtek,rtd1295-irq-mux",
>> +		.data = &rtd1295_irq_mux_info,
>> +	}, {
>> +	}
>> +};
>> +
>> +static int __init rtd119x_irq_mux_init(struct device_node *node,
>> +				       struct device_node *parent)
>> +{
>> +	struct rtd119x_irq_mux_data *data;
>> +	const struct of_device_id *match;
>> +	const struct rtd119x_irq_mux_info *info;
>> +	void __iomem *base;
>> +	u32 val;
>> +
>> +	match = of_match_node(rtd1295_irq_mux_dt_matches, node);
>> +	if (!match)
>> +		return -EINVAL;
>> +
>> +	info = match->data;
>> +	if (!info)
>> +		return -EINVAL;
>> +
>> +	base = of_iomap(node, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->intr_status = base + info->intr_status_offset;
>> +	data->intr_en     = base + info->intr_en_offset;
>> +
>> +	data->irq = irq_of_parse_and_map(node, 0);
>> +	if (data->irq <= 0) {
>> +		kfree(data);
>> +		return -EINVAL;
>> +	}
>> +
>> +	spin_lock_init(&data->lock);
>> +
>> +	data->domain = irq_domain_add_linear(node, 32,
>> +				&rtd119x_mux_irq_domain_ops, data);
>> +	if (!data->domain) {
>> +		kfree(data);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (of_device_is_compatible(node, "realtek,rtd1295-iso-irq-mux")) {
>> +		const int uart0_irq = 2;
>> +
>> +		spin_lock(&data->lock);
>> +
>> +		val = readl(data->intr_en);
>> +		val &= ~BIT(uart0_irq);
>> +		writel(val, data->intr_en);
>> +
>> +		writel(BIT(uart0_irq), data->intr_status);
> 
> Same here. Can you please explain what you're trying to do? The locking
> seems a bit pointless (nobody can request the interrupt yet), and this
> uart0 needs at least a comment, and maybe a description in the device-tree.

Please see v1... downstream has it unconditionally for both iso and
misc. I'm adding a comment that this resolves a hang.
I can drop the lock, was locking all register accesses for consistency.

>> +
>> +		spin_unlock(&data->lock);
>> +	}
>> +
>> +	irq_set_chained_handler_and_data(data->irq, rtd119x_mux_irq_handle, data);
>> +
>> +	return 0;
>> +}
>> +IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd119x_irq_mux_init);
>> +IRQCHIP_DECLARE(rtd1295_mux, "realtek,rtd1295-irq-mux", rtd119x_irq_mux_init);

Thanks for getting back to this. Note that this driver is needed both
for UART and I²C, and no lock-ups have been observed so far. So let's
please try to tidy this up sufficiently for merging into 4.15, so that
we can flush some more of the 100+ patches on top.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)