[PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Some devices have configurable IRQ output polarities. Software might
use IRQ_TYPE_* to determine how to configure such a device's IRQ
output polarity in order to match how the IRQ controller input is
configured. If the board or SoC inverts the signal between the
device's IRQ output and controller's IRQ output, software must be
aware of this fact, in order to program the IRQ output to the correct
(i.e. opposite) polarity. This flag provides that information.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 include/linux/irq.h    | 12 ++++++++++++
 kernel/irq/chip.c      | 24 ++++++++++++++++++++++++
 kernel/irq/irqdomain.c |  4 ++++
 3 files changed, 40 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 7dc10036eff5..535f3937e99e 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -73,6 +73,11 @@ typedef	void (*irq_preflow_handler_t)(struct irq_data *data);
  * IRQ_IS_POLLED		- Always polled by another interrupt. Exclude
  *				  it from the spurious interrupt detection
  *				  mechanism and from core side polling.
+ * IRQ_SRC_DST_INVERTED		- An inverter exists between the source's IRQ
+ *				  output, and the IRQ controller's input.
+ *				  For devices with programmable output polarity
+ *				  this helps the driver match the device output
+ *				  to the controller's input polarity.
  */
 enum {
 	IRQ_TYPE_NONE		= 0x00000000,
@@ -98,6 +103,7 @@ enum {
 	IRQ_NOTHREAD		= (1 << 16),
 	IRQ_PER_CPU_DEVID	= (1 << 17),
 	IRQ_IS_POLLED		= (1 << 18),
+	IRQ_SRC_DST_INVERTED	= (1 << 19),
 };
 
 #define IRQF_MODIFY_MASK	\
@@ -218,6 +224,11 @@ static inline u32 irqd_get_trigger_type(struct irq_data *d)
 	return d->state_use_accessors & IRQD_TRIGGER_MASK;
 }
 
+static inline u32 irqd_get_src_dst_inverted(struct irq_data *d)
+{
+	return d->state_use_accessors & IRQ_SRC_DST_INVERTED;
+}
+
 /*
  * Must only be called inside irq_chip.irq_set_type() functions.
  */
@@ -538,6 +549,7 @@ extern int irq_set_chip(unsigned int irq, struct irq_chip *chip);
 extern int irq_set_handler_data(unsigned int irq, void *data);
 extern int irq_set_chip_data(unsigned int irq, void *data);
 extern int irq_set_irq_type(unsigned int irq, unsigned int type);
+extern int irq_set_src_dst_inverted(unsigned int irq, unsigned int inverted);
 extern int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry);
 extern int irq_set_msi_desc_off(unsigned int irq_base, unsigned int irq_offset,
 				struct msi_desc *entry);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index dc04c166c54d..ff9e13fa1170 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -70,6 +70,30 @@ int irq_set_irq_type(unsigned int irq, unsigned int type)
 EXPORT_SYMBOL(irq_set_irq_type);
 
 /**
+ *	irq_set_src_dst_inverted(unsigned int irq, u32 inverted)
+ *	@irq:		irq number
+ *	@inverted:	IRQ_SRC_DST_INVERTED value - see include/linux/irq.h
+ */
+int irq_set_src_dst_inverted(unsigned int irq, unsigned int inverted)
+{
+	unsigned long flags;
+	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+	int ret = 0;
+
+	if (!desc)
+		return -EINVAL;
+
+	inverted &= IRQ_SRC_DST_INVERTED;
+	if (inverted)
+		irqd_set(&desc->irq_data, inverted);
+	else
+		irqd_clear(&desc->irq_data, inverted);
+	irq_put_desc_busunlock(desc, flags);
+	return ret;
+}
+EXPORT_SYMBOL(irq_set_src_dst_inverted);
+
+/**
  *	irq_set_handler_data - set irq handler data for an irq
  *	@irq:	Interrupt number
  *	@data:	Pointer to interrupt specific data
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index f14033700c25..f9cee747424e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -471,6 +471,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
 	struct irq_domain *domain;
 	irq_hw_number_t hwirq;
 	unsigned int type = IRQ_TYPE_NONE;
+	unsigned int inverted = 0;
 	unsigned int virq;
 
 	domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain;
@@ -487,6 +488,8 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
 		if (domain->ops->xlate(domain, irq_data->np, irq_data->args,
 					irq_data->args_count, &hwirq, &type))
 			return 0;
+		inverted = type & IRQ_SRC_DST_INVERTED;
+		type &= IRQ_TYPE_SENSE_MASK;
 	}
 
 	/* Create mapping */
