Received-SPF: pass (google.com: domain of linux-kernel+bounces-9071-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231201131551.201503-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20231201131551.201503-2-prabhakar.mahadev-lad.rj@bp.renesas.com> <CAMuHMdUiaL__+CDaFxRbUFgrz69SYBNfZm4JvY_qQRKLMTCY0w@mail.gmail.com>
In-Reply-To: <CAMuHMdUiaL__+CDaFxRbUFgrz69SYBNfZm4JvY_qQRKLMTCY0w@mail.gmail.com>
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date: Thu, 21 Dec 2023 21:04:12 +0000
Message-ID: <CA+V-a8tTWf8Kx-Ex=DPsSR2ZWHC29N_pAoEZN1sR5Nqobf139A@mail.gmail.com>
Subject: Re: [PATCH v3 1/3] pinctrl: renesas: rzg2l: Include pinmap in
 RZG2L_GPIO_PORT_PACK() macro
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Magnus Damm <magnus.damm@gmail.com>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Albert Ou <aou@eecs.berkeley.edu>, Linus Walleij <linus.walleij@linaro.org>, 
	linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-riscv@lists.infradead.org, 
	linux-gpio@vger.kernel.org, Biju Das <biju.das.jz@bp.renesas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Geert,

Thank you for the review.

On Wed, Dec 6, 2023 at 1:13=E2=80=AFPM Geert Uytterhoeven <geert@linux-m68k=
.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Dec 1, 2023 at 2:16=E2=80=AFPM Prabhakar <prabhakar.csengg@gmail.=
com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Currently we assume all the port pins are sequential ie always PX_0 to
> > PX_n (n=3D1..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 th=
is
> > 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@bp.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
>
OK.

> 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_P=
INMAP((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.
>
If I use the FIELD_PREP() macro like below I get build issues as below:

#define RZG2L_GPIO_PORT_PIN_CNT_MASK    GENMASK(31, 28)
#define RZG2L_GPIO_PORT_PIN_REG_MASK    GENMASK(27, 20)
#define RZG2L_GPIO_PORT_PIN_CFG_MASK    GENMASK(19, 0)
#define RZG2L_GPIO_PORT_PACK(n, a, f)
FIELD_PREP(RZG2L_GPIO_PORT_PIN_CNT_MASK, n) | \
                    FIELD_PREP(RZG2L_GPIO_PORT_PIN_REG_MASK, a) | \
                    FIELD_PREP(RZG2L_GPIO_PORT_PIN_CFG_MASK, f)


drivers/pinctrl/renesas/pinctrl-rzg2l.c:91:41: note: in expansion of
macro 'FIELD_PREP'
   91 |
FIELD_PREP(RZG2L_GPIO_PORT_PIN_CFG_MASK, f)
      |                                         ^~~~~~~~~~
drivers/pinctrl/renesas/pinctrl-rzg2l.c:1486:9: note: in expansion of
macro 'RZG2L_GPIO_PORT_PACK'
 1486 |         RZG2L_GPIO_PORT_PACK(6, 0x2a,
RZG3S_MPXED_PIN_FUNCS(A)),                        /* P18 */
      |         ^~~~~~~~~~~~~~~~~~~~

Do you have any pointers?

Cheers,
Prabhakar

> >
> >  /*
> > - * 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@linux-m6=
8k.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