[PATCH v4 0/8] ARM: Realtek RTD1195/RTD1295/RTD1395 IRQ mux

[PATCH v4 0/8] ARM: Realtek RTD1195/RTD1295/RTD1395 IRQ mux

[Andreas Färber]

Hello,

This series adds two IRQ muxes for the Realtek RTD1195, RTD1295 and RTD1395
SoC families.

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 and
Synology's RTD1293/96 GPL code dumps.

v3 does various cleanups, renames variables, reworks unmask vs. enable/disable
and adds an isr/scpu_int_en map as well as full RTD1195 support.

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

Have a lot of fun!

Cheers,
Andreas

v3 -> v4:
* Drop no-op .irq_set_affinity callback (Thomas)
* Clear all interrupts (James)
* Updated SPDX-License-identifier
* Use tabular formatting (Thomas)
* Adopt different braces style (Thomas)
* Use raw_spinlock_t (Thomas)
* Shortened callback name (Thomas)
* Fixed of_iomap() error handling
* Don't mask unmapped NMIs
* Cache SCPU_INT_EN (Thomas)
* Renamed binding and source files
* Dropped UART1/UART2 TO interrupts
* Expanded commit messages
* Added RTD1395 patches

v2 -> v3:
* Rebased, adding nodes to rtd129x.dtsi instead of rtd1295.dtsi
* Adopted {readl,writel}_relaxed() (Marc)
* Adopted spin_lock_irqsave() (Marc)
* Implemented RTD1195
* Implemented mapping for non-linear bits such as i2c3

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 <maz@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Aleix Roca Nonell <kernelrocks@gmail.com>
Cc: James Tai <james.tai@realtek.com>

Andreas Färber (8):
  dt-bindings: interrupt-controller: Add Realtek RTD1195/RTD1295 mux
  irqchip: Add Realtek RTD1295 mux driver
  arm64: dts: realtek: rtd129x: Add irq muxes and UART interrupts
  irqchip: rtd1195-mux: Add RTD1195 definitions
  ARM: dts: rtd1195: Add irq muxes and UART interrupts
  dt-bindings: interrupt-controller: rtd1195-mux: Add RTD1395
  irqchip: rtd1195-mux: Add RTD1395 definitions
  arm64: dts: realtek: rtd139x: Add irq muxes and UART interrupts

 .../interrupt-controller/realtek,rtd1195-mux.yaml  |  55 +++
 arch/arm/boot/dts/rtd1195.dtsi                     |  20 +
 arch/arm64/boot/dts/realtek/rtd129x.dtsi           |  22 +
 arch/arm64/boot/dts/realtek/rtd139x.dtsi           |  22 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-rtd1195-mux.c                  | 463 +++++++++++++++++++++
 6 files changed, 583 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml
 create mode 100644 drivers/irqchip/irq-rtd1195-mux.c

-- 
2.16.4

[PATCH v4 1/8] dt-bindings: interrupt-controller: Add Realtek RTD1195/RTD1295 mux

[Andreas Färber]

Add binding for Realtek RTD1295 and RTD1195 IRQ mux.

Acked-by: Rob Herring <robh@kernel.org>
[AF: Converted to YAML schema]
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v3 -> v4:
 * Squashed RTD1195
 * Converted to YAML schema
 * Renamed file from realtek,rtd119x-mux to realtek,rtd1195-mux
 
 v2 -> v3:
 * Renamed non-iso irq mux to "misc" for clarity

 v1 -> v2:
 * Dropped reference to common interrupt.txt bindings (Rob)
 
 .../interrupt-controller/realtek,rtd1195-mux.yaml  | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml
new file mode 100644
index 000000000000..5cf3a28cedba
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/realtek,rtd1195-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTD1195/1295 IRQ Mux Controller
+
+maintainers:
+  - Andreas Färber <afaerber@suse.de>
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd1195-misc-irq-mux
+      - realtek,rtd1195-iso-irq-mux
+      - realtek,rtd1295-misc-irq-mux
+      - realtek,rtd1295-iso-irq-mux
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: Specifies the interrupt line which is mux'ed.
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    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.16.4

[PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

This irq mux driver implements the RTD1295 SoC's non-linear mapping
between status and enable bits.

Based in part on QNAP's arch/arm/mach-rtk119x/rtk_irq_mux.c and
Synology's drivers/irqchip/irq-rtk.c code.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Aleix Roca Nonell <kernelrocks@gmail.com>
Signed-off-by: James Tai <james.tai@realtek.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v3 -> v4:
 * Drop no-op .irq_set_affinity callback (Thomas)
 * Clear all interrupts (James)
 * Updated SPDX-License-identifier
 * Use tabular formatting (Thomas)
 * Adopt different braces style (Thomas)
 * Use raw_spinlock_t (Thomas)
 * Shortened callback from isr_to_scpu_int_en_mask to isr_to_int_en_mask (Thomas)
 * Fixed of_iomap() error handling to not use IS_ERR()
 * Don't mask unmapped NMIs by checking for a non-zero mask
 * Cache SCPU_INT_EN to avoid superfluous reads (Thomas)
 * Renamed functions and variables from rtd119x to rtd1195
 
 v2 -> v3:
 * Adopted spin_lock_irq{save,restore}() (Marc)
 * Adopted single-write masking (Marc)
 * Adopted misc compatible string
 * Introduced explicit bit mapping
 * Adopted looped processing of pending interrupts (Marc)
 * Replaced unmask implementation with UMSK_ISR write
 * Introduced enable/disable ops and dropped no longer needed UART0 quirk
 
 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-rtd1195-mux.c | 283 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 284 insertions(+)
 create mode 100644 drivers/irqchip/irq-rtd1195-mux.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e806dda690ea..d678881eebc8 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -104,3 +104,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-rtd1195-mux.o
diff --git a/drivers/irqchip/irq-rtd1195-mux.c b/drivers/irqchip/irq-rtd1195-mux.c
new file mode 100644
index 000000000000..e6b08438b23c
--- /dev/null
+++ b/drivers/irqchip/irq-rtd1195-mux.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Realtek RTD1295 IRQ mux
+ *
+ * Copyright (c) 2017-2019 Andreas Färber
+ */
+
+#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 rtd1195_irq_mux_info {
+	unsigned int	isr_offset;
+	unsigned int	umsk_isr_offset;
+	unsigned int	scpu_int_en_offset;
+	const u32	*isr_to_int_en_mask;
+};
+
+struct rtd1195_irq_mux_data {
+	void __iomem				*reg_isr;
+	void __iomem				*reg_umsk_isr;
+	void __iomem				*reg_scpu_int_en;
+	const struct rtd1195_irq_mux_info	*info;
+	int					irq;
+	u32					scpu_int_en;
+	struct irq_domain			*domain;
+	raw_spinlock_t				lock;
+};
+
+static void rtd1195_mux_irq_handle(struct irq_desc *desc)
+{
+	struct rtd1195_irq_mux_data *data = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 isr, mask;
+	int i;
+
+	chained_irq_enter(chip, desc);
+
+	isr = readl_relaxed(data->reg_isr);
+
+	while (isr) {
+		i = __ffs(isr);
+		isr &= ~BIT(i);
+
+		mask = data->info->isr_to_int_en_mask[i];
+		if (mask && !(data->scpu_int_en & mask))
+			continue;
+
+		if (!generic_handle_irq(irq_find_mapping(data->domain, i)))
+			writel_relaxed(BIT(i), data->reg_isr);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void rtd1195_mux_mask_irq(struct irq_data *data)
+{
+	struct rtd1195_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
+
+	writel_relaxed(BIT(data->hwirq), mux_data->reg_isr);
+}
+
+static void rtd1195_mux_unmask_irq(struct irq_data *data)
+{
+	struct rtd1195_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
+
+	writel_relaxed(BIT(data->hwirq), mux_data->reg_umsk_isr);
+}
+
+static void rtd1195_mux_enable_irq(struct irq_data *data)
+{
+	struct rtd1195_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u32 mask;
+
+	mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
+	if (!mask)
+		return;
+
+	raw_spin_lock_irqsave(&mux_data->lock, flags);
+
+	mux_data->scpu_int_en |= mask;
+	writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en);
+
+	raw_spin_unlock_irqrestore(&mux_data->lock, flags);
+}
+
+static void rtd1195_mux_disable_irq(struct irq_data *data)
+{
+	struct rtd1195_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u32 mask;
+
+	mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
+	if (!mask)
+		return;
+
+	raw_spin_lock_irqsave(&mux_data->lock, flags);
+
+	mux_data->scpu_int_en &= ~mask;
+	writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en);
+
+	raw_spin_unlock_irqrestore(&mux_data->lock, flags);
+}
+
+static struct irq_chip rtd1195_mux_irq_chip = {
+	.name		= "rtd1195-mux",
+	.irq_mask	= rtd1195_mux_mask_irq,
+	.irq_unmask	= rtd1195_mux_unmask_irq,
+	.irq_enable	= rtd1195_mux_enable_irq,
+	.irq_disable	= rtd1195_mux_disable_irq,
+};
+
+static int rtd1195_mux_irq_domain_map(struct irq_domain *d,
+		unsigned int irq, irq_hw_number_t hw)
+{
+	struct rtd1195_irq_mux_data *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &rtd1195_mux_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, data);
+	irq_set_probe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops rtd1195_mux_irq_domain_ops = {
+	.xlate	= irq_domain_xlate_onecell,
+	.map	= rtd1195_mux_irq_domain_map,
+};
+
+enum rtd1295_iso_isr_bits {
+	RTD1295_ISO_ISR_UR0_SHIFT		= 2,
+	RTD1295_ISO_ISR_IRDA_SHIFT		= 5,
+	RTD1295_ISO_ISR_I2C0_SHIFT		= 8,
+	RTD1295_ISO_ISR_I2C1_SHIFT		= 11,
+	RTD1295_ISO_ISR_RTC_HSEC_SHIFT		= 12,
+	RTD1295_ISO_ISR_RTC_ALARM_SHIFT		= 13,
+	RTD1295_ISO_ISR_GPIOA_SHIFT		= 19,
+	RTD1295_ISO_ISR_GPIODA_SHIFT		= 20,
+	RTD1295_ISO_ISR_GPHY_DV_SHIFT		= 29,
+	RTD1295_ISO_ISR_GPHY_AV_SHIFT		= 30,
+	RTD1295_ISO_ISR_I2C1_REQ_SHIFT		= 31,
+};
+
+static const u32 rtd1295_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1295_ISO_ISR_UR0_SHIFT]		= BIT(2),
+	[RTD1295_ISO_ISR_IRDA_SHIFT]		= BIT(5),
+	[RTD1295_ISO_ISR_I2C0_SHIFT]		= BIT(8),
+	[RTD1295_ISO_ISR_I2C1_SHIFT]		= BIT(11),
+	[RTD1295_ISO_ISR_RTC_HSEC_SHIFT]	= BIT(12),
+	[RTD1295_ISO_ISR_RTC_ALARM_SHIFT]	= BIT(13),
+	[RTD1295_ISO_ISR_GPIOA_SHIFT]		= BIT(19),
+	[RTD1295_ISO_ISR_GPIODA_SHIFT]		= BIT(20),
+	[RTD1295_ISO_ISR_GPHY_DV_SHIFT]		= BIT(29),
+	[RTD1295_ISO_ISR_GPHY_AV_SHIFT]		= BIT(30),
+	[RTD1295_ISO_ISR_I2C1_REQ_SHIFT]	= BIT(31),
+};
+
+enum rtd1295_misc_isr_bits {
+	RTD1295_ISR_UR1_SHIFT			= 3,
+	RTD1295_ISR_UR1_TO_SHIFT		= 5,
+	RTD1295_ISR_UR2_SHIFT			= 8,
+	RTD1295_ISR_RTC_MIN_SHIFT		= 10,
+	RTD1295_ISR_RTC_HOUR_SHIFT		= 11,
+	RTD1295_ISR_RTC_DATA_SHIFT		= 12,
+	RTD1295_ISR_UR2_TO_SHIFT		= 13,
+	RTD1295_ISR_I2C5_SHIFT			= 14,
+	RTD1295_ISR_I2C4_SHIFT			= 15,
+	RTD1295_ISR_GPIOA_SHIFT			= 19,
+	RTD1295_ISR_GPIODA_SHIFT		= 20,
+	RTD1295_ISR_LSADC0_SHIFT		= 21,
+	RTD1295_ISR_LSADC1_SHIFT		= 22,
+	RTD1295_ISR_I2C3_SHIFT			= 23,
+	RTD1295_ISR_SC0_SHIFT			= 24,
+	RTD1295_ISR_I2C2_SHIFT			= 26,
+	RTD1295_ISR_GSPI_SHIFT			= 27,
+	RTD1295_ISR_FAN_SHIFT			= 29,
+};
+
+static const u32 rtd1295_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1295_ISR_UR1_SHIFT]			= BIT(3),
+	[RTD1295_ISR_UR1_TO_SHIFT]		= BIT(5),
+	[RTD1295_ISR_UR2_TO_SHIFT]		= BIT(6),
+	[RTD1295_ISR_UR2_SHIFT]			= BIT(7),
+	[RTD1295_ISR_RTC_MIN_SHIFT]		= BIT(10),
+	[RTD1295_ISR_RTC_HOUR_SHIFT]		= BIT(11),
+	[RTD1295_ISR_RTC_DATA_SHIFT]		= BIT(12),
+	[RTD1295_ISR_I2C5_SHIFT]		= BIT(14),
+	[RTD1295_ISR_I2C4_SHIFT]		= BIT(15),
+	[RTD1295_ISR_GPIOA_SHIFT]		= BIT(19),
+	[RTD1295_ISR_GPIODA_SHIFT]		= BIT(20),
+	[RTD1295_ISR_LSADC0_SHIFT]		= BIT(21),
+	[RTD1295_ISR_LSADC1_SHIFT]		= BIT(22),
+	[RTD1295_ISR_SC0_SHIFT]			= BIT(24),
+	[RTD1295_ISR_I2C2_SHIFT]		= BIT(26),
+	[RTD1295_ISR_GSPI_SHIFT]		= BIT(27),
+	[RTD1295_ISR_I2C3_SHIFT]		= BIT(28),
+	[RTD1295_ISR_FAN_SHIFT]			= BIT(29),
+};
+
+static const struct rtd1195_irq_mux_info rtd1295_iso_irq_mux_info = {
+	.isr_offset		= 0x0,
+	.umsk_isr_offset	= 0x4,
+	.scpu_int_en_offset	= 0x40,
+	.isr_to_int_en_mask	= rtd1295_iso_isr_to_scpu_int_en_mask,
+};
+
+static const struct rtd1195_irq_mux_info rtd1295_misc_irq_mux_info = {
+	.umsk_isr_offset	= 0x8,
+	.isr_offset		= 0xc,
+	.scpu_int_en_offset	= 0x80,
+	.isr_to_int_en_mask	= rtd1295_misc_isr_to_scpu_int_en_mask,
+};
+
+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-misc-irq-mux",
+		.data = &rtd1295_misc_irq_mux_info,
+	},
+	{
+	}
+};
+
+static int __init rtd1195_irq_mux_init(struct device_node *node,
+				       struct device_node *parent)
+{
+	struct rtd1195_irq_mux_data *data;
+	const struct of_device_id *match;
+	const struct rtd1195_irq_mux_info *info;
+	void __iomem *base;
+
+	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 (!base)
+		return -EIO;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->info		= info;
+	data->reg_isr		= base + info->isr_offset;
+	data->reg_umsk_isr	= base + info->umsk_isr_offset;
+	data->reg_scpu_int_en	= base + info->scpu_int_en_offset;
+
+	data->irq = irq_of_parse_and_map(node, 0);
+	if (data->irq <= 0) {
+		kfree(data);
+		return -EINVAL;
+	}
+
+	raw_spin_lock_init(&data->lock);
+
+	writel_relaxed(data->scpu_int_en, data->reg_scpu_int_en);
+
+	data->domain = irq_domain_add_linear(node, 32,
+				&rtd1195_mux_irq_domain_ops, data);
+	if (!data->domain) {
+		kfree(data);
+		return -ENOMEM;
+	}
+
+	irq_set_chained_handler_and_data(data->irq, rtd1195_mux_irq_handle, data);
+
+	return 0;
+}
+IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd1195_irq_mux_init);
+IRQCHIP_DECLARE(rtd1295_misc_mux, "realtek,rtd1295-misc-irq-mux", rtd1195_irq_mux_init);
-- 
2.16.4

[PATCH v4 3/8] arm64: dts: realtek: rtd129x: Add irq muxes and UART interrupts

[Andreas Färber]

Add iso and misc IRQ mux DT nodes to RTD129x SoC family.

Update the UART DT nodes with interrupts from these muxes,
so that UART0 can be used without earlycon.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v3 -> v4:
 * Rebased onto chip-info and r-bus
 * Dropped schema-violating second interrupts for UART1 and UART2
 
 v2 -> v3:
 * Added nodes to rtd129x.dtsi instead of rtd1295.dtsi
 * Adopted misc compatible string
 * Renamed node label from irq_mux to misc_irq_mux for clarity
 
 v1 -> v2:
 * Rebased
 
 arch/arm64/boot/dts/realtek/rtd129x.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/realtek/rtd129x.dtsi b/arch/arm64/boot/dts/realtek/rtd129x.dtsi
index 7d56c9f5d48a..188b4f256917 100644
--- a/arch/arm64/boot/dts/realtek/rtd129x.dtsi
+++ b/arch/arm64/boot/dts/realtek/rtd129x.dtsi
@@ -86,6 +86,14 @@
 				#reset-cells = <1>;
 			};
 
+			iso_irq_mux: interrupt-controller@7000 {
+				compatible = "realtek,rtd1295-iso-irq-mux";
+				reg = <0x7000 0x100>;
+				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
 			iso_reset: reset-controller@7088 {
 				compatible = "snps,dw-low-reset";
 				reg = <0x7088 0x4>;
@@ -105,6 +113,8 @@
 				reg-io-width = <4>;
 				clock-frequency = <27000000>;
 				resets = <&iso_reset RTD1295_ISO_RSTN_UR0>;
+				interrupt-parent = <&iso_irq_mux>;
+				interrupts = <2>;
 				status = "disabled";
 			};
 
@@ -115,6 +125,14 @@
 				      <0x171d8 0x4>;
 			};
 
+			misc_irq_mux: interrupt-controller@1b000 {
+				compatible = "realtek,rtd1295-misc-irq-mux";
+				reg = <0x1b000 0x100>;
+				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
 			uart1: serial@1b200 {
 				compatible = "snps,dw-apb-uart";
 				reg = <0x1b200 0x100>;
@@ -122,6 +140,8 @@
 				reg-io-width = <4>;
 				clock-frequency = <432000000>;
 				resets = <&reset2 RTD1295_RSTN_UR1>;
+				interrupt-parent = <&misc_irq_mux>;
+				interrupts = <3>;
 				status = "disabled";
 			};
 
@@ -132,6 +152,8 @@
 				reg-io-width = <4>;
 				clock-frequency = <432000000>;
 				resets = <&reset2 RTD1295_RSTN_UR2>;
+				interrupt-parent = <&misc_irq_mux>;
+				interrupts = <8>;
 				status = "disabled";
 			};
 		};
-- 
2.16.4

[PATCH v4 4/8] irqchip: rtd1195-mux: Add RTD1195 definitions

[Andreas Färber]

Add compatible strings and bit mappings for Realtek RTD1195 SoC.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v3 -> v4:
 * Use tabular formatting (Thomas)
 * Adopt different braces style (Thomas)
 * Updated with shortened isr_to_int_en_mask callback name (Thomas)
 * Renamed functions and variables from rtd119x_ to rtd1195_
 * Renamed enum values from RTD119X_ to RTD1195_
 
 v3: New
 
 drivers/irqchip/irq-rtd1195-mux.c | 101 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-rtd1195-mux.c b/drivers/irqchip/irq-rtd1195-mux.c
index e6b08438b23c..765d72653383 100644
--- a/drivers/irqchip/irq-rtd1195-mux.c
+++ b/drivers/irqchip/irq-rtd1195-mux.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Realtek RTD1295 IRQ mux
+ * Realtek RTD1195/RTD1295 IRQ mux
  *
  * Copyright (c) 2017-2019 Andreas Färber
  */
@@ -132,6 +132,81 @@ static const struct irq_domain_ops rtd1195_mux_irq_domain_ops = {
 	.map	= rtd1195_mux_irq_domain_map,
 };
 
+enum rtd1195_iso_isr_bits {
+	RTD1195_ISO_ISR_TC3_SHIFT		= 1,
+	RTD1195_ISO_ISR_UR0_SHIFT		= 2,
+	RTD1195_ISO_ISR_IRDA_SHIFT		= 5,
+	RTD1195_ISO_ISR_WDOG_NMI_SHIFT		= 7,
+	RTD1195_ISO_ISR_I2C0_SHIFT		= 8,
+	RTD1195_ISO_ISR_TC4_SHIFT		= 9,
+	RTD1195_ISO_ISR_I2C6_SHIFT		= 10,
+	RTD1195_ISO_ISR_RTC_HSEC_SHIFT		= 12,
+	RTD1195_ISO_ISR_RTC_ALARM_SHIFT		= 13,
+	RTD1195_ISO_ISR_VFD_WDONE_SHIFT		= 14,
+	RTD1195_ISO_ISR_VFD_ARDKPADA_SHIFT	= 15,
+	RTD1195_ISO_ISR_VFD_ARDKPADDA_SHIFT	= 16,
+	RTD1195_ISO_ISR_VFD_ARDSWA_SHIFT	= 17,
+	RTD1195_ISO_ISR_VFD_ARDSWDA_SHIFT	= 18,
+	RTD1195_ISO_ISR_GPIOA_SHIFT		= 19,
+	RTD1195_ISO_ISR_GPIODA_SHIFT		= 20,
+	RTD1195_ISO_ISR_CEC_SHIFT		= 22,
+};
+
+static const u32 rtd1195_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1195_ISO_ISR_UR0_SHIFT]		= BIT(2),
+	[RTD1195_ISO_ISR_IRDA_SHIFT]		= BIT(5),
+	[RTD1195_ISO_ISR_I2C0_SHIFT]		= BIT(8),
+	[RTD1195_ISO_ISR_I2C6_SHIFT]		= BIT(10),
+	[RTD1195_ISO_ISR_RTC_HSEC_SHIFT]	= BIT(12),
+	[RTD1195_ISO_ISR_RTC_ALARM_SHIFT]	= BIT(13),
+	[RTD1195_ISO_ISR_VFD_WDONE_SHIFT]	= BIT(14),
+	[RTD1195_ISO_ISR_VFD_ARDKPADA_SHIFT]	= BIT(15),
+	[RTD1195_ISO_ISR_VFD_ARDKPADDA_SHIFT]	= BIT(16),
+	[RTD1195_ISO_ISR_VFD_ARDSWA_SHIFT]	= BIT(17),
+	[RTD1195_ISO_ISR_VFD_ARDSWDA_SHIFT]	= BIT(18),
+	[RTD1195_ISO_ISR_GPIOA_SHIFT]		= BIT(19),
+	[RTD1195_ISO_ISR_GPIODA_SHIFT]		= BIT(20),
+	[RTD1195_ISO_ISR_CEC_SHIFT]		= BIT(22),
+};
+
+enum rtd1195_misc_isr_bits {
+	RTD1195_MIS_ISR_WDOG_NMI_SHIFT		= 2,
+	RTD1195_MIS_ISR_UR1_SHIFT		= 3,
+	RTD1195_MIS_ISR_I2C1_SHIFT		= 4,
+	RTD1195_MIS_ISR_UR1_TO_SHIFT		= 5,
+	RTD1195_MIS_ISR_TC0_SHIFT		= 6,
+	RTD1195_MIS_ISR_TC1_SHIFT		= 7,
+	RTD1195_MIS_ISR_RTC_HSEC_SHIFT		= 9,
+	RTD1195_MIS_ISR_RTC_MIN_SHIFT		= 10,
+	RTD1195_MIS_ISR_RTC_HOUR_SHIFT		= 11,
+	RTD1195_MIS_ISR_RTC_DATE_SHIFT		= 12,
+	RTD1195_MIS_ISR_I2C5_SHIFT		= 14,
+	RTD1195_MIS_ISR_I2C4_SHIFT		= 15,
+	RTD1195_MIS_ISR_GPIOA_SHIFT		= 19,
+	RTD1195_MIS_ISR_GPIODA_SHIFT		= 20,
+	RTD1195_MIS_ISR_LSADC_SHIFT		= 21,
+	RTD1195_MIS_ISR_I2C3_SHIFT		= 23,
+	RTD1195_MIS_ISR_I2C2_SHIFT		= 26,
+	RTD1195_MIS_ISR_GSPI_SHIFT		= 27,
+};
+
+static const u32 rtd1195_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1195_MIS_ISR_UR1_SHIFT]		= BIT(3),
+	[RTD1195_MIS_ISR_I2C1_SHIFT]		= BIT(4),
+	[RTD1195_MIS_ISR_UR1_TO_SHIFT]		= BIT(5),
+	[RTD1195_MIS_ISR_RTC_MIN_SHIFT]		= BIT(10),
+	[RTD1195_MIS_ISR_RTC_HOUR_SHIFT]	= BIT(11),
+	[RTD1195_MIS_ISR_RTC_DATE_SHIFT]	= BIT(12),
+	[RTD1195_MIS_ISR_I2C5_SHIFT]		= BIT(14),
+	[RTD1195_MIS_ISR_I2C4_SHIFT]		= BIT(15),
+	[RTD1195_MIS_ISR_GPIOA_SHIFT]		= BIT(19),
+	[RTD1195_MIS_ISR_GPIODA_SHIFT]		= BIT(20),
+	[RTD1195_MIS_ISR_LSADC_SHIFT]		= BIT(21),
+	[RTD1195_MIS_ISR_I2C2_SHIFT]		= BIT(26),
+	[RTD1195_MIS_ISR_GSPI_SHIFT]		= BIT(27),
+	[RTD1195_MIS_ISR_I2C3_SHIFT]		= BIT(28),
+};
+
 enum rtd1295_iso_isr_bits {
 	RTD1295_ISO_ISR_UR0_SHIFT		= 2,
 	RTD1295_ISO_ISR_IRDA_SHIFT		= 5,
@@ -202,6 +277,13 @@ static const u32 rtd1295_misc_isr_to_scpu_int_en_mask[32] = {
 	[RTD1295_ISR_FAN_SHIFT]			= BIT(29),
 };
 
+static const struct rtd1195_irq_mux_info rtd1195_iso_irq_mux_info = {
+	.isr_offset		= 0x0,
+	.umsk_isr_offset	= 0x4,
+	.scpu_int_en_offset	= 0x40,
+	.isr_to_int_en_mask	= rtd1195_iso_isr_to_scpu_int_en_mask,
+};
+
 static const struct rtd1195_irq_mux_info rtd1295_iso_irq_mux_info = {
 	.isr_offset		= 0x0,
 	.umsk_isr_offset	= 0x4,
@@ -209,6 +291,13 @@ static const struct rtd1195_irq_mux_info rtd1295_iso_irq_mux_info = {
 	.isr_to_int_en_mask	= rtd1295_iso_isr_to_scpu_int_en_mask,
 };
 
+static const struct rtd1195_irq_mux_info rtd1195_misc_irq_mux_info = {
+	.umsk_isr_offset	= 0x8,
+	.isr_offset		= 0xc,
+	.scpu_int_en_offset	= 0x80,
+	.isr_to_int_en_mask	= rtd1195_misc_isr_to_scpu_int_en_mask,
+};
+
 static const struct rtd1195_irq_mux_info rtd1295_misc_irq_mux_info = {
 	.umsk_isr_offset	= 0x8,
 	.isr_offset		= 0xc,
@@ -217,10 +306,18 @@ static const struct rtd1195_irq_mux_info rtd1295_misc_irq_mux_info = {
 };
 
 static const struct of_device_id rtd1295_irq_mux_dt_matches[] = {
+	{
+		.compatible = "realtek,rtd1195-iso-irq-mux",
+		.data = &rtd1195_iso_irq_mux_info,
+	},
 	{
 		.compatible = "realtek,rtd1295-iso-irq-mux",
 		.data = &rtd1295_iso_irq_mux_info,
 	},
+	{
+		.compatible = "realtek,rtd1195-misc-irq-mux",
+		.data = &rtd1195_misc_irq_mux_info,
+	},
 	{
 		.compatible = "realtek,rtd1295-misc-irq-mux",
 		.data = &rtd1295_misc_irq_mux_info,
@@ -279,5 +376,7 @@ static int __init rtd1195_irq_mux_init(struct device_node *node,
 
 	return 0;
 }
+IRQCHIP_DECLARE(rtd1195_iso_mux, "realtek,rtd1195-iso-irq-mux", rtd1195_irq_mux_init);
 IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd1195_irq_mux_init);
+IRQCHIP_DECLARE(rtd1195_misc_mux, "realtek,rtd1195-misc-irq-mux", rtd1195_irq_mux_init);
 IRQCHIP_DECLARE(rtd1295_misc_mux, "realtek,rtd1295-misc-irq-mux", rtd1195_irq_mux_init);
-- 
2.16.4

[PATCH v4 5/8] ARM: dts: rtd1195: Add irq muxes and UART interrupts

[Andreas Färber]

Add iso and misc IRQ mux DT nodes for the Realtek RTD1195 SoC.

Update the UART DT nodes with interrupts from those muxes,
so that UART0 can be used without earlycon.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v4: New
 
 arch/arm/boot/dts/rtd1195.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/rtd1195.dtsi b/arch/arm/boot/dts/rtd1195.dtsi
index db1171c5adfa..ee7761ae4ee0 100644
--- a/arch/arm/boot/dts/rtd1195.dtsi
+++ b/arch/arm/boot/dts/rtd1195.dtsi
@@ -118,6 +118,14 @@
 				#reset-cells = <1>;
 			};
 
+			iso_irq_mux: interrupt-controller@7000 {
+				compatible = "realtek,rtd1195-iso-irq-mux";
+				reg = <0x7000 0x100>;
+				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
 			iso_reset: reset-controller@7088 {
 				compatible = "snps,dw-low-reset";
 				reg = <0x7088 0x4>;
@@ -137,6 +145,8 @@
 				reg-io-width = <4>;
 				resets = <&iso_reset RTD1195_ISO_RSTN_UR0>;
 				clock-frequency = <27000000>;
+				interrupt-parent = <&iso_irq_mux>;
+				interrupts = <2>;
 				status = "disabled";
 			};
 
@@ -145,6 +155,14 @@
 				reg = <0x1a200 0x8>;
 			};
 
+			misc_irq_mux: interrupt-controller@1b000 {
+				compatible = "realtek,rtd1195-misc-irq-mux";
+				reg = <0x1b000 0x100>;
+				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
 			uart1: serial@1b200 {
 				compatible = "snps,dw-apb-uart";
 				reg = <0x1b200 0x100>;
@@ -152,6 +170,8 @@
 				reg-io-width = <4>;
 				resets = <&reset2 RTD1195_RSTN_UR1>;
 				clock-frequency = <27000000>;
+				interrupt-parent = <&misc_irq_mux>;
+				interrupts = <3>;
 				status = "disabled";
 			};
 		};
-- 
2.16.4

[PATCH v4 6/8] dt-bindings: interrupt-controller: rtd1195-mux: Add RTD1395

[Andreas Färber]

Add compatible strings for Realtek RTD1395 SoC.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v4: New
 
 .../devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml   | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml
index 5cf3a28cedba..7c2a31548d46 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1195-mux.yaml
@@ -19,6 +19,8 @@ properties:
       - realtek,rtd1195-iso-irq-mux
       - realtek,rtd1295-misc-irq-mux
       - realtek,rtd1295-iso-irq-mux
+      - realtek,rtd1395-misc-irq-mux
+      - realtek,rtd1395-iso-irq-mux
 
   reg:
     maxItems: 1
-- 
2.16.4

[PATCH v4 7/8] irqchip: rtd1195-mux: Add RTD1395 definitions

[Andreas Färber]

Add compatible strings and bit mappings for Realtek RTD1395 SoC.

Based on BPI-M4-bsp linux-rtk/drivers/irqchip/irq-rtd139x.h.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v4: New
 
 drivers/irqchip/irq-rtd1195-mux.c | 83 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-rtd1195-mux.c b/drivers/irqchip/irq-rtd1195-mux.c
index 765d72653383..ad4b0ef3071b 100644
--- a/drivers/irqchip/irq-rtd1195-mux.c
+++ b/drivers/irqchip/irq-rtd1195-mux.c
@@ -1,7 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Realtek RTD1195/RTD1295 IRQ mux
+ * Realtek RTD1195/RTD1295/RTD1395 IRQ mux
  *
+ * Copyright (C) 2017 Realtek Semiconductor Corporation
  * Copyright (c) 2017-2019 Andreas Färber
  */
 
@@ -277,6 +278,62 @@ static const u32 rtd1295_misc_isr_to_scpu_int_en_mask[32] = {
 	[RTD1295_ISR_FAN_SHIFT]			= BIT(29),
 };
 
+enum rtd1395_iso_isr_bits {
+	RTD1395_ISO_ISR_UR0_SHIFT		= 2,
+	RTD1395_ISO_ISR_IRDA_SHIFT		= 5,
+	RTD1395_ISO_ISR_I2C0_SHIFT		= 8,
+	RTD1395_ISO_ISR_I2C1_SHIFT		= 11,
+	RTD1395_ISO_ISR_RTC_HSEC_SHIFT		= 12,
+	RTD1395_ISO_ISR_RTC_ALARM_SHIFT		= 13,
+	RTD1395_ISO_ISR_LSADC0_SHIFT		= 16,
+	RTD1395_ISO_ISR_LSADC1_SHIFT		= 17,
+	RTD1395_ISO_ISR_GPIOA_SHIFT		= 19,
+	RTD1395_ISO_ISR_GPIODA_SHIFT		= 20,
+	RTD1395_ISO_ISR_GPHY_HV_SHIFT		= 28,
+	RTD1395_ISO_ISR_GPHY_DV_SHIFT		= 29,
+	RTD1395_ISO_ISR_GPHY_AV_SHIFT		= 30,
+	RTD1395_ISO_ISR_I2C1_REQ_SHIFT		= 31,
+};
+
+static const u32 rtd1395_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1395_ISO_ISR_UR0_SHIFT]		= BIT(2),
+	[RTD1395_ISO_ISR_IRDA_SHIFT]		= BIT(5),
+	[RTD1395_ISO_ISR_I2C0_SHIFT]		= BIT(8),
+	[RTD1395_ISO_ISR_I2C1_SHIFT]		= BIT(11),
+	[RTD1395_ISO_ISR_RTC_HSEC_SHIFT]	= BIT(12),
+	[RTD1395_ISO_ISR_RTC_ALARM_SHIFT]	= BIT(13),
+	[RTD1395_ISO_ISR_LSADC0_SHIFT]		= BIT(16),
+	[RTD1395_ISO_ISR_LSADC1_SHIFT]		= BIT(17),
+	[RTD1395_ISO_ISR_GPIOA_SHIFT]		= BIT(19),
+	[RTD1395_ISO_ISR_GPIODA_SHIFT]		= BIT(20),
+	[RTD1395_ISO_ISR_GPHY_HV_SHIFT]		= BIT(28),
+	[RTD1395_ISO_ISR_GPHY_DV_SHIFT]		= BIT(29),
+	[RTD1395_ISO_ISR_GPHY_AV_SHIFT]		= BIT(30),
+	[RTD1395_ISO_ISR_I2C1_REQ_SHIFT]	= BIT(31),
+};
+
+enum rtd1395_misc_isr_bits {
+	RTD1395_MISC_ISR_UR1_SHIFT		= 3,
+	RTD1395_MISC_ISR_UR1_TO_SHIFT		= 5,
+	RTD1395_MISC_ISR_UR2_SHIFT		= 8,
+	RTD1395_MISC_ISR_UR2_TO_SHIFT		= 13,
+	RTD1395_MISC_ISR_I2C5_SHIFT		= 14,
+	RTD1395_MISC_ISR_SC0_SHIFT		= 24,
+	RTD1395_MISC_ISR_SPI_SHIFT		= 27,
+	RTD1395_MISC_ISR_FAN_SHIFT		= 29,
+};
+
+static const u32 rtd1395_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1395_MISC_ISR_UR1_SHIFT]		= BIT(3),
+	[RTD1395_MISC_ISR_UR1_TO_SHIFT]		= BIT(5),
+	[RTD1395_MISC_ISR_UR2_TO_SHIFT]		= BIT(6),
+	[RTD1395_MISC_ISR_UR2_SHIFT]		= BIT(7),
+	[RTD1395_MISC_ISR_I2C5_SHIFT]		= BIT(14),
+	[RTD1395_MISC_ISR_SC0_SHIFT]		= BIT(24),
+	[RTD1395_MISC_ISR_SPI_SHIFT]		= BIT(27),
+	[RTD1395_MISC_ISR_FAN_SHIFT]		= BIT(29),
+};
+
 static const struct rtd1195_irq_mux_info rtd1195_iso_irq_mux_info = {
 	.isr_offset		= 0x0,
 	.umsk_isr_offset	= 0x4,
@@ -291,6 +348,13 @@ static const struct rtd1195_irq_mux_info rtd1295_iso_irq_mux_info = {
 	.isr_to_int_en_mask	= rtd1295_iso_isr_to_scpu_int_en_mask,
 };
 
+static const struct rtd1195_irq_mux_info rtd1395_iso_irq_mux_info = {
+	.isr_offset		= 0x0,
+	.umsk_isr_offset	= 0x4,
+	.scpu_int_en_offset	= 0x40,
+	.isr_to_int_en_mask	= rtd1395_iso_isr_to_scpu_int_en_mask,
+};
+
 static const struct rtd1195_irq_mux_info rtd1195_misc_irq_mux_info = {
 	.umsk_isr_offset	= 0x8,
 	.isr_offset		= 0xc,
@@ -305,6 +369,13 @@ static const struct rtd1195_irq_mux_info rtd1295_misc_irq_mux_info = {
 	.isr_to_int_en_mask	= rtd1295_misc_isr_to_scpu_int_en_mask,
 };
 
+static const struct rtd1195_irq_mux_info rtd1395_misc_irq_mux_info = {
+	.umsk_isr_offset	= 0x8,
+	.isr_offset		= 0xc,
+	.scpu_int_en_offset	= 0x80,
+	.isr_to_int_en_mask	= rtd1395_misc_isr_to_scpu_int_en_mask,
+};
+
 static const struct of_device_id rtd1295_irq_mux_dt_matches[] = {
 	{
 		.compatible = "realtek,rtd1195-iso-irq-mux",
@@ -314,6 +385,10 @@ 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,rtd1395-iso-irq-mux",
+		.data = &rtd1395_iso_irq_mux_info,
+	},
 	{
 		.compatible = "realtek,rtd1195-misc-irq-mux",
 		.data = &rtd1195_misc_irq_mux_info,
@@ -322,6 +397,10 @@ static const struct of_device_id rtd1295_irq_mux_dt_matches[] = {
 		.compatible = "realtek,rtd1295-misc-irq-mux",
 		.data = &rtd1295_misc_irq_mux_info,
 	},
+	{
+		.compatible = "realtek,rtd1395-misc-irq-mux",
+		.data = &rtd1395_misc_irq_mux_info,
+	},
 	{
 	}
 };
@@ -378,5 +457,7 @@ static int __init rtd1195_irq_mux_init(struct device_node *node,
 }
 IRQCHIP_DECLARE(rtd1195_iso_mux, "realtek,rtd1195-iso-irq-mux", rtd1195_irq_mux_init);
 IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd1195_irq_mux_init);
+IRQCHIP_DECLARE(rtd1395_iso_mux, "realtek,rtd1395-iso-irq-mux", rtd1195_irq_mux_init);
 IRQCHIP_DECLARE(rtd1195_misc_mux, "realtek,rtd1195-misc-irq-mux", rtd1195_irq_mux_init);
 IRQCHIP_DECLARE(rtd1295_misc_mux, "realtek,rtd1295-misc-irq-mux", rtd1195_irq_mux_init);
+IRQCHIP_DECLARE(rtd1395_misc_mux, "realtek,rtd1395-misc-irq-mux", rtd1195_irq_mux_init);
-- 
2.16.4

[PATCH v4 8/8] arm64: dts: realtek: rtd139x: Add irq muxes and UART interrupts

[Andreas Färber]

Add iso and misc IRQ mux DT nodes for Realtek RTD1395 SoC.

Update the UART DT nodes with interrupts from these muxes,
so that UART0 can be used without earlycon.

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

diff --git a/arch/arm64/boot/dts/realtek/rtd139x.dtsi b/arch/arm64/boot/dts/realtek/rtd139x.dtsi
index 706da12f9ea3..f53cb8a5083b 100644
--- a/arch/arm64/boot/dts/realtek/rtd139x.dtsi
+++ b/arch/arm64/boot/dts/realtek/rtd139x.dtsi
@@ -84,6 +84,14 @@
 				#reset-cells = <1>;
 			};
 
+			iso_irq_mux: interrupt-controller@7000 {
+				compatible = "realtek,rtd1395-iso-irq-mux";
+				reg = <0x7000 0x100>;
+				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
 			iso_reset: reset-controller@7088 {
 				compatible = "snps,dw-low-reset";
 				reg = <0x7088 0x4>;
@@ -103,6 +111,8 @@
 				reg-io-width = <4>;
 				clock-frequency = <27000000>;
 				resets = <&iso_reset RTD1295_ISO_RSTN_UR0>;
+				interrupt-parent = <&iso_irq_mux>;
+				interrupts = <2>;
 				status = "disabled";
 			};
 
@@ -111,6 +121,14 @@
 				reg = <0x1a200 0x8>;
 			};
 
+			misc_irq_mux: interrupt-controller@1b000 {
+				compatible = "realtek,rtd1395-misc-irq-mux";
+				reg = <0x1b000 0x100>;
+				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+
 			uart1: serial@1b200 {
 				compatible = "snps,dw-apb-uart";
 				reg = <0x1b200 0x100>;
@@ -118,6 +136,8 @@
 				reg-io-width = <4>;
 				clock-frequency = <432000000>;
 				resets = <&reset2 RTD1295_RSTN_UR1>;
+				interrupt-parent = <&misc_irq_mux>;
+				interrupts = <3>;
 				status = "disabled";
 			};
 
@@ -128,6 +148,8 @@
 				reg-io-width = <4>;
 				clock-frequency = <432000000>;
 				resets = <&reset2 RTD1295_RSTN_UR2>;
+				interrupt-parent = <&misc_irq_mux>;
+				interrupts = <8>;
 				status = "disabled";
 			};
 		};
