ARM: dts: allwinner: minimize irq debounce filter per default

Commit Message

Andreas Feldner Feb. 6, 2023, 7:51 p.m. UTC

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.

Additionally, the clock-frequency attribute is added for each of
the 4 cores to eliminate the kernel error message on boot, that
the attribute is missing.

Signed-off-by: Andreas Feldner <[e-mail]>
---
 .../dts/sun8i-h2-plus-bananapi-m2-zero.dts     | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Andre Przywara Feb. 7, 2023, 1:16 a.m. UTC | #1

On Mon, 6 Feb 2023 20:51:50 +0100
Andreas Feldner <[e-mail]> wrote:

Hi Andreas,

thanks for taking care about this board and sending patches!

> 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.

> Additionally, the clock-frequency attribute is added for each of
> the 4 cores to eliminate the kernel error message on boot, that
> the attribute is missing.
> 
> Signed-off-by: Andreas Feldner <[e-mail]>
> ---
>  .../dts/sun8i-h2-plus-bananapi-m2-zero.dts     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> index d729b7c705db..1fc0d5d1e51a 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> @@ -113,6 +113,22 @@ wifi_pwrseq: wifi_pwrseq {
>  
>  &cpu0 {
>  	cpu-supply = <&reg_vdd_cpux>;
> +	clock-frequency = <1296000000>;

I see where you are coming from, this is really an unnecessary warning
message. However this message should be really removed from the kernel
instead of adding some rather meaningless value here.
The current DT spec marks this property as required, though, so I added
a PR there to get this fixed:
https://github.com/devicetree-org/devicetree-specification/pull/61
Once this is through, we can try to remove the kernel message.

> +};
> +
> +&cpu1 {
> +	cpu-supply = <&reg_vdd_cpux>;

I don't think we need this for every core?

> +	clock-frequency = <1296000000>;
> +};
> +
> +&cpu2 {
> +	cpu-supply = <&reg_vdd_cpux>;
> +	clock-frequency = <1296000000>;
> +};
> +
> +&cpu3 {
> +	cpu-supply = <&reg_vdd_cpux>;
> +	clock-frequency = <1296000000>;
>  };
>  
>  &de {
> @@ -193,6 +209,8 @@ bluetooth {
>  };
>  
>  &pio {
> +	/* 1�s debounce filter on both IRQ banks */

Is that supposed to be <micro> in UTF-8? It seems to have got lost in
translation, or is that just me?

> +	input-debounce = <1 1>;

As mentioned above, I am not so sure this is generic enough to put it
here for PA. And what is the significance of "1 us", in particular? Is
that just the smallest value?

Cheers,
Andre

>  	gpio-line-names =
>  		/* PA */
>  		"CON2-P13", "CON2-P11", "CON2-P22", "CON2-P15",

Andreas Feldner Feb. 8, 2023, 12:50 p.m. UTC | #2

Hi Andre,

Am 07.02.23 um 02:16 schrieb Andre Przywara:
> On Mon, 6 Feb 2023 20:51:50 +0100
> Andreas Feldner <[e-mail]> 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.

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

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

c) is it missing dt binding documentation of the input-debounce
attribute?

Anyway, the combination of these is quite irritating. To me it
seems a sufficiently elegant solution to explicitly include the
setting in the devicetree and leave it to whoever is unhappy
with it, to create a better suited device tree overlay.

>> Additionally, the clock-frequency attribute is added for each of
>> the 4 cores to eliminate the kernel error message on boot, that
>> the attribute is missing.
>>
>> Signed-off-by: Andreas Feldner <[e-mail]>
>> ---
>>   .../dts/sun8i-h2-plus-bananapi-m2-zero.dts     | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
>> index d729b7c705db..1fc0d5d1e51a 100644
>> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
>> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
>> @@ -113,6 +113,22 @@ wifi_pwrseq: wifi_pwrseq {
>>   
>>   &cpu0 {
>>   	cpu-supply = <&reg_vdd_cpux>;
>> +	clock-frequency = <1296000000>;
> I see where you are coming from, this is really an unnecessary warning
> message. However this message should be really removed from the kernel
> instead of adding some rather meaningless value here.
> The current DT spec marks this property as required, though, so I added
> a PR there to get this fixed:
> https://github.com/devicetree-org/devicetree-specification/pull/61
> Once this is through, we can try to remove the kernel message.

OK, so I'll take care to have this change removed from my patch.
I thought so, but then it was the configuration I'd been testing with...

>> +};
>> +
>> +&cpu1 {
>> +	cpu-supply = <&reg_vdd_cpux>;
> I don't think we need this for every core?

I came across a discussion that this was marked required on the
cpu@... level whereas it would make sense on the cpus level. I did
not check if this suggestion was implemented in the meantime,
sorry!

>> +	clock-frequency = <1296000000>;
>> +};
>> +
>> +&cpu2 {
>> +	cpu-supply = <&reg_vdd_cpux>;
>> +	clock-frequency = <1296000000>;
>> +};
>> +
>> +&cpu3 {
>> +	cpu-supply = <&reg_vdd_cpux>;
>> +	clock-frequency = <1296000000>;
>>   };
>>   
>>   &de {
>> @@ -193,6 +209,8 @@ bluetooth {
>>   };
>>   
>>   &pio {
>> +	/* 1�s debounce filter on both IRQ banks */
> Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> translation, or is that just me?
O yes, the Greek character slipped into the comment.
>> +	input-debounce = <1 1>;
> As mentioned above, I am not so sure this is generic enough to put it
> here for PA. And what is the significance of "1 us", in particular? Is
> that just the smallest value?

Yes indeed it's a bit more complicated than I feel it needs to be. The
configuration is taken as microseconds and translated into the best
matching clock and divider by the driver. However, 0 is not translated
to the lowest divider of the high speed clock as would be logical if
you ask for zero microseconds, but to "leave at default". The default
of the board is 0 in the register, translating to lowest divider on the
_low_ speed clock.

To me this is mindboggling.

If you want to keep IRQ bank A as it is today and switch off the
definitely unnecessary (and _potentially_ IRQ eating) debounce off
for bank G only, I'd suggest the following setting:

     input-debounce = <31 1>;

This is because 31 Microseconds is exactly the time that is best
matched by the low speed clock with low divider and translated
to a 0 in the config register by the driver.

The absolutely equivalent setting, with the only drawback that it
would have confused me to death is:

     input-debounce = <0 1>;

(because it skips setting IRQ bank A debouncing, leaving it at 31.25 us)

Or, and that was my suggestion, you set both correctly for
electronic devices and leave the task of switching on debouncing
to the implementors of applications with mechanical switches:

     input-debounce = <1 1>;

To me, any of these being present in the devicetree would have been
of great help, because I would have seen that there is something
to set.


One final question: how would you like this change:

--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -1467,6 +1467,10 @@ static int sunxi_pinctrl_setup_debounce(struct 
sunxi_pinctrl *pctl,
                 writel(src | div << 4,
                        pctl->membase +
sunxi_irq_debounce_reg_from_bank(pctl->desc, i));
+
+               pr_info("Debounce filter for IRQ bank %d configured to "
+                       "%d us (reg %x)\n",
+                       i, debounce, src | div << 4);
         }

         return 0;

It helped me to cross-check what the driver is really doing, and it
again would have helped me with me DHT problem to learn about
the existence of a debouncing filter.

Yours,

Andreas.

Andre Przywara Feb. 9, 2023, 8:29 p.m. UTC | #3

On Wed, 8 Feb 2023 13:50:04 +0100
Andreas Feldner <[e-mail]> 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 <[e-mail]> 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.

I'd say we should try to only fix things that are actually broken: hence
I was asking whether you have seen actual problems. Which apparently you
have, with your sensor, but not on PortG?

> c) is it missing dt binding documentation of the input-debounce
> attribute?

Documentation for what, exactly? The default behaviour? Yes, we should
add that, though not sure that really belongs into the binding.

> Anyway, the combination of these is quite irritating. To me it
> seems a sufficiently elegant solution to explicitly include the
> setting in the devicetree and leave it to whoever is unhappy
> with it, to create a better suited device tree overlay.
> 
> >> Additionally, the clock-frequency attribute is added for each of
> >> the 4 cores to eliminate the kernel error message on boot, that
> >> the attribute is missing.
> >>
> >> Signed-off-by: Andreas Feldner <[e-mail]>
> >> ---
> >>   .../dts/sun8i-h2-plus-bananapi-m2-zero.dts     | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> >> index d729b7c705db..1fc0d5d1e51a 100644
> >> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> >> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> >> @@ -113,6 +113,22 @@ wifi_pwrseq: wifi_pwrseq {
> >>   
> >>   &cpu0 {
> >>   	cpu-supply = <&reg_vdd_cpux>;
> >> +	clock-frequency = <1296000000>;  
> > I see where you are coming from, this is really an unnecessary warning
> > message. However this message should be really removed from the kernel
> > instead of adding some rather meaningless value here.
> > The current DT spec marks this property as required, though, so I added
> > a PR there to get this fixed:
> > https://github.com/devicetree-org/devicetree-specification/pull/61
> > Once this is through, we can try to remove the kernel message.  
> 
> OK, so I'll take care to have this change removed from my patch.
> I thought so, but then it was the configuration I'd been testing with...
> 
> >> +};
> >> +
> >> +&cpu1 {
> >> +	cpu-supply = <&reg_vdd_cpux>;  
> > I don't think we need this for every core?  
> 
> I came across a discussion that this was marked required on the
> cpu@... level whereas it would make sense on the cpus level. I did
> not check if this suggestion was implemented in the meantime,
> sorry!
> 
> >> +	clock-frequency = <1296000000>;
> >> +};
> >> +
> >> +&cpu2 {
> >> +	cpu-supply = <&reg_vdd_cpux>;
> >> +	clock-frequency = <1296000000>;
> >> +};
> >> +
> >> +&cpu3 {
> >> +	cpu-supply = <&reg_vdd_cpux>;
> >> +	clock-frequency = <1296000000>;
> >>   };
> >>   
> >>   &de {
> >> @@ -193,6 +209,8 @@ bluetooth {
> >>   };
> >>   
> >>   &pio {
> >> +	/* 1�s debounce filter on both IRQ banks */  
> > Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> > translation, or is that just me?  
> O yes, the Greek character slipped into the comment.
> >> +	input-debounce = <1 1>;  
> > As mentioned above, I am not so sure this is generic enough to put it
> > here for PA. And what is the significance of "1 us", in particular? Is
> > that just the smallest value?  
> 
> Yes indeed it's a bit more complicated than I feel it needs to be. The
> configuration is taken as microseconds and translated into the best
> matching clock and divider by the driver. However, 0 is not translated
> to the lowest divider of the high speed clock as would be logical if
> you ask for zero microseconds, but to "leave at default". The default
> of the board is 0 in the register, translating to lowest divider on the
> _low_ speed clock.

I'd say the "if (!debounce) continue;" code is just to defend against
the division by zero, which would be the next statement to execute.

We might want to change that to interpret 0 as "lowest possible", which
would be 24MHz/1. Please feel free to send a patch in this regard, and
CC: Maxime, to get some input on that idea.

> To me this is mindboggling.
> 
> If you want to keep IRQ bank A as it is today and switch off the
> definitely unnecessary (and _potentially_ IRQ eating) debounce off
> for bank G only, I'd suggest the following setting:
> 
>      input-debounce = <31 1>;

It should be documented that the effective default is 31, I guess the
binding is the right place.

> This is because 31 Microseconds is exactly the time that is best
> matched by the low speed clock with low divider and translated
> to a 0 in the config register by the driver.
> 
> The absolutely equivalent setting, with the only drawback that it
> would have confused me to death is:
> 
>      input-debounce = <0 1>;
> 
> (because it skips setting IRQ bank A debouncing, leaving it at 31.25 us)
> 
> Or, and that was my suggestion, you set both correctly for
> electronic devices and leave the task of switching on debouncing
> to the implementors of applications with mechanical switches:
> 
>      input-debounce = <1 1>;
> 
> To me, any of these being present in the devicetree would have been
> of great help, because I would have seen that there is something
> to set.
> 
> 
> One final question: how would you like this change:
> 
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -1467,6 +1467,10 @@ static int sunxi_pinctrl_setup_debounce(struct 
> sunxi_pinctrl *pctl,
>                  writel(src | div << 4,
>                         pctl->membase +
> sunxi_irq_debounce_reg_from_bank(pctl->desc, i));
> +
> +               pr_info("Debounce filter for IRQ bank %d configured to "
> +                       "%d us (reg %x)\n",
> +                       i, debounce, src | div << 4);

That looks certainly useful, please send a proper patch.

Cheers,
Andre

>          }
> 
>          return 0;
> 
> It helped me to cross-check what the driver is really doing, and it
> again would have helped me with me DHT problem to learn about
> the existence of a debouncing filter.
> 
> Yours,
> 
> Andreas.
> 
>

Maxime Ripard Feb. 10, 2023, 8:29 a.m. UTC | #4

On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:
> > >>   &pio {
> > >> +	/* 1�s debounce filter on both IRQ banks */
> > > Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> > > translation, or is that just me?
> > O yes, the Greek character slipped into the comment.
> > >> +	input-debounce = <1 1>;
> > > As mentioned above, I am not so sure this is generic enough to put it
> > > here for PA. And what is the significance of "1 us", in particular? Is
> > > that just the smallest value?  
> > 
> > Yes indeed it's a bit more complicated than I feel it needs to be. The
> > configuration is taken as microseconds and translated into the best
> > matching clock and divider by the driver. However, 0 is not translated
> > to the lowest divider of the high speed clock as would be logical if
> > you ask for zero microseconds, but to "leave at default". The default
> > of the board is 0 in the register, translating to lowest divider on the
> > _low_ speed clock.
> 
> I'd say the "if (!debounce) continue;" code is just to defend against
> the division by zero, which would be the next statement to execute.
> 
> We might want to change that to interpret 0 as "lowest possible", which
> would be 24MHz/1. Please feel free to send a patch in this regard, and
> CC: Maxime, to get some input on that idea.

I never had any complaint on that part either, so the default looks sane
to me.

If some board needs a higher debouncing rate, then we should obviously
set it up in the device tree of that board, but changing it for every
user also introduces the risk of breaking other boards that actually
require a lower debouncing frequency.

And any default is always going to be debated, there's one, it seems to
create little controversy, it seems to work in most case, I'd just stick
with it.

> > To me this is mindboggling.
> > 
> > If you want to keep IRQ bank A as it is today and switch off the
> > definitely unnecessary (and _potentially_ IRQ eating) debounce off
> > for bank G only, I'd suggest the following setting:
> > 
> >      input-debounce = <31 1>;
> 
> It should be documented that the effective default is 31, I guess the
> binding is the right place.

Yeah, if the documentation is lacking, we should definitely improve it.

Maxime

Andre Przywara Feb. 10, 2023, 9:44 a.m. UTC | #5

On Fri, 10 Feb 2023 09:29:36 +0100
Maxime Ripard <[e-mail]> wrote:

Hi Maxime,

thanks for the reply!

> On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:
> > > >>   &pio {
> > > >> +	/* 1�s debounce filter on both IRQ banks */  
> > > > Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> > > > translation, or is that just me?  
> > > O yes, the Greek character slipped into the comment.  
> > > >> +	input-debounce = <1 1>;  
> > > > As mentioned above, I am not so sure this is generic enough to put it
> > > > here for PA. And what is the significance of "1 us", in particular? Is
> > > > that just the smallest value?    
> > > 
> > > Yes indeed it's a bit more complicated than I feel it needs to be. The
> > > configuration is taken as microseconds and translated into the best
> > > matching clock and divider by the driver. However, 0 is not translated
> > > to the lowest divider of the high speed clock as would be logical if
> > > you ask for zero microseconds, but to "leave at default". The default
> > > of the board is 0 in the register, translating to lowest divider on the
> > > _low_ speed clock.  
> > 
> > I'd say the "if (!debounce) continue;" code is just to defend against
> > the division by zero, which would be the next statement to execute.
> > 
> > We might want to change that to interpret 0 as "lowest possible", which
> > would be 24MHz/1. Please feel free to send a patch in this regard, and
> > CC: Maxime, to get some input on that idea.  
> 
> I never had any complaint on that part either, so the default looks sane
> to me.
> 
> If some board needs a higher debouncing rate, then we should obviously
> set it up in the device tree of that board, but changing it for every
> user also introduces the risk of breaking other boards that actually
> require a lower debouncing frequency.

Yeah, we definitely should keep the default at 32KHz/1, as this is also
the hardware reset value.

Not sure if you were actually arguing this, but the change I sketched
above (interpreting 0 as 24MHz/1) is separate though, as the current
default is "no DT property", and not 0. There is no input-debounce
property user in the kernel tree at the moment, so we wouldn't break
anyone. The only thing that would change is if a downstream user was
relying on "0" being interpreted as "skip the setup", which isn't
really documented and could be argued to be an implementation detail.

So I'd suggest to implement 0 as "lowest possible", and documenting that
and the 32KHz/1 default if no property is given.

> And any default is always going to be debated, there's one, it seems to
> create little controversy, it seems to work in most case, I'd just stick
> with it.

Agreed.

Cheers,
Andre.

> 
> > > To me this is mindboggling.
> > > 
> > > If you want to keep IRQ bank A as it is today and switch off the
> > > definitely unnecessary (and _potentially_ IRQ eating) debounce off
> > > for bank G only, I'd suggest the following setting:
> > > 
> > >      input-debounce = <31 1>;  
> > 
> > It should be documented that the effective default is 31, I guess the
> > binding is the right place.  
> 
> Yeah, if the documentation is lacking, we should definitely improve it.
> 
> Maxime

Maxime Ripard Feb. 10, 2023, 10:06 a.m. UTC | #6

On Fri, Feb 10, 2023 at 09:44:25AM +0000, Andre Przywara wrote:
> On Fri, 10 Feb 2023 09:29:36 +0100
> Maxime Ripard <[e-mail]> wrote:
> 
> Hi Maxime,
> 
> thanks for the reply!
> 
> > On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:
> > > > >>   &pio {
> > > > >> +	/* 1�s debounce filter on both IRQ banks */  
> > > > > Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> > > > > translation, or is that just me?  
> > > > O yes, the Greek character slipped into the comment.  
> > > > >> +	input-debounce = <1 1>;  
> > > > > As mentioned above, I am not so sure this is generic enough to put it
> > > > > here for PA. And what is the significance of "1 us", in particular? Is
> > > > > that just the smallest value?    
> > > > 
> > > > Yes indeed it's a bit more complicated than I feel it needs to be. The
> > > > configuration is taken as microseconds and translated into the best
> > > > matching clock and divider by the driver. However, 0 is not translated
> > > > to the lowest divider of the high speed clock as would be logical if
> > > > you ask for zero microseconds, but to "leave at default". The default
> > > > of the board is 0 in the register, translating to lowest divider on the
> > > > _low_ speed clock.  
> > > 
> > > I'd say the "if (!debounce) continue;" code is just to defend against
> > > the division by zero, which would be the next statement to execute.
> > > 
> > > We might want to change that to interpret 0 as "lowest possible", which
> > > would be 24MHz/1. Please feel free to send a patch in this regard, and
> > > CC: Maxime, to get some input on that idea.  
> > 
> > I never had any complaint on that part either, so the default looks sane
> > to me.
> > 
> > If some board needs a higher debouncing rate, then we should obviously
> > set it up in the device tree of that board, but changing it for every
> > user also introduces the risk of breaking other boards that actually
> > require a lower debouncing frequency.
> 
> Yeah, we definitely should keep the default at 32KHz/1, as this is also
> the hardware reset value.
> 
> Not sure if you were actually arguing this, but the change I sketched
> above (interpreting 0 as 24MHz/1) is separate though, as the current
> default is "no DT property", and not 0. There is no input-debounce
> property user in the kernel tree at the moment, so we wouldn't break
> anyone. The only thing that would change is if a downstream user was
> relying on "0" being interpreted as "skip the setup", which isn't
> really documented and could be argued to be an implementation detail.
> 
> So I'd suggest to implement 0 as "lowest possible", and documenting that
> and the 32KHz/1 default if no property is given.

Ah, my bad.

There's another thing to consider: there's already a generic per-pin
input-debounce property in pinctrl.

Since we can't control it per pin but per bank, we moved it to the
controller back then, but there's always been this (implicit)
expectation that it was behaving the same way.

And the generic, per-pin, input-debounce documentation says:

> Takes the debounce time in usec as argument or 0 to disable debouncing

I agree that silently ignoring it is not great, but interpreting 0 as
the lowest possible is breaking that behaviour which, I believe, is a
worse outcome.

So I'm not sure what's the best course of action here. Rejecting the
configuration entirely would prevent the entire pinctrl driver from
probing which sounds really bad. Maybe we could just print an error that
we rejected it to make it more obvious?

Maxime

Andre Przywara Feb. 10, 2023, 10:18 a.m. UTC | #7

On Fri, 10 Feb 2023 11:06:20 +0100
Maxime Ripard <[e-mail]> wrote:

> On Fri, Feb 10, 2023 at 09:44:25AM +0000, Andre Przywara wrote:
> > On Fri, 10 Feb 2023 09:29:36 +0100
> > Maxime Ripard <[e-mail]> wrote:
> > 
> > Hi Maxime,
> > 
> > thanks for the reply!
> >   
> > > On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:  
> > > > > >>   &pio {
> > > > > >> +	/* 1�s debounce filter on both IRQ banks */    
> > > > > > Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> > > > > > translation, or is that just me?    
> > > > > O yes, the Greek character slipped into the comment.    
> > > > > >> +	input-debounce = <1 1>;    
> > > > > > As mentioned above, I am not so sure this is generic enough to put it
> > > > > > here for PA. And what is the significance of "1 us", in particular? Is
> > > > > > that just the smallest value?      
> > > > > 
> > > > > Yes indeed it's a bit more complicated than I feel it needs to be. The
> > > > > configuration is taken as microseconds and translated into the best
> > > > > matching clock and divider by the driver. However, 0 is not translated
> > > > > to the lowest divider of the high speed clock as would be logical if
> > > > > you ask for zero microseconds, but to "leave at default". The default
> > > > > of the board is 0 in the register, translating to lowest divider on the
> > > > > _low_ speed clock.    
> > > > 
> > > > I'd say the "if (!debounce) continue;" code is just to defend against
> > > > the division by zero, which would be the next statement to execute.
> > > > 
> > > > We might want to change that to interpret 0 as "lowest possible", which
> > > > would be 24MHz/1. Please feel free to send a patch in this regard, and
> > > > CC: Maxime, to get some input on that idea.    
> > > 
> > > I never had any complaint on that part either, so the default looks sane
> > > to me.
> > > 
> > > If some board needs a higher debouncing rate, then we should obviously
> > > set it up in the device tree of that board, but changing it for every
> > > user also introduces the risk of breaking other boards that actually
> > > require a lower debouncing frequency.  
> > 
> > Yeah, we definitely should keep the default at 32KHz/1, as this is also
> > the hardware reset value.
> > 
> > Not sure if you were actually arguing this, but the change I sketched
> > above (interpreting 0 as 24MHz/1) is separate though, as the current
> > default is "no DT property", and not 0. There is no input-debounce
> > property user in the kernel tree at the moment, so we wouldn't break
> > anyone. The only thing that would change is if a downstream user was
> > relying on "0" being interpreted as "skip the setup", which isn't
> > really documented and could be argued to be an implementation detail.
> > 
> > So I'd suggest to implement 0 as "lowest possible", and documenting that
> > and the 32KHz/1 default if no property is given.  
> 
> Ah, my bad.
> 
> There's another thing to consider: there's already a generic per-pin
> input-debounce property in pinctrl.
> 
> Since we can't control it per pin but per bank, we moved it to the
> controller back then, but there's always been this (implicit)
> expectation that it was behaving the same way.
> 
> And the generic, per-pin, input-debounce documentation says:
> 
> > Takes the debounce time in usec as argument or 0 to disable debouncing  
> 
> I agree that silently ignoring it is not great, but interpreting 0 as
> the lowest possible is breaking that behaviour which, I believe, is a
> worse outcome.

Is it really? If I understand the hardware manuals correctly, we cannot
really turn that feature off, so isn't the lowest possible time period (24
MHz/1 at the moment) the closest we can get to "turn it off"? So
implementing this would bring us actually closer to the documented
behaviour? Or did I get the meaning of this time period wrong?
At least that's my understanding of how it fixed Andreas' problem: 1µs
is still not "off", but much better than the 31µs of the default. The new
0 would then be 0.041µs.

Cheers,
Andre

> So I'm not sure what's the best course of action here. Rejecting the
> configuration entirely would prevent the entire pinctrl driver from
> probing which sounds really bad. Maybe we could just print an error that
> we rejected it to make it more obvious?
> 
> Maxime

Andreas Feldner Feb. 11, 2023, 12:50 p.m. UTC | #8

Am 10.02.23 um 11:18 schrieb Andre Przywara:
> On Fri, 10 Feb 2023 11:06:20 +0100
> Maxime Ripard <[e-mail]> wrote:
>
>> On Fri, Feb 10, 2023 at 09:44:25AM +0000, Andre Przywara wrote:
>>> On Fri, 10 Feb 2023 09:29:36 +0100
>>> Maxime Ripard <[e-mail]> wrote:
>>>
>>> Hi Maxime,
>>>
>>> thanks for the reply!
>>>    
>>>> On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:
>>>>>>>>    &pio {
>>>>>>>> +	/* 1�s debounce filter on both IRQ banks */
>>>>>>> Is that supposed to be <micro> in UTF-8? It seems to have got lost in
>>>>>>> translation, or is that just me?
>>>>>> O yes, the Greek character slipped into the comment.
>>>>>>>> +	input-debounce = <1 1>;
>>>>>>> As mentioned above, I am not so sure this is generic enough to put it
>>>>>>> here for PA. And what is the significance of "1 us", in particular? Is
>>>>>>> that just the smallest value?
>>>>>> Yes indeed it's a bit more complicated than I feel it needs to be. The
>>>>>> configuration is taken as microseconds and translated into the best
>>>>>> matching clock and divider by the driver. However, 0 is not translated
>>>>>> to the lowest divider of the high speed clock as would be logical if
>>>>>> you ask for zero microseconds, but to "leave at default". The default
>>>>>> of the board is 0 in the register, translating to lowest divider on the
>>>>>> _low_ speed clock.
>>>>> I'd say the "if (!debounce) continue;" code is just to defend against
>>>>> the division by zero, which would be the next statement to execute.
>>>>>
>>>>> We might want to change that to interpret 0 as "lowest possible", which
>>>>> would be 24MHz/1. Please feel free to send a patch in this regard, and
>>>>> CC: Maxime, to get some input on that idea.
>>>> I never had any complaint on that part either, so the default looks sane
>>>> to me.
>>>>
>>>> If some board needs a higher debouncing rate, then we should obviously
>>>> set it up in the device tree of that board, but changing it for every
>>>> user also introduces the risk of breaking other boards that actually
>>>> require a lower debouncing frequency.
>>> Yeah, we definitely should keep the default at 32KHz/1, as this is also
>>> the hardware reset value.
>>>
>>> Not sure if you were actually arguing this, but the change I sketched
>>> above (interpreting 0 as 24MHz/1) is separate though, as the current
>>> default is "no DT property", and not 0. There is no input-debounce
>>> property user in the kernel tree at the moment, so we wouldn't break
>>> anyone. The only thing that would change is if a downstream user was
>>> relying on "0" being interpreted as "skip the setup", which isn't
>>> really documented and could be argued to be an implementation detail.
>>>
>>> So I'd suggest to implement 0 as "lowest possible", and documenting that
>>> and the 32KHz/1 default if no property is given.
>> Ah, my bad.
>>
>> There's another thing to consider: there's already a generic per-pin
>> input-debounce property in pinctrl.
>>
>> Since we can't control it per pin but per bank, we moved it to the
>> controller back then, but there's always been this (implicit)
>> expectation that it was behaving the same way.
>>
>> And the generic, per-pin, input-debounce documentation says:
>>
>>> Takes the debounce time in usec as argument or 0 to disable debouncing
>> I agree that silently ignoring it is not great, but interpreting 0 as
>> the lowest possible is breaking that behaviour which, I believe, is a
>> worse outcome.
> Is it really? If I understand the hardware manuals correctly, we cannot
> really turn that feature off, so isn't the lowest possible time period (24
> MHz/1 at the moment) the closest we can get to "turn it off"? So
> implementing this would bring us actually closer to the documented
> behaviour? Or did I get the meaning of this time period wrong?
> At least that's my understanding of how it fixed Andreas' problem: 1µs
> is still not "off", but much better than the 31µs of the default. The new
> 0 would then be 0.041µs.
I would fully agree. There seems to be no way to turn off the debouncing
filter, and in terms of that filter, the lowest possible time is closest 
to "off".
The SoC default is equivalent to 31 us, far, far away from "off", the 
currently
configurable minimum is 1us.

I did a patch that enables to set "0" in the device tree configuration 
and it
takes care not to do a #div0, but to determine the lowest possible time. As
the patch is done in the driver for a device that cannot switch off 
debouncing,
I'd say, the driver patched in that way does its best to come as close 
to the
intended outcome as is possible.

I tested this setting on the Banana M2 Zero board, and it is working (does
the right thing setting the relevant registers to value 0x0001, and the 
board
works in general, w/o producing smoke. (I have no idea how to test if
the debouncing filter is actually faster with setting "0" than with 
setting "1",
I can only confirm it is not significantly slower).

If we can agree on a concrete way to go I'm happy to try to produce a new
patch version. My suggestion from the discussion:

- Change drivers/pinctrl/sunxi/pinctrl-sunxi.c to set the minimum
    possible filter time when input-debounce is configured to "0"
    (corresponding to 1 on the affected hardware register).
    What I don't like is the huge gap between configuration 1 and 0, but
    I have no idea what to do about it without breaking all compatibility.

- in arch/arm/boot/dts/sunxi-h3-h5.dtsi, set input-debounce <31 31>
    corresponding to the default "0" in both affected hardware registers.
    Note that the clocks hosc and losc that make this 31 map to 0 are
    configured exactly here.

- in arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts, set
    input-debounce <31 0> as this board has electronic devices
    attached to bank G and only exposes bank A to its users.
    I'd like to advertise on that one: this board does not require
    debouncing on bank G. Plus it feels the board got more stable
    by this setting: my BananaPi is connected via WiFi (only) and in the
    past it went apparently dead every other day or so. Nothing like
    this happened after switching off input debounce. Anectdotal
    evidence, I know...

- (in my devicetree overlay, I set input-debounce <0 0> to make IRQ
    based drivers like drivers/iio/humidity/dht11.c work on bank A) -
    not part of the patch.

Would that appear right?

Best regards,

Andreas.

PS: Perhaps someone can point me to further reading regarding
drivers for electronic devices attached to GPIO. Assuming I want
to attach a device to a GPIO that is not accidentally covered by
hardware support of the pinctrl subsystem, what options do I
have _apart_ from registering edge IRQs to react on a digital
signal from that device? Isn't it called bit-banging and the
usual technique?

Andre Przywara Feb. 11, 2023, 3:13 p.m. UTC | #9

On Sat, 11 Feb 2023 13:50:54 +0100
[e-mail] wrote:

Hi,

> Am 10.02.23 um 11:18 schrieb Andre Przywara:
> > On Fri, 10 Feb 2023 11:06:20 +0100
> > Maxime Ripard <[e-mail]> wrote:
> >  
> >> On Fri, Feb 10, 2023 at 09:44:25AM +0000, Andre Przywara wrote:  
> >>> On Fri, 10 Feb 2023 09:29:36 +0100
> >>> Maxime Ripard <[e-mail]> wrote:
> >>>
> >>> Hi Maxime,
> >>>
> >>> thanks for the reply!
> >>>      
> >>>> On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:  
> >>>>>>>>    &pio {
> >>>>>>>> +	/* 1�s debounce filter on both IRQ banks */  
> >>>>>>> Is that supposed to be <micro> in UTF-8? It seems to have got lost in
> >>>>>>> translation, or is that just me?  
> >>>>>> O yes, the Greek character slipped into the comment.  
> >>>>>>>> +	input-debounce = <1 1>;  
> >>>>>>> As mentioned above, I am not so sure this is generic enough to put it
> >>>>>>> here for PA. And what is the significance of "1 us", in particular? Is
> >>>>>>> that just the smallest value?  
> >>>>>> Yes indeed it's a bit more complicated than I feel it needs to be. The
> >>>>>> configuration is taken as microseconds and translated into the best
> >>>>>> matching clock and divider by the driver. However, 0 is not translated
> >>>>>> to the lowest divider of the high speed clock as would be logical if
> >>>>>> you ask for zero microseconds, but to "leave at default". The default
> >>>>>> of the board is 0 in the register, translating to lowest divider on the
> >>>>>> _low_ speed clock.  
> >>>>> I'd say the "if (!debounce) continue;" code is just to defend against
> >>>>> the division by zero, which would be the next statement to execute.
> >>>>>
> >>>>> We might want to change that to interpret 0 as "lowest possible", which
> >>>>> would be 24MHz/1. Please feel free to send a patch in this regard, and
> >>>>> CC: Maxime, to get some input on that idea.  
> >>>> I never had any complaint on that part either, so the default looks sane
> >>>> to me.
> >>>>
> >>>> If some board needs a higher debouncing rate, then we should obviously
> >>>> set it up in the device tree of that board, but changing it for every
> >>>> user also introduces the risk of breaking other boards that actually
> >>>> require a lower debouncing frequency.  
> >>> Yeah, we definitely should keep the default at 32KHz/1, as this is also
> >>> the hardware reset value.
> >>>
> >>> Not sure if you were actually arguing this, but the change I sketched
> >>> above (interpreting 0 as 24MHz/1) is separate though, as the current
> >>> default is "no DT property", and not 0. There is no input-debounce
> >>> property user in the kernel tree at the moment, so we wouldn't break
> >>> anyone. The only thing that would change is if a downstream user was
> >>> relying on "0" being interpreted as "skip the setup", which isn't
> >>> really documented and could be argued to be an implementation detail.
> >>>
> >>> So I'd suggest to implement 0 as "lowest possible", and documenting that
> >>> and the 32KHz/1 default if no property is given.  
> >> Ah, my bad.
> >>
> >> There's another thing to consider: there's already a generic per-pin
> >> input-debounce property in pinctrl.
> >>
> >> Since we can't control it per pin but per bank, we moved it to the
> >> controller back then, but there's always been this (implicit)
> >> expectation that it was behaving the same way.
> >>
> >> And the generic, per-pin, input-debounce documentation says:
> >>  
> >>> Takes the debounce time in usec as argument or 0 to disable debouncing  
> >> I agree that silently ignoring it is not great, but interpreting 0 as
> >> the lowest possible is breaking that behaviour which, I believe, is a
> >> worse outcome.  
> > Is it really? If I understand the hardware manuals correctly, we cannot
> > really turn that feature off, so isn't the lowest possible time period (24
> > MHz/1 at the moment) the closest we can get to "turn it off"? So
> > implementing this would bring us actually closer to the documented
> > behaviour? Or did I get the meaning of this time period wrong?
> > At least that's my understanding of how it fixed Andreas' problem: 1µs
> > is still not "off", but much better than the 31µs of the default. The new
> > 0 would then be 0.041µs.  
> I would fully agree. There seems to be no way to turn off the debouncing
> filter, and in terms of that filter, the lowest possible time is closest 
> to "off".
> The SoC default is equivalent to 31 us, far, far away from "off", the 
> currently
> configurable minimum is 1us.
> 
> I did a patch that enables to set "0" in the device tree configuration 
> and it
> takes care not to do a #div0, but to determine the lowest possible time. As
> the patch is done in the driver for a device that cannot switch off 
> debouncing,
> I'd say, the driver patched in that way does its best to come as close 
> to the
> intended outcome as is possible.
> 
> I tested this setting on the Banana M2 Zero board, and it is working (does
> the right thing setting the relevant registers to value 0x0001, and the 
> board
> works in general, w/o producing smoke. (I have no idea how to test if
> the debouncing filter is actually faster with setting "0" than with 
> setting "1",
> I can only confirm it is not significantly slower).
> 
> If we can agree on a concrete way to go I'm happy to try to produce a new
> patch version. My suggestion from the discussion:
> 
> - Change drivers/pinctrl/sunxi/pinctrl-sunxi.c to set the minimum
>     possible filter time when input-debounce is configured to "0"
>     (corresponding to 1 on the affected hardware register).
>     What I don't like is the huge gap between configuration 1 and 0, but
>     I have no idea what to do about it without breaking all compatibility.

I wouldn't be concerned about this gap, as 0 is supposed to mean off,
so there is already an expectation of a fundamentally different
behaviour. Plus the interface chose microseconds for a reason, I guess
for mechanical debouncing a resolution of 1 microsecond is more than
enough.
Can you please send this patch, so that we can have any potential
discussion there, on the code? And please add a Fixes: tag, so it can
be backported to stable kernels.

> - in arch/arm/boot/dts/sunxi-h3-h5.dtsi, set input-debounce <31 31>
>     corresponding to the default "0" in both affected hardware registers.
>     Note that the clocks hosc and losc that make this 31 map to 0 are
>     configured exactly here.

I am not fully decided on that, but there are some good points to it.

> - in arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts, set
>     input-debounce <31 0> as this board has electronic devices
>     attached to bank G and only exposes bank A to its users.
>     I'd like to advertise on that one: this board does not require
>     debouncing on bank G. Plus it feels the board got more stable
>     by this setting: my BananaPi is connected via WiFi (only) and in the
>     past it went apparently dead every other day or so. Nothing like
>     this happened after switching off input debounce. Anectdotal
>     evidence, I know...

I am not convinced of that, for the generic DT in the kernel tree. For
a start, I wouldn't use 0, as this does not do the right thing on older
kernels. But again, I haven't heard of error reports, so I am a bit
reluctant to change that setting.
There are reports of unstable WiFi, but IIUC mostly due to the poor
WiFi chip design and quality or driver problems.

If you can prove this more conclusively, that would be worth
considering, though.

> - (in my devicetree overlay, I set input-debounce <0 0> to make IRQ
>     based drivers like drivers/iio/humidity/dht11.c work on bank A) -
>     not part of the patch.

Yes, but again I'd recommend to use <1 1>, as this is more compatible,
in case you happen to load a stable kernel.

> Would that appear right?
> 
> Best regards,
> 
> Andreas.
> 
> PS: Perhaps someone can point me to further reading regarding
> drivers for electronic devices attached to GPIO. Assuming I want
> to attach a device to a GPIO that is not accidentally covered by
> hardware support of the pinctrl subsystem, what options do I
> have _apart_ from registering edge IRQs to react on a digital
> signal from that device? Isn't it called bit-banging and the
> usual technique?

I am not sure what you mean with "not accidentally covered ...", do you
mean using a hardware peripheral interface like an I2C or SPI
controller?
Yes, there are some examples of bit-banging devices in the kernel,
though it's not considered very efficient, but one might not have a
choice, mostly.

Cheers,
Andre

Samuel Holland Feb. 11, 2023, 7:45 p.m. UTC | #10

Hi Andre,

On 2/9/23 14:29, Andre Przywara wrote:
> On Wed, 8 Feb 2023 13:50:04 +0100
> Andreas Feldner <[e-mail]> 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 <[e-mail]> 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.

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? I imagine no wakeup source will
require a particularly short debounce time.

Regards,
Samuel

Andre Przywara Feb. 13, 2023, 1:51 a.m. UTC | #11

On Sat, 11 Feb 2023 13:45:37 -0600
Samuel Holland <[e-mail]> wrote:

Hi Samuel,

> On 2/9/23 14:29, Andre Przywara wrote:
> > On Wed, 8 Feb 2023 13:50:04 +0100
> > Andreas Feldner <[e-mail]> 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 <[e-mail]> 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

Maxime Ripard Feb. 13, 2023, 8:43 a.m. UTC | #12

On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:
> > > Not sure if you were actually arguing this, but the change I sketched
> > > above (interpreting 0 as 24MHz/1) is separate though, as the current
> > > default is "no DT property", and not 0. There is no input-debounce
> > > property user in the kernel tree at the moment, so we wouldn't break
> > > anyone. The only thing that would change is if a downstream user was
> > > relying on "0" being interpreted as "skip the setup", which isn't
> > > really documented and could be argued to be an implementation detail.
> > > 
> > > So I'd suggest to implement 0 as "lowest possible", and documenting that
> > > and the 32KHz/1 default if no property is given.  
> > 
> > Ah, my bad.
> > 
> > There's another thing to consider: there's already a generic per-pin
> > input-debounce property in pinctrl.
> > 
> > Since we can't control it per pin but per bank, we moved it to the
> > controller back then, but there's always been this (implicit)
> > expectation that it was behaving the same way.
> > 
> > And the generic, per-pin, input-debounce documentation says:
> > 
> > > Takes the debounce time in usec as argument or 0 to disable debouncing  
> > 
> > I agree that silently ignoring it is not great, but interpreting 0 as
> > the lowest possible is breaking that behaviour which, I believe, is a
> > worse outcome.
> 
> Is it really? If I understand the hardware manuals correctly, we cannot
> really turn that feature off, so isn't the lowest possible time period (24
> MHz/1 at the moment) the closest we can get to "turn it off"? So
> implementing this would bring us actually closer to the documented
> behaviour? Or did I get the meaning of this time period wrong?
> At least that's my understanding of how it fixed Andreas' problem: 1µs
> is still not "off", but much better than the 31µs of the default. The new
> 0 would then be 0.041µs.

My point was that the property we share the name (and should share the
semantics with) documents 0 as disabled. We would have a behavior that
doesn't disable it. It's inconsistent.

The reason doesn't really matter, we would share the same name but have
a completely different behavior, this is super confusing to me.

Maxime

Andreas Feldner Feb. 13, 2023, 8:49 a.m. UTC | #13

Am 13.02.23 um 09:43 schrieb Maxime Ripard:
> On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:
>>>> Not sure if you were actually arguing this, but the change I sketched
>>>> above (interpreting 0 as 24MHz/1) is separate though, as the current
>>>> default is "no DT property", and not 0. There is no input-debounce
>>>> property user in the kernel tree at the moment, so we wouldn't break
>>>> anyone. The only thing that would change is if a downstream user was
>>>> relying on "0" being interpreted as "skip the setup", which isn't
>>>> really documented and could be argued to be an implementation detail.
>>>>
>>>> So I'd suggest to implement 0 as "lowest possible", and documenting that
>>>> and the 32KHz/1 default if no property is given.
>>> Ah, my bad.
>>>
>>> There's another thing to consider: there's already a generic per-pin
>>> input-debounce property in pinctrl.
>>>
>>> Since we can't control it per pin but per bank, we moved it to the
>>> controller back then, but there's always been this (implicit)
>>> expectation that it was behaving the same way.
>>>
>>> And the generic, per-pin, input-debounce documentation says:
>>>
>>>> Takes the debounce time in usec as argument or 0 to disable debouncing
>>> I agree that silently ignoring it is not great, but interpreting 0 as
>>> the lowest possible is breaking that behaviour which, I believe, is a
>>> worse outcome.
>> Is it really? If I understand the hardware manuals correctly, we cannot
>> really turn that feature off, so isn't the lowest possible time period (24
>> MHz/1 at the moment) the closest we can get to "turn it off"? So
>> implementing this would bring us actually closer to the documented
>> behaviour? Or did I get the meaning of this time period wrong?
>> At least that's my understanding of how it fixed Andreas' problem: 1µs
>> is still not "off", but much better than the 31µs of the default. The new
>> 0 would then be 0.041µs.
> My point was that the property we share the name (and should share the
> semantics with) documents 0 as disabled. We would have a behavior that
> doesn't disable it. It's inconsistent.
>
> The reason doesn't really matter, we would share the same name but have
> a completely different behavior, this is super confusing to me.

I got the point. As far as I can tell from the datasheet, it is not possible
to actually switch off input-debounce. But as a debounce filter is actually
a low-pass filter, setting the cut-off frequency as high as possible,
appears to be the equivalent to switching it off.
To me it does not appear inconsistent, as any hardware will have an
implicit cut-off frequency given by its physical capabilites.

Cheers,

Andreas.

Maxime Ripard Feb. 13, 2023, 9:18 a.m. UTC | #14

On Mon, Feb 13, 2023 at 09:49:55AM +0100, [e-mail] wrote:
> Am 13.02.23 um 09:43 schrieb Maxime Ripard:
> > On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:
> > > > > Not sure if you were actually arguing this, but the change I sketched
> > > > > above (interpreting 0 as 24MHz/1) is separate though, as the current
> > > > > default is "no DT property", and not 0. There is no input-debounce
> > > > > property user in the kernel tree at the moment, so we wouldn't break
> > > > > anyone. The only thing that would change is if a downstream user was
> > > > > relying on "0" being interpreted as "skip the setup", which isn't
> > > > > really documented and could be argued to be an implementation detail.
> > > > > 
> > > > > So I'd suggest to implement 0 as "lowest possible", and documenting that
> > > > > and the 32KHz/1 default if no property is given.
> > > > Ah, my bad.
> > > > 
> > > > There's another thing to consider: there's already a generic per-pin
> > > > input-debounce property in pinctrl.
> > > > 
> > > > Since we can't control it per pin but per bank, we moved it to the
> > > > controller back then, but there's always been this (implicit)
> > > > expectation that it was behaving the same way.
> > > > 
> > > > And the generic, per-pin, input-debounce documentation says:
> > > > 
> > > > > Takes the debounce time in usec as argument or 0 to disable debouncing
> > > > I agree that silently ignoring it is not great, but interpreting 0 as
> > > > the lowest possible is breaking that behaviour which, I believe, is a
> > > > worse outcome.
> > > Is it really? If I understand the hardware manuals correctly, we cannot
> > > really turn that feature off, so isn't the lowest possible time period (24
> > > MHz/1 at the moment) the closest we can get to "turn it off"? So
> > > implementing this would bring us actually closer to the documented
> > > behaviour? Or did I get the meaning of this time period wrong?
> > > At least that's my understanding of how it fixed Andreas' problem: 1µs
> > > is still not "off", but much better than the 31µs of the default. The new
> > > 0 would then be 0.041µs.
> > My point was that the property we share the name (and should share the
> > semantics with) documents 0 as disabled. We would have a behavior that
> > doesn't disable it. It's inconsistent.
> > 
> > The reason doesn't really matter, we would share the same name but have
> > a completely different behavior, this is super confusing to me.
> 
> I got the point. As far as I can tell from the datasheet, it is not possible
> to actually switch off input-debounce. But as a debounce filter is actually
> a low-pass filter, setting the cut-off frequency as high as possible,
> appears to be the equivalent to switching it off.

It's not really a matter of hardware here, it's a matter of driver
behavior vs generic behavior from the framework. The hardware obviously
influences the former, but it's marginal in that discussion.

As that whole discussion shows, whether the frequency would be high
enough is application dependent, and thus we cannot just claim that it's
equivalent in all circumstances.

Making such an assumption will just bite someone else down the road,
except this time we will have users (you, I'd assume) relying on that
behavior so we wouldn't be able to address it.

But I also agree with the fact that doing nothing with 0 is bad UX and
confusing as well.

I still think that we can address both by just erroring out on 0 /
printing an error message so that it's obvious that we can't support it,
and we wouldn't change the semantics of the property either.

And then you can set the actual debouncing time you need instead of
"whatever" in the device tree.

Maxime

Andre Przywara Feb. 13, 2023, 11:56 a.m. UTC | #15

On Mon, 13 Feb 2023 10:18:03 +0100
Maxime Ripard <[e-mail]> wrote:

Hi,

> On Mon, Feb 13, 2023 at 09:49:55AM +0100, [e-mail] wrote:
> > Am 13.02.23 um 09:43 schrieb Maxime Ripard:  
> > > On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:  
> > > > > > Not sure if you were actually arguing this, but the change I sketched
> > > > > > above (interpreting 0 as 24MHz/1) is separate though, as the current
> > > > > > default is "no DT property", and not 0. There is no input-debounce
> > > > > > property user in the kernel tree at the moment, so we wouldn't break
> > > > > > anyone. The only thing that would change is if a downstream user was
> > > > > > relying on "0" being interpreted as "skip the setup", which isn't
> > > > > > really documented and could be argued to be an implementation detail.
> > > > > > 
> > > > > > So I'd suggest to implement 0 as "lowest possible", and documenting that
> > > > > > and the 32KHz/1 default if no property is given.  
> > > > > Ah, my bad.
> > > > > 
> > > > > There's another thing to consider: there's already a generic per-pin
> > > > > input-debounce property in pinctrl.
> > > > > 
> > > > > Since we can't control it per pin but per bank, we moved it to the
> > > > > controller back then, but there's always been this (implicit)
> > > > > expectation that it was behaving the same way.
> > > > > 
> > > > > And the generic, per-pin, input-debounce documentation says:
> > > > >   
> > > > > > Takes the debounce time in usec as argument or 0 to disable debouncing  
> > > > > I agree that silently ignoring it is not great, but interpreting 0 as
> > > > > the lowest possible is breaking that behaviour which, I believe, is a
> > > > > worse outcome.  
> > > > Is it really? If I understand the hardware manuals correctly, we cannot
> > > > really turn that feature off, so isn't the lowest possible time period (24
> > > > MHz/1 at the moment) the closest we can get to "turn it off"? So
> > > > implementing this would bring us actually closer to the documented
> > > > behaviour? Or did I get the meaning of this time period wrong?
> > > > At least that's my understanding of how it fixed Andreas' problem: 1µs
> > > > is still not "off", but much better than the 31µs of the default. The new
> > > > 0 would then be 0.041µs.  
> > > My point was that the property we share the name (and should share the
> > > semantics with) documents 0 as disabled. We would have a behavior that
> > > doesn't disable it. It's inconsistent.
> > > 
> > > The reason doesn't really matter, we would share the same name but have
> > > a completely different behavior, this is super confusing to me.  
> > 
> > I got the point. As far as I can tell from the datasheet, it is not possible
> > to actually switch off input-debounce. But as a debounce filter is actually
> > a low-pass filter, setting the cut-off frequency as high as possible,
> > appears to be the equivalent to switching it off.  
> 
> It's not really a matter of hardware here, it's a matter of driver
> behavior vs generic behavior from the framework. The hardware obviously
> influences the former, but it's marginal in that discussion.
> 
> As that whole discussion shows, whether the frequency would be high
> enough is application dependent, and thus we cannot just claim that it's
> equivalent in all circumstances.
> 
> Making such an assumption will just bite someone else down the road,
> except this time we will have users (you, I'd assume) relying on that
> behavior so we wouldn't be able to address it.
> 
> But I also agree with the fact that doing nothing with 0 is bad UX and
> confusing as well.
> 
> I still think that we can address both by just erroring out on 0 /
> printing an error message so that it's obvious that we can't support it,
> and we wouldn't change the semantics of the property either.
> 
> And then you can set the actual debouncing time you need instead of
> "whatever" in the device tree.

I am on the same page with regards to discouraging 0 as a proper value, and
that we should warn if this is being used.
However I think we should at the same time try to still get as low as
possible when 0 is specified. The debounce property uses microseconds as
the unit, but even the AW hardware allows us to go lower than this. So we
would leave that on the table, somewhat needlessly: input-debounce = <1>
would give us 1333 ns, when the lowest possible is about 42 ns (1/24MHz).

So what about the following:
We document that 0 does not mean off, but tries to get as low as possible.
If the driver sees 0, it issues a warning, but still tries to lower the
debounce period as much as possible, and reports that, like:
[1.2345678] 1c20800.pinctrl: cannot turn off debouncing, setting to 41.7 ns

Alternatively we use a different property name, if that is a concern. We
could then use nanoseconds as a unit value, and then can error out on 0.
Re-using input-debounce is somewhat dodgy anyway, since the generic
property is for a single value only, per pin (in the pinmux DT node, not
in the controller node), whereas we use an array of some non-obvious
subset of ports.

Cheers,
Andre

Andreas Feldner Feb. 14, 2023, 6:49 p.m. UTC | #16

Am 13.02.23 um 12:56 schrieb Andre Przywara:
> On Mon, 13 Feb 2023 10:18:03 +0100
> Maxime Ripard <[e-mail]> wrote:
>
> Hi,
>
>> On Mon, Feb 13, 2023 at 09:49:55AM +0100, [e-mail] wrote:
>>> Am 13.02.23 um 09:43 schrieb Maxime Ripard:
>>>> On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:
>>>>>>> Not sure if you were actually arguing this, but the change I sketched
>>>>>>> above (interpreting 0 as 24MHz/1) is separate though, as the current
>>>>>>> default is "no DT property", and not 0. There is no input-debounce
>>>>>>> property user in the kernel tree at the moment, so we wouldn't break
>>>>>>> anyone. The only thing that would change is if a downstream user was
>>>>>>> relying on "0" being interpreted as "skip the setup", which isn't
>>>>>>> really documented and could be argued to be an implementation detail.
>>>>>>>
>>>>>>> So I'd suggest to implement 0 as "lowest possible", and documenting that
>>>>>>> and the 32KHz/1 default if no property is given.
>>>>>> Ah, my bad.
>>>>>>
>>>>>> There's another thing to consider: there's already a generic per-pin
>>>>>> input-debounce property in pinctrl.
>>>>>>
>>>>>> Since we can't control it per pin but per bank, we moved it to the
>>>>>> controller back then, but there's always been this (implicit)
>>>>>> expectation that it was behaving the same way.
>>>>>>
>>>>>> And the generic, per-pin, input-debounce documentation says:
>>>>>>    
>>>>>>> Takes the debounce time in usec as argument or 0 to disable debouncing
>>>>>> I agree that silently ignoring it is not great, but interpreting 0 as
>>>>>> the lowest possible is breaking that behaviour which, I believe, is a
>>>>>> worse outcome.
>>>>> Is it really? If I understand the hardware manuals correctly, we cannot
>>>>> really turn that feature off, so isn't the lowest possible time period (24
>>>>> MHz/1 at the moment) the closest we can get to "turn it off"? So
>>>>> implementing this would bring us actually closer to the documented
>>>>> behaviour? Or did I get the meaning of this time period wrong?
>>>>> At least that's my understanding of how it fixed Andreas' problem: 1µs
>>>>> is still not "off", but much better than the 31µs of the default. The new
>>>>> 0 would then be 0.041µs.
>>>> My point was that the property we share the name (and should share the
>>>> semantics with) documents 0 as disabled. We would have a behavior that
>>>> doesn't disable it. It's inconsistent.
>>>>
>>>> The reason doesn't really matter, we would share the same name but have
>>>> a completely different behavior, this is super confusing to me.
>>> I got the point. As far as I can tell from the datasheet, it is not possible
>>> to actually switch off input-debounce. But as a debounce filter is actually
>>> a low-pass filter, setting the cut-off frequency as high as possible,
>>> appears to be the equivalent to switching it off.
>> It's not really a matter of hardware here, it's a matter of driver
>> behavior vs generic behavior from the framework. The hardware obviously
>> influences the former, but it's marginal in that discussion.
>>
>> As that whole discussion shows, whether the frequency would be high
>> enough is application dependent, and thus we cannot just claim that it's
>> equivalent in all circumstances.
>>
>> Making such an assumption will just bite someone else down the road,
>> except this time we will have users (you, I'd assume) relying on that
>> behavior so we wouldn't be able to address it.
>>
>> But I also agree with the fact that doing nothing with 0 is bad UX and
>> confusing as well.
>>
>> I still think that we can address both by just erroring out on 0 /
>> printing an error message so that it's obvious that we can't support it,
>> and we wouldn't change the semantics of the property either.
>>
>> And then you can set the actual debouncing time you need instead of
>> "whatever" in the device tree.
> I am on the same page with regards to discouraging 0 as a proper value, and
> that we should warn if this is being used.
> However I think we should at the same time try to still get as low as
> possible when 0 is specified. The debounce property uses microseconds as
> the unit, but even the AW hardware allows us to go lower than this. So we
> would leave that on the table, somewhat needlessly: input-debounce = <1>
> would give us 1333 ns, when the lowest possible is about 42 ns (1/24MHz).
>
> So what about the following:
> We document that 0 does not mean off, but tries to get as low as possible.
> If the driver sees 0, it issues a warning, but still tries to lower the
> debounce period as much as possible, and reports that, like:
> [1.2345678] 1c20800.pinctrl: cannot turn off debouncing, setting to 41.7 ns
That would be trivial to implement. My only concern: this
leaves no way to configure the minimum setting without getting a
warning.

I'd like to show the acutally selected timing for values >= 1 as well (level
info, though), as it hardly ever exactly matches the value given.
> Alternatively we use a different property name, if that is a concern. We
> could then use nanoseconds as a unit value, and then can error out on 0.
> Re-using input-debounce is somewhat dodgy anyway, since the generic
> property is for a single value only, per pin (in the pinmux DT node, not
> in the controller node), whereas we use an array of some non-obvious
> subset of ports.

How to avoid breaking existing devicetrees? Wouldn't it be required to
handle input-debounce as well, but somehow obsolete it?

Cheers,

Andreas

Maxime Ripard Feb. 15, 2023, 8:36 a.m. UTC | #17

On Mon, Feb 13, 2023 at 11:56:52AM +0000, Andre Przywara wrote:
> On Mon, 13 Feb 2023 10:18:03 +0100
> Maxime Ripard <[e-mail]> wrote:
> > On Mon, Feb 13, 2023 at 09:49:55AM +0100, [e-mail] wrote:
> > > Am 13.02.23 um 09:43 schrieb Maxime Ripard:  
> > > > On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:  
> > > > > > > Not sure if you were actually arguing this, but the change I sketched
> > > > > > > above (interpreting 0 as 24MHz/1) is separate though, as the current
> > > > > > > default is "no DT property", and not 0. There is no input-debounce
> > > > > > > property user in the kernel tree at the moment, so we wouldn't break
> > > > > > > anyone. The only thing that would change is if a downstream user was
> > > > > > > relying on "0" being interpreted as "skip the setup", which isn't
> > > > > > > really documented and could be argued to be an implementation detail.
> > > > > > > 
> > > > > > > So I'd suggest to implement 0 as "lowest possible", and documenting that
> > > > > > > and the 32KHz/1 default if no property is given.  
> > > > > > Ah, my bad.
> > > > > > 
> > > > > > There's another thing to consider: there's already a generic per-pin
> > > > > > input-debounce property in pinctrl.
> > > > > > 
> > > > > > Since we can't control it per pin but per bank, we moved it to the
> > > > > > controller back then, but there's always been this (implicit)
> > > > > > expectation that it was behaving the same way.
> > > > > > 
> > > > > > And the generic, per-pin, input-debounce documentation says:
> > > > > >   
> > > > > > > Takes the debounce time in usec as argument or 0 to disable debouncing  
> > > > > > I agree that silently ignoring it is not great, but interpreting 0 as
> > > > > > the lowest possible is breaking that behaviour which, I believe, is a
> > > > > > worse outcome.  
> > > > > Is it really? If I understand the hardware manuals correctly, we cannot
> > > > > really turn that feature off, so isn't the lowest possible time period (24
> > > > > MHz/1 at the moment) the closest we can get to "turn it off"? So
> > > > > implementing this would bring us actually closer to the documented
> > > > > behaviour? Or did I get the meaning of this time period wrong?
> > > > > At least that's my understanding of how it fixed Andreas' problem: 1µs
> > > > > is still not "off", but much better than the 31µs of the default. The new
> > > > > 0 would then be 0.041µs.  
> > > > My point was that the property we share the name (and should share the
> > > > semantics with) documents 0 as disabled. We would have a behavior that
> > > > doesn't disable it. It's inconsistent.
> > > > 
> > > > The reason doesn't really matter, we would share the same name but have
> > > > a completely different behavior, this is super confusing to me.  
> > > 
> > > I got the point. As far as I can tell from the datasheet, it is not possible
> > > to actually switch off input-debounce. But as a debounce filter is actually
> > > a low-pass filter, setting the cut-off frequency as high as possible,
> > > appears to be the equivalent to switching it off.  
> > 
> > It's not really a matter of hardware here, it's a matter of driver
> > behavior vs generic behavior from the framework. The hardware obviously
> > influences the former, but it's marginal in that discussion.
> > 
> > As that whole discussion shows, whether the frequency would be high
> > enough is application dependent, and thus we cannot just claim that it's
> > equivalent in all circumstances.
> > 
> > Making such an assumption will just bite someone else down the road,
> > except this time we will have users (you, I'd assume) relying on that
> > behavior so we wouldn't be able to address it.
> > 
> > But I also agree with the fact that doing nothing with 0 is bad UX and
> > confusing as well.
> > 
> > I still think that we can address both by just erroring out on 0 /
> > printing an error message so that it's obvious that we can't support it,
> > and we wouldn't change the semantics of the property either.
> > 
> > And then you can set the actual debouncing time you need instead of
> > "whatever" in the device tree.
> 
> I am on the same page with regards to discouraging 0 as a proper value, and
> that we should warn if this is being used.

Great :)

> However I think we should at the same time try to still get as low as
> possible when 0 is specified.

It's still undefined behaviour though. It will be context dependent, and
if we ever encounter a bug at 1/24MHz, we'll change that value to
something else that might break other users that were relying on that
value.

> The debounce property uses microseconds as the unit, but even the AW
> hardware allows us to go lower than this. So we would leave that on
> the table, somewhat needlessly: input-debounce = <1> would give us
> 1333 ns, when the lowest possible is about 42 ns (1/24MHz).
> 
> So what about the following:
> We document that 0 does not mean off, but tries to get as low as possible.

I still don't really like the fact that we're changing the semantics of
that property.

> If the driver sees 0, it issues a warning, but still tries to lower the
> debounce period as much as possible, and reports that, like:
> [1.2345678] 1c20800.pinctrl: cannot turn off debouncing, setting to 41.7 ns

I just thought about another thing that might be a solution: instead of
checking/enforcing the input-debounce value at probe time, we can do so
when the pin is configured.

That way, the pinctrl driver will load without affecting the system boot
too much, but we would still reject pin configurations whose bank are
using 0.

Actually configuring the debounce per bank each time is configured is
probably going to complexify the driver, so we could set it up in the
probe, and default to the lowest debouncing value if it's zero, but we
would still actively check for that value to not be 0 as soon as a pin
is configured which would effectively prevent anyone from using that
value. I think this would be a good compromise?

> Alternatively we use a different property name, if that is a concern. We
> could then use nanoseconds as a unit value, and then can error out on 0.

I mean, if that's a concern, we can also introduce an input-debounce-ns
property that is mutually exclusive with input-debounce, with both
erroring out on 0.

Maxime

Patch

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
index d729b7c705db..1fc0d5d1e51a 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
@@ -113,6 +113,22 @@  wifi_pwrseq: wifi_pwrseq {
 
 &cpu0 {
 	cpu-supply = <&reg_vdd_cpux>;
+	clock-frequency = <1296000000>;
+};
+
+&cpu1 {
+	cpu-supply = <&reg_vdd_cpux>;
+	clock-frequency = <1296000000>;
+};
+
+&cpu2 {
+	cpu-supply = <&reg_vdd_cpux>;
+	clock-frequency = <1296000000>;
+};
+
+&cpu3 {
+	cpu-supply = <&reg_vdd_cpux>;
+	clock-frequency = <1296000000>;
 };
 
 &de {
@@ -193,6 +209,8 @@  bluetooth {
 };
 
 &pio {
+	/* 1µs debounce filter on both IRQ banks */
+	input-debounce = <1 1>;
 	gpio-line-names =
 		/* PA */
 		"CON2-P13", "CON2-P11", "CON2-P22", "CON2-P15",