Received-SPF: pass (google.com: domain of linux-kernel+bounces-22539-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>
 <CA+V-a8tTWf8Kx-Ex=DPsSR2ZWHC29N_pAoEZN1sR5Nqobf139A@mail.gmail.com>
 <CAMuHMdXosvV=EuRtL69r6=UT0SO8Aq-XjWwJMJQpWAhT2z+ffA@mail.gmail.com>
 <CA+V-a8tNeiyvK90urPF9s3JZOav77TRO8xdAEiCmYurrf3-4RA@mail.gmail.com> <CAMuHMdXYEBg7MMJG7tqDib8eaKdvd4Fxo+ZR8K8Vq82DzjSaqg@mail.gmail.com>
In-Reply-To: <CAMuHMdXYEBg7MMJG7tqDib8eaKdvd4Fxo+ZR8K8Vq82DzjSaqg@mail.gmail.com>
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date: Wed, 10 Jan 2024 17:30:01 +0000
Message-ID: <CA+V-a8sBWTd4UfSm=mFN96aLL7BvFMTovfaXjdbC4uH=UDR_Fg@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,

On Thu, Jan 4, 2024 at 4:25=E2=80=AFPM Geert Uytterhoeven <geert@linux-m68k=
org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Jan 4, 2024 at 4:55=E2=80=AFPM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Tue, Jan 2, 2024 at 10:18=E2=80=AFAM Geert Uytterhoeven <geert@linux=
-m68k.org> wrote:
> > > On Thu, Dec 21, 2023 at 10:04=E2=80=AFPM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
> > > > On Wed, Dec 6, 2023 at 1:13=E2=80=AFPM Geert Uytterhoeven <geert@li=
nux-m68k.org> wrote:
> > > > > On Fri, Dec 1, 2023 at 2:16=E2=80=AFPM Prabhakar <prabhakar.cseng=
g@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 pi=
ns P19_1 to
> > > > > > P28_5 which have holes in them, for example only one pin on por=
t19 is
> > > > > > available and that is P19_1 and not P19_0. So to handle such ca=
ses
> > > > > > include pinmap for each port which would indicate the pin avail=
ability
> > > > > > 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_GE=
T_PINCNT()
> > > > > > macro.
> > > > > >
> > > > > > Previously we had a max of 7 pins on each port but on RZ/Five P=
ort-20
> > > > > > has 8 pins, so move the single pin configuration to BIT(63).
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renes=
as.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 i=
ndex
> > > > > >   * 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)(GENM=
ASK_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_PO=
RT_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.
>
> > To clarify, you mean to define PIN_CFG_*_MASK for all
> > PIN_CFG_IOLH_A..PIN_CFG_OEN macros? I ask because we dont extract the
> > respective CFG flags in the code.
>
> The PIN_CFG_IOLH_A..PIN_CFG_OEN macros are single-bit definitions.
> I mean to #define PIN_CFG_*_MASK macros for all multi-bit fields, current=
ly
> accessed using open-coded GENMASK().
>
> You already tried:
>
>     #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)
>
> As they actually share the PIN_CFG_* bit space, I'd call them:
>
>     #define PIN_CFG_PIN_CNT_MASK    GENMASK(31, 28)
>     #define PIN_CFG_PIN_REG_MASK    GENMASK(27, 20)
>     #define PIN_CFG_MASK    GENMASK(19, 0)
>
> Also, you already have:
>
>     #define MUX_PIN_ID_MASK         GENMASK(15, 0)
>     #define MUX_FUNC_MASK           GENMASK(31, 16)
>     #define MUX_FUNC_OFFS           16
>
> But all of
>
>     #define MUX_FUNC(pinconf)       (((pinconf) & MUX_FUNC_MASK) >>
> MUX_FUNC_OFFS)
>
>     pins[i] =3D value & MUX_PIN_ID_MASK;
>
> can use FIELD_GET(), removing the need for MUX_FUNC_OFFS.
>
> Also:
>
>     u8 pincount =3D RZG2L_GPIO_PORT_GET_PINCNT(cfg);
>
> can become
>
>     u8 pincount =3D FIELD_GET(PIN_CFG_PIN_CNT_MASK, cfg);
>
> Same for all the other macros using GENMASK().
>
> I hope this makes it more clear what I had in mind?
> Thanks!
>
Thanks for the detailed explanation. I'll get that sorted soon.

Cheers,
Prabhakar