[PATCH/RFC 0/1] soc: renesas: Add DT fixup code for backwards compatibility

[PATCH/RFC 0/1] soc: renesas: Add DT fixup code for backwards compatibility

[Geert Uytterhoeven]

	Hi all,

When moving functionality from C code to DT, we're regularly faced with
stable DT issues: old DTBs should keep on working. This requires keeping
workaround code in the kernel.

An alternative solution to having workaround C code, would be to
dynamically modify the DT, to add missing device nodes and phandle links.
This has several advantages:
  - All workarounds are kept together,
  - Workarounds can be enabled/disabled using a single Kconfig option,
  - Individual driver code is not polluted by workaround code.

Examples of missing support in DT are:
  - A device node for the R-Car RST (Reset Controller), which a.o.
    provides access to the Mode Pins (currently handled using an
    hardcoded address in platform/driver code), cfr. the series
    "[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for
    obtaining mode pin state" I've just sent
    (http://www.spinics.net/lists/linux-renesas-soc/msg04289.html),
  - A device node for the R-Car SYSC (System Controller), to link CPUs
    to their respective power domains (On R-Car Gen2 CPUs can be
    auto-detected, as there's a register indicating which CPU cores are
    present),
  - Add a device node for the R-Car Gen2 APMU (Advanced Power
    Management Unit), for modern CPU bringup using "enable-method".
    Note that the method from this RFC doesn't work for
    "enable-method", as that is parsed in arm_dt_init_cpu_maps(),
    immediately after unflatten_device_tree(), long before initcalls
    run.

However, there are other possible uses:
  - Workarounds for hardware bugs: early engineering samples of an SoC
    may have non-functional devices. This would allow to describe the
    latest (functional) hardware in the .dtsi, knowing that the fixup
    code will disable non-functional devices when running on an early
    engineering sample, based on reading the PRR (Product Revision
    Register).
  - Handle other differences between SoC versions, e.g. change
    compatible values for an early engineering sample that needs special
    handling, or limit the features of a device.
  - Add SoC-specific compatible values to all device nodes (e.g. add
    "renesas,r8a7795-wdt" to a node already having
    "renesas,rcar-gen3-wdt" when running on r8a7795). This would make
    it easier to share .dtsi files within the same SoC family, without
    relying on e.g. C preprocessor tricks.

This proof-of-concept implements this for the missing R-Car RST (Reset
Controller) node. This poc is not suitable for all of the above, as some
DT structures (e.g. the CPU's "enable-method) are parsed long before
early_initcall(), and would need a different workaround.

What do you think?
Should this be handled at another level? E.g. operate on the FDT?

Thanks!

Geert Uytterhoeven (1):
  soc: renesas: Add DT fixup code for missing r8a7791 RST

 drivers/soc/renesas/Makefile           |   4 +
 drivers/soc/renesas/renesas-dt-fixup.c | 159 +++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 drivers/soc/renesas/renesas-dt-fixup.c

-- 
1.9.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/RFC 1/1] soc: renesas: Add DT fixup code for missing r8a7791 RST

[Geert Uytterhoeven]

Add DT fixup code to add a reset-controller node for the r8a7791 RST
module if it's not yet present.

This allows the R-Car Gen2 clock driver to use the RST driver to obtain
the value of the mode pins, instead of reading from a hardcoded address
in rcar_gen2_read_mode_pins(), while providing backward compatibility
with old DTBs.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
---
 drivers/soc/renesas/Makefile           |   4 +
 drivers/soc/renesas/renesas-dt-fixup.c | 159 +++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 drivers/soc/renesas/renesas-dt-fixup.c

diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
index 0d732ff893dd8fba..e5f4454821ca6f63 100644
--- a/drivers/soc/renesas/Makefile
+++ b/drivers/soc/renesas/Makefile
@@ -1,3 +1,7 @@
+# FIXME Depend on new Kconfig symbol CONFIG_ARCH_RENESAS_LEGACY
+# FIXME Needs OF_DYNAMIC
+obj-$(CONFIG_ARCH_RENESAS)	+= renesas-dt-fixup.o
+
 obj-$(CONFIG_ARCH_RCAR_GEN1)	+= rcar-rst.o
 obj-$(CONFIG_ARCH_RCAR_GEN2)	+= rcar-rst.o
 obj-$(CONFIG_ARCH_R8A7795)	+= rcar-rst.o
diff --git a/drivers/soc/renesas/renesas-dt-fixup.c b/drivers/soc/renesas/renesas-dt-fixup.c
new file mode 100644
index 0000000000000000..83c9ca6b44f8136c
--- /dev/null
+++ b/drivers/soc/renesas/renesas-dt-fixup.c
@@ -0,0 +1,159 @@
+/*
+ * Renesas DT Fixup Code
+ *
+ * Provide backwards compatibility with old DTBs by updating the DT
+ *
+ * Copyright (C) 2016 Glider bvba
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+
+#define RST_BASE	0xe6160000
+#define RST_SIZE	0x0100		/* R-Car Gen2 */
+
+static struct property rst_compatible = {
+	.name = "compatible",
+};
+
+static __be32 rst_reg_values[4] = {
+	cpu_to_be32(0),
+	cpu_to_be32(RST_BASE),
+	cpu_to_be32(0),
+	cpu_to_be32(RST_SIZE),
+};
+
+static struct property rst_name = {
+	.name = "name",
+	.length = sizeof("reset-controller"),
+	.value = "reset-controller",
+};
+
+static struct property rst_reg = {
+	.name = "reg",
+	.length = sizeof(rst_reg_values),
+	.value = rst_reg_values,
+};
+
+static int __init add_rst(struct device_node *parent, const char *path,
+			  char *compatible)
+{
+	struct property *prop[] = { &rst_compatible, &rst_name, &rst_reg };
+	struct device_node *np;
+	char *full_name;
+	unsigned int i;
+	int error;
+
+	pr_debug("Adding RST node %s\n", compatible);
+	np = kzalloc(sizeof(*np), GFP_KERNEL);
+	if (!np)
+		return -ENOMEM;
+
+	np->name = kasprintf(GFP_KERNEL, "%s@%x", (char *)rst_name.value,
+			     RST_BASE);
+	full_name = kasprintf(GFP_KERNEL, "%s/%s", path, np->name);
+	np->full_name = full_name;
+	np->parent = parent;
+
+	of_node_set_flag(np, OF_DYNAMIC);
+	of_node_init(np);
+
+	error = of_attach_node(np);
+	if (error) {
+		pr_err("%s: of_attach_node() %s failed %d\n", __func__,
+		       np->name, error);
+		kfree(np);
+		return error;
+	}
+
+	rst_compatible.value = compatible;
+	rst_compatible.length = strlen(rst_compatible.value) + 1;
+
+	for (i = 0; i < ARRAY_SIZE(prop); i++) {
+		error = of_add_property(np, prop[i]);
+		if (error)
+			pr_err("%s: of_add_property() %s/%s failed %d\n",
+			       __func__, np->name, prop[i]->name, error);
+	}
+
+	return 0;
+}
+
+static int __init add_sysc(struct device_node *parent, const char *path,
+			   char *compatible)
+{
+	pr_debug("NOT YET adding SYSC node %s\n", compatible);
+	return 0;
+}
+
+static const struct of_device_id renesas_matches[] __initconst = {
+	{ .compatible = "renesas,r8a7791", },
+	{ /* sentinel */ }
+};
+
+static int __init renesas_dt_fixup(void)
+{
+	const struct of_device_id *match;
+	struct device_node *np, *parent;
+	const char *parent_path;
+	char *compatible;
+
+	np = of_find_node_by_path("/");
+	if (!np)
+		return -ENODEV;
+
+	for (match = renesas_matches; match->compatible[0]; match++)
+		if (of_device_is_compatible(np, match->compatible))
+			break;
+	of_node_put(np);
+
+	if (!match->compatible[0])
+		return -ENODEV;
+
+	/* Find the main SoC bus node */
+	np = of_find_compatible_node(NULL, NULL, "arm,gic-400");
+	if (!np) {
+		pr_warn("%s: Cannot find %s\n", __func__, "arm,gic-400");
+		return -ENODEV;
+	}
+
+	parent = of_get_parent(np);
+	of_node_put(np);
+	if (!parent) {
+		pr_warn("%s: Cannot find GIC parent\n", __func__);
+		return -ENODEV;
+	}
+
+	parent_path = parent->full_name;
+	/* Prevent double leading slash */
+	if (!parent_path[1])
+		parent_path++;
+
+	compatible = kasprintf(GFP_KERNEL, "%s-rst", match->compatible);
+	if (of_find_compatible_node(NULL, NULL, compatible)) {
+		pr_debug("%s: RST node %s present\n", __func__, compatible);
+		kfree(compatible);
+	} else {
+		add_rst(parent, parent_path, compatible);
+	}
+
+	compatible = kasprintf(GFP_KERNEL, "%s-sysc", match->compatible);
+	if (of_find_compatible_node(NULL, NULL, compatible)) {
+		pr_debug("%s: SYSC node %s present\n", __func__, compatible);
+		kfree(compatible);
+	} else {
+		add_sysc(parent, parent_path, compatible);
+	}
+
+	of_node_put(parent);
+	return 0;
+}
+early_initcall(renesas_dt_fixup);
-- 
1.9.1

Re: [PATCH/RFC 1/1] soc: renesas: Add DT fixup code for missing r8a7791 RST

[Pantelis Antoniou]

Hi Geert,

A few notes.

> On Jun 1, 2016, at 22:50 , Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> Add DT fixup code to add a reset-controller node for the r8a7791 RST
> module if it's not yet present.
> 
> This allows the R-Car Gen2 clock driver to use the RST driver to obtain
> the value of the mode pins, instead of reading from a hardcoded address
> in rcar_gen2_read_mode_pins(), while providing backward compatibility
> with old DTBs.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> ---
> drivers/soc/renesas/Makefile           |   4 +
> drivers/soc/renesas/renesas-dt-fixup.c | 159 +++++++++++++++++++++++++++++++++
> 2 files changed, 163 insertions(+)
> create mode 100644 drivers/soc/renesas/renesas-dt-fixup.c
> 
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
> index 0d732ff893dd8fba..e5f4454821ca6f63 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -1,3 +1,7 @@
> +# FIXME Depend on new Kconfig symbol CONFIG_ARCH_RENESAS_LEGACY
> +# FIXME Needs OF_DYNAMIC
> +obj-$(CONFIG_ARCH_RENESAS)	+= renesas-dt-fixup.o
> +
> obj-$(CONFIG_ARCH_RCAR_GEN1)	+= rcar-rst.o
> obj-$(CONFIG_ARCH_RCAR_GEN2)	+= rcar-rst.o
> obj-$(CONFIG_ARCH_R8A7795)	+= rcar-rst.o
> diff --git a/drivers/soc/renesas/renesas-dt-fixup.c b/drivers/soc/renesas/renesas-dt-fixup.c
> new file mode 100644
> index 0000000000000000..83c9ca6b44f8136c
> --- /dev/null
> +++ b/drivers/soc/renesas/renesas-dt-fixup.c
> @@ -0,0 +1,159 @@
> +/*
> + * Renesas DT Fixup Code
> + *
> + * Provide backwards compatibility with old DTBs by updating the DT
> + *
> + * Copyright (C) 2016 Glider bvba
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#define RST_BASE	0xe6160000
> +#define RST_SIZE	0x0100		/* R-Car Gen2 */
> +
> +static struct property rst_compatible = {
> +	.name = "compatible",
> +};
> +
> +static __be32 rst_reg_values[4] = {
> +	cpu_to_be32(0),
> +	cpu_to_be32(RST_BASE),
> +	cpu_to_be32(0),
> +	cpu_to_be32(RST_SIZE),
> +};
> +
> +static struct property rst_name = {
> +	.name = "name",
> +	.length = sizeof("reset-controller"),
> +	.value = "reset-controller",
> +};
> +
> +static struct property rst_reg = {
> +	.name = "reg",
> +	.length = sizeof(rst_reg_values),
> +	.value = rst_reg_values,
> +};
> +
> +static int __init add_rst(struct device_node *parent, const char *path,
> +			  char *compatible)
> +{
> +	struct property *prop[] = { &rst_compatible, &rst_name, &rst_reg };
> +	struct device_node *np;
> +	char *full_name;
> +	unsigned int i;
> +	int error;
> +
> +	pr_debug("Adding RST node %s\n", compatible);
> +	np = kzalloc(sizeof(*np), GFP_KERNEL);
> +	if (!np)
> +		return -ENOMEM;
> +
> +	np->name = kasprintf(GFP_KERNEL, "%s@%x", (char *)rst_name.value,
> +			     RST_BASE);
> +	full_name = kasprintf(GFP_KERNEL, "%s/%s", path, np->name);
> +	np->full_name = full_name;
> +	np->parent = parent;
> +
> +	of_node_set_flag(np, OF_DYNAMIC);
> +	of_node_init(np);
> +
> +	error = of_attach_node(np);
> +	if (error) {
> +		pr_err("%s: of_attach_node() %s failed %d\n", __func__,
> +		       np->name, error);
> +		kfree(np);
> +		return error;
> +	}
> +
> +	rst_compatible.value = compatible;
> +	rst_compatible.length = strlen(rst_compatible.value) + 1;
> +
> +	for (i = 0; i < ARRAY_SIZE(prop); i++) {
> +		error = of_add_property(np, prop[i]);
> +		if (error)
> +			pr_err("%s: of_add_property() %s/%s failed %d\n",
> +			       __func__, np->name, prop[i]->name, error);
> +	}
> +
> +	return 0;
> +}
> +

Using changesets (especially with the new changeset helpers would make this
so much simpler. Or you could compile in a DTBO that performs the changes
although you will have to tweak it since it appears you’re not using fixed
names and properties all the way.

> +static int __init add_sysc(struct device_node *parent, const char *path,
> +			   char *compatible)
> +{
> +	pr_debug("NOT YET adding SYSC node %s\n", compatible);
> +	return 0;
> +}
> +
> +static const struct of_device_id renesas_matches[] __initconst = {
> +	{ .compatible = "renesas,r8a7791", },
> +	{ /* sentinel */ }
> +};
> +
> +static int __init renesas_dt_fixup(void)
> +{
> +	const struct of_device_id *match;
> +	struct device_node *np, *parent;
> +	const char *parent_path;
> +	char *compatible;
> +
> +	np = of_find_node_by_path("/");
> +	if (!np)
> +		return -ENODEV;
> +
> +	for (match = renesas_matches; match->compatible[0]; match++)
> +		if (of_device_is_compatible(np, match->compatible))
> +			break;
> +	of_node_put(np);
> +
> +	if (!match->compatible[0])
> +		return -ENODEV;
> +
> +	/* Find the main SoC bus node */
> +	np = of_find_compatible_node(NULL, NULL, "arm,gic-400");
> +	if (!np) {
> +		pr_warn("%s: Cannot find %s\n", __func__, "arm,gic-400");
> +		return -ENODEV;
> +	}
> +
> +	parent = of_get_parent(np);
> +	of_node_put(np);
> +	if (!parent) {
> +		pr_warn("%s: Cannot find GIC parent\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	parent_path = parent->full_name;
> +	/* Prevent double leading slash */
> +	if (!parent_path[1])
> +		parent_path++;
> +
> +	compatible = kasprintf(GFP_KERNEL, "%s-rst", match->compatible);
> +	if (of_find_compatible_node(NULL, NULL, compatible)) {
> +		pr_debug("%s: RST node %s present\n", __func__, compatible);
> +		kfree(compatible);
> +	} else {
> +		add_rst(parent, parent_path, compatible);
> +	}
> +
> +	compatible = kasprintf(GFP_KERNEL, "%s-sysc", match->compatible);
> +	if (of_find_compatible_node(NULL, NULL, compatible)) {
> +		pr_debug("%s: SYSC node %s present\n", __func__, compatible);
> +		kfree(compatible);
> +	} else {
> +		add_sysc(parent, parent_path, compatible);
> +	}
> +
> +	of_node_put(parent);
> +	return 0;
> +}
> +early_initcall(renesas_dt_fixup);
> -- 
> 1.9.1
> 

Regards

— Pantelis

Re: [PATCH/RFC 0/1] soc: renesas: Add DT fixup code for backwards compatibility

[Rob Herring]

On Wed, Jun 1, 2016 at 2:50 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>         Hi all,
>
> When moving functionality from C code to DT, we're regularly faced with
> stable DT issues: old DTBs should keep on working. This requires keeping
> workaround code in the kernel.
>
> An alternative solution to having workaround C code, would be to
> dynamically modify the DT, to add missing device nodes and phandle links.
> This has several advantages:
>   - All workarounds are kept together,
>   - Workarounds can be enabled/disabled using a single Kconfig option,
>   - Individual driver code is not polluted by workaround code.
>
> Examples of missing support in DT are:
>   - A device node for the R-Car RST (Reset Controller), which a.o.
>     provides access to the Mode Pins (currently handled using an
>     hardcoded address in platform/driver code), cfr. the series
>     "[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for
>     obtaining mode pin state" I've just sent
>     (http://www.spinics.net/lists/linux-renesas-soc/msg04289.html),
>   - A device node for the R-Car SYSC (System Controller), to link CPUs
>     to their respective power domains (On R-Car Gen2 CPUs can be
>     auto-detected, as there's a register indicating which CPU cores are
>     present),
>   - Add a device node for the R-Car Gen2 APMU (Advanced Power
>     Management Unit), for modern CPU bringup using "enable-method".
>     Note that the method from this RFC doesn't work for
>     "enable-method", as that is parsed in arm_dt_init_cpu_maps(),
>     immediately after unflatten_device_tree(), long before initcalls
>     run.
>
> However, there are other possible uses:
>   - Workarounds for hardware bugs: early engineering samples of an SoC
>     may have non-functional devices. This would allow to describe the
>     latest (functional) hardware in the .dtsi, knowing that the fixup
>     code will disable non-functional devices when running on an early
>     engineering sample, based on reading the PRR (Product Revision
>     Register).
>   - Handle other differences between SoC versions, e.g. change
>     compatible values for an early engineering sample that needs special
>     handling, or limit the features of a device.
>   - Add SoC-specific compatible values to all device nodes (e.g. add
>     "renesas,r8a7795-wdt" to a node already having
>     "renesas,rcar-gen3-wdt" when running on r8a7795). This would make
>     it easier to share .dtsi files within the same SoC family, without
>     relying on e.g. C preprocessor tricks.
>
> This proof-of-concept implements this for the missing R-Car RST (Reset
> Controller) node. This poc is not suitable for all of the above, as some
> DT structures (e.g. the CPU's "enable-method) are parsed long before
> early_initcall(), and would need a different workaround.
>
> What do you think?

I have no objection to this method of dealing with compatibility.
However your handling is still C code. What I would like to see here
is using overlays to apply updates. I would like to be able to take 2
dts files and create an overlay dts based on their diff (or you could
do this step manually). Then build the overlay dtb into the kernel and
apply it on boot based on some match. Then thru the magic of linker
sections, it becomes a matter of just adding the dtbo into the build
and a one line declaration:

DT_QUIRK(my_quirk_dtbo, "vendor,board");

BTW, I'd also like to see tools to apply overlays offline into a new
dtb or compile dts files and overlays to a dtb.

> Should this be handled at another level? E.g. operate on the FDT?

We should try to avoid doing things with the FDT if possible.

Rob

Re: [PATCH/RFC 0/1] soc: renesas: Add DT fixup code for backwards compatibility

[Rob Herring]

On Wed, Jun 1, 2016 at 2:50 PM, Geert Uytterhoeven
<geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org> wrote:
>         Hi all,
>
> When moving functionality from C code to DT, we're regularly faced with
> stable DT issues: old DTBs should keep on working. This requires keeping
> workaround code in the kernel.
>
> An alternative solution to having workaround C code, would be to
> dynamically modify the DT, to add missing device nodes and phandle links.
> This has several advantages:
>   - All workarounds are kept together,
>   - Workarounds can be enabled/disabled using a single Kconfig option,
>   - Individual driver code is not polluted by workaround code.
>
> Examples of missing support in DT are:
>   - A device node for the R-Car RST (Reset Controller), which a.o.
>     provides access to the Mode Pins (currently handled using an
>     hardcoded address in platform/driver code), cfr. the series
>     "[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for
>     obtaining mode pin state" I've just sent
>     (http://www.spinics.net/lists/linux-renesas-soc/msg04289.html),
>   - A device node for the R-Car SYSC (System Controller), to link CPUs
>     to their respective power domains (On R-Car Gen2 CPUs can be
>     auto-detected, as there's a register indicating which CPU cores are
>     present),
>   - Add a device node for the R-Car Gen2 APMU (Advanced Power
>     Management Unit), for modern CPU bringup using "enable-method".
>     Note that the method from this RFC doesn't work for
>     "enable-method", as that is parsed in arm_dt_init_cpu_maps(),
>     immediately after unflatten_device_tree(), long before initcalls
>     run.
>
> However, there are other possible uses:
>   - Workarounds for hardware bugs: early engineering samples of an SoC
>     may have non-functional devices. This would allow to describe the
>     latest (functional) hardware in the .dtsi, knowing that the fixup
>     code will disable non-functional devices when running on an early
>     engineering sample, based on reading the PRR (Product Revision
>     Register).
>   - Handle other differences between SoC versions, e.g. change
>     compatible values for an early engineering sample that needs special
>     handling, or limit the features of a device.
>   - Add SoC-specific compatible values to all device nodes (e.g. add
>     "renesas,r8a7795-wdt" to a node already having
>     "renesas,rcar-gen3-wdt" when running on r8a7795). This would make
>     it easier to share .dtsi files within the same SoC family, without
>     relying on e.g. C preprocessor tricks.
>
> This proof-of-concept implements this for the missing R-Car RST (Reset
> Controller) node. This poc is not suitable for all of the above, as some
> DT structures (e.g. the CPU's "enable-method) are parsed long before
> early_initcall(), and would need a different workaround.
>
> What do you think?

I have no objection to this method of dealing with compatibility.
However your handling is still C code. What I would like to see here
is using overlays to apply updates. I would like to be able to take 2
dts files and create an overlay dts based on their diff (or you could
do this step manually). Then build the overlay dtb into the kernel and
apply it on boot based on some match. Then thru the magic of linker
sections, it becomes a matter of just adding the dtbo into the build
and a one line declaration:

DT_QUIRK(my_quirk_dtbo, "vendor,board");

BTW, I'd also like to see tools to apply overlays offline into a new
dtb or compile dts files and overlays to a dtb.

> Should this be handled at another level? E.g. operate on the FDT?

We should try to avoid doing things with the FDT if possible.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/1] soc: renesas: Add DT fixup code for backwards compatibility

[Laurent Pinchart]

Hi Rob and Geert,

On Wednesday 01 Jun 2016 15:27:59 Rob Herring wrote:
> On Wed, Jun 1, 2016 at 2:50 PM, Geert Uytterhoeven wrote:
> > Hi all,
> > 
> > When moving functionality from C code to DT, we're regularly faced with
> > stable DT issues: old DTBs should keep on working. This requires keeping
> > workaround code in the kernel.
> > 
> > An alternative solution to having workaround C code, would be to
> > dynamically modify the DT, to add missing device nodes and phandle links.
> > 
> > This has several advantages:
> >   - All workarounds are kept together,
> >   - Workarounds can be enabled/disabled using a single Kconfig option,
> >   - Individual driver code is not polluted by workaround code.
> > 
> > Examples of missing support in DT are:
> >   - A device node for the R-Car RST (Reset Controller), which a.o.
> >     provides access to the Mode Pins (currently handled using an
> >     hardcoded address in platform/driver code), cfr. the series
> >     "[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for
> >     obtaining mode pin state" I've just sent
> >     (http://www.spinics.net/lists/linux-renesas-soc/msg04289.html),
> >   
> >   - A device node for the R-Car SYSC (System Controller), to link CPUs
> >     to their respective power domains (On R-Car Gen2 CPUs can be
> >     auto-detected, as there's a register indicating which CPU cores are
> >     present),
> >   
> >   - Add a device node for the R-Car Gen2 APMU (Advanced Power
> >   
> >     Management Unit), for modern CPU bringup using "enable-method".
> >     Note that the method from this RFC doesn't work for
> >     "enable-method", as that is parsed in arm_dt_init_cpu_maps(),
> >     immediately after unflatten_device_tree(), long before initcalls
> >     run.
> > 
> > However, there are other possible uses:
> >   - Workarounds for hardware bugs: early engineering samples of an SoC
> >   
> >     may have non-functional devices. This would allow to describe the
> >     latest (functional) hardware in the .dtsi, knowing that the fixup
> >     code will disable non-functional devices when running on an early
> >     engineering sample, based on reading the PRR (Product Revision
> >     Register).
> >   
> >   - Handle other differences between SoC versions, e.g. change
> >   
> >     compatible values for an early engineering sample that needs special
> >     handling, or limit the features of a device.
> >   
> >   - Add SoC-specific compatible values to all device nodes (e.g. add
> >   
> >     "renesas,r8a7795-wdt" to a node already having
> >     "renesas,rcar-gen3-wdt" when running on r8a7795). This would make
> >     it easier to share .dtsi files within the same SoC family, without
> >     relying on e.g. C preprocessor tricks.
> > 
> > This proof-of-concept implements this for the missing R-Car RST (Reset
> > Controller) node. This poc is not suitable for all of the above, as some
> > DT structures (e.g. the CPU's "enable-method) are parsed long before
> > early_initcall(), and would need a different workaround.
> > 
> > What do you think?
> 
> I have no objection to this method of dealing with compatibility.
> However your handling is still C code. What I would like to see here
> is using overlays to apply updates. I would like to be able to take 2
> dts files and create an overlay dts based on their diff (or you could
> do this step manually). Then build the overlay dtb into the kernel and
> apply it on boot based on some match. Then thru the magic of linker
> sections, it becomes a matter of just adding the dtbo into the build
> and a one line declaration:
> 
> DT_QUIRK(my_quirk_dtbo, "vendor,board");
> 
> BTW, I'd also like to see tools to apply overlays offline into a new
> dtb or compile dts files and overlays to a dtb.

We need to keep the use case in mind. The main (and possibly only) reason why 
we want to patch DT this way is to support systems whose DTB can't be updated 
(otherwise we could just update the DTB) and isn't fully known in advance to 
the kernel (otherwise we would just bundle an updated full DTB with the 
kernel). We thus need a heuristic-based approach at runtime to identify 
missing or outdated DT pieces and patch them, with some level of fuzziness. 
I'm not sure we could handle this with overlays.

> > Should this be handled at another level? E.g. operate on the FDT?
> 
> We should try to avoid doing things with the FDT if possible.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/1] soc: renesas: Add DT fixup code for backwards compatibility

[Geert Uytterhoeven]

On Thu, Jun 2, 2016 at 11:24 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 01 Jun 2016 15:27:59 Rob Herring wrote:
>> On Wed, Jun 1, 2016 at 2:50 PM, Geert Uytterhoeven wrote:
>> > When moving functionality from C code to DT, we're regularly faced with
>> > stable DT issues: old DTBs should keep on working. This requires keeping
>> > workaround code in the kernel.
>> >
>> > An alternative solution to having workaround C code, would be to
>> > dynamically modify the DT, to add missing device nodes and phandle links.
>> >
>> > This has several advantages:
>> >   - All workarounds are kept together,
>> >   - Workarounds can be enabled/disabled using a single Kconfig option,
>> >   - Individual driver code is not polluted by workaround code.
>> >
>> > Examples of missing support in DT are:
>> >   - A device node for the R-Car RST (Reset Controller), which a.o.
>> >     provides access to the Mode Pins (currently handled using an
>> >     hardcoded address in platform/driver code), cfr. the series
>> >     "[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for
>> >     obtaining mode pin state" I've just sent
>> >     (http://www.spinics.net/lists/linux-renesas-soc/msg04289.html),
>> >
>> >   - A device node for the R-Car SYSC (System Controller), to link CPUs
>> >     to their respective power domains (On R-Car Gen2 CPUs can be
>> >     auto-detected, as there's a register indicating which CPU cores are
>> >     present),
>> >
>> >   - Add a device node for the R-Car Gen2 APMU (Advanced Power
>> >
>> >     Management Unit), for modern CPU bringup using "enable-method".
>> >     Note that the method from this RFC doesn't work for
>> >     "enable-method", as that is parsed in arm_dt_init_cpu_maps(),
>> >     immediately after unflatten_device_tree(), long before initcalls
>> >     run.
>> >
>> > However, there are other possible uses:
>> >   - Workarounds for hardware bugs: early engineering samples of an SoC
>> >
>> >     may have non-functional devices. This would allow to describe the
>> >     latest (functional) hardware in the .dtsi, knowing that the fixup
>> >     code will disable non-functional devices when running on an early
>> >     engineering sample, based on reading the PRR (Product Revision
>> >     Register).
>> >
>> >   - Handle other differences between SoC versions, e.g. change
>> >
>> >     compatible values for an early engineering sample that needs special
>> >     handling, or limit the features of a device.
>> >
>> >   - Add SoC-specific compatible values to all device nodes (e.g. add
>> >
>> >     "renesas,r8a7795-wdt" to a node already having
>> >     "renesas,rcar-gen3-wdt" when running on r8a7795). This would make
>> >     it easier to share .dtsi files within the same SoC family, without
>> >     relying on e.g. C preprocessor tricks.
>> >
>> > This proof-of-concept implements this for the missing R-Car RST (Reset
>> > Controller) node. This poc is not suitable for all of the above, as some
>> > DT structures (e.g. the CPU's "enable-method) are parsed long before
>> > early_initcall(), and would need a different workaround.
>> >
>> > What do you think?
>>
>> I have no objection to this method of dealing with compatibility.
>> However your handling is still C code. What I would like to see here
>> is using overlays to apply updates. I would like to be able to take 2
>> dts files and create an overlay dts based on their diff (or you could
>> do this step manually). Then build the overlay dtb into the kernel and
>> apply it on boot based on some match. Then thru the magic of linker
>> sections, it becomes a matter of just adding the dtbo into the build
>> and a one line declaration:
>>
>> DT_QUIRK(my_quirk_dtbo, "vendor,board");
>>
>> BTW, I'd also like to see tools to apply overlays offline into a new
>> dtb or compile dts files and overlays to a dtb.
>
> We need to keep the use case in mind. The main (and possibly only) reason why
> we want to patch DT this way is to support systems whose DTB can't be updated
> (otherwise we could just update the DTB) and isn't fully known in advance to
> the kernel (otherwise we would just bundle an updated full DTB with the
> kernel). We thus need a heuristic-based approach at runtime to identify
> missing or outdated DT pieces and patch them, with some level of fuzziness.
> I'm not sure we could handle this with overlays.

Indeed. While I'm a big fan of DT overlays, I don't think they're suitable for
all kinds of fixups we need.
Simple things like adding a device node for the RST could be handled with a
built-in overlay.
More complex things, like adding SYSC and APMU devices nodes need some
extra bit of logic, to e.g. add phandles to/from the (existing) CPU nodes.
The same is true for fixups that need to check on which revision of the SoC
they're running.

>> > Should this be handled at another level? E.g. operate on the FDT?
>>
>> We should try to avoid doing things with the FDT if possible.

OK. So I should try to hook up my code immediately after
unflatten_device_tree()...

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