@@ -498,6 +501,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
 	if (type != IRQ_TYPE_NONE &&
 	    type != irq_get_trigger_type(virq))
 		irq_set_irq_type(virq, type);
+	irq_set_src_dst_inverted(virq, inverted);
 	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_of_mapping);
-- 
1.8.1.5

[PATCH V3 2/5] dt: define IRQ flags bit 4 as src/dst inversion

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Some devices have configurable IRQ output polarities. Software might
use IRQ specifier flag bits[3:0] to determine how to configure such a
device's IRQ output polarity in order to match how the IRQ controller
input is configured. If the board or SoC inverts the signal between the
device's IRQ output and controller's IRQ output, software must be
aware of this fact, in order to program the IRQ output to the correct
(i.e. opposite) polarity. This flag provides that information.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 Documentation/devicetree/bindings/arm/gic.txt                     | 8 ++++++++
 .../devicetree/bindings/interrupt-controller/interrupts.txt       | 8 ++++++++
 include/dt-bindings/interrupt-controller/irq.h                    | 1 +
 3 files changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index bae0d87a38b2..b0b65a440239 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -33,6 +33,14 @@ Main node required properties:
 		2 = high-to-low edge triggered
 		4 = active high level-sensitive
 		8 = active low level-sensitive
+	bit[4] IRQ is inverted between source and sink.
+	Some devices have configurable IRQ output polarities. Software might
+	use flag bits[3:0] to determine how to configure such a device's IRQ
+	output polarity in order to match how the IRQ controller input is
+	configured. If the board or SoC inverts the signal between the
+	device's IRQ output and controller's IRQ output, software must be
+	aware of this fact, in order to program the IRQ output to the correct
+	(i.e. opposite) polarity. This flag provides that information.
 	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.
diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 1486497a24c1..636eb0f478bf 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
@@ -76,6 +76,14 @@ commonly used:
         2 = high-to-low edge triggered
         4 = active high level-sensitive
         8 = active low level-sensitive
+    - bit[4] IRQ is inverted between source and sink
+        Some devices have configurable IRQ output polarities. Software might
+        use flag bits[3:0] to determine how to configure such a device's IRQ
+        output polarity in order to match how the IRQ controller input is
+        configured. If the board or SoC inverts the signal between the
+        device's IRQ output and controller's IRQ output, software must be
+        aware of this fact, in order to program the IRQ output to the correct
+        (i.e. opposite) polarity. This flag provides that information.
 
   Example:
 
diff --git a/include/dt-bindings/interrupt-controller/irq.h b/include/dt-bindings/interrupt-controller/irq.h
index 33a1003c55aa..5b04918c5c72 100644
--- a/include/dt-bindings/interrupt-controller/irq.h
+++ b/include/dt-bindings/interrupt-controller/irq.h
@@ -15,5 +15,6 @@
 #define IRQ_TYPE_EDGE_BOTH	(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)
 #define IRQ_TYPE_LEVEL_HIGH	4
 #define IRQ_TYPE_LEVEL_LOW	8
+#define IRQ_SRC_DST_INVERTED	16
 
 #endif
-- 
1.8.1.5

[PATCH V3 3/5] irqchip: gic: parse IRQ specifier flag src/dst inversion

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Modify the GIC driver to parse IRQ specifier flag bit 4 (src/dst
inverted) in the xlate function.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 drivers/irqchip/irq-gic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 500e533b9648..97800b349f54 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -847,6 +847,9 @@ static int gic_irq_domain_xlate(struct irq_domain *d,
 		*out_hwirq += 16;
 
 	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+	if (intspec[2] & BIT(4))
+		*out_type |= IRQ_SRC_DST_INVERTED;
+
 	return 0;
 }
 
-- 
1.8.1.5

[PATCH V3 4/5] mfd: palmas: support IRQ inversion at the board level

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Implement the new DT property ti,irq-externally-inverted, and add an
equivalent platform data field to match. This allows the driver to
correctly automatically configure the IRQ output polarity when the board
or SoC contains an inverter between the Palmas IRQ output and IRQ
controller input.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3:
* Use an IRQD flag to represent the data, rather than a custom DT property.
v2:
* Split V1's patch 1/2 into separate patches 1/3 and 2/3.
---
 drivers/mfd/palmas.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index d280d789e55a..3281d4103e9d 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -292,7 +292,8 @@ static int palmas_set_pdata_irq_flag(struct i2c_client *i2c,
 		return -EINVAL;
 	}
 
-	pdata->irq_flags = irqd_get_trigger_type(irq_data);
+	pdata->irq_flags = irqd_get_trigger_type(irq_data) |
+			   irqd_get_src_dst_inverted(irq_data);
 	dev_info(&i2c->dev, "Irq flag is 0x%08x\n", pdata->irq_flags);
 	return 0;
 }
@@ -447,6 +448,8 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
 		reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
 	else
 		reg = 0;
+	if (pdata->irq_flags & IRQ_SRC_DST_INVERTED)
+		reg ^= PALMAS_POLARITY_CTRL_INT_POLARITY;
 	ret = palmas_update_bits(palmas, PALMAS_PU_PD_OD_BASE,
 			PALMAS_POLARITY_CTRL, PALMAS_POLARITY_CTRL_INT_POLARITY,
 			reg);
-- 
1.8.1.5

[PATCH V3 5/5] ARM: tegra: fix Dalmore PMIC IRQ polarity

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The Tegra PMC's resume-from-sleep logic wants an active-low IRQ input
from the PMIC. However, the PMIC IRQ is also routed to the GIC, which
only supports active high IRQs (or rising edge). Hence, the signal must
be inverted in the PMC before being routed to the GIC. This implies that
the PMC DT property nvidia,invert-interrupt must be set, and it is.

The PMIC's DT interrupts property must represent the IRQ level at the
GIC, since that is the PMIC's parent IRQ controller. Fix the PMIC's
interrupts property to correctly describe the GIC input polarity.

However, the PMIC IRQ output's polarity is programmable in HW, and by
default follows the parent IRQ controller's input polarity. We need to
have an active-low output due to the inversion inside the Tegra PMC.
Hence, add the IRQ specifier flag which request that to the PMIC.

