[PATCH v3 0/6] Initial support for the Realtek DHC SoCs

[PATCH v3 0/6] Initial support for the Realtek DHC SoCs

[James Tai]

This series introduces an interrupt controller driver for the Realtek DHC 
(Digital Home Center) SoCs.

Change log:
v2 -> v3:
- Retested the bindings using the new version of the dtschema
- Fixed the order of property items
- Removed redundant files and replaced them with 'realtek,intc.yaml'
- Replaced 'interrupts-extended' with 'interrupts'
- Added a description for 'interrupts'
- Reduced the example code
- Resolved kernel test robot build warnings

v1 -> v2:
- Tested the bindings using 'make dt_binding_check'
- Fixed code style issues
- Resolved kernel test robot build warnings
- Replaced spin_lock_irqsave with raw_spin_lock
- Replaced magic number with macro
- Removed the realtek_intc_set_affinity function

James Tai (6):
  dt-bindings: interrupt-controller: Add support for Realtek DHC SoCs
  irqchip: Add interrupt controller support for Realtek DHC SoCs
  irqchip: Introduce RTD1319 support using the Realtek common interrupt
    controller driver
  irqchip: Introduce RTD1319D support using the Realtek common interrupt
    controller driver
  irqchip: Introduce RTD1325 support using the Realtek common interrupt
    controller driver
  irqchip: Introduce RTD1619B support using the Realtek common interrupt
    controller driver

 .../interrupt-controller/realtek,intc.yaml    |  76 ++++++
 drivers/irqchip/Kconfig                       |  28 +++
 drivers/irqchip/Makefile                      |   5 +
 drivers/irqchip/irq-realtek-intc-common.c     | 208 ++++++++++++++++
 drivers/irqchip/irq-realtek-intc-common.h     |  77 ++++++
 drivers/irqchip/irq-realtek-rtd1319.c         | 218 +++++++++++++++++
 drivers/irqchip/irq-realtek-rtd1319d.c        | 227 ++++++++++++++++++
 drivers/irqchip/irq-realtek-rtd1325.c         | 227 ++++++++++++++++++
 drivers/irqchip/irq-realtek-rtd1619b.c        | 217 +++++++++++++++++
 9 files changed, 1283 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml
 create mode 100644 drivers/irqchip/irq-realtek-intc-common.c
 create mode 100644 drivers/irqchip/irq-realtek-intc-common.h
 create mode 100644 drivers/irqchip/irq-realtek-rtd1319.c
 create mode 100644 drivers/irqchip/irq-realtek-rtd1319d.c
 create mode 100644 drivers/irqchip/irq-realtek-rtd1325.c
 create mode 100644 drivers/irqchip/irq-realtek-rtd1619b.c

-- 
2.25.1

[PATCH v3 1/6] dt-bindings: interrupt-controller: Add support for Realtek DHC SoCs

[James Tai]

Add the YAML documentation for Realtek DHC (Digital Home Center) SoCs.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311180921.ayKhiFHL-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: Rob Herring <robh+dt@kernel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: Conor Dooley <conor+dt@kernel.org>
CC: linux-kernel@vger.kernel.org
CC: devicetree@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Retested the bindings using the new version of the dtschema
- Fixed the order of property items
- Removed redundant files and replaced them with 'realtek,intc.yaml'
- Replaced 'interrupts-extended' with 'interrupts'
- Added a description for 'interrupts'
- Reduced the example code

v1 to v2 change:
- Tested the bindings using 'make dt_binding_check'
- Fixed code style issues

 .../interrupt-controller/realtek,intc.yaml    | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml
new file mode 100644
index 000000000000..3aa863b1549d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/realtek,intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek DHC SoCs Interrupt Controller
+
+maintainers:
+  - James Tai <james.tai@realtek.com>
+
+description:
+  This interrupt controller is a component of Realtek DHC (Digital Home Center)
+  SoCs and is designed to receive interrupts from peripheral devices.
+
+  Each DHC SoC has two sets of interrupt controllers, each capable of
+  handling up to 32 interrupts.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd1319-intc-iso
+      - realtek,rtd1319-intc-misc
+      - realtek,rtd1319d-intc-iso
+      - realtek,rtd1319d-intc-misc
+      - realtek,rtd1325-intc-iso
+      - realtek,rtd1325-intc-misc
+      - realtek,rtd1619b-intc-iso
+      - realtek,rtd1619b-intc-misc
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 1
+
+  '#address-cells':
+    const: 0
+
+  interrupts:
+    description: |
+      Contains the GIC SPI IRQs mapped to the external interrupt lines.
+    minItems: 2
+    maxItems: 4
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - '#interrupt-cells'
+  - '#address-cells'
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    realtek_iso_intc: interrupt-controller@40 {
+      compatible = "realtek,rtd1319-intc-iso";
+      reg = <0x00 0x40>;
+      interrupt-parent = <&gic>;
+      interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+      interrupt-controller;
+      #address-cells = <0>;
+      #interrupt-cells = <1>;
+    };
+...
-- 
2.25.1

[PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai]

Realtek DHC (Digital Home Center) SoCs share a common interrupt controller
design. This universal interrupt controller driver provides support for
various variants within the Realtek DHC SoC family.

Each DHC SoC features two sets of extended interrupt controllers, each
capable of handling up to 32 interrupts. These expansion controllers are
connected to the GIC (Generic Interrupt Controller).

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Resolved kernel test robot build warnings
  https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/

v1 to v2 change:
- Fixed code style issues
- Removed the realtek_intc_set_affinity funcation
- Replaced spin_lock_irqsave with raw_spin_lock

 drivers/irqchip/Kconfig                   |   4 +
 drivers/irqchip/Makefile                  |   1 +
 drivers/irqchip/irq-realtek-intc-common.c | 208 ++++++++++++++++++++++
 drivers/irqchip/irq-realtek-intc-common.h |  77 ++++++++
 4 files changed, 290 insertions(+)
 create mode 100644 drivers/irqchip/irq-realtek-intc-common.c
 create mode 100644 drivers/irqchip/irq-realtek-intc-common.h

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f7149d0f3d45..267c3429b48d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -218,6 +218,10 @@ config RDA_INTC
 	bool
 	select IRQ_DOMAIN
 
+config REALTEK_DHC_INTC
+	tristate
+	select IRQ_DOMAIN
+
 config RENESAS_INTC_IRQPIN
 	bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index ffd945fe71aa..f6774af7fde2 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_IRQ_MIPS_CPU)		+= irq-mips-cpu.o
 obj-$(CONFIG_IXP4XX_IRQ)		+= irq-ixp4xx.o
 obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
