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

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

[Hans de Goede]

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@redhat.com>
---
Changes in v2:
- Add "FW_BUG pin %i: direct_irq_en set but no IRQ assigned, clearing" warning
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 38 ++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 4c01333e1406..a7179aa30b78 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,34 @@ 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;
+			}
+		}
+	}
+
+	dev_warn(vg->dev,
+		 FW_BUG "pin %i: direct_irq_en set but no IRQ assigned, clearing\n", pin);
+	return false;
+}
+
 static void byt_init_irq_valid_mask(struct gpio_chip *chip,
 				    unsigned long *valid_mask,
 				    unsigned int ngpios)
@@ -1492,8 +1521,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

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

[Andy Shevchenko]

On Fri, Jan 07, 2022 at 03:23:43PM +0100, 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:

supports ?

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

...

> +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) {

Can it be like

	u32 direct_irq[16];
	void __iomem *reg;
	void *match;


	memcpy_fromio(...);
	match = memchr(...);
	if (match)
		dev_dbg();
	else
		dev_warn();

	return !!match;

?

> +				dev_dbg(vg->dev, "Pin %i: uses direct IRQ %d (APIC %d)\n",
> +					pin, i + j, 0x43 + i + j);
> +				return true;
> +			}
> +		}
> +	}
> +
> +	dev_warn(vg->dev,
> +		 FW_BUG "pin %i: direct_irq_en set but no IRQ assigned, clearing\n", pin);
> +	return false;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jan 07, 2022 at 06:41:30PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 07, 2022 at 03:23:43PM +0100, Hans de Goede wrote:

...

> > +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) {
> 
> Can it be like
> 
> 	u32 direct_irq[16];
> 	void __iomem *reg;
> 	void *match;
> 
> 
> 	memcpy_fromio(...);
> 	match = memchr(...);
> 	if (match)
> 		dev_dbg();
> 	else
> 		dev_warn();
> 
> 	return !!match;
> 
> ?
> 
> > +				dev_dbg(vg->dev, "Pin %i: uses direct IRQ %d (APIC %d)\n",

> > +					pin, i + j, 0x43 + i + j);

Why 0x43 is hard coded?

> > +				return true;
> > +			}
> > +		}
> > +	}
> > +
> > +	dev_warn(vg->dev,
> > +		 FW_BUG "pin %i: direct_irq_en set but no IRQ assigned, clearing\n", pin);
> > +	return false;
> > +}

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 1/7/22 17:41, Andy Shevchenko wrote:
> On Fri, Jan 07, 2022 at 03:23:43PM +0100, 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:
> 
> supports ?

Ack, fixed for v3.

>> 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.
> 
> ...
> 
>> +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) {
> 
> Can it be like
> 
> 	u32 direct_irq[16];
> 	void __iomem *reg;
> 	void *match;
> 
> 
> 	memcpy_fromio(...);
> 	match = memchr(...);
> 	if (match)
> 		dev_dbg();
> 	else
> 		dev_warn();
> 
> 	return !!match;
> 
> ?

Yes that is a good idea, I've changed to this for v3.

> 
>> +				dev_dbg(vg->dev, "Pin %i: uses direct IRQ %d (APIC %d)\n",
>> +					pin, i + j, 0x43 + i + j);

> Why 0x43 is hard coded?
> 

I noticed that at least for the 'INT33FC:02' controller, which seems
to be the only one on which direct-IRQs get used, the direct-irq 0 slot
corresponds to ACPI resource which points to the APIC IRQ 0x43:
"Interrupt () { 0x00000043 }", and slot 1 points to 0x44, etc.

But I'm not sure what if any the APIC IRQ offset is for the other
GPIO islands, so I've just dropped the  (APIC %d) part of the log
msg for v3.

Regards,

Hans