Re: [PATCH v7 01/12] irqchip: Add driver for Loongson I/O Local Interrupt Controller

[Marc Zyngier]

On 2020-03-25 11:25, Paul Cercueil wrote:
> Hi Jiaxun,
>
>
> Le mer. 25 mars 2020 Ã 10:28, Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx> a 
> Ãcrit :
> > This controller appeared on Loongson family of chips as the primary
> > package interrupt source.
> >
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > Co-developed-by: Huacai Chen <chenhc@xxxxxxxxxx>
> > Signed-off-by: Huacai Chen <chenhc@xxxxxxxxxx>
> > Reviewed-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> > v4-v5:
> > 	Resolve suggestions from maz:
> > 		- Remove DT validation
> > 		- Simplify unnucessary functions & variables
> > ---
> >  drivers/irqchip/Kconfig                |   9 +
> >  drivers/irqchip/Makefile               |   1 +
> >  drivers/irqchip/irq-loongson-liointc.c | 261 
> > +++++++++++++++++++++++++
> >  3 files changed, 271 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-loongson-liointc.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 6d397732138d..c609eaa319d2 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -513,4 +513,13 @@ config EXYNOS_IRQ_COMBINER
> >  	  Say yes here to add support for the IRQ combiner devices embedded
> >  	  in Samsung Exynos chips.
> >
> > +config LOONGSON_LIOINTC
> > +	bool "Loongson Local I/O Interrupt Controller"
> > +	depends on MACH_LOONGSON64
> > +	default y
> > +	select IRQ_DOMAIN
> > +	select GENERIC_IRQ_CHIP
> > +	help
> > +	  Support for the Loongson Local I/O Interrupt Controller.
> > +
> >  endmenu
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index eae0d78cbf22..5e7678efdfe6 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -105,3 +105,4 @@ obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
> >  obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
> >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
> >  obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
> > +obj-$(CONFIG_LOONGSON_LIOINTC)		+= irq-loongson-liointc.o
> > diff --git a/drivers/irqchip/irq-loongson-liointc.c 
> > b/drivers/irqchip/irq-loongson-liointc.c
> > new file mode 100644
> > index 000000000000..18de2c09ece4
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-loongson-liointc.c
> > @@ -0,0 +1,261 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  Copyright (C) 2020, Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > + *  Loongson Local IO Interrupt Controller 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/smp.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +
> > +#include <boot_param.h>
> > +
> > +#define LIOINTC_CHIP_IRQ	32
> > +#define LIOINTC_NUM_PARENT 4
> > +
> > +#define LIOINTC_INTC_CHIP_START	0x20
> > +
> > +#define LIOINTC_REG_INTC_STATUS	(LIOINTC_INTC_CHIP_START + 0x20)
> > +#define LIOINTC_REG_INTC_EN_STATUS	(LIOINTC_INTC_CHIP_START + 0x04)
> > +#define LIOINTC_REG_INTC_ENABLE	(LIOINTC_INTC_CHIP_START + 0x08)
> > +#define LIOINTC_REG_INTC_DISABLE	(LIOINTC_INTC_CHIP_START + 0x0c)
> > +#define LIOINTC_REG_INTC_POL	(LIOINTC_INTC_CHIP_START + 0x10)
> > +#define LIOINTC_REG_INTC_EDGE	(LIOINTC_INTC_CHIP_START + 0x14)
> > +
> > +#define LIOINTC_SHIFT_INTx	4
> > +
> > +struct liointc_handler_data {
> > +	struct liointc_priv	*priv;
> > +	u32			parent_int_map;
> > +};
> > +
> > +struct liointc_priv {
> > +	struct irq_chip_generic		*gc;
> > +	struct liointc_handler_data	handler[LIOINTC_NUM_PARENT];
> > +	u8				map_cache[LIOINTC_CHIP_IRQ];
> > +};
> > +
> > +static void liointc_chained_handle_irq(struct irq_desc *desc)
> > +{
> > +	struct liointc_handler_data *handler = 
> > irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	struct irq_chip_generic *gc = handler->priv->gc;
> > +	u32 pending;
> > +
> > +	chained_irq_enter(chip, desc);
> > +
> > +	pending = readl(gc->reg_base + LIOINTC_REG_INTC_STATUS);
> > +
> > +	if (!pending)
> > +		spurious_interrupt();
> > +
> > +	while (pending) {
> > +		int bit = __ffs(pending);
> > +
> > +		generic_handle_irq(irq_find_mapping(gc->domain, bit));
> > +		pending &= ~BIT(bit);
> > +	}
>
> Consider using the for_each_set_bit() macro from <linux/bitops.h>.
> See drivers/irqchip/irq-ingenic-tcu.c for instance.

which would require changing the pending type to be unsigned long.
Open-coding these if fine if it helps keeping the type system 
consistent.

        M.
--
Jazz is not dead. It just smells funny...