+obj-$(CONFIG_REALTEK_DHC_INTC)		+= irq-realtek-intc-common.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
diff --git a/drivers/irqchip/irq-realtek-intc-common.c b/drivers/irqchip/irq-realtek-intc-common.c
new file mode 100644
index 000000000000..64d656e5a015
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-intc-common.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek DHC SoCs interrupt controller driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include "irq-realtek-intc-common.h"
+
+struct realtek_intc_data;
+
+static inline unsigned int realtek_intc_get_ints(struct realtek_intc_data *data)
+{
+	return readl(data->base + data->info->isr_offset);
+}
+
+static inline void realtek_intc_clear_ints_bit(struct realtek_intc_data *data, int bit)
+{
+	writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
+}
+
+static inline unsigned int realtek_intc_get_inte(struct realtek_intc_data *data)
+{
+	unsigned int val;
+
+	raw_spin_lock(&data->lock);
+	val = readl(data->base + data->info->scpu_int_en_offset);
+	raw_spin_unlock(&data->lock);
+
+	return val;
+}
+
+static void realtek_intc_handler(struct irq_desc *desc)
+{
+	struct realtek_intc_subset_data *subset_data = irq_desc_get_handler_data(desc);
+	struct realtek_intc_data *data = subset_data->common;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 ints, inte, mask;
+	int irq;
+
+	chained_irq_enter(chip, desc);
+
+	ints = realtek_intc_get_ints(data) & subset_data->cfg->ints_mask;
+	inte = realtek_intc_get_inte(data);
+
+	while (ints) {
+		irq = __ffs(ints);
+		ints &= ~BIT(irq);
+
+		mask = data->info->isr_to_scpu_int_en_mask[irq];
+		if (mask != IRQ_ALWAYS_ENABLED && !(inte & mask))
+			continue;
+
+		generic_handle_irq(irq_find_mapping(data->domain, irq));
+		realtek_intc_clear_ints_bit(data, irq);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void realtek_intc_mask_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+
+	writel(BIT(data->hwirq), intc_data->base + intc_data->info->isr_offset);
+}
+
+static void realtek_intc_unmask_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+
+	writel(BIT(data->hwirq), intc_data->base + intc_data->info->umsk_isr_offset);
+}
+
+static void realtek_intc_enable_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+	u32 scpu_int_en, mask;
+
+	mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq];
+	if (!mask)
+		return;
+
+	raw_spin_lock(&intc_data->lock);
+	scpu_int_en = readl(intc_data->base + intc_data->info->scpu_int_en_offset);
+	scpu_int_en |= mask;
+	writel(scpu_int_en, intc_data->base + intc_data->info->umsk_isr_offset);
+	raw_spin_unlock(&intc_data->lock);
+}
+
+static void realtek_intc_disable_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+	u32 scpu_int_en, mask;
+
+	mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq];
+	if (!mask)
+		return;
+
+	raw_spin_lock(&intc_data->lock);
+	scpu_int_en = readl(intc_data->base + intc_data->info->scpu_int_en_offset);
+	scpu_int_en &= ~mask;
+	writel(scpu_int_en, intc_data->base + intc_data->info->umsk_isr_offset);
+	raw_spin_unlock(&intc_data->lock);
+}
+
+static struct irq_chip realtek_intc_chip = {
+	.name		  = "realtek-intc",
+	.irq_mask	  = realtek_intc_mask_irq,
+	.irq_unmask	  = realtek_intc_unmask_irq,
+	.irq_enable	  = realtek_intc_enable_irq,
+	.irq_disable	  = realtek_intc_disable_irq,
+};
+
+static int realtek_intc_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
+{
+	struct realtek_intc_data *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);
+	irq_set_chip_data(irq, data);
+	irq_set_probe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops realtek_intc_domain_ops = {
+	.xlate = irq_domain_xlate_onecell,
+	.map = realtek_intc_domain_map,
+};
+
+static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
+{
+	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
+	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
+	int irq;
+
+	irq = irq_of_parse_and_map(node, index);
+	if (irq <= 0)
+		return irq;
+
+	subset_data->common = data;
+	subset_data->cfg = cfg;
+	subset_data->parent_irq = irq;
+	irq_set_chained_handler_and_data(irq, realtek_intc_handler, subset_data);
+
+	return 0;
+}
+
+int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
+{
+	struct realtek_intc_data *data;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	int ret, i;
+
+	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->base = of_iomap(node, 0);
+	if (!data->base) {
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	data->info = info;
+
+	raw_spin_lock_init(&data->lock);
+
+	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);
+	if (!data->domain) {
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	data->subset_data_num = info->cfg_num;
+	for (i = 0; i < info->cfg_num; i++) {
+		ret = realtek_intc_subset(node, data, i);
+		if (ret) {
+			WARN(ret, "failed to init subset %d: %d", i, ret);
+			ret = -ENOMEM;
+			goto out_cleanup;
+		}
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+
+out_cleanup:
+
+	if (data->base)
+		iounmap(data->base);
+
+	devm_kfree(dev, data);
+
+	return ret;
+}
+EXPORT_SYMBOL(realtek_intc_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Realtek DHC SoC Interrupt Controller Driver");
diff --git a/drivers/irqchip/irq-realtek-intc-common.h b/drivers/irqchip/irq-realtek-intc-common.h
new file mode 100644
index 000000000000..38be116dba60
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-intc-common.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#ifndef _IRQ_REALTEK_COMMON_H
+#define _IRQ_REALTEK_COMMON_H
+
+#include <linux/bits.h>
+#include <linux/limits.h>
+#include <linux/hwspinlock.h>
+
+/**
+ * realtek_intc_subset_cfg - subset interrupt mask
+ * @ints_mask: inetrrupt mask
+ */
+struct realtek_intc_subset_cfg {
+	unsigned int	ints_mask;
+};
+
+/**
+ * realtek_intc_info - interrupt controller data.
+ * @isr_offset: interrupt status register offset.
+ * @umsk_isr_offset: unmask interrupt status register offset.
+ * @scpu_int_en_offset: interrupt enable register offset.
+ * @cfg: cfg of the subset.
+ * @cfg_num: number of cfg.
+ */
+struct realtek_intc_info {
+	const struct realtek_intc_subset_cfg *cfg;
+	unsigned int			     isr_offset;
+	unsigned int			     umsk_isr_offset;
+	unsigned int			     scpu_int_en_offset;
+	const u32			     *isr_to_scpu_int_en_mask;
+	int				     cfg_num;
+};
+
+/**
+ * realtek_intc_subset_data - handler of a interrupt source only handles ints
+ *                            bits in the mask.
+ * @cfg: cfg of the subset.
+ * @common: common data.
+ * @parent_irq: interrupt source.
+ */
+struct realtek_intc_subset_data {
+	const struct realtek_intc_subset_cfg *cfg;
+	struct realtek_intc_data	     *common;
+	int				     parent_irq;
+};
+
+/**
+ * realtek_intc_data - configuration data for realtek interrupt controller driver.
+ * @base: base of interrupt register
+ * @info: info of intc
+ * @domain: interrupt domain
+ * @lock: lock
+ * @saved_en: status of interrupt enable
+ * @subset_data_num: number of subset data
+ * @subset_data: subset data
+ */
+struct realtek_intc_data {
+	void __iomem			*base;
+	const struct realtek_intc_info	*info;
+	struct irq_domain		*domain;
+	struct raw_spinlock		lock;
+	unsigned int			saved_en;
+	int				subset_data_num;
+	struct realtek_intc_subset_data subset_data[];
+};
+
+#define IRQ_ALWAYS_ENABLED U32_MAX
+#define DISABLE_INTC (0)
+#define CLEAN_INTC_STATUS GENMASK(31, 1)
+
+int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info);
+
+#endif /* _IRQ_REALTEK_COMMON_H */
-- 
2.25.1

[PATCH v3 3/6] irqchip: Introduce RTD1319 support using the Realtek common interrupt controller driver

[James Tai]

Add support for the RTD1319 platform.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311061208.hJmxGqym-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Unchanged

v1 to v2 change:
- Resolved kernel test robot build warnings
- Replaced magic number with macro
- Fixed code style issues

 drivers/irqchip/Kconfig               |   6 +
 drivers/irqchip/Makefile              |   1 +
 drivers/irqchip/irq-realtek-rtd1319.c | 218 ++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/irqchip/irq-realtek-rtd1319.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 267c3429b48d..05856ce885fa 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -222,6 +222,12 @@ config REALTEK_DHC_INTC
 	tristate
 	select IRQ_DOMAIN
 
+config REALTEK_RTD1319_INTC
+	tristate "Realtek RTD1319 interrupt controller"
+	select REALTEK_DHC_INTC
+	help
+	  Support for Realtek RTD1319 Interrupt Controller.
+
 config RENESAS_INTC_IRQPIN
 	bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index f6774af7fde2..6a2650b0a924 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_IXP4XX_IRQ)		+= irq-ixp4xx.o
 obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
 obj-$(CONFIG_REALTEK_DHC_INTC)		+= irq-realtek-intc-common.o
+obj-$(CONFIG_REALTEK_RTD1319_INTC)	+= irq-realtek-rtd1319.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
diff --git a/drivers/irqchip/irq-realtek-rtd1319.c b/drivers/irqchip/irq-realtek-rtd1319.c
new file mode 100644
index 000000000000..23c13c218b04
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-rtd1319.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek RTD1319 interrupt controller driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "irq-realtek-intc-common.h"
+
+#define ISO_NORMAL_MASK      0xffffcffe
+#define ISO_RTC_MASK         0x00003001
+#define MISC_NMI_WDT_MASK    0x00000004
+#define MISC_NORMAL_MASK     0xffffc0d2
+#define MISC_UART1_MASK      0x00000028
+#define MISC_UART2_MASK      0x00002100
+
+#define ISO_ISR_EN_OFFSET    0x40
+#define ISO_ISR_OFFSET       0
+#define ISO_ISR_UMSK_OFFSET  0x4
+#define MISC_ISR_EN_OFFSET   0x80
+#define MISC_ISR_OFFSET      0xc
+#define MISC_ISR_UMSK_OFFSET 0x8
+
+enum rtd1319_iso_isr_bits {
+	RTD1319_ISO_ISR_TC3_SHIFT	 = 1,
+	RTD1319_ISO_ISR_UR0_SHIFT	 = 2,
+	RTD1319_ISO_ISR_LSADC0_SHIFT	 = 3,
+	RTD1319_ISO_ISR_IRDA_SHIFT	 = 5,
+	RTD1319_ISO_ISR_SPI1_SHIFT	 = 6,
+	RTD1319_ISO_ISR_WDOG_NMI_SHIFT	 = 7,
+	RTD1319_ISO_ISR_I2C0_SHIFT	 = 8,
+	RTD1319_ISO_ISR_TC4_SHIFT	 = 9,
+	RTD1319_ISO_ISR_TC7_SHIFT	 = 10,
+	RTD1319_ISO_ISR_I2C1_SHIFT	 = 11,
+	RTD1319_ISO_ISR_RTC_HSEC_SHIFT	 = 12,
+	RTD1319_ISO_ISR_RTC_ALARM_SHIFT	 = 13,
+	RTD1319_ISO_ISR_GPIOA_SHIFT	 = 19,
+	RTD1319_ISO_ISR_GPIODA_SHIFT	 = 20,
+	RTD1319_ISO_ISR_ISO_MISC_SHIFT	 = 21,
+	RTD1319_ISO_ISR_CBUS_SHIFT	 = 22,
+	RTD1319_ISO_ISR_ETN_SHIFT	 = 23,
+	RTD1319_ISO_ISR_USB_HOST_SHIFT	 = 24,
+	RTD1319_ISO_ISR_USB_U3_DRD_SHIFT = 25,
+	RTD1319_ISO_ISR_USB_U2_DRD_SHIFT = 26,
+	RTD1319_ISO_ISR_PORB_HV_SHIFT	 = 28,
+	RTD1319_ISO_ISR_PORB_DV_SHIFT	 = 29,
+	RTD1319_ISO_ISR_PORB_AV_SHIFT	 = 30,
+	RTD1319_ISO_ISR_I2C1_REQ_SHIFT	 = 31,
+};
+
+static const u32 rtd1319_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1319_ISO_ISR_SPI1_SHIFT]	  = BIT(1),
+	[RTD1319_ISO_ISR_UR0_SHIFT]	  = BIT(2),
+	[RTD1319_ISO_ISR_LSADC0_SHIFT]	  = BIT(3),
+	[RTD1319_ISO_ISR_IRDA_SHIFT]	  = BIT(5),
+	[RTD1319_ISO_ISR_I2C0_SHIFT]	  = BIT(8),
+	[RTD1319_ISO_ISR_I2C1_SHIFT]	  = BIT(11),
+	[RTD1319_ISO_ISR_RTC_HSEC_SHIFT]  = BIT(12),
+	[RTD1319_ISO_ISR_RTC_ALARM_SHIFT] = BIT(13),
+	[RTD1319_ISO_ISR_GPIOA_SHIFT]	  = BIT(19),
+	[RTD1319_ISO_ISR_GPIODA_SHIFT]	  = BIT(20),
+	[RTD1319_ISO_ISR_PORB_HV_SHIFT]	  = BIT(28),
+	[RTD1319_ISO_ISR_PORB_DV_SHIFT]	  = BIT(29),
+	[RTD1319_ISO_ISR_PORB_AV_SHIFT]	  = BIT(30),
+	[RTD1319_ISO_ISR_I2C1_REQ_SHIFT]  = BIT(31),
+};
+
+enum rtd1319_misc_isr_bits {
+	RTD1319_ISR_WDOG_NMI_SHIFT = 2,
+	RTD1319_ISR_UR1_SHIFT	   = 3,
+	RTD1319_ISR_TC5_SHIFT	   = 4,
+	RTD1319_ISR_UR1_TO_SHIFT   = 5,
+	RTD1319_ISR_TC0_SHIFT	   = 6,
+	RTD1319_ISR_TC1_SHIFT	   = 7,
+	RTD1319_ISR_UR2_SHIFT	   = 8,
+	RTD1319_ISR_RTC_HSEC_SHIFT = 9,
+	RTD1319_ISR_RTC_MIN_SHIFT  = 10,
+	RTD1319_ISR_RTC_HOUR_SHIFT = 11,
+	RTD1319_ISR_RTC_DATE_SHIFT = 12,
+	RTD1319_ISR_UR2_TO_SHIFT   = 13,
+	RTD1319_ISR_I2C5_SHIFT	   = 14,
+	RTD1319_ISR_I2C3_SHIFT	   = 23,
+	RTD1319_ISR_SC0_SHIFT	   = 24,
+	RTD1319_ISR_SC1_SHIFT	   = 25,
+	RTD1319_ISR_SPI_SHIFT	   = 27,
+	RTD1319_ISR_FAN_SHIFT	   = 29,
+};
+
+static const u32 rtd1319_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1319_ISR_UR1_SHIFT]	     = BIT(3),
+	[RTD1319_ISR_UR1_TO_SHIFT]   = BIT(5),
+	[RTD1319_ISR_UR2_TO_SHIFT]   = BIT(6),
+	[RTD1319_ISR_UR2_SHIFT]	     = BIT(7),
+	[RTD1319_ISR_RTC_MIN_SHIFT]  = BIT(10),
+	[RTD1319_ISR_RTC_HOUR_SHIFT] = BIT(11),
+	[RTD1319_ISR_RTC_DATE_SHIFT] = BIT(12),
+	[RTD1319_ISR_I2C5_SHIFT]     = BIT(14),
+	[RTD1319_ISR_SC0_SHIFT]	     = BIT(24),
+	[RTD1319_ISR_SC1_SHIFT]	     = BIT(25),
+	[RTD1319_ISR_SPI_SHIFT]	     = BIT(27),
+	[RTD1319_ISR_I2C3_SHIFT]     = BIT(28),
+	[RTD1319_ISR_FAN_SHIFT]	     = BIT(29),
+	[RTD1319_ISR_WDOG_NMI_SHIFT] = IRQ_ALWAYS_ENABLED,
+};
+
+static struct realtek_intc_subset_cfg rtd1319_intc_iso_cfgs[] = {
+	{ ISO_NORMAL_MASK, },
+	{ ISO_RTC_MASK, },
+};
+
+static const struct realtek_intc_info rtd1319_intc_iso_info = {
+	.isr_offset		 = ISO_ISR_OFFSET,
+	.umsk_isr_offset	 = ISO_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = ISO_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1319_iso_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1319_intc_iso_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1319_intc_iso_cfgs),
+};
+
+static struct realtek_intc_subset_cfg rtd1319_intc_misc_cfgs[] = {
+	{ MISC_NORMAL_MASK, },
+	{ MISC_NMI_WDT_MASK, },
+	{ MISC_UART1_MASK, },
+	{ MISC_UART2_MASK, },
+};
+
+static const struct realtek_intc_info rtd1319_intc_misc_info = {
+	.isr_offset		 = MISC_ISR_OFFSET,
+	.umsk_isr_offset	 = MISC_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = MISC_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1319_misc_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1319_intc_misc_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1319_intc_misc_cfgs),
+};
+
+static const struct of_device_id realtek_intc_rtd1319_dt_matches[] = {
+	{
+		.compatible = "realtek,rtd1319-intc-iso",
+		.data = &rtd1319_intc_iso_info,
+	}, {
+		.compatible = "realtek,rtd1319-intc-misc",
+		.data = &rtd1319_intc_misc_info,
+	},
+	{ /* sentinel */ }
+};
+
+static int realtek_intc_rtd1319_suspend(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	data->saved_en = readl(data->base + info->scpu_int_en_offset);
+
+	writel(DISABLE_INTC, data->base + info->scpu_int_en_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+
+	return 0;
+}
+
+static int realtek_intc_rtd1319_resume(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+	writel(data->saved_en, data->base + info->scpu_int_en_offset);
+
+	return 0;
+}
+
+static const struct dev_pm_ops realtek_intc_rtd1319_pm_ops = {
+	.suspend_noirq = realtek_intc_rtd1319_suspend,
+	.resume_noirq  = realtek_intc_rtd1319_resume,
+};
+
+static int rtd1319_intc_probe(struct platform_device *pdev)
+{
+	const struct realtek_intc_info *info;
+
+	info = of_device_get_match_data(&pdev->dev);
+	if (!info)
+		return -EINVAL;
+
+	return realtek_intc_probe(pdev, info);
+}
+
+static struct platform_driver realtek_intc_rtd1319_driver = {
+	.probe = rtd1319_intc_probe,
+	.driver = {
+		.name = "realtek_intc_rtd1319",
+		.of_match_table = realtek_intc_rtd1319_dt_matches,
+		.suppress_bind_attrs = true,
+		.pm = &realtek_intc_rtd1319_pm_ops,
+	},
+};
+
+static int __init realtek_intc_rtd1319_init(void)
+{
+	return platform_driver_register(&realtek_intc_rtd1319_driver);
+}
+core_initcall(realtek_intc_rtd1319_init);
+
+static void __exit realtek_intc_rtd1319_exit(void)
+{
+	platform_driver_unregister(&realtek_intc_rtd1319_driver);
+}
+module_exit(realtek_intc_rtd1319_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Realtek RTD1319 Interrupt Controller Driver");
-- 
2.25.1

[PATCH v3 4/6] irqchip: Introduce RTD1319D support using the Realtek common interrupt controller driver

[James Tai]

Add support for the RTD1319D platform.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311061137.FRpoKwcm-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Unchanged

v1 to v2 change:
- Resolved kernel test robot build warnings
- Replaced magic number with macro
- Fixed code style issues

 drivers/irqchip/Kconfig                |   6 +
 drivers/irqchip/Makefile               |   1 +
 drivers/irqchip/irq-realtek-rtd1319d.c | 227 +++++++++++++++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 drivers/irqchip/irq-realtek-rtd1319d.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 05856ce885fa..c6552c513442 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -228,6 +228,12 @@ config REALTEK_RTD1319_INTC
 	help
 	  Support for Realtek RTD1319 Interrupt Controller.
 
+config REALTEK_RTD1319D_INTC
+	tristate "Realtek RTD1319D interrupt controller"
+	select REALTEK_DHC_INTC
+	help
+	  Support for Realtek RTD1319D Interrupt Controller.
+
 config RENESAS_INTC_IRQPIN
 	bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 6a2650b0a924..c8adaed4c1b2 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
 obj-$(CONFIG_REALTEK_DHC_INTC)		+= irq-realtek-intc-common.o
 obj-$(CONFIG_REALTEK_RTD1319_INTC)	+= irq-realtek-rtd1319.o
+obj-$(CONFIG_REALTEK_RTD1319D_INTC)	+= irq-realtek-rtd1319d.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
diff --git a/drivers/irqchip/irq-realtek-rtd1319d.c b/drivers/irqchip/irq-realtek-rtd1319d.c
new file mode 100644
index 000000000000..2d4bd6230e17
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-rtd1319d.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek RTD1319D interrupt controller driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "irq-realtek-intc-common.h"
+
+#define ISO_NMI_WDT_MASK     0x08008090
+#define ISO_NORMAL_MASK      0xf7ff7f6e
+#define MISC_NORMAL_MASK     0xffe0ded6
+#define MISC_UART1_MASK      0x00000028
+#define MISC_UART2_MASK      0x00002100
+
+#define ISO_ISR_EN_OFFSET    0x40
+#define ISO_ISR_OFFSET       0
+#define ISO_ISR_UMSK_OFFSET  0x4
+#define MISC_ISR_EN_OFFSET   0x80
+#define MISC_ISR_OFFSET      0xc
+#define MISC_ISR_UMSK_OFFSET 0x8
+
+enum rtd1319d_iso_isr_bits {
+	RTD1319D_ISO_ISR_TC3_SHIFT	   = 1,
+	RTD1319D_ISO_ISR_UR0_SHIFT	   = 2,
+	RTD1319D_ISO_ISR_LSADC0_SHIFT	   = 3,
+	RTD1319D_ISO_ISR_WDOG1_NMI_SHIFT   = 4,
+	RTD1319D_ISO_ISR_IRDA_SHIFT	   = 5,
+	RTD1319D_ISO_ISR_SPI1_SHIFT	   = 6,
+	RTD1319D_ISO_ISR_WDOG2_NMI_SHIFT   = 7,
+	RTD1319D_ISO_ISR_I2C0_SHIFT	   = 8,
+	RTD1319D_ISO_ISR_TC4_SHIFT	   = 9,
+	RTD1319D_ISO_ISR_TC7_SHIFT	   = 10,
+	RTD1319D_ISO_ISR_I2C1_SHIFT	   = 11,
+	RTD1319D_ISO_ISR_HIFI_WAKEUP_SHIFT = 14,
+	RTD1319D_ISO_ISR_WDOG4_NMI_SHIFT   = 15,
+	RTD1319D_ISO_ISR_TC8_SHIFT	   = 16,
+	RTD1319D_ISO_ISR_VFD_SHIFT	   = 17,
+	RTD1319D_ISO_ISR_VTC_SHIFT	   = 18,
+	RTD1319D_ISO_ISR_GPIOA_SHIFT	   = 19,
+	RTD1319D_ISO_ISR_GPIODA_SHIFT	   = 20,
+	RTD1319D_ISO_ISR_ISO_MISC_SHIFT	   = 21,
+	RTD1319D_ISO_ISR_CBUS_SHIFT	   = 22,
+	RTD1319D_ISO_ISR_ETN_SHIFT	   = 23,
+	RTD1319D_ISO_ISR_USB_HOST_SHIFT	   = 24,
+	RTD1319D_ISO_ISR_USB_U3_DRD_SHIFT  = 25,
+	RTD1319D_ISO_ISR_USB_U2_DRD_SHIFT  = 26,
+	RTD1319D_ISO_ISR_WDOG3_NMI_SHIFT   = 27,
+	RTD1319D_ISO_ISR_PORB_HV_CEN_SHIFT = 28,
+	RTD1319D_ISO_ISR_PORB_DV_CEN_SHIFT = 29,
+	RTD1319D_ISO_ISR_PORB_AV_CEN_SHIFT = 30,
+	RTD1319D_ISO_ISR_I2C1_REQ_SHIFT	   = 31,
+};
+
+static const u32 rtd1319d_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1319D_ISO_ISR_SPI1_SHIFT]	     = BIT(1),
+	[RTD1319D_ISO_ISR_UR0_SHIFT]	     = BIT(2),
+	[RTD1319D_ISO_ISR_LSADC0_SHIFT]	     = BIT(3),
+	[RTD1319D_ISO_ISR_IRDA_SHIFT]	     = BIT(5),
+	[RTD1319D_ISO_ISR_I2C0_SHIFT]	     = BIT(8),
+	[RTD1319D_ISO_ISR_I2C1_SHIFT]	     = BIT(11),
+	[RTD1319D_ISO_ISR_VFD_SHIFT]	     = BIT(17),
+	[RTD1319D_ISO_ISR_GPIOA_SHIFT]	     = BIT(19),
+	[RTD1319D_ISO_ISR_GPIODA_SHIFT]	     = BIT(20),
+	[RTD1319D_ISO_ISR_PORB_HV_CEN_SHIFT] = BIT(28),
+	[RTD1319D_ISO_ISR_PORB_DV_CEN_SHIFT] = BIT(29),
+	[RTD1319D_ISO_ISR_PORB_AV_CEN_SHIFT] = BIT(30),
+	[RTD1319D_ISO_ISR_I2C1_REQ_SHIFT]    = BIT(31),
+	[RTD1319D_ISO_ISR_WDOG1_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1319D_ISO_ISR_WDOG2_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1319D_ISO_ISR_WDOG3_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1319D_ISO_ISR_WDOG4_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+};
+
+enum rtd1319d_misc_isr_bits {
+	RTD1319D_ISR_UR1_SHIFT	      = 3,
+	RTD1319D_ISR_TC5_SHIFT	      = 4,
+	RTD1319D_ISR_UR1_TO_SHIFT     = 5,
+	RTD1319D_ISR_TC0_SHIFT	      = 6,
+	RTD1319D_ISR_TC1_SHIFT	      = 7,
+	RTD1319D_ISR_UR2_SHIFT	      = 8,
+	RTD1319D_ISR_UR2_TO_SHIFT     = 13,
+	RTD1319D_ISR_I2C5_SHIFT	      = 14,
+	RTD1319D_ISR_I2C4_SHIFT	      = 15,
+	RTD1319D_ISR_DRTC_HSEC_SHIFT  = 16,
+	RTD1319D_ISR_DRTC_MIN_SHIFT   = 17,
+	RTD1319D_ISR_DRTC_HOUR_SHIFT  = 18,
+	RTD1319D_ISR_DRTC_DATE_SHIFT  = 19,
+	RTD1319D_ISR_DRTC_ALARM_SHIFT = 20,
+	RTD1319D_ISR_I2C3_SHIFT	      = 23,
+	RTD1319D_ISR_SC0_SHIFT	      = 24,
+	RTD1319D_ISR_SC1_SHIFT	      = 25,
+	RTD1319D_ISR_SPI_SHIFT	      = 27,
+	RTD1319D_ISR_FAN_SHIFT	      = 29,
+};
+
+static const u32 rtd1319d_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1319D_ISR_UR1_SHIFT]	= BIT(3),
+	[RTD1319D_ISR_UR1_TO_SHIFT]	= BIT(5),
+	[RTD1319D_ISR_UR2_TO_SHIFT]	= BIT(6),
+	[RTD1319D_ISR_UR2_SHIFT]	= BIT(7),
+	[RTD1319D_ISR_I2C5_SHIFT]	= BIT(14),
+	[RTD1319D_ISR_I2C4_SHIFT]	= BIT(15),
+	[RTD1319D_ISR_DRTC_HSEC_SHIFT]	= BIT(16),
+	[RTD1319D_ISR_DRTC_MIN_SHIFT]	= BIT(17),
+	[RTD1319D_ISR_DRTC_HOUR_SHIFT]	= BIT(18),
+	[RTD1319D_ISR_DRTC_DATE_SHIFT]	= BIT(19),
+	[RTD1319D_ISR_DRTC_ALARM_SHIFT] = BIT(20),
+	[RTD1319D_ISR_SC0_SHIFT]	= BIT(24),
+	[RTD1319D_ISR_SC1_SHIFT]	= BIT(25),
+	[RTD1319D_ISR_SPI_SHIFT]	= BIT(27),
+	[RTD1319D_ISR_I2C3_SHIFT]	= BIT(28),
+	[RTD1319D_ISR_FAN_SHIFT]	= BIT(29),
+};
+
+static struct realtek_intc_subset_cfg rtd1319d_intc_iso_cfgs[] = {
+	{ ISO_NORMAL_MASK, },
+	{ ISO_NMI_WDT_MASK, },
+};
+
+static const struct realtek_intc_info rtd1319d_intc_iso_info = {
+	.isr_offset		 = ISO_ISR_OFFSET,
+	.umsk_isr_offset	 = ISO_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = ISO_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1319d_iso_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1319d_intc_iso_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1319d_intc_iso_cfgs),
+};
+
+static struct realtek_intc_subset_cfg rtd1319d_intc_misc_cfgs[] = {
+	{ MISC_NORMAL_MASK, },
+	{ MISC_UART1_MASK, },
+	{ MISC_UART2_MASK, },
+};
+
+static const struct realtek_intc_info rtd1319d_intc_misc_info = {
+	.isr_offset		 = MISC_ISR_OFFSET,
+	.umsk_isr_offset	 = MISC_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = MISC_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1319d_misc_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1319d_intc_misc_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1319d_intc_misc_cfgs),
+};
+
+static const struct of_device_id realtek_intc_rtd1319d_dt_matches[] = {
+	{
+		.compatible = "realtek,rtd1319d-intc-iso",
+		.data = &rtd1319d_intc_iso_info,
+	}, {
+		.compatible = "realtek,rtd1319d-intc-misc",
+		.data = &rtd1319d_intc_misc_info,
+	},
+	{ /* sentinel */ }
+};
+
+static int realtek_intc_rtd1319d_suspend(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	data->saved_en = readl(data->base + info->scpu_int_en_offset);
+
+	writel(DISABLE_INTC, data->base + info->scpu_int_en_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+
+	return 0;
+}
+
+static int realtek_intc_rtd1319d_resume(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+	writel(data->saved_en, data->base + info->scpu_int_en_offset);
+
+	return 0;
+}
+
+static const struct dev_pm_ops realtek_intc_rtd1319d_pm_ops = {
+	.suspend_noirq = realtek_intc_rtd1319d_suspend,
+	.resume_noirq  = realtek_intc_rtd1319d_resume,
+};
+
+static int rtd1319d_intc_probe(struct platform_device *pdev)
+{
+	const struct realtek_intc_info *info;
+
+	info = of_device_get_match_data(&pdev->dev);
+	if (!info)
+		return -EINVAL;
+
+	return realtek_intc_probe(pdev, info);
+}
+
+static struct platform_driver realtek_intc_rtd1319d_driver = {
+	.probe = rtd1319d_intc_probe,
+	.driver = {
+		.name = "realtek_intc_rtd1319d",
+		.of_match_table = realtek_intc_rtd1319d_dt_matches,
+		.suppress_bind_attrs = true,
+		.pm = &realtek_intc_rtd1319d_pm_ops,
+	},
+};
+
+static int __init realtek_intc_rtd1319d_init(void)
+{
+	return platform_driver_register(&realtek_intc_rtd1319d_driver);
+}
+core_initcall(realtek_intc_rtd1319d_init);
+
+static void __exit realtek_intc_rtd1319d_exit(void)
+{
+	platform_driver_unregister(&realtek_intc_rtd1319d_driver);
+}
+module_exit(realtek_intc_rtd1319d_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Realtek RTD1319D Interrupt Controller Driver");
-- 
2.25.1

[PATCH v3 5/6] irqchip: Introduce RTD1325 support using the Realtek common interrupt controller driver

[James Tai]

Add support for the RTD1325 platform.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311061408.qjl1jfVl-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Unchanged

v1 to v2 change:
- Resolved kernel test robot build warnings
- Replaced magic number with macro
- Fixed code style issues

 drivers/irqchip/Kconfig               |   6 +
 drivers/irqchip/Makefile              |   1 +
 drivers/irqchip/irq-realtek-rtd1325.c | 227 ++++++++++++++++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 drivers/irqchip/irq-realtek-rtd1325.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c6552c513442..65e2d67d1505 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -234,6 +234,12 @@ config REALTEK_RTD1319D_INTC
 	help
 	  Support for Realtek RTD1319D Interrupt Controller.
 
+config REALTEK_RTD1325_INTC
+	tristate "Realtek RTD1325 interrupt controller"
+	select REALTEK_DHC_INTC
+	help
+	  Support for Realtek RTD1325 Interrupt Controller.
+
 config RENESAS_INTC_IRQPIN
 	bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index c8adaed4c1b2..eaa12928d60b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
 obj-$(CONFIG_REALTEK_DHC_INTC)		+= irq-realtek-intc-common.o
 obj-$(CONFIG_REALTEK_RTD1319_INTC)	+= irq-realtek-rtd1319.o
 obj-$(CONFIG_REALTEK_RTD1319D_INTC)	+= irq-realtek-rtd1319d.o
+obj-$(CONFIG_REALTEK_RTD1325_INTC)	+= irq-realtek-rtd1325.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
diff --git a/drivers/irqchip/irq-realtek-rtd1325.c b/drivers/irqchip/irq-realtek-rtd1325.c
new file mode 100644
index 000000000000..7ff164795634
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-rtd1325.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek RTD1325 interrupt controller driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "irq-realtek-intc-common.h"
+
+#define ISO_NMI_WDT_MASK     0x08008090
+#define ISO_NORMAL_MASK      0xf7ff7f6e
+#define MISC_NORMAL_MASK     0xffe0ded6
+#define MISC_UART1_MASK      0x00000028
+#define MISC_UART2_MASK      0x00002100
+
+#define ISO_ISR_EN_OFFSET    0x40
+#define ISO_ISR_OFFSET       0
+#define ISO_ISR_UMSK_OFFSET  0x4
+#define MISC_ISR_EN_OFFSET   0x80
+#define MISC_ISR_OFFSET      0xc
+#define MISC_ISR_UMSK_OFFSET 0x8
+
+enum rtd1325_iso_isr_bits {
+	RTD1325_ISO_ISR_TC3_SHIFT	  = 1,
+	RTD1325_ISO_ISR_UR0_SHIFT	  = 2,
+	RTD1325_ISO_ISR_LSADC0_SHIFT	  = 3,
+	RTD1325_ISO_ISR_WDOG1_NMI_SHIFT	  = 4,
+	RTD1325_ISO_ISR_IRDA_SHIFT	  = 5,
+	RTD1325_ISO_ISR_SPI1_SHIFT	  = 6,
+	RTD1325_ISO_ISR_WDOG2_NMI_SHIFT	  = 7,
+	RTD1325_ISO_ISR_I2C0_SHIFT	  = 8,
+	RTD1325_ISO_ISR_TC4_SHIFT	  = 9,
+	RTD1325_ISO_ISR_TC7_SHIFT	  = 10,
+	RTD1325_ISO_ISR_I2C1_SHIFT	  = 11,
+	RTD1325_ISO_ISR_HIFI_WAKEUP_SHIFT = 14,
+	RTD1325_ISO_ISR_WDOG4_NMI_SHIFT	  = 15,
+	RTD1325_ISO_ISR_TC8_SHIFT	  = 16,
+	RTD1325_ISO_ISR_VFD_SHIFT	  = 17,
+	RTD1325_ISO_ISR_VTC_SHIFT	  = 18,
+	RTD1325_ISO_ISR_GPIOA_SHIFT	  = 19,
+	RTD1325_ISO_ISR_GPIODA_SHIFT	  = 20,
+	RTD1325_ISO_ISR_ISO_MISC_SHIFT	  = 21,
+	RTD1325_ISO_ISR_CBUS_SHIFT	  = 22,
+	RTD1325_ISO_ISR_ETN_SHIFT	  = 23,
+	RTD1325_ISO_ISR_USB_HOST_SHIFT	  = 24,
+	RTD1325_ISO_ISR_USB_U3_DRD_SHIFT  = 25,
+	RTD1325_ISO_ISR_USB_U2_DRD_SHIFT  = 26,
+	RTD1325_ISO_ISR_WDOG3_NMI_SHIFT	  = 27,
+	RTD1325_ISO_ISR_PORB_HV_CEN_SHIFT = 28,
+	RTD1325_ISO_ISR_PORB_DV_CEN_SHIFT = 29,
+	RTD1325_ISO_ISR_PORB_AV_CEN_SHIFT = 30,
+	RTD1325_ISO_ISR_I2C1_REQ_SHIFT	  = 31,
+};
+
+static const u32 rtd1325_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1325_ISO_ISR_SPI1_SHIFT]	    = BIT(1),
+	[RTD1325_ISO_ISR_UR0_SHIFT]	    = BIT(2),
+	[RTD1325_ISO_ISR_LSADC0_SHIFT]	    = BIT(3),
+	[RTD1325_ISO_ISR_IRDA_SHIFT]	    = BIT(5),
+	[RTD1325_ISO_ISR_I2C0_SHIFT]	    = BIT(8),
+	[RTD1325_ISO_ISR_I2C1_SHIFT]	    = BIT(11),
+	[RTD1325_ISO_ISR_VFD_SHIFT]	    = BIT(17),
+	[RTD1325_ISO_ISR_GPIOA_SHIFT]	    = BIT(19),
+	[RTD1325_ISO_ISR_GPIODA_SHIFT]	    = BIT(20),
+	[RTD1325_ISO_ISR_PORB_HV_CEN_SHIFT] = BIT(28),
+	[RTD1325_ISO_ISR_PORB_DV_CEN_SHIFT] = BIT(29),
+	[RTD1325_ISO_ISR_PORB_AV_CEN_SHIFT] = BIT(30),
+	[RTD1325_ISO_ISR_I2C1_REQ_SHIFT]    = BIT(31),
+	[RTD1325_ISO_ISR_WDOG1_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1325_ISO_ISR_WDOG2_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1325_ISO_ISR_WDOG3_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1325_ISO_ISR_WDOG4_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+};
+
+enum rtd1325_misc_isr_bits {
+	RTD1325_ISR_UR1_SHIFT	     = 3,
+	RTD1325_ISR_TC5_SHIFT	     = 4,
+	RTD1325_ISR_UR1_TO_SHIFT     = 5,
+	RTD1325_ISR_TC0_SHIFT	     = 6,
+	RTD1325_ISR_TC1_SHIFT	     = 7,
+	RTD1325_ISR_UR2_SHIFT	     = 8,
+	RTD1325_ISR_UR2_TO_SHIFT     = 13,
+	RTD1325_ISR_I2C5_SHIFT	     = 14,
+	RTD1325_ISR_I2C4_SHIFT	     = 15,
+	RTD1325_ISR_DRTC_HSEC_SHIFT  = 16,
+	RTD1325_ISR_DRTC_MIN_SHIFT   = 17,
+	RTD1325_ISR_DRTC_HOUR_SHIFT  = 18,
+	RTD1325_ISR_DRTC_DATE_SHIFT  = 19,
+	RTD1325_ISR_DRTC_ALARM_SHIFT = 20,
+	RTD1325_ISR_I2C3_SHIFT	     = 23,
+	RTD1325_ISR_SC0_SHIFT	     = 24,
+	RTD1325_ISR_SC1_SHIFT	     = 25,
+	RTD1325_ISR_SPI_SHIFT	     = 27,
+	RTD1325_ISR_FAN_SHIFT	     = 29,
+};
+
+static const u32 rtd1325_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1325_ISR_UR1_SHIFT]	       = BIT(3),
+	[RTD1325_ISR_UR1_TO_SHIFT]     = BIT(5),
+	[RTD1325_ISR_UR2_TO_SHIFT]     = BIT(6),
+	[RTD1325_ISR_UR2_SHIFT]	       = BIT(7),
+	[RTD1325_ISR_I2C5_SHIFT]       = BIT(14),
+	[RTD1325_ISR_I2C4_SHIFT]       = BIT(15),
+	[RTD1325_ISR_DRTC_HSEC_SHIFT]  = BIT(16),
+	[RTD1325_ISR_DRTC_MIN_SHIFT]   = BIT(17),
+	[RTD1325_ISR_DRTC_HOUR_SHIFT]  = BIT(18),
+	[RTD1325_ISR_DRTC_DATE_SHIFT]  = BIT(19),
+	[RTD1325_ISR_DRTC_ALARM_SHIFT] = BIT(20),
+	[RTD1325_ISR_SC0_SHIFT]	       = BIT(24),
+	[RTD1325_ISR_SC1_SHIFT]	       = BIT(25),
+	[RTD1325_ISR_SPI_SHIFT]	       = BIT(27),
+	[RTD1325_ISR_I2C3_SHIFT]       = BIT(28),
+	[RTD1325_ISR_FAN_SHIFT]	       = BIT(29),
+};
+
+static struct realtek_intc_subset_cfg rtd1325_intc_iso_cfgs[] = {
+	{ ISO_NORMAL_MASK, },
+	{ ISO_NMI_WDT_MASK, },
+};
+
+static const struct realtek_intc_info rtd1325_intc_iso_info = {
+	.isr_offset		 = ISO_ISR_OFFSET,
+	.umsk_isr_offset	 = ISO_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = ISO_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1325_iso_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1325_intc_iso_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1325_intc_iso_cfgs),
+};
+
+static struct realtek_intc_subset_cfg rtd1325_intc_misc_cfgs[] = {
+	{ MISC_NORMAL_MASK, },
+	{ MISC_UART1_MASK, },
+	{ MISC_UART2_MASK, },
+};
+
+static const struct realtek_intc_info rtd1325_intc_misc_info = {
+	.isr_offset		 = MISC_ISR_OFFSET,
+	.umsk_isr_offset	 = MISC_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = MISC_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1325_misc_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1325_intc_misc_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1325_intc_misc_cfgs),
+};
+
+static const struct of_device_id realtek_intc_rtd1325_dt_matches[] = {
+	{
+		.compatible = "realtek,rtd1325-intc-iso",
+		.data = &rtd1325_intc_iso_info,
+	}, {
+		.compatible = "realtek,rtd1325-intc-misc",
+		.data = &rtd1325_intc_misc_info,
+	},
+	{ /* sentinel */ }
+};
+
+static int realtek_intc_rtd1325_suspend(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	data->saved_en = readl(data->base + info->scpu_int_en_offset);
+
+	writel(DISABLE_INTC, data->base + info->scpu_int_en_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+
+	return 0;
+}
+
+static int realtek_intc_rtd1325_resume(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+	writel(data->saved_en, data->base + info->scpu_int_en_offset);
+
+	return 0;
+}
+
+static const struct dev_pm_ops realtek_intc_rtd1325_pm_ops = {
+	.suspend_noirq = realtek_intc_rtd1325_suspend,
+	.resume_noirq  = realtek_intc_rtd1325_resume,
+};
+
+static int rtd1325_intc_probe(struct platform_device *pdev)
+{
+	const struct realtek_intc_info *info;
+
+	info = of_device_get_match_data(&pdev->dev);
+	if (!info)
+		return -EINVAL;
+
+	return realtek_intc_probe(pdev, info);
+}
+
+static struct platform_driver realtek_intc_rtd1325_driver = {
+	.probe = rtd1325_intc_probe,
+	.driver = {
+		.name = "realtek_intc_rtd1325",
+		.of_match_table = realtek_intc_rtd1325_dt_matches,
+		.suppress_bind_attrs = true,
+		.pm = &realtek_intc_rtd1325_pm_ops,
+	},
+};
+
+static int __init realtek_intc_rtd1325_init(void)
+{
+	return platform_driver_register(&realtek_intc_rtd1325_driver);
+}
+core_initcall(realtek_intc_rtd1325_init);
+
+static void __exit realtek_intc_rtd1325_exit(void)
+{
+	platform_driver_unregister(&realtek_intc_rtd1325_driver);
+}
+module_exit(realtek_intc_rtd1325_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Realtek RTD1325 Interrupt Controller Driver");
-- 
2.25.1

[PATCH v3 6/6] irqchip: Introduce RTD1619B support using the Realtek common interrupt controller driver

[James Tai]

Add support for the RTD1619B platform.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311061822.551ieaoI-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Unchanged

v1 to v2 change:
- Resolved kernel test robot build warnings
- Replaced magic number with macro
- Fixed code style issues

 drivers/irqchip/Kconfig                |   6 +
 drivers/irqchip/Makefile               |   1 +
 drivers/irqchip/irq-realtek-rtd1619b.c | 217 +++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/irqchip/irq-realtek-rtd1619b.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 65e2d67d1505..c5b2762df420 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -240,6 +240,12 @@ config REALTEK_RTD1325_INTC
 	help
 	  Support for Realtek RTD1325 Interrupt Controller.
 
+config REALTEK_RTD1619B_INTC
+	tristate "Realtek RTD1619B interrupt controller"
+	select REALTEK_DHC_INTC
+	help
+	  Support for Realtek RTD1619B Interrupt Controller.
+
 config RENESAS_INTC_IRQPIN
 	bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index eaa12928d60b..da308aefcb87 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_REALTEK_DHC_INTC)		+= irq-realtek-intc-common.o
 obj-$(CONFIG_REALTEK_RTD1319_INTC)	+= irq-realtek-rtd1319.o
 obj-$(CONFIG_REALTEK_RTD1319D_INTC)	+= irq-realtek-rtd1319d.o
 obj-$(CONFIG_REALTEK_RTD1325_INTC)	+= irq-realtek-rtd1325.o
+obj-$(CONFIG_REALTEK_RTD1619B_INTC)	+= irq-realtek-rtd1619b.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
diff --git a/drivers/irqchip/irq-realtek-rtd1619b.c b/drivers/irqchip/irq-realtek-rtd1619b.c
new file mode 100644
index 000000000000..27bc4f9ef3bf
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-rtd1619b.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek RTD1619B interrupt controller driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "irq-realtek-intc-common.h"
+
+#define ISO_NMI_WDT_MASK     0x08008090
+#define ISO_NORMAL_MASK      0xf7ff7f6e
+#define MISC_NORMAL_MASK     0xffffded6
+#define MISC_UART1_MASK      0x00000028
+#define MISC_UART2_MASK      0x00002100
+
+#define ISO_ISR_EN_OFFSET    0x40
+#define ISO_ISR_OFFSET       0
+#define ISO_ISR_UMSK_OFFSET  0x4
+#define MISC_ISR_EN_OFFSET   0x80
+#define MISC_ISR_OFFSET      0xc
+#define MISC_ISR_UMSK_OFFSET 0x8
+
+enum rtd1619b_iso_isr_bits {
+	RTD1619B_ISO_ISR_TC3_SHIFT	   = 1,
+	RTD1619B_ISO_ISR_UR0_SHIFT	   = 2,
+	RTD1619B_ISO_ISR_LSADC0_SHIFT	   = 3,
+	RTD1619B_ISO_ISR_WDOG1_NMI_SHIFT   = 4,
+	RTD1619B_ISO_ISR_IRDA_SHIFT	   = 5,
+	RTD1619B_ISO_ISR_SPI1_SHIFT	   = 6,
+	RTD1619B_ISO_ISR_WDOG2_NMI_SHIFT   = 7,
+	RTD1619B_ISO_ISR_I2C0_SHIFT	   = 8,
+	RTD1619B_ISO_ISR_TC4_SHIFT	   = 9,
+	RTD1619B_ISO_ISR_TC7_SHIFT	   = 10,
+	RTD1619B_ISO_ISR_I2C1_SHIFT	   = 11,
+	RTD1619B_ISO_ISR_HIFI_WAKEUP_SHIFT = 14,
+	RTD1619B_ISO_ISR_WDOG4_NMI_SHIFT   = 15,
+	RTD1619B_ISO_ISR_TC8_SHIFT	   = 16,
+	RTD1619B_ISO_ISR_VFD_SHIFT	   = 17,
+	RTD1619B_ISO_ISR_VTC_SHIFT	   = 18,
+	RTD1619B_ISO_ISR_GPIOA_SHIFT	   = 19,
+	RTD1619B_ISO_ISR_GPIODA_SHIFT	   = 20,
+	RTD1619B_ISO_ISR_ISO_MISC_SHIFT	   = 21,
+	RTD1619B_ISO_ISR_CBUS_SHIFT	   = 22,
+	RTD1619B_ISO_ISR_ETN_SHIFT	   = 23,
+	RTD1619B_ISO_ISR_USB_HOST_SHIFT	   = 24,
+	RTD1619B_ISO_ISR_USB_U3_DRD_SHIFT  = 25,
+	RTD1619B_ISO_ISR_USB_U2_DRD_SHIFT  = 26,
+	RTD1619B_ISO_ISR_WDOG3_NMI_SHIFT   = 27,
+	RTD1619B_ISO_ISR_PORB_HV_CEN_SHIFT = 28,
+	RTD1619B_ISO_ISR_PORB_DV_CEN_SHIFT = 29,
+	RTD1619B_ISO_ISR_PORB_AV_CEN_SHIFT = 30,
+	RTD1619B_ISO_ISR_I2C1_REQ_SHIFT	   = 31,
+};
+
+static const u32 rtd1619b_iso_isr_to_scpu_int_en_mask[32] = {
+	[RTD1619B_ISO_ISR_SPI1_SHIFT]	     = BIT(1),
+	[RTD1619B_ISO_ISR_UR0_SHIFT]	     = BIT(2),
+	[RTD1619B_ISO_ISR_LSADC0_SHIFT]	     = BIT(3),
+	[RTD1619B_ISO_ISR_IRDA_SHIFT]	     = BIT(5),
+	[RTD1619B_ISO_ISR_I2C0_SHIFT]	     = BIT(8),
+	[RTD1619B_ISO_ISR_I2C1_SHIFT]	     = BIT(11),
+	[RTD1619B_ISO_ISR_VFD_SHIFT]	     = BIT(17),
+	[RTD1619B_ISO_ISR_GPIOA_SHIFT]	     = BIT(19),
+	[RTD1619B_ISO_ISR_GPIODA_SHIFT]	     = BIT(20),
+	[RTD1619B_ISO_ISR_PORB_HV_CEN_SHIFT] = BIT(28),
+	[RTD1619B_ISO_ISR_PORB_DV_CEN_SHIFT] = BIT(29),
+	[RTD1619B_ISO_ISR_PORB_AV_CEN_SHIFT] = BIT(30),
+	[RTD1619B_ISO_ISR_I2C1_REQ_SHIFT]    = BIT(31),
+	[RTD1619B_ISO_ISR_WDOG1_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1619B_ISO_ISR_WDOG2_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1619B_ISO_ISR_WDOG3_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+	[RTD1619B_ISO_ISR_WDOG4_NMI_SHIFT]   = IRQ_ALWAYS_ENABLED,
+};
+
+enum rtd1619b_misc_isr_bits {
+	RTD1619B_ISR_UR1_SHIFT	  = 3,
+	RTD1619B_ISR_TC5_SHIFT	  = 4,
+	RTD1619B_ISR_UR1_TO_SHIFT = 5,
+	RTD1619B_ISR_TC0_SHIFT	  = 6,
+	RTD1619B_ISR_TC1_SHIFT	  = 7,
+	RTD1619B_ISR_UR2_SHIFT	  = 8,
+	RTD1619B_ISR_UR2_TO_SHIFT = 13,
+	RTD1619B_ISR_I2C5_SHIFT	  = 14,
+	RTD1619B_ISR_I2C4_SHIFT	  = 15,
+	RTD1619B_ISR_I2C3_SHIFT	  = 23,
+	RTD1619B_ISR_SC0_SHIFT	  = 24,
+	RTD1619B_ISR_SC1_SHIFT	  = 25,
+	RTD1619B_ISR_SPI_SHIFT	  = 27,
+	RTD1619B_ISR_FAN_SHIFT	  = 29,
+};
+
+static const u32 rtd1619b_misc_isr_to_scpu_int_en_mask[32] = {
+	[RTD1619B_ISR_UR1_SHIFT]    = BIT(3),
+	[RTD1619B_ISR_UR1_TO_SHIFT] = BIT(5),
+	[RTD1619B_ISR_UR2_TO_SHIFT] = BIT(6),
+	[RTD1619B_ISR_UR2_SHIFT]    = BIT(7),
+	[RTD1619B_ISR_I2C5_SHIFT]   = BIT(14),
+	[RTD1619B_ISR_I2C4_SHIFT]   = BIT(15),
+	[RTD1619B_ISR_SC0_SHIFT]    = BIT(24),
+	[RTD1619B_ISR_SC1_SHIFT]    = BIT(25),
+	[RTD1619B_ISR_SPI_SHIFT]    = BIT(27),
+	[RTD1619B_ISR_I2C3_SHIFT]   = BIT(28),
+	[RTD1619B_ISR_FAN_SHIFT]    = BIT(29),
+};
+
+static struct realtek_intc_subset_cfg rtd1619b_intc_iso_cfgs[] = {
+	{ ISO_NORMAL_MASK, },
+	{ ISO_NMI_WDT_MASK, },
+};
+
+static const struct realtek_intc_info rtd1619b_intc_iso_info = {
+	.isr_offset		 = ISO_ISR_OFFSET,
+	.umsk_isr_offset	 = ISO_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = ISO_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1619b_iso_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1619b_intc_iso_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1619b_intc_iso_cfgs),
+};
+
+static struct realtek_intc_subset_cfg rtd1619b_intc_misc_cfgs[] = {
+	{ MISC_NORMAL_MASK, },
+	{ MISC_UART1_MASK, },
+	{ MISC_UART2_MASK, },
+};
+
+static const struct realtek_intc_info rtd1619b_intc_misc_info = {
+	.isr_offset		 = MISC_ISR_OFFSET,
+	.umsk_isr_offset	 = MISC_ISR_UMSK_OFFSET,
+	.scpu_int_en_offset	 = MISC_ISR_EN_OFFSET,
+	.isr_to_scpu_int_en_mask = rtd1619b_misc_isr_to_scpu_int_en_mask,
+	.cfg			 = rtd1619b_intc_misc_cfgs,
+	.cfg_num		 = ARRAY_SIZE(rtd1619b_intc_misc_cfgs),
+};
+
+static const struct of_device_id realtek_intc_rtd1619b_dt_matches[] = {
+	{
+		.compatible = "realtek,rtd1619b-intc-iso",
+		.data = &rtd1619b_intc_iso_info,
+	}, {
+		.compatible = "realtek,rtd1619b-intc-misc",
+		.data = &rtd1619b_intc_misc_info,
+	},
+	{ /* sentinel */ }
+};
+
+static int realtek_intc_rtd1619b_suspend(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	data->saved_en = readl(data->base + info->scpu_int_en_offset);
+
+	writel(DISABLE_INTC, data->base + info->scpu_int_en_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+
+	return 0;
+}
+
+static int realtek_intc_rtd1619b_resume(struct device *dev)
+{
+	struct realtek_intc_data *data = dev_get_drvdata(dev);
+	const struct realtek_intc_info *info = data->info;
+
+	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
+	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
+	writel(data->saved_en, data->base + info->scpu_int_en_offset);
+
+	return 0;
+}
+
+static const struct dev_pm_ops realtek_intc_rtd1619b_pm_ops = {
+	.suspend_noirq = realtek_intc_rtd1619b_suspend,
+	.resume_noirq  = realtek_intc_rtd1619b_resume,
+};
+
+static int rtd1619b_intc_probe(struct platform_device *pdev)
+{
+	const struct realtek_intc_info *info;
+
+	info = of_device_get_match_data(&pdev->dev);
+	if (!info)
+		return -EINVAL;
+
+	return realtek_intc_probe(pdev, info);
+}
+
+static struct platform_driver realtek_intc_rtd1619b_driver = {
+	.probe = rtd1619b_intc_probe,
+	.driver = {
+		.name = "realtek_intc_rtd1619b",
+		.of_match_table = realtek_intc_rtd1619b_dt_matches,
+		.suppress_bind_attrs = true,
+		.pm = &realtek_intc_rtd1619b_pm_ops,
+	},
+};
+
+static int __init realtek_intc_rtd1619b_init(void)
+{
+	return platform_driver_register(&realtek_intc_rtd1619b_driver);
+}
+core_initcall(realtek_intc_rtd1619b_init);
+
+static void __exit realtek_intc_rtd1619b_exit(void)
+{
+	platform_driver_unregister(&realtek_intc_rtd1619b_driver);
+}
+module_exit(realtek_intc_rtd1619b_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Realtek RTD1619B Interrupt Controller Driver");
-- 
2.25.1

Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Dan Carpenter]

On Wed, Nov 29, 2023 at 01:43:35PM +0800, James Tai wrote:
> +static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
> +{
> +	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
> +	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
> +	int irq;
> +
> +	irq = irq_of_parse_and_map(node, index);
> +	if (irq <= 0)
> +		return irq;

I don't think irq_of_parse_and_map() can return negatives.  Only zero
on error.  Returning zero on error is a historical artifact with IRQ
functions and a constant source of bugs.  But here returning zero is
success.  See my blog for more details:
https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-zero-irq-error-codes/

So this should probably be:

	irq = irq_of_parse_and_map(node, index);
	if (!irq)
		return -ENODEV;

I should create a static checker warning for this...  Possibly I should
create a static checker warning any time someone does:

	if (foo <= 0)
		return foo;

> +
> +	subset_data->common = data;
> +	subset_data->cfg = cfg;
> +	subset_data->parent_irq = irq;
> +	irq_set_chained_handler_and_data(irq, realtek_intc_handler, subset_data);
> +
> +	return 0;
> +}
> +
> +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> +{
> +	struct realtek_intc_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret, i;
> +
> +	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->base = of_iomap(node, 0);
> +	if (!data->base) {
> +		ret = -ENOMEM;
> +		goto out_cleanup;

devm_ allocations are cleaned up automatically so there is no need to
call devm_kfree() before returning.

regards,
dan carpenter

> +	}
> +
> +	data->info = info;
> +
> +	raw_spin_lock_init(&data->lock);
> +
> +	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);
> +	if (!data->domain) {
> +		ret = -ENOMEM;
> +		goto out_cleanup;
> +	}
> +
> +	data->subset_data_num = info->cfg_num;
> +	for (i = 0; i < info->cfg_num; i++) {
> +		ret = realtek_intc_subset(node, data, i);
> +		if (ret) {
> +			WARN(ret, "failed to init subset %d: %d", i, ret);
> +			ret = -ENOMEM;
> +			goto out_cleanup;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +
> +out_cleanup:
> +
> +	if (data->base)
> +		iounmap(data->base);
> +
> +	devm_kfree(dev, data);
> +
> +	return ret;
> +}

Re: [PATCH v3 1/6] dt-bindings: interrupt-controller: Add support for Realtek DHC SoCs

[Krzysztof Kozlowski]

On 29/11/2023 06:43, James Tai wrote:
> Add the YAML documentation for Realtek DHC (Digital Home Center) SoCs.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202311180921.ayKhiFHL-lkp@intel.com/

Drop both. They are not applicable to this patch.

> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Marc Zyngier <maz@kernel.org>

> CC: Rob Herring <robh+dt@kernel.org>
> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> CC: Conor Dooley <conor+dt@kernel.org>
> CC: linux-kernel@vger.kernel.org
> CC: devicetree@vger.kernel.org

Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.

If you need it for your own patch management purposes, keep it under the
--- separator.

> Signed-off-by: James Tai <james.tai@realtek.com>
> ---
> v2 to v3 change:
> - Retested the bindings using the new version of the dtschema
> - Fixed the order of property items
> - Removed redundant files and replaced them with 'realtek,intc.yaml'
> - Replaced 'interrupts-extended' with 'interrupts'
> - Added a description for 'interrupts'
> - Reduced the example code
> 
> v1 to v2 change:
> - Tested the bindings using 'make dt_binding_check'
> - Fixed code style issues
> 
>  .../interrupt-controller/realtek,intc.yaml    | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml
> new file mode 100644
> index 000000000000..3aa863b1549d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,intc.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/realtek,intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs Interrupt Controller
> +
> +maintainers:
> +  - James Tai <james.tai@realtek.com>
> +
> +description:
> +  This interrupt controller is a component of Realtek DHC (Digital Home Center)
> +  SoCs and is designed to receive interrupts from peripheral devices.
> +
> +  Each DHC SoC has two sets of interrupt controllers, each capable of
> +  handling up to 32 interrupts.
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd1319-intc-iso
> +      - realtek,rtd1319-intc-misc
> +      - realtek,rtd1319d-intc-iso
> +      - realtek,rtd1319d-intc-misc
> +      - realtek,rtd1325-intc-iso
> +      - realtek,rtd1325-intc-misc
> +      - realtek,rtd1619b-intc-iso
> +      - realtek,rtd1619b-intc-misc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +  '#address-cells':
> +    const: 0
> +
> +  interrupts:
> +    description: |
> +      Contains the GIC SPI IRQs mapped to the external interrupt lines.
> +    minItems: 2
> +    maxItems: 4

My previous comments were not addressed. Why lines are not described
(items: description:)? Are they all the same? Why you did not respond to
clarify this comment?

The rest of my comment here was also ignored. You cannot just ignore
comments, but must respond to them or implement them.

To clarify: I expect allOf: block after required: constraining the
interrupts per variant.



Best regards,
Krzysztof

Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Dan Carpenter]

On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
> > +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> > +{
> > +	struct realtek_intc_data *data;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node = dev->of_node;
> > +	int ret, i;
> > +
> > +	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->base = of_iomap(node, 0);
> > +	if (!data->base) {
> > +		ret = -ENOMEM;
> > +		goto out_cleanup;
> 
> devm_ allocations are cleaned up automatically so there is no need to
> call devm_kfree() before returning.
> 
> regards,
> dan carpenter
> 
> > +	}
> > +
> > +	data->info = info;
> > +
> > +	raw_spin_lock_init(&data->lock);
> > +
> > +	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);

Btw, as I was testing the other static checker warning for <= 0, my
static checker really wants this irq_domain_add_linear() to be cleaned
up on the error path.

Otherwise it probably leads to a use after free because we free data
(automatically or manually) but it's still on a list somewhere.

> > +	if (!data->domain) {
> > +		ret = -ENOMEM;
> > +		goto out_cleanup;
> > +	}
> > +
> > +	data->subset_data_num = info->cfg_num;
> > +	for (i = 0; i < info->cfg_num; i++) {
> > +		ret = realtek_intc_subset(node, data, i);
> > +		if (ret) {
> > +			WARN(ret, "failed to init subset %d: %d", i, ret);
> > +			ret = -ENOMEM;
> > +			goto out_cleanup;

This error path.

regards,
dan carpenter


> > +		}
> > +	}
> > +
> > +	platform_set_drvdata(pdev, data);
> > +
> > +	return 0;
> > +
> > +out_cleanup:
> > +
> > +	if (data->base)
> > +		iounmap(data->base);
> > +
> > +	devm_kfree(dev, data);
> > +
> > +	return ret;
> > +}

Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Rob Herring]

