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

[Boris Brezillon]

Hi Thomas,

On Tue, 13 Jan 2015 11:38:14 +0100 (CET)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> 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.

Ok, I'll add that part.

> 
> > 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.

Hmm, my question was not really clear: I was talking about irq flags [1]
(those that are set with irq_modify_status in the generic irq chip [2]).

> 
> > +/**
> > + * 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.

Okay, just thought one might need a dumb demuxer with more than 32 (or
64) outputs, but I guess we can limit it to an unsigned long for now.

> 
> > +/**
> > + *	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.

Sure.

> 
> > +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.

Yep.

> 
> > +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.

I'll fix that

> 
> > +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

and that too.

Thanks,

Boris


[1]http://lxr.free-electrons.com/source/kernel/irq/settings.h#L21
[2]http://lxr.free-electrons.com/source/kernel/irq/generic-chip.c#L394

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/