Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Zyngier <marc.zyngier@arm.com>
To: Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc: linux-mips@vger.kernel.org, tglx@linutronix.de,
	jason@lakedaemon.net, robh+dt@kernel.org, mark.rutland@arm.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v3 1/2] irqchip: Add driver for Loongson-1 interrupt controller
Date: Thu, 24 Jan 2019 09:54:32 +0000
Message-ID: <86pnsm8jon.wl-marc.zyngier@arm.com> (raw)
In-Reply-To: <20190124032730.18237-2-jiaxun.yang@flygoat.com>

On Thu, 24 Jan 2019 03:27:29 +0000,
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> This controller appeared on Loongson-1 family MCUs
> including Loongson-1B and Loongson-1C.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  drivers/irqchip/Kconfig    |   9 ++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/irq-ls1x.c | 176 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
>  create mode 100644 drivers/irqchip/irq-ls1x.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3d1e60779078..5dcb5456cd14 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -406,6 +406,15 @@ config IMX_IRQSTEER
>  	help
>  	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
>  
> +config LS1X_IRQ
> +	bool "Loongson-1 Interrupt Controller"
> +	depends on MACH_LOONGSON32
> +	default y
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> +	help
> +	  Support for the Loongson-1 platform Interrupt Controller.
> +
>  endmenu
>  
>  config SIFIVE_PLIC
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c93713d24b86..7acd0e36d0b4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -94,3 +94,4 @@ obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
>  obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> +obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> diff --git a/drivers/irqchip/irq-ls1x.c b/drivers/irqchip/irq-ls1x.c
> new file mode 100644
> index 000000000000..de92ca04cf9f
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls1x.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright (C) 2019, Jiaxun Yang <jiaxun.yang@flygoat.com>
> + *  Loongson-1 platform IRQ support
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/irqchip.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/irqchip/chained_irq.h>
> +
> +#include <asm/mach-loongson32/irq.h>
> +
> +
> +#define MAX_CHIPS	5
> +#define LS_REG_INTC_STATUS	0x00
> +#define LS_REG_INTC_EN	0x04
> +#define LS_REG_INTC_SET	0x08
> +#define LS_REG_INTC_CLR	0x0c
> +#define LS_REG_INTC_POL	0x10
> +#define LS_REG_INTC_EDGE	0x14
> +#define CHIP_SIZE	0x18
> +
> +static void ls_chained_handle_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip_generic *gc = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	u32 pending;
> +
> +	chained_irq_enter(chip, desc);
> +	pending = readl(gc->reg_base + LS_REG_INTC_STATUS) &
> +			readl(gc->reg_base + LS_REG_INTC_EN);
> +
> +	if (!pending) {
> +		spurious_interrupt();
> +		chained_irq_exit(chip, desc);
> +		return;
> +	}

Given the context, this is the same as writing:

	if (!pending)
		spurious_interrupt();

and let it go through.

> +
> +	while (pending) {
> +		int bit = __ffs(pending);
> +
> +		generic_handle_irq(gc->irq_base + bit);
> +		pending &= ~BIT(bit);
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void ls_intc_set_bit(struct irq_chip_generic *gc,
> +							unsigned int offset,
> +							u32 mask, bool set)
> +{
> +	if (set)
> +		writel(readl(gc->reg_base + offset) | mask,
> +		gc->reg_base + offset);
> +	else
> +		writel(readl(gc->reg_base + offset) & ~mask,
> +		gc->reg_base + offset);
> +}
> +
> +static int ls_intc_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
> +	u32 mask = data->mask;
> +
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, false);
> +		ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, true);
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, false);
> +		ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, false);
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, true);
> +		ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, true);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, true);
> +		ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, false);
> +		break;
> +	case IRQ_TYPE_NONE:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

I just realised that in your DT binding, you define the interrupt
specifier as having a single cell. Which means that you never describe
the interrupt trigger there. Why?

Does it mean that all the drivers have to configure their interrupt
trigger themselves, and cannot rely on DT to do it? This seems quite
backward.

> +
> +	return 0;
> +}
> +
> +
> +static int __init ls_intc_of_init(struct device_node *node,
> +				       struct device_node *parent)
> +{
> +	struct irq_chip_generic *gc[MAX_CHIPS];
> +	struct irq_chip_type *ct;
> +	struct irq_domain *domain;
> +	void __iomem *base;
> +	int parent_irq[MAX_CHIPS], err = 0;
> +	unsigned int i = 0;
> +
> +	unsigned int num_chips = of_irq_count(node);
> +
> +	base = of_iomap(node, 0);
> +	if (!base)
> +		return -ENODEV;
> +
> +	for (i = 0; i < num_chips; i++) {
> +		/* Mask all irqs */
> +		writel(0x0, base + (i * CHIP_SIZE) +
> +		       LS_REG_INTC_EN);
> +
> +		/* Set all irqs to high level triggered */
> +		writel(0xffffffff, base + (i * CHIP_SIZE) +
> +		       LS_REG_INTC_POL);
> +
> +		parent_irq[i] = irq_of_parse_and_map(node, i);
> +
> +		if (!parent_irq[i]) {
> +			pr_warn("unable to get parent irq for irqchip %u\n", i);
> +			goto out_err;
> +		}
> +
> +		gc[i] = irq_alloc_generic_chip("INTC", 1,
> +					    LS1X_IRQ_BASE + (i * 32),
> +					    base + (i * CHIP_SIZE),
> +					    handle_level_irq);
> +
> +		ct = gc[i]->chip_types;
> +		ct->regs.mask = LS_REG_INTC_EN;
> +		ct->regs.ack = LS_REG_INTC_CLR;
> +		ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +		ct->chip.irq_mask = irq_gc_mask_clr_bit;
> +		ct->chip.irq_ack = irq_gc_ack_set_bit;
> +		ct->chip.irq_set_type = ls_intc_set_type;
> +
> +		irq_setup_generic_chip(gc[i], IRQ_MSK(32), 0, 0,
> +				       IRQ_NOPROBE | IRQ_LEVEL);
> +	}
> +
> +	domain = irq_domain_add_legacy(node, num_chips * 32, LS1X_IRQ_BASE, 0,
> +		&irq_domain_simple_ops, NULL);

Why a legacy domain? This is usually reserved to old drivers that are
converted to a new infrastructure, while needing some form of platform
hacks. I don't see this being the case here.

It is also worrying that although you have up to 5 irqchips, they all
share a single domain. What does this mean? each irqchip is expected
to have its own domain.

> +	if (!domain) {
> +		pr_warn("unable to register IRQ domain\n");
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	for (i = 0; i < num_chips; i++)
> +		irq_set_chained_handler_and_data(parent_irq[i],
> +		ls_chained_handle_irq, gc[i]);
> +
> +	pr_info("ls1x-irq: %u chips registered\n", num_chips);
> +
> +	return 0;
> +
> +out_err:
> +	for (i = 0; i < MAX_CHIPS; i++) {
> +		if (gc[i])

But you've never initialised gc[], nor parent_irq[]. So you'll get
whatever the stack had at this point. Not great.

> +			irq_destroy_generic_chip(gc[i], IRQ_MSK(32),
> +				       IRQ_NOPROBE | IRQ_LEVEL, 0);
> +		if (parent_irq[i])
> +			irq_dispose_mapping(parent_irq[i]);
> +	}
> +	return err;
> +}
> +
> +IRQCHIP_DECLARE(ls1x_intc, "loongson,ls1x-intc", ls_intc_of_init);
> -- 
> 2.20.1
> 

Thanks,

	M.

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

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-22 15:45 [PATCH " Jiaxun Yang
2019-01-22 15:45 ` [PATCH 2/2] dt-bindings: interrupt-controller: loongson ls1x intc Jiaxun Yang
2019-01-23  6:23 ` irqchip: Add driver for Loongson-1 intc v2 Jiaxun Yang
2019-01-23  6:23   ` [PATCH v2 2/2] dt-bindings: interrupt-controller: loongson ls1x intc Jiaxun Yang
2019-01-23  6:23   ` [PATCH v2 1/2] irqchip: Add driver for Loongson-1 interrupt controller Jiaxun Yang
2019-01-23  8:36     ` Marc Zyngier
2019-01-24  3:27 ` irqchip: Add driver for Loongson-1 intc v3 Jiaxun Yang
2019-01-24  3:27   ` [PATCH v3 1/2] irqchip: Add driver for Loongson-1 interrupt controller Jiaxun Yang
2019-01-24  9:54     ` Marc Zyngier [this message]
2019-01-25 10:56       ` Jiaxun Yang
2019-01-26 12:09         ` Marc Zyngier
2019-01-24  3:27   ` [PATCH v3 2/2] dt-bindings: interrupt-controller: loongson ls1x intc Jiaxun Yang
2019-01-30 16:41     ` Rob Herring
2019-01-28 14:46 ` irqchip: Add driver for Loongson-1 intc v3 Jiaxun Yang
2019-01-28 14:46   ` [PATCH v4 1/2] irqchip: Add driver for Loongson-1 interrupt controller Jiaxun Yang
2019-01-28 15:20     ` Marc Zyngier
2019-01-28 14:46   ` [PATCH v4 2/2] dt-bindings: interrupt-controller: loongson ls1x intc Jiaxun Yang
2019-01-29  3:04 ` irqchip: Add driver for Loongson-1 intc v5 Jiaxun Yang
2019-01-29  3:05   ` [PATCH v5 1/2] irqchip: Add driver for Loongson-1 interrupt controller Jiaxun Yang
2019-01-29  3:05   ` [PATCH v5 2/2] dt-bindings: interrupt-controller: loongson ls1x intc Jiaxun Yang
2019-02-01  6:22 ` irqchip: Add driver for Loongson-1 intc v6 Jiaxun Yang
2019-02-01  6:22   ` [PATCH v6 1/2] irqchip: Add driver for Loongson-1 interrupt controller Jiaxun Yang
2019-02-01  6:22   ` [PATCH v6 2/2] dt-bindings: interrupt-controller: loongson ls1x intc Jiaxun Yang
2019-02-14 10:36   ` irqchip: Add driver for Loongson-1 intc v6 Marc Zyngier

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=86pnsm8jon.wl-marc.zyngier@arm.com \
    --to=marc.zyngier@arm.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jason@lakedaemon.net \
    --cc=jiaxun.yang@flygoat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org linux-mips@archiver.kernel.org
	public-inbox-index linux-mips


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/ public-inbox