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! /
---------------
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
