From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752116AbbAMK6g (ORCPT ); Tue, 13 Jan 2015 05:58:36 -0500 Received: from down.free-electrons.com ([37.187.137.238]:32954 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751107AbbAMK6e (ORCPT ); Tue, 13 Jan 2015 05:58:34 -0500 Date: Tue, 13 Jan 2015 11:58:30 +0100 From: Boris Brezillon To: Thomas Gleixner Cc: Nicolas Ferre , Jean-Christophe Plagniol-Villard , Alexandre Belloni , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Rafael J. Wysocki" Subject: Re: [RFC PATCH] irqchip: add dumb demultiplexer implementation Message-ID: <20150113115830.43ef505d@bbrezillon> In-Reply-To: References: <1420725159-20720-1-git-send-email-boris.brezillon@free-electrons.com> X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.23; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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