Reported-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: Update to match changes to IRQ specifier DT binding.
v2: No change.
---
 arch/arm/boot/dts/tegra114-dalmore.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index 8de543777882..f4319ac1a401 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -893,7 +893,8 @@
 		palmas: tps65913@58 {
 			compatible = "ti,palmas";
 			reg = <0x58>;
-			interrupts = <0 86 IRQ_TYPE_LEVEL_LOW>;
+			interrupts = <0 86
+				(IRQ_TYPE_LEVEL_HIGH | IRQ_SRC_DST_INVERTED)>;
 
 			#interrupt-cells = <2>;
 			interrupt-controller;
-- 
1.8.1.5

Re: [PATCH V3 4/5] mfd: palmas: support IRQ inversion at the board level

[Lee Jones]

> From: Stephen Warren <swarren@nvidia.com>
> 
> Implement the new DT property ti,irq-externally-inverted, and add an
> equivalent platform data field to match. This allows the driver to
> correctly automatically configure the IRQ output polarity when the board
> or SoC contains an inverter between the Palmas IRQ output and IRQ
> controller input.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v3:
> * Use an IRQD flag to represent the data, rather than a custom DT property.
> v2:
> * Split V1's patch 1/2 into separate patches 1/3 and 2/3.
> ---
>  drivers/mfd/palmas.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Looks good to me. Let me know how you want to deal with this. I'm
happy to create an immutable branch for everyone to pull from.

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[Thomas Gleixner]

On Mon, 3 Mar 2014, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> Some devices have configurable IRQ output polarities. Software might
> use IRQ_TYPE_* to determine how to configure such a device's IRQ
> output polarity in order to match how the IRQ controller input is
> configured. If the board or SoC inverts the signal between the
> device's IRQ output and controller's IRQ output, software must be
> aware of this fact, in order to program the IRQ output to the correct
> (i.e. opposite) polarity. This flag provides that information.

So what you're saying is:

Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input.

And you're storing the information about the presence of the inverter
logic in the irq itself, but the core does not make any use of it and
you let the device driver deal with the outcome.

This sucks as all affected drivers have to implement the same sanity
logic for this.

Why don't you implement a core function which tells the driver which
polarity to select? That requires a few more changes, but I think it's
worth it for other reasons.

Right now the set_type logic requires the irq chip drivers to
implement sanity checking and default selections for TYPE_NONE. We can
be more clever about that and add this information to the irq chip
flags.

enum {
     IRQ_CHIP_TYPES_MASK	= 0x0f,
     IRQ_CHIP_DEFAULT_MASK	= 0xf0,
     IRQ_CHIP_EXISTING_FLAGS	....
}

Now the irq_chip setup tells the core which types are available and
which one is the default fallback for TYPE_NONE.

So the core can do the sanity checks and we can kill quite some
repeated stuff from the irq chip implementations. For the inverted
logic case you can handle the inversion in the core as well, i.e. if a
driver requests IRQ_TYPE_LEVEL_HIGH you select IRQ_TYPE_LEVEL_LOW for
the chip, if possible.

For the case where the irq chip can only handle a single polarity you
can provide a core function to figure out to which polarity the driver
should set the device IRQ output line.

int irq_get_device_irq_polarity(int irq, int device_types)
{
	/*
	 * Handle the inversion logic and select a proper
	 * device irq polarity from irq_chip(@irq)->flags and
	 * @device_types.
	 *
	 * Return a proper error code if no match.
	 */
}

Let's look at an example:

      irq_chip.flags = IRQ_TYPE_LEVEL_HIGH;
      device_types = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;

Now for the non inverted case, this returns IRQ_TYPE_LEVEL_HIGH, for
the inverted case it returns IRQ_TYPE_LEVEL_LOW.

In both cases the irq_set_type() logic handles this correctly:

Non-Inverted case:
  	     irq_set_type(irq, IRQ_TYPE_LEVEL_HIGH);
	     -> Success

Inverted case:
  	     irq_set_type(irq, IRQ_TYPE_LEVEL_LOW);
	     invert -> IRQ_TYPE_LEVEL_HIGH
	     -> Success

To make this work for interrupt chips which have no set_type callback
we can do the following in irq_set_type():

       if (irq_is_inverted(irq))
	  type = invert(type);

       ret = irq_check_type(chip, &type);
       if (ret < 0 || !chip->irq_settype)
	  return ret;

       return chip->irq_settype();

And irq_check_type() does:

       if (!(chip->flags & IRQ_CHIP_TYPES_MASK))
       	  return chip->irq_settype ? 0 : -ENOTSUP;

       if (*type == IRQ_TYPE_NONE)
       	  *type == (chip->flags & IRQ_CHIP_DEFAULT_MASK) >> 4;
	
       return type_supported(chip->flags, *type);

Thanks,

	tglx
       	  

Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[Thomas Gleixner]

On Tue, 4 Mar 2014, Thomas Gleixner wrote:

> On Mon, 3 Mar 2014, Stephen Warren wrote:
> 
> > From: Stephen Warren <swarren@nvidia.com>
> > 
> > Some devices have configurable IRQ output polarities. Software might
> > use IRQ_TYPE_* to determine how to configure such a device's IRQ
> > output polarity in order to match how the IRQ controller input is
> > configured. If the board or SoC inverts the signal between the
> > device's IRQ output and controller's IRQ output, software must be
> > aware of this fact, in order to program the IRQ output to the correct
> > (i.e. opposite) polarity. This flag provides that information.
> 
> So what you're saying is:
> 
> Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input.
> 
> And you're storing the information about the presence of the inverter
> logic in the irq itself, but the core does not make any use of it and
> you let the device driver deal with the outcome.
> 
> This sucks as all affected drivers have to implement the same sanity
> logic for this.
> 
> Why don't you implement a core function which tells the driver which
> polarity to select? That requires a few more changes, but I think it's
> worth it for other reasons.
> 
> Right now the set_type logic requires the irq chip drivers to
> implement sanity checking and default selections for TYPE_NONE. We can
> be more clever about that and add this information to the irq chip
> flags.
> 
> enum {
>      IRQ_CHIP_TYPES_MASK	= 0x0f,
>      IRQ_CHIP_DEFAULT_MASK	= 0xf0,
>      IRQ_CHIP_EXISTING_FLAGS	....
> }

We need to extend the mask to indicate whether the chip supports
BOTH_EDGES. A chip can support FALLING and RISING, but not both at the
same time. For the set_type side the current BOTH = FALLING | RISING
is fine, but for checking the supported type it's not sufficient.

Thanks,

	tglx

Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[Stephen Warren]

On 03/04/2014 03:04 AM, Thomas Gleixner wrote:
> On Mon, 3 Mar 2014, Stephen Warren wrote:
> 
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Some devices have configurable IRQ output polarities. Software might
>> use IRQ_TYPE_* to determine how to configure such a device's IRQ
>> output polarity in order to match how the IRQ controller input is
>> configured. If the board or SoC inverts the signal between the
>> device's IRQ output and controller's IRQ output, software must be
>> aware of this fact, in order to program the IRQ output to the correct
>> (i.e. opposite) polarity. This flag provides that information.
> 
> So what you're saying is:
> 
> Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input.
> 
> And you're storing the information about the presence of the inverter
> logic in the irq itself, but the core does not make any use of it and
> you let the device driver deal with the outcome.
> 
> This sucks as all affected drivers have to implement the same sanity
> logic for this.
> 
> Why don't you implement a core function which tells the driver which
> polarity to select? That requires a few more changes, but I think it's
> worth it for other reasons.
> 
> Right now the set_type logic requires the irq chip drivers to
> implement sanity checking and default selections for TYPE_NONE. We can
> be more clever about that and add this information to the irq chip
> flags.

I don't see any such checking in drivers/irqchip/irq-gic.c; it rejects
any type other than IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING, and I
don't see any mention of TYPE_NONE in that file. Is the driver incomplete?

Instead of adding all this extra logic to the core, what do you think of
simply telling each driver that has a configurable interrupt output
polarity exactly which polarity to use. This information would come from
device tree or platform data. This is what I implemented in V1/V2 of
this series:

http://www.spinics.net/lists/devicetree/msg23648.html

Is that any better, or do you definitely want the IRQ core to manage this?

Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors

[Thomas Gleixner]

On Tue, 4 Mar 2014, Stephen Warren wrote:

> On 03/04/2014 03:04 AM, Thomas Gleixner wrote:
> > On Mon, 3 Mar 2014, Stephen Warren wrote:
> > 
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> Some devices have configurable IRQ output polarities. Software might
> >> use IRQ_TYPE_* to determine how to configure such a device's IRQ
> >> output polarity in order to match how the IRQ controller input is
> >> configured. If the board or SoC inverts the signal between the
> >> device's IRQ output and controller's IRQ output, software must be
> >> aware of this fact, in order to program the IRQ output to the correct
> >> (i.e. opposite) polarity. This flag provides that information.
> > 
> > So what you're saying is:
> > 
> > Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input.
> > 
> > And you're storing the information about the presence of the inverter
> > logic in the irq itself, but the core does not make any use of it and
> > you let the device driver deal with the outcome.
> > 
> > This sucks as all affected drivers have to implement the same sanity
> > logic for this.
> > 
> > Why don't you implement a core function which tells the driver which
> > polarity to select? That requires a few more changes, but I think it's
> > worth it for other reasons.
> > 
> > Right now the set_type logic requires the irq chip drivers to
> > implement sanity checking and default selections for TYPE_NONE. We can
> > be more clever about that and add this information to the irq chip
> > flags.
> 
> I don't see any such checking in drivers/irqchip/irq-gic.c; it rejects
> any type other than IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING, and I
> don't see any mention of TYPE_NONE in that file. Is the driver incomplete?

No. The IRQ_TYPE_NONE is a hysterical leftover.
 
> Instead of adding all this extra logic to the core, what do you think of
> simply telling each driver that has a configurable interrupt output
> polarity exactly which polarity to use. This information would come from
> device tree or platform data. This is what I implemented in V1/V2 of
> this series:
>
> http://www.spinics.net/lists/devicetree/msg23648.html
> 
> Is that any better, or do you definitely want the IRQ core to manage this?

Oh, yes.

Simply because any driver which is not aware of that inversion will
trip over the issue that it requests by the best of its knowledge
IRQ_TYPE_LEVEL_HIGH while it should actually request
IRQ_TYPE_LEVEL_LOW due to the inversion.

Are you really going to make all possibly affected drivers aware of
that? Good luck!

That's why we want to move such stuff to core code. Assume the
following scenario:

    driverX which works perfectly fine on SoCA is reused for SoCB
    where the inverter sits between the device and the SoCB irq
    controller.

With your DT scheme the whole thing falls flat on the nose simply
because neither the driverX nor the core code is aware of the
incompability.

So driverX is happily using the IRQ_TYPE_LEVEL_HIGH setting which was
used when the driver was written and the core works nicely with that
because the irq chips supports IRQ_TYPE_LEVEL_HIGH. Just the fact that
the inverter is in the hardware results in an infinite interrupt
storm. Are you going to handle the bug reports and "remote" debug
sessions for this kind of crap?

Fuck no. You don't want to deal with this and that's why it is way
better to build that kind of support into the core where everything
which trips over the issue tells the random driver user/developer
what's wrong.

Dammit. I explained you very detailed WHY this is useful aside of the
inverted logic situation. Is it that hard to understand?

This usecase clearly shows a shortcoming at the core level along with
a potential for consolidation. So why are you trying to convince me
that this can be solved with some DT/device drivers hackery?

When will you SoC folks ever understand that nothing on your
SoCs/board is special? I'm telling that you for years but you simply
refuse to get a gripe.

Thanks,

	tglx



 

Re: [PATCH V3 4/5] mfd: palmas: support IRQ inversion at the board level

[Alexandre Courbot]

Stephen, any news about this patch? I'm waiting for it to be merged in
order to resend some Tegra DTs, but still cannot see it in -next.

Thanks,
Alex.

On Tue, Mar 4, 2014 at 4:51 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Implement the new DT property ti,irq-externally-inverted, and add an
> equivalent platform data field to match. This allows the driver to
> correctly automatically configure the IRQ output polarity when the board
> or SoC contains an inverter between the Palmas IRQ output and IRQ
> controller input.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v3:
> * Use an IRQD flag to represent the data, rather than a custom DT property.
> v2:
> * Split V1's patch 1/2 into separate patches 1/3 and 2/3.
> ---
>  drivers/mfd/palmas.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> index d280d789e55a..3281d4103e9d 100644
> --- a/drivers/mfd/palmas.c
> +++ b/drivers/mfd/palmas.c
> @@ -292,7 +292,8 @@ static int palmas_set_pdata_irq_flag(struct i2c_client *i2c,
>                 return -EINVAL;
>         }
>
> -       pdata->irq_flags = irqd_get_trigger_type(irq_data);
> +       pdata->irq_flags = irqd_get_trigger_type(irq_data) |
> +                          irqd_get_src_dst_inverted(irq_data);
>         dev_info(&i2c->dev, "Irq flag is 0x%08x\n", pdata->irq_flags);
>         return 0;
>  }
> @@ -447,6 +448,8 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
>                 reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
>         else
>                 reg = 0;
> +       if (pdata->irq_flags & IRQ_SRC_DST_INVERTED)
> +               reg ^= PALMAS_POLARITY_CTRL_INT_POLARITY;
>         ret = palmas_update_bits(palmas, PALMAS_PU_PD_OD_BASE,
>                         PALMAS_POLARITY_CTRL, PALMAS_POLARITY_CTRL_INT_POLARITY,
>                         reg);
> --
> 1.8.1.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 4/5] mfd: palmas: support IRQ inversion at the board level

