diff mbox series

[v2,2/2] irqchip: Add StarFive external interrupt controller

Message ID 20240130055843.216342-3-changhuang.liang@starfivetech.com
State New
Headers
Series Add JH8100 external interrupt controller support |

Commit Message

Changhuang Liang Jan. 30, 2024, 5:58 a.m. UTC 
  Add StarFive external interrupt controller for JH8100 SoC.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
---
 MAINTAINERS                                |   6 +
 drivers/irqchip/Kconfig                    |  11 ++
 drivers/irqchip/Makefile                   |   1 +
 drivers/irqchip/irq-starfive-jh8100-intc.c | 180 +++++++++++++++++++++
 4 files changed, 198 insertions(+)
 create mode 100644 drivers/irqchip/irq-starfive-jh8100-intc.c

Comments

Thomas Gleixner Feb. 13, 2024, 9:03 a.m. UTC | #1 
On Mon, Jan 29 2024 at 21:58, Changhuang Liang wrote:
> +
> +struct starfive_irq_chip {
> +	void __iomem *base;
> +	struct irq_domain *root_domain;
> +	struct clk *clk;
> +};

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Please.

> +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32 reg, u32 mask, u32 data)
> +{
> +	u32 value;
> +
> +	value = ioread32(irqc->base + reg) & ~mask;
> +	data &= mask;

Why?

> +	data |= value;
> +	iowrite32(data, irqc->base + reg);

How is this serialized against concurrent invocations of this code on
different CPUs for different interrupts?

It's not and this requires a raw_spinlock for protection.

> +}
> +
> +static void starfive_intc_unmask(struct irq_data *d)
> +{
> +	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +
> +	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), 0);
> +}
> +
> +static void starfive_intc_mask(struct irq_data *d)
> +{
> +	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +
> +	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), BIT(d->hwirq));
> +}
> +
> +static struct irq_chip intc_dev = {
> +	.name = "starfive jh8100 intc",
> +	.irq_unmask = starfive_intc_unmask,
> +	.irq_mask = starfive_intc_mask,
> +};

See documentation please.

