[v2,02/10] pinctrl: sh-pfc: Validate pinmux tables at runtime when debugging
diff mbox series

Message ID 20190125165305.8567-3-geert+renesas@glider.be
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series
  • pinctrl: sh-pfc: Validation and compile-testing
Related show

Commit Message

Geert Uytterhoeven Jan. 25, 2019, 4:52 p.m. UTC
Perform some basic sanity checks on all built-in pinmux tables when
DEBUG is defined, to help catching bugs early.

For now the following checks are included:
  - Check register and field widths in descriptors for config registers
    with variable-width fields,
  - Check relations between pin groups and functions:
      - All pin functions must refer to existing pin groups,
      - All pin groups must be referred to by a pin function,
      - Warn if a pin group is referred to by multiple pin functions
	(which is OK for backwards-compatibility aliases),
  - Provide suggestions for reducing table sizes: reserved fields of
    more than 3 bits can better be split in smaller subfields, as the
    storage need is proportional to the square of the width of the
    (sub)field,

Note that a dummy non-matching entry is added to the DT match table for
checking r8a7795es1_pinmux_info, as R-Car H3 ES1.0 is matched using
soc_device_match() in r8a7795_pinmux_init(), instead of by the DT match
table.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Drop RFC state,
  - Drop validation of fixed-with config register fields, as this is now
    done at build time,
  - Check relations between pin groups and functions,
  - Move compile-test support out,
  - Move checks depending on enum ID absorption out,
  - Move call to sh_pfc_check_driver() from sh_pfc_probe() to
    sh_pfc_init(), so the checks are even performed on non-native
    platforms when compile-testing.
---
 drivers/pinctrl/sh-pfc/core.c | 123 ++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

Comments

Simon Horman Feb. 15, 2019, 8:17 a.m. UTC | #1
On Fri, Jan 25, 2019 at 05:52:57PM +0100, Geert Uytterhoeven wrote:
> Perform some basic sanity checks on all built-in pinmux tables when
> DEBUG is defined, to help catching bugs early.
> 
> For now the following checks are included:
>   - Check register and field widths in descriptors for config registers
>     with variable-width fields,
>   - Check relations between pin groups and functions:
>       - All pin functions must refer to existing pin groups,
>       - All pin groups must be referred to by a pin function,
>       - Warn if a pin group is referred to by multiple pin functions
> 	(which is OK for backwards-compatibility aliases),
>   - Provide suggestions for reducing table sizes: reserved fields of
>     more than 3 bits can better be split in smaller subfields, as the
>     storage need is proportional to the square of the width of the
>     (sub)field,
> 
> Note that a dummy non-matching entry is added to the DT match table for
> checking r8a7795es1_pinmux_info, as R-Car H3 ES1.0 is matched using
> soc_device_match() in r8a7795_pinmux_init(), instead of by the DT match
> table.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Hi Geert,

I noted a few nits below, but in writing them I got to feel
that perhaps its just me not appreciating the merit of
some of the idioms used. So feel free to ignore them.

