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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

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