* [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
@ 2019-02-16 13:49 marek.vasut
2019-02-18 8:43 ` Geert Uytterhoeven
0 siblings, 1 reply; 9+ messages in thread
From: marek.vasut @ 2019-02-16 13:49 UTC (permalink / raw)
To: linux-gpio
Cc: Marek Vasut, Geert Uytterhoeven, Linus Walleij, Simon Horman,
Wolfram Sang, linux-renesas-soc
From: Marek Vasut <marek.vasut+renesas@gmail.com>
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@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-gpio@vger.kernel.org
---
drivers/pinctrl/sh-pfc/core.c | 4 ++++
drivers/pinctrl/sh-pfc/pfc-r8a7795-es1.c | 10 ++++++++++
drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 10 ++++++++++
drivers/pinctrl/sh-pfc/pfc-r8a7796.c | 11 +++++++++++
drivers/pinctrl/sh-pfc/pfc-r8a77965.c | 10 ++++++++++
drivers/pinctrl/sh-pfc/pfc-r8a77970.c | 9 +++++++++
drivers/pinctrl/sh-pfc/pfc-r8a77980.c | 9 +++++++++
drivers/pinctrl/sh-pfc/pfc-r8a77990.c | 11 +++++++++++
drivers/pinctrl/sh-pfc/sh_pfc.h | 5 +++++
9 files changed, 79 insertions(+)
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index f1cfcc8c6544..d23cedbd58d4 100644
--- 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;
}
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795-es1.c b/drivers/pinctrl/sh-pfc/pfc-r8a7795-es1.c
index 287cfbb7e992..d5aa18705b6c 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7795-es1.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795-es1.c
@@ -5570,6 +5570,15 @@ static int r8a7795es1_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin,
return bit;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct pinmux_bias_reg pinmux_bias_regs[] = {
{ PINMUX_BIAS_REG("PUEN0", 0xe6060400, "PUD0", 0xe6060440) {
[ 0] = PIN_NUMBER('W', 3), /* QSPI0_SPCLK */
@@ -5877,6 +5886,7 @@ const struct sh_pfc_soc_info r8a7795es1_pinmux_info = {
.drive_regs = pinmux_drive_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
index db9add1405c5..7c21186cf624 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
@@ -5919,6 +5919,15 @@ static int r8a7795_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin, u32 *poc
return bit;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct pinmux_bias_reg pinmux_bias_regs[] = {
{ PINMUX_BIAS_REG("PUEN0", 0xe6060400, "PUD0", 0xe6060440) {
[ 0] = PIN_NUMBER('W', 3), /* QSPI0_SPCLK */
@@ -6240,6 +6249,7 @@ const struct sh_pfc_soc_info r8a7795_pinmux_info = {
.drive_regs = pinmux_drive_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7796.c b/drivers/pinctrl/sh-pfc/pfc-r8a7796.c
index 72348a4f2ece..1e4854ae2262 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7796.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7796.c
@@ -5877,6 +5877,15 @@ static int r8a7796_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin, u32 *poc
return bit;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct pinmux_bias_reg pinmux_bias_regs[] = {
{ PINMUX_BIAS_REG("PUEN0", 0xe6060400, "PUD0", 0xe6060440) {
[ 0] = PIN_NUMBER('W', 3), /* QSPI0_SPCLK */
@@ -6185,6 +6194,7 @@ const struct sh_pfc_soc_info r8a774a1_pinmux_info = {
.drive_regs = pinmux_drive_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
@@ -6212,6 +6222,7 @@ const struct sh_pfc_soc_info r8a7796_pinmux_info = {
.drive_regs = pinmux_drive_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77965.c b/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
index 14c4b671cddf..e3447fe7b7ae 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
@@ -6034,6 +6034,15 @@ static int r8a77965_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin, u32 *po
return bit;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct pinmux_bias_reg pinmux_bias_regs[] = {
{ PINMUX_BIAS_REG("PUEN0", 0xe6060400, "PUD0", 0xe6060440) {
[ 0] = PIN_NUMBER('W', 3), /* QSPI0_SPCLK */
@@ -6341,6 +6350,7 @@ const struct sh_pfc_soc_info r8a77965_pinmux_info = {
.drive_regs = pinmux_drive_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
index c5e67ba29f7c..55f68e53caae 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77970.c
@@ -2438,6 +2438,15 @@ static int r8a77970_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin,
return -EINVAL;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct sh_pfc_soc_operations pinmux_ops = {
.pin_to_pocctrl = r8a77970_pin_to_pocctrl,
};
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77980.c b/drivers/pinctrl/sh-pfc/pfc-r8a77980.c
index b807b67ae143..e892c0eeb203 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a77980.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77980.c
@@ -2867,6 +2867,15 @@ static int r8a77980_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin,
return -EINVAL;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct sh_pfc_soc_operations pinmux_ops = {
.pin_to_pocctrl = r8a77980_pin_to_pocctrl,
};
diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77990.c b/drivers/pinctrl/sh-pfc/pfc-r8a77990.c
index 151640c30e9d..2a3f05c6b365 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a77990.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a77990.c
@@ -5019,6 +5019,15 @@ static int r8a77990_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin,
return bit;
}
+enum tdsel_regs {
+ TDSELCTRL0,
+};
+
+static const struct pinmux_tdsel_reg pinmux_tdsel_regs[] = {
+ [TDSELCTRL0] = { 0xe60603c0, },
+ { /* sentinel */ },
+};
+
static const struct pinmux_bias_reg pinmux_bias_regs[] = {
{ PINMUX_BIAS_REG("PUEN0", 0xe6060400, "PUD0", 0xe6060440) {
[0] = RCAR_GP_PIN(2, 23), /* RD# */
@@ -5292,6 +5301,7 @@ const struct sh_pfc_soc_info r8a774c0_pinmux_info = {
.cfg_regs = pinmux_config_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
@@ -5318,6 +5328,7 @@ const struct sh_pfc_soc_info r8a77990_pinmux_info = {
.cfg_regs = pinmux_config_regs,
.bias_regs = pinmux_bias_regs,
.ioctrl_regs = pinmux_ioctrl_regs,
+ .tdsel_regs = pinmux_tdsel_regs,
.pinmux_data = pinmux_data,
.pinmux_data_size = ARRAY_SIZE(pinmux_data),
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index 56016cb76769..56e08f2c5cbf 100644
--- 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;
--
2.19.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-16 13:49 [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume marek.vasut
@ 2019-02-18 8:43 ` Geert Uytterhoeven
2019-02-18 12:58 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-02-18 8:43 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
Hi Marek,
On Sat, Feb 16, 2019 at 2:49 PM <marek.vasut@gmail.com> 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@gmail.com>
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/.
> --- 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?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 8:43 ` Geert Uytterhoeven
@ 2019-02-18 12:58 ` Marek Vasut
2019-02-18 13:32 ` Geert Uytterhoeven
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2019-02-18 12:58 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
On 2/18/19 9:43 AM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> On Sat, Feb 16, 2019 at 2:49 PM <marek.vasut@gmail.com> 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@gmail.com>
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 12:58 ` Marek Vasut
@ 2019-02-18 13:32 ` Geert Uytterhoeven
2019-02-18 13:38 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-02-18 13:32 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
Hi Marek,
On Mon, Feb 18, 2019 at 1:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 2/18/19 9:43 AM, Geert Uytterhoeven wrote:
> > On Sat, Feb 16, 2019 at 2:49 PM <marek.vasut@gmail.com> 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@gmail.com>
> >
> > 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@linux-m68k.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 13:32 ` Geert Uytterhoeven
@ 2019-02-18 13:38 ` Marek Vasut
2019-02-18 13:41 ` Geert Uytterhoeven
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2019-02-18 13:38 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
On 2/18/19 2:32 PM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
[...]
>>>> 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.
I thought the array is called pinmux_ioctrl_reg[] because that's what
the pocctrl was called in older datasheets ? At least that's how you
explained it on IRC last time.
>> 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[].
If that's what you think is better and makes the driver less confusing,
sure ...
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 13:38 ` Marek Vasut
@ 2019-02-18 13:41 ` Geert Uytterhoeven
2019-02-18 13:47 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-02-18 13:41 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
Hi Marek,
On Mon, Feb 18, 2019 at 2:38 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 2/18/19 2:32 PM, Geert Uytterhoeven wrote:
> >>>> 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.
>
> I thought the array is called pinmux_ioctrl_reg[] because that's what
> the pocctrl was called in older datasheets ? At least that's how you
> explained it on IRC last time.
Ah, that's where the misunderstanding comes from: both POCCTRLx and
TDSELy registers are sometimes called IOCTRLz registers.
> >> 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[].
>
> If that's what you think is better and makes the driver less confusing,
> sure ...
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 13:41 ` Geert Uytterhoeven
@ 2019-02-18 13:47 ` Marek Vasut
2019-02-18 13:52 ` Geert Uytterhoeven
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2019-02-18 13:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
On 2/18/19 2:41 PM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> On Mon, Feb 18, 2019 at 2:38 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 2/18/19 2:32 PM, Geert Uytterhoeven wrote:
>>>>>> 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.
>>
>> I thought the array is called pinmux_ioctrl_reg[] because that's what
>> the pocctrl was called in older datasheets ? At least that's how you
>> explained it on IRC last time.
>
> Ah, that's where the misunderstanding comes from: both POCCTRLx and
> TDSELy registers are sometimes called IOCTRLz registers.
>
Then shouldn't we rename IOCTRL30 to POCCTRL first, and then add TDSEL
into the list ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 13:47 ` Marek Vasut
@ 2019-02-18 13:52 ` Geert Uytterhoeven
2019-02-18 13:55 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-02-18 13:52 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
Hi Marek,
On Mon, Feb 18, 2019 at 2:48 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 2/18/19 2:41 PM, Geert Uytterhoeven wrote:
> > On Mon, Feb 18, 2019 at 2:38 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 2/18/19 2:32 PM, Geert Uytterhoeven wrote:
> >>>>>> 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.
> >>
> >> I thought the array is called pinmux_ioctrl_reg[] because that's what
> >> the pocctrl was called in older datasheets ? At least that's how you
> >> explained it on IRC last time.
> >
> > Ah, that's where the misunderstanding comes from: both POCCTRLx and
> > TDSELy registers are sometimes called IOCTRLz registers.
> >
>
> Then shouldn't we rename IOCTRL30 to POCCTRL first, and then add TDSEL
> into the list ?
Sure. I've already done so, but haven't sent out the patches yet.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume
2019-02-18 13:52 ` Geert Uytterhoeven
@ 2019-02-18 13:55 ` Marek Vasut
0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2019-02-18 13:55 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:GPIO SUBSYSTEM, Marek Vasut, Geert Uytterhoeven,
Linus Walleij, Simon Horman, Wolfram Sang, Linux-Renesas
On 2/18/19 2:52 PM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> On Mon, Feb 18, 2019 at 2:48 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 2/18/19 2:41 PM, Geert Uytterhoeven wrote:
>>> On Mon, Feb 18, 2019 at 2:38 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 2/18/19 2:32 PM, Geert Uytterhoeven wrote:
>>>>>>>> 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.
>>>>
>>>> I thought the array is called pinmux_ioctrl_reg[] because that's what
>>>> the pocctrl was called in older datasheets ? At least that's how you
>>>> explained it on IRC last time.
>>>
>>> Ah, that's where the misunderstanding comes from: both POCCTRLx and
>>> TDSELy registers are sometimes called IOCTRLz registers.
>>>
>>
>> Then shouldn't we rename IOCTRL30 to POCCTRL first, and then add TDSEL
>> into the list ?
>
> Sure. I've already done so, but haven't sent out the patches yet.
Then we'll have a conflict once I add the TDSEL. I can either cook
similar patch and send two patches or you send yours and I'll send this
one later. I'd prefer the former to make conflict resolution easier .
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-18 14:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-16 13:49 [PATCH] pinctrl: sh-pfc: Retain TDSELCTRL register across suspend/resume marek.vasut
2019-02-18 8:43 ` Geert Uytterhoeven
2019-02-18 12:58 ` Marek Vasut
2019-02-18 13:32 ` Geert Uytterhoeven
2019-02-18 13:38 ` Marek Vasut
2019-02-18 13:41 ` Geert Uytterhoeven
2019-02-18 13:47 ` Marek Vasut
2019-02-18 13:52 ` Geert Uytterhoeven
2019-02-18 13:55 ` Marek Vasut
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 NNTP newsgroup(s).