| Message ID | 1407895884-18131-2-git-send-email-srv_yingjoe.chen@mediatek.com |
|---|---|
| State | New |
| Headers | show |
Hi Joe, On 13/08/14 03:11, Joe.C wrote: > From: "Joe.C" <yingjoe.chen@mediatek.com> > > GIC supports the combination with external extensions. But this > is not reflected in the checks of the interrupt type flag. > This patch allows interrupt types other than the one supported by GIC, > if an architecture extension is present and supports them. > > Signed-off-by: Joe.C <yingjoe.chen@mediatek.com> > --- > drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 57d165e..66485ab 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > u32 confoff = (gicirq / 16) * 4; > bool enabled = false; > u32 val; > + int ret = 0; > > /* Interrupt configuration for SGIs can't be changed */ > if (gicirq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > - return -EINVAL; > - > raw_spin_lock(&irq_controller_lock); > > - if (gic_arch_extn.irq_set_type) > - gic_arch_extn.irq_set_type(d, type); > + if (gic_arch_extn.irq_set_type) { > + ret = gic_arch_extn.irq_set_type(d, type); > + if (ret) > + goto out; > + } else if (type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) { > + ret = -EINVAL; > + goto out; > + } > > val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > - if (type == IRQ_TYPE_LEVEL_HIGH) > + /* Check for both edge and level here, so we can support GIC irq > + polarity extension in gic_arch_extn.irq_set_type. If arch > + doesn't support polarity extension, the check above will reject > + improper type. */ > + if (type & IRQ_TYPE_LEVEL_MASK) > val &= ~confmask; > - else if (type == IRQ_TYPE_EDGE_RISING) > + else if (type & IRQ_TYPE_EDGE_BOTH) > val |= confmask; > > /* > @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > if (enabled) > writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); > - > +out: > raw_spin_unlock(&irq_controller_lock); > > - return 0; > + return ret; > } > > static int gic_retrigger(struct irq_data *d) > You're really abusing the gic_arch_extn feature. I know this is tempting, but this is pushing it a bit too far. This feature exist for one particular reason: if your GIC is in the same power-domain as the CPUs, it will go down as well when you suspend the system, hence being enable to wake the CPU up. You then need a shadow interrupt controller to take over. This is exactly why we call the hook on every GIC-related operation. Here, you're using it to program something that sits between the device and the GIC. This is a separate block, with its own hardware configuration, that modifies the interrupt signal. This should be reflected in the device-tree and the code paths. You can probably model this as a separate irqchip for the few interrupts that require this, or have it configured at boot time (assuming the configuration never changes). Thanks, M.
Hi, Thanks for your suggestions. On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote: > Hi Joe, > > On 13/08/14 03:11, Joe.C wrote: > > From: "Joe.C" <yingjoe.chen@mediatek.com> > > > > GIC supports the combination with external extensions. But this > > is not reflected in the checks of the interrupt type flag. > > This patch allows interrupt types other than the one supported by GIC, > > if an architecture extension is present and supports them. > > > > Signed-off-by: Joe.C <yingjoe.chen@mediatek.com> > > --- > > drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > > index 57d165e..66485ab 100644 > > --- a/drivers/irqchip/irq-gic.c > > +++ b/drivers/irqchip/irq-gic.c > > @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > u32 confoff = (gicirq / 16) * 4; > > bool enabled = false; > > u32 val; > > + int ret = 0; > > > > /* Interrupt configuration for SGIs can't be changed */ > > if (gicirq < 16) > > return -EINVAL; > > > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > > - return -EINVAL; > > - > > raw_spin_lock(&irq_controller_lock); > > > > - if (gic_arch_extn.irq_set_type) > > - gic_arch_extn.irq_set_type(d, type); > > + if (gic_arch_extn.irq_set_type) { > > + ret = gic_arch_extn.irq_set_type(d, type); > > + if (ret) > > + goto out; > > + } else if (type != IRQ_TYPE_LEVEL_HIGH && > > + type != IRQ_TYPE_EDGE_RISING) { > > + ret = -EINVAL; > > + goto out; > > + } > > > > val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > > - if (type == IRQ_TYPE_LEVEL_HIGH) > > + /* Check for both edge and level here, so we can support GIC irq > > + polarity extension in gic_arch_extn.irq_set_type. If arch > > + doesn't support polarity extension, the check above will reject > > + improper type. */ > > + if (type & IRQ_TYPE_LEVEL_MASK) > > val &= ~confmask; > > - else if (type == IRQ_TYPE_EDGE_RISING) > > + else if (type & IRQ_TYPE_EDGE_BOTH) > > val |= confmask; > > > > /* > > @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > > > if (enabled) > > writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); > > - > > +out: > > raw_spin_unlock(&irq_controller_lock); > > > > - return 0; > > + return ret; > > } > > > > static int gic_retrigger(struct irq_data *d) > > > > You're really abusing the gic_arch_extn feature. I know this is > tempting, but this is pushing it a bit too far. > > This feature exist for one particular reason: if your GIC is in the same > power-domain as the CPUs, it will go down as well when you suspend the > system, hence being enable to wake the CPU up. You then need a shadow > interrupt controller to take over. This is exactly why we call the hook > on every GIC-related operation. Actually we are doing this too, it is called SYS_CIRQ in our IC, and we'll need to support that in the future. This intpol is part of CIRQ function block. Does it make more senses if I add skeleton for CIRQ support, and implement intpol inside it? > Here, you're using it to program something that sits between the device > and the GIC. This is a separate block, with its own hardware > configuration, that modifies the interrupt signal. This should be > reflected in the device-tree and the code paths. > > You can probably model this as a separate irqchip for the few interrupts > that require this, or have it configured at boot time (assuming the > configuration never changes). The boot loader did setup interrupt polarity for those used in boot loader, but not all of them. Datasheet lists components irqs as low active, so I think it makes more sense to allow driver to use IRQF_TRIGGER_LOW instead of having them to notice GIC only support high active and use IRQF_TRIGGER_HIGH. This rule out configure it at boot loader or kernel init irq time. If I implement this as a separate irqchip, I need to reuse most gic irqchip functions. Without changing irq-gic.c to make them global, I can only think of hack like this: gic_init_bases(..) /* init gic */ gic_chip = irq_get_chip(0); /* to get gic irq_chip */ org_gic_set_type = gic_chip->irq_set_type; gic_chip->irq_set_type = mt_irq_polarity_set_type; and calling original gic_set_type from mt_irq_polarity_set_type with irq type fixup. Joe.C
Marc, On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote: > Here, you're using it to program something that sits between the > device and the GIC. This is a separate block, with its own hardware > configuration, that modifies the interrupt signal. This should be > reflected in the device-tree and the code paths. > > You can probably model this as a separate irqchip for the few > interrupts that require this, or have it configured at boot time > (assuming the configuration never changes). It seems to me that using a separate irqchip for a simple inverter would add the run-time overhead of passing through wrapper functions on every IRQ. Do you have an idea how this could be avoided without using the gic_arch_extn feature? As in the DT the actual IRQ polarity should be used, simply configuring the HW IRQ polarity in the bootloader is not enough without telling the GIC driver which polarity is supported on which IRQ, right? Regards, Jan
Hi Jan, On 27/08/14 10:55, Jan Lübbe wrote: > Marc, > > On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote: >> Here, you're using it to program something that sits between the >> device and the GIC. This is a separate block, with its own hardware >> configuration, that modifies the interrupt signal. This should be >> reflected in the device-tree and the code paths. >> >> You can probably model this as a separate irqchip for the few >> interrupts that require this, or have it configured at boot time >> (assuming the configuration never changes). > > It seems to me that using a separate irqchip for a simple inverter would > add the run-time overhead of passing through wrapper functions on every > IRQ. Do you have an idea how this could be avoided without using the > gic_arch_extn feature? Well, from the rather vague description, it could be slightly more than a simple inverter, like being able to generate interrupts on both rising and falling edges. Sorry, but this is not the GIC as ARM has architected it. Yes, the additional irqchip adds some overhead. But the DT has to reflect the fact that there is something on the interrupt path that does some form of conversion. > As in the DT the actual IRQ polarity should be used, simply configuring > the HW IRQ polarity in the bootloader is not enough without telling the > GIC driver which polarity is supported on which IRQ, right? Looking a bit closer at things, what you describe in DT is the IRQ polarity the interrupt controller sees. Nothing else should interpret that field. So it is legal (IMO) to have a device with an interrupt specifier describing a rising edge interrupt, and yet have the device generating a falling edge, with Mediatek's special sauce doing the conversion in between. Something will have to configure the polarity widget though, but that can be left outside of the GIC. Thanks, M.
On Wed, 27 Aug 2014, Marc Zyngier wrote: > > As in the DT the actual IRQ polarity should be used, simply configuring > > the HW IRQ polarity in the bootloader is not enough without telling the > > GIC driver which polarity is supported on which IRQ, right? > > Looking a bit closer at things, what you describe in DT is the IRQ > polarity the interrupt controller sees. Nothing else should interpret > that field. > > So it is legal (IMO) to have a device with an interrupt specifier > describing a rising edge interrupt, and yet have the device generating a > falling edge, with Mediatek's special sauce doing the conversion in between. > > Something will have to configure the polarity widget though, but that > can be left outside of the GIC. This seems to become a popular topic and it looks like the whole GIC extension thing is going to explode sooner than later. We are currently discussing hierarchical irq domains to solve a different issue in x86 land. See the related discussion here: https://lkml.org/lkml/2014/8/1/67 Now looking at these GIC plus extra sauce problems, I wonder whether this wouldn't be solvable in a similar way. If you look at it from the HW perspective you have: --------- --------- ---| MAGIC |------|ARM GIC| ---| |------| | ---| |------| | ---| |------| | ---| |------| | --------- --------- The MAGIC interrupt controller only provides functionality which is not available from the ARM architected GIC but maps 1:1 to the ARM GIC interrupt lines. So it looks like a variation to the more dynamic mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86. The idea is to have two irq domains: magic_domain and armgic_domain. The magic_domain is the frontend facing the devices and the armgic_domain is the parent domain. This is also reflected in hierarchical data structures, i.e. irq_desc->irq_data will get a new field parent_data, which points to the irq_data of the parent interrupt controller, which is allocated separately when the interrupt line is instantiated. So in the above case the hotpath ack/eoi functions of the irq chip associated to the device interrupt, i.e. magic_chip, would do: irq_ack(struct irq_data *d) { struct irq_data *pd = d->parent_data; pd->chip->irq_ack(pd); } Granted, that's another level of indirection, but this is going to be more efficient than a boatload of conditional hooks in the GIC code proper. Not to talk about the maintainability of the whole maze. The irq_set_type() function would do: irq_set_type(struct irq_data *d, int type) { struct irq_data *pd = d->parent_data; gic_type = set_magic_chip_type(d, type); return pd->chip->irq_set_type(d, gic_type); } Switching to this allows to completely avoid the gazillion of hooks in the gic code and should work nicely with multiplatform kernels by simpling hooking up the domain parent relation ship to the proper magic domain or leave the armgic as the direct device interface in case the SoC does not have the magic chip in front of the arm gic. Thoughts? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Adding Jiang Liu to the spam-fest] Hi Thomas, On Wed, Aug 27 2014 at 1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, 27 Aug 2014, Marc Zyngier wrote: >> > As in the DT the actual IRQ polarity should be used, simply configuring >> > the HW IRQ polarity in the bootloader is not enough without telling the >> > GIC driver which polarity is supported on which IRQ, right? >> >> Looking a bit closer at things, what you describe in DT is the IRQ >> polarity the interrupt controller sees. Nothing else should interpret >> that field. >> >> So it is legal (IMO) to have a device with an interrupt specifier >> describing a rising edge interrupt, and yet have the device generating a >> falling edge, with Mediatek's special sauce doing the conversion in between. >> >> Something will have to configure the polarity widget though, but that >> can be left outside of the GIC. > > This seems to become a popular topic and it looks like the whole GIC > extension thing is going to explode sooner than later. > > We are currently discussing hierarchical irq domains to solve a > different issue in x86 land. See the related discussion here: > > https://lkml.org/lkml/2014/8/1/67 Ah, very interesting. Thanks for pointing that out. > Now looking at these GIC plus extra sauce problems, I wonder whether > this wouldn't be solvable in a similar way. If you look at it from the > HW perspective you have: > > --------- --------- > ---| MAGIC |------|ARM GIC| > ---| |------| | > ---| |------| | > ---| |------| | > ---| |------| | > --------- --------- > > The MAGIC interrupt controller only provides functionality which is > not available from the ARM architected GIC but maps 1:1 to the ARM GIC > interrupt lines. So it looks like a variation to the more dynamic > mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86. > > The idea is to have two irq domains: magic_domain and armgic_domain. > > The magic_domain is the frontend facing the devices and the > armgic_domain is the parent domain. This is also reflected in > hierarchical data structures, i.e. irq_desc->irq_data will get a new > field parent_data, which points to the irq_data of the parent > interrupt controller, which is allocated separately when the interrupt > line is instantiated. > > So in the above case the hotpath ack/eoi functions of the irq chip > associated to the device interrupt, i.e. magic_chip, would do: > > irq_ack(struct irq_data *d) > { > struct irq_data *pd = d->parent_data; > > pd->chip->irq_ack(pd); > } > > Granted, that's another level of indirection, but this is going to be > more efficient than a boatload of conditional hooks in the GIC code > proper. Not to talk about the maintainability of the whole maze. > > The irq_set_type() function would do: > > irq_set_type(struct irq_data *d, int type) > { > struct irq_data *pd = d->parent_data; > > gic_type = set_magic_chip_type(d, type); > > return pd->chip->irq_set_type(d, gic_type); > } > > Switching to this allows to completely avoid the gazillion of hooks in > the gic code and should work nicely with multiplatform kernels by > simpling hooking up the domain parent relation ship to the proper > magic domain or leave the armgic as the direct device interface in > case the SoC does not have the magic chip in front of the arm gic. > > Thoughts? I very much like that kind of approach. Stacking domains seems to solve a number of issues at once: - NVIDIA's gic extension - TI's crossbar - ARM's GICv2m - Mediatek's glorified line inverter - ... and probably the next madness that's going to land tomorrow I haven't completly groked the way we're going to allocate domains and irq_data structures, but this is definitely something worth investigating. I'm not too worried about the indirection either - at least we end up with maintainable code. We need to work out how to drive the whole stacking from a DT perspective: Mark, any idea? Jiang, would you mind keeping us ARM folks posted when you resend your patch series? That'd be much appreciated. In the meantime, I'll furbish an axe and aim it squarely at the GIC code... ;-) Thanks again, M.
On Wed, 27 Aug 2014, Marc Zyngier wrote: > I very much like that kind of approach. Stacking domains seems to solve > a number of issues at once: > > - NVIDIA's gic extension > - TI's crossbar > - ARM's GICv2m > - Mediatek's glorified line inverter > - ... and probably the next madness that's going to land tomorrow Its probably already there you just don't know about it yet :) > I haven't completly groked the way we're going to allocate domains and > irq_data structures, but this is definitely something worth All we have for now is a rough idea and some pseudo code in my lengthy reply to Jiang, but it shouldn't be hard to implement something useful. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote: > [Adding Jiang Liu to the spam-fest] > > Hi Thomas, > > On Wed, Aug 27 2014 at 1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Wed, 27 Aug 2014, Marc Zyngier wrote: > >> > As in the DT the actual IRQ polarity should be used, simply configuring > >> > the HW IRQ polarity in the bootloader is not enough without telling the > >> > GIC driver which polarity is supported on which IRQ, right? > >> > >> Looking a bit closer at things, what you describe in DT is the IRQ > >> polarity the interrupt controller sees. Nothing else should interpret > >> that field. > >> > >> So it is legal (IMO) to have a device with an interrupt specifier > >> describing a rising edge interrupt, and yet have the device generating a > >> falling edge, with Mediatek's special sauce doing the conversion in between. > >> > >> Something will have to configure the polarity widget though, but that > >> can be left outside of the GIC. > > > > This seems to become a popular topic and it looks like the whole GIC > > extension thing is going to explode sooner than later. > > > > We are currently discussing hierarchical irq domains to solve a > > different issue in x86 land. See the related discussion here: > > > > https://lkml.org/lkml/2014/8/1/67 > > Ah, very interesting. Thanks for pointing that out. > > > Now looking at these GIC plus extra sauce problems, I wonder whether > > this wouldn't be solvable in a similar way. If you look at it from the > > HW perspective you have: > > > > --------- --------- > > ---| MAGIC |------|ARM GIC| > > ---| |------| | > > ---| |------| | > > ---| |------| | > > ---| |------| | > > --------- --------- > > > > The MAGIC interrupt controller only provides functionality which is > > not available from the ARM architected GIC but maps 1:1 to the ARM GIC > > interrupt lines. So it looks like a variation to the more dynamic > > mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86. > > > > The idea is to have two irq domains: magic_domain and armgic_domain. > > > > The magic_domain is the frontend facing the devices and the > > armgic_domain is the parent domain. This is also reflected in > > hierarchical data structures, i.e. irq_desc->irq_data will get a new > > field parent_data, which points to the irq_data of the parent > > interrupt controller, which is allocated separately when the interrupt > > line is instantiated. > > > > So in the above case the hotpath ack/eoi functions of the irq chip > > associated to the device interrupt, i.e. magic_chip, would do: > > > > irq_ack(struct irq_data *d) > > { > > struct irq_data *pd = d->parent_data; > > > > pd->chip->irq_ack(pd); > > } > > > > Granted, that's another level of indirection, but this is going to be > > more efficient than a boatload of conditional hooks in the GIC code > > proper. Not to talk about the maintainability of the whole maze. > > > > The irq_set_type() function would do: > > > > irq_set_type(struct irq_data *d, int type) > > { > > struct irq_data *pd = d->parent_data; > > > > gic_type = set_magic_chip_type(d, type); > > > > return pd->chip->irq_set_type(d, gic_type); > > } > > > > Switching to this allows to completely avoid the gazillion of hooks in > > the gic code and should work nicely with multiplatform kernels by > > simpling hooking up the domain parent relation ship to the proper > > magic domain or leave the armgic as the direct device interface in > > case the SoC does not have the magic chip in front of the arm gic. > > > > Thoughts? > > I very much like that kind of approach. Stacking domains seems to solve > a number of issues at once: > > - NVIDIA's gic extension > - TI's crossbar > - ARM's GICv2m > - Mediatek's glorified line inverter > - ... and probably the next madness that's going to land tomorrow > > I haven't completly groked the way we're going to allocate domains and > irq_data structures, but this is definitely something worth > investigating. I'm not too worried about the indirection either - at > least we end up with maintainable code. > > We need to work out how to drive the whole stacking from a DT > perspective: Mark, any idea? Describing the lines the magic irqchip has to its parent irqchip is simple, the standard interrupts property can handle that: parent: parent { ... #interrupt-cells = <1>; }; magic { ... interrupt-parent = <&parent>; interrupts = <3>, <6>, <17>, <2>; #interrupt-cells = <1>; }; The harder part is describing the configuration of each interrupt to the magic irqchip (e.g. edge vs level triggered), which is inseparable form the line identifier in the interrupt-specifier. If there interrupts don't share the same configuration then I'm not sure how we describe that in the DT. That's especially problematic if the assignment of parent line is dynamic (as with the crossbar iirc). Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 27 Aug 2014, Mark Rutland wrote: > On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote: > > We need to work out how to drive the whole stacking from a DT > > perspective: Mark, any idea? > > Describing the lines the magic irqchip has to its parent irqchip is > simple, the standard interrupts property can handle that: > > parent: parent { > ... > #interrupt-cells = <1>; > }; > > magic { > ... > interrupt-parent = <&parent>; > interrupts = <3>, <6>, <17>, <2>; > #interrupt-cells = <1>; > }; > > The harder part is describing the configuration of each interrupt to the > magic irqchip (e.g. edge vs level triggered), which is inseparable form > the line identifier in the interrupt-specifier. Do we really need to decribe that at the this level? You have the parent ARMGIC and the maGIC with a specified (or even dynamic) routing of the maGIC lines to the ARMGIC. Now the device which uses a maGIC interrupt specifies the interrupt type at the maGIC. So the type setter function of the maGIC configures the maGIC hardware in such a way that the output to the ARMGIC results in a type which the ARMGIC can handle and calls the type setter function of the maGIC with that type. I don't think you need to decribe this in the magic/parent relation ship at all. maGIC should know what kind of output it can provide to ARMGIC so that should just work, unless I'm missing something. > If there interrupts don't share the same configuration then I'm not sure > how we describe that in the DT. That's especially problematic if the > assignment of parent line is dynamic (as with the crossbar iirc). Why is this an issue? There are two ways to solve that: 1) You can do a fully dynamic allocation at the parent level, which of course requires that the whole parent range is dynamically managed. That's what we are planning for the MSI/Remap/Vector stacking 2) You define the irq lines at the parent which are available for dynamic stacking and let the allocation code hand one out. 3) You tell the maGIC controller which lines of the parent are available for it, so it can only stack onto those lines and instead of letting the parent allocate a free one, the maGIC needs to map its stuff to one of those ARMGIC lines. Which one to chose depends on the particular maGIC incarnation, but its not a big issue to select one ... Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 57d165e..66485ab 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type) u32 confoff = (gicirq / 16) * 4; bool enabled = false; u32 val; + int ret = 0; /* Interrupt configuration for SGIs can't be changed */ if (gicirq < 16) return -EINVAL; - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) - return -EINVAL; - raw_spin_lock(&irq_controller_lock); - if (gic_arch_extn.irq_set_type) - gic_arch_extn.irq_set_type(d, type); + if (gic_arch_extn.irq_set_type) { + ret = gic_arch_extn.irq_set_type(d, type); + if (ret) + goto out; + } else if (type != IRQ_TYPE_LEVEL_HIGH && + type != IRQ_TYPE_EDGE_RISING) { + ret = -EINVAL; + goto out; + } val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); - if (type == IRQ_TYPE_LEVEL_HIGH) + /* Check for both edge and level here, so we can support GIC irq + polarity extension in gic_arch_extn.irq_set_type. If arch + doesn't support polarity extension, the check above will reject + improper type. */ + if (type & IRQ_TYPE_LEVEL_MASK) val &= ~confmask; - else if (type == IRQ_TYPE_EDGE_RISING) + else if (type & IRQ_TYPE_EDGE_BOTH) val |= confmask; /* @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type) if (enabled) writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); - +out: raw_spin_unlock(&irq_controller_lock); - return 0; + return ret; } static int gic_retrigger(struct irq_data *d)