[PATCH v4] pinctrl: baytrail: Clear direct_irq_en flag on broken configs

* [PATCH v4] pinctrl: baytrail: Clear direct_irq_en flag on broken configs
@ 2022-01-12 21:02 Hans de Goede
  2022-01-13  8:29 ` Mika Westerberg
  2022-01-24 14:31 ` Andy Shevchenko
  0 siblings, 2 replies; 3+ messages in thread
From: Hans de Goede @ 2022-01-12 21:02 UTC (permalink / raw)
  To: Mika Westerberg, Andy Shevchenko, Bartosz Golaszewski, Linus Walleij
  Cc: Hans de Goede, linux-gpio

Some boards set the direct_irq_en flag in the conf0 register without
setting the correct trigger bits. The direct_irq_en flag just means that
the GPIO will send IRQs directly to the APIC instead of going through
the shared interrupt for the GPIO controller, in order for the pin to be
able to actually generate IRQs the trigger flags must configure the IRQ
as a level-high or level-low active IRQ.

Note testing shows that using edge trigger add the conf0 register level
does NOT work, instead edge triggering should be set at the IO-APIC level.

I believe that the direct_irq_en flag connects the output of the GPIO's IRQ
trigger block, which normally sets the status flag in the IRQ status reg at
0x800 to one of the IO-APIC pins according to the direct IRQ mux.

This means that the TRIG_LVL bit *must* be set, so that the GPIO's input
value is directly passed (1:1 or inverted) to the IO-APIC pin, if TRIG_LVL
is not set, selecting edge mode operation then on the first edge the
selected IO-APIC pin goes high, but since no write-to-clear write will be
done to the IRQ status reg at 0x800, the detected edge condition will never
get cleared.

This APIC pin stuck high condition can be observed with the pin configured
as level-high active, in the form of an interrupt storm. Clearing the
TRIG_MASK bits of conf0 stops the storm, reconfiguring them as edge again
results in a storm again as soon as the edge is triggered once.

Detect invalid trigger flags, log a FW_BUG warning when encountering this
and clear the direct_irq_en flag so that a driver can actually use the pin
as IRQ through gpiod_to_irq().

Specifically this allows the edt-ft5x06 touchscreen driver to use
INT33FC:02 pin 3 as touchscreen IRQ on the Nextbook Ares 8 tablet,
accompanied by the following new log message

byt_gpio INT33FC:02: [Firmware Bug]: pin 3: direct_irq_en set without trigger, clearing

The new byt_direct_irq_sanity_check() function also checks that the
pin is actually appointed to one of the 16 direct-IRQs which the GPIO
controller supports and on success prints debug messages like these:

byt_gpio INT33FC:02: Pin 0: uses direct IRQ 0 (IO-APIC 67)
byt_gpio INT33FC:02: Pin 15: uses direct IRQ 2 (IO-APIC 69)

This is useful to figure out the GPIO pin belonging to ACPI
resources like this one: "Interrupt () { 0x00000043 }" or
the other way around.

The strict checking of valid trigger flags this introduces does result in
FW_BUG messages on quite a few devices. E.g. on the Yoga Tablet 2 1051L:
 byt_gpio INT33FC:00: [Firmware Bug]: pin 92: direct_irq_en set but no IRQ assigned, clearing
 byt_gpio INT33FC:00: [Firmware Bug]: pin 93: direct_irq_en set but no IRQ assigned, clearing
  These 2 also have mux set to 7 and fall + rise + level trigger bits set,
  presumably something has written 0xffffffff to their conf0 registers
 byt_gpio INT33FC:02: Pin 3: uses direct IRQ 1 (IO-APIC 68)
 byt_gpio INT33FC:02: [Firmware Bug]: pin 3: direct_irq_en set without trigger (conf0: 2803cc00h), clearing
  Most tablets seem to have this, looking at DSDTs this seems intended for
  use with an I2C HID sensor-hub and is still set on devices without one.

To make sure this does not cause any regressions this has been tested,
including checking disabled direct-IRQs are not used in the DSDT,
on the following devices:

Asus ME176C
Asus TF103C
Chuwi Vi10 (with its Windows BIOS)
HP x2 10-n000nd
Lenovo Yoga Tablet 2 1050L (Android version, without EC, with buggy DSDT)
Lenovo Yoga Tablet 2 1051L (Windows version, with EC)

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Only level-high or level-low active conf0 trigger settings are valid for
  direct IRQs, make byt_direct_irq_sanity_check() check this
