[PATCH] genirq: Introduce irq_read_line()

[PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

Introduce the irq_read_line() function to allow device drivers to read
the current logical state of an input when the hardware only exposes
this through status bits in the interrupt controller.

The new function is backed by a new callback function in the irq_chip -
irq_read_line() - that can be implemented by irq_chips that owns such
status bits.

Based on rfc patch from April 2011 by Abhijeet.

Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

Due to address size limitations in the SSBI protocol used to communicate with
various Qualcomm PMICs most accesses are banked. Furthermore the input/status
bits for the adc, battery monitor, charger, gpio, mpp and usb blocks are all
banked as interrupt status bits.

Therefor to be able to implement any of those drivers we need to make a call
into the pm8xxx irq_chip code to acquire the state of those bits - as my first
hack [1] did.


The patch is based on Abhijeet's RFC found here [2], but my reasons for having
this accessor function is different.

[1] https://lkml.org/lkml/2014/7/7/892
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048277.html

 include/linux/interrupt.h |    1 +
 include/linux/irq.h       |    2 ++
 kernel/irq/manage.c       |   25 +++++++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 698ad05..8f3af5d 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -361,6 +361,7 @@ static inline int disable_irq_wake(unsigned int irq)
 	return irq_set_irq_wake(irq, 0);
 }
 
+extern int irq_read_line(unsigned int irq);
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 extern bool force_irqthreads;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 0d998d8..f656590 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -294,6 +294,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @irq_retrigger:	resend an IRQ to the CPU
  * @irq_set_type:	set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
  * @irq_set_wake:	enable/disable power-management wake-on of an IRQ
+ * @irq_read_line:	read the logical state of an irq line
  * @irq_bus_lock:	function to lock access to slow bus (i2c) chips
  * @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips
  * @irq_cpu_online:	configure an interrupt source for a secondary CPU
@@ -326,6 +327,7 @@ struct irq_chip {
 	int		(*irq_retrigger)(struct irq_data *data);
 	int		(*irq_set_type)(struct irq_data *data, unsigned int flow_type);
 	int		(*irq_set_wake)(struct irq_data *data, unsigned int on);
+	int		(*irq_read_line)(struct irq_data *data);
 
 	void		(*irq_bus_lock)(struct irq_data *data);
 	void		(*irq_bus_sync_unlock)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..33bf9c7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -565,6 +565,31 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
 }
 EXPORT_SYMBOL(irq_set_irq_wake);
 
