| 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
On Wed, Nov 20, 2024 at 11:05:42AM +0100, Paul Kocialkowski wrote: > Le Wed 20 Nov 24, 09:01, Maxime Ripard a écrit : > > > > 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. Note that this statement would be true for any default. The current, the one you suggest, or any other really. The fact that we have a way to override it is an acknowledgement that it's not a one size fits all situation. > The fact that I'm the first person to bring it up in 12 years doesn't > change that. Sure. It does however hint that it seems like it's a sane enough default. > 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. It's not about good or bad design. Buttons bounce, HPD signals bounce, it's just the world we live in. But let me rephrase if my main objection wasn't clear enough: you want to introduce an ABI breaking change. With the possibility of breaking devices that have worked fine so far. That's not ok. Maxime
Hi Maxime, Le Fri 29 Nov 24, 16:37, Maxime Ripard a écrit : > On Wed, Nov 20, 2024 at 11:05:42AM +0100, Paul Kocialkowski wrote: > > Le Wed 20 Nov 24, 09:01, Maxime Ripard a écrit : > > > > > 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. > > Note that this statement would be true for any default. The current, the > one you suggest, or any other really. The fact that we have a way to > override it is an acknowledgement that it's not a one size fits all > situation. Again the debate is about which option has the best advantages over disadvantages. I'm not saying the default I suggest has no issues, but rather that the benefits very clearly outweight the issues. Hence the many situations that would be supported with the shortest debouncing period (both short and frequent interrupts) versus the few broken-hardware use-cases (related to interrupt storms) support with the largest period. > > The fact that I'm the first person to bring it up in 12 years doesn't > > change that. > > Sure. It does however hint that it seems like it's a sane enough > default. Or maybe people didn't realize this mechanism existed, failed to understand why their device didn't work with Allwinner platforms and just moved on to use something else. Indeed it's all very subjective interpretation. > > 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. > > It's not about good or bad design. Buttons bounce, HPD signals bounce, > it's just the world we live in. Well I'm an electrical engineer and the first thing we were told about buttons and connectors is to include hardware debouncing. The second thing is that it can be done in software (which again is done in a number of drivers) by just disabling the interrupt for a while if it happens too often. So I'm quite affirmative that taking none of these into account is constitutive of a broken hardware design. No electrical engineer is told that they shouldn't care about this because the SoC will filter interrupts for them. Of course it's fine to use this mechanism when it exists, but it's not a reasonable expectation to just assume it will always be there. This is why I think it's not a legitimate reason to make it a default. > But let me rephrase if my main objection wasn't clear enough: you want > to introduce an ABI breaking change. With the possibility of breaking > devices that have worked fine so far. That's not ok. I believe it is highly questionable that this constitutes ABI breakage. To me there was no defined behavior in the first place, since the debouncing configuration is inherited either from the reset value or the boot stage. There is also no formal indication of what the default is, anywhere. Changing the default configuration of hardware is commonplace. One might for example introduce a reset to a driver to get a clean state because it happened that some boot software would mess it up and it went unnoticed for a while. Would you also call that ABI breakage? I think there's a number of situations where it's much more sensible to change a default state to avoid very visible and practical issues. And it does happen. Also my understanding of the "ABI breakage" rule in the kernel is that no userspace program should stop working when it was implemented to (correctly) follow some kernel-related ABI. It doesn't mean that we cannot change any default state, or that everything the kernel does should remain exactly the same ad vitam. Sometimes it just makes a lot more sense to have these fine adjustments. And honestly I would be extremely surprised that this specific change resulted in any actual system breakage, as we can be hopeful that electrical engineers did their jobs correctly. Cheers, Paul
On Sat, Nov 30, 2024 at 11:34:08AM +0100, Paul Kocialkowski wrote: > Hi Maxime, > > Le Fri 29 Nov 24, 16:37, Maxime Ripard a écrit : > > On Wed, Nov 20, 2024 at 11:05:42AM +0100, Paul Kocialkowski wrote: > > > Le Wed 20 Nov 24, 09:01, Maxime Ripard a écrit : > > > > > > 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. > > > > Note that this statement would be true for any default. The current, the > > one you suggest, or any other really. The fact that we have a way to > > override it is an acknowledgement that it's not a one size fits all > > situation. > > Again the debate is about which option has the best advantages over > disadvantages. I'm not saying the default I suggest has no issues, but > rather that the benefits very clearly outweight the issues. Hence the many > situations that would be supported with the shortest debouncing period (both > short and frequent interrupts) versus the few broken-hardware use-cases > (related to interrupt storms) support with the largest period. > > > > The fact that I'm the first person to bring it up in 12 years doesn't > > > change that. > > > > Sure. It does however hint that it seems like it's a sane enough > > default. > > Or maybe people didn't realize this mechanism existed, failed to understand > why their device didn't work with Allwinner platforms and just moved on to > use something else. Indeed it's all very subjective interpretation. > > > > 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. > > > > It's not about good or bad design. Buttons bounce, HPD signals bounce, > > it's just the world we live in. > > Well I'm an electrical engineer and the first thing we were told about > buttons and connectors is to include hardware debouncing. The second thing > is that it can be done in software (which again is done in a number of drivers) > by just disabling the interrupt for a while if it happens too often. > > So I'm quite affirmative that taking none of these into account is constitutive > of a broken hardware design. No electrical engineer is told that they shouldn't > care about this because the SoC will filter interrupts for them. The SoC provides the hardware debouncing. There's no reason not to use it, or to choose something redundant. Some might, but it's also perfectly valid to just rely on the SoC there. > Of course it's fine to use this mechanism when it exists, but it's not a > reasonable expectation to just assume it will always be there. This is why > I think it's not a legitimate reason to make it a default. Nobody ever designed a board without considering the SoC features but rather by adhering to a dogma. The SoC features, components chosen and their price, etc. all play a role. > > But let me rephrase if my main objection wasn't clear enough: you want > > to introduce an ABI breaking change. With the possibility of breaking > > devices that have worked fine so far. That's not ok. > > I believe it is highly questionable that this constitutes ABI breakage. > To me there was no defined behavior in the first place, since the debouncing > configuration is inherited either from the reset value or the boot stage. > There is also no formal indication of what the default is, anywhere. Depending on the interpretation, it either means that you change the default, or add a default, to a device-tree property. That constitutes an ABI breakage on its own right. And then we can introduce regressions for boards, which is another breakage. > Changing the default configuration of hardware is commonplace. One might > for example introduce a reset to a driver to get a clean state because it > happened that some boot software would mess it up and it went unnoticed for > a while. Would you also call that ABI breakage? No, because it doesn't require a change in the default state Linux expects when it boots, or changing anything in the device tree. It's a self-contained change, and thus there's no interface to break. > I think there's a number of situations where it's much more sensible to change > a default state to avoid very visible and practical issues. And it does happen. > > Also my understanding of the "ABI breakage" rule in the kernel is that no > userspace program should stop working when it was implemented to (correctly) > follow some kernel-related ABI. It doesn't mean that we cannot change any > default state If applications rely on that default one way or another, it's absolutely what it means. The only criteria is whether this will create a regression for any application. Maxime
Hi, Le Mon 02 Dec 24, 12:03, Maxime Ripard a écrit : > On Sat, Nov 30, 2024 at 11:34:08AM +0100, Paul Kocialkowski wrote: > > Well I'm an electrical engineer and the first thing we were told about > > buttons and connectors is to include hardware debouncing. The second thing > > is that it can be done in software (which again is done in a number of drivers) > > by just disabling the interrupt for a while if it happens too often. > > > > So I'm quite affirmative that taking none of these into account is constitutive > > of a broken hardware design. No electrical engineer is told that they shouldn't > > care about this because the SoC will filter interrupts for them. > > The SoC provides the hardware debouncing. There's no reason not to use > it, or to choose something redundant. Some might, but it's also > perfectly valid to just rely on the SoC there. > > > Of course it's fine to use this mechanism when it exists, but it's not a > > reasonable expectation to just assume it will always be there. This is why > > I think it's not a legitimate reason to make it a default. > > Nobody ever designed a board without considering the SoC features but > rather by adhering to a dogma. The SoC features, components chosen and > their price, etc. all play a role. Okay that's fair enough. I guess what I had in mind was that it would be unusual and unlikely. I don't think it disqualifies the fact that it would be sensible to enable this behavior with the property and not the other way round though. > > > But let me rephrase if my main objection wasn't clear enough: you want > > > to introduce an ABI breaking change. With the possibility of breaking > > > devices that have worked fine so far. That's not ok. > > > > I believe it is highly questionable that this constitutes ABI breakage. > > To me there was no defined behavior in the first place, since the debouncing > > configuration is inherited either from the reset value or the boot stage. > > There is also no formal indication of what the default is, anywhere. > > Depending on the interpretation, it either means that you change the > default, or add a default, to a device-tree property. That constitutes > an ABI breakage on its own right. And then we can introduce regressions > for boards, which is another breakage. It feels very questionable that adding a default over undefined behavior constitutes an ABI breakage. > > Changing the default configuration of hardware is commonplace. One might > > for example introduce a reset to a driver to get a clean state because it > > happened that some boot software would mess it up and it went unnoticed for > > a while. Would you also call that ABI breakage? > > No, because it doesn't require a change in the default state Linux > expects when it boots, or changing anything in the device tree. It's a > self-contained change, and thus there's no interface to break. It could definitely result in changes in behavior though. One could imagine the case of a sensor that was configured with 4x gain by the boot software, which wasn't reset by the driver. Now when the driver is updated to include a reset the sensor will fall back to 1x gain, with a direct result on the values that are read with default configuration. This would however be perfectly fine since it wasn't specified anywhere that the sensor should report values with 4x gain in its default state. It's a behavior change that has direct effect, but doesn't break any ABI at all. > > I think there's a number of situations where it's much more sensible to change > > a default state to avoid very visible and practical issues. And it does happen. > > > > Also my understanding of the "ABI breakage" rule in the kernel is that no > > userspace program should stop working when it was implemented to (correctly) > > follow some kernel-related ABI. It doesn't mean that we cannot change any > > default state > > If applications rely on that default one way or another, it's absolutely > what it means. The only criteria is whether this will create a > regression for any application. For any application *that uses the ABI correctly*. Any example of defaults that are inherited from the boot phase clearly shows that it is not a reasonable expectation to rely on them and doing so is not correct use of the ABI. It is rather very clearly relying on undefined behavior. Cheers, 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,