> +static int starfive_intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_domain_set_info(d, irq, hwirq, &intc_dev, d->host_data,
> +			    handle_level_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops starfive_intc_domain_ops = {
> +	.xlate = irq_domain_xlate_onecell,
> +	.map = starfive_intc_map,
> +};

Ditto.

> +static void starfive_intc_irq_handler(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct starfive_irq_chip *irqc = irq_data_get_irq_handler_data(&desc->irq_data);

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations

> +	unsigned long value = 0;

Pointless initialization

> +	int hwirq;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	value = ioread32(irqc->base + STARFIVE_INTC_SRC0_INT);
> +	while (value) {
> +		hwirq = ffs(value) - 1;
> +
> +		generic_handle_domain_irq(irqc->root_domain, hwirq);
> +
> +		starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_CLEAR, BIT(hwirq), BIT(hwirq));
> +		starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_CLEAR, BIT(hwirq), 0);
> +
> +		clear_bit(hwirq, &value);
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int __init starfive_intc_init(struct device_node *intc,
> +				     struct device_node *parent)
> +{
> +	struct starfive_irq_chip *irqc;
> +	struct reset_control *rst;
> +	int ret;
> +	int parent_irq;

See Documentation

> +	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> +	if (!irqc)
> +		return -ENOMEM;
> +
> +	irqc->base = of_iomap(intc, 0);
> +	if (!irqc->base) {
> +		pr_err("Unable to map IC registers\n");
> +		ret = -ENXIO;
> +		goto err_free;
> +	}
> +
> +	rst = of_reset_control_get_exclusive(intc, NULL);
> +	if (IS_ERR(rst)) {
> +		pr_err("Unable to get reset control %pe\n", rst);
> +		ret = PTR_ERR(rst);
> +		goto err_unmap;
> +	}
> +
> +	irqc->clk = of_clk_get(intc, 0);
> +	if (IS_ERR(irqc->clk)) {
> +		pr_err("Unable to get clock\n");
> +		ret = PTR_ERR(irqc->clk);
> +		goto err_rst;
> +	}
> +
> +	ret = reset_control_deassert(rst);
> +	if (ret)
> +		goto err_clk;
> +
> +	ret = clk_prepare_enable(irqc->clk);
> +	if (ret)
> +		goto err_clk;
> +
> +	irqc->root_domain = irq_domain_add_linear(intc, STARFIVE_INTC_SRC_IRQ_NUM,
> +						  &starfive_intc_domain_ops, irqc);
> +	if (!irqc->root_domain) {
> +		pr_err("Unable to create IRQ domain\n");
> +		ret = -EINVAL;
> +		goto err_clk;
> +	}
> +
> +	parent_irq = of_irq_get(intc, 0);
> +	if (parent_irq < 0) {
> +		pr_err("Failed to get main IRQ: %d\n", parent_irq);
> +		ret = parent_irq;
> +		goto err_clk;

Leaks the interrupt domain, no?

Thanks,

        tglx
Changhuang Liang Feb. 18, 2024, 2:36 a.m. UTC | #2 
Hi, Thomas

Thanks for your comment.

> On Mon, Jan 29 2024 at 21:58, Changhuang Liang wrote:
[...]
> > +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32
> > +reg, u32 mask, u32 data) {
> > +	u32 value;
> > +
> > +	value = ioread32(irqc->base + reg) & ~mask;
> > +	data &= mask;
> 
> Why?
> 

If I want to update the reg  GENMASK(7, 4)  to value 5, the data I will pass in 5 << 4

> > +	data |= value;
> > +	iowrite32(data, irqc->base + reg);
> 
> How is this serialized against concurrent invocations of this code on different
> CPUs for different interrupts?
> 
> It's not and this requires a raw_spinlock for protection.
> 

Will use raw_spinlock.

> > +}
> > +
> > +static void starfive_intc_unmask(struct irq_data *d) {
> > +	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +
> > +	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), 0);
> > +}
> > +
> > +static void starfive_intc_mask(struct irq_data *d) {
> > +	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +
> > +	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq),
> > +BIT(d->hwirq)); }
> > +
[...]
> > +	irqc->root_domain = irq_domain_add_linear(intc,
> STARFIVE_INTC_SRC_IRQ_NUM,
> > +						  &starfive_intc_domain_ops, irqc);
> > +	if (!irqc->root_domain) {
> > +		pr_err("Unable to create IRQ domain\n");
> > +		ret = -EINVAL;
> > +		goto err_clk;
> > +	}
> > +
> > +	parent_irq = of_irq_get(intc, 0);
> > +	if (parent_irq < 0) {
> > +		pr_err("Failed to get main IRQ: %d\n", parent_irq);
> > +		ret = parent_irq;
> > +		goto err_clk;
> 
> Leaks the interrupt domain, no?
> 
> Thanks,
> 

Will use irq_domain_remove() free domain.

regards
Changhuang
Thomas Gleixner Feb. 20, 2024, 9:28 a.m. UTC | #3 
On Sun, Feb 18 2024 at 02:36, Changhuang Liang wrote:
>> On Mon, Jan 29 2024 at 21:58, Changhuang Liang wrote:
> [...]
>> > +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32
>> > +reg, u32 mask, u32 data) {
>> > +	u32 value;
>> > +
>> > +	value = ioread32(irqc->base + reg) & ~mask;
>> > +	data &= mask;
>> 
>> Why?
>> 
>
> If I want to update the reg  GENMASK(7, 4)  to value 5, the data I
> will pass in 5 << 4

All call sites pass a single bit to set/clear, right? So this GENMASK
argument does not make sense at all.

Thanks,

        tglx
Changhuang Liang Feb. 20, 2024, 9:39 a.m. UTC | #4 
Hi, Thomas

> -----邮件原件-----
> 发件人: Thomas Gleixner <tglx@linutronix.de>
> 发送时间: 2024年2月20日 17:28
> 收件人: Changhuang Liang <changhuang.liang@starfivetech.com>; Rob
> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Philipp Zabel <p.zabel@pengutronix.de>
> 抄送: Leyfoon Tan <leyfoon.tan@starfivetech.com>; Jack Zhu
> <jack.zhu@starfivetech.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> 主题: Re: 回复: [PATCH v2 2/2] irqchip: Add StarFive external interrupt
> controller
> 
> On Sun, Feb 18 2024 at 02:36, Changhuang Liang wrote:
> >> On Mon, Jan 29 2024 at 21:58, Changhuang Liang wrote:
> > [...]
> >> > +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32
> >> > +reg, u32 mask, u32 data) {
> >> > +	u32 value;
> >> > +
> >> > +	value = ioread32(irqc->base + reg) & ~mask;
> >> > +	data &= mask;
> >>
> >> Why?
> >>
> >
> > If I want to update the reg  GENMASK(7, 4)  to value 5, the data I
> > will pass in 5 << 4
> 
> All call sites pass a single bit to set/clear, right? So this GENMASK argument
> does not make sense at all.
> 

Yes, I will update this function
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..ef678f04c830 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20956,6 +20956,12 @@  F:	Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
 F:	drivers/phy/starfive/phy-jh7110-pcie.c
 F:	drivers/phy/starfive/phy-jh7110-usb.c
 
+STARFIVE JH8100 EXTERNAL INTERRUPT CONTROLLER DRIVER
+M:	Changhuang Liang <changhuang.liang@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/interrupt-controller/starfive,jh8100-intc.yaml
+F:	drivers/irqchip/irq-starfive-jh8100-intc.c
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <peterz@infradead.org>
 M:	Josh Poimboeuf <jpoimboe@kernel.org>
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f7149d0f3d45..72c07a12f5e1 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -546,6 +546,17 @@  config SIFIVE_PLIC
 	select IRQ_DOMAIN_HIERARCHY
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
+config STARFIVE_JH8100_INTC
+	bool "StarFive JH8100 External Interrupt Controller"
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	default ARCH_STARFIVE
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables support for the INTC chip found in StarFive JH8100
+	  SoC.
+
+	  If you don't know what to do here, say Y.
+
 config EXYNOS_IRQ_COMBINER
 	bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
 	depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index ffd945fe71aa..ec4a18380998 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -96,6 +96,7 @@  obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
+obj-$(CONFIG_STARFIVE_JH8100_INTC)	+= irq-starfive-jh8100-intc.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
 obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
diff --git a/drivers/irqchip/irq-starfive-jh8100-intc.c b/drivers/irqchip/irq-starfive-jh8100-intc.c
new file mode 100644
index 000000000000..344f7d871518
--- /dev/null
+++ b/drivers/irqchip/irq-starfive-jh8100-intc.c
@@ -0,0 +1,180 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StarFive JH8100 External Interrupt Controller driver
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ *
+ * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
+ */
+
+#define pr_fmt(fmt) "irq-starfive-jh8100: " fmt
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/reset.h>
+
+#define STARFIVE_INTC_SRC0_CLEAR	0x10
+#define STARFIVE_INTC_SRC0_MASK		0x14
+#define STARFIVE_INTC_SRC0_INT		0x1c
+
+#define STARFIVE_INTC_SRC_IRQ_NUM	32
+
+struct starfive_irq_chip {
+	void __iomem *base;
+	struct irq_domain *root_domain;
+	struct clk *clk;
+};
+
+static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32 reg, u32 mask, u32 data)
+{
+	u32 value;
+
+	value = ioread32(irqc->base + reg) & ~mask;
+	data &= mask;
+	data |= value;
+	iowrite32(data, irqc->base + reg);
+}
+
+static void starfive_intc_unmask(struct irq_data *d)
+{
+	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
+
+	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), 0);
+}
+
+static void starfive_intc_mask(struct irq_data *d)
+{
+	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
+
+	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), BIT(d->hwirq));
+}
+
+static struct irq_chip intc_dev = {
+	.name = "starfive jh8100 intc",
+	.irq_unmask = starfive_intc_unmask,
+	.irq_mask = starfive_intc_mask,
+};
+
+static int starfive_intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_domain_set_info(d, irq, hwirq, &intc_dev, d->host_data,
+			    handle_level_irq, NULL, NULL);
+
+	return 0;
+}
+
+static const struct irq_domain_ops starfive_intc_domain_ops = {
+	.xlate = irq_domain_xlate_onecell,
+	.map = starfive_intc_map,
+};
+
+static void starfive_intc_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct starfive_irq_chip *irqc = irq_data_get_irq_handler_data(&desc->irq_data);
+	unsigned long value = 0;
+	int hwirq;
+
+	chained_irq_enter(chip, desc);
+
+	value = ioread32(irqc->base + STARFIVE_INTC_SRC0_INT);
+	while (value) {
+		hwirq = ffs(value) - 1;
+
+		generic_handle_domain_irq(irqc->root_domain, hwirq);
+
+		starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_CLEAR, BIT(hwirq), BIT(hwirq));
+		starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_CLEAR, BIT(hwirq), 0);
+
+		clear_bit(hwirq, &value);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int __init starfive_intc_init(struct device_node *intc,
+				     struct device_node *parent)
+{
+	struct starfive_irq_chip *irqc;
+	struct reset_control *rst;
+	int ret;
+	int parent_irq;
+
+	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
+	if (!irqc)
+		return -ENOMEM;
+
+	irqc->base = of_iomap(intc, 0);
+	if (!irqc->base) {
+		pr_err("Unable to map IC registers\n");
+		ret = -ENXIO;
+		goto err_free;
+	}
+
+	rst = of_reset_control_get_exclusive(intc, NULL);
+	if (IS_ERR(rst)) {
+		pr_err("Unable to get reset control %pe\n", rst);
+		ret = PTR_ERR(rst);
+		goto err_unmap;
+	}
+
+	irqc->clk = of_clk_get(intc, 0);
+	if (IS_ERR(irqc->clk)) {
+		pr_err("Unable to get clock\n");
+		ret = PTR_ERR(irqc->clk);
+		goto err_rst;
+	}
+
+	ret = reset_control_deassert(rst);
+	if (ret)
+		goto err_clk;
+
+	ret = clk_prepare_enable(irqc->clk);
+	if (ret)
+		goto err_clk;
+
+	irqc->root_domain = irq_domain_add_linear(intc, STARFIVE_INTC_SRC_IRQ_NUM,
+						  &starfive_intc_domain_ops, irqc);
+	if (!irqc->root_domain) {
+		pr_err("Unable to create IRQ domain\n");
+		ret = -EINVAL;
+		goto err_clk;
+	}
+
+	parent_irq = of_irq_get(intc, 0);
+	if (parent_irq < 0) {
+		pr_err("Failed to get main IRQ: %d\n", parent_irq);
+		ret = parent_irq;
+		goto err_clk;
+	}
+
+	irq_set_chained_handler_and_data(parent_irq, starfive_intc_irq_handler, irqc);
+
+	pr_info("Interrupt controller register, nr_irqs %d\n", STARFIVE_INTC_SRC_IRQ_NUM);
+
+	return 0;
+
+err_clk:
+	clk_put(irqc->clk);
+err_rst:
+	reset_control_put(rst);
+err_unmap:
+	iounmap(irqc->base);
+err_free:
+	kfree(irqc);
+	return ret;
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(starfive_intc)
+IRQCHIP_MATCH("starfive,jh8100-intc", starfive_intc_init)
+IRQCHIP_PLATFORM_DRIVER_END(starfive_intc)
+
+MODULE_DESCRIPTION("StarFive JH8100 External Interrupt Controller");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");