Re: [PATCH] MIPS, IRQCHIP: Move i8259 irqchip driver to drivers/irqchip.

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 7/8/2015 3:46 PM, Ralf Baechle wrote:

>   arch/mips/Kconfig           |   4 -
>   arch/mips/kernel/Makefile   |   1 -
>   arch/mips/kernel/i8259.c    | 384 --------------------------------------------
>   drivers/irqchip/Kconfig     |   4 +
>   drivers/irqchip/Makefile    |   1 +
>   drivers/irqchip/irq-i8259.c | 383 +++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 388 insertions(+), 389 deletions(-)

[...]

> diff --git a/drivers/irqchip/irq-i8259.c b/drivers/irqchip/irq-i8259.c
> new file mode 100644
> index 0000000..a29638a
> --- /dev/null
> +++ b/drivers/irqchip/irq-i8259.c
> @@ -0,0 +1,383 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Code to handle x86 style IRQs plus some generic interrupt stuff.
> + *
> + * Copyright (C) 1992 Linus Torvalds
> + * Copyright (C) 1994 - 2000 Ralf Baechle
> + */
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of_irq.h>
> +#include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
> +#include <linux/irq.h>
> +
> +#include <asm/i8259.h>
> +#include <asm/io.h>
> +
> +/*
> + * This is the 'legacy' 8259A Programmable Interrupt Controller,
> + * present in the majority of PC/AT boxes.
> + * plus some generic x86 specific things if generic specifics makes
> + * any sense at all.
> + * this file should become arch/i386/kernel/irq.c when the old irq.c
> + * moves to arch independent land

    This comment doesn't make sense anymore, does it?

> +static struct irq_chip i8259A_chip = {
> +	.name			= "XT-PIC",

    This name is wrong, wrong, wrong. XT only had single 8259 (and is not 
supported by Linux anyway) while jhere we drive the AT specific two cascaded 
8259s (which is just wrong in my opinion as well).

> +	.irq_mask		= disable_8259A_irq,
> +	.irq_disable		= disable_8259A_irq,
> +	.irq_unmask		= enable_8259A_irq,
> +	.irq_mask_ack		= mask_and_ack_8259A,

    I have always thought 8259 was the "fast-EOI" class interrupt chip, I've 
never quite understood all this mask-and-ACK type handling for 8259...

[...]
> +/*
> + * Careful! The 8259A is a fragile beast, it pretty
> + * much _has_ to be done exactly like this (mask it
> + * first, _then_ send the EOI, and the order of EOI
> + * to the two 8259s is important!
> + */
> +static void mask_and_ack_8259A(struct irq_data *d)
> +{
[...]
> +handle_real_irq:
> +	if (irq & 8) {
> +		inb(PIC_SLAVE_IMR);	/* DUMMY - (do we need this?) */

    Hardly.

> +		outb(cached_slave_mask, PIC_SLAVE_IMR);
> +		outb(0x60+(irq&7), PIC_SLAVE_CMD);/* 'Specific EOI' to slave */

   Need spaces around ops.

> +		outb(0x60+PIC_CASCADE_IR, PIC_MASTER_CMD); /* 'Specific EOI' to master-IRQ2 */
> +	} else {
> +		inb(PIC_MASTER_IMR);	/* DUMMY - (do we need this?) */
> +		outb(cached_master_mask, PIC_MASTER_IMR);
> +		outb(0x60+irq, PIC_MASTER_CMD); /* 'Specific EOI to master */

    Same here...

> +	}
> +	raw_spin_unlock_irqrestore(&i8259A_lock, flags);
> +	return;

> +	{
> +		static int spurious_irq_mask;
> +		/*
> +		 * At this point we can be sure the IRQ is spurious,
> +		 * lets ACK and report it. [once per IRQ]

    There's no point in ACKing spurious interrupt, if my memory serves. The 
in-srvice register doesn't have the bit set, so no need to clear it.

> +		 */
> +		if (!(spurious_irq_mask & irqmask)) {
> +			printk(KERN_DEBUG "spurious 8259A interrupt: IRQ%d.\n", irq);
> +			spurious_irq_mask |= irqmask;
> +		}
> +		atomic_inc(&irq_err_count);
> +		/*
> +		 * Theoretically we do not have to handle this IRQ,
> +		 * but in Linux this does not cause problems and is
> +		 * simpler for us.
> +		 */
> +		goto handle_real_irq;

    Hum... only because it doesn't cause issues?

WBR, Sergei