[PATCH v4 0/3] genirq: Saving/restoring the irqchip state of an irq line

[PATCH v4 0/3] genirq: Saving/restoring the irqchip state of an irq line

[Marc Zyngier]

Despite Linux offering a rather fine grained control over the life
cycle of an interrupt, there is a few cases where it would be very
useful to snapshot (or even set) the internal state of the interrupt
controller for a given interrupt line:

- With KVM, a device shared between VMs must have its whole context
  switched, and that includes the interrupt line state. KVM/arm is
  moving to using this.
- Some GPIO controllers seem to require peeking into the interrupt
  controller they are connected to to report their internal state.

Instead of letting people facing this situation doing horrible
(controller specific) hacks in their code, let's offer a couple of new
entry points that allow a few attributes to be read and set.

Of course, this is a very dangerous thing to do if you don't know what
you doing, and I wouldn't expect most drivers to use this. But this
can also be a life saver at times.

This patch series implement said API, and adds support for this to the
two main ARM interrupt controllers (GIC and GICv3). Available at:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/irqchip_state

This has been fairly heavily tested with KVM on ARM, which makes use
of this feature to context switch the timer interrupt (see the last
three patch in the above branch).

Thanks,

	M.

* From v3 [3]
- Rebased on top of 4.0-rc4

* From v2 [2]:
- Rebased on top of 3.19-rc3
- Fixed irq_set_irqchip_state return value

* From v1 [1]:
- Rebased on top of 3.18-rc7 + tip/irq/irqdomain-arm
- Now deals with stacked domains
- Changed the API to be less ambiguous (after review by tglx)
- Added IRQCHIP_STATE_LINE_LEVEL (as requested by Bjorn Anderson)

[1] https://lkml.org/lkml/2014/10/25/134
[2] https://lkml.org/lkml/2014/12/3/612
[3] http://lkml.iu.edu/hypermail/linux/kernel/1501.0/03473.html

Marc Zyngier (3):
  genirq: Allow the irqchip state of an IRQ to be save/restored
  irqchip: GIC: Add support for irq_{get,set}_irqchip_state
  irqchip: GICv3: Add support for irq_{get,set}_irqchip_state

 drivers/irqchip/irq-gic-v3.c | 83 +++++++++++++++++++++++++++++++++-------
 drivers/irqchip/irq-gic.c    | 69 +++++++++++++++++++++++++++++++--
 include/linux/interrupt.h    | 14 +++++++
 include/linux/irq.h          |  6 +++
 kernel/irq/manage.c          | 91 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 246 insertions(+), 17 deletions(-)

-- 
2.1.4

[PATCH v4 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Marc Zyngier]

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 parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
  IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
- irq_set_irqchip_state similarly sets the state of the interrupt.

Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/interrupt.h | 14 ++++++++
 include/linux/irq.h       |  6 ++++
 kernel/irq/manage.c       | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2e88580..011cc64 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -363,6 +363,20 @@ static inline int disable_irq_wake(unsigned int irq)
 	return irq_set_irq_wake(irq, 0);
 }
 
+/*
+ * irq_get_irqchip_state/irq_set_irqchip_state specific flags
+ */
+enum irqchip_irq_state {
+	IRQCHIP_STATE_PENDING,		/* Is interrupt pending? */
+	IRQCHIP_STATE_ACTIVE,		/* Is interrupt in progress? */
+	IRQCHIP_STATE_MASKED,		/* Is interrupt masked? */
+	IRQCHIP_STATE_LINE_LEVEL,	/* Is IRQ line high? */
+};
+
+extern int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+				 bool *state);
+extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+				 bool state);
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 extern bool force_irqthreads;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..77dd2e7 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -30,6 +30,7 @@
 struct seq_file;
 struct module;
 struct msi_msg;
+enum irqchip_irq_state;
 
 /*
  * IRQ line status.
@@ -324,6 +325,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  *				irq_request_resources
  * @irq_compose_msi_msg:	optional to compose message content for MSI
  * @irq_write_msi_msg:	optional to write message content for MSI
+ * @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 {
@@ -363,6 +366,9 @@ struct irq_chip {
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
 	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
 
+	int		(*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state);
+	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
+
 	unsigned long	flags;
 };
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 886d09e..08308e1 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1766,3 +1766,94 @@ 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
+ *	@which: One of IRQCHIP_STATE_* the caller wants to know about
+ *	@state: a pointer to a boolean where the state is to be storeed
+ *
+ *	This call snapshots the internal irqchip state of an
+ *	interrupt, returning into @state the bit corresponding to
+ *	stage @which
+ *
+ *	This function should be called with preemption disabled if the
+ *	interrupt controller has per-cpu registers.
+ */
+int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+			  bool *state)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int err = -EINVAL;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return err;
+
+	data = irq_desc_get_irq_data(desc);
+
+	do {
+		chip = irq_data_get_irq_chip(data);
+		if (chip->irq_get_irqchip_state)
+			break;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		data = data->parent_data;
+#else
+		data = NULL;
+#endif
+	} while (data);
+
+	if (data)
+		err = chip->irq_get_irqchip_state(data, which, state);
+
+	irq_put_desc_busunlock(desc, flags);
+	return err;
+}
+
+/**
+ *	irq_set_irqchip_state - set the state of a forwarded interrupt.
+ *	@irq: Interrupt line that is forwarded to a VM
+ *	@which: State to be restored (one of IRQCHIP_STATE_*)
+ *	@val: Value corresponding to @which
+ *
+ *	This call sets the internal irqchip state of an interrupt,
+ *	depending on the value of @which.
+ *
+ *	This function should be called with preemption disabled if the
+ *	interrupt controller has per-cpu registers.
+ */
+int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+			  bool val)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int err = -EINVAL;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return err;
+
+	data = irq_desc_get_irq_data(desc);
+
+	do {
+		chip = irq_data_get_irq_chip(data);
+		if (chip->irq_set_irqchip_state)
+			break;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		data = data->parent_data;
+#else
+		data = NULL;
+#endif
+	} while (data);
+
+	if (data)
+		err = chip->irq_set_irqchip_state(data, which, val);
+
+	irq_put_desc_busunlock(desc, flags);
+	return err;
+}
-- 
2.1.4

