RE: [PATCH 1/4] gpio: rcar: Enhance gpio-ranges support

[Biju Das <biju.das@bp.renesas.com>]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven [mailto:geert@linux-m68k.org]
> Sent: 30 July 2018 11:04
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; Simon Horman <horms@verge.net.au>;
> Geert Uytterhoeven <geert+renesas@glider.be>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>; Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>
> Subject: Re: [PATCH 1/4] gpio: rcar: Enhance gpio-ranges support
>
> Hi Biju,
>
> On Fri, Jul 27, 2018 at 11:57 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > Enhance gpio-ranges to support more than one gpio-range.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Thanks for your patch!
>
> However, I'm wondering if this works as intended, as the discontinuity is not
> in the pins, but in the GPIO bits.
>
> > ---
> > cat /sys/kernel/debug/pinctrl/e6060000.pin-controller-sh-pfc/gpio-
> > GPIO ranges handled:
> > 0: e6050000.gpio GPIOS [1001 - 1023] PINS [0 - 22]
> > 0: e6051000.gpio GPIOS [978 - 1000] PINS [32 - 54]
> > 0: e6052000.gpio GPIOS [946 - 977] PINS [64 - 95]
> > 0: e6053000.gpio GPIOS [926 - 942] PINS [96 - 112]
> > 17: e6053000.gpio GPIOS [943 - 945] PINS [123 - 125]
>
> The above two lines are the result of:
>
> +                       gpio-ranges = <&pfc 0 96 17>, <&pfc 17 123 3>;
>
> > 0: e6054000.gpio GPIOS [900 - 925] PINS [128 - 153]
> > 0: e6055000.gpio GPIOS [868 - 899] PINS [160 - 191]
> > ---
> >  drivers/gpio/gpio-rcar.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index
> > 350390c..a7bbe78 100644
> > --- a/drivers/gpio/gpio-rcar.c
> > +++ b/drivers/gpio/gpio-rcar.c
> > @@ -399,13 +399,22 @@ static int gpio_rcar_parse_dt(struct
> gpio_rcar_priv *p, unsigned int *npins)
> >         struct device_node *np = p->pdev->dev.of_node;
> >         const struct gpio_rcar_info *info;
> >         struct of_phandle_args args;
> > -       int ret;
> > +       int index = 0, ret;
> >
> >         info = of_device_get_match_data(&p->pdev->dev);
> >
> > -       ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
> &args);
> > -       *npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;
> >         p->has_both_edge_trigger = info->has_both_edge_trigger;
> > +       *npins = 0;
> > +       for (;; index++) {
> > +               ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> > +                                                       index, &args);
> > +               if (ret) {
> > +                       if (index == 0)
> > +                               *npins = RCAR_MAX_GPIO_PER_BANK;
> > +                       break;
> > +               }
> > +               *npins += args.args[2];
> > +       }
>
> So after this, *npins will be the total number of GPIOs present in this bank
> (17 + 3 = 20), which will be used as gpio_chip.ngpio.
>
> All GPIO operations will use the passed offset as the bit number.
> gpio_rcar_resume() will resume bits 0..ngpio - 1 of each register.
> gpio_rcar_set_multiple() uses GENMASK(chip->ngpio - 1, 0) as the bank
> mask, and uses it to mask of bits in the registers.
>
> Hence all of the above assumes the register bits for the GPIOs are 0..19.
> However, according to the datasheet, the GPIOs in bank 3 are 0..16 and
> 27..29. So accessing GPIOs 17..19 in the bank will write to bits 17..19, not to
> 27..29!

I have done the below  GPIO operation using sysfs to check this. (As per this gpio 943 mapped to gpiobit 17 which request Pin123(GP3_27)
But I haven't checked GPIO registers to check whether it is actually setting the values to  GP3_17 or GP3_27. Will check this.

root@iwg23s:~# echo 943 > /sys/class/gpio/export
[   51.796570] sh-pfc e6060000.pin-controller: request pin 123 (GP_3_27) for e6053000.gpio:943
[   51.804960] sh-pfc e6060000.pin-controller: write_reg addr = e6060010, value = 0x0, field = 4, r_width = 32, f_width = 1

Kernel logs:-
----------------
[    0.110156] gpio gpiochip3: (e6053000.gpio): created GPIO range 0->16 ==> e6060000.pin-controller PIN 96->112
[    0.110187] gpio gpiochip3: (e6053000.gpio): created GPIO range 17->19 ==> e6060000.pin-controller PIN 123->125
[    0.110363] gpio gpiochip3: (e6053000.gpio): added GPIO chardev (254:3)
[    0.110460] gpiochip_setup_dev: registered GPIOs 926 to 945 on device: gpiochip3 (e6053000.gpio)

PIncontrol  related for GPIO 3
----------------------------------------

pin 96 (GP_3_0): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 97 (GP_3_1): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 98 (GP_3_2): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 99 (GP_3_3): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 100 (GP_3_4): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 101 (GP_3_5): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 102 (GP_3_6): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 103 (GP_3_7): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 104 (GP_3_8): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 105 (GP_3_9): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 106 (GP_3_10): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 107 (GP_3_11): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 108 (GP_3_12): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 109 (GP_3_13): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 110 (GP_3_14): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 111 (GP_3_15): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 112 (GP_3_16): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 123 (GP_3_27): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 124 (GP_3_28): (MUX UNCLAIMED) (GPIO UNCLAIMED)
 pin 125 (GP_3_29): (MUX UNCLAIMED) (GPIO UNCLAIMED)

range seen by pin control driver
-------------------------------------------
cat /sys/kernel/debug/pinctrl/e6060000.pin-controller-sh-pfc/gpio-ranges

0: e6050000.gpio GPIOS [1001 - 1023] PINS [0 - 22]
 0: e6051000.gpio GPIOS [978 - 1000] PINS [32 - 54]
 0: e6052000.gpio GPIOS [946 - 977] PINS [64 - 95]
 0: e6053000.gpio GPIOS [926 - 942] PINS [96 - 112]
 17: e6053000.gpio GPIOS [943 - 945] PINS [123 - 125]-- > New gpio range mapping pin 123-125
 0: e6054000.gpio GPIOS [900 - 925] PINS [128 - 153]
 0: e6055000.gpio GPIOS [868 - 899] PINS [160 - 191]

range seen by gpio driver
-----------------------------
[    0.105883] gpio_rcar e6050000.gpio: driving 23 GPIOs
[    0.106328] gpio_rcar e6051000.gpio: driving 23 GPIOs
[    0.106682] gpio_rcar e6052000.gpio: driving 32 GPIOs
[    0.107028] gpio_rcar e6053000.gpio: driving 20 GPIOs
[    0.107388] gpio_rcar e6054000.gpio: driving 26 GPIOs
[    0.107718] gpio_rcar e6055000.gpio: driving 32 GPIOs

Regards,
Biju

> A simple way to work around this is to set ngpios to the highest bit number in
> use + 1. But you still need a mechanism to avoid accessing the unused bits in
> the gap between 16 and 27.

> >
> >         if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) {
> >                 dev_warn(&p->pdev->dev,
>
> 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



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.