On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
> On Wed, Nov 29, 2023 at 01:43:35PM +0800, James Tai wrote:
> > +static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
> > +{
> > +	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
> > +	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
> > +	int irq;
> > +
> > +	irq = irq_of_parse_and_map(node, index);
> > +	if (irq <= 0)
> > +		return irq;
> 
> I don't think irq_of_parse_and_map() can return negatives.  Only zero
> on error.  Returning zero on error is a historical artifact with IRQ
> functions and a constant source of bugs.  But here returning zero is
> success.  See my blog for more details:
> https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-zero-irq-error-codes/

It's worse than that. The irq functions historically returned NO_IRQ on 
error, but that could be 0 or -1 depending on the arch.

Use of_irq_get() instead. It's a bit better in that it returns an error 
code for most cases. But still returns 0 on mapping failure.

Rob

RE: [PATCH v3 1/6] dt-bindings: interrupt-controller: Add support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Krzysztof,

>On 29/11/2023 06:43, James Tai wrote:
>> Add the YAML documentation for Realtek DHC (Digital Home Center) SoCs.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/202311180921.ayKhiFHL-lkp@intel.
>> com/
>
>Drop both. They are not applicable to this patch.
>
Okay. I will drop them.

>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Marc Zyngier <maz@kernel.org>
>
>> CC: Rob Herring <robh+dt@kernel.org>
>> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> CC: Conor Dooley <conor+dt@kernel.org>
>> CC: linux-kernel@vger.kernel.org
>> CC: devicetree@vger.kernel.org
>
>Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
>commit msg. There is no single need to store automated output of
>get_maintainers.pl in the git log. It can be easily re-created at any given time,
>thus its presence in the git history is redundant and obfuscates the log.
>
>If you need it for your own patch management purposes, keep it under the
>--- separator.
>
I will move the CC-entries under the '---' separator.

>My previous comments were not addressed. Why lines are not described
>(items: description:)? Are they all the same? Why you did not respond to clarify
>this comment?
>
I've addressed the previous comments and will include a description for each line in the next patches.
I misunderstood your meaning, so I did not provide a clear response.

>The rest of my comment here was also ignored. You cannot just ignore
>comments, but must respond to them or implement them.
>
I will improve this part.

>To clarify: I expect allOf: block after required: constraining the interrupts per
>variant.
>
I will adjust it in next patches.

Regards,
James

RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Dan,

>> devm_ allocations are cleaned up automatically so there is no need to
>> call devm_kfree() before returning.
>>
>> regards,
>> dan carpenter
>
I will remove it. 

>> > +   }
>> > +
>> > +   data->info = info;
>> > +
>> > +   raw_spin_lock_init(&data->lock);
>> > +
>> > +   data->domain = irq_domain_add_linear(node, 32,
>> > + &realtek_intc_domain_ops, data);
>
>Btw, as I was testing the other static checker warning for <= 0, my static
>checker really wants this irq_domain_add_linear() to be cleaned up on the error
>path.
>
>Otherwise it probably leads to a use after free because we free data
>(automatically or manually) but it's still on a list somewhere.
>
I will add 'irq_domain_remove()' to release it. 

