Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Coiby Xu <coiby.xu@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	"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: Tue, 3 Nov 2020 11:12:08 +0100
Message-ID: <2f4706a1-502f-75f0-9596-cc25b4933b6c@redhat.com> (raw)
In-Reply-To: <20201103000507.ufzukd2vkb5h2e3b@Rk>

Hi,

On 11/3/20 1:05 AM, Coiby Xu wrote:
> On Tue, Oct 27, 2020 at 11:09:11AM +0100, Hans de Goede wrote:
>> Hi,
>>
> ...
>>
>> So I see 2 ways to move forward with his:
>>
>> 1. Just disable the debounce filter for level type IRQs; or
>> 2. Add a helper to sanitize the debounce pulse-duration setting and
>>   call that when setting the IRQ type.
>>   This helper would read the setting check it is not crazy long for
>>   an IRQ-line (lets say anything above 1 ms is crazy long) and if it
>>   is crazy long then overwrite it with a saner value.
>>
>> 2. is a bit tricky, because if the IRQ line comes from a chip then
>> obviously max 1ms debouncing to catch eletrical interference should be
>> fine. But sometimes cheap buttons for things like volume up/down on tablets
>> are directly connected to GPIOs and then we may want longer debouncing...
>>
>> So if we do 2. we may want to limit it to only level type IRQs too.
>>
>> Note I have contacted AMD about this and asked them for some input on this,
>> ideally they can tell us how exactly we should program the debounce filter
>> and based on which data we should do that.
> 
> Is there any update from AMD?

I'm afraid not.

> Based on the discussion, I'm going to
> submit a patch to disable debounce filter for both level and edge
> type IRQs, i.e. to remove relevant code in amd_gpio_irq_set_type of
> drivers/pinctrl/pinctrl-amd.c since setting debounce filter is
> orthogonal to setting irq type and Andy has submitted the patch to
> support setting debounce setting supplied by ACPI in gpiolib-acpi.c

Ok.

> Btw, did you contact AMD through a representative?

Yes I'm using Red Hat's contacts in to AMD's server department,
which are putting me in contact with AMD'se client department.

> Obviously CC them
> didn't get their attention. There is an inconsistency for configuring
> debounce timeout in pinctrl-amd as was spotted by Andy [1]. I also need
> their feedback for this matter.
> 
> [1] https://lore.kernel.org/patchwork/comment/1522675/

This is a case where Andy is obviously right and you should just use the
higher precision "unit = 15625" value (except probably that is wrong too,
see below).

We have had similar issues with the docs for getting the TSC frequency
on some Intel chips, where the docs said 16.6 MHz for a certain register
value, where what they meant was 100/6 MHz which really is significantly
different. This was leading to a time drift of 5 minutes / day on non
networked (so no NTP) Linux systems.

I think this is what Andy was referring to when he wrote:
"What the heck with HW companies! (Just an emotion based on the experience)"

So the lesson learned there is when you can be reasonable certain that
the value really is a/b and the resulting digits of the value in the
hw doc match that taking the lousy precision into account then you
should probably assume the value really is a/b and not the lousy
precision value given in the docs (or the code comment in this case).

I mean 15.6 msec has 3 significant numbers, that gives an imprecision /
error of approx. 1000 ppm where as a decent clock crystal is in the order
of 50 ppm, so the hardware has a drift / error of approx. 50 ppm which
makes using a value with an error of 1000 ppm in the code really really
bad.

Actually all the values look somewhat suspect. The comment:

>                 Debounce        Debounce        Timer   Max
>                 TmrLarge        TmrOutUnit      Unit    Debounce
>                                                         Time
>                 0       0       61 usec (2 RtcClk)      976 usec
>                 0       1       244 usec (8 RtcClk)     3.9 msec
>                 1       0       15.6 msec (512 RtcClk)  250 msec
>                 1       1       62.5 msec (2048 RtcClk) 1 sec

