arm: mediatek: Add support for GIC interrupt polarity extension.

From: "Joe.C" <[email protected]>

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 <[email protected]>

---
 drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

drivers/irqchip/irq-gic.c +16 -7

index 57d165e..66485ab 100644
@@ -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)

Hi Joe,

Quoted 60 lines · Joe.C

From: "Joe.C" <[email protected]>

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 <[email protected]>

---
 drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

drivers/irqchip/irq-gic.c +16 -7

index 57d165e..66485ab 100644
@@ -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)

drivers/irqchip/irq-gic.c static int gic_set_type(struct irq_data *d, unsigned int type) · line 244

Quoted 10 lines

drivers/irqchip/irq-gic.c +2 -2

 		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).

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).

Marc,

Quoted 5 lines · Marc Zyngier

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.

Quoted 3 lines

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?

Quoted 3 lines

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.

Quoted 8 lines · Marc Zyngier

Quoted 3 lines

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.

Quoted 6 lines

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?

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

[Adding Jiang Liu to the spam-fest]

Hi Thomas,

Quoted 16 lines · Thomas Gleixner

Quoted 8 lines · Marc Zyngier

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 51 lines

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);
}

Quoted 7 lines

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?

Quoted 2 lines · Marc Zyngier

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).

From: "Joe.C" <[email protected]>

Add binding documentation for Mediatek SoC GIC interrupt polarity extension.

Signed-off-by Joe.C <[email protected]>

---
 .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt new file +16 -1

new file mode 100644
index 0000000..16ea372
@@ -0,0 +1,16 @@
+Mediatek 65xx/81xx GIC interrupt polarity extension
+
+Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
+these can be used as GIC interrupt polarity extension.
+
+Required properties:
+- compatible: Compatible property value should be "mediatek,mt6577-intpol"
+
+- reg: Physical base address of the int pol registers and length of memory
+  mapped region.
+
+Example:
+       intpol: intpol at 10200100 {
+               compatible = "mediatek,mt6577-intpol";
+               reg = <0x10200100 0x1c>;
+       };
--

1.8.1.1.dirty

2014-08-13 4:11 GMT+02:00 Joe.C <[email protected]>:

Quoted 9 lines

From: "Joe.C" <[email protected]>

Add binding documentation for Mediatek SoC GIC interrupt polarity extension.

Signed-off-by Joe.C <[email protected]>

---
 .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

I think the place for the file should be in Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as it is a interrupt-controller extension of the mediatek architecture.

Quoted 26 lines

Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt new file +16 -1

new file mode 100644
index 0000000..16ea372
@@ -0,0 +1,16 @@
+Mediatek 65xx/81xx GIC interrupt polarity extension
+
+Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
+these can be used as GIC interrupt polarity extension.
+
+Required properties:
+- compatible: Compatible property value should be "mediatek,mt6577-intpol"
+
+- reg: Physical base address of the int pol registers and length of memory
+  mapped region.
+
+Example:
+       intpol: intpol at 10200100 {
+               compatible = "mediatek,mt6577-intpol";
+               reg = <0x10200100 0x1c>;
+       };
--

1.8.1.1.dirty

[Sorry, resend to include all maintainers.]

This series add support for mediatek GIC interrupt polarity extension. Several components in mediatek SoC have low level triggered interrupt and require this support.

This also correct 6589 timer irq polarity. Previous version works because 6589 boot loader already set correct polarity for timer interrupt.

The patch set is based on Matthias's Mediatek basic support for v3.17 [1].

v2:
- Make mt6589.dtsi changes as a separate commit as Matthias suggest.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272561.html

Joe.C (4):
irqchip: gic: Change irq type check when extension is
arm: mediatek: Add support for GIC interrupt polarity
arm: mediatek: Add intpol in mt6589.dtsi
dt-bindings: add bindings for mediatek intpol

 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt |   16 ++
 arch/arm/boot/dts/mt6589.dtsi                                              |    7 -
 arch/arm/mach-mediatek/Makefile                                            |    2
 arch/arm/mach-mediatek/common.h                                            |   19 +++
 arch/arm/mach-mediatek/intpol.c                                            |   61 ++++++++++
 arch/arm/mach-mediatek/mediatek.c                                          |   10 +
 drivers/irqchip/irq-gic.c                                                  |   27 ++--
 7 files changed, 131 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
 create mode 100644 arch/arm/mach-mediatek/common.h
 create mode 100644 arch/arm/mach-mediatek/intpol.c