- Add a comment explaining why this is done, and also update the commit msg

Changes in v3:
- Rework code to check if the pin is assigned one of the 16 direct IRQs
  (new code suggested-by Andy)
- Drop dev_dbg of the (likely?) APIC IRQ, only log the direct IRQ index

Changes in v2:
- Add "FW_BUG pin %i: direct_irq_en set but no IRQ assigned, clearing" warning
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 55 +++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 4c01333e1406..ceee6c65dbc1 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -32,6 +32,7 @@
 #define BYT_VAL_REG		0x008
 #define BYT_DFT_REG		0x00c
 #define BYT_INT_STAT_REG	0x800
+#define BYT_DIRECT_IRQ_REG	0x980
 #define BYT_DEBOUNCE_REG	0x9d0
 
 /* BYT_CONF0_REG register bits */
@@ -1465,6 +1466,51 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
 	chip->irq_eoi(data);
 }
 
+static bool byt_direct_irq_sanity_check(struct intel_pinctrl *vg, int pin, u32 conf0)
+{
+	int direct_irq, ioapic_direct_irq_base;
+	u8 *match, direct_irq_mux[16];
+	u32 trig;
+
+	memcpy_fromio(direct_irq_mux, vg->communities->pad_regs + BYT_DIRECT_IRQ_REG,
+		      sizeof(direct_irq_mux));
+	match = memchr(direct_irq_mux, pin, sizeof(direct_irq_mux));
+	if (!match) {
+		dev_warn(vg->dev, FW_BUG "pin %i: direct_irq_en set but no IRQ assigned, clearing\n", pin);
+		return false;
+	}
+
+	direct_irq = match - direct_irq_mux;
+	/* Base IO-APIC pin numbers come from atom-e3800-family-datasheet.pdf */
+	ioapic_direct_irq_base = (vg->communities->npins == BYT_NGPIO_SCORE) ? 51 : 67;
+	dev_dbg(vg->dev, "Pin %i: uses direct IRQ %d (IO-APIC %d)\n", pin,
+		direct_irq, direct_irq + ioapic_direct_irq_base);
+
+	/*
+	 * Testing has shown that the way direct IRQs work is that the combination of the
+	 * direct-irq-en flag and the direct IRQ mux connect the output of the GPIO's IRQ
+	 * trigger block, which normally sets the status flag in the IRQ status reg at
+	 * 0x800, to one of the IO-APIC pins according to the mux registers.
+	 *
+	 * This means that:
+	 * 1. The TRIG_MASK bits must be set to configure the GPIO's IRQ trigger block
+	 * 2. The TRIG_LVL bit *must* be set, so that the GPIO's input value is directly
+	 *    passed (1:1 or inverted) to the IO-APIC pin, if TRIG_LVL is not set,
+	 *    selecting edge mode operation then on the first edge the IO-APIC pin goes
+	 *    high, but since no write-to-clear write will be done to the IRQ status reg
+	 *    at 0x800, the detected edge condition will never get cleared.
+	 */
+	trig = conf0 & BYT_TRIG_MASK;
+	if (trig != (BYT_TRIG_POS | BYT_TRIG_LVL) &&
+	    trig != (BYT_TRIG_NEG | BYT_TRIG_LVL)) {
+		dev_warn(vg->dev, FW_BUG "pin %i: direct_irq_en set without trigger (conf0: %xh), clearing\n",
+			 pin, conf0);
+		return false;
+	}
+
+	return true;
+}
+
 static void byt_init_irq_valid_mask(struct gpio_chip *chip,
 				    unsigned long *valid_mask,
 				    unsigned int ngpios)
@@ -1492,8 +1538,13 @@ static void byt_init_irq_valid_mask(struct gpio_chip *chip,
 
 		value = readl(reg);
 		if (value & BYT_DIRECT_IRQ_EN) {
-			clear_bit(i, valid_mask);
-			dev_dbg(vg->dev, "excluding GPIO %d from IRQ domain\n", i);
+			if (byt_direct_irq_sanity_check(vg, i, value)) {
+				clear_bit(i, valid_mask);
+			} else {
+				value &= ~(BYT_DIRECT_IRQ_EN | BYT_TRIG_POS |
+					   BYT_TRIG_NEG | BYT_TRIG_LVL);
+				writel(value, reg);
+			}
 		} else if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i)) {
 			byt_gpio_clear_triggering(vg, i);
 			dev_dbg(vg->dev, "disabling GPIO %d\n", i);
-- 
2.33.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread