[PATCH 0/5] pinctrl: renesas: Fixes and checker improvements

[PATCH 0/5] pinctrl: renesas: Fixes and checker improvements

[Geert Uytterhoeven]

	Hi all,

This is a small set of fixes and improvements for the Renesas pin
control drivers.  Most of them are related to the built-in checker,
which is only enabled in DEBUG builds.

I have a few more checks pending, but they need more polishing to make
them suitable for upstream.  Note that there are no pending issues in
the actual driver that can be detected by these checker improvements, as
all fixes for such issues are already upstream.

To be queued in renesas-pinctrl for v5.16.

Thanks for your comments!

Geert Uytterhoeven (5):
  pinctrl: renesas: Fix save/restore on SoCs with pull-down only pins
  pinctrl: renesas: checker: Fix off-by-one bug in drive register check
  pinctrl: renesas: checker: Move overlapping field check
  pinctrl: renesas: checker: Fix bias checks on SoCs with pull-down only
    pins
  pinctrl: renesas: checker: Prefix common checker output

 drivers/pinctrl/renesas/core.c | 73 +++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 28 deletions(-)

-- 
2.25.1

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

[PATCH 1/5] pinctrl: renesas: Fix save/restore on SoCs with pull-down only pins

[Geert Uytterhoeven]

If some bits in a pin Pull-Up control register (PUPR) control pin
pull-down instead of pin pull-up, there are two pinmux_bias_reg entries:
a first one with the puen field filled in, listing pins with pull-up
functionality, and a second one with the pud field filled in, listing
pins with pull-down functionality.  On encountering the second entry,
where puen is NULL, the for-loop terminates early, causing the remaining
bias registers not to be saved/restored during PSCI system suspend.
Fortunately this does not trigger on any supported system yet, as PSCI
is only used on R-Car Gen3 and RZ/G2 systems, which all have separate
pin Pull-Enable (PUEN) and pin Pull-Up/Down control (PUD) registers.

Avoid this ever becoming a problem by treating pinmux_bias_reg.puen and
pinmux_bias_reg.pud the same.  Note that a register controlling both
pull-up and pull-down pins would be saved and restored twice, which is
harmless.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/renesas/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/renesas/core.c b/drivers/pinctrl/renesas/core.c
index ef8ef05ba93030b9..b0d6103d012e7821 100644
--- a/drivers/pinctrl/renesas/core.c
+++ b/drivers/pinctrl/renesas/core.c
@@ -675,8 +675,10 @@ static unsigned int sh_pfc_walk_regs(struct sh_pfc *pfc,
 			do_reg(pfc, pfc->info->drive_regs[i].reg, n++);
 
 	if (pfc->info->bias_regs)
-		for (i = 0; pfc->info->bias_regs[i].puen; i++) {
-			do_reg(pfc, pfc->info->bias_regs[i].puen, n++);
+		for (i = 0; pfc->info->bias_regs[i].puen ||
+			    pfc->info->bias_regs[i].pud; i++) {
+			if (pfc->info->bias_regs[i].puen)
+				do_reg(pfc, pfc->info->bias_regs[i].puen, n++);
 			if (pfc->info->bias_regs[i].pud)
 				do_reg(pfc, pfc->info->bias_regs[i].pud, n++);
 		}
-- 
2.25.1

[PATCH 2/5] pinctrl: renesas: checker: Fix off-by-one bug in drive register check

[Geert Uytterhoeven]

The GENMASK(h, l) macro creates a contiguous bitmask starting at bit
position @l and ending at position @h, inclusive.

This did not trigger any error checks, as the individual register fields
cover at most 3 of the 4 available bits.

Fixes: 08df16e07ad0a1ec ("pinctrl: sh-pfc: checker: Add drive strength register checks")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/renesas/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/renesas/core.c b/drivers/pinctrl/renesas/core.c
index b0d6103d012e7821..464d07ead56879fc 100644
--- a/drivers/pinctrl/renesas/core.c
+++ b/drivers/pinctrl/renesas/core.c
@@ -892,7 +892,7 @@ static void __init sh_pfc_check_drive_reg(const struct sh_pfc_soc_info *info,
 		if (!field->pin && !field->offset && !field->size)
 			continue;
 
-		mask = GENMASK(field->offset + field->size, field->offset);
+		mask = GENMASK(field->offset + field->size - 1, field->offset);
 		if (mask & seen)
 			sh_pfc_err("drive_reg 0x%x: field %u overlap\n",
 				   drive->reg, i);
-- 
2.25.1

[PATCH 3/5] pinctrl: renesas: checker: Move overlapping field check

[Geert Uytterhoeven]

Move the check for overlapping drive register fields from
sh_pfc_check_drive_reg() to sh_pfc_check_reg(), so it can be used for
other register types, too.  This requires passing the covered register
bits to sh_pfc_check_reg().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/renesas/core.c | 55 +++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/renesas/core.c b/drivers/pinctrl/renesas/core.c
index 464d07ead56879fc..ab8bdb81e8df5069 100644
--- a/drivers/pinctrl/renesas/core.c
+++ b/drivers/pinctrl/renesas/core.c
@@ -745,7 +745,10 @@ static int sh_pfc_suspend_init(struct sh_pfc *pfc) { return 0; }
 
 static unsigned int sh_pfc_errors __initdata;
 static unsigned int sh_pfc_warnings __initdata;
-static u32 *sh_pfc_regs __initdata;
+static struct {
+	u32 reg;
+	u32 bits;
+} *sh_pfc_regs __initdata;
 static u32 sh_pfc_num_regs __initdata;
 static u16 *sh_pfc_enums __initdata;
 static u32 sh_pfc_num_enums __initdata;
@@ -780,22 +783,30 @@ static bool __init same_name(const char *a, const char *b)
 	return !strcmp(a, b);
 }
 
-static void __init sh_pfc_check_reg(const char *drvname, u32 reg)
+static void __init sh_pfc_check_reg(const char *drvname, u32 reg, u32 bits)
 {
 	unsigned int i;
 
-	for (i = 0; i < sh_pfc_num_regs; i++)
-		if (reg == sh_pfc_regs[i]) {
-			sh_pfc_err("reg 0x%x conflict\n", reg);
-			return;
-		}
+	for (i = 0; i < sh_pfc_num_regs; i++) {
+		if (reg != sh_pfc_regs[i].reg)
+			continue;
+
+		if (bits & sh_pfc_regs[i].bits)
+			sh_pfc_err("reg 0x%x: bits 0x%x conflict\n", reg,
+				   bits & sh_pfc_regs[i].bits);
+
+		sh_pfc_regs[i].bits |= bits;
+		return;
+	}
 
 	if (sh_pfc_num_regs == SH_PFC_MAX_REGS) {
 		pr_warn_once("%s: Please increase SH_PFC_MAX_REGS\n", drvname);
 		return;
 	}
 
-	sh_pfc_regs[sh_pfc_num_regs++] = reg;
+	sh_pfc_regs[sh_pfc_num_regs].reg = reg;
+	sh_pfc_regs[sh_pfc_num_regs].bits = bits;
+	sh_pfc_num_regs++;
 }
 
 static int __init sh_pfc_check_enum(const char *drvname, u16 enum_id)
@@ -850,7 +861,8 @@ static void __init sh_pfc_check_cfg_reg(const char *drvname,
 {
 	unsigned int i, n, rw, fw;
 
-	sh_pfc_check_reg(drvname, cfg_reg->reg);
+	sh_pfc_check_reg(drvname, cfg_reg->reg,
+			 GENMASK(cfg_reg->reg_width - 1, 0));
 
 	if (cfg_reg->field_width) {
 		n = cfg_reg->reg_width / cfg_reg->field_width;
@@ -881,22 +893,17 @@ static void __init sh_pfc_check_cfg_reg(const char *drvname,
 static void __init sh_pfc_check_drive_reg(const struct sh_pfc_soc_info *info,
 					  const struct pinmux_drive_reg *drive)
 {
-	const char *drvname = info->name;
-	unsigned long seen = 0, mask;
 	unsigned int i;
 
-	sh_pfc_check_reg(info->name, drive->reg);
 	for (i = 0; i < ARRAY_SIZE(drive->fields); i++) {
 		const struct pinmux_drive_reg_field *field = &drive->fields[i];
 
 		if (!field->pin && !field->offset && !field->size)
 			continue;
 
-		mask = GENMASK(field->offset + field->size - 1, field->offset);
-		if (mask & seen)
-			sh_pfc_err("drive_reg 0x%x: field %u overlap\n",
-				   drive->reg, i);
-		seen |= mask;
+		sh_pfc_check_reg(info->name, drive->reg,
+				 GENMASK(field->offset + field->size - 1,
+					 field->offset));
 
 		sh_pfc_check_pin(info, drive->reg, field->pin);
 	}
@@ -906,10 +913,15 @@ static void __init sh_pfc_check_bias_reg(const struct sh_pfc_soc_info *info,
 					 const struct pinmux_bias_reg *bias)
 {
 	unsigned int i;
+	u32 bits;
+
+	for (i = 0, bits = 0; i < ARRAY_SIZE(bias->pins); i++)
+		if (bias->pins[i] != SH_PFC_PIN_NONE)
+			bits |= BIT(i);
 
-	sh_pfc_check_reg(info->name, bias->puen);
+	sh_pfc_check_reg(info->name, bias->puen, bits);
 	if (bias->pud)
-		sh_pfc_check_reg(info->name, bias->pud);
+		sh_pfc_check_reg(info->name, bias->pud, bits);
 	for (i = 0; i < ARRAY_SIZE(bias->pins); i++)
 		sh_pfc_check_pin(info, bias->puen, bias->pins[i]);
 }
@@ -1017,11 +1029,12 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info)
 
 	/* Check ioctrl registers */
 	for (i = 0; info->ioctrl_regs && info->ioctrl_regs[i].reg; i++)
-		sh_pfc_check_reg(drvname, info->ioctrl_regs[i].reg);
+		sh_pfc_check_reg(drvname, info->ioctrl_regs[i].reg, U32_MAX);
 
 	/* Check data registers */
 	for (i = 0; info->data_regs && info->data_regs[i].reg; i++) {
-		sh_pfc_check_reg(drvname, info->data_regs[i].reg);
+		sh_pfc_check_reg(drvname, info->data_regs[i].reg,
+				 GENMASK(info->data_regs[i].reg_width - 1, 0));
 		sh_pfc_check_reg_enums(drvname, info->data_regs[i].reg,
 				       info->data_regs[i].enum_ids,
 				       info->data_regs[i].reg_width);
-- 
2.25.1

[PATCH 4/5] pinctrl: renesas: checker: Fix bias checks on SoCs with pull-down only pins

[Geert Uytterhoeven]

If some bits in a pin Pull-Up control register (PUPR) control pin
pull-down instead of pin pull-up, there are two pinmux_bias_reg entries:
a first one with the puen field filled in, listing pins with pull-up
functionality, and a second one with the pud field filled in, listing
pins with pull-down functionality.  On encountering the second entry,
where puen is NULL, the for-loop terminates early, causing the remaining
bias registers not to be checked.  In addition, sh_pfc_check_bias_reg()
does not handle such entries.

Fix this by treating pinmux_bias_reg.puen and pinmux_bias_reg.pud the
same.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/renesas/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/renesas/core.c b/drivers/pinctrl/renesas/core.c
index ab8bdb81e8df5069..9d791a5a5f957c6e 100644
--- a/drivers/pinctrl/renesas/core.c
+++ b/drivers/pinctrl/renesas/core.c
@@ -919,7 +919,8 @@ static void __init sh_pfc_check_bias_reg(const struct sh_pfc_soc_info *info,
 		if (bias->pins[i] != SH_PFC_PIN_NONE)
 			bits |= BIT(i);
 
-	sh_pfc_check_reg(info->name, bias->puen, bits);
+	if (bias->puen)
+		sh_pfc_check_reg(info->name, bias->puen, bits);
 	if (bias->pud)
 		sh_pfc_check_reg(info->name, bias->pud, bits);
 	for (i = 0; i < ARRAY_SIZE(bias->pins); i++)
@@ -928,6 +929,7 @@ static void __init sh_pfc_check_bias_reg(const struct sh_pfc_soc_info *info,
 
 static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info)
 {
+	const struct pinmux_bias_reg *bias_regs = info->bias_regs;
 	const char *drvname = info->name;
 	unsigned int *refcnts;
 	unsigned int i, j, k;
@@ -1024,8 +1026,8 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info)
 		sh_pfc_check_drive_reg(info, &info->drive_regs[i]);
 
 	/* Check bias registers */
-	for (i = 0; info->bias_regs && info->bias_regs[i].puen; i++)
-		sh_pfc_check_bias_reg(info, &info->bias_regs[i]);
+	for (i = 0; bias_regs && (bias_regs[i].puen || bias_regs[i].pud); i++)
+		sh_pfc_check_bias_reg(info, &bias_regs[i]);
 
 	/* Check ioctrl registers */
 	for (i = 0; info->ioctrl_regs && info->ioctrl_regs[i].reg; i++)
-- 
2.25.1

[PATCH 5/5] pinctrl: renesas: checker: Prefix common checker output

[Geert Uytterhoeven]

Add a "sh_pfc: " prefix to common checker output that is not yet
prefixed by a subdriver-specific prefix ("<SoC-part-number>_pfc: "), for
easier grepping.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/renesas/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/renesas/core.c b/drivers/pinctrl/renesas/core.c
index 9d791a5a5f957c6e..0d4ea2e22a53599f 100644
--- a/drivers/pinctrl/renesas/core.c
+++ b/drivers/pinctrl/renesas/core.c
@@ -934,7 +934,7 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info)
 	unsigned int *refcnts;
 	unsigned int i, j, k;
 
-	pr_info("Checking %s\n", drvname);
+	pr_info("sh_pfc: Checking %s\n", drvname);
 	sh_pfc_num_regs = 0;
 	sh_pfc_num_enums = 0;
 
@@ -1081,7 +1081,7 @@ static void __init sh_pfc_check_driver(const struct platform_driver *pdrv)
 	if (!sh_pfc_enums)
 		goto free_regs;
 
-	pr_warn("Checking builtin pinmux tables\n");
+	pr_warn("sh_pfc: Checking builtin pinmux tables\n");
 
 	for (i = 0; pdrv->id_table[i].name[0]; i++)
 		sh_pfc_check_info((void *)pdrv->id_table[i].driver_data);
@@ -1091,7 +1091,7 @@ static void __init sh_pfc_check_driver(const struct platform_driver *pdrv)
 		sh_pfc_check_info(pdrv->driver.of_match_table[i].data);
 #endif
 
-	pr_warn("Detected %u errors and %u warnings\n", sh_pfc_errors,
+	pr_warn("sh_pfc: Detected %u errors and %u warnings\n", sh_pfc_errors,
 		sh_pfc_warnings);
 
 	kfree(sh_pfc_enums);
-- 
2.25.1