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
