Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Jiaxun Yang <jiaxun.yang@flygoat.com>
To: Marc Zyngier <marc.zyngier@arm.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: Fri, 25 Jan 2019 18:56:33 +0800
Message-ID: <a0041cdb-1db6-9fee-411a-ae9d2a575349@flygoat.com> (raw)
In-Reply-To: <86pnsm8jon.wl-marc.zyngier@arm.com>

Hi Marc

Thanks for your suggestions, I'm working on v4 and I would like to ask 
if it is better to have a driver for only one irqchip

and create dt nodes for each chip, or just register all the chips in a 
single driver with only one dt node.

ÔÚ 2019/1/24 ÏÂÎç5:54, Marc Zyngier дµÀ:
> 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.

My fault, will fix it later.

>
>> +
>> +	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.

Yes, I do like this for backward compatible reason. I'm turning

a legacy platform device mach(arch/mips/loongson32) in to

dt based generic mach and I would like to do it step by step rather than 
one time.

So I use legacy domain in order to keep IRQ same with the

old driver exist on arch/mips/loongson32/common/irq.c

>> +	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.
>

  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
2019-01-25 10:56       ` Jiaxun Yang [this message]
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=a0041cdb-1db6-9fee-411a-ae9d2a575349@flygoat.com \
    --to=jiaxun.yang@flygoat.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jason@lakedaemon.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --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