>> > +   if (!data->domain) {
>> > +           ret = -ENOMEM;
>> > +           goto out_cleanup;
>> > +   }
>> > +
>> > +   data->subset_data_num = info->cfg_num;
>> > +   for (i = 0; i < info->cfg_num; i++) {
>> > +           ret = realtek_intc_subset(node, data, i);
>> > +           if (ret) {
>> > +                   WARN(ret, "failed to init subset %d: %d", i, ret);
>> > +                   ret = -ENOMEM;
>> > +                   goto out_cleanup;
>
>This error path.
>
>regards,
>dan carpenter
>
I will add 'irq_domain_remove()' before goto cleanup.

	for (i = 0; i < info->cfg_num; i++) {
		ret = realtek_intc_subset(node, data, i);
		if (ret) {
			WARN(ret, "failed to init subset %d: %d", i, ret);
			irq_domain_remove(data->domain);
			ret = -ENOMEM;
			goto out_cleanup;
		}
	}

Thank you for your feedback.

Regards,
James

Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Dan Carpenter]

On Fri, Dec 08, 2023 at 08:21:10AM +0000, James Tai [戴志峰] wrote:
> Hi Dan,
> 
> >> devm_ allocations are cleaned up automatically so there is no need to
> >> call devm_kfree() before returning.
> >>
> >> regards,
> >> dan carpenter
> >
> I will remove it. 
> 
> >> > +   }
> >> > +
> >> > +   data->info = info;
> >> > +
> >> > +   raw_spin_lock_init(&data->lock);
> >> > +
> >> > +   data->domain = irq_domain_add_linear(node, 32,
> >> > + &realtek_intc_domain_ops, data);
> >
> >Btw, as I was testing the other static checker warning for <= 0, my static
> >checker really wants this irq_domain_add_linear() to be cleaned up on the error
> >path.
> >
> >Otherwise it probably leads to a use after free because we free data
> >(automatically or manually) but it's still on a list somewhere.
> >
> I will add 'irq_domain_remove()' to release it. 
> 
> >> > +   if (!data->domain) {
> >> > +           ret = -ENOMEM;
> >> > +           goto out_cleanup;
> >> > +   }
> >> > +
> >> > +   data->subset_data_num = info->cfg_num;
> >> > +   for (i = 0; i < info->cfg_num; i++) {
> >> > +           ret = realtek_intc_subset(node, data, i);
> >> > +           if (ret) {
> >> > +                   WARN(ret, "failed to init subset %d: %d", i, ret);
> >> > +                   ret = -ENOMEM;
> >> > +                   goto out_cleanup;
> >
> >This error path.
> >
> >regards,
> >dan carpenter
> >
> I will add 'irq_domain_remove()' before goto cleanup.
> 
> 	for (i = 0; i < info->cfg_num; i++) {
> 		ret = realtek_intc_subset(node, data, i);
> 		if (ret) {
> 			WARN(ret, "failed to init subset %d: %d", i, ret);
> 			irq_domain_remove(data->domain);
> 			ret = -ENOMEM;
> 			goto out_cleanup;
> 		}
> 	}
> 
> Thank you for your feedback.

You're running into the issue because you're using One Err Label style
error handling.  It would be better to use normal unwind laddering.
See my blog for more info:

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter

Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Thomas Gleixner]

On Wed, Nov 29 2023 at 13:43, James Tai wrote:
> Realtek DHC (Digital Home Center) SoCs share a common interrupt controller
> design. This universal interrupt controller driver provides support for
> various variants within the Realtek DHC SoC family.
>
> Each DHC SoC features two sets of extended interrupt controllers, each
> capable of handling up to 32 interrupts. These expansion controllers are
> connected to the GIC (Generic Interrupt Controller).
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/

These tags are pointless as they are not related to anything in
tree. You addressed review comments and 0-day fallout, but neither Dan
nor 0-day reported that the interrupt controller for Realtek DHC SoCs is
missing.

> +#include "irq-realtek-intc-common.h"
> +
> +struct realtek_intc_data;

struct realtek_intc_data is declared in irq-realtek-intc-common.h, so
what's the point of this forward declaration?

> +static inline unsigned int realtek_intc_get_ints(struct realtek_intc_data *data)
> +{
> +	return readl(data->base + data->info->isr_offset);
> +}
> +
> +static inline void realtek_intc_clear_ints_bit(struct realtek_intc_data *data, int bit)
> +{
> +	writel(BIT(bit) & ~1, data->base + data->info->isr_offset);

That '& ~1' solves what aside of preventing bit 0 from being written?

> +static int realtek_intc_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> +{
> +	struct realtek_intc_data *data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);
> +	irq_set_chip_data(irq, data);
> +	irq_set_probe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops realtek_intc_domain_ops = {
> +	.xlate = irq_domain_xlate_onecell,
> +	.map = realtek_intc_domain_map,

	.xlate	= irq_domain_xlate_onecell,
	.map	= realtek_intc_domain_map,

Please.

> +};
> +
> +static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
> +{
> +	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
> +	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
> +	int irq;
> +
> +	irq = irq_of_parse_and_map(node, index);

irq_of_parse_and_map() returns an 'unsigned int' where 0 is fail.

> +	if (irq <= 0)
> +		return irq;
> +
> +	subset_data->common = data;
> +	subset_data->cfg = cfg;
> +	subset_data->parent_irq = irq;
> +	irq_set_chained_handler_and_data(irq, realtek_intc_handler, subset_data);
> +
> +	return 0;
> +}
> +
> +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> +{
> +	struct realtek_intc_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret, i;
> +
> +	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->base = of_iomap(node, 0);
> +	if (!data->base) {
> +		ret = -ENOMEM;
> +		goto out_cleanup;

devm_kzalloc() is automatically cleaned up when the probe function
fails, so 'return -ENOMEM;' is sufficient.

> +	}
> +
> +	data->info = info;
> +
> +	raw_spin_lock_init(&data->lock);
> +
> +	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);
> +	if (!data->domain) {
> +		ret = -ENOMEM;

This 'ret = -ENOMEM;' is pointless as the only error code returned in this
function is -ENOMEM. So you can just return -ENOMEM in the error path, no?

> +		goto out_cleanup;
> +	}
> +
> +	data->subset_data_num = info->cfg_num;
> +	for (i = 0; i < info->cfg_num; i++) {
> +		ret = realtek_intc_subset(node, data, i);
> +		if (ret) {
> +			WARN(ret, "failed to init subset %d: %d", i, ret);
> +			ret = -ENOMEM;
> +			goto out_cleanup;

                if (WARN(ret, "....."))
                	goto cleanup;

> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +
> +out_cleanup:
> +
> +	if (data->base)
> +		iounmap(data->base);

Leaks the irqdomain.

> +
> +	devm_kfree(dev, data);

Pointless exercise.

> +	return ret;
> +}
> +EXPORT_SYMBOL(realtek_intc_probe);

EXPORT_SYMBOL_GPL

> +/**
> + * realtek_intc_subset_cfg - subset interrupt mask
> + * @ints_mask: inetrrupt mask
> + */
> +struct realtek_intc_subset_cfg {
> +	unsigned int	ints_mask;
> +};

The value of a struct wrapping a single 'unsigned int' is? What's wrong
with using unsigned int (actually you want u32 as this represents a
hardware mask) directly? Not enough obfuscation, right?

> +/**
> + * realtek_intc_info - interrupt controller data.
> + * @isr_offset: interrupt status register offset.
> + * @umsk_isr_offset: unmask interrupt status register offset.
> + * @scpu_int_en_offset: interrupt enable register offset.
> + * @cfg: cfg of the subset.
> + * @cfg_num: number of cfg.

 * @isr_offset:		interrupt status register offset
 * @umsk_isr_offset:	unmask interrupt status register offset
 * @scpu_int_en_offset:	interrupt enable register offset

Can you spot the difference?

Please fix all over the place.

> + */
> +struct realtek_intc_info {
> +	const struct realtek_intc_subset_cfg *cfg;
> +	unsigned int			     isr_offset;
> +	unsigned int			     umsk_isr_offset;
> +	unsigned int			     scpu_int_en_offset;
> +	const u32			     *isr_to_scpu_int_en_mask;
> +	int				     cfg_num;
> +};
> +
> +/**
> + * realtek_intc_subset_data - handler of a interrupt source only handles ints
> + *                            bits in the mask.
> + * @cfg: cfg of the subset.

Seriously. 'cfg of'? This is a description, so can you spell the words
out? This is really neither space constraint nor subject to Xitter
rules. Fix this all over the place please.

> + * @common: common data.
> + * @parent_irq: interrupt source.
> + */
> +struct realtek_intc_subset_data {
> +	const struct realtek_intc_subset_cfg *cfg;
> +	struct realtek_intc_data	     *common;
> +	int				     parent_irq;
> +};
> +
> +/**
> + * realtek_intc_data - configuration data for realtek interrupt controller driver.
> + * @base: base of interrupt register
> + * @info: info of intc
> + * @domain: interrupt domain
> + * @lock: lock
> + * @saved_en: status of interrupt enable
> + * @subset_data_num: number of subset data
> + * @subset_data: subset data
> + */
> +struct realtek_intc_data {
> +	void __iomem			*base;
> +	const struct realtek_intc_info	*info;
> +	struct irq_domain		*domain;
> +	struct raw_spinlock		lock;
> +	unsigned int			saved_en;
> +	int				subset_data_num;
> +	struct realtek_intc_subset_data subset_data[];
> +};
> +
> +#define IRQ_ALWAYS_ENABLED U32_MAX
> +#define DISABLE_INTC (0)
> +#define CLEAN_INTC_STATUS GENMASK(31, 1)

#define IRQ_ALWAYS_ENABLED	U32_MAX
#define DISABLE_INTC		(0)
#define CLEAN_INTC_STATUS	GENMASK(31, 1)

Please, as that makes this readable.

Thanks,

        tglx

Re: [PATCH v3 3/6] irqchip: Introduce RTD1319 support using the Realtek common interrupt controller driver

[Thomas Gleixner]

On Wed, Nov 29 2023 at 13:43, James Tai wrote:
> Add support for the RTD1319 platform.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202311061208.hJmxGqym-lkp@intel.com/

Again: These tags are just wrong.

> +static struct platform_driver realtek_intc_rtd1319_driver = {
> +	.probe = rtd1319_intc_probe,
> +	.driver = {
> +		.name = "realtek_intc_rtd1319",
> +		.of_match_table = realtek_intc_rtd1319_dt_matches,
> +		.suppress_bind_attrs = true,
> +		.pm = &realtek_intc_rtd1319_pm_ops,
> +	},

	.probe	= rtd1319_intc_probe,
	.driver	= {
		.name			= "realtek_intc_rtd1319",
		.of_match_table		= realtek_intc_rtd1319_dt_matches,
                ....
                
Please.

> +};
> +
> +static int __init realtek_intc_rtd1319_init(void)
> +{
> +	return platform_driver_register(&realtek_intc_rtd1319_driver);
> +}
> +core_initcall(realtek_intc_rtd1319_init);

What? This can be built as a module. So how is core_initcall() in any
way correct here? module_init() perhaps?

Thanks,

        tglx

Re: [PATCH v3 6/6] irqchip: Introduce RTD1619B support using the Realtek common interrupt controller driver

[Thomas Gleixner]

On Wed, Nov 29 2023 at 13:43, James Tai wrote:
> +static int realtek_intc_rtd1619b_suspend(struct device *dev)
> +{
> +	struct realtek_intc_data *data = dev_get_drvdata(dev);
> +	const struct realtek_intc_info *info = data->info;
> +
> +	data->saved_en = readl(data->base + info->scpu_int_en_offset);
> +
> +	writel(DISABLE_INTC, data->base + info->scpu_int_en_offset);
> +	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
> +	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
> +
> +	return 0;
> +}
> +
> +static int realtek_intc_rtd1619b_resume(struct device *dev)
> +{
> +	struct realtek_intc_data *data = dev_get_drvdata(dev);
> +	const struct realtek_intc_info *info = data->info;
> +
> +	writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
> +	writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
> +	writel(data->saved_en, data->base + info->scpu_int_en_offset);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops realtek_intc_rtd1619b_pm_ops = {
> +	.suspend_noirq = realtek_intc_rtd1619b_suspend,
> +	.resume_noirq  = realtek_intc_rtd1619b_resume,
> +};

So this is the 4th copy of the same code, really? Why is this not part
of the common code?

Thanks,

        tglx

RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Dan,

>You're running into the issue because you're using One Err Label style error
>handling.  It would be better to use normal unwind laddering.
>See my blog for more info:
>
>https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
>

Thanks for your guidance. I will adjust the error handling flow.

Regards,
James

RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Rob,

>
>On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
>> On Wed, Nov 29, 2023 at 01:43:35PM +0800, James Tai wrote:
>> > +static int realtek_intc_subset(struct device_node *node, struct
>> > +realtek_intc_data *data, int index) {
>> > +   struct realtek_intc_subset_data *subset_data =
>&data->subset_data[index];
>> > +   const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
>> > +   int irq;
>> > +
>> > +   irq = irq_of_parse_and_map(node, index);
>> > +   if (irq <= 0)
>> > +           return irq;
>>
>> I don't think irq_of_parse_and_map() can return negatives.  Only zero
>> on error.  Returning zero on error is a historical artifact with IRQ
>> functions and a constant source of bugs.  But here returning zero is
>> success.  See my blog for more details:
>> https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-ze
>> ro-irq-error-codes/
>
>It's worse than that. The irq functions historically returned NO_IRQ on error, but
>that could be 0 or -1 depending on the arch.
>
>Use of_irq_get() instead. It's a bit better in that it returns an error code for most
>cases. But still returns 0 on mapping failure.
>

I will use of_irq_get() instead and adjust the return value of realtek_intc_subset() in the next patches.

Thanks for your feedback.

Regards,
James

Re: [PATCH v3 3/6] irqchip: Introduce RTD1319 support using the Realtek common interrupt controller driver

[Rob Herring]

On Tue, Nov 28, 2023 at 11:44 PM James Tai <james.tai@realtek.com> wrote:
>
> Add support for the RTD1319 platform.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202311061208.hJmxGqym-lkp@intel.com/
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Marc Zyngier <maz@kernel.org>
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: James Tai <james.tai@realtek.com>
> ---
> v2 to v3 change:
> - Unchanged
>
> v1 to v2 change:
> - Resolved kernel test robot build warnings
> - Replaced magic number with macro
> - Fixed code style issues
>
>  drivers/irqchip/Kconfig               |   6 +
>  drivers/irqchip/Makefile              |   1 +
>  drivers/irqchip/irq-realtek-rtd1319.c | 218 ++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/irqchip/irq-realtek-rtd1319.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 267c3429b48d..05856ce885fa 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -222,6 +222,12 @@ config REALTEK_DHC_INTC
>         tristate
>         select IRQ_DOMAIN
>
> +config REALTEK_RTD1319_INTC
> +       tristate "Realtek RTD1319 interrupt controller"
> +       select REALTEK_DHC_INTC
> +       help
> +         Support for Realtek RTD1319 Interrupt Controller.
> +
>  config RENESAS_INTC_IRQPIN
>         bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
>         select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index f6774af7fde2..6a2650b0a924 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_IXP4XX_IRQ)              += irq-ixp4xx.o
>  obj-$(CONFIG_JCORE_AIC)                        += irq-jcore-aic.o
>  obj-$(CONFIG_RDA_INTC)                 += irq-rda-intc.o
>  obj-$(CONFIG_REALTEK_DHC_INTC)         += irq-realtek-intc-common.o
> +obj-$(CONFIG_REALTEK_RTD1319_INTC)     += irq-realtek-rtd1319.o
>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)      += irq-renesas-intc-irqpin.o
>  obj-$(CONFIG_RENESAS_IRQC)             += irq-renesas-irqc.o
>  obj-$(CONFIG_RENESAS_RZA1_IRQC)                += irq-renesas-rza1.o
> diff --git a/drivers/irqchip/irq-realtek-rtd1319.c b/drivers/irqchip/irq-realtek-rtd1319.c
> new file mode 100644
> index 000000000000..23c13c218b04
> --- /dev/null
> +++ b/drivers/irqchip/irq-realtek-rtd1319.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +/*
> + * Realtek RTD1319 interrupt controller driver
> + *
> + * Copyright (c) 2023 Realtek Semiconductor Corporation
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/of_device.h>

You probably don't need this header and the implicit includes it makes
are dropped now in linux-next. Please check what you actually need and
make them explicit.

Rob

RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Thomas,

>On Wed, Nov 29 2023 at 13:43, James Tai wrote:
>> Realtek DHC (Digital Home Center) SoCs share a common interrupt
>> controller design. This universal interrupt controller driver provides
>> support for various variants within the Realtek DHC SoC family.
>>
>> Each DHC SoC features two sets of extended interrupt controllers, each
>> capable of handling up to 32 interrupts. These expansion controllers
>> are connected to the GIC (Generic Interrupt Controller).
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <error27@gmail.com>
>> Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/
>
>These tags are pointless as they are not related to anything in tree. You
>addressed review comments and 0-day fallout, but neither Dan nor 0-day
>reported that the interrupt controller for Realtek DHC SoCs is missing.
>
I will remove it.

>> +#include "irq-realtek-intc-common.h"
>> +
>> +struct realtek_intc_data;
>
>struct realtek_intc_data is declared in irq-realtek-intc-common.h, so what's the
>point of this forward declaration?
>
This is a meaningless declaration, and I will remove it.

>> +static inline unsigned int realtek_intc_get_ints(struct
>> +realtek_intc_data *data) {
>> +     return readl(data->base + data->info->isr_offset); }
>> +
>> +static inline void realtek_intc_clear_ints_bit(struct
>> +realtek_intc_data *data, int bit) {
>> +     writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
>
>That '& ~1' solves what aside of preventing bit 0 from being written?
>
The ISR register uses bit 0 to clear or set ISR status.
Write 0 to clear bits and write 1 to set bits.
Therefore, to clear the interrupt status, bit 0 should consistently be set to '0'.

>> +static int realtek_intc_domain_map(struct irq_domain *d, unsigned int
>> +irq, irq_hw_number_t hw) {
>> +     struct realtek_intc_data *data = d->host_data;
>> +
>> +     irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);
>> +     irq_set_chip_data(irq, data);
>> +     irq_set_probe(irq);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct irq_domain_ops realtek_intc_domain_ops = {
>> +     .xlate = irq_domain_xlate_onecell,
>> +     .map = realtek_intc_domain_map,
>
>        .xlate  = irq_domain_xlate_onecell,
>        .map    = realtek_intc_domain_map,
>
>Please.
>
I will fix it.

>> +};
>> +
>> +static int realtek_intc_subset(struct device_node *node, struct
>> +realtek_intc_data *data, int index) {
>> +     struct realtek_intc_subset_data *subset_data =
>&data->subset_data[index];
>> +     const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
>> +     int irq;
>> +
>> +     irq = irq_of_parse_and_map(node, index);
>
>irq_of_parse_and_map() returns an 'unsigned int' where 0 is fail.
>
I will use of_irq_get() instead.

>> +     if (irq <= 0)
>> +             return irq;
>> +
>> +     subset_data->common = data;
>> +     subset_data->cfg = cfg;
>> +     subset_data->parent_irq = irq;
>> +     irq_set_chained_handler_and_data(irq, realtek_intc_handler,
>> + subset_data);
>> +
>> +     return 0;
>> +}
>> +
>> +int realtek_intc_probe(struct platform_device *pdev, const struct
>> +realtek_intc_info *info) {
>> +     struct realtek_intc_data *data;
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +     int ret, i;
>> +
>> +     data = devm_kzalloc(dev, struct_size(data, subset_data,
>info->cfg_num), GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +
>> +     data->base = of_iomap(node, 0);
>> +     if (!data->base) {
>> +             ret = -ENOMEM;
>> +             goto out_cleanup;
>
>devm_kzalloc() is automatically cleaned up when the probe function fails, so
>'return -ENOMEM;' is sufficient.

I will adjust it.

>
>> +     }
>> +
>> +     data->info = info;
>> +
>> +     raw_spin_lock_init(&data->lock);
>> +
>> +     data->domain = irq_domain_add_linear(node, 32,
>&realtek_intc_domain_ops, data);
>> +     if (!data->domain) {
>> +             ret = -ENOMEM;
>
>This 'ret = -ENOMEM;' is pointless as the only error code returned in this
>function is -ENOMEM. So you can just return -ENOMEM in the error path, no?
>
Yes. it is pointless and I will adjust it.

>> +             goto out_cleanup;
>> +     }
>> +
>> +     data->subset_data_num = info->cfg_num;
>> +     for (i = 0; i < info->cfg_num; i++) {
>> +             ret = realtek_intc_subset(node, data, i);
>> +             if (ret) {
>> +                     WARN(ret, "failed to init subset %d: %d", i, ret);
>> +                     ret = -ENOMEM;
>> +                     goto out_cleanup;
>
>                if (WARN(ret, "....."))
>                        goto cleanup;
>
I will fix it.

>> +             }
>> +     }
>> +
>> +     platform_set_drvdata(pdev, data);
>> +
>> +     return 0;
>> +
>> +out_cleanup:
>> +
>> +     if (data->base)
>> +             iounmap(data->base);
>
>Leaks the irqdomain.
>
I will add error handle for irqdomain.

>> +
>> +     devm_kfree(dev, data);
>
>Pointless exercise.
>
I will remove it.

>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(realtek_intc_probe);
>
>EXPORT_SYMBOL_GPL
>
I will fix it.

>> +/**
>> + * realtek_intc_subset_cfg - subset interrupt mask
>> + * @ints_mask: inetrrupt mask
>> + */
>> +struct realtek_intc_subset_cfg {
>> +     unsigned int    ints_mask;
>> +};
>
>The value of a struct wrapping a single 'unsigned int' is? What's wrong with
>using unsigned int (actually you want u32 as this represents a hardware mask)
>directly? Not enough obfuscation, right?
>
Yes, it represents a set of hardware masks, so using u32 instead of 'unsigned int' is more appropriate.
There are no issues with obfuscation.

>> +/**
>> + * realtek_intc_info - interrupt controller data.
>> + * @isr_offset: interrupt status register offset.
>> + * @umsk_isr_offset: unmask interrupt status register offset.
>> + * @scpu_int_en_offset: interrupt enable register offset.
>> + * @cfg: cfg of the subset.
>> + * @cfg_num: number of cfg.
>
> * @isr_offset:         interrupt status register offset
> * @umsk_isr_offset:    unmask interrupt status register offset
> * @scpu_int_en_offset: interrupt enable register offset
>
>Can you spot the difference?
>
The interrupt control registers of the DHC platform are not linearly arranged, which necessitates the use of a mechanism for efficient reading and writing of these registers.
The 'scpu_int_en' register serves the purpose of controlling whether peripheral devices' interrupts are enabled or disabled.
The 'isr' register represents the interrupt status. It can mask interrupts using the 'scpu_int_en' register. When the 'scpu_int_en' register disables interrupts, the 'isr' register will not reflect the interrupt status.
The 'umsk_isr' register also represents the interrupt status but is not influenced by any other control register to mask interrupts. In short, it reflects the original interrupt status.

>Please fix all over the place.
>
I will fix it.

>> + */
>> +struct realtek_intc_info {
>> +     const struct realtek_intc_subset_cfg *cfg;
>> +     unsigned int                         isr_offset;
>> +     unsigned int                         umsk_isr_offset;
>> +     unsigned int                         scpu_int_en_offset;
>> +     const u32
>*isr_to_scpu_int_en_mask;
>> +     int                                  cfg_num;
>> +};
>> +
>> +/**
>> + * realtek_intc_subset_data - handler of a interrupt source only handles ints
>> + *                            bits in the mask.
>> + * @cfg: cfg of the subset.
>
>Seriously. 'cfg of'? This is a description, so can you spell the words out? This is
>really neither space constraint nor subject to Xitter rules. Fix this all over the
>place please.

I will adjust the description so that it clearly describes the intended purpose.

>
>> + * @common: common data.
>> + * @parent_irq: interrupt source.
>> + */
>> +struct realtek_intc_subset_data {
>> +     const struct realtek_intc_subset_cfg *cfg;
>> +     struct realtek_intc_data             *common;
>> +     int                                  parent_irq;
>> +};
>> +
>> +/**
>> + * realtek_intc_data - configuration data for realtek interrupt controller
>driver.
>> + * @base: base of interrupt register
>> + * @info: info of intc
>> + * @domain: interrupt domain
>> + * @lock: lock
>> + * @saved_en: status of interrupt enable
>> + * @subset_data_num: number of subset data
>> + * @subset_data: subset data
>> + */
>> +struct realtek_intc_data {
>> +     void __iomem                    *base;
>> +     const struct realtek_intc_info  *info;
>> +     struct irq_domain               *domain;
>> +     struct raw_spinlock             lock;
>> +     unsigned int                    saved_en;
>> +     int                             subset_data_num;
>> +     struct realtek_intc_subset_data subset_data[]; };
>> +
>> +#define IRQ_ALWAYS_ENABLED U32_MAX
>> +#define DISABLE_INTC (0)
>> +#define CLEAN_INTC_STATUS GENMASK(31, 1)
>
>#define IRQ_ALWAYS_ENABLED      U32_MAX
>#define DISABLE_INTC            (0)
>#define CLEAN_INTC_STATUS       GENMASK(31, 1)
>
>Please, as that makes this readable.
>
I will improve it.

Thanks for your feedback. 

Regards,
James

RE: [PATCH v3 6/6] irqchip: Introduce RTD1619B support using the Realtek common interrupt controller driver

[James Tai [戴志峰]]

Hi Thomas,

>On Wed, Nov 29 2023 at 13:43, James Tai wrote:
>> +static int realtek_intc_rtd1619b_suspend(struct device *dev) {
>> +     struct realtek_intc_data *data = dev_get_drvdata(dev);
>> +     const struct realtek_intc_info *info = data->info;
>> +
>> +     data->saved_en = readl(data->base + info->scpu_int_en_offset);
>> +
>> +     writel(DISABLE_INTC, data->base + info->scpu_int_en_offset);
>> +     writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
>> +     writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
>> +
>> +     return 0;
>> +}
>> +
>> +static int realtek_intc_rtd1619b_resume(struct device *dev) {
>> +     struct realtek_intc_data *data = dev_get_drvdata(dev);
>> +     const struct realtek_intc_info *info = data->info;
>> +
>> +     writel(CLEAN_INTC_STATUS, data->base + info->umsk_isr_offset);
>> +     writel(CLEAN_INTC_STATUS, data->base + info->isr_offset);
>> +     writel(data->saved_en, data->base + info->scpu_int_en_offset);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct dev_pm_ops realtek_intc_rtd1619b_pm_ops = {
>> +     .suspend_noirq = realtek_intc_rtd1619b_suspend,
>> +     .resume_noirq  = realtek_intc_rtd1619b_resume, };
>
>So this is the 4th copy of the same code, really? Why is this not part of the
>common code?
>
Yes it is the same code. 
I had originally assumed that each platform would require some different settings, 
but I've realized that there is no such requirement in the current architecture.
I will move this part to the common code.

Thanks for your feedback.

Regards,
James

RE: [PATCH v3 3/6] irqchip: Introduce RTD1319 support using the Realtek common interrupt controller driver

[James Tai [戴志峰]]

Hi Rob,

>> --- /dev/null
>> +++ b/drivers/irqchip/irq-realtek-rtd1319.c
>> @@ -0,0 +1,218 @@
>> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
>> +/*
>> + * Realtek RTD1319 interrupt controller driver
>> + *
>> + * Copyright (c) 2023 Realtek Semiconductor Corporation  */
>> +
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of_device.h>
>
>You probably don't need this header and the implicit includes it makes are
>dropped now in linux-next. Please check what you actually need and make them
>explicit.
>
Thanks for the reminder. I will adjust it.

Regards,
James

RE: [PATCH v3 3/6] irqchip: Introduce RTD1319 support using the Realtek common interrupt controller driver

[James Tai [戴志峰]]

Hi Thomas,

>On Wed, Nov 29 2023 at 13:43, James Tai wrote:
>> Add support for the RTD1319 platform.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/202311061208.hJmxGqym-lkp@intel.
>> com/
>
>Again: These tags are just wrong.

I will remove it.

>> +static struct platform_driver realtek_intc_rtd1319_driver = {
>> +     .probe = rtd1319_intc_probe,
>> +     .driver = {
>> +             .name = "realtek_intc_rtd1319",
>> +             .of_match_table = realtek_intc_rtd1319_dt_matches,
>> +             .suppress_bind_attrs = true,
>> +             .pm = &realtek_intc_rtd1319_pm_ops,
>> +     },
>
>        .probe  = rtd1319_intc_probe,
>        .driver = {
>                .name                   = "realtek_intc_rtd1319",
>                .of_match_table         =
>realtek_intc_rtd1319_dt_matches,
>                ....
>
>Please.
>
I will fix it.

>> +};
>> +
>> +static int __init realtek_intc_rtd1319_init(void) {
>> +     return platform_driver_register(&realtek_intc_rtd1319_driver);
>> +}
>> +core_initcall(realtek_intc_rtd1319_init);
>
>What? This can be built as a module. So how is core_initcall() in any way correct
>here? module_init() perhaps?
>
I want the driver to be buildable as a module. Based on my test, the 'core_initcall()' works.
But, I will use 'module_init()' instead.

Thanks for your feedback.

Regards,
James

RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Thomas Gleixner]

On Tue, Dec 19 2023 at 03:15, James Tai [戴志峰] wrote:
>>On Wed, Nov 29 2023 at 13:43, James Tai wrote:
>>> +static inline void realtek_intc_clear_ints_bit(struct
>>> +realtek_intc_data *data, int bit) {
>>> +     writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
>>
>>That '& ~1' solves what aside of preventing bit 0 from being written?
>>
> The ISR register uses bit 0 to clear or set ISR status.
> Write 0 to clear bits and write 1 to set bits.
> Therefore, to clear the interrupt status, bit 0 should consistently be
> set to '0'.

And how does BIT(bit) with 1 <= bit <= 31 end up having bit 0 set?

Also a comment explaining the reasoning here would be helpful.

RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[James Tai [戴志峰]]

Hi Thomas,

>>>On Wed, Nov 29 2023 at 13:43, James Tai wrote:
>>>> +static inline void realtek_intc_clear_ints_bit(struct
>>>> +realtek_intc_data *data, int bit) {
>>>> +     writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
>>>
>>>That '& ~1' solves what aside of preventing bit 0 from being written?
>>>
>> The ISR register uses bit 0 to clear or set ISR status.
>> Write 0 to clear bits and write 1 to set bits.
>> Therefore, to clear the interrupt status, bit 0 should consistently be
>> set to '0'.
>
>And how does BIT(bit) with 1 <= bit <= 31 end up having bit 0 set?
>
>Also a comment explaining the reasoning here would be helpful.

To perform the clearing action in the ISR register, it's essential that bit 0 remains set to 0.
This is because bit 0 in the ISR register has a specific function for controlling the interrupt status.
When BIT(bit) is used with 1 <= bit <= 31, it generates a bitmask with only that particular bit set to 1, and all other bits, including bit 0, set to 0.
The '& ~1' operation is then applied to ensure that even if bit 0 was somehow set, it will be cleared, maintaining the register's functionality. 

For example, suppose the current value of the ISR register is 0x4, and we want to clear bit 3.
In that case, writing 0x4 (which represents bit 3 set to 1) to the ISR register will clear bit 3.

Thanks for your feedback.

Regards,
James