Re: [PATCH 1/2] drivers: of: implement overlay support

[Sam Ravnborg <sam@ravnborg.org>]

Hi Pascal.

On Tue, Jun 05, 2018 at 05:40:51PM +0000, Pascal Vizeli wrote:
> Origin by Jan Luebbe:
> http://lists.infradead.org/pipermail/barebox/2015-March/022674.html
> 
> I rebase the patches and fix the resolve for __local_fixups__, they
> was wrong copy/ported from linux kernel.
> 
> I merge the overlay code from oftree into own command: of_overlay
> 
> Signed-off-by: Pascal Vizeli <pvizeli@syshack.ch>

You left out the "Signed-off-by: Jan ..."
Is this because you fundamentally changed the patches, then please say so.
Otherwise add it.

This comment from Jans changelog is missing:
This is based on Pantelis Antoniou's overlay support for the kernel. It
has been simplified by leaving out transaction support, locking and
other details required by the Linux DT support.

Is this because the above info is no longer valid?
Otherwise include it.


> ---
>  Documentation/user/devicetree.rst     | 101 ++++++++-
>  arch/sandbox/dts/Makefile             |   5 +-
>  arch/sandbox/dts/sandbox-overlay.dtso |  26 +++
>  arch/sandbox/dts/sandbox.dts          |   6 +
>  commands/Kconfig                      |  10 +
>  commands/Makefile                     |   1 +
>  commands/of_overlay.c                 | 106 +++++++++
>  drivers/of/Kconfig                    |   6 +
>  drivers/of/Makefile                   |   1 +
>  drivers/of/overlay.c                  | 234 +++++++++++++++++++
>  drivers/of/resolver.c                 | 314 ++++++++++++++++++++++++++
>  include/of.h                          |  71 ++++++
>  scripts/Makefile.lib                  |   5 +-
>  13 files changed, 882 insertions(+), 4 deletions(-)
>  create mode 100644 arch/sandbox/dts/sandbox-overlay.dtso
>  create mode 100644 commands/of_overlay.c
>  create mode 100644 drivers/of/overlay.c
>  create mode 100644 drivers/of/resolver.c
> 
> diff --git a/Documentation/user/devicetree.rst b/Documentation/user/devicetree.rst
> index 17934d86e..8b7be4f5b 100644
> --- a/Documentation/user/devicetree.rst
> +++ b/Documentation/user/devicetree.rst
> @@ -21,7 +21,7 @@ The internal devicetree
>  -----------------------
>  
>  The devicetree consulted by barebox plays a special role. It is referred to
> -as the "internal devicetree." The barebox devicetree commands work on this
> +as the "internal devicetree". The barebox devicetree commands work on this
>  devicetree. The devicetree source (DTS) files are kept in sync with the kernel DTS
>  files. As the FDT files are meant to be backward compatible, it should always be possible
>  to start a kernel with the barebox internal devicetree. However, since the barebox
> @@ -83,3 +83,102 @@ you can exchange the internal devicetree during runtime using the
>  
>     oftree -f
>     oftree -l /new/dtb
> +
> +Devicetree overlays
> +-------------------
> +
> +Since version 3.19, the Linux kernel supports applying "devicetree overlays" to
> +its loaded device tree. This can be used to inform the kernel about additional
> +non-discoverable devices after the system has booted, which is useful for modular
> +boards and FPGAs. The details of the overlay format are specified in the Linux
> +`kernel documentation <https://www.kernel.org/doc/Documentation/devicetree/overlay-notes.txt>`_
> +and an updated DTC is required to compile the overlays.
> +
> +The use cases for overlays in barebox are a bit different:
> +
> +* some of the modular devices are needed to boot Linux to userspace, but barebox
> +  can detect which module variant is connected
> +* one of several parallel or LVDS displays (which use timing data from devicetree)
> +  can be connected to the SoC and should be used for boot messages
> +* a generic Linux (distribution) kernel should be booted on a modular
> +  system and support additional hardware on modules
> +
> +barebox supports applying overlays in the internal devicetree was well using the
> +:ref:`command_oftree` command with option ``-o``:
> +
> +.. code-block:: none
> +
> +   $ ./barebox -d arch/sandbox/dts/sandbox.dtb -i arch/sandbox/dts/sandbox-overlay.dtbo
> +   add fd0 backed by file arch/sandbox/dts/sandbox-overlay.dtbo
> +
> +   barebox 2015.02.0 #26 Wed Mar 4 09:41:19 CET 2015
> +   ...
> +   barebox@barebox sandbox:/ of_dump
> +   ...
> +     dummy@0 {
> +       status = "disabled";
> +       linux,phandle = <0x1>;
> +       phandle = <0x1>;
> +     };
> +     dummy@1 {
> +       status = "disabled";
> +       linux,phandle = <0x2>;
> +       phandle = <0x2>;
> +     };
> +     __symbols__ {
> +       dummy0 = "/dummy@0";
> +       dummy1 = "/dummy@1";
> +     };
> +   ...
> +   barebox@barebox sandbox:/ of_dump -f /dev/fd0
> +    {
> +     fragment@0 {
> +       target = <0xdeadbeef>;
> +       __overlay__ {
> +         status = "okay";
> +         child {
> +           compatible = "barebox,dummy";
> +         };
> +       };
> +     };
> +     fragment@1 {
> +       target = <0xdeadbeef>;
> +       __overlay__ {
> +         status = "okay";
> +         child {
> +           compatible = "barebox,dummy";
> +         };
> +       };
> +     };
> +     __fixups__ {
> +       dummy0 = "/fragment@0:target:0";
> +       dummy1 = "/fragment@1:target:0";
> +     };
> +   };
> +   barebox@barebox sandbox:/ of_overlay /dev/fd0
> +   barebox@barebox sandbox:/ of_dump
> +   ...
> +     dummy@0 {
> +       linux,phandle = <0x1>;
> +       phandle = <0x1>;
> +       status = "okay";
> +       child {
> +         compatible = "barebox,dummy";
> +       };
> +     };
> +     dummy@1 {
> +       linux,phandle = <0x2>;
> +       phandle = <0x2>;
> +       status = "okay";
> +       child {
> +         compatible = "barebox,dummy";
> +       };
> +     };
> +     __symbols__ {
> +       dummy0 = "/dummy@0";
> +       dummy1 = "/dummy@1";
> +     };
> +   ...
> +
> +If you need to use a different base devicetree instead of the one compiled into
> +barebox, it needs to be replaced as described in the previous section.
> diff --git a/arch/sandbox/dts/Makefile b/arch/sandbox/dts/Makefile
> index 6f6838857..ede219e6f 100644
> --- a/arch/sandbox/dts/Makefile
> +++ b/arch/sandbox/dts/Makefile
> @@ -1,6 +1,7 @@
>  ifeq ($(CONFIG_OFTREE),y)
>  dtb-y += \
> -	sandbox.dtb
> +	sandbox.dtb \
> +	sandbox-overlay.dtbo
>  endif
>  
>  # just to build a built-in.o. Otherwise compilation fails when no devicetree is
> @@ -8,4 +9,4 @@ endif
>  obj- += dummy.o
>  
>  always := $(dtb-y)
> -clean-files := *.dtb *.dtb.S .*.dtc .*.pre .*.dts
> +clean-files := *.dtb *.dtb.S .*.dtc .*.pre .*.dts *.dtbo
> diff --git a/arch/sandbox/dts/sandbox-overlay.dtso b/arch/sandbox/dts/sandbox-overlay.dtso
> new file mode 100644
> index 000000000..f9ced04ca
> --- /dev/null
> +++ b/arch/sandbox/dts/sandbox-overlay.dtso
> @@ -0,0 +1,26 @@
> +/dts-v1/;
> +
> +/plugin/;
> +
> +/ {
> +        fragment@0 {
> +                target = <&dummy0>;
> +                __overlay__ {
> +			status = "okay";
> +                        child {
> +                                compatible = "barebox,dummy";
> +                        };
> +                };
> +        };
> +
> +        fragment@1 {
> +                target = <&dummy1>;
> +                __overlay__ {
> +			status = "okay";
> +                        child {
> +                                compatible = "barebox,dummy";
> +                        };
> +                };
> +        };
> +};
> +
> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
> index 2595aa13f..1b99a4960 100644
> --- a/arch/sandbox/dts/sandbox.dts
> +++ b/arch/sandbox/dts/sandbox.dts
> @@ -3,5 +3,11 @@
>  #include "skeleton.dtsi"
>  
>  / {
> +	dummy0: dummy@0 {
> +		status = "disabled";
> +	};
>  
> +	dummy1: dummy@1 {
> +		status = "disabled";
> +	};
>  };
> diff --git a/commands/Kconfig b/commands/Kconfig
> index 951a86963..22d131da3 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -2033,6 +2033,16 @@ config CMD_OF_NODE
>  		  -c	create a new node
>  		  -d	delete a node
>  
> +config CMD_OF_OVERLAY
> +	tristate
> +	select OFTREE
> +	select OFTREE_OVERLAY
> +	prompt "of_overlay"
> +	help
> +	  Apply a dtb overlay to internal device tree
> +
> +	  Usage: of_overlay DTBO
> +
>  config CMD_OF_PROPERTY
>  	tristate
>  	select OFTREE
> diff --git a/commands/Makefile b/commands/Makefile
> index eb4796389..f404338fa 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_CMD_USB)		+= usb.o
>  obj-$(CONFIG_CMD_TIME)		+= time.o
>  obj-$(CONFIG_CMD_OFTREE)	+= oftree.o
>  obj-$(CONFIG_CMD_OF_PROPERTY)	+= of_property.o
> +obj-$(CONFIG_CMD_OF_OVERLAY)	+= of_overlay.o
>  obj-$(CONFIG_CMD_OF_NODE)	+= of_node.o
>  obj-$(CONFIG_CMD_OF_DUMP)	+= of_dump.o
>  obj-$(CONFIG_CMD_OF_DISPLAY_TIMINGS)	+= of_display_timings.o
> diff --git a/commands/of_overlay.c b/commands/of_overlay.c
> new file mode 100644
> index 000000000..6c74c72bc
> --- /dev/null
> +++ b/commands/of_overlay.c
> @@ -0,0 +1,106 @@
> +/*
> + * of_overlay.c - device tree overlay command support
> + *
> + * Copyright (c) 2018 Pascal Vizeli <pvizeli@syshack.ch>, Hass.io

Most of the code was made by Jan,
so you should keep his Copyright.

> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <common.h>
> +#include <environment.h>
> +#include <fdt.h>
> +#include <libfile.h>
> +#include <of.h>
> +#include <command.h>
> +#include <fs.h>
> +#include <malloc.h>
> +#include <linux/ctype.h>
> +#include <linux/err.h>
> +#include <asm/byteorder.h>
> +#include <errno.h>
> +#include <getopt.h>
> +#include <init.h>
> +#include <fcntl.h>
> +#include <complete.h>
> +
> +static int do_of_overlay(int argc, char *argv[])
> +{
> +	struct fdt_header *fdt = NULL;
> +	size_t size;
> +	char *filename = NULL;
> +	int ret = 0, ovinfo_cnt;
> +	struct device_node *overlay = NULL, *root;
> +	struct of_overlay_info *ovinfo;
> +
> +	if (argc != 2 || !IS_ENABLED(CONFIG_OFTREE_OVERLAY))
> +		return COMMAND_ERROR_USAGE;

CONFIG_OFTREE_OVERLAY is "select" when adding this command,
so this check should not be needed.


> +
> +	filename = argv[1];
> +
> +	root = of_get_root_node();
> +	if (!root) {
> +		printf("no oftree loaded\n");
> +		goto out;
> +	}
This will result in return 0 - which indicate that there was no error.
Should be return 1 or something

> +
> +	fdt = read_file(filename, &size);
> +	if (!fdt) {
> +		printf("unable to read %s\n", filename);
> +		return 1;
> +	}
Like here.

> +
> +	overlay = of_unflatten_dtb(fdt);
> +	free(fdt);
> +
> +	if (IS_ERR(overlay)) {
> +		printf("parse oftree: %s\n", strerror(-ret));
> +		return PTR_ERR(overlay);
> +	}
> +
> +	ret = of_resolve(overlay);
of_resolve return a negative error in case of errors.
> +	if (ret) {
> +		printf("resolve oftree overlay: %s\n", strerror(-ret));
> +		goto out;
So here we will return a negative number from this command.
> +	}

> +
> +	ret = of_build_overlay_info(overlay, &ovinfo_cnt, &ovinfo);
> +	if (ret) {
> +		printf("prepare oftree overlay: %s\n", strerror(-ret));
> +		goto out;
> +	}
Likewise

> +
> +	ret = of_overlay(ovinfo_cnt, ovinfo);
of_overlay may return a positive or negative number
> +	if (ret) {
> +		printf("apply oftree overlay: %s\n", strerror(-ret));
So this may not print what you want.

> +		goto out;
> +	}
> +
> +	ret = 0;
> +
> +out:
> +	of_delete_node(overlay);
> +	return ret;
> +}
> +
> +BAREBOX_CMD_HELP_START(of_overlay)
> +BAREBOX_CMD_HELP_TEXT("Apply a dtb overlay to internal device tree")
> +BAREBOX_CMD_HELP_END
> +
> +BAREBOX_CMD_START(of_overlay)
> +	.cmd		= do_of_overlay,
> +	BAREBOX_CMD_DESC("Apply a dtb overlay")
> +	BAREBOX_CMD_OPTS("DTBO")
> +	BAREBOX_CMD_GROUP(CMD_GRP_MISC)
> +	BAREBOX_CMD_HELP(cmd_of_overlay_help)
> +BAREBOX_CMD_END


	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox