[PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Guo Ren]

This patch add C-SKY two interrupt conrollers.

 - irq-csky-apb-intc is a simple SOC interrupt controller which is
   used in a lot of C-SKY SOC products.

 - irq-csky-mpintc is C-SKY smp system interrupt controller and it
   could support 16 soft irqs, 16 private irqs, and 992 max common
   irqs.

Changelog:
 - add support-pulse-signal in irq-csky-apb-intc.c
 - change name with upstream feed-back
 - remove CSKY_VECIRQ_LEGENCY
 - change irq map, reserve soft_irq & private_irq space
 - add License and Copyright
 - change to generic irq chip framework
 - support set_affinity for irq balance in SMP
 - add INTC_IFR to clear irq-pending
 - use irq_domain_add_linear instead of leagcy

Signed-off-by: Guo Ren <ren_guo@c-sky.com>
---
 drivers/irqchip/Kconfig           |  16 +++
 drivers/irqchip/Makefile          |   2 +
 drivers/irqchip/irq-csky-mpintc.c | 191 ++++++++++++++++++++++++++++
 irq-csky-apb-intc.c               | 260 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 469 insertions(+)
 create mode 100644 drivers/irqchip/irq-csky-mpintc.c
 create mode 100644 irq-csky-apb-intc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 383e7b7..bf12549 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -371,6 +371,22 @@ config QCOM_PDC
 	  Power Domain Controller driver to manage and configure wakeup
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
+config CSKY_MPINTC
+	bool "C-SKY Multi Processor Interrupt Controller"
+	depends on CSKY
+	help
+	  Say yes here to enable C-SKY SMP interrupt controller driver used
+	  for C-SKY SMP system. In fact it's not mmio map and it use ld/st
+	  to visit the controller's register inside CPU.
+
+config CSKY_APB_INTC
+	bool "C-SKY APB Interrupt Controller"
+	depends on CSKY
+	help
+	  Say yes here to enable C-SKY APB interrupt controller driver used
+	  by C-SKY single core SOC system. It use mmio map apb-bus to visit
+	  the controller's register.
+
 endmenu
 
 config SIFIVE_PLIC
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index fbd1ec8..72eaf53 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -87,4 +87,6 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
 obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
+obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
new file mode 100644
index 0000000..2b2f75c
--- /dev/null
+++ b/drivers/irqchip/irq-csky-mpintc.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd.
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/module.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/traps.h>
+#include <asm/reg_ops.h>
+#include <asm/smp.h>
+
+static void __iomem *INTCG_base;
+static void __iomem *INTCL_base;
+
+#define COMM_IRQ_BASE	32
+
+#define INTCG_SIZE	0x8000
+#define INTCL_SIZE	0x1000
+#define INTC_SIZE	INTCL_SIZE*nr_cpu_ids + INTCG_SIZE
+
+#define INTCG_ICTLR	0x0
+#define INTCG_CICFGR	0x100
+#define INTCG_CIDSTR	0x1000
+
+#define INTCL_PICTLR	0x0
+#define INTCL_SIGR	0x60
+#define INTCL_HPPIR	0x68
+#define INTCL_RDYIR	0x6c
+#define INTCL_SENR	0xa0
+#define INTCL_CENR	0xa4
+#define INTCL_CACR	0xb4
+
+#define INTC_IRQS	256
+
+static DEFINE_PER_CPU(void __iomem *, intcl_reg);
+
+static void csky_mpintc_handler(struct pt_regs *regs)
+{
+	void __iomem *reg_base = this_cpu_read(intcl_reg);
+
+	do {
+		handle_domain_irq(NULL,
+				  readl_relaxed(reg_base + INTCL_RDYIR),
+				  regs);
+	} while (readl_relaxed(reg_base + INTCL_HPPIR) & BIT(31));
+}
+
+static void csky_mpintc_enable(struct irq_data *d)
+{
+	void __iomem *reg_base = this_cpu_read(intcl_reg);
+
+	writel_relaxed(d->hwirq, reg_base + INTCL_SENR);
+}
+
+static void csky_mpintc_disable(struct irq_data *d)
+{
+	void __iomem *reg_base = this_cpu_read(intcl_reg);
+
+	writel_relaxed(d->hwirq, reg_base + INTCL_CENR);
+}
+
+static void csky_mpintc_eoi(struct irq_data *d)
+{
+	void __iomem *reg_base = this_cpu_read(intcl_reg);
+
+	writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
+}
+
+#ifdef CONFIG_SMP
+static int csky_irq_set_affinity(struct irq_data *d,
+				 const struct cpumask *mask_val,
+				 bool force)
+{
+	unsigned int cpu;
+	unsigned int offset = 4 * (d->hwirq - COMM_IRQ_BASE);
+
+	if (!force)
+		cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	else
+		cpu = cpumask_first(mask_val);
+
+	if (cpu >= nr_cpu_ids)
+		return -EINVAL;
+
+	/* Enable interrupt destination */
+	cpu |= BIT(31);
+
+	writel_relaxed(cpu, INTCG_base + INTCG_CIDSTR + offset);
+
+	irq_data_update_effective_affinity(d, cpumask_of(cpu));
+
+	return IRQ_SET_MASK_OK_DONE;
+}
+#endif
+
+static struct irq_chip csky_irq_chip = {
+	.name           = "C-SKY SMP Intc V2",
+	.irq_eoi	= csky_mpintc_eoi,
+	.irq_enable	= csky_mpintc_enable,
+	.irq_disable	= csky_mpintc_disable,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = csky_irq_set_affinity,
+#endif
+};
+
+static int csky_irqdomain_map(struct irq_domain *d, unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	if(hwirq < COMM_IRQ_BASE) {
+		irq_set_percpu_devid(irq);
+		irq_set_chip_and_handler(irq, &csky_irq_chip, handle_percpu_irq);
+	} else {
+		irq_set_chip_and_handler(irq, &csky_irq_chip, handle_fasteoi_irq);
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops csky_irqdomain_ops = {
+	.map	= csky_irqdomain_map,
+	.xlate	= irq_domain_xlate_onecell,
+};
+
+#ifdef CONFIG_SMP
+static void csky_mpintc_send_ipi(const unsigned long *mask, unsigned long irq)
+{
+	void __iomem *reg_base = this_cpu_read(intcl_reg);
+
+	/*
+	 * INTCL_SIGR[3:0] INTID
+	 * INTCL_SIGR[8:15] CPUMASK
+	 */
+	writel_relaxed((*mask) << 8 | irq, reg_base + INTCL_SIGR);
+}
+#endif
+
+/* C-SKY multi processor interrupt controller */
+static int __init
+csky_mpintc_init(struct device_node *node, struct device_node *parent)
+{
+	struct irq_domain *root_domain;
+	unsigned int cpu, nr_irq;
+	int ret;
+
+	if (parent)
+		return 0;
+
+	ret = of_property_read_u32(node, "csky,num-irqs", &nr_irq);
+	if (ret < 0) {
+		nr_irq = INTC_IRQS;
+	}
+
+	if (INTCG_base == NULL) {
+		INTCG_base = ioremap(mfcr("cr<31, 14>"), INTC_SIZE);
+		if (INTCG_base == NULL)
+			return -EIO;
+
+		INTCL_base = INTCG_base + INTCG_SIZE;
+
+		writel_relaxed(BIT(0), INTCG_base + INTCG_ICTLR);
+	}
+
+	root_domain = irq_domain_add_linear(node, nr_irq, &csky_irqdomain_ops,
+					    NULL);
+	if (!root_domain)
+		return -ENXIO;
+
+	irq_set_default_host(root_domain);
+
+	/* for every cpu */
+	for_each_present_cpu(cpu) {
+		per_cpu(intcl_reg, cpu) = INTCL_base + (INTCL_SIZE * cpu);
+		writel_relaxed(BIT(0), per_cpu(intcl_reg, cpu) + INTCL_PICTLR);
+	}
+
+	set_handle_irq(&csky_mpintc_handler);
+
+#ifdef CONFIG_SMP
+	set_send_ipi(&csky_mpintc_send_ipi);
+#endif
+
+	return 0;
+}
+IRQCHIP_DECLARE(csky_mpintc, "csky,mpintc", csky_mpintc_init);
diff --git a/irq-csky-apb-intc.c b/irq-csky-apb-intc.c
new file mode 100644
index 0000000..d56e6b5
--- /dev/null
+++ b/irq-csky-apb-intc.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd.
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/module.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <asm/irq.h>
+#include <asm/io.h>
+
+#define INTC_IRQS		64
+
+#define CK_INTC_ICR		0x00
+#define CK_INTC_PEN31_00	0x14
+#define CK_INTC_PEN63_32	0x2c
+#define CK_INTC_NEN31_00	0x10
+#define CK_INTC_NEN63_32	0x28
+#define CK_INTC_SOURCE		0x40
+#define CK_INTC_DUAL_BASE	0x100
+
+#define GX_INTC_PEN31_00	0x00
+#define GX_INTC_PEN63_32	0x04
+#define GX_INTC_NEN31_00	0x40
+#define GX_INTC_NEN63_32	0x44
+#define GX_INTC_NMASK31_00	0x50
+#define GX_INTC_NMASK63_32	0x54
+#define GX_INTC_SOURCE		0x60
+
+static void __iomem *reg_base;
+static struct irq_domain *root_domain;
+
+static int nr_irq = INTC_IRQS;
+
+/*
+ * When controller support pulse signal, the PEN_reg will hold on signal
+ * without software trigger.
+ *
+ * So, to support pulse signal we need to clear IFR_reg and the address of
+ * IFR_offset is NEN_offset - 8.
+ */
+static void irq_ck_mask_set_bit(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	unsigned long ifr = ct->regs.mask - 8;
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	*ct->mask_cache |= mask;
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
+	irq_reg_writel(gc, irq_reg_readl(gc, ifr) & ~mask, ifr);
+	irq_gc_unlock(gc);
+}
+
+static void __init ck_set_gc(struct device_node *node, void __iomem *reg_base,
+			     u32 mask_reg, u32 irq_base)
+{
+	struct irq_chip_generic *gc;
+
+	gc = irq_get_domain_generic_chip(root_domain, irq_base);
+	gc->reg_base = reg_base;
+	gc->chip_types[0].regs.mask = mask_reg;
+	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
+	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
+
+	if (of_find_property(node, "support-pulse-signal", NULL))
+		gc->chip_types[0].chip.irq_unmask = irq_ck_mask_set_bit;
+}
+
+static inline u32 build_channel_val(u32 idx, u32 magic)
+{
+	u32 res;
+
+	/*
+	 * Set the same index for each channel
+	 */
+	res = idx | (idx << 8) | (idx << 16) | (idx << 24);
+
+	/*
+	 * Set the channel magic number in descending order.
+	 * The magic is 0x00010203 for ck-intc
+	 * The magic is 0x03020100 for gx6605s-intc
+	 */
+	return res | magic;
+}
+
+static inline void setup_irq_channel(u32 magic, void __iomem *reg_addr)
+{
+	u32 i;
+
+	/* Setup 64 channel slots */
+	for (i = 0; i < INTC_IRQS; i += 4) {
+		writel_relaxed(build_channel_val(i, magic), reg_addr + i);
+	}
+}
+
+static int __init
+ck_intc_init_comm(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+
+	if (parent) {
+		pr_err("C-SKY Intc not a root irq controller\n");
+		return -EINVAL;
+	}
+
+	reg_base = of_iomap(node, 0);
+	if (!reg_base) {
+		pr_err("C-SKY Intc unable to map: %p.\n", node);
+		return -EINVAL;
+	}
+
+	root_domain = irq_domain_add_linear(node, nr_irq, &irq_generic_chip_ops, NULL);
+	if (!root_domain) {
+		pr_err("C-SKY Intc irq_domain_add failed.\n");
+		return -ENOMEM;
+	}
+
+	ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
+					     "csky_intc", handle_level_irq,
+					     IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN,
+					     0, 0);
+	if (ret) {
+		pr_err("C-SKY Intc irq_alloc_gc failed.\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static inline int handle_irq_perbit(struct pt_regs *regs, u32 hwirq, u32 irq_base)
+{
+	u32 irq;
+
+	if (hwirq == 0) return 0;
+
+	while (hwirq) {
+		irq = __ffs(hwirq);
+		hwirq &= ~BIT(irq);
+		handle_domain_irq(root_domain, irq_base + irq, regs);
+	}
+
+	return 1;
+}
+
+/* gx6605s 64 irqs interrupt controller */
+static void gx_irq_handler(struct pt_regs *regs)
+{
+	int ret;
+
+	do {
+		ret  = handle_irq_perbit(regs ,readl_relaxed(reg_base + GX_INTC_PEN31_00), 0);
+		ret |= handle_irq_perbit(regs ,readl_relaxed(reg_base + GX_INTC_PEN63_32), 32);
+	} while(ret);
+}
+
+static int __init
+gx_intc_init(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+
+	ret = ck_intc_init_comm(node, parent);
+	if (ret)
+		return ret;
+
+	/* Initial enable reg to disable all interrupts */
+	writel_relaxed(0x0, reg_base + GX_INTC_NEN31_00);
+	writel_relaxed(0x0, reg_base + GX_INTC_NEN63_32);
+
+	/* Initial mask reg with all unmasked, becasue we only use enalbe reg */
+	writel_relaxed(0x0, reg_base + GX_INTC_NMASK31_00);
+	writel_relaxed(0x0, reg_base + GX_INTC_NMASK63_32);
+
+	setup_irq_channel(0x03020100, reg_base + GX_INTC_SOURCE);
+
+	ck_set_gc(node, reg_base, GX_INTC_NEN31_00, 0);
+	ck_set_gc(node, reg_base, GX_INTC_NEN63_32, 32);
+
+	set_handle_irq(gx_irq_handler);
+
+	return 0;
+}
+IRQCHIP_DECLARE(csky_gx6605s_intc, "csky,gx6605s-intc", gx_intc_init);
+
+/* C-SKY simple 64 irqs interrupt controller, dual-together could support 128 irqs */
+static void ck_irq_handler(struct pt_regs *regs)
+{
+	int ret;
+
+	do {
+		/* handle 0 - 31 irqs */
+		ret  = handle_irq_perbit(regs, readl_relaxed(reg_base + CK_INTC_PEN31_00), 0);
+		ret |= handle_irq_perbit(regs, readl_relaxed(reg_base + CK_INTC_PEN63_32), 32);
+
+		if (nr_irq == INTC_IRQS) continue;
+
+		/* handle 64 - 127 irqs */
+		ret |= handle_irq_perbit(regs,
+			   readl_relaxed(reg_base + CK_INTC_PEN31_00 + CK_INTC_DUAL_BASE), 64);
+		ret |= handle_irq_perbit(regs,
+			   readl_relaxed(reg_base + CK_INTC_PEN63_32 + CK_INTC_DUAL_BASE), 96);
+	} while(ret);
+}
+
+static int __init
+ck_intc_init(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+
+	ret = ck_intc_init_comm(node, parent);
+	if (ret)
+		return ret;
+
+	/* Initial enable reg to disable all interrupts */
+	writel_relaxed(0, reg_base + CK_INTC_NEN31_00);
+	writel_relaxed(0, reg_base + CK_INTC_NEN63_32);
+
+	/* Enable irq intc */
+	writel_relaxed(BIT(31), reg_base + CK_INTC_ICR);
+
+	ck_set_gc(node, reg_base, CK_INTC_NEN31_00, 0);
+	ck_set_gc(node, reg_base, CK_INTC_NEN63_32, 32);
+
+	setup_irq_channel(0x00010203, reg_base + CK_INTC_SOURCE);
+
+	set_handle_irq(ck_irq_handler);
+
+	return 0;
+}
+IRQCHIP_DECLARE(ck_intc, "csky,apb-intc", ck_intc_init);
+
+static int __init
+ck_dual_intc_init(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+
+	/* dual-apb-intc up to 128 irq sources*/
+	nr_irq = INTC_IRQS * 2;
+
+	ret = ck_intc_init(node, parent);
+	if (ret)
+		return ret;
+
+	/* Initial enable reg to disable all interrupts */
+	writel_relaxed(0, reg_base + CK_INTC_NEN31_00 + CK_INTC_DUAL_BASE);
+	writel_relaxed(0, reg_base + CK_INTC_NEN63_32 + CK_INTC_DUAL_BASE);
+
+	ck_set_gc(node, reg_base + CK_INTC_DUAL_BASE, CK_INTC_NEN31_00, 64);
+	ck_set_gc(node, reg_base + CK_INTC_DUAL_BASE, CK_INTC_NEN63_32, 96);
+
+	setup_irq_channel(0x00010203, reg_base + CK_INTC_SOURCE + CK_INTC_DUAL_BASE);
+
+	return 0;
+}
+IRQCHIP_DECLARE(ck_dual_intc, "csky,dual-apb-intc", ck_dual_intc_init);
-- 
2.7.4

[PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Guo Ren]

Signed-off-by: Guo Ren <ren_guo@c-sky.com>
---
 .../interrupt-controller/csky,apb-intc.txt         | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
new file mode 100644
index 0000000..be7c3d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
@@ -0,0 +1,70 @@
+==============================
+C-SKY APB Interrupt Controller
+==============================
+
+C-SKY APB Interrupt Controller is a simple soc interrupt controller
+on the apb bus and we only use it as root irq controller.
+
+ - csky,apb-intc is used in a lot of csky fpgas and socs, it support 64 irq nums.
+ - csky,dual-apb-intc consists of 2 apb-intc and 128 irq nums supported.
+ - csky,gx6605s-intc is gx6605s soc internal irq interrupt controller, 64 irq nums.
+
+=============================
+intc node bindings definition
+=============================
+
+        Description: Describes APB interrupt controller
+
+        PROPERTIES
+
+        - compatible
+                Usage: required
+                Value type: <string>
+                Definition: must be "csky,apb-intc"
+				    "csky,dual-apb-intc"
+				    "csky,gx6605s-intc"
+        - #interrupt-cells
+                Usage: required
+                Value type: <u32>
+                Definition: must be <1>
+	- reg
+		Usage: required
+		Value type: <u32 u32>
+                Definition: <phyaddr size> in soc from cpu view
+        - interrupt-controller:
+                Usage: required
+        - support-pulse-signal:
+                Usage: select
+		Description: to support pulse signal flag
+
+Examples:
+---------
+
+	intc: interrupt-controller@500000 {
+		compatible = "csky,apb-intc";
+		#interrupt-cells = <1>;
+		reg = <0x00500000 0x400>;
+		interrupt-controller;
+	};
+
+	intc: interrupt-controller@500000 {
+		compatible = "csky,apb-intc";
+		#interrupt-cells = <1>;
+		reg = <0x00500000 0x400>;
+		interrupt-controller;
+		support-pulse-signal;
+	};
+
+	intc: interrupt-controller@500000 {
+		compatible = "csky,dual-apb-intc";
+		#interrupt-cells = <1>;
+		reg = <0x00500000 0x400>;
+		interrupt-controller;
+	};
+
+	intc: interrupt-controller@500000 {
+		compatible = "csky,gx6605s-intc";
+		#interrupt-cells = <1>;
+		reg = <0x00500000 0x400>;
+		interrupt-controller;
+	};
-- 
2.7.4

[PATCH V5 3/3] dt-bindings: interrupt-controller: C-SKY SMP intc

[Guo Ren]

Signed-off-by: Guo Ren <ren_guo@c-sky.com>
---
 .../bindings/interrupt-controller/csky,mpintc.txt  | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
new file mode 100644
index 0000000..49d1658
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
@@ -0,0 +1,40 @@
+===========================================
+C-SKY Multi-processors Interrupt Controller
+===========================================
+
+C-SKY Multi-processors Interrupt Controller is designed for ck807/ck810/ck860
+SMP soc, and it also could be used in non-SMP system.
+
+Interrupt number definition:
+
+  0-15  : software irq, and we use 15 as our IPI_IRQ.
+ 16-31  : private  irq, and we use 16 as the co-processor timer.
+ 31-1024: common irq for soc ip.
+
+=============================
+intc node bindings definition
+=============================
+
+	Description: Describes SMP interrupt controller
+
+	PROPERTIES
+
+        - compatible
+		Usage: required
+                Value type: <string>
+                Definition: must be "csky,mpintc"
+        - interrupt-cells
+                Usage: required
+                Value type: <u32>
+                Definition: must be <1>
+        - interrupt-controller:
+                Usage: required
+
+Examples:
+---------
+
+	intc: interrupt-controller {
+		compatible = "csky,mpintc";
+		#interrupt-cells = <1>;
+		interrupt-controller;
+	};
-- 
2.7.4

Re: [PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Marc Zyngier]

On Sun, 16 Sep 2018 09:50:02 +0100,
Guo Ren <ren_guo@c-sky.com> wrote:
> 
> This patch add C-SKY two interrupt conrollers.
> 
>  - irq-csky-apb-intc is a simple SOC interrupt controller which is
>    used in a lot of C-SKY SOC products.
> 
>  - irq-csky-mpintc is C-SKY smp system interrupt controller and it
>    could support 16 soft irqs, 16 private irqs, and 992 max common
>    irqs.
> 
> Changelog:
>  - add support-pulse-signal in irq-csky-apb-intc.c
>  - change name with upstream feed-back
>  - remove CSKY_VECIRQ_LEGENCY
>  - change irq map, reserve soft_irq & private_irq space
>  - add License and Copyright
>  - change to generic irq chip framework
>  - support set_affinity for irq balance in SMP
>  - add INTC_IFR to clear irq-pending
>  - use irq_domain_add_linear instead of leagcy
> 
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> ---
>  drivers/irqchip/Kconfig           |  16 +++
>  drivers/irqchip/Makefile          |   2 +
>  drivers/irqchip/irq-csky-mpintc.c | 191 ++++++++++++++++++++++++++++
>  irq-csky-apb-intc.c               | 260 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 469 insertions(+)
>  create mode 100644 drivers/irqchip/irq-csky-mpintc.c
>  create mode 100644 irq-csky-apb-intc.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 383e7b7..bf12549 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -371,6 +371,22 @@ config QCOM_PDC
>  	  Power Domain Controller driver to manage and configure wakeup
>  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>  
> +config CSKY_MPINTC
> +	bool "C-SKY Multi Processor Interrupt Controller"
> +	depends on CSKY
> +	help
> +	  Say yes here to enable C-SKY SMP interrupt controller driver used
> +	  for C-SKY SMP system. In fact it's not mmio map and it use ld/st
> +	  to visit the controller's register inside CPU.
> +
> +config CSKY_APB_INTC
> +	bool "C-SKY APB Interrupt Controller"
> +	depends on CSKY
> +	help
> +	  Say yes here to enable C-SKY APB interrupt controller driver used
> +	  by C-SKY single core SOC system. It use mmio map apb-bus to visit
> +	  the controller's register.
> +
>  endmenu
>  
>  config SIFIVE_PLIC
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index fbd1ec8..72eaf53 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -87,4 +87,6 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>  obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
> +obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
> new file mode 100644
> index 0000000..2b2f75c
> --- /dev/null
> +++ b/drivers/irqchip/irq-csky-mpintc.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd.
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/module.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <asm/irq.h>
> +#include <asm/io.h>
> +#include <asm/traps.h>
> +#include <asm/reg_ops.h>
> +#include <asm/smp.h>
> +
> +static void __iomem *INTCG_base;
> +static void __iomem *INTCL_base;
> +
> +#define COMM_IRQ_BASE	32
> +
> +#define INTCG_SIZE	0x8000
> +#define INTCL_SIZE	0x1000
> +#define INTC_SIZE	INTCL_SIZE*nr_cpu_ids + INTCG_SIZE
> +
> +#define INTCG_ICTLR	0x0
> +#define INTCG_CICFGR	0x100
> +#define INTCG_CIDSTR	0x1000
> +
> +#define INTCL_PICTLR	0x0
> +#define INTCL_SIGR	0x60
> +#define INTCL_HPPIR	0x68
> +#define INTCL_RDYIR	0x6c
> +#define INTCL_SENR	0xa0
> +#define INTCL_CENR	0xa4
> +#define INTCL_CACR	0xb4
> +
> +#define INTC_IRQS	256
> +
> +static DEFINE_PER_CPU(void __iomem *, intcl_reg);
> +
> +static void csky_mpintc_handler(struct pt_regs *regs)
> +{
> +	void __iomem *reg_base = this_cpu_read(intcl_reg);
> +
> +	do {
> +		handle_domain_irq(NULL,

It is definitely odd to call into handle_domain_irq without a
domain. A new architecture (which C-SKY apparently is) shouldn't
depend on this, and should always provide a domain.

> +				  readl_relaxed(reg_base + INTCL_RDYIR),
> +				  regs);
> +	} while (readl_relaxed(reg_base + INTCL_HPPIR) & BIT(31));
> +}
> +
> +static void csky_mpintc_enable(struct irq_data *d)
> +{
> +	void __iomem *reg_base = this_cpu_read(intcl_reg);
> +
> +	writel_relaxed(d->hwirq, reg_base + INTCL_SENR);
> +}
> +
> +static void csky_mpintc_disable(struct irq_data *d)
> +{
> +	void __iomem *reg_base = this_cpu_read(intcl_reg);
> +
> +	writel_relaxed(d->hwirq, reg_base + INTCL_CENR);
> +}
> +
> +static void csky_mpintc_eoi(struct irq_data *d)
> +{
> +	void __iomem *reg_base = this_cpu_read(intcl_reg);
> +
> +	writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
> +}
> +
> +#ifdef CONFIG_SMP
> +static int csky_irq_set_affinity(struct irq_data *d,
> +				 const struct cpumask *mask_val,
> +				 bool force)
> +{
> +	unsigned int cpu;
> +	unsigned int offset = 4 * (d->hwirq - COMM_IRQ_BASE);
> +
> +	if (!force)
> +		cpu = cpumask_any_and(mask_val, cpu_online_mask);
> +	else
> +		cpu = cpumask_first(mask_val);
> +
> +	if (cpu >= nr_cpu_ids)
> +		return -EINVAL;
> +
> +	/* Enable interrupt destination */
> +	cpu |= BIT(31);
> +
> +	writel_relaxed(cpu, INTCG_base + INTCG_CIDSTR + offset);
> +
> +	irq_data_update_effective_affinity(d, cpumask_of(cpu));
> +
> +	return IRQ_SET_MASK_OK_DONE;
> +}
> +#endif
> +
> +static struct irq_chip csky_irq_chip = {
> +	.name           = "C-SKY SMP Intc V2",
> +	.irq_eoi	= csky_mpintc_eoi,
> +	.irq_enable	= csky_mpintc_enable,
> +	.irq_disable	= csky_mpintc_disable,
> +#ifdef CONFIG_SMP
> +	.irq_set_affinity = csky_irq_set_affinity,
> +#endif
> +};
> +
> +static int csky_irqdomain_map(struct irq_domain *d, unsigned int irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	if(hwirq < COMM_IRQ_BASE) {
> +		irq_set_percpu_devid(irq);
> +		irq_set_chip_and_handler(irq, &csky_irq_chip, handle_percpu_irq);
> +	} else {
> +		irq_set_chip_and_handler(irq, &csky_irq_chip, handle_fasteoi_irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops csky_irqdomain_ops = {
> +	.map	= csky_irqdomain_map,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +
> +#ifdef CONFIG_SMP
> +static void csky_mpintc_send_ipi(const unsigned long *mask, unsigned long irq)
> +{
> +	void __iomem *reg_base = this_cpu_read(intcl_reg);
> +
> +	/*
> +	 * INTCL_SIGR[3:0] INTID
> +	 * INTCL_SIGR[8:15] CPUMASK
> +	 */
> +	writel_relaxed((*mask) << 8 | irq, reg_base + INTCL_SIGR);
> +}
> +#endif
> +
> +/* C-SKY multi processor interrupt controller */
> +static int __init
> +csky_mpintc_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *root_domain;
> +	unsigned int cpu, nr_irq;
> +	int ret;
> +
> +	if (parent)
> +		return 0;
> +
> +	ret = of_property_read_u32(node, "csky,num-irqs", &nr_irq);
> +	if (ret < 0) {
> +		nr_irq = INTC_IRQS;
> +	}

Drop the extra braces.

> +
> +	if (INTCG_base == NULL) {
> +		INTCG_base = ioremap(mfcr("cr<31, 14>"), INTC_SIZE);
> +		if (INTCG_base == NULL)
> +			return -EIO;
> +
> +		INTCL_base = INTCG_base + INTCG_SIZE;
> +
> +		writel_relaxed(BIT(0), INTCG_base + INTCG_ICTLR);
> +	}
> +
> +	root_domain = irq_domain_add_linear(node, nr_irq, &csky_irqdomain_ops,
> +					    NULL);
> +	if (!root_domain)
> +		return -ENXIO;
> +
> +	irq_set_default_host(root_domain);

Please drop this. There is no reason to use this on any modern, DT
based architecture.

> +
> +	/* for every cpu */
> +	for_each_present_cpu(cpu) {
> +		per_cpu(intcl_reg, cpu) = INTCL_base + (INTCL_SIZE * cpu);
> +		writel_relaxed(BIT(0), per_cpu(intcl_reg, cpu) + INTCL_PICTLR);
> +	}
> +
> +	set_handle_irq(&csky_mpintc_handler);
> +
> +#ifdef CONFIG_SMP
> +	set_send_ipi(&csky_mpintc_send_ipi);
> +#endif
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(csky_mpintc, "csky,mpintc", csky_mpintc_init);
> diff --git a/irq-csky-apb-intc.c b/irq-csky-apb-intc.c
> new file mode 100644
> index 0000000..d56e6b5
> --- /dev/null
> +++ b/irq-csky-apb-intc.c

This is a separate driver, right? Please make it a separate patch.

> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd.
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/module.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <asm/irq.h>
> +#include <asm/io.h>
> +
> +#define INTC_IRQS		64
> +
> +#define CK_INTC_ICR		0x00
> +#define CK_INTC_PEN31_00	0x14
> +#define CK_INTC_PEN63_32	0x2c
> +#define CK_INTC_NEN31_00	0x10
> +#define CK_INTC_NEN63_32	0x28
> +#define CK_INTC_SOURCE		0x40
> +#define CK_INTC_DUAL_BASE	0x100
> +
> +#define GX_INTC_PEN31_00	0x00
> +#define GX_INTC_PEN63_32	0x04
> +#define GX_INTC_NEN31_00	0x40
> +#define GX_INTC_NEN63_32	0x44
> +#define GX_INTC_NMASK31_00	0x50
> +#define GX_INTC_NMASK63_32	0x54
> +#define GX_INTC_SOURCE		0x60
> +
> +static void __iomem *reg_base;
> +static struct irq_domain *root_domain;
> +
> +static int nr_irq = INTC_IRQS;
> +
> +/*
> + * When controller support pulse signal, the PEN_reg will hold on signal
> + * without software trigger.
> + *
> + * So, to support pulse signal we need to clear IFR_reg and the address of
> + * IFR_offset is NEN_offset - 8.
> + */
> +static void irq_ck_mask_set_bit(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	unsigned long ifr = ct->regs.mask - 8;
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	*ct->mask_cache |= mask;
> +	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
> +	irq_reg_writel(gc, irq_reg_readl(gc, ifr) & ~mask, ifr);
> +	irq_gc_unlock(gc);
> +}
> +
> +static void __init ck_set_gc(struct device_node *node, void __iomem *reg_base,
> +			     u32 mask_reg, u32 irq_base)
> +{
> +	struct irq_chip_generic *gc;
> +
> +	gc = irq_get_domain_generic_chip(root_domain, irq_base);
> +	gc->reg_base = reg_base;
> +	gc->chip_types[0].regs.mask = mask_reg;
> +	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
> +	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
> +
> +	if (of_find_property(node, "support-pulse-signal", NULL))
> +		gc->chip_types[0].chip.irq_unmask = irq_ck_mask_set_bit;
> +}
> +
> +static inline u32 build_channel_val(u32 idx, u32 magic)
> +{
> +	u32 res;
> +
> +	/*
> +	 * Set the same index for each channel
> +	 */
> +	res = idx | (idx << 8) | (idx << 16) | (idx << 24);
> +
> +	/*
> +	 * Set the channel magic number in descending order.
> +	 * The magic is 0x00010203 for ck-intc
> +	 * The magic is 0x03020100 for gx6605s-intc
> +	 */
> +	return res | magic;
> +}
> +
> +static inline void setup_irq_channel(u32 magic, void __iomem *reg_addr)
> +{
> +	u32 i;
> +
> +	/* Setup 64 channel slots */
> +	for (i = 0; i < INTC_IRQS; i += 4) {
> +		writel_relaxed(build_channel_val(i, magic), reg_addr + i);
> +	}
> +}
> +
> +static int __init
> +ck_intc_init_comm(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +
> +	if (parent) {
> +		pr_err("C-SKY Intc not a root irq controller\n");
> +		return -EINVAL;
> +	}
> +
> +	reg_base = of_iomap(node, 0);
> +	if (!reg_base) {
> +		pr_err("C-SKY Intc unable to map: %p.\n", node);
> +		return -EINVAL;
> +	}
> +
> +	root_domain = irq_domain_add_linear(node, nr_irq, &irq_generic_chip_ops, NULL);
> +	if (!root_domain) {
> +		pr_err("C-SKY Intc irq_domain_add failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
> +					     "csky_intc", handle_level_irq,
> +					     IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN,
> +					     0, 0);
> +	if (ret) {
> +		pr_err("C-SKY Intc irq_alloc_gc failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int handle_irq_perbit(struct pt_regs *regs, u32 hwirq, u32 irq_base)

Consider using bool as the return type, and use true/false as return
values.

> +{
> +	u32 irq;
> +
> +	if (hwirq == 0) return 0;
> +
> +	while (hwirq) {
> +		irq = __ffs(hwirq);
> +		hwirq &= ~BIT(irq);
> +		handle_domain_irq(root_domain, irq_base + irq, regs);
> +	}
> +
> +	return 1;
> +}
> +
> +/* gx6605s 64 irqs interrupt controller */
> +static void gx_irq_handler(struct pt_regs *regs)
> +{
> +	int ret;

bool.

> +
> +	do {
> +		ret  = handle_irq_perbit(regs ,readl_relaxed(reg_base + GX_INTC_PEN31_00), 0);
> +		ret |= handle_irq_perbit(regs ,readl_relaxed(reg_base + GX_INTC_PEN63_32), 32);
> +	} while(ret);
> +}
> +
> +static int __init
> +gx_intc_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +
> +	ret = ck_intc_init_comm(node, parent);
> +	if (ret)
> +		return ret;
> +
> +	/* Initial enable reg to disable all interrupts */
> +	writel_relaxed(0x0, reg_base + GX_INTC_NEN31_00);
> +	writel_relaxed(0x0, reg_base + GX_INTC_NEN63_32);
> +
> +	/* Initial mask reg with all unmasked, becasue we only use enalbe reg */
> +	writel_relaxed(0x0, reg_base + GX_INTC_NMASK31_00);
> +	writel_relaxed(0x0, reg_base + GX_INTC_NMASK63_32);
> +
> +	setup_irq_channel(0x03020100, reg_base + GX_INTC_SOURCE);
> +
> +	ck_set_gc(node, reg_base, GX_INTC_NEN31_00, 0);
> +	ck_set_gc(node, reg_base, GX_INTC_NEN63_32, 32);
> +
> +	set_handle_irq(gx_irq_handler);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(csky_gx6605s_intc, "csky,gx6605s-intc", gx_intc_init);
> +
> +/* C-SKY simple 64 irqs interrupt controller, dual-together could support 128 irqs */
> +static void ck_irq_handler(struct pt_regs *regs)
> +{
> +	int ret;

bool.

> +
> +	do {
> +		/* handle 0 - 31 irqs */
> +		ret  = handle_irq_perbit(regs, readl_relaxed(reg_base + CK_INTC_PEN31_00), 0);
> +		ret |= handle_irq_perbit(regs, readl_relaxed(reg_base + CK_INTC_PEN63_32), 32);
> +
> +		if (nr_irq == INTC_IRQS) continue;
> +
> +		/* handle 64 - 127 irqs */
> +		ret |= handle_irq_perbit(regs,
> +			   readl_relaxed(reg_base + CK_INTC_PEN31_00 + CK_INTC_DUAL_BASE), 64);
> +		ret |= handle_irq_perbit(regs,
> +			   readl_relaxed(reg_base + CK_INTC_PEN63_32 + CK_INTC_DUAL_BASE), 96);
> +	} while(ret);
> +}
> +
> +static int __init
> +ck_intc_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +
> +	ret = ck_intc_init_comm(node, parent);
> +	if (ret)
> +		return ret;
> +
> +	/* Initial enable reg to disable all interrupts */
> +	writel_relaxed(0, reg_base + CK_INTC_NEN31_00);
> +	writel_relaxed(0, reg_base + CK_INTC_NEN63_32);
> +
> +	/* Enable irq intc */
> +	writel_relaxed(BIT(31), reg_base + CK_INTC_ICR);
> +
> +	ck_set_gc(node, reg_base, CK_INTC_NEN31_00, 0);
> +	ck_set_gc(node, reg_base, CK_INTC_NEN63_32, 32);
> +
> +	setup_irq_channel(0x00010203, reg_base + CK_INTC_SOURCE);
> +
> +	set_handle_irq(ck_irq_handler);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(ck_intc, "csky,apb-intc", ck_intc_init);
> +
> +static int __init
> +ck_dual_intc_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +
> +	/* dual-apb-intc up to 128 irq sources*/
> +	nr_irq = INTC_IRQS * 2;
> +
> +	ret = ck_intc_init(node, parent);
> +	if (ret)
> +		return ret;
> +
> +	/* Initial enable reg to disable all interrupts */
> +	writel_relaxed(0, reg_base + CK_INTC_NEN31_00 + CK_INTC_DUAL_BASE);
> +	writel_relaxed(0, reg_base + CK_INTC_NEN63_32 + CK_INTC_DUAL_BASE);
> +
> +	ck_set_gc(node, reg_base + CK_INTC_DUAL_BASE, CK_INTC_NEN31_00, 64);
> +	ck_set_gc(node, reg_base + CK_INTC_DUAL_BASE, CK_INTC_NEN63_32, 96);
> +
> +	setup_irq_channel(0x00010203, reg_base + CK_INTC_SOURCE + CK_INTC_DUAL_BASE);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(ck_dual_intc, "csky,dual-apb-intc", ck_dual_intc_init);
> -- 
> 2.7.4
> 

Thanks,

	M.

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

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Marc Zyngier]

On Sun, 16 Sep 2018 09:50:03 +0100,
Guo Ren <ren_guo@c-sky.com> wrote:
> 
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>

Please write a commit message. Same thing for the following patch.

Thanks,

	M.

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

Re: [PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Guo Ren]

On Sun, Sep 16, 2018 at 08:07:55PM +0100, Marc Zyngier wrote:
> On Sun, 16 Sep 2018 09:50:02 +0100,
> Guo Ren <ren_guo@c-sky.com> wrote:
> > +static void csky_mpintc_handler(struct pt_regs *regs)
> > +{
> > +	void __iomem *reg_base = this_cpu_read(intcl_reg);
> > +
> > +	do {
> > +		handle_domain_irq(NULL,
> 
> It is definitely odd to call into handle_domain_irq without a
> domain. A new architecture (which C-SKY apparently is) shouldn't
> depend on this, and should always provide a domain.
Ok, I'll change it to handle_domain_irq(root_domain and prepare the
root_domain definition.

> > +	ret = of_property_read_u32(node, "csky,num-irqs", &nr_irq);
> > +	if (ret < 0) {
> > +		nr_irq = INTC_IRQS;
> > +	}
> 
> Drop the extra braces.
Ok, and change it to: 
	if (ret < 0)
		nr_irq = INTC_IRQS

> > +
> > +	irq_set_default_host(root_domain);
> 
> Please drop this. There is no reason to use this on any modern, DT
> based architecture.
Please let me keep this and in my arch/csky/kernel/smp.c:

void __init setup_smp_ipi(void)
{
	...
	irq_create_mapping(NULL, IPI_IRQ);

I need irq_set_default_host to pass the domain and this api is used by
a lot of drivers:

$ grep irq_set_default_host drivers/irqchip -r | wc -l
16

In future we could find a better solution.

> > diff --git a/irq-csky-apb-intc.c b/irq-csky-apb-intc.c
> > new file mode 100644
> > index 0000000..d56e6b5
> > --- /dev/null
> > +++ b/irq-csky-apb-intc.c
> 
> This is a separate driver, right? Please make it a separate patch.
Ok, no problem.

> > +static inline int handle_irq_perbit(struct pt_regs *regs, u32 hwirq, u32 irq_base)
> 
> Consider using bool as the return type, and use true/false as return
> values.
Ok, no problem.


> > +	int ret;
> 
> bool.
Ok, no problem.


> > +	int ret;
> 
> bool.
Ok, no problem.

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Guo Ren]

On Sun, Sep 16, 2018 at 08:27:01PM +0100, Marc Zyngier wrote:
> On Sun, 16 Sep 2018 09:50:03 +0100,
> Guo Ren <ren_guo@c-sky.com> wrote:
> > 
> > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> 
> Please write a commit message. Same thing for the following patch.
Ok.

Best Regards
 Guo Ren

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Rob Herring]

On Sun, Sep 16, 2018 at 04:50:03PM +0800, Guo Ren wrote:
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>

Needs a commit description.

> ---
>  .../interrupt-controller/csky,apb-intc.txt         | 70 ++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> new file mode 100644
> index 0000000..be7c3d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> @@ -0,0 +1,70 @@
> +==============================
> +C-SKY APB Interrupt Controller
> +==============================
> +
> +C-SKY APB Interrupt Controller is a simple soc interrupt controller
> +on the apb bus and we only use it as root irq controller.
> +
> + - csky,apb-intc is used in a lot of csky fpgas and socs, it support 64 irq nums.
> + - csky,dual-apb-intc consists of 2 apb-intc and 128 irq nums supported.
> + - csky,gx6605s-intc is gx6605s soc internal irq interrupt controller, 64 irq nums.

Is there a relationship between csky,gx6605s-intc and csky,apb-intc?

> +
> +=============================
> +intc node bindings definition
> +=============================
> +
> +        Description: Describes APB interrupt controller
> +
> +        PROPERTIES
> +
> +        - compatible
> +                Usage: required
> +                Value type: <string>
> +                Definition: must be "csky,apb-intc"
> +				    "csky,dual-apb-intc"
> +				    "csky,gx6605s-intc"
> +        - #interrupt-cells
> +                Usage: required
> +                Value type: <u32>
> +                Definition: must be <1>
> +	- reg
> +		Usage: required
> +		Value type: <u32 u32>
> +                Definition: <phyaddr size> in soc from cpu view
> +        - interrupt-controller:
> +                Usage: required
> +        - support-pulse-signal:
> +                Usage: select
> +		Description: to support pulse signal flag

What is this for?

> +
> +Examples:
> +---------
> +
> +	intc: interrupt-controller@500000 {
> +		compatible = "csky,apb-intc";
> +		#interrupt-cells = <1>;
> +		reg = <0x00500000 0x400>;
> +		interrupt-controller;
> +	};
> +
> +	intc: interrupt-controller@500000 {
> +		compatible = "csky,apb-intc";
> +		#interrupt-cells = <1>;
> +		reg = <0x00500000 0x400>;
> +		interrupt-controller;
> +		support-pulse-signal;
> +	};

Not really worth 2 examples for just 1 property difference.

> +
> +	intc: interrupt-controller@500000 {
> +		compatible = "csky,dual-apb-intc";
> +		#interrupt-cells = <1>;
> +		reg = <0x00500000 0x400>;
> +		interrupt-controller;
> +	};
> +
> +	intc: interrupt-controller@500000 {
> +		compatible = "csky,gx6605s-intc";
> +		#interrupt-cells = <1>;
> +		reg = <0x00500000 0x400>;
> +		interrupt-controller;
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Guo Ren]

On Mon, Sep 17, 2018 at 02:23:36AM -0400, Rob Herring wrote:
> On Sun, Sep 16, 2018 at 04:50:03PM +0800, Guo Ren wrote:
> > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> 
> Needs a commit description.
> 
> > ---
> >  .../interrupt-controller/csky,apb-intc.txt         | 70 ++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > new file mode 100644
> > index 0000000..be7c3d1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > @@ -0,0 +1,70 @@
> > +==============================
> > +C-SKY APB Interrupt Controller
> > +==============================
> > +
> > +C-SKY APB Interrupt Controller is a simple soc interrupt controller
> > +on the apb bus and we only use it as root irq controller.
> > +
> > + - csky,apb-intc is used in a lot of csky fpgas and socs, it support 64 irq nums.
> > + - csky,dual-apb-intc consists of 2 apb-intc and 128 irq nums supported.
> > + - csky,gx6605s-intc is gx6605s soc internal irq interrupt controller, 64 irq nums.
> 
> Is there a relationship between csky,gx6605s-intc and csky,apb-intc?
They all use pending register to get irq num and use enable register to
mask/unmask.

> > +        - support-pulse-signal:
> > +                Usage: select
> > +		Description: to support pulse signal flag
> 
> What is this for?
This is a level-triger interrupt controller at first, but we want it
to support pulse signal. It means that when the pulse signal coming,
the pending register will hold signals without clear the IFR reg.

Some C-SKY cpu's socs need this feature to support pulse interrupt
signal.

> > +	intc: interrupt-controller@500000 {
> > +		compatible = "csky,apb-intc";
> > +		#interrupt-cells = <1>;
> > +		reg = <0x00500000 0x400>;
> > +		interrupt-controller;
> > +	};
> Not really worth 2 examples for just 1 property difference.
Ok.
I'll retain the above, because only a few socs use support-pulse-signal.

Re: [PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Marc Zyngier]

On Mon, 17 Sep 2018 03:09:29 +0100,
Guo Ren <ren_guo@c-sky.com> wrote:

[...]

> > > +
> > > +	irq_set_default_host(root_domain);
> > 
> > Please drop this. There is no reason to use this on any modern, DT
> > based architecture.
> Please let me keep this and in my arch/csky/kernel/smp.c:
> 
> void __init setup_smp_ipi(void)
> {
> 	...
> 	irq_create_mapping(NULL, IPI_IRQ);

This looks quite wrong. Reading the code at
https://lkml.org/lkml/2018/9/12/674, it really looks like you're
assuming that IPI_IRQ will be mapped to a Linux IRQ with the same
number. Nothing could be farther from the truth.

The Linux IRQ is returned as the result of irq_create_mapping, which
you're ignoring. You'd be better off creating this mapping from the
irqchip code, and expose the resulting Linux IRQ to oyu SMP code by
any mean of your choice (such as moving the send_ipi_message into the
irqchip code as well).

> 
> I need irq_set_default_host to pass the domain and this api is used by
> a lot of drivers:
> 
> $ grep irq_set_default_host drivers/irqchip -r | wc -l
> 16

These are all legacy calls, and predates DT conversion. In a number of
cases, these calls could be removed.

> In future we could find a better solution.

I respectfully disagree. There is strictly no reason to merge a new
architecture relying on deprecated features, specially as the code
appears to be buggy. Now is the right time to fix it.

Thanks,

	M.

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

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Rob Herring]

On Mon, Sep 17, 2018 at 1:36 AM Guo Ren <ren_guo@c-sky.com> wrote:
>
> On Mon, Sep 17, 2018 at 02:23:36AM -0400, Rob Herring wrote:
> > On Sun, Sep 16, 2018 at 04:50:03PM +0800, Guo Ren wrote:
> > > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> >
> > Needs a commit description.
> >
> > > ---
> > >  .../interrupt-controller/csky,apb-intc.txt         | 70 ++++++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > new file mode 100644
> > > index 0000000..be7c3d1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > @@ -0,0 +1,70 @@
> > > +==============================
> > > +C-SKY APB Interrupt Controller
> > > +==============================
> > > +
> > > +C-SKY APB Interrupt Controller is a simple soc interrupt controller
> > > +on the apb bus and we only use it as root irq controller.
> > > +
> > > + - csky,apb-intc is used in a lot of csky fpgas and socs, it support 64 irq nums.
> > > + - csky,dual-apb-intc consists of 2 apb-intc and 128 irq nums supported.
> > > + - csky,gx6605s-intc is gx6605s soc internal irq interrupt controller, 64 irq nums.
> >
> > Is there a relationship between csky,gx6605s-intc and csky,apb-intc?
> They all use pending register to get irq num and use enable register to
> mask/unmask.
>
> > > +        - support-pulse-signal:
> > > +                Usage: select
> > > +           Description: to support pulse signal flag
> >
> > What is this for?
> This is a level-triger interrupt controller at first, but we want it
> to support pulse signal. It means that when the pulse signal coming,
> the pending register will hold signals without clear the IFR reg.
>
> Some C-SKY cpu's socs need this feature to support pulse interrupt
> signal.

So an edge triggered interrupt. We have a way to support that on a per
interrupt basis with a 2nd cell (which you should consider if you may
need anyways). But this is a global setting for all interrupts the
interrupt controller serves? If so, it's fine, but does need a vendor
prefix.

Rob

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Guo Ren]

On Mon, Sep 17, 2018 at 07:23:36AM -0700, Rob Herring wrote:
> On Mon, Sep 17, 2018 at 1:36 AM Guo Ren <ren_guo@c-sky.com> wrote:
> >
> > On Mon, Sep 17, 2018 at 02:23:36AM -0400, Rob Herring wrote:
> > > On Sun, Sep 16, 2018 at 04:50:03PM +0800, Guo Ren wrote:
> > > > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> > >
> > > Needs a commit description.
> > >
> > > > ---
> > > >  .../interrupt-controller/csky,apb-intc.txt         | 70 ++++++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > > new file mode 100644
> > > > index 0000000..be7c3d1
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > > @@ -0,0 +1,70 @@
> > > > +==============================
> > > > +C-SKY APB Interrupt Controller
> > > > +==============================
> > > > +
> > > > +C-SKY APB Interrupt Controller is a simple soc interrupt controller
> > > > +on the apb bus and we only use it as root irq controller.
> > > > +
> > > > + - csky,apb-intc is used in a lot of csky fpgas and socs, it support 64 irq nums.
> > > > + - csky,dual-apb-intc consists of 2 apb-intc and 128 irq nums supported.
> > > > + - csky,gx6605s-intc is gx6605s soc internal irq interrupt controller, 64 irq nums.
> > >
> > > Is there a relationship between csky,gx6605s-intc and csky,apb-intc?
> > They all use pending register to get irq num and use enable register to
> > mask/unmask.
> >
> > > > +        - support-pulse-signal:
> > > > +                Usage: select
> > > > +           Description: to support pulse signal flag
> > >
> > > What is this for?
> > This is a level-triger interrupt controller at first, but we want it
> > to support pulse signal. It means that when the pulse signal coming,
> > the pending register will hold signals without clear the IFR reg.
> >
> > Some C-SKY cpu's socs need this feature to support pulse interrupt
> > signal.
> 
> So an edge triggered interrupt. We have a way to support that on a per
> interrupt basis with a 2nd cell (which you should consider if you may
> need anyways). But this is a global setting for all interrupts the
> interrupt controller serves? If so, it's fine, 
Yes, it's a global setting for all interrupts. not really edge triggered
interrupt for every interrupt.

> but does need a vendor prefix. 
vendor prefix? Em ... it's just used in fpga now.

Best Regards
 Guo Ren

Re: [PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Guo Ren]

On Mon, Sep 17, 2018 at 02:27:31PM +0100, Marc Zyngier wrote:
> On Mon, 17 Sep 2018 03:09:29 +0100,
> Guo Ren <ren_guo@c-sky.com> wrote:
> 
> [...]
> 
> > > > +
> > > > +	irq_set_default_host(root_domain);
> > > 
> > > Please drop this. There is no reason to use this on any modern, DT
> > > based architecture.
Ok.

> > Please let me keep this and in my arch/csky/kernel/smp.c:
> > 
> > void __init setup_smp_ipi(void)
> > {
> > 	...
> > 	irq_create_mapping(NULL, IPI_IRQ);
> >	rc = request_percpu_irq(IPI_IRQ, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
> 
> This looks quite wrong. Reading the code at
> https://lkml.org/lkml/2018/9/12/674, it really looks like you're
> assuming that IPI_IRQ will be mapped to a Linux IRQ with the same
> number. Nothing could be farther from the truth.
Yes, you are right. I should use irq_create_mapping() return value as
the arg for request_percpu_irq. It's a stupid bug, thoug it happens to
work.

> The Linux IRQ is returned as the result of irq_create_mapping, which
> you're ignoring. You'd be better off creating this mapping from the
> irqchip code, and expose the resulting Linux IRQ to oyu SMP code by
> any mean of your choice (such as moving the send_ipi_message into the
> irqchip code as well).
Ok, see my diff below, is that OK?

--- a/drivers/irqchip/irq-csky-mpintc.c
+++ b/drivers/irqchip/irq-csky-mpintc.c
@@ -16,6 +16,7 @@
 #include <asm/reg_ops.h>
 #include <asm/smp.h>
 
+static struct irq_domain *root_domain;
 static void __iomem *INTCG_base;
 static void __iomem *INTCL_base;
 
@@ -46,7 +47,7 @@ static void csky_mpintc_handler(struct pt_regs *regs)
 	void __iomem *reg_base = this_cpu_read(intcl_reg);
 
 	do {
-		handle_domain_irq(NULL,
+		handle_domain_irq(root_domain,
 				  readl_relaxed(reg_base + INTCL_RDYIR),
 				  regs);
 	} while (readl_relaxed(reg_base + INTCL_HPPIR) & BIT(31));
@@ -139,13 +140,17 @@ static void csky_mpintc_send_ipi(const unsigned long *mask, unsigned long irq)
 	 */
 	writel_relaxed((*mask) << 8 | irq, reg_base + INTCL_SIGR);
 }
+
+static int csky_mpintc_ipi_irq_mapping(void)
+{
+	return irq_create_mapping(root_domain, IPI_IRQ);
+}
 #endif
 
 /* C-SKY multi processor interrupt controller */
 static int __init
 csky_mpintc_init(struct device_node *node, struct device_node *parent)
 {
-	struct irq_domain *root_domain;
 	unsigned int cpu, nr_irq;
 	int ret;
 
@@ -172,8 +177,6 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
 	if (!root_domain)
 		return -ENXIO;
 
-	irq_set_default_host(root_domain);
-
 	/* for every cpu */
 	for_each_present_cpu(cpu) {
 		per_cpu(intcl_reg, cpu) = INTCL_base + (INTCL_SIZE * cpu);
@@ -184,6 +187,8 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
 
 #ifdef CONFIG_SMP
 	set_send_ipi(&csky_mpintc_send_ipi);
+
+	set_ipi_irq_mapping(&csky_mpintc_ipi_irq_mapping);
 #endif
 
 	return 0;

diff --git a/arch/csky/include/asm/smp.h b/arch/csky/include/asm/smp.h
index 9a53abf..fed3a5a 100644
--- a/arch/csky/include/asm/smp.h
+++ b/arch/csky/include/asm/smp.h
@@ -7,6 +7,8 @@
 
 #ifdef CONFIG_SMP
 
+#define IPI_IRQ	15
+
 void __init setup_smp(void);
 
 void __init setup_smp_ipi(void);
@@ -19,6 +21,8 @@ void arch_send_call_function_single_ipi(int cpu);
 
 void __init set_send_ipi(void (*func)(const unsigned long *, unsigned long));
 
+void __init set_ipi_irq_mapping(int (*func)(void));
+
 #define raw_smp_processor_id()	(current_thread_info()->cpu)
 
 #endif /* CONFIG_SMP */
diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
index 522c73f..f8343f6 100644
--- a/arch/csky/kernel/smp.c
+++ b/arch/csky/kernel/smp.c
@@ -20,8 +20,6 @@
 #include <asm/mmu_context.h>
 #include <asm/pgalloc.h>
 
-#define IPI_IRQ	15
-
 static struct {
 	unsigned long bits ____cacheline_aligned;
 } ipi_data[NR_CPUS] __cacheline_aligned;
@@ -121,13 +119,23 @@ void __init enable_smp_ipi(void)
 	enable_percpu_irq(IPI_IRQ, 0);
 }
 
+static int (*arch_ipi_irq_mapping)(void) = NULL;
+
+void __init set_ipi_irq_mapping(int (*func)(void))
+{
+	if (arch_ipi_irq_mapping)
+		return;
+
+	arch_ipi_irq_mapping = func;
+}
+
 void __init setup_smp_ipi(void)
 {
-	int rc;
+	int rc, irq;
 
-	irq_create_mapping(NULL, IPI_IRQ);
+	irq = arch_ipi_irq_mapping();
 
-	rc = request_percpu_irq(IPI_IRQ, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
+	rc = request_percpu_irq(irq, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
 	if (rc)
 		panic("%s IRQ request failed\n", __func__);

Re: [PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Marc Zyngier]

On Tue, 18 Sep 2018 09:43:31 +0100,
Guo Ren <ren_guo@c-sky.com> wrote:
> 
> On Mon, Sep 17, 2018 at 02:27:31PM +0100, Marc Zyngier wrote:
> > On Mon, 17 Sep 2018 03:09:29 +0100,
> > Guo Ren <ren_guo@c-sky.com> wrote:
> > 
> > [...]
> > 
> > > > > +
> > > > > +	irq_set_default_host(root_domain);
> > > > 
> > > > Please drop this. There is no reason to use this on any modern, DT
> > > > based architecture.
> Ok.
> 
> > > Please let me keep this and in my arch/csky/kernel/smp.c:
> > > 
> > > void __init setup_smp_ipi(void)
> > > {
> > > 	...
> > > 	irq_create_mapping(NULL, IPI_IRQ);
> > >	rc = request_percpu_irq(IPI_IRQ, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
> > 
> > This looks quite wrong. Reading the code at
> > https://lkml.org/lkml/2018/9/12/674, it really looks like you're
> > assuming that IPI_IRQ will be mapped to a Linux IRQ with the same
> > number. Nothing could be farther from the truth.
> Yes, you are right. I should use irq_create_mapping() return value as
> the arg for request_percpu_irq. It's a stupid bug, thoug it happens to
> work.
> 
> > The Linux IRQ is returned as the result of irq_create_mapping, which
> > you're ignoring. You'd be better off creating this mapping from the
> > irqchip code, and expose the resulting Linux IRQ to oyu SMP code by
> > any mean of your choice (such as moving the send_ipi_message into the
> > irqchip code as well).
> Ok, see my diff below, is that OK?
> 
> --- a/drivers/irqchip/irq-csky-mpintc.c
> +++ b/drivers/irqchip/irq-csky-mpintc.c
> @@ -16,6 +16,7 @@
>  #include <asm/reg_ops.h>
>  #include <asm/smp.h>
>  
> +static struct irq_domain *root_domain;
>  static void __iomem *INTCG_base;
>  static void __iomem *INTCL_base;
>  
> @@ -46,7 +47,7 @@ static void csky_mpintc_handler(struct pt_regs *regs)
>  	void __iomem *reg_base = this_cpu_read(intcl_reg);
>  
>  	do {
> -		handle_domain_irq(NULL,
> +		handle_domain_irq(root_domain,
>  				  readl_relaxed(reg_base + INTCL_RDYIR),
>  				  regs);
>  	} while (readl_relaxed(reg_base + INTCL_HPPIR) & BIT(31));
> @@ -139,13 +140,17 @@ static void csky_mpintc_send_ipi(const unsigned long *mask, unsigned long irq)
>  	 */
>  	writel_relaxed((*mask) << 8 | irq, reg_base + INTCL_SIGR);
>  }
> +
> +static int csky_mpintc_ipi_irq_mapping(void)
> +{
> +	return irq_create_mapping(root_domain, IPI_IRQ);
> +}
>  #endif
>  
>  /* C-SKY multi processor interrupt controller */
>  static int __init
>  csky_mpintc_init(struct device_node *node, struct device_node *parent)
>  {
> -	struct irq_domain *root_domain;
>  	unsigned int cpu, nr_irq;
>  	int ret;
>  
> @@ -172,8 +177,6 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
>  	if (!root_domain)
>  		return -ENXIO;
>  
> -	irq_set_default_host(root_domain);
> -
>  	/* for every cpu */
>  	for_each_present_cpu(cpu) {
>  		per_cpu(intcl_reg, cpu) = INTCL_base + (INTCL_SIZE * cpu);
> @@ -184,6 +187,8 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
>  
>  #ifdef CONFIG_SMP
>  	set_send_ipi(&csky_mpintc_send_ipi);
> +
> +	set_ipi_irq_mapping(&csky_mpintc_ipi_irq_mapping);
>  #endif
>  
>  	return 0;
> 
> diff --git a/arch/csky/include/asm/smp.h b/arch/csky/include/asm/smp.h
> index 9a53abf..fed3a5a 100644
> --- a/arch/csky/include/asm/smp.h
> +++ b/arch/csky/include/asm/smp.h
> @@ -7,6 +7,8 @@
>  
>  #ifdef CONFIG_SMP
>  
> +#define IPI_IRQ	15
> +

It feels really bizarre that the function that maps the interrupt is
specific to the interrupt controller, and yet the interrupt number is
defined at the architecture level. I'd expect this to be just as
interrupt controller specific.

>  void __init setup_smp(void);
>  
>  void __init setup_smp_ipi(void);
> @@ -19,6 +21,8 @@ void arch_send_call_function_single_ipi(int cpu);
>  
>  void __init set_send_ipi(void (*func)(const unsigned long *, unsigned long));
>  
> +void __init set_ipi_irq_mapping(int (*func)(void));
> +
>  #define raw_smp_processor_id()	(current_thread_info()->cpu)
>  
>  #endif /* CONFIG_SMP */
> diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
> index 522c73f..f8343f6 100644
> --- a/arch/csky/kernel/smp.c
> +++ b/arch/csky/kernel/smp.c
> @@ -20,8 +20,6 @@
>  #include <asm/mmu_context.h>
>  #include <asm/pgalloc.h>
>  
> -#define IPI_IRQ	15
> -
>  static struct {
>  	unsigned long bits ____cacheline_aligned;
>  } ipi_data[NR_CPUS] __cacheline_aligned;
> @@ -121,13 +119,23 @@ void __init enable_smp_ipi(void)
>  	enable_percpu_irq(IPI_IRQ, 0);
>  }
>  
> +static int (*arch_ipi_irq_mapping)(void) = NULL;
> +
> +void __init set_ipi_irq_mapping(int (*func)(void))
> +{
> +	if (arch_ipi_irq_mapping)
> +		return;
> +
> +	arch_ipi_irq_mapping = func;
> +}
> +
>  void __init setup_smp_ipi(void)
>  {
> -	int rc;
> +	int rc, irq;
>  
> -	irq_create_mapping(NULL, IPI_IRQ);
> +	irq = arch_ipi_irq_mapping();

How about checking the validity of the interrupt and that
arch_ipi_irq_mapping is actually non-NULL?

>  
> -	rc = request_percpu_irq(IPI_IRQ, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
> +	rc = request_percpu_irq(irq, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
>  	if (rc)
>  		panic("%s IRQ request failed\n", __func__);

To be honest, I'd tend to question the need for this level of
abstraction, unless you actually plan for multiple SMP-capable
interrupt controllers... But at the end of the day, that's your call,
and the above code looks mostly correct.

Thanks,

	M.

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

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Rob Herring]

On Mon, Sep 17, 2018 at 8:41 AM Guo Ren <ren_guo@c-sky.com> wrote:
>
> On Mon, Sep 17, 2018 at 07:23:36AM -0700, Rob Herring wrote:
> > On Mon, Sep 17, 2018 at 1:36 AM Guo Ren <ren_guo@c-sky.com> wrote:
> > >
> > > On Mon, Sep 17, 2018 at 02:23:36AM -0400, Rob Herring wrote:
> > > > On Sun, Sep 16, 2018 at 04:50:03PM +0800, Guo Ren wrote:
> > > > > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> > > >
> > > > Needs a commit description.
> > > >
> > > > > ---
> > > > >  .../interrupt-controller/csky,apb-intc.txt         | 70 ++++++++++++++++++++++
> > > > >  1 file changed, 70 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > > > new file mode 100644
> > > > > index 0000000..be7c3d1
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,apb-intc.txt
> > > > > @@ -0,0 +1,70 @@
> > > > > +==============================
> > > > > +C-SKY APB Interrupt Controller
> > > > > +==============================
> > > > > +
> > > > > +C-SKY APB Interrupt Controller is a simple soc interrupt controller
> > > > > +on the apb bus and we only use it as root irq controller.
> > > > > +
> > > > > + - csky,apb-intc is used in a lot of csky fpgas and socs, it support 64 irq nums.
> > > > > + - csky,dual-apb-intc consists of 2 apb-intc and 128 irq nums supported.
> > > > > + - csky,gx6605s-intc is gx6605s soc internal irq interrupt controller, 64 irq nums.
> > > >
> > > > Is there a relationship between csky,gx6605s-intc and csky,apb-intc?
> > > They all use pending register to get irq num and use enable register to
> > > mask/unmask.
> > >
> > > > > +        - support-pulse-signal:
> > > > > +                Usage: select
> > > > > +           Description: to support pulse signal flag
> > > >
> > > > What is this for?
> > > This is a level-triger interrupt controller at first, but we want it
> > > to support pulse signal. It means that when the pulse signal coming,
> > > the pending register will hold signals without clear the IFR reg.
> > >
> > > Some C-SKY cpu's socs need this feature to support pulse interrupt
> > > signal.
> >
> > So an edge triggered interrupt. We have a way to support that on a per
> > interrupt basis with a 2nd cell (which you should consider if you may
> > need anyways). But this is a global setting for all interrupts the
> > interrupt controller serves? If so, it's fine,
> Yes, it's a global setting for all interrupts. not really edge triggered
> interrupt for every interrupt.
>
> > but does need a vendor prefix.
> vendor prefix? Em ... it's just used in fpga now.

What I mean is make it: csky,support-pulse-signal

>
> Best Regards
>  Guo Ren

Re: [PATCH V5 2/3] dt-bindings: interrupt-controller: C-SKY APB intc

[Guo Ren]

On Tue, Sep 18, 2018 at 05:56:50PM -0700, Rob Herring wrote:
> > > but does need a vendor prefix.
> > vendor prefix? Em ... it's just used in fpga now.
> 
> What I mean is make it: csky,support-pulse-signal
Ok. no problem.

Best Regards
 Guo Ren

Re: [PATCH V5 1/3] irqchip: add C-SKY irqchip drivers

[Guo Ren]

Hi Marc,

On Tue, Sep 18, 2018 at 04:41:22PM +0100, Marc Zyngier wrote:
> > +#define IPI_IRQ	15
> > +
> 
> It feels really bizarre that the function that maps the interrupt is
> specific to the interrupt controller, and yet the interrupt number is
> defined at the architecture level. I'd expect this to be just as
> interrupt controller specific.
>
Ok, move IPI_IRQ to irq-csky-mpintc.c. See my PATCH V8


> > +	irq = arch_ipi_irq_mapping();
> 
> How about checking the validity of the interrupt and that
> arch_ipi_irq_mapping is actually non-NULL?
Ok. 

> > -	rc = request_percpu_irq(IPI_IRQ, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
> > +	rc = request_percpu_irq(irq, handle_ipi, "IPI Interrupt", &ipi_dummy_dev);
> >  	if (rc)
> >  		panic("%s IRQ request failed\n", __func__);
>
> To be honest, I'd tend to question the need for this level of
> abstraction, unless you actually plan for multiple SMP-capable
> interrupt controllers... But at the end of the day, that's your call,
> and the above code looks mostly correct.
Thx for the review. I will consider your suggestion.

Best Regards
 GUo Ren