Hi Thomas,
On Tue, 13 Jan 2015 11:38:14 +0100 (CET)
Thomas Gleixner <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
