Hi Jiaxun, On Wed, 23 Jan 2019 06:23:36 +0000, Jiaxun Yang <jiaxun.y...@flygoat.com> wrote: > > This controller appeared on Loongson-1 family MCUs > including Loongson-1B and Loongson-1C. > > Signed-off-by: Jiaxun Yang <jiaxun.y...@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.y...@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.