Message-ID: <ZzzdT0wr0u1ApVgV@collins>
Date: Tue, 19 Nov 2024 19:47:43 +0100
From: Paul Kocialkowski <paulk@...-base.io>
To: Maxime Ripard <mripard@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Paul Kocialkowski <contact@...lk.fr>
Subject: Re: [PATCH] pinctrl: sunxi: Use minimal debouncing period as default

Le Tue 19 Nov 24, 16:43, Maxime Ripard a écrit :
> On Tue, Nov 19, 2024 at 04:00:48PM +0100, Paul Kocialkowski wrote:
> > Hi Maxime,
> > 
> > Le Tue 19 Nov 24, 15:43, Maxime Ripard a écrit :
> > > On Tue, Nov 19, 2024 at 03:08:05PM +0100, Paul Kocialkowski wrote:
> > > > From: Paul Kocialkowski <contact@...lk.fr>
> > > > 
> > > > The sunxi external interrupts (available from GPIO pins) come with a
> > > > built-in debouncing mechanism that cannot be disabled. It can be
> > > > configured to use either the low-frequency oscillator (32 KHz) or the
> > > > high-frequency oscillator (24 MHz), with a pre-scaler.
> > > > 
> > > > The pinctrl code supports an input-debounce device-tree property to set
> > > > a specific debouncing period and choose which clock source is most
> > > > relevant. However the property is specified in microseconds, which is
> > > > longer than the minimal period achievable from the high-frequency
> > > > oscillator without a pre-scaler.
> > > 
> > > That can be fixed by introducing a new property with a ns resolution.
> > 
> > Sure but my point here is rather about what should be default behavior.
> > 
> > The issue I had will remain unsolved by default even with a new property,
> > since people will still need to patch their device-tree to apply it.
> > 
> > > > When the property is missing, the reset configuration is kept, which
> > > > selects the low-frequency oscillator without pre-scaling. This severely
> > > > limits the possible interrupt periods that can be detected.
> > > > 
> > > > Instead of keeping this default, use the minimal debouncing period from
> > > > the high-frequency oscillator without a pre-scaler to allow the largest
> > > > possible range of interrupt periods.
> > > > 
> > > > This issue was encountered with a peripheral that generates active-low
> > > > interrupts for 1 us. No interrupt was detected with the default setup,
> > > > while it is now correctly detected with this change.
> > > 
> > > I don't think it's wise. If the debouncing is kept as is, the worst case
> > > scenario is the one you had: a device doesn't work, you change it,
> > > everything works.
> > 
> > I think this worst-case scenario is very bad and not what people will
> > expect. In addition it is difficult to debug the issue without specific
> > knowledge about the SoC.
> >
> > My use-case here was hooking up a sparkfun sensor board by the way,
> > not some very advanced corner-case.
> 
> Are you really arguing that a single sparkfun sensor not working is a
> worse outcome than the system not booting?

No, what I'm saying is that this is a very common and basic use-case that
most users will expect to work out-of-the-box.

Also the possibility of interrupt storms happening is nothing new (and it can
still happen with any non-external interrupt). It would typically result from a
hardware-related issue and there's no reason why it would happen on
correctly-designed boards. If anything, it would allow spotting these isues
more easily.

I think it comes down to whether we expect an interrupt controller to "just
report interrupts" or whether it's reasonable that it applies extra policy
to cover for unlikely (yet very problematic) situations. I think it's good
that it supports that, but also that it should not enforce such a
restrictive policy by default.

> > > If we set it up as fast as it can however, then our risk becomes
> > > thousands of spurious interrupts, which is much more detrimental to the
> > > system.
> > 
> > Keep in mind that this only concerns external GPIO-based interrupts,
> > which have to be explicitely hooked to a device. If a device or circuit
> > does generate such spurious interrupts, I think it makes sense that it
> > would be reflected by default.
> 
> I mean... debouncing is here for a reason. Any hardware button will
> generate plenty of interrupts when you press it precisely because it
> bounces.

Well this is why we have both electronics to filter out these frequencies
and code in related drivers to implement such debouncing.

I am not arguing that debouncing is not important, I am saying that it
should not be that agressive on every interrupt line by default.

> > Also the notion of spurious interrupt is pretty vague. Having lots of
> > interrupts happening may be the desired behavior in many cases.
> 
> Which cases?

Any kind of data sampling happening at high-speeds really.
And this situation also concerns interrupts that are short even if not very
frequent. That's a very large scope of use cases.

> > In any case I don't think it makes sense for the platform code to impose
> > what a reasonable period for interrupts is (especially with such a large
> > period as default).
> 
> So you don't think it makes sense for the platform code to impose a
> reasonable period, so you want to impose a (more, obviously) reasonable
> period?

Yes absolutely. Anything that brings us closer to "you get what is really
happening with the hardware". The sunxi controller doesn't allow disabling
debouncing entirely, so the next best thing is to have it with the smallest
period.

> If anything, the status quo doesn't impose anything, it just rolls with
> the hardware default. Yours would impose one though.

The result is that it puts a strong limitation and breaks many use cases by
default. I don't think we have to accept whatever register default was chosen
by hardware engineers as the most sensible default choice and pretend that this
is not a policy decision.

> > Some drivers also have mechanisms to detect spurious interrupts based
> > on their specific use case.
> > 
> > > And that's without accounting the fact that devices might have relied on
> > > that default for years
> > 
> > They definitely shouldn't have. This feels much closer to a bug, and relying
> > on a bug not being fixed is not a reasonable expectation.
> 
> No, it's not a bug, really. It might be inconvenient to you, and that's
> fine, but it's definitely not a bug.

I agree it's not a bug, just a poor default choice that is neither documented
nor explicitely announced. For all we know U-Boot could configure that to
something completely different and that would break the assumption too.

Cheers,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Specialist in multimedia, graphics and embedded hardware support with Linux.

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists