Hi Thomas,

On Tue, 13 Jan 2015 11:38:14 +0100 (CET)
Thomas Gleixner [off-list ref] wrote:

On Thu, 8 Jan 2015, Boris Brezillon wrote:

quoted

1) Is it close to what you had in mind ?

Yes.

quoted

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.

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+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

quoted

+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