From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@free-electrons.com (Boris Brezillon) Date: Tue, 13 Jan 2015 11:58:30 +0100 Subject: [RFC PATCH] irqchip: add dumb demultiplexer implementation In-Reply-To: References: <1420725159-20720-1-git-send-email-boris.brezillon@free-electrons.com> Message-ID: <20150113115830.43ef505d@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Thomas, On Tue, 13 Jan 2015 11:38:14 +0100 (CET) Thomas Gleixner 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