Feedback-ID: i8771445c:Fastmail
Date:   Fri, 10 Feb 2023 11:06:20 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Andre Przywara <andre.przywara@arm.com>
Cc:     Andreas Feldner <andreas@feldner-bv.de>,
        Andreas Feldner <pelzi@flying-snail.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ARM: dts: allwinner: minimize irq debounce filter per
 default
Message-ID: <20230210100620.z6j7rvkiwyu7paij@houat>
References: <Y+FaVorMl37F5Dve@debian-qemu.internal.flying-snail.de>
 <20230207011608.2ce24d17@slackpad.lan>
 <d0534762-3785-ec2d-8d1e-aba0e39f701b@feldner-bv.de>
 <20230209202952.673d5a60@slackpad.lan>
 <20230210082936.qefzz4fsp3jpalvp@houat>
 <20230210094425.474cfba5@donnerap.cambridge.arm.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="dptlal3vq5nbftml"
Content-Disposition: inline
In-Reply-To: <20230210094425.474cfba5@donnerap.cambridge.arm.com>
Precedence: bulk


--dptlal3vq5nbftml
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 10, 2023 at 09:44:25AM +0000, Andre Przywara wrote:
> On Fri, 10 Feb 2023 09:29:36 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
>=20
> Hi Maxime,
>=20
> thanks for the reply!
>=20
> > On Thu, Feb 09, 2023 at 08:29:52PM +0000, Andre Przywara wrote:
> > > > >>   &pio {
> > > > >> +	/* 1=EF=BF=BDs debounce filter on both IRQ banks */ =20
> > > > > Is that supposed to be <micro> in UTF-8? It seems to have got los=
t in
> > > > > translation, or is that just me? =20
> > > > O yes, the Greek character slipped into the comment. =20
> > > > >> +	input-debounce =3D <1 1>; =20
> > > > > As mentioned above, I am not so sure this is generic enough to pu=
t it
> > > > > here for PA. And what is the significance of "1 us", in particula=
r? Is
> > > > > that just the smallest value?   =20
> > > >=20
> > > > 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 transla=
ted
> > > > 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 defau=
lt
> > > > of the board is 0 in the register, translating to lowest divider on=
 the
> > > > _low_ speed clock. =20
> > >=20
> > > 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.
> > >=20
> > > We might want to change that to interpret 0 as "lowest possible", whi=
ch
> > > would be 24MHz/1. Please feel free to send a patch in this regard, and
> > > CC: Maxime, to get some input on that idea. =20
> >=20
> > I never had any complaint on that part either, so the default looks sane
> > to me.
> >=20
> > 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.
>=20
> Yeah, we definitely should keep the default at 32KHz/1, as this is also
> the hardware reset value.
>=20
> 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.
>=20
> 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

--dptlal3vq5nbftml
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY+YXFwAKCRDj7w1vZxhR
xTFrAQCdGF3qrNAzaAFjIUXOhNcaohzIyQtoXdUESrPe4GjXMAD+JilJvMPYMQ5m
AXTNk41unbUwC1kxQEaZheLAV9LBHgg=
=iTHV
-----END PGP SIGNATURE-----

--dptlal3vq5nbftml--