Re: [PATCH 0/2] arm64: dts: renesas: r8a779a0: Add INTC-EX support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Kieran,

On Wed, Sep 22, 2021 at 4:14 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> I've been trying to test this on the Falcon-V3U

Thanks for testing!

> On 24/06/2021 09:07, Geert Uytterhoeven wrote:
> > On Wed, Jun 23, 2021 at 6:13 PM Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> >> On 23/06/2021 16:02, Geert Uytterhoeven wrote:
> >>> This patch series adds support for the Interrupt Controller for External
> >>> Devices (INT-EC) in the Renesas R-Car V3U (r8a779a0) SoC.
> >>>
> >>> As there are two known issues, I'm posting this to a limited audience:
> >>>
> >>>   1. External interrupts have not been tested.

> >>>      Alternatively, with physical access, IRQ0 is available on test
> >>>      point CP47, and IRQ2 on the GPIO CN.
> >>
> >> I do have physical access, so I can trigger this - Is there a suitable
> >> voltage or condition I can apply? (I.e. take a signal from a nearby pin
> >> to short it?)
> >
> > As IRQ0 is driven by the single gate U59, you better don't cause logic
> > conflicts, and play with IRQ2 instead.
> >
> > Note that high level is SPI_D1.8V/3.3V, which is 1.8V by default!
> > The GPIO CN connector carries a.o. SPI_D1.8V/3.3V and GND.
> > Internal pull-up should be enabled for IRQ2 by reset state, but you
> > may want to measure the pin's voltage to be sure.
>
> Pin7 appears to be IRQ2. It is reading at 1.8v.
> Pin 5 (SPI_D1.8v/3.3v) is reading at 1.8v
> Pin 3 (D3.3v) no prizes for guessing here.
>
> And of course pin 1 is ground.
>
> So I have some wires I can play with.
>
> > To configure pin control, you need to add the following, and hook it
> > up to the pfc node:
> >
> >         irq2_pins: irq2 {
> >                 groups = "intc_ex_irq2";
> >                 function = "intc_ex";
> >         };
> >
> > You should be able to test this using gpio-keys, with a key subnode that
> > has an interrupts instead of a gpios property.
>
> I'm afraid I haven't been able to successfully test this. I have this
> series applied and have tried the following:

> --- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> @@ -10,6 +10,10 @@
>  #include "r8a779a0-falcon-csi-dsi.dtsi"
>  #include "r8a779a0-falcon-ethernet.dtsi"
>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/gpio-keys.h>
> +#include <dt-bindings/input/input.h>
> +
>  / {
>         model = "Renesas Falcon CPU and Breakout boards based on r8a779a0";
>         compatible = "renesas,falcon-breakout", "renesas,falcon-cpu",
> "renesas,r8a779a0";
> @@ -17,6 +21,23 @@ / {
>         aliases {
>                 ethernet0 = &avb0;
>         };
> +
> +       gpio_keys {
> +               compatible = "gpio-keys";
> +
> +               btn1 {
> +                       pinctrl-0 = <&irq2_pins>;
> +                       pinctrl-names = "default";
> +
> +                       debounce-interval = <50>;
> +                       label = "button1";
> +                       linux,code = <KEY_1>;
> +                       interrupt-parent = <&intc_ex>;
> +                       interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> +
> +                       //gpios = <&gpio1 26 GPIO_ACTIVE_LOW>;
> +               };
> +       };
>  };
>
>  &avb0 {
> @@ -45,6 +66,14 @@ eeprom@51 {
>  };
>
>  &pfc {
> +       // Intc_ex testing
> +       irq2_pins: irq2 {
> +               groups = "intc_ex_irq2";
> +               function = "intc_ex";
> +
> +               bias-pull-up;
> +       };
> +
>         avb0_pins: avb0 {
>                 mux {
>                         groups = "avb0_link", "avb0_mdio", "avb0_rgmii",

Looks good to me.

> diff --git a/drivers/pinctrl/renesas/core.c b/drivers/pinctrl/renesas/core.c
> index ef8ef05ba930..966883c6c64c 100644
> --- a/drivers/pinctrl/renesas/core.c
> +++ b/drivers/pinctrl/renesas/core.c
> @@ -228,7 +228,7 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
>
>         sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
>
> -       dev_dbg(pfc->dev, "write_reg addr = %x, value = 0x%x, field = %u, "
> +       dev_err(pfc->dev, "KB: write_reg addr = %x, value = 0x%x, field = %u, "
>                 "r_width = %u, f_width = %u\n",
>                 crp->reg, value, field, crp->reg_width, hweight32(mask));
>
> And I have ribbon cable with the pitch for cn4 now which allows me to
> connect pin 7 to pin 1 to ground it.
>
>
> I use
>  sudo evtest /dev/input/event0
>
> to monitor the line for changes, and grounding pin7 has no effect in
> this configuration.
>
>
> However, to try to sanity check my test, I changed the gpio-keys to use
> gpio1 26 directly - disabling the pinctrl, and using the gpios reference
> directly instead :
>
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> index 1286b553e370..af85881de2c4 100644
> --- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> @@ -26,16 +26,11 @@ gpio_keys {
>                 compatible = "gpio-keys";
>
>                 btn1 {
> -                       pinctrl-0 = <&irq2_pins>;
> -                       pinctrl-names = "default";
> -
>                         debounce-interval = <50>;
>                         label = "button1";
>                         linux,code = <KEY_1>;
> -                       interrupt-parent = <&intc_ex>;
> -                       interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
>
> -                       //gpios = <&gpio1 26 GPIO_ACTIVE_LOW>;
> +                       gpios = <&gpio1 26 GPIO_ACTIVE_LOW>;
>                 };
>         };
>  };

Looks almost good to me.
You probably still want to enable bias-pull-up for GP1_26, but as
it works without...

> This shows active key events when grounding pin 7 to pin 1...
> > kbingham@falcon-v3u:~$ sudo evtest /dev/input/event0
> > Input driver version is 1.0.1
> > Input device ID: bus 0x19 vendor 0x1 product 0x1 version 0x100
> > Input device name: "gpio_keys"
> > Supported events:
> >   Event type 0 (EV_SYN)
> >   Event type 1 (EV_KEY)
> >     Event code 2 (KEY_1)
> > Properties:
> > Testing ... (interrupt to exit)
> > Event: time 1632319971.126234, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > Event: time 1632319971.126234, -------------- SYN_REPORT ------------
> > Event: time 1632319971.579966, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > Event: time 1632319971.579966, -------------- SYN_REPORT ------------
> > Event: time 1632319981.461018, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > Event: time 1632319981.461018, -------------- SYN_REPORT ------------
> > Event: time 1632319981.835693, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > Event: time 1632319981.835693, -------------- SYN_REPORT ------------
> > Event: time 1632319982.112104, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > Event: time 1632319982.112104, -------------- SYN_REPORT ------------

Good!

> If there's anything else you'd like me to test or change let me know.

There are still several things that could be wrong:
  - Bug in the pin control tables,
  - Wrong parent interrupt description,
  - Undocumented INTC-EX module clock is turned off
    (does it retain register values?),
  - Wrong initial values in INTC-EX registers the driver doesn't touch,
  - ...

Can you print the value of the INTC-EX MONITOR register?
It should match the state of the external pins.

> > This might be a good opportunity to wire up the slide and push switches
> > (SW46-49) as gpio-keys, too...

Any chance with these? ;-)

Thanks again!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds