All of lore.kernel.org
 help / color / mirror / Atom feed
From: Coiby Xu <coiby.xu@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	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, 30 Oct 2020 12:54:21 +0800	[thread overview]
Message-ID: <20201030045421.si3r5zsbeeyhkwns@Rk> (raw)
In-Reply-To: <CAHp75VcEr0AcdM2+ig5neMQn+5cfuttpFuAak7CkSO-qVjPeCw@mail.gmail.com>

On Tue, Oct 27, 2020 at 06:09:49PM +0200, Andy Shevchenko wrote:
>On Tue, Oct 27, 2020 at 6:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 10/27/20 4:13 PM, Andy Shevchenko wrote:
>> > On Tue, Oct 27, 2020 at 4:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> >> On 10/26/20 11:54 PM, Coiby Xu wrote:
>> >>> Hi Hans and Linus,
>> >>>
>> >>> Will you interpret the 0x0000 value for debounce timeout in GPIO
>> >>> Interrupt Connection Resource Descriptor as disabling debouncing
>> >>> filter?
>> >>>
>> >>> GpioInt (EdgeLevel, ActiveLevel, Shared, PinConfig, DebounceTimeout, ResourceSource,
>> >>> ResourceSourceIndex, ResourceUsage, DescriptorName, VendorData) {PinList}
>> >>>
>> >>> I'm not sure if Windows' implementation is the de facto standard like
>> >>> i2c-hid. But if we are going to conform to the ACPI specs and we would
>> >>> regard 0x0000 debounce timeout as disabling debouncing filter, then we
>> >>> can fix this touchpad issue and potentially some related issues by
>> >>> implementing the feature of supporting configuring debounce timeout in
>> >>> drivers/gpio/gpiolib-acpi.c and removing all debounce filter
>> >>> configuration in amd_gpio_irq_set_type of drivers/pinctrl/pinctrl-amd.c.
>> >>> What do you think?
>> >>>
>> >>> A favorable evidence is I've collected five DSDT tables when
>> >>> investigating this issue. All 5 DSDT tables have an GpioInt specifying
>> >>> an non-zero debounce timeout value for the edge type irq and for all
>> >>> the level type irq, the debounce timeout is set to 0x0000.
>> >>
>> >> That is a very interesting observation and this matches with my
>> >> instincts which say that we should just disable the debounce filter
>> >> for level triggered interrupts in pinctrl-amd.c
>> >>
>> >> Yes that is a bit of a shortcut vs reading the valie from the ACPI
>> >> table, but I'm not sure that 0 always means disabled.
>> >>
>> >> Specifically the ACPI 6.2 spec also has a notion of pinconf settings
>> >> and the docs on "PinConfig()"  say:
>> >>
>> >> Note: There is some overlap between the properties set by GpioIo/GpioInt/ PinFunction and
>> >> PinConfig descriptors. For example, both are setting properties such as pull-ups. If the same
>> >> property is specified by multiple descriptors for the same pins, the order in which these properties
>> >> are applied is undetermined. To avoid any conflicts, GpioInt/GpioIo/PinFunction should provide a
>> >> default value for these properties when PinConfig is used. If PinConfig is used to set pin bias,
>> >> PullDefault should be used for GpioIo/GpioInt/ PinFunction. *If PinConfig is used to set debounce
>> >> timeout, 0 should be used for GpioIo/GpioInt.*
>> >>
>> >> So that suggests that a value of 0 does not necessarily mean "disabled" but
>> >> it means use a default, or possibly get the value from somewhere else such
>> >> as from a ACPI PinConfig description (if present).
>> >
>> > Nope, it was added to get rid of disambiguation when both Gpio*() and
>> > PinConfig() are given.
>> > So, 0 means default *if and only if* PinConfig() is present.
>> >
>> > I.o.w. the OS layers should do this:
>> >
>> >  - if Gpio*() provides Debounce != 0, we use it, otherwise
>> >  - if PinConfig() is present for this pin with a debounce set, use it, otherwise
>> >  - debounce is disabled.
>> >
>> > Now we missed a midentry implementation in the Linux kernel, hence go
>> > to last, i.e. disable debounce.
>> > But it should be rather done in gpiolib-acpi.c.
>> >
>> > Hope this helps.
>> >
>> > I Cc'ed this to Mika as co-author of that part of specification, he
>> > may correct me if I'm wrong.
>>
>> I see, so then the right thing to do for the bug which we are seeing
>> on some AMD platforms would be to honor the debounce setting I guess ?
>>
>> Can you and/or Mika write a patch(set) for this ?
>
>I will look at it, but meanwhile I would postpone until having a
>Mika's Ack on the action that my understanding and course of actions
>is correct.
>
If you don't mind, let me write this patch(set) instead:) I feel itchy
to fix this touchpad issue by myself after spending about a month of
my internship at Linux Foundation investigating this touchpad issue.
There are many enthusiastic Linux users waiting to get their touchpads
fixed and I could prioritize this task since I don't have other
obligations. I have provided a fallback solution [1] to save their
touchpads but it seems patches on gpiolib-acpi.c and pinctrl-amd could
reach mainline kernel much earlier.

