Re: [PATCH v4 03/10] irqchip: Add driver for Loongson-3 HyperTransport PIC controller

[Marc Zyngier <maz@kernel.org>]

On Fri, 21 Feb 2020 05:09:18 +0000,
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> This controller appeared on Loongson-3 family of chips to receive interrupts
> from PCH PIC.

Please explain for us mere mortals what the relationship is between
this interrupt controller and the i8259.

>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  arch/mips/include/asm/i8259.h        |   1 +
>  drivers/irqchip/Kconfig              |  10 ++
>  drivers/irqchip/Makefile             |   1 +
>  drivers/irqchip/irq-i8259.c          |   6 +-
>  drivers/irqchip/irq-loongson-htpic.c | 146 +++++++++++++++++++++++++++
>  5 files changed, 161 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/irqchip/irq-loongson-htpic.c
> 
> diff --git a/arch/mips/include/asm/i8259.h b/arch/mips/include/asm/i8259.h
> index 97a5e41ed1ab..1ec3dbb1588f 100644
> --- a/arch/mips/include/asm/i8259.h
> +++ b/arch/mips/include/asm/i8259.h
> @@ -36,6 +36,7 @@ extern raw_spinlock_t i8259A_lock;
>  extern void make_8259A_irq(unsigned int irq);
>  
>  extern void init_i8259_irqs(void);
> +extern struct irq_domain *of_init_i8259_irqs(struct device_node *node);
>  
>  /**
>   * i8159_set_poll() - Override the i8259 polling function
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index c609eaa319d2..cae6f480c987 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -522,4 +522,14 @@ config LOONGSON_LIOINTC
>  	help
>  	  Support for the Loongson Local I/O Interrupt Controller.
>  
> +config LOONGSON_HTPIC
> +	bool "Loongson3 HyperTransport PIC Controller"
> +	depends on MACH_LOONGSON64
> +	default y
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> +	select I8259
> +	help
> +	  Support for the Loongson-3 HyperTransport PIC Controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 5e7678efdfe6..37bbe39bf909 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -106,3 +106,4 @@ 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
> +obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
> diff --git a/drivers/irqchip/irq-i8259.c b/drivers/irqchip/irq-i8259.c
> index d000870d9b6b..9d79acce6c0c 100644
> --- a/drivers/irqchip/irq-i8259.c
> +++ b/drivers/irqchip/irq-i8259.c
> @@ -309,7 +309,7 @@ static const struct irq_domain_ops i8259A_ops = {
>   * driver compatibility reasons interrupts 0 - 15 to be the i8259
>   * interrupts even if the hardware uses a different interrupt numbering.
>   */
> -struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
> +struct irq_domain * __init of_init_i8259_irqs(struct device_node *node)

I don't think this renaming brings much to the table.

