Re: [RFC PATCH] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Thu, 8 Jan 2015, Boris Brezillon wrote:
> 1) Is it close to what you had in mind ?

Yes.

> 2) I'm not sure which part of the code should go where, so don't hesitate
>    to point out misplaced sections.

Looks sane. Nits below.

> 3) Do I need to disable the source irq (the one feeding the irqchip) when
>    entering suspend (and enable it on resume) ?

That probably needs to be part of the dumb mask/unmask functions.,
i.e. if no demux interrupt is enabled, the source irq should be
masked.

> 4) I'm not sure of what flags should be set/cleared when mapping an
>    interrupt. Should I let the caller decide of this config (something
>    similar to what is done in generic-chip) ?

I don't think you need to set/clear anything. Lets look at that dumb
demux chip as a real electronic circuit

     	     	  |----------------|
		  |                |
     	     	  |  --|Unmasked|--|---- Demux0
		  |  |             |
     SRC irq -----|-- -|Unmasked|--|---- Demux1
		  |  |             |
     	     	  |  --|Unmasked|--|---- Demux2
		  |                |
     	     	  |----------------|

Whether a demultiplexed interrupt is mapped or not is not really
important. The only relevant information is whether its masked or
not. So the default state is masked until a demultiplexed interrupt
gets requested.

> +/**
> + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips
> + * @IRQ_DD_INIT_NESTED_LOCK:	Set the lock class of the irqs to nested for
> + *				irq chips which need to call irq_set_wake() on
> + *				the parent irq. Usually GPIO implementations
> + */
> +enum irq_dumb_demux_flags {
> +	IRQ_DD_INIT_NESTED_LOCK		= 1 << 0,
> +};
> +
> +/**
> + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
> + * @domain:		irq domain pointer
> + * @max_irq:		Last valid irq
> + * @available:		Bitfield of valid irqs
> + * @unmasked:		Bitfield containing irqs status
> + * @flags:		irq_dumb_demux_flags flags
> + *
> + * Note, that irq_chip_generic can have multiple irq_chip_type
> + * implementations which can be associated to a particular irq line of
> + * an irq_chip_generic instance. That allows to share and protect
> + * state in an irq_chip_generic instance when we need to implement
> + * different flow mechanisms (level/edge) for it.
> + */
> +struct irq_chip_dumb_demux {
> +	struct irq_domain *domain;
> +	int max_irq;
> +	unsigned long *available;
> +	unsigned long *unmasked;

Why pointers? A single ulong is certainly enough.

> +/**
> + *	handle_dumb_demux_irq - Dumb demuxer irq handle function.
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Dumb demux interrupts are sent from a demultiplexing interrupt handler
> + *	which is not able to decide which child interrupt interrupt handler
> + *	should be called.
> + *
> + *	Note: The caller is expected to handle the ack, clear, mask and
> + *	unmask issues if necessary.
> + */
> +irqreturn_t
> +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	irqreturn_t retval = IRQ_NONE;
> +
> +	raw_spin_lock(&desc->lock);
> +
> +	if (!irq_may_run(desc))
> +		goto out_unlock;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +	kstat_incr_irqs_this_cpu(irq, desc);
> +
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		goto out_unlock;
> +	}
> +
> +	retval = handle_irq_event_no_spurious_check(desc);
> +
> +out_unlock:
> +	raw_spin_unlock(&desc->lock);
> +
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq);

This should go into the new file as well, so it gets compiled out when
not enabled.

> +static void irq_dumb_demux_mask(struct irq_data *d)
> +{
> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> +	clear_bit(d->hwirq, demux->unmasked);
> +}
> +
> +static void irq_dumb_demux_unmask(struct irq_data *d)
> +{
> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> +	set_bit(d->hwirq, demux->unmasked);
> +}

So this needs the handling of enabling/disabling the source irq.

> +static struct irq_chip irq_dumb_demux_chip = {
> +	.name = "dumb-demux-irq",
> +	.irq_mask = irq_dumb_demux_mask,
> +	.irq_unmask = irq_dumb_demux_unmask,

+	.name		= "dumb-demux-irq",
+	.irq_mask	= irq_dumb_demux_mask,
+	.irq_unmask	= irq_dumb_demux_unmask,

Makes it simpler to read.

> +struct irq_domain_ops irq_dumb_demux_domain_ops = {
> +	.map	= irq_map_dumb_demux_chip,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +EXPORT_SYMBOL(irq_dumb_demux_domain_ops);

SYMBOL_GPL please

Rest looks good.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/