Re: [PATCH v4 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Bjorn Andersson]

On Wed, Mar 18, 2015 at 4:01 AM, 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
>
> 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 parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
>   IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
> - irq_set_irqchip_state similarly sets the state of the interrupt.
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Sorry for bothering you Thomas, but we have a couple of driver for the
Qualcomm platforms that depends on this patch (the line level part).
Could you please have a look at it?

Thanks,
Bjorn

Re: [PATCH v4 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Srinivas Kandagatla]

On 08/04/15 18:48, Bjorn Andersson wrote:
> On Wed, Mar 18, 2015 at 4:01 AM, 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
>>
>> 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 parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
>>    IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
>> - irq_set_irqchip_state similarly sets the state of the interrupt.
>>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>
> Sorry for bothering you Thomas, but we have a couple of driver for the
> Qualcomm platforms that depends on this patch (the line level part).

+1

Couple of Qualcomm board support are left incomplete due to missing 
drivers using these patches.
Would be nice to get this patch mainlined sooner.

-srini

> Could you please have a look at it?
>
> Thanks,
> Bjorn
> --
> 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
>

Re: [PATCH v4 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Marc Zyngier]

On 13/04/15 17:18, Srinivas Kandagatla wrote:
> 
> 
> On 08/04/15 18:48, Bjorn Andersson wrote:
>> On Wed, Mar 18, 2015 at 4:01 AM, 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
>>>
>>> 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 parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
>>>    IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
>>> - irq_set_irqchip_state similarly sets the state of the interrupt.
>>>
>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>
>> Sorry for bothering you Thomas, but we have a couple of driver for the
>> Qualcomm platforms that depends on this patch (the line level part).
> 
> +1
> 
> Couple of Qualcomm board support are left incomplete due to missing 
> drivers using these patches.
> Would be nice to get this patch mainlined sooner.

These patches are already in -next. Should everything go according to
plan, they should hit 4.1.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[tip:irq/core] genirq: Allow the irqchip state of an IRQ to be save/restored

[tip-bot for Marc Zyngier]

Commit-ID:  1b7047edfcfb257f69e306c9afbab150cb987717
Gitweb:     http://git.kernel.org/tip/1b7047edfcfb257f69e306c9afbab150cb987717
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Wed, 18 Mar 2015 11:01:22 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 8 Apr 2015 23:28:28 +0200

genirq: Allow the irqchip state of an IRQ to be save/restored

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 parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
  IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
- irq_set_irqchip_state similarly sets the state of the interrupt.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Phong Vo <pvo@apm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Tin Huynh <tnhuynh@apm.com>
Cc: Y Vo <yvo@apm.com>
Cc: Toan Le <toanle@apm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Arnd Bergmann <arnd@arndb.de>
Link: http://lkml.kernel.org/r/1426676484-21812-2-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/interrupt.h | 14 ++++++++
 include/linux/irq.h       |  6 ++++
 kernel/irq/manage.c       | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 150dde0..950ae45 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -361,6 +361,20 @@ static inline int disable_irq_wake(unsigned int irq)
 	return irq_set_irq_wake(irq, 0);
 }
 
+/*
+ * irq_get_irqchip_state/irq_set_irqchip_state specific flags
+ */
+enum irqchip_irq_state {
+	IRQCHIP_STATE_PENDING,		/* Is interrupt pending? */
+	IRQCHIP_STATE_ACTIVE,		/* Is interrupt in progress? */
+	IRQCHIP_STATE_MASKED,		/* Is interrupt masked? */
+	IRQCHIP_STATE_LINE_LEVEL,	/* Is IRQ line high? */
+};
+
+extern int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+				 bool *state);
+extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+				 bool state);
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 extern bool force_irqthreads;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..77dd2e7 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -30,6 +30,7 @@
 struct seq_file;
 struct module;
 struct msi_msg;
+enum irqchip_irq_state;
 
 /*
  * IRQ line status.
@@ -324,6 +325,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  *				irq_request_resources
  * @irq_compose_msi_msg:	optional to compose message content for MSI
  * @irq_write_msi_msg:	optional to write message content for MSI
+ * @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 {
@@ -363,6 +366,9 @@ struct irq_chip {
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
 	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
 
+	int		(*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state);
+	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
+
 	unsigned long	flags;
 };
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c0a1100..e68932b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1798,3 +1798,94 @@ 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
+ *	@which: One of IRQCHIP_STATE_* the caller wants to know about
+ *	@state: a pointer to a boolean where the state is to be storeed
+ *
+ *	This call snapshots the internal irqchip state of an
+ *	interrupt, returning into @state the bit corresponding to
+ *	stage @which
+ *
+ *	This function should be called with preemption disabled if the
+ *	interrupt controller has per-cpu registers.
+ */
+int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+			  bool *state)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int err = -EINVAL;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return err;
+
+	data = irq_desc_get_irq_data(desc);
+
+	do {
+		chip = irq_data_get_irq_chip(data);
+		if (chip->irq_get_irqchip_state)
+			break;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		data = data->parent_data;
+#else
+		data = NULL;
+#endif
+	} while (data);
+
+	if (data)
+		err = chip->irq_get_irqchip_state(data, which, state);
+
+	irq_put_desc_busunlock(desc, flags);
+	return err;
+}
+
+/**
+ *	irq_set_irqchip_state - set the state of a forwarded interrupt.
+ *	@irq: Interrupt line that is forwarded to a VM
+ *	@which: State to be restored (one of IRQCHIP_STATE_*)
+ *	@val: Value corresponding to @which
+ *
+ *	This call sets the internal irqchip state of an interrupt,
+ *	depending on the value of @which.
+ *
+ *	This function should be called with preemption disabled if the
+ *	interrupt controller has per-cpu registers.
+ */
+int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+			  bool val)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int err = -EINVAL;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return err;
+
+	data = irq_desc_get_irq_data(desc);
+
+	do {
+		chip = irq_data_get_irq_chip(data);
+		if (chip->irq_set_irqchip_state)
+			break;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		data = data->parent_data;
+#else
+		data = NULL;
+#endif
+	} while (data);
+
+	if (data)
+		err = chip->irq_set_irqchip_state(data, which, val);
+
+	irq_put_desc_busunlock(desc, flags);
+	return err;
+}

[PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

Add the required hooks for the internal state of an interrupt
to be exposed to other subsystems.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 471e1cd..0b4a4d0 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -151,13 +151,24 @@ static inline unsigned int gic_irq(struct irq_data *d)
 /*
  * Routines to acknowledge, disable and enable interrupts
  */
-static void gic_mask_irq(struct irq_data *d)
+static void gic_poke_irq(struct irq_data *d, u32 offset)
+{
+	u32 mask = 1 << (gic_irq(d) % 32);
+	writel_relaxed(mask, gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4);
+}
+
+static int gic_peek_irq(struct irq_data *d, u32 offset)
 {
 	u32 mask = 1 << (gic_irq(d) % 32);
+	return !!(readl_relaxed(gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4) & mask);
+}
+
+static void gic_mask_irq(struct irq_data *d)
+{
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
-	writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_CLEAR + (gic_irq(d) / 32) * 4);
+	gic_poke_irq(d, GIC_DIST_ENABLE_CLEAR);
 	if (gic_arch_extn.irq_mask)
 		gic_arch_extn.irq_mask(d);
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
@@ -165,13 +176,12 @@ static void gic_mask_irq(struct irq_data *d)
 
 static void gic_unmask_irq(struct irq_data *d)
 {
-	u32 mask = 1 << (gic_irq(d) % 32);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 	if (gic_arch_extn.irq_unmask)
 		gic_arch_extn.irq_unmask(d);
-	writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_SET + (gic_irq(d) / 32) * 4);
+	gic_poke_irq(d, GIC_DIST_ENABLE_SET);
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }
 
@@ -186,6 +196,55 @@ static void gic_eoi_irq(struct irq_data *d)
 	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
 }
 
+static int gic_irq_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool val)
+{
+	u32 reg;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		reg = val ? GIC_DIST_PENDING_SET : GIC_DIST_PENDING_CLEAR;
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		reg = val ? GIC_DIST_ACTIVE_SET : GIC_DIST_ACTIVE_CLEAR;
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		reg = val ? GIC_DIST_ENABLE_CLEAR : GIC_DIST_ENABLE_SET;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	gic_poke_irq(d, reg);
+	return 0;
+}
+
+static int gic_irq_get_irqchip_state(struct irq_data *d,
+				      enum irqchip_irq_state which, bool *val)
+{
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		*val = gic_peek_irq(d, GIC_DIST_PENDING_SET);
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		*val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		*val = !gic_peek_irq(d, GIC_DIST_ENABLE_SET);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
@@ -329,6 +388,8 @@ static struct irq_chip gic_chip = {
 	.irq_set_affinity	= gic_set_affinity,
 #endif
 	.irq_set_wake		= gic_set_wake,
+	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 };
 
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
-- 
2.1.4

[tip:irq/core] irqchip: GIC: Add support for irq_[get, set] _irqchip_state()

[tip-bot for Marc Zyngier]

Commit-ID:  567178077c369ae6a32f90926fb4172f7cf1f87f
Gitweb:     http://git.kernel.org/tip/567178077c369ae6a32f90926fb4172f7cf1f87f
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Wed, 18 Mar 2015 11:01:23 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 8 Apr 2015 23:28:28 +0200

irqchip: GIC: Add support for irq_[get, set]_irqchip_state()

Add the required hooks for the internal state of an interrupt
to be exposed to other subsystems.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Phong Vo <pvo@apm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Tin Huynh <tnhuynh@apm.com>
Cc: Y Vo <yvo@apm.com>
Cc: Toan Le <toanle@apm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Arnd Bergmann <arnd@arndb.de>
Link: http://lkml.kernel.org/r/1426676484-21812-3-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/irq-gic.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 471e1cd..0b4a4d0 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -151,13 +151,24 @@ static inline unsigned int gic_irq(struct irq_data *d)
 /*
  * Routines to acknowledge, disable and enable interrupts
  */
-static void gic_mask_irq(struct irq_data *d)
+static void gic_poke_irq(struct irq_data *d, u32 offset)
+{
+	u32 mask = 1 << (gic_irq(d) % 32);
+	writel_relaxed(mask, gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4);
+}
+
+static int gic_peek_irq(struct irq_data *d, u32 offset)
 {
 	u32 mask = 1 << (gic_irq(d) % 32);
+	return !!(readl_relaxed(gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4) & mask);
+}
+
+static void gic_mask_irq(struct irq_data *d)
+{
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
-	writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_CLEAR + (gic_irq(d) / 32) * 4);
+	gic_poke_irq(d, GIC_DIST_ENABLE_CLEAR);
 	if (gic_arch_extn.irq_mask)
 		gic_arch_extn.irq_mask(d);
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
@@ -165,13 +176,12 @@ static void gic_mask_irq(struct irq_data *d)
 
 static void gic_unmask_irq(struct irq_data *d)
 {
-	u32 mask = 1 << (gic_irq(d) % 32);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 	if (gic_arch_extn.irq_unmask)
 		gic_arch_extn.irq_unmask(d);
-	writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_SET + (gic_irq(d) / 32) * 4);
+	gic_poke_irq(d, GIC_DIST_ENABLE_SET);
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }
 
@@ -186,6 +196,55 @@ static void gic_eoi_irq(struct irq_data *d)
 	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
 }
 
+static int gic_irq_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool val)
+{
+	u32 reg;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		reg = val ? GIC_DIST_PENDING_SET : GIC_DIST_PENDING_CLEAR;
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		reg = val ? GIC_DIST_ACTIVE_SET : GIC_DIST_ACTIVE_CLEAR;
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		reg = val ? GIC_DIST_ENABLE_CLEAR : GIC_DIST_ENABLE_SET;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	gic_poke_irq(d, reg);
+	return 0;
+}
+
+static int gic_irq_get_irqchip_state(struct irq_data *d,
+				      enum irqchip_irq_state which, bool *val)
+{
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		*val = gic_peek_irq(d, GIC_DIST_PENDING_SET);
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		*val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		*val = !gic_peek_irq(d, GIC_DIST_ENABLE_SET);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
@@ -329,6 +388,8 @@ static struct irq_chip gic_chip = {
 	.irq_set_affinity	= gic_set_affinity,
 #endif
 	.irq_set_wake		= gic_set_wake,
+	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 };
 
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

Marc:

Sorry for top posting. It seems with GIC-400, the get_irqstate is not a reliable
way of accessing the GPIO status. In our case, the gpio interrupt status can
only be read correctly with the SPISR register. So it seems we have to go
back to the old way of mapping the SPISR portion of the GIC in our GPIO
driver. We wanted to ask you if that is acceptable since SPISR in not
standard in all GIC, perhaps we can treat it differently.

On Wed, Mar 18, 2015 at 4:01 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Add the required hooks for the internal state of an interrupt
> to be exposed to other subsystems.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 471e1cd..0b4a4d0 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -151,13 +151,24 @@ static inline unsigned int gic_irq(struct irq_data *d)
>  /*
>   * Routines to acknowledge, disable and enable interrupts
>   */
> -static void gic_mask_irq(struct irq_data *d)
> +static void gic_poke_irq(struct irq_data *d, u32 offset)
> +{
> +       u32 mask = 1 << (gic_irq(d) % 32);
> +       writel_relaxed(mask, gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4);
> +}
> +
> +static int gic_peek_irq(struct irq_data *d, u32 offset)
>  {
>         u32 mask = 1 << (gic_irq(d) % 32);
> +       return !!(readl_relaxed(gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4) & mask);
> +}
> +
> +static void gic_mask_irq(struct irq_data *d)
> +{
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&irq_controller_lock, flags);
> -       writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_CLEAR + (gic_irq(d) / 32) * 4);
> +       gic_poke_irq(d, GIC_DIST_ENABLE_CLEAR);
>         if (gic_arch_extn.irq_mask)
>                 gic_arch_extn.irq_mask(d);
>         raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
> @@ -165,13 +176,12 @@ static void gic_mask_irq(struct irq_data *d)
>
>  static void gic_unmask_irq(struct irq_data *d)
>  {
> -       u32 mask = 1 << (gic_irq(d) % 32);
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&irq_controller_lock, flags);
>         if (gic_arch_extn.irq_unmask)
>                 gic_arch_extn.irq_unmask(d);
> -       writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_SET + (gic_irq(d) / 32) * 4);
> +       gic_poke_irq(d, GIC_DIST_ENABLE_SET);
>         raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
>  }
>
> @@ -186,6 +196,55 @@ static void gic_eoi_irq(struct irq_data *d)
>         writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>  }
>
> +static int gic_irq_set_irqchip_state(struct irq_data *d,
> +                                    enum irqchip_irq_state which, bool val)
> +{
> +       u32 reg;
> +
> +       switch (which) {
> +       case IRQCHIP_STATE_PENDING:
> +               reg = val ? GIC_DIST_PENDING_SET : GIC_DIST_PENDING_CLEAR;
> +               break;
> +
> +       case IRQCHIP_STATE_ACTIVE:
> +               reg = val ? GIC_DIST_ACTIVE_SET : GIC_DIST_ACTIVE_CLEAR;
> +               break;
> +
> +       case IRQCHIP_STATE_MASKED:
> +               reg = val ? GIC_DIST_ENABLE_CLEAR : GIC_DIST_ENABLE_SET;
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       gic_poke_irq(d, reg);
> +       return 0;
> +}
> +
> +static int gic_irq_get_irqchip_state(struct irq_data *d,
> +                                     enum irqchip_irq_state which, bool *val)
> +{
> +       switch (which) {
> +       case IRQCHIP_STATE_PENDING:
> +               *val = gic_peek_irq(d, GIC_DIST_PENDING_SET);
> +               break;
> +
> +       case IRQCHIP_STATE_ACTIVE:
> +               *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
> +               break;
> +
> +       case IRQCHIP_STATE_MASKED:
> +               *val = !gic_peek_irq(d, GIC_DIST_ENABLE_SET);
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  static int gic_set_type(struct irq_data *d, unsigned int type)
>  {
>         void __iomem *base = gic_dist_base(d);
> @@ -329,6 +388,8 @@ static struct irq_chip gic_chip = {
>         .irq_set_affinity       = gic_set_affinity,
>  #endif
>         .irq_set_wake           = gic_set_wake,
> +       .irq_get_irqchip_state  = gic_irq_get_irqchip_state,
> +       .irq_set_irqchip_state  = gic_irq_set_irqchip_state,
>  };
>
>  void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
> --
> 2.1.4
>
> --
> 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/

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Wed, May 13, 2015 at 4:25 AM, Feng Kan <fkan@apm.com> wrote:

> Marc:
>
> Sorry for top posting. It seems with GIC-400, the get_irqstate is not a reliable
> way of accessing the GPIO status. In our case, the gpio interrupt status can
> only be read correctly with the SPISR register. So it seems we have to go
> back to the old way of mapping the SPISR portion of the GIC in our GPIO
> driver. We wanted to ask you if that is acceptable since SPISR in not
> standard in all GIC, perhaps we can treat it differently.

As GPIO maintainer I am curious about what this is...

Can you describe the scenario where your GPIO (!) driver needs
to be poking around in the GIC SPISR?

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

On Wed, May 13, 2015 at 4:58 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 13, 2015 at 4:25 AM, Feng Kan <fkan@apm.com> wrote:
>
>> Marc:
>>
>> Sorry for top posting. It seems with GIC-400, the get_irqstate is not a reliable
>> way of accessing the GPIO status. In our case, the gpio interrupt status can
>> only be read correctly with the SPISR register. So it seems we have to go
>> back to the old way of mapping the SPISR portion of the GIC in our GPIO
>> driver. We wanted to ask you if that is acceptable since SPISR in not
>> standard in all GIC, perhaps we can treat it differently.
>
> As GPIO maintainer I am curious about what this is...
>
> Can you describe the scenario where your GPIO (!) driver needs
> to be poking around in the GIC SPISR?

We are using the gpio driver with gpio_key for power button. The gpio
status can only be retrieved from the GIC register when the gpio is
acting as a interrupt. Marc had provided a patch which allow the
getting of the irq status
using the PENDING_SET register, however it seems it is not reliable.
Another register in the GIC call the SPISR always give the correct
status but it is only available in the GIC-400 and hence not likely to
be incorporated into the general GIC code. We would like to go back
the version 1 of the patch and ioremap the region in the gpio driver
and access the status that way. Please let us know what you think.

>
> Yours,
> Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Wed, May 13, 2015 at 5:44 PM, Feng Kan <fkan@apm.com> wrote:

> We are using the gpio driver with gpio_key for power button. The gpio
> status can only be retrieved from the GIC register when the gpio is
> acting as a interrupt.

OK I understand so much. SPISR is a status register of the state
of the IRQ lines of shared peripherals.

But surely the GPIO block has its own status register, so are
you saying that this register is unreliable?

I can think of a few reasons, like transient IRQs etc but
what is actually causing this?

Techically the GIC would normally be higher up the food
chain, i.e. one IRQ on the GIC is cascaded to sub-IRQs on
the GPIO chip by virtue of its own struct irq_chip and
irqdomain. The exception is typically only systems where
the GPIO block is fused with the IRQ controller so that each
GPIO line has its own unique IRQ line on the primary,
top-level interrupt controller.

Which GPIO driver is this? Is it upstream?

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 13, 2015 at 5:44 PM, Feng Kan <fkan@apm.com> wrote:
>
>> We are using the gpio driver with gpio_key for power button. The gpio
>> status can only be retrieved from the GIC register when the gpio is
>> acting as a interrupt.
>
> OK I understand so much. SPISR is a status register of the state
> of the IRQ lines of shared peripherals.
>
> But surely the GPIO block has its own status register, so are
> you saying that this register is unreliable?

When the GPIO is used as interrupt, the gpio block does not report the
status anymore. Which leaves us stuck with SPISR.
>
> I can think of a few reasons, like transient IRQs etc but
> what is actually causing this?

I won't say the obvious.
>
> Techically the GIC would normally be higher up the food
> chain, i.e. one IRQ on the GIC is cascaded to sub-IRQs on
> the GPIO chip by virtue of its own struct irq_chip and
> irqdomain. The exception is typically only systems where
> the GPIO block is fused with the IRQ controller so that each
> GPIO line has its own unique IRQ line on the primary,
> top-level interrupt controller.
>
> Which GPIO driver is this? Is it upstream?
Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to
think that we ought to switch to use some gpio poll driver rather than
using gpio-key.
>
> Yours,
> Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
> On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> But surely the GPIO block has its own status register, so are
>> you saying that this register is unreliable?
>
> When the GPIO is used as interrupt, the gpio block does not report the
> status anymore. Which leaves us stuck with SPISR.
>>
>> I can think of a few reasons, like transient IRQs etc but
>> what is actually causing this?
>
> I won't say the obvious.

Yeah I see your problem now :(

I think it's better to fix the access functions so that you can
cross-call to the GIC driver to get the SPISR flag out though.
Let's see what Marc says.

>> Which GPIO driver is this? Is it upstream?
>
> Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to
> think that we ought to switch to use some gpio poll driver rather than
> using gpio-key.

There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah
that's possible. But honestly I think it's better to deal with this
problem for real because IRQ is more efficient.

So the way I perceive it this is the real problem:

+static int gic_irq_get_irqchip_state(struct irq_data *d,
+                                     enum irqchip_irq_state which, bool *val)
+{
+       switch (which) {
(...)
+       case IRQCHIP_STATE_ACTIVE:
+               *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
+               break;

Here it reads the status from 0x300 where DIST_ACTIVE_SET
is, so if you change GIC_DIST_ACTIVE_SET to
0xd04 (SPISR), does it start working?

I would *guess* that maybe you want to implement and
ask for IRQCHIP_STATE_LINE_LEVEL instead of
IRQCHIP_STATE_ACTIVE, and have that case: read
from 0xd04 (SPISR) instead, because that makes more
sense to me, or am I wrong at it?

+       case IRQCHIP_STATE_LINE_LEVEL:
+               *val = gic_peek_irq(d, GIC_DIST_SPISR);
+               break;

And then put a define into <linux/irqchip/arm-gic.h> for
GIC_DIST_SPISR.

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

On Tue, 19 May 2015 09:40:21 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> >> But surely the GPIO block has its own status register, so are
> >> you saying that this register is unreliable?
> >
> > When the GPIO is used as interrupt, the gpio block does not report the
> > status anymore. Which leaves us stuck with SPISR.
> >>
> >> I can think of a few reasons, like transient IRQs etc but
> >> what is actually causing this?
> >
> > I won't say the obvious.
> 
> Yeah I see your problem now :(
> 
> I think it's better to fix the access functions so that you can
> cross-call to the GIC driver to get the SPISR flag out though.
> Let's see what Marc says.
> 
> >> Which GPIO driver is this? Is it upstream?
> >
> > Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to
> > think that we ought to switch to use some gpio poll driver rather than
> > using gpio-key.
> 
> There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah
> that's possible. But honestly I think it's better to deal with this
> problem for real because IRQ is more efficient.
> 
> So the way I perceive it this is the real problem:
> 
> +static int gic_irq_get_irqchip_state(struct irq_data *d,
> +                                     enum irqchip_irq_state which, bool *val)
> +{
> +       switch (which) {
> (...)
> +       case IRQCHIP_STATE_ACTIVE:
> +               *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
> +               break; case: read
> from 0xd04 (SPISR) instead, because that makes more
> sense to me, or am I wrong at it?
> 
> +       case IRQCHIP_STATE_LINE_LEVEL:
> +               *val = gic_peek_irq(d, GIC_DIST_SPISR);
> +               break;
> 
> And then put a define into <linux/irqchip/arm-gic.h> for
> GIC_DIST_SPISR.

What worries me here is that the PENDING state should already give you
the right level of information (this is what the GIC-400 TRM says). The
only reason why SPISR exists is that software can write to the PENDING
register, while SPISR is RO.

If reading the pending state doesn't work, then I'd like to know
exactly *why*. Only then we can add support for LINE_LEVEL using SPISR
(which has to be GIC-400 specific, as it is not architected).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Linus Walleij]

On Tue, May 19, 2015 at 12:01 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:

> If reading the pending state doesn't work, then I'd like to know
> exactly *why*. Only then we can add support for LINE_LEVEL using SPISR
> (which has to be GIC-400 specific, as it is not architected).

That's a good point. Is it one of these usual cases of hardware
thrown over the wall and the HW engineer mostly yawning or
rolling his eyes if we ask things like this?

Or is there someone who can actually answer the call?

Yours,
Linus Walleij

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Feng Kan]

On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Tue, 19 May 2015 09:40:21 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
>> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> >> But surely the GPIO block has its own status register, so are
>> >> you saying that this register is unreliable?
>> >
>> > When the GPIO is used as interrupt, the gpio block does not report the
>> > status anymore. Which leaves us stuck with SPISR.
>> >>
>> >> I can think of a few reasons, like transient IRQs etc but
>> >> what is actually causing this?
>> >
>> > I won't say the obvious.
>>
>> Yeah I see your problem now :(
>>
>> I think it's better to fix the access functions so that you can
>> cross-call to the GIC driver to get the SPISR flag out though.
>> Let's see what Marc says.
>>
>> >> Which GPIO driver is this? Is it upstream?
>> >
>> > Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to
>> > think that we ought to switch to use some gpio poll driver rather than
>> > using gpio-key.
>>
>> There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah
>> that's possible. But honestly I think it's better to deal with this
>> problem for real because IRQ is more efficient.
>>
>> So the way I perceive it this is the real problem:
>>
>> +static int gic_irq_get_irqchip_state(struct irq_data *d,
>> +                                     enum irqchip_irq_state which, bool *val)
>> +{
>> +       switch (which) {
>> (...)
>> +       case IRQCHIP_STATE_ACTIVE:
>> +               *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
>> +               break; case: read
>> from 0xd04 (SPISR) instead, because that makes more
>> sense to me, or am I wrong at it?
>>
>> +       case IRQCHIP_STATE_LINE_LEVEL:
>> +               *val = gic_peek_irq(d, GIC_DIST_SPISR);
>> +               break;
>>
>> And then put a define into <linux/irqchip/arm-gic.h> for
>> GIC_DIST_SPISR.
>
> What worries me here is that the PENDING state should already give you
> the right level of information (this is what the GIC-400 TRM says). The
> only reason why SPISR exists is that software can write to the PENDING
> register, while SPISR is RO.
>
> If reading the pending state doesn't work, then I'd like to know
> exactly *why*. Only then we can add support for LINE_LEVEL using SPISR
> (which has to be GIC-400 specific, as it is not architected).

IS_PENDING and IS_ACTIVE works fine for the ISR context. However,
the nature of the register is meant for IRQ handling and not to read
the status of a GPIO. By the time the gpio_key throws a work queue
and check the status of the PENDING register, it is no long relevant.



>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.

Re: [PATCH v4 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

On Tue, 19 May 2015 22:45:26 +0100
Feng Kan <fkan@apm.com> wrote:

> On Tue, May 19, 2015 at 3:01 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On Tue, 19 May 2015 09:40:21 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> >> On Thu, May 14, 2015 at 10:14 PM, Feng Kan <fkan@apm.com> wrote:
> >> > On Thu, May 14, 2015 at 3:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>
> >> >> But surely the GPIO block has its own status register, so are
> >> >> you saying that this register is unreliable?
> >> >
> >> > When the GPIO is used as interrupt, the gpio block does not report the
> >> > status anymore. Which leaves us stuck with SPISR.
> >> >>
> >> >> I can think of a few reasons, like transient IRQs etc but
> >> >> what is actually causing this?
> >> >
> >> > I won't say the obvious.
> >>
> >> Yeah I see your problem now :(
> >>
> >> I think it's better to fix the access functions so that you can
> >> cross-call to the GIC driver to get the SPISR flag out though.
> >> Let's see what Marc says.
> >>
> >> >> Which GPIO driver is this? Is it upstream?
> >> >
> >> > Yes, it is upstream. It is the xgene slimpro gpio driver. I am starting to
> >> > think that we ought to switch to use some gpio poll driver rather than
> >> > using gpio-key.
> >>
> >> There is both gpio_keys_polled and IRQ-driven gpio_keys so yeah
> >> that's possible. But honestly I think it's better to deal with this
> >> problem for real because IRQ is more efficient.
> >>
> >> So the way I perceive it this is the real problem:
> >>
> >> +static int gic_irq_get_irqchip_state(struct irq_data *d,
> >> +                                     enum irqchip_irq_state which, bool *val)
> >> +{
> >> +       switch (which) {
> >> (...)
> >> +       case IRQCHIP_STATE_ACTIVE:
> >> +               *val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
> >> +               break; case: read
> >> from 0xd04 (SPISR) instead, because that makes more
> >> sense to me, or am I wrong at it?
> >>
> >> +       case IRQCHIP_STATE_LINE_LEVEL:
> >> +               *val = gic_peek_irq(d, GIC_DIST_SPISR);
> >> +               break;
> >>
> >> And then put a define into <linux/irqchip/arm-gic.h> for
> >> GIC_DIST_SPISR.
> >
> > What worries me here is that the PENDING state should already give you
> > the right level of information (this is what the GIC-400 TRM says). The
> > only reason why SPISR exists is that software can write to the PENDING
> > register, while SPISR is RO.
> >
> > If reading the pending state doesn't work, then I'd like to know
> > exactly *why*. Only then we can add support for LINE_LEVEL using SPISR
> > (which has to be GIC-400 specific, as it is not architected).
> 
> IS_PENDING and IS_ACTIVE works fine for the ISR context. However,
> the nature of the register is meant for IRQ handling and not to read
> the status of a GPIO. By the time the gpio_key throws a work queue
> and check the status of the PENDING register, it is no long relevant.

This is far more subtle that that. Assuming a level-triggered
interrupt, the PENDING state is completely independent of the state of
the interrupt (see the GIC state machine), and will accurately reflect
the input line (an edge-triggered interrupt is of course a different
matter). As outlined in the GIC-400 TRM, both GICD_ICPENDINGRn,
GICD_ISPENDINGRn and GICD_SPISRn are providing the same information
(the only difference being that SPISRn is offering a RO view before the
latches).

SPISRn is essentially a debug register, not something that is meant to
be used in normal code, GPIO or otherwise. So assuming we have an
actual GIC-400 implementation here, I want to know why this isn't
working, and you are in a position to tell me (I assume you can talk to
the HW people that did the integration).

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

[PATCH v4 3/3] irqchip: GICv3: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

Add the required hooks for the internal state of an interrupt
to be exposed to other subsystems.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 83 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 13 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index fd8850d..4f2fb62 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -195,6 +195,19 @@ static void gic_enable_redist(bool enable)
 /*
  * Routines to disable, enable, EOI and route interrupts
  */
+static int gic_peek_irq(struct irq_data *d, u32 offset)
+{
+	u32 mask = 1 << (gic_irq(d) % 32);
+	void __iomem *base;
+
+	if (gic_irq_in_rdist(d))
+		base = gic_data_rdist_sgi_base();
+	else
+		base = gic_data.dist_base;
+
+	return !!(readl_relaxed(base + offset + (gic_irq(d) / 32) * 4) & mask);
+}
+
 static void gic_poke_irq(struct irq_data *d, u32 offset)
 {
 	u32 mask = 1 << (gic_irq(d) % 32);
@@ -223,6 +236,61 @@ static void gic_unmask_irq(struct irq_data *d)
 	gic_poke_irq(d, GICD_ISENABLER);
 }
 
+static int gic_irq_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool val)
+{
+	u32 reg;
+
+	if (d->hwirq >= gic_data.irq_nr) /* PPI/SPI only */
+		return -EINVAL;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		reg = val ? GICD_ISPENDR : GICD_ICPENDR;
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		reg = val ? GICD_ISACTIVER : GICD_ICACTIVER;
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		reg = val ? GICD_ICENABLER : GICD_ISENABLER;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	gic_poke_irq(d, reg);
+	return 0;
+}
+
+static int gic_irq_get_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool *val)
+{
+	if (d->hwirq >= gic_data.irq_nr) /* PPI/SPI only */
+		return -EINVAL;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		*val = gic_peek_irq(d, GICD_ISPENDR);
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		*val = gic_peek_irq(d, GICD_ISACTIVER);
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		*val = !gic_peek_irq(d, GICD_ISENABLER);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void gic_eoi_irq(struct irq_data *d)
 {
 	gic_write_eoir(gic_irq(d));
@@ -418,19 +486,6 @@ static void gic_cpu_init(void)
 }
 
 #ifdef CONFIG_SMP
-static int gic_peek_irq(struct irq_data *d, u32 offset)
-{
-	u32 mask = 1 << (gic_irq(d) % 32);
-	void __iomem *base;
-
-	if (gic_irq_in_rdist(d))
-		base = gic_data_rdist_sgi_base();
-	else
-		base = gic_data.dist_base;
-
-	return !!(readl_relaxed(base + offset + (gic_irq(d) / 32) * 4) & mask);
-}
-
 static int gic_secondary_init(struct notifier_block *nfb,
 			      unsigned long action, void *hcpu)
 {
@@ -601,6 +656,8 @@ static struct irq_chip gic_chip = {
 	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
 	.irq_set_affinity	= gic_set_affinity,
+	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 };
 
 #define GIC_ID_NR		(1U << gic_data.rdists.id_bits)
-- 
2.1.4

[tip:irq/core] irqchip: GICv3: Add support for irq_[get, set] _irqchip_state()

[tip-bot for Marc Zyngier]

Commit-ID:  b594c6e20c7ff65e0f0775cb1866e97501c96846
Gitweb:     http://git.kernel.org/tip/b594c6e20c7ff65e0f0775cb1866e97501c96846
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Wed, 18 Mar 2015 11:01:24 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 8 Apr 2015 23:28:28 +0200

irqchip: GICv3: Add support for irq_[get, set]_irqchip_state()

Add the required hooks for the internal state of an interrupt
to be exposed to other subsystems.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Phong Vo <pvo@apm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Tin Huynh <tnhuynh@apm.com>
Cc: Y Vo <yvo@apm.com>
Cc: Toan Le <toanle@apm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Arnd Bergmann <arnd@arndb.de>
Link: http://lkml.kernel.org/r/1426676484-21812-4-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/irq-gic-v3.c | 83 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 13 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index fd8850d..4f2fb62 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -195,6 +195,19 @@ static void gic_enable_redist(bool enable)
 /*
  * Routines to disable, enable, EOI and route interrupts
  */
+static int gic_peek_irq(struct irq_data *d, u32 offset)
+{
+	u32 mask = 1 << (gic_irq(d) % 32);
+	void __iomem *base;
+
+	if (gic_irq_in_rdist(d))
+		base = gic_data_rdist_sgi_base();
+	else
+		base = gic_data.dist_base;
+
+	return !!(readl_relaxed(base + offset + (gic_irq(d) / 32) * 4) & mask);
+}
+
 static void gic_poke_irq(struct irq_data *d, u32 offset)
 {
 	u32 mask = 1 << (gic_irq(d) % 32);
@@ -223,6 +236,61 @@ static void gic_unmask_irq(struct irq_data *d)
 	gic_poke_irq(d, GICD_ISENABLER);
 }
 
+static int gic_irq_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool val)
+{
+	u32 reg;
+
+	if (d->hwirq >= gic_data.irq_nr) /* PPI/SPI only */
+		return -EINVAL;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		reg = val ? GICD_ISPENDR : GICD_ICPENDR;
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		reg = val ? GICD_ISACTIVER : GICD_ICACTIVER;
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		reg = val ? GICD_ICENABLER : GICD_ISENABLER;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	gic_poke_irq(d, reg);
+	return 0;
+}
+
+static int gic_irq_get_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool *val)
+{
+	if (d->hwirq >= gic_data.irq_nr) /* PPI/SPI only */
+		return -EINVAL;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		*val = gic_peek_irq(d, GICD_ISPENDR);
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		*val = gic_peek_irq(d, GICD_ISACTIVER);
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		*val = !gic_peek_irq(d, GICD_ISENABLER);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void gic_eoi_irq(struct irq_data *d)
 {
 	gic_write_eoir(gic_irq(d));
@@ -418,19 +486,6 @@ static void gic_cpu_init(void)
 }
 
 #ifdef CONFIG_SMP
-static int gic_peek_irq(struct irq_data *d, u32 offset)
-{
-	u32 mask = 1 << (gic_irq(d) % 32);
-	void __iomem *base;
-
-	if (gic_irq_in_rdist(d))
-		base = gic_data_rdist_sgi_base();
-	else
-		base = gic_data.dist_base;
-
-	return !!(readl_relaxed(base + offset + (gic_irq(d) / 32) * 4) & mask);
-}
-
 static int gic_secondary_init(struct notifier_block *nfb,
 			      unsigned long action, void *hcpu)
 {
@@ -601,6 +656,8 @@ static struct irq_chip gic_chip = {
 	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
 	.irq_set_affinity	= gic_set_affinity,
+	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 };
 
 #define GIC_ID_NR		(1U << gic_data.rdists.id_bits)

Re: [PATCH v4 0/3] genirq: Saving/restoring the irqchip state of an irq line

[Eric Auger]

Hi Marc,

You can apply my R-b and T-b on the whole series. To be precise, I
tested the get path only with my kvm-vfio forward series.

Best Regards

Eric

Reviewed-by: Eric Auger <eric.auger@linaro.org>
Tested-by: Eric Auger <eric.auger@linaro.org>


On 03/18/2015 12:01 PM, Marc Zyngier wrote:
> Despite Linux offering a rather fine grained control over the life
> cycle of an interrupt, there is a few cases where it would be very
> useful to snapshot (or even set) the internal state of the interrupt
> controller for a given interrupt line:
> 
> - With KVM, a device shared between VMs must have its whole context
>   switched, and that includes the interrupt line state. KVM/arm is
>   moving to using this.
> - Some GPIO controllers seem to require peeking into the interrupt
>   controller they are connected to to report their internal state.
> 
> Instead of letting people facing this situation doing horrible
> (controller specific) hacks in their code, let's offer a couple of new
> entry points that allow a few attributes to be read and set.
> 
> Of course, this is a very dangerous thing to do if you don't know what
> you doing, and I wouldn't expect most drivers to use this. But this
> can also be a life saver at times.
> 
> This patch series implement said API, and adds support for this to the
> two main ARM interrupt controllers (GIC and GICv3). Available at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/irqchip_state
> 
> This has been fairly heavily tested with KVM on ARM, which makes use
> of this feature to context switch the timer interrupt (see the last
> three patch in the above branch).
> 
> Thanks,
> 
> 	M.
> 
> * From v3 [3]
> - Rebased on top of 4.0-rc4
> 
> * From v2 [2]:
> - Rebased on top of 3.19-rc3
> - Fixed irq_set_irqchip_state return value
> 
> * From v1 [1]:
> - Rebased on top of 3.18-rc7 + tip/irq/irqdomain-arm
> - Now deals with stacked domains
> - Changed the API to be less ambiguous (after review by tglx)
> - Added IRQCHIP_STATE_LINE_LEVEL (as requested by Bjorn Anderson)
> 
> [1] https://lkml.org/lkml/2014/10/25/134
> [2] https://lkml.org/lkml/2014/12/3/612
> [3] http://lkml.iu.edu/hypermail/linux/kernel/1501.0/03473.html
> 
> Marc Zyngier (3):
>   genirq: Allow the irqchip state of an IRQ to be save/restored
>   irqchip: GIC: Add support for irq_{get,set}_irqchip_state
>   irqchip: GICv3: Add support for irq_{get,set}_irqchip_state
> 
>  drivers/irqchip/irq-gic-v3.c | 83 +++++++++++++++++++++++++++++++++-------
>  drivers/irqchip/irq-gic.c    | 69 +++++++++++++++++++++++++++++++--
>  include/linux/interrupt.h    | 14 +++++++
>  include/linux/irq.h          |  6 +++
>  kernel/irq/manage.c          | 91 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 246 insertions(+), 17 deletions(-)
>