RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

Hi Geert Uytterhoeven,

> Subject: RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> GPIO interrupt input
> 
> Hi Geert,
> 
> > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter
> > for GPIO interrupt input
> >
> > Hi Biju,
> >
> > On Mon, Sep 18, 2023 at 3:18 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > wrote:
> > > > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise
> > > > filter for GPIO interrupt input
> > > >
> > > > On Mon, Sep 18, 2023 at 2:34 PM Biju Das
> > > > <biju.das.jz@xxxxxxxxxxxxxx>
> > > > wrote:
> > > > > As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO
> > > > > Interrupt
> > > > > (TINT) and 41.4.1 Operation for GPIO function, we need to set
> > > > > digital noise filter for GPIO interrupt.
> > > > >
> > > > > This patch enables noise filter for GPIO interrupt in
> > > > > rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
> > > > >
> > > > > Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > > > domain to handle GPIO interrupt")
> > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > @@ -96,6 +96,7 @@
> > > > >  #define PIN(n)                 (0x0800 + 0x10 + (n))
> > > > >  #define IOLH(n)                        (0x1000 + (n) * 8)
> > > > >  #define IEN(n)                 (0x1800 + (n) * 8)
> > > > > +#define FILONOFF(n)            (0x2080 + (n) * 8)
> > > > >  #define ISEL(n)                        (0x2c80 + (n) * 8)
> > > > >  #define PWPR                   (0x3014)
> > > > >  #define SD_CH(n)               (0x3000 + (n) * 4)
> > > >
> > > > LGTM, but shouldn't you configure the Digital Noise Filter Number
> > > > (FILNUM) and Clock Selection (FILCLKSEL) registers, too?
> > >
> > > Currently it uses reset values.
> > >
> > > 00b: 4-stage filter (41.666 ns x 4 = 166.666 ns) (initial value) for
> > > FILNUM and
> > >
> > > 00b: Not divided (initial value) for FILCLKSEL
> > >
> > > Do you mean we should provide these settings to DT, so that it is
> > > customised based on the PCB design and the environment the board is
> > > used in? I guess this will make it easier for customers to make the
> > > required changes for their application.
> >
> > If the optimal values are board-dependent, you should indeed add a way
> > to configure this from DT.
> 
> Looks like the above values are signal based. I am checking with HW person
> about this. I will confirm once I have update on this.

I rechecked this,

As per section 8.7.3 GPIO Interrupt (TINT), It is not a must. But we should have the flexibility to configure through DT.

 "The noise filter function of GPIO pins assigned to TINT can be enabled by GPIO register setting."

The confusion came because of the section "41.4.1 Operation for GPIO Function"

● Set the FILONOFF register when using GPIO to set a digital noise filter for interrupt input and GPIO input.

So I would like to drop this patch and will send separate patches
for configuring it through DT, if anyone plans to use digital noise
filters.

Cheers,
Biju