[1] https://lore.kernel.org/patchwork/patch/1323245/
>> > P.S. Does RedHat have a representative in ASWG?
>>
>> I think so yes, but mainly focussed on server related things I guess...
>
>
>--
>With Best Regards,
>Andy Shevchenko

--
Best regards,
Coiby

WARNING: multiple messages have this Message-ID (diff)
From: Coiby Xu <coiby.xu@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	linux-kernel-mentees@lists.linuxfoundation.org,
	Linus Walleij <linus.walleij@linaro.org>,
	wang jun <availa@outlook.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
Subject: Re: [Linux-kernel-mentees] Any other ways to debug GPIO interrupt controller (pinctrl-amd) for broken touchpads of a new laptop model?
Date: Fri, 30 Oct 2020 12:54:21 +0800	[thread overview]
Message-ID: <20201030045421.si3r5zsbeeyhkwns@Rk> (raw)
In-Reply-To: <CAHp75VcEr0AcdM2+ig5neMQn+5cfuttpFuAak7CkSO-qVjPeCw@mail.gmail.com>

On Tue, Oct 27, 2020 at 06:09:49PM +0200, Andy Shevchenko wrote:
>On Tue, Oct 27, 2020 at 6:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 10/27/20 4:13 PM, Andy Shevchenko wrote:
>> > On Tue, Oct 27, 2020 at 4:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> >> On 10/26/20 11:54 PM, Coiby Xu wrote:
>> >>> Hi Hans and Linus,
>> >>>
>> >>> Will you interpret the 0x0000 value for debounce timeout in GPIO
>> >>> Interrupt Connection Resource Descriptor as disabling debouncing
>> >>> filter?
>> >>>
>> >>> GpioInt (EdgeLevel, ActiveLevel, Shared, PinConfig, DebounceTimeout, ResourceSource,
>> >>> ResourceSourceIndex, ResourceUsage, DescriptorName, VendorData) {PinList}
>> >>>
>> >>> I'm not sure if Windows' implementation is the de facto standard like
>> >>> i2c-hid. But if we are going to conform to the ACPI specs and we would
>> >>> regard 0x0000 debounce timeout as disabling debouncing filter, then we
>> >>> can fix this touchpad issue and potentially some related issues by
>> >>> implementing the feature of supporting configuring debounce timeout in
>> >>> drivers/gpio/gpiolib-acpi.c and removing all debounce filter
>> >>> configuration in amd_gpio_irq_set_type of drivers/pinctrl/pinctrl-amd.c.
>> >>> What do you think?
>> >>>
>> >>> A favorable evidence is I've collected five DSDT tables when
>> >>> investigating this issue. All 5 DSDT tables have an GpioInt specifying
>> >>> an non-zero debounce timeout value for the edge type irq and for all
>> >>> the level type irq, the debounce timeout is set to 0x0000.
>> >>
>> >> That is a very interesting observation and this matches with my
>> >> instincts which say that we should just disable the debounce filter
>> >> for level triggered interrupts in pinctrl-amd.c
>> >>
>> >> Yes that is a bit of a shortcut vs reading the valie from the ACPI
>> >> table, but I'm not sure that 0 always means disabled.
>> >>
>> >> Specifically the ACPI 6.2 spec also has a notion of pinconf settings
>> >> and the docs on "PinConfig()"  say:
>> >>
>> >> Note: There is some overlap between the properties set by GpioIo/GpioInt/ PinFunction and
>> >> PinConfig descriptors. For example, both are setting properties such as pull-ups. If the same
>> >> property is specified by multiple descriptors for the same pins, the order in which these properties
>> >> are applied is undetermined. To avoid any conflicts, GpioInt/GpioIo/PinFunction should provide a
>> >> default value for these properties when PinConfig is used. If PinConfig is used to set pin bias,
>> >> PullDefault should be used for GpioIo/GpioInt/ PinFunction. *If PinConfig is used to set debounce
>> >> timeout, 0 should be used for GpioIo/GpioInt.*
>> >>
>> >> So that suggests that a value of 0 does not necessarily mean "disabled" but
>> >> it means use a default, or possibly get the value from somewhere else such
>> >> as from a ACPI PinConfig description (if present).
>> >
>> > Nope, it was added to get rid of disambiguation when both Gpio*() and
>> > PinConfig() are given.
>> > So, 0 means default *if and only if* PinConfig() is present.
>> >
>> > I.o.w. the OS layers should do this:
>> >
>> >  - if Gpio*() provides Debounce != 0, we use it, otherwise
>> >  - if PinConfig() is present for this pin with a debounce set, use it, otherwise
>> >  - debounce is disabled.
>> >
>> > Now we missed a midentry implementation in the Linux kernel, hence go
>> > to last, i.e. disable debounce.
>> > But it should be rather done in gpiolib-acpi.c.
>> >
>> > Hope this helps.
>> >
>> > I Cc'ed this to Mika as co-author of that part of specification, he
>> > may correct me if I'm wrong.
>>
>> I see, so then the right thing to do for the bug which we are seeing
>> on some AMD platforms would be to honor the debounce setting I guess ?
>>
>> Can you and/or Mika write a patch(set) for this ?
>
>I will look at it, but meanwhile I would postpone until having a
>Mika's Ack on the action that my understanding and course of actions
>is correct.
>
If you don't mind, let me write this patch(set) instead:) I feel itchy
to fix this touchpad issue by myself after spending about a month of
my internship at Linux Foundation investigating this touchpad issue.
There are many enthusiastic Linux users waiting to get their touchpads
fixed and I could prioritize this task since I don't have other
obligations. I have provided a fallback solution [1] to save their
touchpads but it seems patches on gpiolib-acpi.c and pinctrl-amd could
reach mainline kernel much earlier.

