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

[Liviu Dudau <Liviu.Dudau@arm.com>]

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