-- 
2.16.4

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 2019-11-19 02:19, Andreas Färber wrote:
> This irq mux driver implements the RTD1295 SoC's non-linear mapping
> between status and enable bits.
>
> Based in part on QNAP's arch/arm/mach-rtk119x/rtk_irq_mux.c and
> Synology's drivers/irqchip/irq-rtk.c code.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Aleix Roca Nonell <kernelrocks@gmail.com>
> Signed-off-by: James Tai <james.tai@realtek.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  v3 -> v4:
>  * Drop no-op .irq_set_affinity callback (Thomas)
>  * Clear all interrupts (James)
>  * Updated SPDX-License-identifier
>  * Use tabular formatting (Thomas)
>  * Adopt different braces style (Thomas)
>  * Use raw_spinlock_t (Thomas)
>  * Shortened callback from isr_to_scpu_int_en_mask to
> isr_to_int_en_mask (Thomas)
>  * Fixed of_iomap() error handling to not use IS_ERR()
>  * Don't mask unmapped NMIs by checking for a non-zero mask
>  * Cache SCPU_INT_EN to avoid superfluous reads (Thomas)
>  * Renamed functions and variables from rtd119x to rtd1195
>
>  v2 -> v3:
>  * Adopted spin_lock_irq{save,restore}() (Marc)
>  * Adopted single-write masking (Marc)
>  * Adopted misc compatible string
>  * Introduced explicit bit mapping
>  * Adopted looped processing of pending interrupts (Marc)
>  * Replaced unmask implementation with UMSK_ISR write
>  * Introduced enable/disable ops and dropped no longer needed UART0 
> quirk
>
>  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-rtd1195-mux.c | 283
> ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 284 insertions(+)
>  create mode 100644 drivers/irqchip/irq-rtd1195-mux.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e806dda690ea..d678881eebc8 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -104,3 +104,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-rtd1195-mux.o
> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
> b/drivers/irqchip/irq-rtd1195-mux.c
> new file mode 100644
> index 000000000000..e6b08438b23c
> --- /dev/null
> +++ b/drivers/irqchip/irq-rtd1195-mux.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Realtek RTD1295 IRQ mux
> + *
> + * Copyright (c) 2017-2019 Andreas Färber
> + */
> +
> +#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 rtd1195_irq_mux_info {
> +	unsigned int	isr_offset;
> +	unsigned int	umsk_isr_offset;
> +	unsigned int	scpu_int_en_offset;
> +	const u32	*isr_to_int_en_mask;
> +};
> +
> +struct rtd1195_irq_mux_data {
> +	void __iomem				*reg_isr;
> +	void __iomem				*reg_umsk_isr;
> +	void __iomem				*reg_scpu_int_en;
> +	const struct rtd1195_irq_mux_info	*info;
> +	int					irq;
> +	u32					scpu_int_en;
> +	struct irq_domain			*domain;
> +	raw_spinlock_t				lock;
> +};
> +
> +static void rtd1195_mux_irq_handle(struct irq_desc *desc)
> +{
> +	struct rtd1195_irq_mux_data *data = 
> irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	u32 isr, mask;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	isr = readl_relaxed(data->reg_isr);
> +
> +	while (isr) {
> +		i = __ffs(isr);
> +		isr &= ~BIT(i);
> +
> +		mask = data->info->isr_to_int_en_mask[i];
> +		if (mask && !(data->scpu_int_en & mask))
> +			continue;
> +
> +		if (!generic_handle_irq(irq_find_mapping(data->domain, i)))
> +			writel_relaxed(BIT(i), data->reg_isr);

What does this write do exactly? It is the same thing as a 'mask',
which is pretty odd. So either:

- this is not doing anything and your 'mask' callback is bogus
   (otherwise you'd never have more than a single interrupt)

- or this is an ACK operation, and this should be described as
   such (and then fix the mask/unmask/enable/disable mess that
   results from it).

as I can't see how the same register can be used for both purposes.
You should be able to verify this experimentally, even without
documentation.

> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void rtd1195_mux_mask_irq(struct irq_data *data)
> +{
> +	struct rtd1195_irq_mux_data *mux_data = 
> irq_data_get_irq_chip_data(data);
> +
> +	writel_relaxed(BIT(data->hwirq), mux_data->reg_isr);
> +}
> +
> +static void rtd1195_mux_unmask_irq(struct irq_data *data)
> +{
> +	struct rtd1195_irq_mux_data *mux_data = 
> irq_data_get_irq_chip_data(data);
> +
> +	writel_relaxed(BIT(data->hwirq), mux_data->reg_umsk_isr);
> +}
> +
> +static void rtd1195_mux_enable_irq(struct irq_data *data)
> +{
> +	struct rtd1195_irq_mux_data *mux_data = 
> irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 mask;
> +
> +	mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
> +	if (!mask)
> +		return;

How can this happen? You've mapped the interrupt, so it exists.
I can't see how you can decide to fail such enable.

> +
> +	raw_spin_lock_irqsave(&mux_data->lock, flags);
> +
> +	mux_data->scpu_int_en |= mask;
> +	writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en);
> +
> +	raw_spin_unlock_irqrestore(&mux_data->lock, flags);
> +}
> +
> +static void rtd1195_mux_disable_irq(struct irq_data *data)
> +{
> +	struct rtd1195_irq_mux_data *mux_data = 
> irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 mask;
> +
> +	mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
> +	if (!mask)
> +		return;
> +
> +	raw_spin_lock_irqsave(&mux_data->lock, flags);
> +
> +	mux_data->scpu_int_en &= ~mask;
> +	writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en);
> +
> +	raw_spin_unlock_irqrestore(&mux_data->lock, flags);
> +}
> +
> +static struct irq_chip rtd1195_mux_irq_chip = {
> +	.name		= "rtd1195-mux",
> +	.irq_mask	= rtd1195_mux_mask_irq,
> +	.irq_unmask	= rtd1195_mux_unmask_irq,
> +	.irq_enable	= rtd1195_mux_enable_irq,
> +	.irq_disable	= rtd1195_mux_disable_irq,
> +};

[...]

Although the code is pretty clean, the way you drive the HW looks
suspicious, and requires clarification.

Thanks,

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

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Am 19.11.19 um 13:01 schrieb Marc Zyngier:
> On 2019-11-19 02:19, Andreas Färber wrote:
>> +static void rtd1195_mux_enable_irq(struct irq_data *data)
>> +{
>> +    struct rtd1195_irq_mux_data *mux_data =
>> irq_data_get_irq_chip_data(data);
>> +    unsigned long flags;
>> +    u32 mask;
>> +
>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>> +    if (!mask)
>> +        return;
> 
> How can this happen? You've mapped the interrupt, so it exists.
> I can't see how you can decide to fail such enable.

The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same.

My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but
are sparsely populated. So there are circumstances such as WDOG_NMI as
well as reserved bits that we cannot enable. This check should be
identical to v3; the equivalent mask check inside the interrupt handler
was extended with "mask &&" to do the same in this v4.

The other question I'll need to dig into, it's been two years since I
wrote that code - first very simple guesswork, then more elaborate
quirks like the above.

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On Tue, 19 Nov 2019 21:56:48 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
> > On 2019-11-19 02:19, Andreas Färber wrote:  
> >> +static void rtd1195_mux_enable_irq(struct irq_data *data)
> >> +{
> >> +    struct rtd1195_irq_mux_data *mux_data =
> >> irq_data_get_irq_chip_data(data);
> >> +    unsigned long flags;
> >> +    u32 mask;
> >> +
> >> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
> >> +    if (!mask)
> >> +        return;  
> > 
> > How can this happen? You've mapped the interrupt, so it exists.
> > I can't see how you can decide to fail such enable.  
> 
> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same.
> 
> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but
> are sparsely populated. So there are circumstances such as WDOG_NMI as
> well as reserved bits that we cannot enable.

But the you should have failed the map. The moment you allow the
mapping to occur, you have accepted the contract that this interrupt is
usable.

> This check should be
> identical to v3; the equivalent mask check inside the interrupt handler
> was extended with "mask &&" to do the same in this v4.

Spurious interrupts are a different matter. What I'm objecting to here
is a simple question of logic, whether or not you are allowed to fail
enabling an interrupt that you've otherwise allowed to be populated.
 
	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Am 19.11.19 um 13:01 schrieb Marc Zyngier:
> On 2019-11-19 02:19, Andreas Färber wrote:
>> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
>> b/drivers/irqchip/irq-rtd1195-mux.c
>> new file mode 100644
>> index 000000000000..e6b08438b23c
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-rtd1195-mux.c
[...]
>> +static void rtd1195_mux_irq_handle(struct irq_desc *desc)
>> +{
>> +    struct rtd1195_irq_mux_data *data = irq_desc_get_handler_data(desc);
>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>> +    u32 isr, mask;
>> +    int i;
>> +
>> +    chained_irq_enter(chip, desc);
>> +
>> +    isr = readl_relaxed(data->reg_isr);
>> +
>> +    while (isr) {
>> +        i = __ffs(isr);
>> +        isr &= ~BIT(i);
>> +
>> +        mask = data->info->isr_to_int_en_mask[i];
>> +        if (mask && !(data->scpu_int_en & mask))
>> +            continue;
>> +
>> +        if (!generic_handle_irq(irq_find_mapping(data->domain, i)))
>> +            writel_relaxed(BIT(i), data->reg_isr);
> 
> What does this write do exactly? It is the same thing as a 'mask',
> which is pretty odd. So either:
> 
> - this is not doing anything and your 'mask' callback is bogus
>   (otherwise you'd never have more than a single interrupt)
> 
> - or this is an ACK operation, and this should be described as
>   such (and then fix the mask/unmask/enable/disable mess that
>   results from it).

This is supposed to be an ACK, i.e. clear-1-bits operation.

The BSP had extended various drivers, such as 8250 UART, to do this ack
in their interrupt handler through an additional DT reg region. I tried
to clean that up by handling it centrally here in the irqchip driver.

> 
> as I can't see how the same register can be used for both purposes.
> You should be able to verify this experimentally, even without
> documentation.

There are three registers here:

MIS_UMSK_ISR    - MISC unmasked interrupt status register
MIS_ISR         - MISC   masked interrupt status register
MIS_SCPU_INT_EN - MISC SCPU interrupt enable register

The latter is a regular R/W register; the former two have a write_data
field as BIT(0), with 1 indicating a write vs. 0 indicating clear, RAZ.

By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but
not in _UMSK_ISR.

Does that shed any more light?

So given that we're iterating over reg_isr above, we could probably drop
the mask check here...

In addition there are MIS_[UMSK_]ISR_SWC and MIS_SETTING_SWC registers
for Secure World, and MIS_FAST_INT_EN_* and MIS_FAST_ISR as well as
various GPIO interrupt registers.

Regards,
Andreas

>> +    }
>> +
>> +    chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void rtd1195_mux_mask_irq(struct irq_data *data)
>> +{
>> +    struct rtd1195_irq_mux_data *mux_data =
>> irq_data_get_irq_chip_data(data);
>> +
>> +    writel_relaxed(BIT(data->hwirq), mux_data->reg_isr);
>> +}
>> +
>> +static void rtd1195_mux_unmask_irq(struct irq_data *data)
>> +{
>> +    struct rtd1195_irq_mux_data *mux_data =
>> irq_data_get_irq_chip_data(data);
>> +
>> +    writel_relaxed(BIT(data->hwirq), mux_data->reg_umsk_isr);
>> +}
>> +
>> +static void rtd1195_mux_enable_irq(struct irq_data *data)
>> +{
>> +    struct rtd1195_irq_mux_data *mux_data =
>> irq_data_get_irq_chip_data(data);
>> +    unsigned long flags;
>> +    u32 mask;
>> +
>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>> +    if (!mask)
>> +        return;
> 
> How can this happen? You've mapped the interrupt, so it exists.
> I can't see how you can decide to fail such enable.
> 
>> +
>> +    raw_spin_lock_irqsave(&mux_data->lock, flags);
>> +
>> +    mux_data->scpu_int_en |= mask;
>> +    writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en);
>> +
>> +    raw_spin_unlock_irqrestore(&mux_data->lock, flags);
>> +}
>> +
>> +static void rtd1195_mux_disable_irq(struct irq_data *data)
>> +{
>> +    struct rtd1195_irq_mux_data *mux_data =
>> irq_data_get_irq_chip_data(data);
>> +    unsigned long flags;
>> +    u32 mask;
>> +
>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>> +    if (!mask)
>> +        return;
>> +
>> +    raw_spin_lock_irqsave(&mux_data->lock, flags);
>> +
>> +    mux_data->scpu_int_en &= ~mask;
>> +    writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en);
>> +
>> +    raw_spin_unlock_irqrestore(&mux_data->lock, flags);
>> +}
>> +
>> +static struct irq_chip rtd1195_mux_irq_chip = {
>> +    .name        = "rtd1195-mux",
>> +    .irq_mask    = rtd1195_mux_mask_irq,
>> +    .irq_unmask    = rtd1195_mux_unmask_irq,
>> +    .irq_enable    = rtd1195_mux_enable_irq,
>> +    .irq_disable    = rtd1195_mux_disable_irq,
>> +};
> 
> [...]
> 
> Although the code is pretty clean, the way you drive the HW looks
> suspicious, and requires clarification.
> 
> Thanks,
> 
>         M.


-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Am 19.11.19 um 23:29 schrieb Marc Zyngier:
> On Tue, 19 Nov 2019 21:56:48 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>> On 2019-11-19 02:19, Andreas Färber wrote:  
>>>> +static void rtd1195_mux_enable_irq(struct irq_data *data)
>>>> +{
>>>> +    struct rtd1195_irq_mux_data *mux_data =
>>>> irq_data_get_irq_chip_data(data);
>>>> +    unsigned long flags;
>>>> +    u32 mask;
>>>> +
>>>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>>>> +    if (!mask)
>>>> +        return;  
>>>
>>> How can this happen? You've mapped the interrupt, so it exists.
>>> I can't see how you can decide to fail such enable.  
>>
>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same.
>>
>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but
>> are sparsely populated. So there are circumstances such as WDOG_NMI as
>> well as reserved bits that we cannot enable.
> 
> But the you should have failed the map. The moment you allow the
> mapping to occur, you have accepted the contract that this interrupt is
> usable.
> 
>> This check should be
>> identical to v3; the equivalent mask check inside the interrupt handler
>> was extended with "mask &&" to do the same in this v4.
> 
> Spurious interrupts are a different matter. What I'm objecting to here
> is a simple question of logic, whether or not you are allowed to fail
> enabling an interrupt that you've otherwise allowed to be populated.

Then what are you suggesting instead? I don't see how my array map
lookup could fail other than returning a zero value, given its static
initialization. Check for a zero mask in rtd1195_mux_irq_domain_map()?
Then we wouldn't be able to use the mentioned WDOG_NMI. Add another
per-mux info field for which interrupts are valid to map?

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 2019-11-19 23:25, Andreas Färber wrote:
> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>> On 2019-11-19 02:19, Andreas Färber wrote:
>>> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
>>> b/drivers/irqchip/irq-rtd1195-mux.c
>>> new file mode 100644
>>> index 000000000000..e6b08438b23c
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-rtd1195-mux.c
> [...]
>>> +static void rtd1195_mux_irq_handle(struct irq_desc *desc)
>>> +{
>>> +    struct rtd1195_irq_mux_data *data = 
>>> irq_desc_get_handler_data(desc);
>>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +    u32 isr, mask;
>>> +    int i;
>>> +
>>> +    chained_irq_enter(chip, desc);
>>> +
>>> +    isr = readl_relaxed(data->reg_isr);
>>> +
>>> +    while (isr) {
>>> +        i = __ffs(isr);
>>> +        isr &= ~BIT(i);
>>> +
>>> +        mask = data->info->isr_to_int_en_mask[i];
>>> +        if (mask && !(data->scpu_int_en & mask))
>>> +            continue;
>>> +
>>> +        if (!generic_handle_irq(irq_find_mapping(data->domain, 
>>> i)))
>>> +            writel_relaxed(BIT(i), data->reg_isr);
>>
>> What does this write do exactly? It is the same thing as a 'mask',
>> which is pretty odd. So either:
>>
>> - this is not doing anything and your 'mask' callback is bogus
>>   (otherwise you'd never have more than a single interrupt)
>>
>> - or this is an ACK operation, and this should be described as
>>   such (and then fix the mask/unmask/enable/disable mess that
>>   results from it).
>
> This is supposed to be an ACK, i.e. clear-1-bits operation.

If it is an ACK, model it as such, and do not open-code it.

>
> The BSP had extended various drivers, such as 8250 UART, to do this 
> ack
> in their interrupt handler through an additional DT reg region. I 
> tried
> to clean that up by handling it centrally here in the irqchip driver.
>
>>
>> as I can't see how the same register can be used for both purposes.
>> You should be able to verify this experimentally, even without
>> documentation.
>
> There are three registers here:
>
> MIS_UMSK_ISR    - MISC unmasked interrupt status register
> MIS_ISR         - MISC   masked interrupt status register
> MIS_SCPU_INT_EN - MISC SCPU interrupt enable register
>
> The latter is a regular R/W register; the former two have a 
> write_data
> field as BIT(0), with 1 indicating a write vs. 0 indicating clear, 
> RAZ.
>
> By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but
> not in _UMSK_ISR.
>
> Does that shed any more light?

None whatsoever. Your mask callback doesn't make any sense, since it
actually acks the interrupt. My gut feeling is that your enable/disable
should really be mask/unmask.

>
> So given that we're iterating over reg_isr above, we could probably 
> drop
> the mask check here...
>
> In addition there are MIS_[UMSK_]ISR_SWC and MIS_SETTING_SWC 
> registers
> for Secure World, and MIS_FAST_INT_EN_* and MIS_FAST_ISR as well as
> various GPIO interrupt registers.

This doesn't seem relevant to the discussion here.

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

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 2019-11-19 23:33, Andreas Färber wrote:
> Am 19.11.19 um 23:29 schrieb Marc Zyngier:
>> On Tue, 19 Nov 2019 21:56:48 +0100
>> Andreas Färber <afaerber@suse.de> wrote:
>>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>>> On 2019-11-19 02:19, Andreas Färber wrote:
>>>>> +static void rtd1195_mux_enable_irq(struct irq_data *data)
>>>>> +{
>>>>> +    struct rtd1195_irq_mux_data *mux_data =
>>>>> irq_data_get_irq_chip_data(data);
>>>>> +    unsigned long flags;
>>>>> +    u32 mask;
>>>>> +
>>>>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>>>>> +    if (!mask)
>>>>> +        return;
>>>>
>>>> How can this happen? You've mapped the interrupt, so it exists.
>>>> I can't see how you can decide to fail such enable.
>>>
>>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the 
>>> same.
>>>
>>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, 
>>> but
>>> are sparsely populated. So there are circumstances such as WDOG_NMI 
>>> as
>>> well as reserved bits that we cannot enable.
>>
>> But the you should have failed the map. The moment you allow the
>> mapping to occur, you have accepted the contract that this interrupt 
>> is
>> usable.
>>
>>> This check should be
>>> identical to v3; the equivalent mask check inside the interrupt 
>>> handler
>>> was extended with "mask &&" to do the same in this v4.
>>
>> Spurious interrupts are a different matter. What I'm objecting to 
>> here
>> is a simple question of logic, whether or not you are allowed to 
>> fail
>> enabling an interrupt that you've otherwise allowed to be populated.
>
> Then what are you suggesting instead? I don't see how my array map
> lookup could fail other than returning a zero value, given its static
> initialization. Check for a zero mask in 
> rtd1195_mux_irq_domain_map()?
> Then we wouldn't be able to use the mentioned WDOG_NMI. Add another
> per-mux info field for which interrupts are valid to map?

I'm suggesting that you fail the map if you're unable to allow the
interrupt to be enabled.

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

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Am 20.11.19 um 11:18 schrieb Marc Zyngier:
> On 2019-11-19 23:25, Andreas Färber wrote:
>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>> On 2019-11-19 02:19, Andreas Färber wrote:
>>>> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
>>>> b/drivers/irqchip/irq-rtd1195-mux.c
>>>> new file mode 100644
>>>> index 000000000000..e6b08438b23c
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-rtd1195-mux.c
>> [...]
>>>> +static void rtd1195_mux_irq_handle(struct irq_desc *desc)
>>>> +{
>>>> +    struct rtd1195_irq_mux_data *data =
>>>> irq_desc_get_handler_data(desc);
>>>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> +    u32 isr, mask;
>>>> +    int i;
>>>> +
>>>> +    chained_irq_enter(chip, desc);
>>>> +
>>>> +    isr = readl_relaxed(data->reg_isr);
>>>> +
>>>> +    while (isr) {
>>>> +        i = __ffs(isr);
>>>> +        isr &= ~BIT(i);
>>>> +
>>>> +        mask = data->info->isr_to_int_en_mask[i];
>>>> +        if (mask && !(data->scpu_int_en & mask))
>>>> +            continue;
>>>> +
>>>> +        if (!generic_handle_irq(irq_find_mapping(data->domain, i)))
>>>> +            writel_relaxed(BIT(i), data->reg_isr);
>>>
>>> What does this write do exactly? It is the same thing as a 'mask',
>>> which is pretty odd. So either:
>>>
>>> - this is not doing anything and your 'mask' callback is bogus
>>>   (otherwise you'd never have more than a single interrupt)
>>>
>>> - or this is an ACK operation, and this should be described as
>>>   such (and then fix the mask/unmask/enable/disable mess that
>>>   results from it).
>>
>> This is supposed to be an ACK, i.e. clear-1-bits operation.
> 
> If it is an ACK, model it as such, and do not open-code it.

I have found an .irq_ack callback - moving this there appears to work.

Alternatively there is an irq_eoi callback and an IRQCHIP_EOI_IF_HANDLED
flag.

It would really help me if you spelled out explicitly where you think I
should be moving code, as the documentation in irq.h is not all that
helpful in terms of when are they called and what should be done there.
In case not obvious, this is my first irqchip driver.

>>
>> The BSP had extended various drivers, such as 8250 UART, to do this ack
>> in their interrupt handler through an additional DT reg region. I tried
>> to clean that up by handling it centrally here in the irqchip driver.
>>
>>>
>>> as I can't see how the same register can be used for both purposes.
>>> You should be able to verify this experimentally, even without
>>> documentation.
>>
>> There are three registers here:
>>
>> MIS_UMSK_ISR    - MISC unmasked interrupt status register
>> MIS_ISR         - MISC   masked interrupt status register
>> MIS_SCPU_INT_EN - MISC SCPU interrupt enable register
>>
>> The latter is a regular R/W register; the former two have a write_data
>> field as BIT(0), with 1 indicating a write vs. 0 indicating clear, RAZ.
>>
>> By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but
>> not in _UMSK_ISR.
>>
>> Does that shed any more light?
> 
> None whatsoever. Your mask callback doesn't make any sense, since it
> actually acks the interrupt. My gut feeling is that your enable/disable
> should really be mask/unmask.

Renaming enable -> unmask and disable -> mask works okay.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 2019-11-20 12:12, Andreas Färber wrote:
> Am 20.11.19 um 11:18 schrieb Marc Zyngier:
>> On 2019-11-19 23:25, Andreas Färber wrote:
>>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>>> On 2019-11-19 02:19, Andreas Färber wrote:
>>>>> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
>>>>> b/drivers/irqchip/irq-rtd1195-mux.c
>>>>> new file mode 100644
>>>>> index 000000000000..e6b08438b23c
>>>>> --- /dev/null
>>>>> +++ b/drivers/irqchip/irq-rtd1195-mux.c
>>> [...]
>>>>> +static void rtd1195_mux_irq_handle(struct irq_desc *desc)
>>>>> +{
>>>>> +    struct rtd1195_irq_mux_data *data =
>>>>> irq_desc_get_handler_data(desc);
>>>>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>>> +    u32 isr, mask;
>>>>> +    int i;
>>>>> +
>>>>> +    chained_irq_enter(chip, desc);
>>>>> +
>>>>> +    isr = readl_relaxed(data->reg_isr);
>>>>> +
>>>>> +    while (isr) {
>>>>> +        i = __ffs(isr);
>>>>> +        isr &= ~BIT(i);
>>>>> +
>>>>> +        mask = data->info->isr_to_int_en_mask[i];
>>>>> +        if (mask && !(data->scpu_int_en & mask))
>>>>> +            continue;
>>>>> +
>>>>> +        if (!generic_handle_irq(irq_find_mapping(data->domain, 
>>>>> i)))
>>>>> +            writel_relaxed(BIT(i), data->reg_isr);
>>>>
>>>> What does this write do exactly? It is the same thing as a 'mask',
>>>> which is pretty odd. So either:
>>>>
>>>> - this is not doing anything and your 'mask' callback is bogus
>>>>   (otherwise you'd never have more than a single interrupt)
>>>>
>>>> - or this is an ACK operation, and this should be described as
>>>>   such (and then fix the mask/unmask/enable/disable mess that
>>>>   results from it).
>>>
>>> This is supposed to be an ACK, i.e. clear-1-bits operation.
>>
>> If it is an ACK, model it as such, and do not open-code it.
>
> I have found an .irq_ack callback - moving this there appears to 
> work.
>
> Alternatively there is an irq_eoi callback and an 
> IRQCHIP_EOI_IF_HANDLED
> flag.
>
> It would really help me if you spelled out explicitly where you think 
> I
> should be moving code, as the documentation in irq.h is not all that
> helpful in terms of when are they called and what should be done 
> there.
> In case not obvious, this is my first irqchip driver.

Implementing one callback or the other really depends on how the HW
behaves. The irq framework gives you a wide range of flows that allow
you to plug your driver in the stack, but the prerequisite is that you
know *exactly* how the HW behaves. Ack and EOI have very different
meanings, are called from different flows, and correspond to different
states in the interrupt life cycle.

Use the wrong one, and you will lose interrupts. If you don't know how
the HW behaves, then the chances of something bad happening are pretty
high (you'll end-up in deadlock land at some point). I'm afraid I 
cannot
help you with that, short of being given access to some documentation
that doesn't seem to exist.

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

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Am 20.11.19 um 11:20 schrieb Marc Zyngier:
> On 2019-11-19 23:33, Andreas Färber wrote:
>> Am 19.11.19 um 23:29 schrieb Marc Zyngier:
>>> On Tue, 19 Nov 2019 21:56:48 +0100
>>> Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>>>> On 2019-11-19 02:19, Andreas Färber wrote:
>>>>>> +static void rtd1195_mux_enable_irq(struct irq_data *data)
>>>>>> +{
>>>>>> +    struct rtd1195_irq_mux_data *mux_data =
>>>>>> irq_data_get_irq_chip_data(data);
>>>>>> +    unsigned long flags;
>>>>>> +    u32 mask;
>>>>>> +
>>>>>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>>>>>> +    if (!mask)
>>>>>> +        return;
>>>>>
>>>>> How can this happen? You've mapped the interrupt, so it exists.
>>>>> I can't see how you can decide to fail such enable.
>>>>
>>>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same.
>>>>
>>>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but
>>>> are sparsely populated. So there are circumstances such as WDOG_NMI as
>>>> well as reserved bits that we cannot enable.
>>>
>>> But the you should have failed the map. The moment you allow the
>>> mapping to occur, you have accepted the contract that this interrupt is
>>> usable.
>>>
>>>> This check should be
>>>> identical to v3; the equivalent mask check inside the interrupt handler
>>>> was extended with "mask &&" to do the same in this v4.
>>>
>>> Spurious interrupts are a different matter. What I'm objecting to here
>>> is a simple question of logic, whether or not you are allowed to fail
>>> enabling an interrupt that you've otherwise allowed to be populated.
>>
>> Then what are you suggesting instead? I don't see how my array map
>> lookup could fail other than returning a zero value, given its static
>> initialization. Check for a zero mask in rtd1195_mux_irq_domain_map()?
>> Then we wouldn't be able to use the mentioned WDOG_NMI. Add another
>> per-mux info field for which interrupts are valid to map?
> 
> I'm suggesting that you fail the map if you're unable to allow the
> interrupt to be enabled.

The NMI will always be enabled, it just can't be disabled.

I have added a check to suppress a zero hwirq. Suppressing reserved IRQ
bits will take some more effort to distinguish from NMIs. In particular
if we flag this in the ..._isr_to_scpu_int_en array by some magic mask
value like 0xffffffff then all users need to check for two rather than
one value - but if we reduce the users, it shouldn't matter too much.

With contract I assume you're referring to these callbacks having a void
return type, unable to return an error to the caller, and there being no
is_enabled/is_masked callbacks for anyone to discover this.

Unfortunately NMI handling appears to be only used in GICv3 and is not
very intuitive for me: Apparently I can only flag the whole irq_chip as
being NMI but not individual IRQs? Would that mean that this driver
would need to instantiate a second irq_chip for that one IRQ? How would
that work for mapping from DT? Given that this mux relies on a maskable
GICv2 IRQ, it's not a "true" NMI in the Linux sense anyway, other than
the .irq_mask callback not being applicable. While I don't need that NMI
immediately, I would prefer not to merge a driver that by design can't
cope with it later.

I'll try to post a v5 with rsv and nmi blocked in map for further
discussion tonight.

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 2019-11-20 13:34, Andreas Färber wrote:
> Am 20.11.19 um 11:20 schrieb Marc Zyngier:
>> On 2019-11-19 23:33, Andreas Färber wrote:
>>> Am 19.11.19 um 23:29 schrieb Marc Zyngier:
>>>> On Tue, 19 Nov 2019 21:56:48 +0100
>>>> Andreas Färber <afaerber@suse.de> wrote:
>>>>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>>>>> On 2019-11-19 02:19, Andreas Färber wrote:
>>>>>>> +static void rtd1195_mux_enable_irq(struct irq_data *data)
>>>>>>> +{
>>>>>>> +    struct rtd1195_irq_mux_data *mux_data =
>>>>>>> irq_data_get_irq_chip_data(data);
>>>>>>> +    unsigned long flags;
>>>>>>> +    u32 mask;
>>>>>>> +
>>>>>>> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
>>>>>>> +    if (!mask)
>>>>>>> +        return;
>>>>>>
>>>>>> How can this happen? You've mapped the interrupt, so it exists.
>>>>>> I can't see how you can decide to fail such enable.
>>>>>
>>>>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the 
>>>>> same.
>>>>>
>>>>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) 
>>>>> lookup, but
>>>>> are sparsely populated. So there are circumstances such as 
>>>>> WDOG_NMI as
>>>>> well as reserved bits that we cannot enable.
>>>>
>>>> But the you should have failed the map. The moment you allow the
>>>> mapping to occur, you have accepted the contract that this 
>>>> interrupt is
>>>> usable.
>>>>
>>>>> This check should be
>>>>> identical to v3; the equivalent mask check inside the interrupt 
>>>>> handler
>>>>> was extended with "mask &&" to do the same in this v4.
>>>>
>>>> Spurious interrupts are a different matter. What I'm objecting to 
>>>> here
>>>> is a simple question of logic, whether or not you are allowed to 
>>>> fail
>>>> enabling an interrupt that you've otherwise allowed to be 
>>>> populated.
>>>
>>> Then what are you suggesting instead? I don't see how my array map
>>> lookup could fail other than returning a zero value, given its 
>>> static
>>> initialization. Check for a zero mask in 
>>> rtd1195_mux_irq_domain_map()?
>>> Then we wouldn't be able to use the mentioned WDOG_NMI. Add another
>>> per-mux info field for which interrupts are valid to map?
>>
>> I'm suggesting that you fail the map if you're unable to allow the
>> interrupt to be enabled.
>
> The NMI will always be enabled, it just can't be disabled.

If I really cared, I'd cry. This HW is useless.

> I have added a check to suppress a zero hwirq. Suppressing reserved 
> IRQ
> bits will take some more effort to distinguish from NMIs. In 
> particular
> if we flag this in the ..._isr_to_scpu_int_en array by some magic 
> mask
> value like 0xffffffff then all users need to check for two rather 
> than
> one value - but if we reduce the users, it shouldn't matter too much.

1) you can't suppress a level interrupt that cannot be disabled. It 
will
fire back at you.
2) given that you have to demux things using MMIO accesses, performance
is the least of anybody's worry.

>
> With contract I assume you're referring to these callbacks having a 
> void
> return type, unable to return an error to the caller, and there being 
> no
> is_enabled/is_masked callbacks for anyone to discover this.
>
> Unfortunately NMI handling appears to be only used in GICv3 and is 
> not
> very intuitive for me: Apparently I can only flag the whole irq_chip 
> as
> being NMI but not individual IRQs? Would that mean that this driver
> would need to instantiate a second irq_chip for that one IRQ? How 
> would
> that work for mapping from DT? Given that this mux relies on a 
> maskable
> GICv2 IRQ, it's not a "true" NMI in the Linux sense anyway, other 
> than
> the .irq_mask callback not being applicable. While I don't need that 
> NMI
> immediately, I would prefer not to merge a driver that by design 
> can't
> cope with it later.

You are missing the point of the pseudo-NMI infrastructure. To be 
useful,
it *must* be the root interrupt controller. Otherwise, you cannot 
distinguish
it from the other interrupts it is muxed with. Your 'NMI' is absolutely
unusable, and whoever designed this HW should be actively prevented
from ever designing another interrupt controller again.

> I'll try to post a v5 with rsv and nmi blocked in map for further
> discussion tonight.

I don't plan to review any of this until after the merge window, so 
please
take as long as you want.

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