[Lee Jones]

> Stephen, any news about this patch? I'm waiting for it to be merged in
> order to resend some Tegra DTs, but still cannot see it in -next.

I doubt it's been applied.

No one got back to me with reference to how to deal with the set [1],
so I certainly didn't apply it.

[1] http://www.spinics.net/lists/arm-kernel/msg312694.html

> On Tue, Mar 4, 2014 at 4:51 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > From: Stephen Warren <swarren@nvidia.com>
> >
> > Implement the new DT property ti,irq-externally-inverted, and add an
> > equivalent platform data field to match. This allows the driver to
> > correctly automatically configure the IRQ output polarity when the board
> > or SoC contains an inverter between the Palmas IRQ output and IRQ
> > controller input.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > ---
> > v3:
> > * Use an IRQD flag to represent the data, rather than a custom DT property.
> > v2:
> > * Split V1's patch 1/2 into separate patches 1/3 and 2/3.
> > ---
> >  drivers/mfd/palmas.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> > index d280d789e55a..3281d4103e9d 100644
> > --- a/drivers/mfd/palmas.c
> > +++ b/drivers/mfd/palmas.c
> > @@ -292,7 +292,8 @@ static int palmas_set_pdata_irq_flag(struct i2c_client *i2c,
> >                 return -EINVAL;
> >         }
> >
> > -       pdata->irq_flags = irqd_get_trigger_type(irq_data);
> > +       pdata->irq_flags = irqd_get_trigger_type(irq_data) |
> > +                          irqd_get_src_dst_inverted(irq_data);
> >         dev_info(&i2c->dev, "Irq flag is 0x%08x\n", pdata->irq_flags);
> >         return 0;
> >  }
> > @@ -447,6 +448,8 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
> >                 reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
> >         else
> >                 reg = 0;
> > +       if (pdata->irq_flags & IRQ_SRC_DST_INVERTED)
> > +               reg ^= PALMAS_POLARITY_CTRL_INT_POLARITY;
> >         ret = palmas_update_bits(palmas, PALMAS_PU_PD_OD_BASE,
> >                         PALMAS_POLARITY_CTRL, PALMAS_POLARITY_CTRL_INT_POLARITY,
> >                         reg);
> >

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH V3 4/5] mfd: palmas: support IRQ inversion at the board level

