From 689e008877402564ce8a7884f21c9d2ed3ecb2dc Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 12 Jan 2022 22:02:00 +0100 Subject: [PATCH] pinctrl: baytrail: Clear direct_irq_en flag on broken configs 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 Signed-off-by: Hans de Goede Acked-by: Mika Westerberg Signed-off-by: Andy Shevchenko --- 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);