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, Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] irqchip: Add driver for Loongson-1 interrupt controller
Date: Wed, 23 Jan 2019 08:36:31 +0000
Message-ID: <86y37b93e8.wl-marc.zyngier@arm.com> (raw)
In-Reply-To: <20190123062341.8957-3-jiaxun.yang@flygoat.com>

Hi Jiaxun,

On Wed, 23 Jan 2019 06:23:36 +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 | 194 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 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..b15b01237830
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls1x.c
> @@ -0,0 +1,194 @@
> +// 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 <asm/mach-loongson32/irq.h>
> +
> +struct ls_intc_data {
> +	void __iomem *base;
> +	unsigned int chip;
> +};
> +
> +#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 irqreturn_t intc_cascade(int irq, void *data)
> +{
> +	struct ls_intc_data *intc = irq_get_handler_data(irq);
> +	uint32_t irq_reg;
> +
> +	irq_reg = readl(intc->base + (CHIP_SIZE * intc->chip)
> +					+ LS_REG_INTC_STATUS);
> +	generic_handle_irq(__fls(irq_reg) + (intc->chip * 32) + LS1X_IRQ_BASE);
> +
> +	return IRQ_HANDLED;
> +}

No. A chained interrupt is not a dealt with as a normal interrupt, as
it is a flow handler itself.

> +
> +static void ls_intc_set_bit(
> +	struct irq_chip_generic *gc, unsigned int offset,

Please put the first parameter on the same line as the function name.

> +	u32 mask, bool set)
> +{
> +	if (set)
> +		writel(readl(gc->reg_base + offset) |
> +		mask, gc->reg_base + offset);

Put "mask" on the same line as the rest of the expression.

> +	else
> +		writel(readl(gc->reg_base + offset) &
> +		~mask, gc->reg_base + offset);

Same here.

> +}
> +
> +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;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static struct irqaction intc_cascade_action = {
> +	.handler = intc_cascade,
> +	.name = "intc cascade interrupt",
> +};
> +
> +static int __init ls_intc_of_init(struct device_node *node,
> +				       unsigned int num_chips)
> +{
> +	struct ls_intc_data *intc[MAX_CHIPS];
> +	struct irq_chip_generic *gc;
> +	struct irq_chip_type *ct;
> +	struct irq_domain *domain;
> +	void __iomem *base;
> +	int parent_irq[MAX_CHIPS], err = 0;

Initialise both arrays so that you can simplify error handling.

> +	unsigned int i = 0;
> +
> +	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);
> +
> +		intc[i] = kzalloc(sizeof(*intc[i]), GFP_KERNEL);
> +		if (!intc[i]) {
> +			err = -ENOMEM;
> +			goto out_err;
> +		}
> +
> +		intc[i]->base = base;
> +		intc[i]->chip = i;
> +
> +		parent_irq[i] = irq_of_parse_and_map(node, i);
> +		if (!parent_irq[i]) {
> +			pr_warn("unable to get parent irq for irqchip %u", i);
> +			kfree(intc[i]);
> +			intc[i] = NULL;
> +			err = -EINVAL;

You're making life very complicated for yourself. Just set err and
branch to the error handler.

> +			goto out_err;
> +		}
> +
> +		err = irq_set_handler_data(parent_irq[i], intc[i]);
> +		if (err)
> +			goto out_err;
> +
> +		gc = irq_alloc_generic_chip("INTC", 1,
> +					    LS1X_IRQ_BASE + (i * 32),
> +					    base + (i * CHIP_SIZE),
> +					    handle_level_irq);
> +
> +		ct = gc->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, 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);
> +	if (!domain) {
> +		pr_warn("unable to register IRQ domain\n");
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	for (i = 0; i < num_chips; i++)
> +		setup_irq(parent_irq[i], &intc_cascade_action);

This is not how we do things anymore (some of this code feels like it
has escaped from a 2.2 kernel). See irq_set_chained_handler, and use
that to properly configure your cascades. There are tons of example in
the tree.

> +
> +	return 0;
> +
> +out_err:
> +	for (i = 0; i < MAX_CHIPS; i++) {
> +		if (intc[i]) {
> +			kfree(intc[i]);
> +			irq_dispose_mapping(parent_irq[i]);
> +		} else {
> +			break;
> +		}
> +	}

	for (i = 0; i < MAX_CHIPS; i++) {
		kfree(intc[i]);
		if (parent_irq[i])
			irq_dispose_mapping(parent_irq[i]);		
	}

> +	return err;
> +}
> +
> +static int __init intc_4chip_of_init(struct device_node *node,
> +				     struct device_node *parent)
> +{
> +	return ls_intc_of_init(node, 4);
> +}
> +IRQCHIP_DECLARE(ls1b_intc, "loongson,ls1b-intc", intc_4chip_of_init);
> +
> +static int __init intc_5chip_of_init(struct device_node *node,
> +	struct device_node *parent)
> +{
> +	return ls_intc_of_init(node, 5);
> +}
> +IRQCHIP_DECLARE(ls1c_intc, "loongson,ls1c-intc", intc_5chip_of_init);

Do you really need this distinction? You could easily find out how
many interrupts you have based on the DT, right? OR do you need this
for anything else?

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 [this message]
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
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=86y37b93e8.wl-marc.zyngier@arm.com \
    --to=marc.zyngier@arm.com \
    --cc=jason@lakedaemon.net \
    --cc=jiaxun.yang@flygoat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.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