[Stephen Warren]

On 04/10/2014 01:04 AM, Alexandre Courbot wrote:
> Stephen, any news about this patch? I'm waiting for it to be merged in
> order to resend some Tegra DTs, but still cannot see it in -next.

To be honest, this became too painful to work on just to fix a warning
at boot which has no practical side-effect in our case. The best thing
to do is just merge your DTs for 3.16, and keep ignoring the warning
just like we always have on Dalmore. Hopefully at some point I (or
someone) will run out of higher priority work items and get back to this.

> Thanks,
> Alex.
> 
> On Tue, Mar 4, 2014 at 4:51 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Implement the new DT property ti,irq-externally-inverted, and add an
>> equivalent platform data field to match. This allows the driver to
>> correctly automatically configure the IRQ output polarity when the board
>> or SoC contains an inverter between the Palmas IRQ output and IRQ
>> controller input.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> v3:
>> * Use an IRQD flag to represent the data, rather than a custom DT property.
>> v2:
>> * Split V1's patch 1/2 into separate patches 1/3 and 2/3.
>> ---
>>  drivers/mfd/palmas.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
>> index d280d789e55a..3281d4103e9d 100644
>> --- a/drivers/mfd/palmas.c
>> +++ b/drivers/mfd/palmas.c
>> @@ -292,7 +292,8 @@ static int palmas_set_pdata_irq_flag(struct i2c_client *i2c,
>>                 return -EINVAL;
>>         }
>>
>> -       pdata->irq_flags = irqd_get_trigger_type(irq_data);
>> +       pdata->irq_flags = irqd_get_trigger_type(irq_data) |
>> +                          irqd_get_src_dst_inverted(irq_data);
>>         dev_info(&i2c->dev, "Irq flag is 0x%08x\n", pdata->irq_flags);
>>         return 0;
>>  }
>> @@ -447,6 +448,8 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
>>                 reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
>>         else
>>                 reg = 0;
>> +       if (pdata->irq_flags & IRQ_SRC_DST_INVERTED)
>> +               reg ^= PALMAS_POLARITY_CTRL_INT_POLARITY;
>>         ret = palmas_update_bits(palmas, PALMAS_PU_PD_OD_BASE,
>>                         PALMAS_POLARITY_CTRL, PALMAS_POLARITY_CTRL_INT_POLARITY,
>>                         reg);
>> --
>> 1.8.1.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH V3 4/5] mfd: palmas: support IRQ inversion at the board level

[Alexandre Courbot]

On Fri, Apr 11, 2014 at 12:42 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/10/2014 01:04 AM, Alexandre Courbot wrote:
>> Stephen, any news about this patch? I'm waiting for it to be merged in
>> order to resend some Tegra DTs, but still cannot see it in -next.
>
> To be honest, this became too painful to work on just to fix a warning
> at boot which has no practical side-effect in our case. The best thing
> to do is just merge your DTs for 3.16, and keep ignoring the warning
> just like we always have on Dalmore. Hopefully at some point I (or
> someone) will run out of higher priority work items and get back to this.

Ok, fine by me. I will wait for -rc1, see if I can enable more devices
with 3.16 and send you the updated DTs soon after.

Thanks,
Alex.