Helpfully gives the values in RtcClks. A typical RTC clock crystal
is 32 KHz which gives us 31.25 usec per tick, so I would expect the
values to be:

                 0       0       62.500 usec (2 RtcClk)      
                 0       1       250.00 usec (8 RtcClk)    
                 1       0       16.000 msec (512 RtcClk) 
                 1       1       64.000 msec (2048 RtcClk)

And the max multiplier seems to be 15, not 16 as is used for the
Max Debounce Time's in the comment, so those are wrong too. I have
a feeling the table was build the wrong way around (minus the
RtcClk parts). They started with a Max Debounce Time of 1 sec, then
divided that by 16 given them 62.5 msec, where as in reality
we have 2048 ticks of a 32 KHz clock giving us 64 millisec, etc.

I also wonder if the 0-15 divider really is a 0-15 divider or a
1-16 divider... This suggests:

                if (debounce < 61) {
                        pin_reg |= 1;

It really is a 0-15 divider, so without docs we should just assume
that it is 0-15 for now, which makes the divide 1 second by 16 thing
which got them 62.5 msec (or so I believe) a bit suspect. Either the
divide by 16 is wrong, or the divider really is a 1-16 divider...

Regards,

Hans


  reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-01 13:22 Coiby Xu
2020-10-01 20:57 ` Linus Walleij
2020-10-02  9:40   ` Hans de Goede
2020-10-02 12:42     ` Coiby Xu
2020-10-02 13:36       ` Hans de Goede
2020-10-02 14:51         ` Coiby Xu
2020-10-02 19:44           ` Hans de Goede
2020-10-02 22:45             ` Coiby Xu
2020-10-03 13:22               ` Hans de Goede
2020-10-03 23:03                 ` Coiby Xu
2020-10-04  5:16                   ` Coiby Xu
2020-10-06  4:49                     ` Coiby Xu
2020-10-06  6:28                       ` Hans de Goede
2020-10-06  8:31                         ` Coiby Xu
2020-10-06  8:55                           ` Hans de Goede
2020-10-06  9:28                             ` Hans de Goede
2020-10-06  9:29                               ` Hans de Goede
2020-10-08 16:32                                 ` Coiby Xu
2020-10-14  4:24                                 ` Coiby Xu
2020-10-14 11:34                                   ` Coiby Xu
2020-10-14 11:46                                   ` Hans de Goede
2020-10-15  3:27                                     ` Coiby Xu
2020-10-15  4:06                                     ` Coiby Xu
2020-10-26 22:54                                     ` Coiby Xu
2020-10-27  9:52                                       ` Andy Shevchenko
2020-10-30  4:58                                         ` Coiby Xu
2020-10-27 10:09                                       ` Hans de Goede
2020-10-27 15:13                                         ` Andy Shevchenko
2020-10-27 16:00                                           ` Hans de Goede
2020-10-27 16:09                                             ` Andy Shevchenko
2020-10-29  8:04                                               ` Mika Westerberg
2020-10-30  4:54                                               ` Coiby Xu
2020-11-02 19:06                                                 ` Andy Shevchenko
2020-11-02 22:56                                                   ` Coiby Xu
2020-11-03  0:05                                         ` Coiby Xu
2020-11-03 10:12                                           ` Hans de Goede [this message]
2020-11-03 10:49                                             ` Andy Shevchenko
2020-11-03 11:00                                               ` Hans de Goede
2020-10-08 16:26                               ` Coiby Xu
2020-10-06  9:16                           ` Linus Walleij
2020-10-08 16:40                             ` Coiby Xu
2020-10-02 10:59   ` 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=2f4706a1-502f-75f0-9596-cc25b4933b6c@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=Nehal-bakulchandra.Shah@amd.com \
    --cc=Shyam-sundar.S-k@amd.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=availa@outlook.com \
    --cc=coiby.xu@gmail.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    /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

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git