| Message ID | 1417197340-27298-1-git-send-email-Liviu.Dudau@arm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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. >
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;
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.
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.
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.
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. >
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.
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. >
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;
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(-)