Re: [RFC] MIPS: PS2: Interrupt request (IRQ) support

[Thomas Gleixner <tglx@linutronix.de>]

On Sun, 18 Mar 2018, Fredrik Noring wrote:

> Hi Maciej and Thomas,
> 
> Thomas: Please have a look at the first questions below, regarding
> irq_data->mask and irq_chip->irq_calc_mask. Are they supposed to be usable?
> 
> > +static volatile unsigned long intc_mask = 0;	/* FIXME: Why volatile? */
> > +
> > +static inline void intc_enable_irq(struct irq_data *data)
> > +{
> > +	if (!(intc_mask & (1 << data->irq))) {
> > +		intc_mask |= (1 << data->irq);
> > +		outl(1 << data->irq, INTC_MASK);
> > +	}
> > +}
> 
> The intc_mask variable can be removed, since INTC_MASK is readable, although
> perhaps there are performance reasons to not read the register directly?
> 
> I also noticed that struct irq_data contains a mask field, which allows
> simplifications to
> 
> static inline void intc_enable_irq(struct irq_data *data)
> {
> 	if (!(inl(INTC_MASK) & data->mask))
> 		outl(data->mask, INTC_MASK);

That's a pointless exercise. The core code already knows whether an
interrupt is masked or not and makes the calls conditionally.

> }
> 
> provided the following patch is applied to kernel/irq/irqdesc.c:
> 
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -109,6 +109,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
>  	desc->irq_common_data.msi_desc = NULL;
>  
>  	desc->irq_data.common = &desc->irq_common_data;
> +	desc->irq_data.mask = 1 << irq; /* FIXME: What about irq_calc_mask? */
> 
> Perhaps the mask field ought to be assigned "1 << irq_data->hwirq" instead,
> unless irq_calc_mask is provided. The mask documentation is not entirely
> clear on the use and any restrictions, and it does not seem to be used all
> that much.

Neither works. @irq is the virtual Linux interrupt number and there is no
guarantee that it maps 1:1 to a hardware irq number. Also this falls apart
when @irq >= 32 because the mask field is 32bit....

> The mask field and irq_calc_mask were introduced by Thomas Gleixner in
> commits 966dc736b819 "genirq: Generic chip: Cache per irq bit mask" and
> d0051816e619 "genirq: irqchip: Add a mask calculation function" in 2013.

Yes, The generic irq chip uses this. I havent seen the full irqchip patch
so I can't tell whether this driver could/should use the generic chip.

Thanks,

	tglx