| Message ID | 1414232097-4328-2-git-send-email-marc.zyngier@arm.com |
|---|---|
| State | New |
| Headers | show |
On Sat, 25 Oct 2014, Marc Zyngier wrote: > +int irq_get_irqchip_state(unsigned int irq, int state) > +{ > + struct irq_desc *desc; > + struct irq_data *data; > + struct irq_chip *chip; > + int val; > + > + desc = irq_to_desc(irq); > + if (!desc) > + return -EINVAL; > + > + data = irq_desc_get_irq_data(desc); > + > + chip = irq_desc_get_chip(desc); > + if (!chip->irq_get_irqchip_state) > + return -EINVAL; > + > + chip_bus_lock(desc); > + val = chip->irq_get_irqchip_state(data, state); Hmm. What's the irq_get_irqchip_state() callback supposed to return? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 25 Oct 2014, Thomas Gleixner wrote: Bah, hit send way too fast :) > On Sat, 25 Oct 2014, Marc Zyngier wrote: > > +int irq_get_irqchip_state(unsigned int irq, int state) get_state(state) does not make sense. get_state(which) probably more so. And 'which' wants to be an enum btw. > > + chip_bus_lock(desc); > > + val = chip->irq_get_irqchip_state(data, state); > > Hmm. What's the irq_get_irqchip_state() callback supposed to return? Either an error code or a boolean value, right? Does not mix very well I think. int irq_get_irqchip_state(unsigned int irq, enum xxx which, bool *val) Might be a more clear interface. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/10/14 20:42, Thomas Gleixner wrote: > On Sat, 25 Oct 2014, Thomas Gleixner wrote: > > Bah, hit send way too fast :) > >> On Sat, 25 Oct 2014, Marc Zyngier wrote: >>> +int irq_get_irqchip_state(unsigned int irq, int state) > > get_state(state) does not make sense. get_state(which) probably more > so. And 'which' wants to be an enum btw. > >>> + chip_bus_lock(desc); >>> + val = chip->irq_get_irqchip_state(data, state); >> >> Hmm. What's the irq_get_irqchip_state() callback supposed to return? > > Either an error code or a boolean value, right? Does not mix very well > I think. > > int irq_get_irqchip_state(unsigned int irq, enum xxx which, bool *val) > > Might be a more clear interface. Agreed, this makes a lot of sense. Will respin it. Thanks, M.
On Sat, Oct 25, 2014 at 12:14 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > There is a number of cases where a kernel subsystem may want to > introspect the state of an interrupt at the irqchip level: > > - When a peripheral is shared between virtual machines, its interrupt > state becomes part of the guest's state, and must be switched accordingly. > KVM on arm/arm64 requires this for its guest-visible timer > - Some GPIO controllers seem to require peeking into the interrupt controller > they are connected to to report their internal state I'd like to know exactly what this means, for GPIO. As mentioned in conversation with Arnd, there is since before the case where a GPIO irqchip gets its irqs "stolen" by some other hardware that is in the always-on domain, and I call these "latent irqs". There is a third usecase here since ages (pre-git) in arch/arm/mach-integrator/integrator_cp.c: /* * It seems that the card insertion interrupt remains active after * we've acknowledged it. We therefore ignore the interrupt, and * rely on reading it from the SIC. This also means that we must * clear the latched interrupt. */ static unsigned int mmc_status(struct device *dev) { unsigned int status = readl(__io_address(0xca000000 + 4)); writel(8, intcp_con_base + 8); return status & 8; } static struct mmci_platform_data mmc_data = { .ocr_mask = MMC_VDD_32_33|MMC_VDD_33_34, .status = mmc_status, .gpio_wp = -1, .gpio_cd = -1, }; This just goes in and peeks around in the Integrator SIC, this patch would solve also this I think. Or are the added calls good for clearing the latched IRQ too? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 29/10/14 10:12, Linus Walleij wrote: > On Sat, Oct 25, 2014 at 12:14 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > >> There is a number of cases where a kernel subsystem may want to >> introspect the state of an interrupt at the irqchip level: >> >> - When a peripheral is shared between virtual machines, its interrupt >> state becomes part of the guest's state, and must be switched accordingly. >> KVM on arm/arm64 requires this for its guest-visible timer >> - Some GPIO controllers seem to require peeking into the interrupt controller >> they are connected to to report their internal state > > I'd like to know exactly what this means, for GPIO. As mentioned in > conversation with Arnd, there is since before the case where a GPIO > irqchip gets its irqs "stolen" by some other hardware that is in the > always-on domain, and I call these "latent irqs". It looks like a slightly different issue: http://patchwork.ozlabs.org/patch/397657/ Basically, the GPIO chip cannot report its own state, and has to introspect the parent irqchip to find out. > There is a third usecase here since ages (pre-git) in > arch/arm/mach-integrator/integrator_cp.c: > > /* > * It seems that the card insertion interrupt remains active after > * we've acknowledged it. We therefore ignore the interrupt, and > * rely on reading it from the SIC. This also means that we must > * clear the latched interrupt. > */ > static unsigned int mmc_status(struct device *dev) > { > unsigned int status = readl(__io_address(0xca000000 + 4)); > writel(8, intcp_con_base + 8); > > return status & 8; > } > > static struct mmci_platform_data mmc_data = { > .ocr_mask = MMC_VDD_32_33|MMC_VDD_33_34, > .status = mmc_status, > .gpio_wp = -1, > .gpio_cd = -1, > }; > > This just goes in and peeks around in the Integrator SIC, this > patch would solve also this I think. Or are the added calls good > for clearing the latched IRQ too? Pretty funky. You could also use this to clear the pending bit (assuming there is one on the CP). I'm amazed at the number of similar hacks that are coming out of the wood now... Thanks, M.
On Wed, Oct 29, 2014 at 12:17 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 29/10/14 10:12, Linus Walleij wrote: >> On Sat, Oct 25, 2014 at 12:14 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> >>> There is a number of cases where a kernel subsystem may want to >>> introspect the state of an interrupt at the irqchip level: >>> >>> - When a peripheral is shared between virtual machines, its interrupt >>> state becomes part of the guest's state, and must be switched accordingly. >>> KVM on arm/arm64 requires this for its guest-visible timer >>> - Some GPIO controllers seem to require peeking into the interrupt controller >>> they are connected to to report their internal state >> >> I'd like to know exactly what this means, for GPIO. As mentioned in >> conversation with Arnd, there is since before the case where a GPIO >> irqchip gets its irqs "stolen" by some other hardware that is in the >> always-on domain, and I call these "latent irqs". > > It looks like a slightly different issue: > http://patchwork.ozlabs.org/patch/397657/ > > Basically, the GPIO chip cannot report its own state, and has to > introspect the parent irqchip to find out. That's incidentally the same problem as in the Integrator/CP, wowsers. What goes around comes around. >> This just goes in and peeks around in the Integrator SIC, this >> patch would solve also this I think. Or are the added calls good >> for clearing the latched IRQ too? > > Pretty funky. You could also use this to clear the pending bit (assuming > there is one on the CP). I'm amazed at the number of similar hacks that > are coming out of the wood now... Yeah we should have fixed this with the Integrator in 2001 or so. Not too late anyways, let's clean it out now :) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Oct 27, 2014 at 4:47 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: [...] > > Agreed, this makes a lot of sense. Will respin it. > Any update on this? I have a couple of drivers on the way that needs this (querying level of an interrupt line). Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index 69517a2..80818b4 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -356,6 +356,8 @@ static inline int disable_irq_wake(unsigned int irq) return irq_set_irq_wake(irq, 0); } +extern int irq_get_irqchip_state(unsigned int irq, int state); +extern int irq_set_irqchip_state(unsigned int irq, int state, int val); #ifdef CONFIG_IRQ_FORCED_THREADING extern bool force_irqthreads; diff --git a/include/linux/irq.h b/include/linux/irq.h index 03f48d9..257d59a 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -315,6 +315,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) * any other callback related to this irq * @irq_release_resources: optional to release resources acquired with * irq_request_resources + * @irq_get_irqchip_state: return the internal state of an interrupt + * @irq_set_irqchip_state: set the internal state of a interrupt * @flags: chip specific flags */ struct irq_chip { @@ -351,6 +353,9 @@ struct irq_chip { int (*irq_request_resources)(struct irq_data *data); void (*irq_release_resources)(struct irq_data *data); + int (*irq_get_irqchip_state)(struct irq_data *data, int state); + void (*irq_set_irqchip_state)(struct irq_data *data, int state, int val); + unsigned long flags; }; @@ -376,6 +381,19 @@ enum { IRQCHIP_EOI_THREADED = (1 << 6), }; +/* + * irq_get_irqchip_state/irq_set_irqchip_state specific flags: + * + * IRQCHIP_STATE_PENDING: Interrupt asserted at the pin level + * IRQCHIP_STATE_ACTIVE: Interrupt in progress (ACKed, but not EOIed) + * IRQCHIP_STATE_MASKED: Interrupt is masked + */ +enum { + IRQCHIP_STATE_PENDING, + IRQCHIP_STATE_ACTIVE, + IRQCHIP_STATE_MASKED, +}; + /* This include will go away once we isolated irq_desc usage to core code */ #include <linux/irqdesc.h> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 0a9104b..6a4c03f 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1756,3 +1756,74 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler, return retval; } + +/** + * irq_get_irqchip_state - returns the irqchip state of a interrupt. + * @irq: Interrupt line that is forwarded to a VM + * @state: One of IRQCHIP_STATE_* the caller wants to know about + * + * This call snapshots the internal irqchip state of an + * interrupt, returning the bit corresponding to the requested + * @state. + * + * This function should be called with preemption disabled if the + * interrupt controller has per-cpu registers. + */ +int irq_get_irqchip_state(unsigned int irq, int state) +{ + struct irq_desc *desc; + struct irq_data *data; + struct irq_chip *chip; + int val; + + desc = irq_to_desc(irq); + if (!desc) + return -EINVAL; + + data = irq_desc_get_irq_data(desc); + + chip = irq_desc_get_chip(desc); + if (!chip->irq_get_irqchip_state) + return -EINVAL; + + chip_bus_lock(desc); + val = chip->irq_get_irqchip_state(data, state); + chip_bus_sync_unlock(desc); + + return val; +} + +/** + * irq_set_irqchip_state - set the state of a forwarded interrupt. + * @irq: Interrupt line that is forwarded to a VM + * @state: State to be restored (one of IRQCHIP_STATE_*) + * @val: value corresponding to @state + * + * This call sets the internal irqchip state of an interrupt, + * depending on the value of @state. + * + * This function should be called with preemption disabled if the + * interrupt controller has per-cpu registers. + */ +int irq_set_irqchip_state(unsigned int irq, int state, int val) +{ + struct irq_desc *desc; + struct irq_data *data; + struct irq_chip *chip; + + desc = irq_to_desc(irq); + if (!desc) + return -EINVAL; + + data = irq_desc_get_irq_data(desc); + + chip = irq_desc_get_chip(desc); + if (!chip->irq_set_irqchip_state) + return -EINVAL; + + chip_bus_lock(desc); + chip->irq_set_irqchip_state(data, state, val); + chip_bus_sync_unlock(desc); + + return 0; +}
There is a number of cases where a kernel subsystem may want to introspect the state of an interrupt at the irqchip level: - When a peripheral is shared between virtual machines, its interrupt state becomes part of the guest's state, and must be switched accordingly. KVM on arm/arm64 requires this for its guest-visible timer - Some GPIO controllers seem to require peeking into the interrupt controller they are connected to to report their internal state This seem to be a pattern that is common enough for the core code to try and support this without too many horrible hacks. Introduce a pair of accessors (irq_get_irqchip_state/irq_set_irqchip_state) to retrieve the bits that can be of interest to another subsystem: pending, active, and masked. - irq_get_irqchip_state returns the state of the interrupt according to a state parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE or IRQCHIP_STATE_MASKED. - irq_set_irqchip_state sets the state of the interrupt according to a similar state. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- include/linux/interrupt.h | 2 ++ include/linux/irq.h | 18 ++++++++++++ kernel/irq/manage.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+)