Re: [PATCH] ARM: dts: allwinner: minimize irq debounce filter per default

[Andre Przywara]

On Sat, 11 Feb 2023 13:45:37 -0600
Samuel Holland <samuel@xxxxxxxxxxxx> wrote:

Hi Samuel,

> On 2/9/23 14:29, Andre Przywara wrote:
> > On Wed, 8 Feb 2023 13:50:04 +0100
> > Andreas Feldner <andreas@xxxxxxxxxxxxx> wrote:
> > 
> > Hi Andreas,
> > 
> > CC:ing Maxime, who wrote the debouncing code back then.
> >   
> >> Am 07.02.23 um 02:16 schrieb Andre Przywara:  
> >>> On Mon, 6 Feb 2023 20:51:50 +0100
> >>> Andreas Feldner <pelzi@xxxxxxxxxxxxxxx> wrote:
> >>>
> >>> Hi Andreas,
> >>>
> >>> thanks for taking care about this board and sending patches!    
> >> Thank YOU for maintaining it!  
> >>>> The SoC features debounce logic for external interrupts. Per default,
> >>>> this is based on a 32kHz oscillator, in effect filtering away multiple
> >>>> interrupts separated by less than roughly 100�s.
> >>>>
> >>>> This patch sets different defaults for this filter for this board:
> >>>> PG is connected to non-mechanical components, without any risk for
> >>>> showing bounces. PA is mostly exposed to GPIO pins, however the
> >>>> existence of a debounce filter is undesirable as well if electronic
> >>>> components are connected.    
> >>> So how do you know if that's the case? It seems to be quite normal to
> >>> just connect mechanical switches to GPIO pins.
> >>>
> >>> If you are trying to fix a particular issue you encountered, please
> >>> describe that here, and say how (or at least that) the patch fixes it.
> >>>
> >>> And I would suggest to treat port G and port A differently. If you
> >>> need a lower debounce threshold for port A, you can apply a DT overlay
> >>> in U-Boot, just for your board.    
> >>
> >> Fair enough. You run into problems when you connect (electronic)
> >> devices to bank A (typically by the 40pin CON2 connector), where
> >> the driver requires fast IRQs to work. In my case this has been a
> >> DHT22 sensor, and the default debounce breaking the dht11.ko
> >> driver.  
> > 
> > Sure, what I meant is that this is a property of your particular
> > setup (because you attach something to the *headers*) , so it shouldn't
> > be in the generic DT, but just in your copy. Which ideally means using
> > a DT overlay.
> >   
> >> Now, what kind of problem is this - I'm no way sure:
> >>
> >> a) is it an unlucky default, because whoever connects a mechanical
> >> switch will know about the problem of bouncing and be taking
> >> care to deal with it (whereas at least I was complete unsuspecting
> >> when connecting an electronic device that a debounce function
> >> might be in place), or  
> > 
> > The Linux default is basically the reset default: just leave the
> > register at 0x0. It seems like you cannot really turn that off at all
> > in hardware, and the reset setting is indeed 32KHz/1. So far there
> > haven't been any complaints, though I don't know if people just
> > don't use it in anger, or cannot be bothered to send a report to the
> > list.
> >   
> >> b) is it a bug in the devicetree for (at least) the BananaPi M2 Zero,
> >> because the IRQ bank G is hard wired to electronic devices that
> >> should not be fenced by a debouncing function, or  
> > 
> > Well, we could try to turn that "off" as much as possible, but on the
> > other hand the debounce only affects *GPIO* *interrupts*, so not sure
> > that gives us anything. The PortG pins are used for the SDIO Wifi, BT
> > UART, and the wakeup pins for the Wifi chip. Only the wakeup pins would
> > be affected, and I doubt that we wake up that often that it matters. If
> > you've made other observations, please let me know.
> > 
> > Certainly no board with an in-tree DT sets the debounce property, which
> > means everyone uses 32KHz/1, and also did so before the functionality
> > was introduced.  
> 
> One side note relevant to wakeup pins: if the debounce clock source is
> set to HOSC, and the 24 MHz oscillator is disabled, then IRQs for those
> pins will never fire.

That's a good point.

> Currently, Crust does not check the debounce configuration when deciding
> if it can turn off the 24 MHz crystal during system suspend (or fake-off
> on boards without PMICs), so any wakeup-capable GPIOs need to use LOSC
> as their debounce clock.
> 
> Do you have any thoughts about if/how we should handle this
> automatically? Should Linux (or Crust) override the debounce
> configuration when entering suspend?

My feeling is since it's Crust's decision to disable the 24MHz
oscillator, it should make sure that's a workable configuration. So it
would be Crust's responsibility to avoid using 24 MHz as the debounce
clock, I'd say. There could be an argument about Linux re-initialising
the GPIO during resume, but that wouldn't help you anyway.

> I imagine no wakeup source will
> require a particularly short debounce time.

Since it all seems to work fine with the current 32KHz/1 setup, I
wouldn't expect any issues due to too short pulses being eaten by the
debouncing logic. And a too short debounce period shouldn't matter for
wakeup either, since it's the first edge that counts.

Cheers,
Andre