On 2/18/19 9:43 AM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> 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
>> --- 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
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.
--
Best regards,
Marek Vasut