>  {
>  	struct irq_domain *domain;
>  
> @@ -330,7 +330,7 @@ struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
>  
>  void __init init_i8259_irqs(void)
>  {
> -	__init_i8259_irqs(NULL);
> +	of_init_i8259_irqs(NULL);
>  }
>  
>  static void i8259_irq_dispatch(struct irq_desc *desc)
> @@ -351,7 +351,7 @@ int __init i8259_of_init(struct device_node *node, struct device_node *parent)
>  	struct irq_domain *domain;
>  	unsigned int parent_irq;
>  
> -	domain = __init_i8259_irqs(node);
> +	domain = of_init_i8259_irqs(node);
>  
>  	parent_irq = irq_of_parse_and_map(node, 0);
>  	if (!parent_irq) {
> diff --git a/drivers/irqchip/irq-loongson-htpic.c b/drivers/irqchip/irq-loongson-htpic.c
> new file mode 100644
> index 000000000000..a90cf4357285
> --- /dev/null
> +++ b/drivers/irqchip/irq-loongson-htpic.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright (C) 2020, Jiaxun Yang <jiaxun.yang@flygoat.com>
> + *  Loongson HTPIC IRQ support
> + */
> +
> +#include <linux/init.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irq.h>
> +#include <linux/io.h>
> +#include <linux/syscore_ops.h>
> +
> +#include <asm/i8259.h>
> +
> +#define HTPIC_MAX_PARENT_IRQ	4
> +#define HTINT_NUM_VECTORS	8
> +#define HTINT_EN_OFF		0x20
> +
> +struct loongson_htpic {
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +};
> +
> +struct loongson_htpic *htpic;

static?

> +
> +static void htpic_irq_dispatch(struct irq_desc *desc)
> +{
> +	struct loongson_htpic *priv = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	uint32_t pending;
> +
> +	chained_irq_enter(chip, desc);
> +	pending = readl(priv->base);
> +	/* Ack all IRQs */
> +	writel(pending, priv->base);

Why isn't this done as part of the irq_ack() callback instead? If the
flow handling is different from the 8259, then maybe you shouldn't be
hijacking it...

> +
> +	if (!pending)
> +		spurious_interrupt();
> +
> +	while (pending) {
> +		int bit = __ffs(pending);
> +
> +		if (unlikely(bit > 15))
> +			spurious_interrupt();

and yet you continue handling it?

> +
> +		generic_handle_irq(irq_linear_revmap(priv->domain, bit));
> +		pending &= ~BIT(bit);
> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void htpic_reg_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < HTINT_NUM_VECTORS; i++) {
> +		uint32_t val;
> +
> +		/* Disable all HT Vectors */
> +		writel(0x0, htpic->base + HTINT_EN_OFF + i * 0x4);
> +		val = readl(htpic->base + i * 0x4);
> +		/* Ack all possible pending IRQs */
> +		writel(GENMASK(31, 0), htpic->base + i * 0x4);
> +	}
> +
> +	/* Enable 16 vectors for PIC */
> +	writel(0xffff, htpic->base + HTINT_EN_OFF);
> +}
> +
> +static void htpic_resume(void)
> +{
> +	htpic_reg_init();
> +}
> +
> +struct syscore_ops htpic_syscore_ops = {
> +	.resume		= htpic_resume,
> +};
> +
> +int __init htpic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	unsigned int parent_irq[4];
> +	int i, err;
> +	int num_parents = 0;
> +
> +	if (htpic) {
> +		pr_err("loongson-htpic: Only one HTPIC is allowed in the system\n");
> +		return -ENODEV;
> +	}
> +
> +	htpic = kzalloc(sizeof(*htpic), GFP_KERNEL);
> +	if (!htpic) {
> +		err = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	htpic->base = of_iomap(node, 0);
> +	if (!htpic->base) {
> +		err = -ENODEV;
> +		goto out_free;
> +	}
> +
> +	htpic->domain = of_init_i8259_irqs(node);
> +	if (!htpic->domain) {
> +		pr_err("loongson-htpic: Failed to initialize i8259 IRQs\n");
> +		err = -ENOMEM;
> +		goto out_iounmap;
> +	}
> +
> +	for (i = 0; i < HTPIC_MAX_PARENT_IRQ; i++) {
> +		parent_irq[i] = irq_of_parse_and_map(node, 0);

So you only care about the first interrupt in the DT?

> +		if (parent_irq[i] < 0)

0 is an invalid interrupt.

> +			break;
> +
> +		num_parents++;
> +	}
> +
> +	if (!num_parents) {
> +		pr_err("loongson-htpic: Failed to get parent irqs\n");
> +		err = -ENODEV;
> +		goto out_remove_domain;
> +	}
> +
> +	htpic_reg_init();
> +
> +	for (i = 0; i < num_parents; i++) {
> +		irq_set_chained_handler_and_data(parent_irq[i],
> +						htpic_irq_dispatch, htpic);
> +	}

What is the mapping between the 16 inputs and the 4 outputs?

> +
> +	register_syscore_ops(&htpic_syscore_ops);
> +
> +	return 0;
> +
> +out_remove_domain:
> +	irq_domain_remove(htpic->domain);
> +out_iounmap:
> +	iounmap(htpic->base);
> +out_free:
> +	kfree(htpic);
> +	return err;
> +}
> +
> +IRQCHIP_DECLARE(loongson_htpic, "loongson,htpic-1.0", htpic_of_init);

Thanks,

	M.

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