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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 1/7/22 15:18, Hans de Goede wrote:
> Some boards set the direct_irq_en flag in the conf0 register without
> setting any of the 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 still be set.
> 
> So having the direct_irq_en flag set without any trigger flags is
> non-sense, log a FW_BUG warning when encountering this and clear the 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 support and on success prints debug msg like these:
> 
> byt_gpio INT33FC:02: Pin 0: uses direct IRQ 0 (APIC 67)
> byt_gpio INT33FC:02: Pin 15: uses direct IRQ 2 (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.
> 
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/pinctrl/intel/pinctrl-baytrail.c | 36 ++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 4c01333e1406..dfb54804e6e6 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,32 @@ 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 value)
> +{
> +	void __iomem *reg;
> +	int i, j;
> +
> +	if (!(value & (BYT_TRIG_POS | BYT_TRIG_NEG))) {
> +		dev_warn(vg->dev,
> +			 FW_BUG "pin %i: direct_irq_en set without trigger, clearing\n", pin);
> +		return false;
> +	}
> +
> +	reg = vg->communities->pad_regs + BYT_DIRECT_IRQ_REG;
> +	for (i = 0; i < 16; i += 4) {
> +		value = readl(reg + i);
> +		for (j = 0; j < 4; j++) {
> +			if (((value >> j * 8) & 0xff) == pin) {
> +				dev_dbg(vg->dev, "Pin %i: uses direct IRQ %d (APIC %d)\n",
> +					pin, i + j, 0x43 + i + j);
> +				return true;
> +			}
> +		}
> +	}
> +

Ugh, I just realized that this exit path also needs a dev_warn FW_BUG, I'll prep a v2.

Note I've never seen this path get hit, but still.

Regards,

Hans


> +	return false;
> +}
> +
>  static void byt_init_irq_valid_mask(struct gpio_chip *chip,
>  				    unsigned long *valid_mask,
>  				    unsigned int ngpios)
> @@ -1492,8 +1519,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);
>