| 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
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@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? 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
On Tue, Nov 19, 2024 at 07:47:43PM +0100, Paul Kocialkowski wrote: > > > 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. That's an opinion, not a fact. > > 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. You're making it much worse than it is. It doesn't "break many use cases" it broke one, by default, with a supported way to unbreak it, in 12 years. Maxime
Le Wed 20 Nov 24, 09:01, Maxime Ripard a écrit : > On Tue, Nov 19, 2024 at 07:47:43PM +0100, Paul Kocialkowski wrote: > > > > 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. > > That's an opinion, not a fact. Yes and I'm trying to explain why I believe this is the most sensible opinion here. I'm not saying everyone *has to* agree with me. > > > 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. > > You're making it much worse than it is. It doesn't "break many use > cases" it broke one, by default, with a supported way to unbreak it, in > 12 years. I think this is exaggerated. Like I mentioned previously there are *many* situations that are not covered by the default. The fact that I'm the first person to bring it up in 12 years doesn't change that. Sofar the downside you brought up boils down to: badly-designed hardware may have relied on this mechanism to avoid interrupt storms that could prevent the system from booting. If that happens people can always increase the debouncing time with the property as they need. My point here is that the property should be used to accommodate for broken hardware and that the default should work for most use cases, including things as simple as an off-the-shelf sensor board. That's all. Paul
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,