Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Marek,

On Mon, Feb 18, 2019 at 1:59 PM Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
> On 2/18/19 9:43 AM, Geert Uytterhoeven wrote:
> > On Sat, Feb 16, 2019 at 2:49 PM <marek.vasut@xxxxxxxxx> wrote:
> >> The TDSELCTRL register is responsible for configuring the SDHI clock
> >> return path delay and may be adjusted by the bootloader. Retain the
> >> value across suspend/resume to prevent hardware instability after
> >> resume.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > Looks good to me. Two comments though.
> >
> > R-Car D3 (r8a77995) also has a TDSEL register, but it is not handled by
> > your patch.  Adding that would mean s/SDHI/SDHI and MMC/.
>
> Added

Thanks!

> >> --- a/drivers/pinctrl/sh-pfc/core.c
> >> +++ b/drivers/pinctrl/sh-pfc/core.c
> >> @@ -657,6 +657,10 @@ static unsigned int sh_pfc_walk_regs(struct sh_pfc *pfc,
> >>                 for (i = 0; pfc->info->ioctrl_regs[i].reg; i++)
> >>                         do_reg(pfc, pfc->info->ioctrl_regs[i].reg, n++);
> >>
> >> +       if (pfc->info->tdsel_regs)
> >> +               for (i = 0; pfc->info->tdsel_regs[i].reg; i++)
> >> +                       do_reg(pfc, pfc->info->tdsel_regs[i].reg, n++);
> >> +
> >>         return n;
> >>  }
> >
> >> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> >> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> >> @@ -176,6 +176,10 @@ struct pinmux_ioctrl_reg {
> >>         u32 reg;
> >>  };
> >>
> >> +struct pinmux_tdsel_reg {
> >> +       u32 reg;
> >> +};
> >> +
> >>  struct pinmux_data_reg {
> >>         u32 reg;
> >>         u8 reg_width;
> >> @@ -270,6 +274,7 @@ struct sh_pfc_soc_info {
> >>         const struct pinmux_drive_reg *drive_regs;
> >>         const struct pinmux_bias_reg *bias_regs;
> >>         const struct pinmux_ioctrl_reg *ioctrl_regs;
> >> +       const struct pinmux_tdsel_reg *tdsel_regs;
> >>         const struct pinmux_data_reg *data_regs;
> >>
> >>         const u16 *pinmux_data;
> >
> > Is there any special reason why you added a new block of registers with
> > separate handling, instead of adding TDSEL to the existing
> > pinmux_ioctrl_reg[] arrays, which list other IOCTRL registers like
> > POCCTRL?
>
> For one, It's unrelated register to POCCTRL, so I don't want to mix them

That's why the array is called pinmux_ioctrl_reg[], not pinmux_pocctrl_reg[]:
it is meant to cover various I/O control registers, including POCCTRL and
TDSEL, to be saved/restored for PSCI system suspend.

> together. The other reason is this r8a779*_pin_to_pocctrl(), which uses
> the ioctrl_regs[] array ; if I added the TDSEL there, that'd just make
> things extra confusing.

How r8a779*_pin_to_pocctrl() gets the registers is an implementation detail,
IMHO (The function used to contain hardcoded register addresses).

So I think you should just add the TDSEL registers to pinmux_ioctrl_reg[].

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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