Nits aside,

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
> v2:
>   - Drop RFC state,
>   - Drop validation of fixed-with config register fields, as this is now
>     done at build time,
>   - Check relations between pin groups and functions,
>   - Move compile-test support out,
>   - Move checks depending on enum ID absorption out,
>   - Move call to sh_pfc_check_driver() from sh_pfc_probe() to
>     sh_pfc_init(), so the checks are even performed on non-native
>     platforms when compile-testing.
> ---
>  drivers/pinctrl/sh-pfc/core.c | 123 ++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index f1cfcc8c65446662..f9371e160872f2b4 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -571,6 +571,13 @@ static const struct of_device_id sh_pfc_of_table[] = {
>  		.compatible = "renesas,pfc-r8a7795",
>  		.data = &r8a7795_pinmux_info,
>  	},
> +#ifdef DEBUG
> +	{
> +		/* For sanity checks only (nothing matches against this) */
> +		.compatible = "renesas,pfc-r8a77950",	/* R-Car H3 ES1.0 */
> +		.data = &r8a7795es1_pinmux_info,
> +	},
> +#endif /* DEBUG */
>  #endif
>  #ifdef CONFIG_PINCTRL_PFC_R8A7796
>  	{
> @@ -709,6 +716,121 @@ static int sh_pfc_suspend_init(struct sh_pfc *pfc) { return 0; }
>  #define DEV_PM_OPS	NULL
>  #endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
>  
> +#ifdef DEBUG
> +static bool is0s(const u16 *enum_ids, unsigned int n)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < n; i++)
> +		if (enum_ids[i])
> +			return false;
> +
> +	return true;
> +}
> +
> +static unsigned int sh_pfc_errors;
> +static unsigned int sh_pfc_warnings;
> +
> +static void sh_pfc_check_cfg_reg(const char *drvname,
> +				 const struct pinmux_cfg_reg *cfg_reg)
> +{
> +	unsigned int i, n, rw, fw;
> +
> +	if (cfg_reg->field_width) {
> +		/* Checked at build time */
> +		return;
> +	}
> +
> +	for (i = 0, n = 0, rw = 0; (fw = cfg_reg->var_field_width[i]); i++) {

nit: The assignment in the conditional expression in the for loop is
     perhaps not the clearest way to express this, and its
     not clear that the parentheses are required.

> +		if (fw > 3 && is0s(&cfg_reg->enum_ids[n], 1 << fw)) {
> +			pr_warn("%s: reg 0x%x: reserved field [%u:%u] can be split to reduce table size\n",
> +				drvname, cfg_reg->reg, rw, rw + fw - 1);
> +			sh_pfc_warnings++;
> +		}
> +		n += 1 << fw;
> +		rw += fw;
> +	}
> +
> +	if (rw != cfg_reg->reg_width) {
> +		pr_err("%s: reg 0x%x: var_field_width declares %u instead of %u bits\n",
> +		       drvname, cfg_reg->reg, rw, cfg_reg->reg_width);
> +		sh_pfc_errors++;
> +	}
> +}
> +
> +static void sh_pfc_check_info(const struct sh_pfc_soc_info *info)
> +{
> +	const struct sh_pfc_function *func;
> +	const char *drvname = info->name;
> +	unsigned int *refcnts;
> +	unsigned int i, j, k;
> +
> +	pr_info("Checking %s\n", drvname);
> +
> +	/* Check groups and functions */
> +	refcnts = kcalloc(info->nr_groups, sizeof(*refcnts), GFP_KERNEL);
> +	if (!refcnts)
> +		return;
> +
> +	for (i = 0; func = &info->functions[i], i < info->nr_functions; i++) {

nit: It might be clearer to set (and declare) func inside the loop.

> +		for (j = 0; j < func->nr_groups; j++) {
> +			for (k = 0; k < info->nr_groups; k++) {
> +				if (!strcmp(func->groups[j],
> +					    info->groups[k].name)) {
> +					refcnts[k]++;
> +					goto next;

nit: goto

> +				}
> +			}
> +
> +			pr_err("%s: function %s: group %s not found\n",
> +			       drvname, func->name, func->groups[j]);
> +			sh_pfc_errors++;
> +
> +next:			;
> +		}
> +	}
> +
> +	for (i = 0; i < info->nr_groups; i++) {
> +		if (!refcnts[i]) {
> +			pr_err("%s: orphan group %s\n", drvname,
> +			       info->groups[i].name);
> +			sh_pfc_errors++;
> +		} else if (refcnts[i] > 1) {
> +			pr_err("%s: group %s referred by %u functions\n",
> +			       drvname, info->groups[i].name, refcnts[i]);
> +			sh_pfc_warnings++;
> +		}
> +	}
> +
> +	kfree(refcnts);
> +
> +	/* Check config register descriptions */
> +	for (i = 0; info->cfg_regs && info->cfg_regs[i].reg; i++)
> +		sh_pfc_check_cfg_reg(drvname, &info->cfg_regs[i]);
> +}
> +
> +static void sh_pfc_check_driver(const struct platform_driver *pdrv)
> +{
> +	unsigned int i;
> +
> +	pr_warn("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);
> +
> +#ifdef CONFIG_OF
> +	for (i = 0; pdrv->driver.of_match_table[i].compatible[0]; i++)
> +		sh_pfc_check_info(pdrv->driver.of_match_table[i].data);
> +#endif
> +
> +	pr_warn("Detected %u errors and %u warnings\n", sh_pfc_errors,
> +		sh_pfc_warnings);
> +}
> +
> +#else /* !DEBUG */
> +static inline void sh_pfc_check_driver(struct platform_driver *pdrv) {}
> +#endif /* !DEBUG */
> +
>  static int sh_pfc_probe(struct platform_device *pdev)
>  {
>  #ifdef CONFIG_OF
> @@ -840,6 +962,7 @@ static struct platform_driver sh_pfc_driver = {
>  
>  static int __init sh_pfc_init(void)
>  {
> +	sh_pfc_check_driver(&sh_pfc_driver);
>  	return platform_driver_register(&sh_pfc_driver);
>  }
>  postcore_initcall(sh_pfc_init);
> -- 
> 2.17.1
>
Geert Uytterhoeven Feb. 15, 2019, 8:43 a.m. UTC | #2
Hi Simon,

On Fri, Feb 15, 2019 at 9:17 AM Simon Horman <horms@verge.net.au> wrote:
> On Fri, Jan 25, 2019 at 05:52:57PM +0100, Geert Uytterhoeven wrote:
> > Perform some basic sanity checks on all built-in pinmux tables when
> > DEBUG is defined, to help catching bugs early.
> >
> > For now the following checks are included:
> >   - Check register and field widths in descriptors for config registers
> >     with variable-width fields,
> >   - Check relations between pin groups and functions:
> >       - All pin functions must refer to existing pin groups,
> >       - All pin groups must be referred to by a pin function,
> >       - Warn if a pin group is referred to by multiple pin functions
> >       (which is OK for backwards-compatibility aliases),
> >   - Provide suggestions for reducing table sizes: reserved fields of
> >     more than 3 bits can better be split in smaller subfields, as the
> >     storage need is proportional to the square of the width of the
> >     (sub)field,
> >
> > Note that a dummy non-matching entry is added to the DT match table for
> > checking r8a7795es1_pinmux_info, as R-Car H3 ES1.0 is matched using
> > soc_device_match() in r8a7795_pinmux_init(), instead of by the DT match
> > table.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Hi Geert,
>
> I noted a few nits below, but in writing them I got to feel
> that perhaps its just me not appreciating the merit of
> some of the idioms used. So feel free to ignore them.
>
> Nits aside,
>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Thank you!

> > --- a/drivers/pinctrl/sh-pfc/core.c
> > +++ b/drivers/pinctrl/sh-pfc/core.c

> > +static void sh_pfc_check_cfg_reg(const char *drvname,
> > +                              const struct pinmux_cfg_reg *cfg_reg)
> > +{
> > +     unsigned int i, n, rw, fw;
> > +
> > +     if (cfg_reg->field_width) {
> > +             /* Checked at build time */
> > +             return;
> > +     }
> > +
> > +     for (i = 0, n = 0, rw = 0; (fw = cfg_reg->var_field_width[i]); i++) {
>
> nit: The assignment in the conditional expression in the for loop is
>      perhaps not the clearest way to express this, and its

The alternative is:

        for (i = 0, n = 0, rw = 0; cfg_reg->var_field_width[i]; i++) {
                fw = cfg_reg->var_field_width[i];

Or a while (1) { ... } with a break.
IMHO all more ugly and/or less concise than the above.

>      not clear that the parentheses are required.

Oh yes, just like in the conditions of "if" and "while" statements:

    warning: suggest parentheses around assignment used as truth value

>
> > +             if (fw > 3 && is0s(&cfg_reg->enum_ids[n], 1 << fw)) {
> > +                     pr_warn("%s: reg 0x%x: reserved field [%u:%u] can be split to reduce table size\n",
> > +                             drvname, cfg_reg->reg, rw, rw + fw - 1);
> > +                     sh_pfc_warnings++;
> > +             }
> > +             n += 1 << fw;
> > +             rw += fw;
> > +     }

> > +static void sh_pfc_check_info(const struct sh_pfc_soc_info *info)
> > +{
> > +     const struct sh_pfc_function *func;
> > +     const char *drvname = info->name;
> > +     unsigned int *refcnts;
> > +     unsigned int i, j, k;
> > +
> > +     pr_info("Checking %s\n", drvname);
> > +
> > +     /* Check groups and functions */
> > +     refcnts = kcalloc(info->nr_groups, sizeof(*refcnts), GFP_KERNEL);
> > +     if (!refcnts)
> > +             return;
> > +
> > +     for (i = 0; func = &info->functions[i], i < info->nr_functions; i++) {
>
> nit: It might be clearer to set (and declare) func inside the loop.

I agree (probably it made sense at some point during development).

> > +             for (j = 0; j < func->nr_groups; j++) {
> > +                     for (k = 0; k < info->nr_groups; k++) {
> > +                             if (!strcmp(func->groups[j],
> > +                                         info->groups[k].name)) {
> > +                                     refcnts[k]++;
> > +                                     goto next;
>
> nit: goto

Yeah, it's a pity C, unlike PERL, doesn't have "next <LABEL>", to escape
out of one or more loop constructs ;-)

The alternative is "break"...

>
> > +                             }
> > +                     }

... and wrap the below in "if (k == info->nr_groups) { ... }", which makes
sense, as it still doesn't make indentation too deep (probably it did at
some point during development).

> > +
> > +                     pr_err("%s: function %s: group %s not found\n",
> > +                            drvname, func->name, func->groups[j]);
> > +                     sh_pfc_errors++;
> > +
> > +next:                        ;
> > +             }

Thanks!

Gr{oetje,eeting}s,

                        Geert

Patch
diff mbox series

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index f1cfcc8c65446662..f9371e160872f2b4 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -571,6 +571,13 @@  static const struct of_device_id sh_pfc_of_table[] = {
 		.compatible = "renesas,pfc-r8a7795",
 		.data = &r8a7795_pinmux_info,
 	},
+#ifdef DEBUG
+	{
+		/* For sanity checks only (nothing matches against this) */
+		.compatible = "renesas,pfc-r8a77950",	/* R-Car H3 ES1.0 */
+		.data = &r8a7795es1_pinmux_info,
+	},
+#endif /* DEBUG */
 #endif
 #ifdef CONFIG_PINCTRL_PFC_R8A7796
 	{
@@ -709,6 +716,121 @@  static int sh_pfc_suspend_init(struct sh_pfc *pfc) { return 0; }
 #define DEV_PM_OPS	NULL
 #endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
 
+#ifdef DEBUG
+static bool is0s(const u16 *enum_ids, unsigned int n)
+{
+	unsigned int i;
+
+	for (i = 0; i < n; i++)
+		if (enum_ids[i])
+			return false;
+
+	return true;
+}
+
+static unsigned int sh_pfc_errors;
+static unsigned int sh_pfc_warnings;
+
+static void sh_pfc_check_cfg_reg(const char *drvname,
+				 const struct pinmux_cfg_reg *cfg_reg)
+{
+	unsigned int i, n, rw, fw;
+
+	if (cfg_reg->field_width) {
+		/* Checked at build time */
+		return;
+	}
+
+	for (i = 0, n = 0, rw = 0; (fw = cfg_reg->var_field_width[i]); i++) {
+		if (fw > 3 && is0s(&cfg_reg->enum_ids[n], 1 << fw)) {
+			pr_warn("%s: reg 0x%x: reserved field [%u:%u] can be split to reduce table size\n",
+				drvname, cfg_reg->reg, rw, rw + fw - 1);
+			sh_pfc_warnings++;
+		}
+		n += 1 << fw;
+		rw += fw;
+	}
+
+	if (rw != cfg_reg->reg_width) {
+		pr_err("%s: reg 0x%x: var_field_width declares %u instead of %u bits\n",
+		       drvname, cfg_reg->reg, rw, cfg_reg->reg_width);
+		sh_pfc_errors++;
+	}
+}
+
+static void sh_pfc_check_info(const struct sh_pfc_soc_info *info)
+{
+	const struct sh_pfc_function *func;
+	const char *drvname = info->name;
+	unsigned int *refcnts;
+	unsigned int i, j, k;
+
+	pr_info("Checking %s\n", drvname);
+
+	/* Check groups and functions */
+	refcnts = kcalloc(info->nr_groups, sizeof(*refcnts), GFP_KERNEL);
+	if (!refcnts)
+		return;
+
+	for (i = 0; func = &info->functions[i], i < info->nr_functions; i++) {
+		for (j = 0; j < func->nr_groups; j++) {
+			for (k = 0; k < info->nr_groups; k++) {
+				if (!strcmp(func->groups[j],
+					    info->groups[k].name)) {
+					refcnts[k]++;
+					goto next;
+				}
+			}
+
+			pr_err("%s: function %s: group %s not found\n",
+			       drvname, func->name, func->groups[j]);
+			sh_pfc_errors++;
+
+next:			;
+		}
+	}
+
+	for (i = 0; i < info->nr_groups; i++) {
+		if (!refcnts[i]) {
+			pr_err("%s: orphan group %s\n", drvname,
+			       info->groups[i].name);
+			sh_pfc_errors++;
+		} else if (refcnts[i] > 1) {
+			pr_err("%s: group %s referred by %u functions\n",
+			       drvname, info->groups[i].name, refcnts[i]);
+			sh_pfc_warnings++;
+		}
+	}
+
+	kfree(refcnts);
+
+	/* Check config register descriptions */
+	for (i = 0; info->cfg_regs && info->cfg_regs[i].reg; i++)
+		sh_pfc_check_cfg_reg(drvname, &info->cfg_regs[i]);
+}
+
+static void sh_pfc_check_driver(const struct platform_driver *pdrv)
+{
+	unsigned int i;
+
+	pr_warn("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);
+
+#ifdef CONFIG_OF
+	for (i = 0; pdrv->driver.of_match_table[i].compatible[0]; i++)
+		sh_pfc_check_info(pdrv->driver.of_match_table[i].data);
+#endif
+
+	pr_warn("Detected %u errors and %u warnings\n", sh_pfc_errors,
+		sh_pfc_warnings);
+}
+
+#else /* !DEBUG */
+static inline void sh_pfc_check_driver(struct platform_driver *pdrv) {}
+#endif /* !DEBUG */
+
 static int sh_pfc_probe(struct platform_device *pdev)
 {
 #ifdef CONFIG_OF
@@ -840,6 +962,7 @@  static struct platform_driver sh_pfc_driver = {
 
 static int __init sh_pfc_init(void)
 {
+	sh_pfc_check_driver(&sh_pfc_driver);
 	return platform_driver_register(&sh_pfc_driver);
 }
 postcore_initcall(sh_pfc_init);