From: Biju Das <biju.das.jz@bp.renesas.com>
To: geert <geert@linux-m68k.org>, biju.das.au <biju.das.au@gmail.com>
Cc: Linus Walleij <linusw@kernel.org>,
Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>,
"linux-renesas-soc@vger.kernel.org"
<linux-renesas-soc@vger.kernel.org>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: RE: [PATCH v2] pinctrl: renesas: rzg2l: Fix save/restore of {IOLH,IEN,PUPD,SMT} registers
Date: Thu, 26 Mar 2026 15:06:37 +0000 [thread overview]
Message-ID: <TY3PR01MB11346A5A7DA755F372ECE3D968656A@TY3PR01MB11346.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdW5jiR5CqV0xhxio4vtV_bPczO5u1Da86CWJO+=6WU4+Q@mail.gmail.com>
Hi Geert,
Thanks for the feedback
> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 26 March 2026 11:31
> Subject: Re: [PATCH v2] pinctrl: renesas: rzg2l: Fix save/restore of {IOLH,IEN,PUPD,SMT} registers
>
> Hi Biju,
>
> On Wed, 25 Mar 2026 at 19:28, Biju <biju.das.au@gmail.com> wrote:
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > The rzg2l_pinctrl_pm_setup_regs() handles save/restore of
> > {IOLH,IEN,PUPD,SMT} registers during s2ram, but only for ports where
> > all pins share the same pincfg. Extend the code to also support ports
> > with variable pincfg per pin, so that {IOLH,IEN,PUPD,SMT} registers
> > are correctly saved and restored for all pins.
> >
> > Fixes: 254203f9a94c ("pinctrl: renesas: rzg2l: Add suspend/resume
> > support")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Thanks for the update!
>
> > ---
> > v1->v2:
> > * Updated commit description
> > * Improved the logic.
>
> You also fixed the double application of the pin_off index, right?
Yes, it is my mistake, I haven't looked the proposed code during
our internal review as the testing showed the results are OK with ethernet OEN
Pins during suspend/resume cycle of s2ram.
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -3001,9 +3001,12 @@ static void rzg2l_pinctrl_pm_setup_regs(struct
> > rzg2l_pinctrl *pctrl, bool suspen {
> > u32 nports = pctrl->data->n_port_pins / RZG2L_PINS_PER_PORT;
> > struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
> > + u32 pin_off = 0;
> >
> > - for (u32 port = 0; port < nports; port++) {
> > + for (u32 port = 0; port < nports; port++, pin_off += RZG2L_PINS_PER_PORT) {
> > + const struct pinctrl_pin_desc *pin_desc =
> > + &pctrl->desc.pins[pin_off];
>
> Here you index pctrl->desc.pins[]...
OK.
>
> > bool has_iolh, has_ien, has_pupd, has_smt;
> > + u64 *pin_data = pin_desc->drv_data;
>
> Here you get pin_data, so below you will actually index
> pctrl->desc.pins[pin_off].drv_data[]. That is not only confusing,
> but also only works because pctrl->desc.pins[i].drv_data is initialized in rzg2l_pinctrl_register()
> like:
>
> pins[i].drv_data = &pin_data[i];
>
> All these new variables (incl. pin_off) are only used inside the new if () below, so please confine
> everything to that block.
OK.
>
> > u32 off, caps;
> > u8 pincnt;
> > u64 cfg;
> > @@ -3012,6 +3015,11 @@ static void rzg2l_pinctrl_pm_setup_regs(struct rzg2l_pinctrl *pctrl, bool
> suspen
> > off = RZG2L_PIN_CFG_TO_PORT_OFFSET(cfg);
> > pincnt = hweight8(FIELD_GET(PIN_CFG_PIN_MAP_MASK,
> > cfg));
> >
> > + if (cfg & RZG2L_VARIABLE_CFG) {
> > + for (unsigned int i = 1; i <
> > + RZG2L_PINS_PER_PORT; i++)
>
> Shouldn't the loop start at zero? The PIN_CFG_MASK part of a variable config is always zero.
>
> > + cfg |= *pin_data++;
>
> Please use pctrl->desc.pins[pin_off + i].drv_data here.
> And pin_off can be replaced by port * RZG2L_PINS_PER_PORT.
OK, will replace it as in the next version as below
+ if (cfg & RZG2L_VARIABLE_CFG) {
+ unsigned int pin = port * RZG2L_PINS_PER_PORT;
+
+ for (unsigned int i = 0; i < RZG2L_PINS_PER_PORT; i++)
+ cfg |= *(u64 *)pctrl->desc.pins[ pin + i ].drv_data;
+ }
+
Cheers,
Biju
prev parent reply other threads:[~2026-03-26 15:06 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-25 18:28 [PATCH v2] pinctrl: renesas: rzg2l: Fix save/restore of {IOLH,IEN,PUPD,SMT} registers Biju
2026-03-26 11:30 ` Geert Uytterhoeven
2026-03-26 15:06 ` Biju Das [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=TY3PR01MB11346A5A7DA755F372ECE3D968656A@TY3PR01MB11346.jpnprd01.prod.outlook.com \
--to=biju.das.jz@bp.renesas.com \
--cc=biju.das.au@gmail.com \
--cc=claudiu.beznea.uj@bp.renesas.com \
--cc=geert@linux-m68k.org \
--cc=linusw@kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).