| Message ID | 20241119140805.3345412-1-paulk@sys-base.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: sunxi: Use minimal debouncing period as default | expand |
On Tue, Nov 19, 2024 at 03:08:05PM +0100, Paul Kocialkowski wrote: > From: Paul Kocialkowski <contact@paulk.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. > 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. 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. And that's without accounting the fact that devices might have relied on that default for years Maxime
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@paulk.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. > 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. Also the notion of spurious interrupt is pretty vague. Having lots of interrupts happening may be the desired behavior in many 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). 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. Cheers, Paul
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@paulk.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? > > 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. > Also the notion of spurious interrupt is pretty vague. Having lots of > interrupts happening may be the desired behavior in many cases. Which 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? If anything, the status quo doesn't impose anything, it just rolls with the hardware default. Yours would impose one though. > 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. Maxime
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c index 73bcf806af0e..06c650d97645 100644 --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c @@ -1416,6 +1416,7 @@ static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl, unsigned int hosc_diff, losc_diff; unsigned int hosc_div, losc_div; struct clk *hosc, *losc; + bool debounce_minimal = false; u8 div, src; int i, ret; @@ -1423,9 +1424,9 @@ static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl, if (of_clk_get_parent_count(node) != 3) return 0; - /* If we don't have any setup, bail out */ + /* If we don't have any setup, use minimal debouncing. */ if (!of_property_present(node, "input-debounce")) - return 0; + debounce_minimal = true; losc = devm_clk_get(pctl->dev, "losc"); if (IS_ERR(losc)) @@ -1439,29 +1440,37 @@ static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl, unsigned long debounce_freq; u32 debounce; - ret = of_property_read_u32_index(node, "input-debounce", - i, &debounce); - if (ret) - return ret; + if (!debounce_minimal) { + ret = of_property_read_u32_index(node, "input-debounce", + i, &debounce); + if (ret) + return ret; - if (!debounce) - continue; + if (!debounce) + continue; - debounce_freq = DIV_ROUND_CLOSEST(USEC_PER_SEC, debounce); - losc_div = sunxi_pinctrl_get_debounce_div(losc, - debounce_freq, - &losc_diff); + debounce_freq = DIV_ROUND_CLOSEST(USEC_PER_SEC, + debounce); - hosc_div = sunxi_pinctrl_get_debounce_div(hosc, - debounce_freq, - &hosc_diff); + losc_div = sunxi_pinctrl_get_debounce_div(losc, + debounce_freq, + &losc_diff); - if (hosc_diff < losc_diff) { - div = hosc_div; - src = 1; + hosc_div = sunxi_pinctrl_get_debounce_div(hosc, + debounce_freq, + &hosc_diff); + + if (hosc_diff < losc_diff) { + div = hosc_div; + src = 1; + } else { + div = losc_div; + src = 0; + } } else { - div = losc_div; - src = 0; + /* Achieve minimal debouncing using undivided hosc. */ + div = 0; + src = 1; } writel(src | div << 4,