+/**
+ *	irq_read_line - read the logical state of an irq line
+ *	@irq:	interrupt to read
+ *
+ *	This function may be called from IRQ context only when
+ *	desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
+ */
+int irq_read_line(unsigned int irq)
+{
+	struct irq_desc *desc;
+	unsigned long flags;
+	int ret = -ENOSYS;
+
+	desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+	if (!desc)
+		return -EINVAL;
+
+	if (desc->irq_data.chip->irq_read_line)
+		ret = desc->irq_data.chip->irq_read_line(&desc->irq_data);
+
+	irq_put_desc_busunlock(desc, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(irq_read_line);
+
 /*
  * Internal function that tells the architecture code whether a
  * particular irq has been exclusively allocated or is available
-- 
1.7.9.5

[PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> Introduce the irq_read_line() function to allow device drivers to read
> the current logical state of an input when the hardware only exposes
> this through status bits in the interrupt controller.
>
> The new function is backed by a new callback function in the irq_chip -
> irq_read_line() - that can be implemented by irq_chips that owns such
> status bits.
>
> Based on rfc patch from April 2011 by Abhijeet.
>
> Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

ping?

[PATCH] genirq: Introduce irq_read_line()

[Thomas Gleixner]

On Thu, 4 Sep 2014, Bjorn Andersson wrote:

> On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > Introduce the irq_read_line() function to allow device drivers to read
> > the current logical state of an input when the hardware only exposes
> > this through status bits in the interrupt controller.
> >
> > The new function is backed by a new callback function in the irq_chip -
> > irq_read_line() - that can be implemented by irq_chips that owns such
> > status bits.
> >
> > Based on rfc patch from April 2011 by Abhijeet.
> >
> > Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> 
> ping?

Sorry, slipped through the cracks. I was talking about this to Marc
last week and he needs it for yet another reason. He had some thoughts
about the state representation, so I wait for him to comment.

Thanks,

	tglx

[PATCH] genirq: Introduce irq_read_line()

[Arnd Bergmann]

On Tuesday 21 October 2014 11:22:40 Thomas Gleixner wrote:
> On Thu, 4 Sep 2014, Bjorn Andersson wrote:
> 
> > On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
> > <bjorn.andersson@sonymobile.com> wrote:
> > > Introduce the irq_read_line() function to allow device drivers to read
> > > the current logical state of an input when the hardware only exposes
> > > this through status bits in the interrupt controller.
> > >
> > > The new function is backed by a new callback function in the irq_chip -
> > > irq_read_line() - that can be implemented by irq_chips that owns such
> > > status bits.
> > >
> > > Based on rfc patch from April 2011 by Abhijeet.
> > >
> > > Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > 
> > ping?
> 
> Sorry, slipped through the cracks. I was talking about this to Marc
> last week and he needs it for yet another reason. He had some thoughts
> about the state representation, so I wait for him to comment.

I looked up my older email as well and found which gpio driver had the
requirement to get the irq status from the upstream controller. It was
"gpio: Add APM X-Gene standby GPIO controller driver". I'm adding the
APM developers on Cc here so they can participate in the discussion.

As a brief summary: APM have a GPIO driver that needs to read the status
from the ARM GIC in order to implement gpio_get_value. The KVM hypervisor
needs multiple bits from the GIC in order to implement suspend/resume,
and Qualcomm MSM (as Bjorn wrote above) also needs to expose the status
of the IRQ line like APM does.

These should all use the same interface for the interrupt controller,
but the interface still needs to be added.

	Arnd

[PATCH] genirq: Introduce irq_read_line()

[Feng Kan]

On Tue, Oct 21, 2014 at 2:30 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 21 October 2014 11:22:40 Thomas Gleixner wrote:
>> On Thu, 4 Sep 2014, Bjorn Andersson wrote:
>>
>> > On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
>> > <bjorn.andersson@sonymobile.com> wrote:
>> > > Introduce the irq_read_line() function to allow device drivers to read
>> > > the current logical state of an input when the hardware only exposes
>> > > this through status bits in the interrupt controller.
>> > >
>> > > The new function is backed by a new callback function in the irq_chip -
>> > > irq_read_line() - that can be implemented by irq_chips that owns such
>> > > status bits.
>> > >
>> > > Based on rfc patch from April 2011 by Abhijeet.
>> > >
>> > > Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> >
>> > ping?
>>
>> Sorry, slipped through the cracks. I was talking about this to Marc
>> last week and he needs it for yet another reason. He had some thoughts
>> about the state representation, so I wait for him to comment.
>
> I looked up my older email as well and found which gpio driver had the
> requirement to get the irq status from the upstream controller. It was
> "gpio: Add APM X-Gene standby GPIO controller driver". I'm adding the
> APM developers on Cc here so they can participate in the discussion.
>
> As a brief summary: APM have a GPIO driver that needs to read the status
> from the ARM GIC in order to implement gpio_get_value.

Thanks, we will rebase our standby gpio patch on top of this change.

 The KVM hypervisor
> needs multiple bits from the GIC in order to implement suspend/resume,
> and Qualcomm MSM (as Bjorn wrote above) also needs to expose the status
> of the IRQ line like APM does.
>
> These should all use the same interface for the interrupt controller,
> but the interface still needs to be added.
>
>         Arnd
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
> is for the sole use of the intended recipient(s) and contains information
> that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries.
> It is to be used solely for the purpose of furthering the parties' business relationship.
> All unauthorized review, use, disclosure or distribution is prohibited.
> If you are not the intended recipient, please contact the sender by reply e-mail
> and destroy all copies of the original message.
>

[PATCH] genirq: Introduce irq_read_line()

[Marc Zyngier]

Hi all,

On 21/10/14 10:22, Thomas Gleixner wrote:
> On Thu, 4 Sep 2014, Bjorn Andersson wrote:
> 
>> On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
>> <bjorn.andersson@sonymobile.com> wrote:
>>> Introduce the irq_read_line() function to allow device drivers to read
>>> the current logical state of an input when the hardware only exposes
>>> this through status bits in the interrupt controller.
>>>
>>> The new function is backed by a new callback function in the irq_chip -
>>> irq_read_line() - that can be implemented by irq_chips that owns such
>>> status bits.
>>>
>>> Based on rfc patch from April 2011 by Abhijeet.
>>>
>>> Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>
>> ping?
> 
> Sorry, slipped through the cracks. I was talking about this to Marc
> last week and he needs it for yet another reason. He had some thoughts
> about the state representation, so I wait for him to comment.

Thanks for putting me in the loop. For the record, here's the RFC I
posted back in June:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266328.html

and the patch implementing a similar concept:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266331.html

Basic idea is that you can read (and possibly write back) various
low-level attributes (pending, masked, active) that an interrupt
controller may implement. Given your use case, we should loose the
checks on the interrupt being forwarded, as this makes little sense
outside of virtualization.

I'm planning to respin the series this week, as I have a number of
changes (there is hardly any need for the various states to be reported
atomically, for example), and a number of bugs have been found.

Thanks,

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

[PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

On Tue, Oct 21, 2014 at 2:34 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi all,
>
> On 21/10/14 10:22, Thomas Gleixner wrote:
>> On Thu, 4 Sep 2014, Bjorn Andersson wrote:
>>
>>> On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
>>> <bjorn.andersson@sonymobile.com> wrote:
>>>> Introduce the irq_read_line() function to allow device drivers to read
>>>> the current logical state of an input when the hardware only exposes
>>>> this through status bits in the interrupt controller.
>>>>
>>>> The new function is backed by a new callback function in the irq_chip -
>>>> irq_read_line() - that can be implemented by irq_chips that owns such
>>>> status bits.
>>>>
>>>> Based on rfc patch from April 2011 by Abhijeet.
>>>>
>>>> Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>>
>>> ping?
>>
>> Sorry, slipped through the cracks. I was talking about this to Marc
>> last week and he needs it for yet another reason. He had some thoughts
>> about the state representation, so I wait for him to comment.
>
> Thanks for putting me in the loop. For the record, here's the RFC I
> posted back in June:
>

Thanks for having a similar problem as me :)

> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266328.html
>
> and the patch implementing a similar concept:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266331.html
>

I would prefer to see that you had explicit functions for the various things
that you would like to set and get. It would in my eyes give cleaner client
code, at the cost of a few extra functions.

> Basic idea is that you can read (and possibly write back) various
> low-level attributes (pending, masked, active) that an interrupt
> controller may implement. Given your use case, we should loose the
> checks on the interrupt being forwarded, as this makes little sense
> outside of virtualization.
>

Relaxing the forwarding requirement seems to solve my use cases. May I ask why
your accessors would only be made available for forwarded IRQs - at a framework
level?

Stephen Boyd talked about the need to be able to mask/unmask interrupts from
client code in the Qualcomm platform as well - most likely to block wakeup
sources(?)

> I'm planning to respin the series this week, as I have a number of
> changes (there is hardly any need for the various states to be reported
> atomically, for example), and a number of bugs have been found.
>

Sounds good, for clarity I would like them to be explicit separate functions.
But as long as it's not limited to forwarded IRQs we should be able to use it.

Regards,
Bjorn

[PATCH] genirq: Introduce irq_read_line()

[Marc Zyngier]

Hi Bjorn,

On 24/10/14 18:31, Bjorn Andersson wrote:
> On Tue, Oct 21, 2014 at 2:34 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> Hi all,
>>
>> On 21/10/14 10:22, Thomas Gleixner wrote:
>>> On Thu, 4 Sep 2014, Bjorn Andersson wrote:
>>>
>>>> On Tue, Aug 19, 2014 at 1:23 PM, Bjorn Andersson
>>>> <bjorn.andersson@sonymobile.com> wrote:
>>>>> Introduce the irq_read_line() function to allow device drivers to read
>>>>> the current logical state of an input when the hardware only exposes
>>>>> this through status bits in the interrupt controller.
>>>>>
>>>>> The new function is backed by a new callback function in the irq_chip -
>>>>> irq_read_line() - that can be implemented by irq_chips that owns such
>>>>> status bits.
>>>>>
>>>>> Based on rfc patch from April 2011 by Abhijeet.
>>>>>
>>>>> Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>>>
>>>> ping?
>>>
>>> Sorry, slipped through the cracks. I was talking about this to Marc
>>> last week and he needs it for yet another reason. He had some thoughts
>>> about the state representation, so I wait for him to comment.
>>
>> Thanks for putting me in the loop. For the record, here's the RFC I
>> posted back in June:
>>
> 
> Thanks for having a similar problem as me :)
> 
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266328.html
>>
>> and the patch implementing a similar concept:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266331.html
>>
> 
> I would prefer to see that you had explicit functions for the various things
> that you would like to set and get. It would in my eyes give cleaner client
> code, at the cost of a few extra functions.

I don't really like the idea of having a gazillion of accessors for each
individual property. I'd rather have an extensible API.

>> Basic idea is that you can read (and possibly write back) various
>> low-level attributes (pending, masked, active) that an interrupt
>> controller may implement. Given your use case, we should loose the
>> checks on the interrupt being forwarded, as this makes little sense
>> outside of virtualization.
>>
> 
> Relaxing the forwarding requirement seems to solve my use cases. May I ask why
> your accessors would only be made available for forwarded IRQs - at a framework
> level?

This was never intended for wider use, and restricting it to forwarded
interrupts was a concious design decision, as I find the idea of a
device driver messing with the internal state of the interrupt
controller positively repulsive... But hey, the more the merrier.

> Stephen Boyd talked about the need to be able to mask/unmask interrupts from
> client code in the Qualcomm platform as well - most likely to block wakeup
> sources(?)

What's wrong with irq_disable?

>> I'm planning to respin the series this week, as I have a number of
>> changes (there is hardly any need for the various states to be reported
>> atomically, for example), and a number of bugs have been found.
>>
> 
> Sounds good, for clarity I would like them to be explicit separate functions.
> But as long as it's not limited to forwarded IRQs we should be able to use it.

I just pushed out a branch:
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
irq/irqchip_state

Please let me know if that's useful for you.

Thanks,

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

[PATCH] genirq: Introduce irq_read_line()

[Stephen Boyd]

On 10/24/2014 10:59 AM, Marc Zyngier wrote:
> Hi Bjorn,
>
> On 24/10/14 18:31, Bjorn Andersson wrote:
>>
>> Stephen Boyd talked about the need to be able to mask/unmask interrupts from
>> client code in the Qualcomm platform as well - most likely to block wakeup
>> sources(?)
> What's wrong with irq_disable?

The problem is irq_disable() is lazy and doesn't actually disable the
interrupt.

This is the scenario. During idle we want to communicate with another
processor in the SoC and tell it to turn off something after we go to
idle. The communication mechanism uses some shared memory and an
interrupt to trigger the other processor to go look at what we told it
to do. When the other processor is done handling the message it will
send us an ack interrupt. We want to temporarily ignore that ack
interrupt because 1) we're going to execute a wfi to trigger a deep idle
and any pending interrupt will abort that instruction and 2) if the
interrupt comes after we execute the wfi it will wake us up out of deep
idle unnecessarily and waste power. There isn't anything in the
communication protocol that says we don't want the interrupt to be sent
and it would probably make things more complicated anyway. The solution
is to mask the interrupt before we send the message, and then execute
the wfi. Once we wake up, we unmask the interrupt and then handle the ack.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] genirq: Introduce irq_read_line()

[Thomas Gleixner]

On Fri, 24 Oct 2014, Stephen Boyd wrote:

> On 10/24/2014 10:59 AM, Marc Zyngier wrote:
> > Hi Bjorn,
> >
> > On 24/10/14 18:31, Bjorn Andersson wrote:
> >>
> >> Stephen Boyd talked about the need to be able to mask/unmask interrupts from
> >> client code in the Qualcomm platform as well - most likely to block wakeup
> >> sources(?)
> > What's wrong with irq_disable?
> 
> The problem is irq_disable() is lazy and doesn't actually disable the
> interrupt.

Nothing prevents you from adding your own irq_disable() callback for
those interrupts. Just the default is lazy.

Thanks,

	tglx

[PATCH] genirq: Introduce irq_read_line()

[Stephen Boyd]

On 10/25/2014 01:34 AM, Thomas Gleixner wrote:
> On Fri, 24 Oct 2014, Stephen Boyd wrote:
>
>> On 10/24/2014 10:59 AM, Marc Zyngier wrote:
>>> Hi Bjorn,
>>>
>>> On 24/10/14 18:31, Bjorn Andersson wrote:
>>>> Stephen Boyd talked about the need to be able to mask/unmask interrupts from
>>>> client code in the Qualcomm platform as well - most likely to block wakeup
>>>> sources(?)
>>> What's wrong with irq_disable?
>> The problem is irq_disable() is lazy and doesn't actually disable the
>> interrupt.
> Nothing prevents you from adding your own irq_disable() callback for
> those interrupts. Just the default is lazy.
>
>

Ok, if we did that it would be global for the entire arm gic right? I see:

void irq_disable(struct irq_desc *desc)
{
        irq_state_set_disabled(desc);
        if (desc->irq_data.chip->irq_disable) {
                desc->irq_data.chip->irq_disable(&desc->irq_data);
                irq_state_set_masked(desc);
        }
}       

so we would need to add some return value to irq_disable() so that we
could tell if this particular interrupt needs to be disabled or not or
we would need to set a different chip for this particular interrupt with
the irq_disable callback set? Plus any scheme would need to be SoC
specific somehow and be setup early when the gic is probed. Maybe we can
encode this information in the DT specifier somehow to indicate that we
want disable_irq() to actually mask the irq? This is all under the
assumption that we can't just force every gic interrupt to mask on disable.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

On Fri, Oct 24, 2014 at 10:59 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Bjorn,
>
> On 24/10/14 18:31, Bjorn Andersson wrote:
>> On Tue, Oct 21, 2014 at 2:34 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
[..]
>> I would prefer to see that you had explicit functions for the various things
>> that you would like to set and get. It would in my eyes give cleaner client
>> code, at the cost of a few extra functions.
>
> I don't really like the idea of having a gazillion of accessors for each
> individual property. I'd rather have an extensible API.
>

Okay, either way will work. Feel free to proceed with your approach.

>>> Basic idea is that you can read (and possibly write back) various
>>> low-level attributes (pending, masked, active) that an interrupt
>>> controller may implement. Given your use case, we should loose the
>>> checks on the interrupt being forwarded, as this makes little sense
>>> outside of virtualization.
>>>
>>
>> Relaxing the forwarding requirement seems to solve my use cases. May I ask why
>> your accessors would only be made available for forwarded IRQs - at a framework
>> level?
>
> This was never intended for wider use, and restricting it to forwarded
> interrupts was a concious design decision, as I find the idea of a
> device driver messing with the internal state of the interrupt
> controller positively repulsive... But hey, the more the merrier.
>

Makes sense and now that it comes without the forwarding part it looks
reasonable.

[..]
>>> I'm planning to respin the series this week, as I have a number of
>>> changes (there is hardly any need for the various states to be reported
>>> atomically, for example), and a number of bugs have been found.
>>>
>>
>> Sounds good, for clarity I would like them to be explicit separate functions.
>> But as long as it's not limited to forwarded IRQs we should be able to use it.
>
> I just pushed out a branch:
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> irq/irqchip_state
>
> Please let me know if that's useful for you.
>

I think that my irq_read_line() would be equivalent of
irq_get_irqchip_state(IRQCHIP_STATE_PENDING), i.e. the state of the
interrupt ignoring masking. And the rest would be EINVAL.
So I think this would work out just fine for us!


Is ACTIVE the line level with the mask considered and setting ACTIVE,
would that be the same as acking the interrupt? What's the expected
behaviour of setting PENDING?

I would appreciate if you commented the state enum, to make it obvious
what pending and active means.

Regards,
Bjorn

[PATCH] genirq: Introduce irq_read_line()

[Marc Zyngier]

On Sat, Oct 25 2014 at 12:12:55 am BST, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Fri, Oct 24, 2014 at 10:59 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> I just pushed out a branch:
>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>> irq/irqchip_state
>>
>> Please let me know if that's useful for you.
>>
>
> I think that my irq_read_line() would be equivalent of
> irq_get_irqchip_state(IRQCHIP_STATE_PENDING), i.e. the state of the
> interrupt ignoring masking. And the rest would be EINVAL.
> So I think this would work out just fine for us!

Excellent.

> Is ACTIVE the line level with the mask considered and setting ACTIVE,
> would that be the same as acking the interrupt?

ACTIVE describes the state that exists once the interrupt has been
ACKed, and before it has been EOIed. It indicates an interrupt "in
progress".

It probably doesn't make any sense for most users, but is extremely
useful with KVM: when a device is shared between VMs (most obvious one
is a simple timer), the interrupt state is part of the whole context,
and must be save/restored as well.

> What's the expected behaviour of setting PENDING?

Its expected behaviour is to inject an interrupt, just as if the device
signaled an interrupt. Probably not easy to implement on anything but
the ARM GICs.

> I would appreciate if you commented the state enum, to make it obvious
> what pending and active means.

Done. I'll probably post this today.

Thanks,

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

[PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

On Sat, Oct 25, 2014 at 2:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Sat, Oct 25 2014 at 12:12:55 am BST, Bjorn Andersson <bjorn@kryo.se> wrote:
>> On Fri, Oct 24, 2014 at 10:59 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> I just pushed out a branch:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>> irq/irqchip_state
>>>
>>> Please let me know if that's useful for you.
>>>
>>
>> I think that my irq_read_line() would be equivalent of
>> irq_get_irqchip_state(IRQCHIP_STATE_PENDING), i.e. the state of the
>> interrupt ignoring masking. And the rest would be EINVAL.
>> So I think this would work out just fine for us!
>
> Excellent.
>

Hi Marc,

We gave this some more thought and read up on what "pending" and
"active" means according to the ARM GIC - PENDING is not what we want
:(

In the Qualcomm pmic we have two interrupt status registers
"interrupt" and "real-time". I think the "interrupt" status register
would be the one related to your defined constants. However what we
need to access in our use cases are the "real-time" status register,
which basically is a representation of the input to the interrupt
logic.

As far as I can see the GIC does not offer anything like that, but I
hope we could add another constant to your enum list and utilise your
api for this.

I'm not entirely sure what we should call it though,
IRQCHIP_STATE_LEVEL seems somewhat conflicting with level trigger and
the Qualcomm name IRQCHIP_STATE_REALTIME isn't very self describing.
Maybe IRQCHIP_STATE_LINE_LEVEL?

Regards,
Bjorn

[PATCH] genirq: Introduce irq_read_line()

[Marc Zyngier]

On 28/10/14 15:41, Bjorn Andersson wrote:
> On Sat, Oct 25, 2014 at 2:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On Sat, Oct 25 2014 at 12:12:55 am BST, Bjorn Andersson <bjorn@kryo.se> wrote:
>>> On Fri, Oct 24, 2014 at 10:59 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> I just pushed out a branch:
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>>> irq/irqchip_state
>>>>
>>>> Please let me know if that's useful for you.
>>>>
>>>
>>> I think that my irq_read_line() would be equivalent of
>>> irq_get_irqchip_state(IRQCHIP_STATE_PENDING), i.e. the state of the
>>> interrupt ignoring masking. And the rest would be EINVAL.
>>> So I think this would work out just fine for us!
>>
>> Excellent.
>>
> 
> Hi Marc,
> 
> We gave this some more thought and read up on what "pending" and
> "active" means according to the ARM GIC - PENDING is not what we want
> :(
> 
> In the Qualcomm pmic we have two interrupt status registers
> "interrupt" and "real-time". I think the "interrupt" status register
> would be the one related to your defined constants. However what we
> need to access in our use cases are the "real-time" status register,
> which basically is a representation of the input to the interrupt
> logic.

Fancy. This really look like a i2c GPIO expander (not my best memories...).

> As far as I can see the GIC does not offer anything like that, but I
> hope we could add another constant to your enum list and utilise your
> api for this.
> 
> I'm not entirely sure what we should call it though,
> IRQCHIP_STATE_LEVEL seems somewhat conflicting with level trigger and
> the Qualcomm name IRQCHIP_STATE_REALTIME isn't very self describing.
> Maybe IRQCHIP_STATE_LINE_LEVEL?

Sure, that should be descriptive enough. I really we don't get too many
of these though...

Do you want me to wrap this into the next version?

Thanks,

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

[PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

On Tue, Oct 28, 2014 at 9:05 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 28/10/14 15:41, Bjorn Andersson wrote:
>> On Sat, Oct 25, 2014 at 2:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
[..]
>> In the Qualcomm pmic we have two interrupt status registers
>> "interrupt" and "real-time". I think the "interrupt" status register
>> would be the one related to your defined constants. However what we
>> need to access in our use cases are the "real-time" status register,
>> which basically is a representation of the input to the interrupt
>> logic.
>
> Fancy. This really look like a i2c GPIO expander (not my best memories...).
>

It really is a kitchen sink, with among other things a set of gpios.

Due to limitations in address space many of the input bits are handled
through banked reads of the interrupt status bits. So that's where we
have access to gpio input, but also things like battery availability,
usb connected, charging status and so on.

>> As far as I can see the GIC does not offer anything like that, but I
>> hope we could add another constant to your enum list and utilise your
>> api for this.
>>
>> I'm not entirely sure what we should call it though,
>> IRQCHIP_STATE_LEVEL seems somewhat conflicting with level trigger and
>> the Qualcomm name IRQCHIP_STATE_REALTIME isn't very self describing.
>> Maybe IRQCHIP_STATE_LINE_LEVEL?
>
> Sure, that should be descriptive enough. I really we don't get too many
> of these though...
>
> Do you want me to wrap this into the next version?
>

Yes please.

Regards,
Bjorn