linux-kernel - Re: [PATCH v3 1/3] pinctrl: renesas: rzg2l: Include pinmap in RZG2L_GPIO_PORT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Dec 2023 14:13:10 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Prabhakar <prabhakar.csengg@...il.com>
Cc:     Magnus Damm <magnus.damm@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
        linux-gpio@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 1/3] pinctrl: renesas: rzg2l: Include pinmap in
 RZG2L_GPIO_PORT_PACK() macro

Hi Prabhakar,

On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Currently we assume all the port pins are sequential ie always PX_0 to
> PX_n (n=1..7) exist, but on RZ/Five SoC we have additional pins P19_1 to
> P28_5 which have holes in them, for example only one pin on port19 is
> available and that is P19_1 and not P19_0. So to handle such cases
> include pinmap for each port which would indicate the pin availability
> on each port. As the pincount can be calculated based on pinmap drop this
> from RZG2L_GPIO_PORT_PACK() macro and update RZG2L_GPIO_PORT_GET_PINCNT()
> macro.
>
> Previously we had a max of 7 pins on each port but on RZ/Five Port-20
> has 8 pins, so move the single pin configuration to BIT(63).
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -80,15 +80,17 @@
>   * n indicates number of pins in the port, a is the register index
>   * and f is pin configuration capabilities supported.
>   */
> -#define RZG2L_GPIO_PORT_PACK(n, a, f)  (((n) << 28) | ((a) << 20) | (f))
> -#define RZG2L_GPIO_PORT_GET_PINCNT(x)  (((x) & GENMASK(30, 28)) >> 28)
> +#define RZG2L_GPIO_PORT_PACK(n, a, f)  (((n) > 0 ? ((u64)(GENMASK_ULL(((n) - 1 + 28), 28))) : 0) | \

The mask creation can be simplified to

    ((1ULL << (n)) - 1) << 28

but see below...

> +                                        ((a) << 20) | (f))
> +#define RZG2L_GPIO_PORT_GET_PINMAP(x)  (((x) & GENMASK_ULL(35, 28)) >> 28)
> +#define RZG2L_GPIO_PORT_GET_PINCNT(x)  (hweight8(RZG2L_GPIO_PORT_GET_PINMAP((x))))

I think we've reached the point where it would be easier for the
casual reviewer to #define PIN_CFG_*_MASK for all fields, and use
FIELD_{PREP,GET}() to pack resp. extract values.  That would also
make it more obvious which bits are in use, and how many bits are
still available for future use.

>
>  /*
> - * BIT(31) indicates dedicated pin, p is the register index while
> + * BIT(63) indicates dedicated pin, p is the register index while
>   * referencing to SR/IEN/IOLH/FILxx registers, b is the register bits
>   * (b * 8) and f is the pin configuration capabilities supported.
>   */
> -#define RZG2L_SINGLE_PIN               BIT(31)
> +#define RZG2L_SINGLE_PIN               BIT_ULL(63)
>  #define RZG2L_SINGLE_PIN_PACK(p, b, f) (RZG2L_SINGLE_PIN | \
>                                          ((p) << 24) | ((b) << 20) | (f))
>  #define RZG2L_SINGLE_PIN_GET_BIT(x)    (((x) & GENMASK(22, 20)) >> 20)

Likewise.

> @@ -180,12 +182,12 @@ struct rzg2l_hwcfg {
>
>  struct rzg2l_dedicated_configs {
>         const char *name;
> -       u32 config;
> +       u64 config;
>  };

The rest LGTM.  It's a pity we have to switch to 64 bits, but I'm
afraid there is not much we can do about that...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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