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

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:

quoted

On 01/12/14 11:23, Russell King - ARM Linux wrote:

quoted

On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:

quoted

Hi Russell,

On 01/12/14 11:03, Russell King - ARM Linux wrote:

quoted

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!  /
  ---------------
    ?\_(?)_/?