[PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Liviu Dudau]

During a recent cleanup of the arm64 DTs it has become clear that
the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs
for GICv2 and later allow for "implementation defined" support for
setting the edge or level type of the PPI interrupts and don't restrict
the activation level of the signal. Current ARM implementations
do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees
of the IP can decide to shoot themselves in the foot at any time.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 Documentation/devicetree/bindings/arm/gic.txt |  8 ++++++--
 drivers/irqchip/irq-gic-v3.c                  | 12 ++++++++++++
 drivers/irqchip/irq-gic.c                     | 12 ++++++++++++
 drivers/irqchip/irq-hip04.c                   | 12 ++++++++++++
 4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 8112d0c..c97484b 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -32,12 +32,16 @@ Main node required properties:
   The 3rd cell is the flags, encoded as follows:
 	bits[3:0] trigger type and level flags.
 		1 = low-to-high edge triggered
-		2 = high-to-low edge triggered
+		2 = high-to-low edge triggered (invalid for SPIs)
 		4 = active high level-sensitive
-		8 = active low level-sensitive
+		8 = active low level-sensitive (invalid for SPIs).
 	bits[15:8] PPI interrupt cpu mask.  Each bit corresponds to each of
 	the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
 	the interrupt is wired to that CPU.  Only valid for PPI interrupts.
+	Also note that the configurability of PPI interrupts is IMPLEMENTATION
+	DEFINED and as such not guaranteed to be present (most SoC available
+	in 2014 seem to ignore the setting of this flag and use the hardware
+	default value).
 
 - reg : Specifies base physical address(s) and size of the GIC registers. The
   first region is the GIC distributor register base and size. The 2nd region is
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1a146cc..f529ba5 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -238,6 +238,18 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	if (irq < 16)
 		return -EINVAL;
 
+	/*
+	 * PPIs are optionally configurable, but we cannot distinguish
+	 * between high and low, nor falling and rising. Change the
+	 * type so that it passes the next check.
+	 */
+	if (gicirq < 32) {
+		if (type == IRQ_TYPE_LEVEL_LOW)
+			type = IRQ_TYPE_LEVEL_HIGH;
+		if (type == IRQ_TYPE_EDGE_FALLING)
+			type = IRQ_TYPE_EDGE_RISING;
+	}
+
 	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
 		return -EINVAL;
 
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d617ee5..0d56673 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -193,6 +193,18 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	if (gicirq < 16)
 		return -EINVAL;
 
+	/*
+	 * PPIs are optionally configurable, but we cannot distinguish
+	 * between high and low, nor falling and rising. Change the
+	 * type so that it passes the next check.
+	 */
+	if (gicirq < 32) {
+		if (type == IRQ_TYPE_LEVEL_LOW)
+			type = IRQ_TYPE_LEVEL_HIGH;
+		if (type == IRQ_TYPE_EDGE_FALLING)
+			type = IRQ_TYPE_EDGE_RISING;
+	}
+
 	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
 		return -EINVAL;
 
diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c
index 29b8f21..32fdedf 100644
--- a/drivers/irqchip/irq-hip04.c
+++ b/drivers/irqchip/irq-hip04.c
@@ -125,6 +125,18 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type)
 	if (irq < 16)
 		return -EINVAL;
 
+	/*
+	 * PPIs are optionally configurable, but we cannot distinguish
+	 * between high and low, nor falling and rising. Change the
+	 * type so that it passes the next check.
+	 */
+	if (gicirq < 32) {
+		if (type == IRQ_TYPE_LEVEL_LOW)
+			type = IRQ_TYPE_LEVEL_HIGH;
+		if (type == IRQ_TYPE_EDGE_FALLING)
+			type = IRQ_TYPE_EDGE_RISING;
+	}
+
 	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
 		return -EINVAL;
 
-- 
2.1.3

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Russell King - ARM Linux]

On Fri, Nov 28, 2014 at 05:55:40PM +0000, Liviu Dudau wrote:
> +	/*
> +	 * PPIs are optionally configurable, but we cannot distinguish
> +	 * between high and low, nor falling and rising. Change the
> +	 * type so that it passes the next check.

This comment could do with a /lot/ of improvement.  It sounds like the
only reason this code exists is to bypass the check.  If that's all
that's being done, there's better ways to code it.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Liviu Dudau]

On Mon, Dec 01, 2014 at 10:41:45AM +0000, Russell King - ARM Linux wrote:
> On Fri, Nov 28, 2014 at 05:55:40PM +0000, Liviu Dudau wrote:
> > +	/*
> > +	 * PPIs are optionally configurable, but we cannot distinguish
> > +	 * between high and low, nor falling and rising. Change the
> > +	 * type so that it passes the next check.
> 
> This comment could do with a /lot/ of improvement.  It sounds like the
> only reason this code exists is to bypass the check.  If that's all
> that's being done, there's better ways to code it.

Hi Russell,

You are right, all I want to do is bypass the next check because *if*
the PPIs can be configured, then any combination is valid (edge 
raising/falling, level low/high). In real systems, PPIs tend to be
configured with active level low. That falls the existing check.

If I understood more your thoughts on improving the comment then I can
try to come up with a better version.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Russell King - ARM Linux]

On Mon, Dec 01, 2014 at 10:46:13AM +0000, Liviu Dudau wrote:
> On Mon, Dec 01, 2014 at 10:41:45AM +0000, Russell King - ARM Linux wrote:
> > On Fri, Nov 28, 2014 at 05:55:40PM +0000, Liviu Dudau wrote:
> > > +	/*
> > > +	 * PPIs are optionally configurable, but we cannot distinguish
> > > +	 * between high and low, nor falling and rising. Change the
> > > +	 * type so that it passes the next check.
> > 
> > This comment could do with a /lot/ of improvement.  It sounds like the
> > only reason this code exists is to bypass the check.  If that's all
> > that's being done, there's better ways to code it.
> 
> Hi Russell,
> 
> You are right, all I want to do is bypass the next check because *if*
> the PPIs can be configured, then any combination is valid (edge 
> raising/falling, level low/high). In real systems, PPIs tend to be
> configured with active level low. That falls the existing check.

 "fails" :)

If all you want to do is to bypass the following check, what's wrong
with actually doing that:

-	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
+	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
+	    type != IRQ_TYPE_EDGE_RISING)
		return -EINVAL;

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Marc Zyngier]

Hi Russell,

On 01/12/14 11:03, Russell King - ARM Linux wrote:
> On Mon, Dec 01, 2014 at 10:46:13AM +0000, Liviu Dudau wrote:
>> On Mon, Dec 01, 2014 at 10:41:45AM +0000, Russell King - ARM Linux wrote:
>>> On Fri, Nov 28, 2014 at 05:55:40PM +0000, Liviu Dudau wrote:
>>>> +	/*
>>>> +	 * PPIs are optionally configurable, but we cannot distinguish
>>>> +	 * between high and low, nor falling and rising. Change the
>>>> +	 * type so that it passes the next check.
>>>
>>> This comment could do with a /lot/ of improvement.  It sounds like the
>>> only reason this code exists is to bypass the check.  If that's all
>>> that's being done, there's better ways to code it.
>>
>> Hi Russell,
>>
>> You are right, all I want to do is bypass the next check because *if*
>> the PPIs can be configured, then any combination is valid (edge 
>> raising/falling, level low/high). In real systems, PPIs tend to be
>> configured with active level low. That falls the existing check.
> 
>  "fails" :)
> 
> If all you want to do is to bypass the following check, what's wrong
> with actually doing that:
> 
> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
> +	    type != IRQ_TYPE_EDGE_RISING)
> 		return -EINVAL;
> 

I think that will require some additional changes to gic_configure_irq
(in irq-gic-common.c).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Russell King - ARM Linux]

On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:
> Hi Russell,
> 
> On 01/12/14 11:03, Russell King - ARM Linux wrote:
> > If all you want to do is to bypass the following check, what's wrong
> > with actually doing that:
> > 
> > -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> > +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
> > +	    type != IRQ_TYPE_EDGE_RISING)
> > 		return -EINVAL;
> > 
> 
> I think that will require some additional changes to gic_configure_irq
> (in irq-gic-common.c).

I don't think so - gic_configure_irq() will treat it as a no-op as far
as trying to configure the IRQ settings.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Marc Zyngier]

On 01/12/14 11:23, Russell King - ARM Linux wrote:
> On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:
>> Hi Russell,
>>
>> On 01/12/14 11:03, Russell King - ARM Linux wrote:
>>> If all you want to do is to bypass the following check, what's wrong
>>> with actually doing that:
>>>
>>> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>>> +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
>>> +	    type != IRQ_TYPE_EDGE_RISING)
>>> 		return -EINVAL;
>>>
>>
>> I think that will require some additional changes to gic_configure_irq
>> (in irq-gic-common.c).
> 
> I don't think so - gic_configure_irq() will treat it as a no-op as far
> as trying to configure the IRQ settings.

I agree. But that's following ARM's tradition of making PPIs
non-configurable. I seem to remember that there is at least one
occurrence of a GIC with configurable PPIs (Qualcomm, IIRC).

With this use case in mind, Liviu's patch allows an active-low interrupt
to be correctly configured as level, for example.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Liviu Dudau]

On Mon, Dec 01, 2014 at 11:23:02AM +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:
> > Hi Russell,
> > 
> > On 01/12/14 11:03, Russell King - ARM Linux wrote:
> > > If all you want to do is to bypass the following check, what's wrong
> > > with actually doing that:
> > > 
> > > -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> > > +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
> > > +	    type != IRQ_TYPE_EDGE_RISING)
> > > 		return -EINVAL;
> > > 
> > 
> > I think that will require some additional changes to gic_configure_irq
> > (in irq-gic-common.c).
> 
> I don't think so - gic_configure_irq() will treat it as a no-op as far
> as trying to configure the IRQ settings.

Doesn't that assume then that reset value is correct for the type that
we are trying to program the PPIs?

This is all very academic, as I don't have any real example of a GIC doing
this, but ...

 - if the PPI is set at reset value to be level triggered
 - and we want to set PPIx to be level LOW triggered

with your proposed patch the change will not happen, right?

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Russell King - ARM Linux]

On Mon, Dec 01, 2014 at 11:31:05AM +0000, Marc Zyngier wrote:
> On 01/12/14 11:23, Russell King - ARM Linux wrote:
> > On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:
> >> Hi Russell,
> >>
> >> On 01/12/14 11:03, Russell King - ARM Linux wrote:
> >>> If all you want to do is to bypass the following check, what's wrong
> >>> with actually doing that:
> >>>
> >>> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> >>> +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
> >>> +	    type != IRQ_TYPE_EDGE_RISING)
> >>> 		return -EINVAL;
> >>>
> >>
> >> I think that will require some additional changes to gic_configure_irq
> >> (in irq-gic-common.c).
> > 
> > I don't think so - gic_configure_irq() will treat it as a no-op as far
> > as trying to configure the IRQ settings.
> 
> I agree. But that's following ARM's tradition of making PPIs
> non-configurable. I seem to remember that there is at least one
> occurrence of a GIC with configurable PPIs (Qualcomm, IIRC).
> 
> With this use case in mind, Liviu's patch allows an active-low interrupt
> to be correctly configured as level, for example.

Liviu's patch is nothing more than a hack.  It changes (eg) the active
low level to be an active high level with the explicitly stated reason to
bypass a test, and then hopes that the remaining functions do the right
thing.

It would be better to explicitly bypass the test, and then explicitly
handle other cases in gic_configure_irq().

It would be even better to make the code reflect reality right the way
through.  If PPIs are non-configurable, then we should return -EINVAL if
trying to set them to a setting which is not supported.  For example,
pass through all states to gic_configure_irq(), and have gic_configure_irq()
return whether the state is valid.

Then, gic_configure_irq() can read back the register after trying to set
the appropriate value, and see whether it was taken.  If the bits remain
unchanged, then it can decide that the requested mode is not supported,
and return -EINVAL.

In any case, let's not hack the code in the way that Liviu is trying to
do.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

[Liviu Dudau]

On Mon, Dec 01, 2014 at 11:54:00AM +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 01, 2014 at 11:31:05AM +0000, Marc Zyngier wrote:
> > On 01/12/14 11:23, Russell King - ARM Linux wrote:
> > > On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:
> > >> Hi Russell,
> > >>
> > >> On 01/12/14 11:03, Russell King - ARM Linux wrote:
> > >>> If all you want to do is to bypass the following check, what's wrong
> > >>> with actually doing that:
> > >>>
> > >>> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> > >>> +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
> > >>> +	    type != IRQ_TYPE_EDGE_RISING)
> > >>> 		return -EINVAL;
> > >>>
> > >>
> > >> I think that will require some additional changes to gic_configure_irq
> > >> (in irq-gic-common.c).
> > > 
> > > I don't think so - gic_configure_irq() will treat it as a no-op as far
> > > as trying to configure the IRQ settings.
> > 
> > I agree. But that's following ARM's tradition of making PPIs
> > non-configurable. I seem to remember that there is at least one
> > occurrence of a GIC with configurable PPIs (Qualcomm, IIRC).
> > 
> > With this use case in mind, Liviu's patch allows an active-low interrupt
> > to be correctly configured as level, for example.
> 
> Liviu's patch is nothing more than a hack.  It changes (eg) the active
> low level to be an active high level with the explicitly stated reason to
> bypass a test, and then hopes that the remaining functions do the right
> thing.

I was trying to make the smallest amount of changes possible. Given that
ARM's GICs don't distinguish between high/low or raising/falling all I wanted
was to get the level vs edge type being sticky.

> 
> It would be better to explicitly bypass the test, and then explicitly
> handle other cases in gic_configure_irq().
> 
> It would be even better to make the code reflect reality right the way
> through.  If PPIs are non-configurable, then we should return -EINVAL if
> trying to set them to a setting which is not supported.  For example,
> pass through all states to gic_configure_irq(), and have gic_configure_irq()
> return whether the state is valid.
> 
> Then, gic_configure_irq() can read back the register after trying to set
> the appropriate value, and see whether it was taken.  If the bits remain
> unchanged, then it can decide that the requested mode is not supported,
> and return -EINVAL.
> 
> In any case, let's not hack the code in the way that Liviu is trying to
> do.

OK, I will send a v2 patch, doing the right thing.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