| Message ID | 20230918123355.262115-3-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix for GPIO interrupts | expand |
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@bp.renesas.com> > > 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@bp.renesas.com> > > > > 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@bp.renesas.com> > > > > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > > > > > 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
On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Why is this called "digital noise filter" when the rest of the world calls it "debounce"? It is a bit confusing to have differing terminology in the kernel. (There might be a good reason, such that the noise in question is very different from mechanical button bounce noise.) Is this only applicable to interrupt lines or also to GPIO lines in general? Because I just want to ascertain that in that case the existing binding for @PIN_CONFIG_INPUT_DEBOUNCE and the corresponding backend call from gpio_chip() maps to the same feature as well. It seems hard-coded for some pins in the Renesas driver right now but the day you make this configurable from DT please call it debounce, if the usecase is similar. Yours, Linus Walleij
Hi Linus Walleij, Thanks for the feedback. > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for > GPIO interrupt input > > On Wed, Sep 20, 2023 at 2:37 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > 2) Digital Noise Filter Clock Selection Register (FILCLKSEL): > > > > The FILCLKSEL register selects the divided clock to be input to digital > noise filters. > > > > 00b: Not divided (initial value) > > 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns) > > 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns) > > 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns) > > Note: This value is the value when the external clock is 24MHz. > > > > Q1) What is the recommended way to associate the above values with > > PIN_CONFIG_INPUT_DEBOUNCE? > > > > Eg: I need to configure filter on, 8 stage filter , a divisor of 18000 > for a mechanical button bounce noise. > > As per the generic pin config and DT bindings: > > * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce > mode, > * which means it will wait for signals to settle when reading inputs. > The > * argument gives the debounce time in usecs. Setting the > * argument to zero turns debouncing off. > > input-debounce: > $ref: /schemas/types.yaml#/definitions/uint32 > description: Takes the debounce time in usec as argument or 0 to > disable > debouncing > > The recommended way is to pass the desired clock cycle in microseconds as > the argument to the pin config. How to add number of FF stages info on top of this clock cycle? Use some encoding in DT and decode it in driver?? Cheers, Biju
On Wed, Sep 20, 2023 at 4:06 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > 2) Digital Noise Filter Clock Selection Register (FILCLKSEL): > > > > > > The FILCLKSEL register selects the divided clock to be input to digital > > noise filters. > > > > > > 00b: Not divided (initial value) > > > 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns) > > > 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns) > > > 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns) > > > Note: This value is the value when the external clock is 24MHz. > > > > > > Q1) What is the recommended way to associate the above values with > > > PIN_CONFIG_INPUT_DEBOUNCE? > > > > > > Eg: I need to configure filter on, 8 stage filter , a divisor of 18000 > > for a mechanical button bounce noise. > > > > As per the generic pin config and DT bindings: > > > > * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce > > mode, > > * which means it will wait for signals to settle when reading inputs. > > The > > * argument gives the debounce time in usecs. Setting the > > * argument to zero turns debouncing off. > > > > input-debounce: > > $ref: /schemas/types.yaml#/definitions/uint32 > > description: Takes the debounce time in usec as argument or 0 to > > disable > > debouncing > > > > The recommended way is to pass the desired clock cycle in microseconds as > > the argument to the pin config. > > How to add number of FF stages info on top of this clock cycle? You don't, look below. > Use some encoding in DT and decode it in driver?? No you put in the desired delay in microseconds, the you use the table that you already provided to look up the appropriate divisor: > 00b: Not divided (initial value) > 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns) > 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns) > 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns) For 0, 375, 750 and 1500 us... > Note: This value is the value when the external clock is 24MHz. And this can be provided from the clock framework, and then you need some more elaborate math to calculate the right divisor from the parameter in the device tree. Yours, Linus Walleij
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index 2ea6ef99cc70..6d3fa962ac97 100644 --- 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) @@ -1169,9 +1170,9 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); + void __iomem *addr, *noise_filter_addr; unsigned int hwirq = irqd_to_hwirq(d); unsigned long flags; - void __iomem *addr; u32 port; u8 bit; @@ -1181,12 +1182,15 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d) bit = RZG2L_PIN_ID_TO_PIN(hwirq); addr = pctrl->base + ISEL(port); + noise_filter_addr = pctrl->base + FILONOFF(port); if (bit >= 4) { bit -= 4; addr += 4; + noise_filter_addr += 4; } spin_lock_irqsave(&pctrl->lock, flags); + writel(readl(noise_filter_addr) & ~BIT(bit * 8), noise_filter_addr); writel(readl(addr) & ~BIT(bit * 8), addr); spin_unlock_irqrestore(&pctrl->lock, flags); @@ -1197,9 +1201,9 @@ static void rzg2l_gpio_irq_enable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); + void __iomem *addr, *noise_filter_addr; unsigned int hwirq = irqd_to_hwirq(d); unsigned long flags; - void __iomem *addr; u32 port; u8 bit; @@ -1209,13 +1213,16 @@ static void rzg2l_gpio_irq_enable(struct irq_data *d) bit = RZG2L_PIN_ID_TO_PIN(hwirq); addr = pctrl->base + ISEL(port); + noise_filter_addr = pctrl->base + FILONOFF(port); if (bit >= 4) { bit -= 4; addr += 4; + noise_filter_addr += 4; } spin_lock_irqsave(&pctrl->lock, flags); writel(readl(addr) | BIT(bit * 8), addr); + writel(readl(noise_filter_addr) | BIT(bit * 8), noise_filter_addr); spin_unlock_irqrestore(&pctrl->lock, flags); irq_chip_enable_parent(d);
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@bp.renesas.com> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> --- drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)