[1] https://lore.kernel.org/patchwork/patch/1323245/
>> > P.S. Does RedHat have a representative in ASWG?
>>
>> I think so yes, but mainly focussed on server related things I guess...
>
>
>--
>With Best Regards,
>Andy Shevchenko

--
Best regards,
Coiby
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

  parent reply	other threads:[~2020-10-30  4:54 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-01 13:22 Any other ways to debug GPIO interrupt controller (pinctrl-amd) for broken touchpads of a new laptop model? Coiby Xu
2020-10-01 13:22 ` [Linux-kernel-mentees] " Coiby Xu
2020-10-01 20:57 ` Linus Walleij
2020-10-01 20:57   ` [Linux-kernel-mentees] " Linus Walleij
2020-10-02  9:40   ` Hans de Goede
2020-10-02  9:40     ` [Linux-kernel-mentees] " Hans de Goede
2020-10-02 12:42     ` Coiby Xu
2020-10-02 12:42       ` [Linux-kernel-mentees] " Coiby Xu
2020-10-02 13:36       ` Hans de Goede
2020-10-02 13:36         ` [Linux-kernel-mentees] " Hans de Goede
2020-10-02 14:51         ` Coiby Xu
2020-10-02 14:51           ` [Linux-kernel-mentees] " Coiby Xu
2020-10-02 19:44           ` Hans de Goede
2020-10-02 19:44             ` [Linux-kernel-mentees] " Hans de Goede
2020-10-02 22:45             ` Coiby Xu
2020-10-02 22:45               ` [Linux-kernel-mentees] " Coiby Xu
2020-10-03 13:22               ` Hans de Goede
2020-10-03 13:22                 ` [Linux-kernel-mentees] " Hans de Goede
2020-10-03 23:03                 ` Coiby Xu
2020-10-03 23:03                   ` [Linux-kernel-mentees] " Coiby Xu
2020-10-04  5:16                   ` Coiby Xu
2020-10-04  5:16                     ` [Linux-kernel-mentees] " Coiby Xu
2020-10-06  4:49                     ` Coiby Xu
2020-10-06  4:49                       ` [Linux-kernel-mentees] " Coiby Xu
2020-10-06  6:28                       ` Hans de Goede
2020-10-06  6:28                         ` [Linux-kernel-mentees] " Hans de Goede
2020-10-06  8:31                         ` Coiby Xu
2020-10-06  8:31                           ` [Linux-kernel-mentees] " Coiby Xu
2020-10-06  8:55                           ` Hans de Goede
2020-10-06  8:55                             ` [Linux-kernel-mentees] " Hans de Goede
2020-10-06  9:28                             ` Hans de Goede
2020-10-06  9:28                               ` [Linux-kernel-mentees] " Hans de Goede
2020-10-06  9:29                               ` Hans de Goede
2020-10-06  9:29                                 ` [Linux-kernel-mentees] " Hans de Goede
2020-10-08 16:32                                 ` Coiby Xu
2020-10-08 16:32                                   ` [Linux-kernel-mentees] " Coiby Xu
2020-10-14  4:24                                 ` Coiby Xu
2020-10-14  4:24                                   ` [Linux-kernel-mentees] " Coiby Xu
2020-10-14 11:34                                   ` Coiby Xu
2020-10-14 11:34                                     ` [Linux-kernel-mentees] " Coiby Xu
2020-10-14 11:46                                   ` Hans de Goede
2020-10-14 11:46                                     ` [Linux-kernel-mentees] " Hans de Goede
2020-10-15  3:27                                     ` Coiby Xu
2020-10-15  3:27                                       ` [Linux-kernel-mentees] " Coiby Xu
2020-10-15  4:06                                     ` Coiby Xu
2020-10-15  4:06                                       ` [Linux-kernel-mentees] " Coiby Xu
2020-10-26 22:54                                     ` Coiby Xu
2020-10-26 22:54                                       ` [Linux-kernel-mentees] " Coiby Xu
2020-10-27  9:52                                       ` Andy Shevchenko
2020-10-27  9:52                                         ` [Linux-kernel-mentees] " Andy Shevchenko
2020-10-30  4:58                                         ` Coiby Xu
2020-10-30  4:58                                           ` [Linux-kernel-mentees] " Coiby Xu
2020-10-27 10:09                                       ` Hans de Goede
2020-10-27 10:09                                         ` [Linux-kernel-mentees] " Hans de Goede
2020-10-27 15:13                                         ` Andy Shevchenko
2020-10-27 15:13                                           ` [Linux-kernel-mentees] " Andy Shevchenko
2020-10-27 16:00                                           ` Hans de Goede
2020-10-27 16:00                                             ` [Linux-kernel-mentees] " Hans de Goede
2020-10-27 16:09                                             ` Andy Shevchenko
2020-10-27 16:09                                               ` [Linux-kernel-mentees] " Andy Shevchenko
2020-10-29  8:04                                               ` Mika Westerberg
2020-10-29  8:04                                                 ` [Linux-kernel-mentees] " Mika Westerberg
2020-10-30  4:54                                               ` Coiby Xu [this message]
2020-10-30  4:54                                                 ` Coiby Xu
2020-11-02 19:06                                                 ` Andy Shevchenko
2020-11-02 19:06                                                   ` [Linux-kernel-mentees] " Andy Shevchenko
2020-11-02 22:56                                                   ` Coiby Xu
2020-11-02 22:56                                                     ` [Linux-kernel-mentees] " Coiby Xu
2020-11-03  0:05                                         ` Coiby Xu
2020-11-03  0:05                                           ` [Linux-kernel-mentees] " Coiby Xu
2020-11-03 10:12                                           ` Hans de Goede
2020-11-03 10:12                                             ` [Linux-kernel-mentees] " Hans de Goede
2020-11-03 10:49                                             ` Andy Shevchenko
2020-11-03 10:49                                               ` [Linux-kernel-mentees] " Andy Shevchenko
2020-11-03 11:00                                               ` Hans de Goede
2020-11-03 11:00                                                 ` [Linux-kernel-mentees] " Hans de Goede
2020-10-08 16:26                               ` Coiby Xu
2020-10-08 16:26                                 ` [Linux-kernel-mentees] " Coiby Xu
2020-10-06  9:16                           ` Linus Walleij
2020-10-06  9:16                             ` [Linux-kernel-mentees] " Linus Walleij
2020-10-08 16:40                             ` Coiby Xu
2020-10-08 16:40                               ` [Linux-kernel-mentees] " Coiby Xu
2020-10-02 10:59   ` Coiby Xu
2020-10-02 10:59     ` [Linux-kernel-mentees] " Coiby Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201030045421.si3r5zsbeeyhkwns@Rk \
    --to=coiby.xu@gmail.com \
    --cc=Nehal-bakulchandra.Shah@amd.com \
    --cc=Shyam-sundar.S-k@amd.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=availa@outlook.com \
    --cc=hdegoede@redhat.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=mika.westerberg@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.