Re: Any other ways to debug GPIO interrupt controller (pinctrl-amd) for broken touchpads of a new laptop model?

From: Coiby Xu <coiby.xu@gmail.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	wang jun <availa@outlook.com>,
	Nehal Shah <Nehal-bakulchandra.Shah@amd.com>,
	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: Any other ways to debug GPIO interrupt controller (pinctrl-amd) for broken touchpads of a new laptop model?
Date: Fri, 9 Oct 2020 00:26:14 +0800	[thread overview]
Message-ID: <20201008162614.md4sw6ciodn7m5iw@Rk> (raw)
In-Reply-To: <4f02cbdf-e1dd-b138-4975-118dd4f86089@redhat.com>

On Tue, Oct 06, 2020 at 11:28:06AM +0200, Hans de Goede wrote:
>Hi,
>
>On 10/6/20 10:55 AM, Hans de Goede wrote:
>>Hi,
>>
>>On 10/6/20 10:31 AM, Coiby Xu wrote:
>>>On Tue, Oct 06, 2020 at 08:28:40AM +0200, Hans de Goede wrote:
>>>>Hi,
>>>>
>>>>On 10/6/20 6:49 AM, Coiby Xu wrote:
>>>>>Hi Hans and Linus,
>>>>>
>>>>>I've found the direct evidence proving the GPIO interrupt controller is
>>>>>malfunctioning.
>>>>>
>>>>>I've found a way to let the GPIO chip trigger an interrupt by accident
>>>>>when playing with the GPIO sysfs interface,
>>>>>
>>>>> - export pin130 which is used by the touchad
>>>>> - set the direction to be "out"
>>>>> - `echo 0 > value` will trigger the GPIO controller's parent irq and
>>>>>   "echo 1 > value" will make it stop firing
>>>>>
>>>>>(I'm not sure if this is yet another bug of the GPIO chip. Anyway I can
>>>>>manually trigger an interrupt now.)
>>>>>
>>>>>I wrote a C program is to let GPIO controller quickly generate some
>>>>>interrupts then disable the firing of interrupts by toggling pin#130's
>>>>>value with an specified time interval, i.e., set the value to 0 first
>>>>>and then after some time, re-set the value to 1. There is no interrupt
>>>>>firing unless time internal > 120ms (~7Hz). This explains why we can
>>>>>only see 7 interrupts for the GPIO controller's parent irq.
>>>>
>>>>That is a great find, well done.
>>>>
>>>>>My hypothesis is the GPIO doesn't have proper power setting so it stays
>>>>>in an idle state or its clock frequency is too low by default thus not
>>>>>quick enough to read interrupt input. Then pinctrl-amd must miss some
>>>>>code to configure the chip and I need a hardware reference manual of this
>>>>>GPIO chip (HID: AMDI0030) or reverse-engineer the driver for Windows
>>>>>since I couldn't find a copy of reference manual online? What would you
>>>>>suggest?
>>>>
>>>>This sounds like it might have something to do with the glitch filter.
>>>>The code in pinctrl-amd.c to setup the trigger-type also configures
>>>>the glitch filter, you could try changing that code to disable the
>>>>glitch-filter. The defines for setting the glitch-filter bits to
>>>>disabled are already there.
>>>>
>>>
>>>Disabling the glitch filter works like a charm! Other enthusiastic
>>>Linux users who have been troubled by this issue for months would
>>>also feel great to know this small tweaking could bring their
>>>touchpad back to life:) Thank you!
>>
>>That is good to hear, I'm glad that we have finally found a solution.
>>
>>>$ git diff
>>>diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
>>>index 9a760f5cd7ed..e786d779d6c8 100644
>>>--- a/drivers/pinctrl/pinctrl-amd.c
>>>+++ b/drivers/pinctrl/pinctrl-amd.c
>>>@@ -463,7 +463,7 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>>                 pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
>>>                 pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF;
>>>                 pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
>>>-               pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF;
>>>+               /** pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF; */
>>>                 irq_set_handler_locked(d, handle_level_irq);
>>>                 break;
>>>
>>>I will learn more about the glitch filter and the implementation of
>>>pinctrl and see if I can disable glitch filter only for this touchpad.
>>
>>The glitch filter likely also has settings for how long a glitch
>>lasts, which apparently goes all the way up to 120ms. If it would
>>only delay reporting by say 0.1ms and consider any pulse longer
>>then 0.1s not a glitch, then having it enabled would be fine.
>>
>>I don't think we want some sort of quirk here to only disable the
>>glitch filter for some touchpads. One approach might be to simply
>>disable it completely for level type irqs.
>>
>>What we really need here is some input from AMD engineers with how
>>this is all supposed to work.
>>
>>E.g. maybe the glitch-filter is setup by the BIOS and we should not
>>touch it all ?
>>
>>Or maybe instead of DB_TYPE_PRESERVE_HIGH_GLITCH low level interrupts
>>should use DB_TYPE_PRESERVE_LOW_GLITCH ?   Some docs for the hw
>>would really help here ...
>
>So I've been digging through the history of the pinctrl-amd.c driver
>and once upon a time it used to set a default debounce time of
>2.75 ms.
>
>See the patch generated by doing:
>
>git format-patch 8cf4345575a416e6856a6856ac6eaa31ad883126~..8cf4345575a416e6856a6856ac6eaa31ad883126
>
>In a linux kernel checkout.
>
>So it would be interesting to add a debugging printk to see
>what the value of pin_reg & DB_TMR_OUT_MASK is for the troublesome
>GPIO.
>
>I guess that it might be all 1s (0xfffffffff) or some such which
>might be a way to check that we should disable the glitch-filter
>for this pin?
>

I guess you mean the value of "pin_reg" (not
"pin_reg & DB_TMR_OUT_MASK"), then the value is 0x500e8. So it means
BIOS or Bootloader has set the debounce for us.

>Regards,
>
>Hans
>

--
Best regards,
Coiby