[PATCH v6 0/4] dtc: Dynamic DT support

[PATCH v6 0/4] dtc: Dynamic DT support

[Pantelis Antoniou]

The first two patches enabled generation of symbol & fixup
information for use with dynamic DT loading, along with the
documentation about the internal operation.

The third patch enables backward compatibility with already
present overlays out in the wild.

The fourth introduces a new magic number and new output/input
format options marking dynamic objects.

Only the first two patches are required; the last two are
optional.

Changes since v5:
* Rebase to latest dtc version.
* Addressed all the maintainer requested changes from v5
* Added new magic value for dynamic objects and new format

Changes since v4:
* Rebase to latest dtc version.
* Completely redesigned the generation of resolution data.
Now instead of being generated as part of blob generation
they are created in the live tree.
* Consequently the patchset is much smaller.
* Added -A auto-label alias generation option.
* Addressed maintainer comments.
* Added syntactic sugar for overlays in the form of .dtsi
* Added /dts-v1/ /plugin/ preferred plugin form and deprecate
the previous form (although still works for backward compatibility)

Changes since v3:
* Rebase to latest dtc version.

Changes since v2:
* Split single patch to a patchset.
* Updated to dtc mainline.
* Changed __local_fixups__ format
* Clean up for better legibility.



Pantelis Antoniou (4):
  dtc: Document the dynamic plugin internals
  dtc: Plugin and fixup support
  plugin: Transparently support old style syntax
  DTBO magic and dtbo format options

 Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
 Documentation/manual.txt             |  23 +++
 checks.c                             |   8 +-
 dtc-lexer.l                          |   5 +
 dtc-parser.y                         |  56 +++++-
 dtc.c                                |  37 +++-
 dtc.h                                |  32 +++-
 fdtdump.c                            |   2 +-
 flattree.c                           |  13 +-
 fstree.c                             |   2 +-
 libfdt/fdt.c                         |   2 +-
 libfdt/fdt.h                         |   3 +-
 libfdt/libfdt_internal.h             |   1 +
 livetree.c                           | 217 +++++++++++++++++++++++-
 tests/mangle-layout.c                |   7 +-
 15 files changed, 695 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/dt-object-internal.txt

-- 
1.7.12

[PATCH v6 1/4] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Provides the document explaining the internal mechanics of
plugins and options.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
 1 file changed, 318 insertions(+)
 create mode 100644 Documentation/dt-object-internal.txt

diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
new file mode 100644
index 0000000..734f447
--- /dev/null
+++ b/Documentation/dt-object-internal.txt
@@ -0,0 +1,318 @@
+Device Tree Dynamic Object format internals
+-------------------------------------------
+
+The Device Tree for most platforms is a static representation of
+the hardware capabilities. This is insufficient for many platforms
+that need to dynamically insert device tree fragments to the
+running kernel's live tree.
+
+This document explains the the device tree object format and the
+modifications made to the device tree compiler, which make it possible.
+
+1. Simplified Problem Definition
+--------------------------------
+
+Assume we have a platform which boots using following simplified device tree.
+
+---- foo.dts -----------------------------------------------------------------
+	/* FOO platform */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+		};
+	};
+---- foo.dts -----------------------------------------------------------------
+
+We have a number of peripherals that after probing (using some undefined method)
+should result in different device tree configuration.
+
+We cannot boot with this static tree because due to the configuration of the
+foo platform there exist multiple conficting peripherals DT fragments.
+
+So for the bar peripheral we would have this:
+
+---- foo+bar.dts -------------------------------------------------------------
+	/* FOO platform + bar peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			};
+		};
+	};
+---- foo+bar.dts -------------------------------------------------------------
+
+While for the baz peripheral we would have this:
+
+---- foo+baz.dts -------------------------------------------------------------
+	/* FOO platform + baz peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+			/* baz resources */
+			baz_res: res_baz { ... };
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* baz peripheral */
+			baz {
+				compatible = "corp,baz";
+				/* reference to another point in the tree */
+				ref-to-res = <&baz_res>;
+				... /* various properties and child nodes */
+			};
+		};
+	};
+---- foo+baz.dts -------------------------------------------------------------
+
+We note that the baz case is more complicated, since the baz peripheral needs to
+reference another node in the DT tree.
+
+2. Device Tree Object Format Requirements
+-----------------------------------------
+
+Since the device tree is used for booting a number of very different hardware
+platforms it is imperative that we tread very carefully.
+
+2.a) No changes to the Device Tree binary format for the base tree. We cannot
+modify the tree format at all and all the information we require should be
+encoded using device tree itself. We can add nodes that can be safely ignored
+by both bootloaders and the kernel. The plugin dtb's are optionally tagged
+with a different magic number in the header but otherwise they too are simple
+blobs.
+
+2.b) Changes to the DTS source format should be absolutely minimal, and should
+only be needed for the DT fragment definitions, and not the base boot DT.
+
+2.c) An explicit option should be used to instruct DTC to generate the required
+information needed for object resolution. Platforms that don't use the
+dynamic object format can safely ignore it.
+
+2.d) Finally, DT syntax changes should be kept to a minimum. It should be
+possible to express everything using the existing DT syntax.
+
+3. Implementation
+-----------------
+
+The basic unit of addressing in Device Tree is the phandle. Turns out it's
+relatively simple to extend the way phandles are generated and referenced
+so that it's possible to dynamically convert symbolic references (labels)
+to phandle values. This is a valid assumption as long as the author uses
+reference syntax and does not assign phandle values manually (which might
+be a problem with decompiled source files).
+
+We can roughly divide the operation into two steps.
+
+3.a) Compilation of the base board DTS file using the '-@' option
+generates a valid DT blob with an added __symbols__ node at the root node,
+containing a list of all nodes that are marked with a label.
+
+Using the foo.dts file above the following node will be generated;
+
+$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
+$ fdtdump foo.dtb
+...
+/ {
+	...
+	res {
+		...
+		phandle = <0x00000001>;
+		...
+	};
+	ocp {
+		...
+		phandle = <0x00000002>;
+		...
+	};
+	__symbols__ {
+		res="/res";
+		ocp="/ocp";
+	};
+};
+
+Notice that all the nodes that had a reference have been recorded, and that
+phandles have been generated for them.
+
+This blob can be used to boot the board normally, the __symbols__ node will
+be safely ignored both by the bootloader and the kernel (the only loss will
+be a few bytes of memory and disk space).
+
+3.b) The Device Tree fragments must be compiled with the same option but they
+must also have a tag (/plugin/) that allows undefined references to nodes
+that are not present at compilation time to be recorded so that the runtime
+loader can fix them.
+
+So the bar peripheral's DTS format would be of the form:
+
+/dts-v1/ /plugin/;	/* allow undefined references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&ocp>;
+		__overlay__ {
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+
+Note that there's a target property that specifies the location where the
+contents of the overlay node will be placed, and it references the node
+in the foo.dts file.
+
+$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
+$ fdtdump bar.dtbo
+...
+/ {
+	... /* properties */
+	fragment@0 {
+		target = <0xffffffff>;
+		__overlay__ {
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+	__fixups__ {
+	    ocp = "/fragment@0:target:0";
+	};
+};
+
+No __symbols__ has been generated (no label in bar.dts).
+Note that the target's ocp label is undefined, so the phandle handle
+value is filled with the illegal value '0xffffffff', while a __fixups__
+node has been generated, which marks the location in the tree where
+the label lookup should store the runtime phandle value of the ocp node.
+
+The format of the __fixups__ node entry is
+
+	<label> = "<local-full-path>:<property-name>:<offset>";
+
+<label> 		Is the label we're referring
+<local-full-path>	Is the full path of the node the reference is
+<property-name>		Is the name of the property containing the
+			reference
+<offset>		The offset (in bytes) of where the property's
+			phandle value is located.
+
+Doing the same with the baz peripheral's DTS format is a little bit more
+involved, since baz contains references to local labels which require
+local fixups.
+
+/dts-v1/ /plugin/;	/* allow undefined label references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&res>;
+		__overlay__ {
+			/* baz resources */
+			baz_res: res_baz { ... };
+		};
+	};
+	fragment@1 {
+		target = <&ocp>;
+		__overlay__ {
+			/* baz peripheral */
+			baz {
+				compatible = "corp,baz";
+				/* reference to another point in the tree */
+				ref-to-res = <&baz_res>;
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+
+Note that &bar_res reference.
+
+$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
+$ fdtdump baz.dtbo
+...
+/ {
+	... /* properties */
+	fragment@0 {
+		target = <0xffffffff>;
+		__overlay__ {
+			res_baz {
+				....
+				phandle = <0x00000001>;
+			};
+		};
+	};
+	fragment@1 {
+		target = <0xffffffff>;
+		__overlay__ {
+			baz {
+				compatible = "corp,baz";
+				... /* various properties and child nodes */
+				ref-to-res = <0x00000001>;
+			}
+		};
+	};
+	__fixups__ {
+		res = "/fragment@0:target:0";
+		ocp = "/fragment@1:target:0";
+	};
+	__local_fixups__ {
+		fragment@1 {
+			__overlay__ {
+				baz {
+					ref-to-res = <0>;
+				};
+			};
+		};
+	};
+};
+
+This is similar to the bar case, but the reference of a local label by the
+baz node generates a __local_fixups__ entry that records the place that the
+local reference is being made. No matter how phandles are allocated from dtc
+the run time loader must apply an offset to each phandle in every dynamic
+DT object loaded. The __local_fixups__ node records the place of every
+local reference so that the loader can apply the offset.
+
+There is an alternative syntax to the expanded form for overlays with phandle
+targets which makes the format similar to the one using in .dtsi include files.
+
+So for the &ocp target example above one can simply write:
+
+/dts-v1/ /plugin/;
+&ocp {
+	/* bar peripheral */
+	bar {
+		compatible = "corp,bar";
+		... /* various properties and child nodes */
+	}
+};
+
+The resulting dtb object is identical.
-- 
1.7.12

Re: [PATCH v6 1/4] dtc: Document the dynamic plugin internals

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 11906 bytes --]

One small nit in the document itself.

I have other comments, but they're about the overlay format itself,
rather than this patch as such.

On Thu, May 05, 2016 at 10:48:41PM +0300, Pantelis Antoniou wrote:
> Provides the document explaining the internal mechanics of
> plugins and options.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
>  1 file changed, 318 insertions(+)
>  create mode 100644 Documentation/dt-object-internal.txt
> 
> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> new file mode 100644
> index 0000000..734f447
> --- /dev/null
> +++ b/Documentation/dt-object-internal.txt
> @@ -0,0 +1,318 @@
> +Device Tree Dynamic Object format internals
> +-------------------------------------------
> +
> +The Device Tree for most platforms is a static representation of
> +the hardware capabilities. This is insufficient for many platforms
> +that need to dynamically insert device tree fragments to the
> +running kernel's live tree.
> +
> +This document explains the the device tree object format and the
> +modifications made to the device tree compiler, which make it possible.
> +
> +1. Simplified Problem Definition
> +--------------------------------
> +
> +Assume we have a platform which boots using following simplified device tree.
> +
> +---- foo.dts -----------------------------------------------------------------
> +	/* FOO platform */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +		};
> +	};
> +---- foo.dts -----------------------------------------------------------------
> +
> +We have a number of peripherals that after probing (using some undefined method)
> +should result in different device tree configuration.
> +
> +We cannot boot with this static tree because due to the configuration of the
> +foo platform there exist multiple conficting peripherals DT fragments.
> +
> +So for the bar peripheral we would have this:
> +
> +---- foo+bar.dts -------------------------------------------------------------
> +	/* FOO platform + bar peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			};
> +		};
> +	};
> +---- foo+bar.dts -------------------------------------------------------------
> +
> +While for the baz peripheral we would have this:
> +
> +---- foo+baz.dts -------------------------------------------------------------
> +	/* FOO platform + baz peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +			/* baz resources */
> +			baz_res: res_baz { ... };
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* baz peripheral */
> +			baz {
> +				compatible = "corp,baz";
> +				/* reference to another point in the tree */
> +				ref-to-res = <&baz_res>;
> +				... /* various properties and child nodes */
> +			};
> +		};
> +	};
> +---- foo+baz.dts -------------------------------------------------------------
> +
> +We note that the baz case is more complicated, since the baz peripheral needs to
> +reference another node in the DT tree.
> +
> +2. Device Tree Object Format Requirements
> +-----------------------------------------
> +
> +Since the device tree is used for booting a number of very different hardware
> +platforms it is imperative that we tread very carefully.
> +
> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
> +modify the tree format at all and all the information we require should be
> +encoded using device tree itself. We can add nodes that can be safely ignored
> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
> +with a different magic number in the header but otherwise they too are simple
> +blobs.
> +
> +2.b) Changes to the DTS source format should be absolutely minimal, and should
> +only be needed for the DT fragment definitions, and not the base boot DT.
> +
> +2.c) An explicit option should be used to instruct DTC to generate the required
> +information needed for object resolution. Platforms that don't use the
> +dynamic object format can safely ignore it.
> +
> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> +possible to express everything using the existing DT syntax.
> +
> +3. Implementation
> +-----------------
> +
> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> +relatively simple to extend the way phandles are generated and referenced
> +so that it's possible to dynamically convert symbolic references (labels)
> +to phandle values. This is a valid assumption as long as the author uses
> +reference syntax and does not assign phandle values manually (which might
> +be a problem with decompiled source files).
> +
> +We can roughly divide the operation into two steps.
> +
> +3.a) Compilation of the base board DTS file using the '-@' option
> +generates a valid DT blob with an added __symbols__ node at the root node,
> +containing a list of all nodes that are marked with a label.
> +
> +Using the foo.dts file above the following node will be generated;
> +
> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> +$ fdtdump foo.dtb
> +...
> +/ {
> +	...
> +	res {
> +		...
> +		phandle = <0x00000001>;
> +		...
> +	};
> +	ocp {
> +		...
> +		phandle = <0x00000002>;
> +		...
> +	};
> +	__symbols__ {
> +		res="/res";
> +		ocp="/ocp";
> +	};
> +};
> +
> +Notice that all the nodes that had a reference have been recorded, and that

s/reference/label/

> +phandles have been generated for them.


> +This blob can be used to boot the board normally, the __symbols__ node will
> +be safely ignored both by the bootloader and the kernel (the only loss will
> +be a few bytes of memory and disk space).
> +
> +3.b) The Device Tree fragments must be compiled with the same option but they
> +must also have a tag (/plugin/) that allows undefined references to nodes
> +that are not present at compilation time to be recorded so that the runtime
> +loader can fix them.
> +
> +So the bar peripheral's DTS format would be of the form:
> +
> +/dts-v1/ /plugin/;	/* allow undefined references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc. */
> +	fragment@0 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +};
> +
> +Note that there's a target property that specifies the location where the
> +contents of the overlay node will be placed, and it references the node
> +in the foo.dts file.

Ugh.. I really don't like the target stuff appearing in the dts like
this.  I thought we were changing this so these appeared in the blob,
but in the source we just used the existing overlay syntax, so for the
above, something like:

&ocp {
	...
};

Or have I gotten confused by the history of things.

> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
> +$ fdtdump bar.dtbo
> +...
> +/ {
> +	... /* properties */
> +	fragment@0 {
> +		target = <0xffffffff>;
> +		__overlay__ {
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +	__fixups__ {
> +	    ocp = "/fragment@0:target:0";

I still hate this parse-requiring string, but I guess we're stuck with
it.

> +	};
> +};
> +
> +No __symbols__ has been generated (no label in bar.dts).
> +Note that the target's ocp label is undefined, so the phandle handle
> +value is filled with the illegal value '0xffffffff', while a __fixups__
> +node has been generated, which marks the location in the tree where
> +the label lookup should store the runtime phandle value of the ocp node.
> +
> +The format of the __fixups__ node entry is
> +
> +	<label> = "<local-full-path>:<property-name>:<offset>";
> +
> +<label> 		Is the label we're referring
> +<local-full-path>	Is the full path of the node the reference is
> +<property-name>		Is the name of the property containing the
> +			reference
> +<offset>		The offset (in bytes) of where the property's
> +			phandle value is located.
> +
> +Doing the same with the baz peripheral's DTS format is a little bit more
> +involved, since baz contains references to local labels which require
> +local fixups.
> +
> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc. */
> +	fragment@0 {
> +		target = <&res>;
> +		__overlay__ {
> +			/* baz resources */
> +			baz_res: res_baz { ... };
> +		};
> +	};
> +	fragment@1 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* baz peripheral */
> +			baz {
> +				compatible = "corp,baz";
> +				/* reference to another point in the tree */
> +				ref-to-res = <&baz_res>;
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +};
> +
> +Note that &bar_res reference.
> +
> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
> +$ fdtdump baz.dtbo
> +...
> +/ {
> +	... /* properties */
> +	fragment@0 {
> +		target = <0xffffffff>;
> +		__overlay__ {
> +			res_baz {
> +				....
> +				phandle = <0x00000001>;
> +			};
> +		};
> +	};
> +	fragment@1 {
> +		target = <0xffffffff>;
> +		__overlay__ {
> +			baz {
> +				compatible = "corp,baz";
> +				... /* various properties and child nodes */
> +				ref-to-res = <0x00000001>;
> +			}
> +		};
> +	};
> +	__fixups__ {
> +		res = "/fragment@0:target:0";
> +		ocp = "/fragment@1:target:0";
> +	};
> +	__local_fixups__ {
> +		fragment@1 {
> +			__overlay__ {
> +				baz {
> +					ref-to-res = <0>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +This is similar to the bar case, but the reference of a local label by the
> +baz node generates a __local_fixups__ entry that records the place that the
> +local reference is being made. No matter how phandles are allocated from dtc
> +the run time loader must apply an offset to each phandle in every dynamic
> +DT object loaded. The __local_fixups__ node records the place of every
> +local reference so that the loader can apply the offset.

I'm still utterly baffled why the __local_fixups__ encoding is totally
different from the __fixups__ encoding.  But, again, stuck with it, I
guess.

I'd really like to see a simplified, consolidated format defined and
deprecate this one, though.

> +There is an alternative syntax to the expanded form for overlays with phandle
> +targets which makes the format similar to the one using in .dtsi include files.
> +
> +So for the &ocp target example above one can simply write:
> +
> +/dts-v1/ /plugin/;
> +&ocp {
> +	/* bar peripheral */
> +	bar {
> +		compatible = "corp,bar";
> +		... /* various properties and child nodes */
> +	}
> +};
> +
> +The resulting dtb object is identical.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 1/4] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Hi David,

> On May 24, 2016, at 07:58 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> One small nit in the document itself.
> 
> I have other comments, but they're about the overlay format itself,
> rather than this patch as such.
> 

OK.

> On Thu, May 05, 2016 at 10:48:41PM +0300, Pantelis Antoniou wrote:
>> Provides the document explaining the internal mechanics of
>> plugins and options.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>> Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 318 insertions(+)
>> create mode 100644 Documentation/dt-object-internal.txt
>> 
>> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
>> new file mode 100644
>> index 0000000..734f447
>> --- /dev/null
>> +++ b/Documentation/dt-object-internal.txt
>> @@ -0,0 +1,318 @@
>> +Device Tree Dynamic Object format internals
>> +-------------------------------------------
>> +
>> +The Device Tree for most platforms is a static representation of
>> +the hardware capabilities. This is insufficient for many platforms
>> +that need to dynamically insert device tree fragments to the
>> +running kernel's live tree.
>> +
>> +This document explains the the device tree object format and the
>> +modifications made to the device tree compiler, which make it possible.
>> +
>> +1. Simplified Problem Definition
>> +--------------------------------
>> +
>> +Assume we have a platform which boots using following simplified device tree.
>> +
>> +---- foo.dts -----------------------------------------------------------------
>> +	/* FOO platform */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +		};
>> +	};
>> +---- foo.dts -----------------------------------------------------------------
>> +
>> +We have a number of peripherals that after probing (using some undefined method)
>> +should result in different device tree configuration.
>> +
>> +We cannot boot with this static tree because due to the configuration of the
>> +foo platform there exist multiple conficting peripherals DT fragments.
>> +
>> +So for the bar peripheral we would have this:
>> +
>> +---- foo+bar.dts -------------------------------------------------------------
>> +	/* FOO platform + bar peripheral */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			};
>> +		};
>> +	};
>> +---- foo+bar.dts -------------------------------------------------------------
>> +
>> +While for the baz peripheral we would have this:
>> +
>> +---- foo+baz.dts -------------------------------------------------------------
>> +	/* FOO platform + baz peripheral */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +			/* baz resources */
>> +			baz_res: res_baz { ... };
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +
>> +			/* baz peripheral */
>> +			baz {
>> +				compatible = "corp,baz";
>> +				/* reference to another point in the tree */
>> +				ref-to-res = <&baz_res>;
>> +				... /* various properties and child nodes */
>> +			};
>> +		};
>> +	};
>> +---- foo+baz.dts -------------------------------------------------------------
>> +
>> +We note that the baz case is more complicated, since the baz peripheral needs to
>> +reference another node in the DT tree.
>> +
>> +2. Device Tree Object Format Requirements
>> +-----------------------------------------
>> +
>> +Since the device tree is used for booting a number of very different hardware
>> +platforms it is imperative that we tread very carefully.
>> +
>> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
>> +modify the tree format at all and all the information we require should be
>> +encoded using device tree itself. We can add nodes that can be safely ignored
>> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
>> +with a different magic number in the header but otherwise they too are simple
>> +blobs.
>> +
>> +2.b) Changes to the DTS source format should be absolutely minimal, and should
>> +only be needed for the DT fragment definitions, and not the base boot DT.
>> +
>> +2.c) An explicit option should be used to instruct DTC to generate the required
>> +information needed for object resolution. Platforms that don't use the
>> +dynamic object format can safely ignore it.
>> +
>> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
>> +possible to express everything using the existing DT syntax.
>> +
>> +3. Implementation
>> +-----------------
>> +
>> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
>> +relatively simple to extend the way phandles are generated and referenced
>> +so that it's possible to dynamically convert symbolic references (labels)
>> +to phandle values. This is a valid assumption as long as the author uses
>> +reference syntax and does not assign phandle values manually (which might
>> +be a problem with decompiled source files).
>> +
>> +We can roughly divide the operation into two steps.
>> +
>> +3.a) Compilation of the base board DTS file using the '-@' option
>> +generates a valid DT blob with an added __symbols__ node at the root node,
>> +containing a list of all nodes that are marked with a label.
>> +
>> +Using the foo.dts file above the following node will be generated;
>> +
>> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
>> +$ fdtdump foo.dtb
>> +...
>> +/ {
>> +	...
>> +	res {
>> +		...
>> +		phandle = <0x00000001>;
>> +		...
>> +	};
>> +	ocp {
>> +		...
>> +		phandle = <0x00000002>;
>> +		...
>> +	};
>> +	__symbols__ {
>> +		res="/res";
>> +		ocp="/ocp";
>> +	};
>> +};
>> +
>> +Notice that all the nodes that had a reference have been recorded, and that
> 
> s/reference/label/
> 

OK

>> +phandles have been generated for them.
> 
> 
>> +This blob can be used to boot the board normally, the __symbols__ node will
>> +be safely ignored both by the bootloader and the kernel (the only loss will
>> +be a few bytes of memory and disk space).
>> +
>> +3.b) The Device Tree fragments must be compiled with the same option but they
>> +must also have a tag (/plugin/) that allows undefined references to nodes
>> +that are not present at compilation time to be recorded so that the runtime
>> +loader can fix them.
>> +
>> +So the bar peripheral's DTS format would be of the form:
>> +
>> +/dts-v1/ /plugin/;	/* allow undefined references and record them */
>> +/ {
>> +	....	/* various properties for loader use; i.e. part id etc. */
>> +	fragment@0 {
>> +		target = <&ocp>;
>> +		__overlay__ {
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		};
>> +	};
>> +};
>> +
>> +Note that there's a target property that specifies the location where the
>> +contents of the overlay node will be placed, and it references the node
>> +in the foo.dts file.
> 
> Ugh.. I really don't like the target stuff appearing in the dts like
> this.  I thought we were changing this so these appeared in the blob,
> but in the source we just used the existing overlay syntax, so for the
> above, something like:
> 
> &ocp {
> 	...
> };
> 

This works, but it’s just syntactic sugar.

It does not cover the cases where the target is a path, or a different
kind of target.

Besides the sugary part, a target is something that doesn’t have anything to
do with the plugin format.

> Or have I gotten confused by the history of things.
> 

It’s got a long history for sure :)

>> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
>> +$ fdtdump bar.dtbo
>> +...
>> +/ {
>> +	... /* properties */
>> +	fragment@0 {
>> +		target = <0xffffffff>;
>> +		__overlay__ {
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		};
>> +	};
>> +	__fixups__ {
>> +	    ocp = "/fragment@0:target:0";
> 
> I still hate this parse-requiring string, but I guess we're stuck with
> it.
> 
>> +	};
>> +};
>> +
>> +No __symbols__ has been generated (no label in bar.dts).
>> +Note that the target's ocp label is undefined, so the phandle handle
>> +value is filled with the illegal value '0xffffffff', while a __fixups__
>> +node has been generated, which marks the location in the tree where
>> +the label lookup should store the runtime phandle value of the ocp node.
>> +
>> +The format of the __fixups__ node entry is
>> +
>> +	<label> = "<local-full-path>:<property-name>:<offset>";
>> +
>> +<label> 		Is the label we're referring
>> +<local-full-path>	Is the full path of the node the reference is
>> +<property-name>		Is the name of the property containing the
>> +			reference
>> +<offset>		The offset (in bytes) of where the property's
>> +			phandle value is located.
>> +
>> +Doing the same with the baz peripheral's DTS format is a little bit more
>> +involved, since baz contains references to local labels which require
>> +local fixups.
>> +
>> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
>> +/ {
>> +	....	/* various properties for loader use; i.e. part id etc. */
>> +	fragment@0 {
>> +		target = <&res>;
>> +		__overlay__ {
>> +			/* baz resources */
>> +			baz_res: res_baz { ... };
>> +		};
>> +	};
>> +	fragment@1 {
>> +		target = <&ocp>;
>> +		__overlay__ {
>> +			/* baz peripheral */
>> +			baz {
>> +				compatible = "corp,baz";
>> +				/* reference to another point in the tree */
>> +				ref-to-res = <&baz_res>;
>> +				... /* various properties and child nodes */
>> +			}
>> +		};
>> +	};
>> +};
>> +
>> +Note that &bar_res reference.
>> +
>> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
>> +$ fdtdump baz.dtbo
>> +...
>> +/ {
>> +	... /* properties */
>> +	fragment@0 {
>> +		target = <0xffffffff>;
>> +		__overlay__ {
>> +			res_baz {
>> +				....
>> +				phandle = <0x00000001>;
>> +			};
>> +		};
>> +	};
>> +	fragment@1 {
>> +		target = <0xffffffff>;
>> +		__overlay__ {
>> +			baz {
>> +				compatible = "corp,baz";
>> +				... /* various properties and child nodes */
>> +				ref-to-res = <0x00000001>;
>> +			}
>> +		};
>> +	};
>> +	__fixups__ {
>> +		res = "/fragment@0:target:0";
>> +		ocp = "/fragment@1:target:0";
>> +	};
>> +	__local_fixups__ {
>> +		fragment@1 {
>> +			__overlay__ {
>> +				baz {
>> +					ref-to-res = <0>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +};
>> +
>> +This is similar to the bar case, but the reference of a local label by the
>> +baz node generates a __local_fixups__ entry that records the place that the
>> +local reference is being made. No matter how phandles are allocated from dtc
>> +the run time loader must apply an offset to each phandle in every dynamic
>> +DT object loaded. The __local_fixups__ node records the place of every
>> +local reference so that the loader can apply the offset.
> 
> I'm still utterly baffled why the __local_fixups__ encoding is totally
> different from the __fixups__ encoding.  But, again, stuck with it, I
> guess.
> 
> I'd really like to see a simplified, consolidated format defined and
> deprecate this one, though.
> 

I did explain it before, so here it goes again.

Although the names are similar (fixups vs local fixups) the operation
performed on them is completely different.

The fixups are there so that we can resolve symbolic names to phandles, while
the local fixups are there to track where local phandles exist in order to
adjust them to be valid when the overlay is applied.

So:

fixups -> symbolic name to phandle
local fixups -> local phandle locations to be adjusted on resolution
 

>> +There is an alternative syntax to the expanded form for overlays with phandle
>> +targets which makes the format similar to the one using in .dtsi include files.
>> +
>> +So for the &ocp target example above one can simply write:
>> +
>> +/dts-v1/ /plugin/;
>> +&ocp {
>> +	/* bar peripheral */
>> +	bar {
>> +		compatible = "corp,bar";
>> +		... /* various properties and child nodes */
>> +	}
>> +};
>> +
>> +The resulting dtb object is identical.
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards

— Pantelis

Re: [PATCH v6 1/4] dtc: Document the dynamic plugin internals

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 14982 bytes --]

On Tue, May 24, 2016 at 10:43:29AM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On May 24, 2016, at 07:58 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > One small nit in the document itself.
> > 
> > I have other comments, but they're about the overlay format itself,
> > rather than this patch as such.
> > 
> 
> OK.
> 
> > On Thu, May 05, 2016 at 10:48:41PM +0300, Pantelis Antoniou wrote:
> >> Provides the document explaining the internal mechanics of
> >> plugins and options.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> ---
> >> Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
> >> 1 file changed, 318 insertions(+)
> >> create mode 100644 Documentation/dt-object-internal.txt
> >> 
> >> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> >> new file mode 100644
> >> index 0000000..734f447
> >> --- /dev/null
> >> +++ b/Documentation/dt-object-internal.txt
> >> @@ -0,0 +1,318 @@
> >> +Device Tree Dynamic Object format internals
> >> +-------------------------------------------
> >> +
> >> +The Device Tree for most platforms is a static representation of
> >> +the hardware capabilities. This is insufficient for many platforms
> >> +that need to dynamically insert device tree fragments to the
> >> +running kernel's live tree.
> >> +
> >> +This document explains the the device tree object format and the
> >> +modifications made to the device tree compiler, which make it possible.
> >> +
> >> +1. Simplified Problem Definition
> >> +--------------------------------
> >> +
> >> +Assume we have a platform which boots using following simplified device tree.
> >> +
> >> +---- foo.dts -----------------------------------------------------------------
> >> +	/* FOO platform */
> >> +	/ {
> >> +		compatible = "corp,foo";
> >> +
> >> +		/* shared resources */
> >> +		res: res {
> >> +		};
> >> +
> >> +		/* On chip peripherals */
> >> +		ocp: ocp {
> >> +			/* peripherals that are always instantiated */
> >> +			peripheral1 { ... };
> >> +		};
> >> +	};
> >> +---- foo.dts -----------------------------------------------------------------
> >> +
> >> +We have a number of peripherals that after probing (using some undefined method)
> >> +should result in different device tree configuration.
> >> +
> >> +We cannot boot with this static tree because due to the configuration of the
> >> +foo platform there exist multiple conficting peripherals DT fragments.
> >> +
> >> +So for the bar peripheral we would have this:
> >> +
> >> +---- foo+bar.dts -------------------------------------------------------------
> >> +	/* FOO platform + bar peripheral */
> >> +	/ {
> >> +		compatible = "corp,foo";
> >> +
> >> +		/* shared resources */
> >> +		res: res {
> >> +		};
> >> +
> >> +		/* On chip peripherals */
> >> +		ocp: ocp {
> >> +			/* peripherals that are always instantiated */
> >> +			peripheral1 { ... };
> >> +
> >> +			/* bar peripheral */
> >> +			bar {
> >> +				compatible = "corp,bar";
> >> +				... /* various properties and child nodes */
> >> +			};
> >> +		};
> >> +	};
> >> +---- foo+bar.dts -------------------------------------------------------------
> >> +
> >> +While for the baz peripheral we would have this:
> >> +
> >> +---- foo+baz.dts -------------------------------------------------------------
> >> +	/* FOO platform + baz peripheral */
> >> +	/ {
> >> +		compatible = "corp,foo";
> >> +
> >> +		/* shared resources */
> >> +		res: res {
> >> +			/* baz resources */
> >> +			baz_res: res_baz { ... };
> >> +		};
> >> +
> >> +		/* On chip peripherals */
> >> +		ocp: ocp {
> >> +			/* peripherals that are always instantiated */
> >> +			peripheral1 { ... };
> >> +
> >> +			/* baz peripheral */
> >> +			baz {
> >> +				compatible = "corp,baz";
> >> +				/* reference to another point in the tree */
> >> +				ref-to-res = <&baz_res>;
> >> +				... /* various properties and child nodes */
> >> +			};
> >> +		};
> >> +	};
> >> +---- foo+baz.dts -------------------------------------------------------------
> >> +
> >> +We note that the baz case is more complicated, since the baz peripheral needs to
> >> +reference another node in the DT tree.
> >> +
> >> +2. Device Tree Object Format Requirements
> >> +-----------------------------------------
> >> +
> >> +Since the device tree is used for booting a number of very different hardware
> >> +platforms it is imperative that we tread very carefully.
> >> +
> >> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
> >> +modify the tree format at all and all the information we require should be
> >> +encoded using device tree itself. We can add nodes that can be safely ignored
> >> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
> >> +with a different magic number in the header but otherwise they too are simple
> >> +blobs.
> >> +
> >> +2.b) Changes to the DTS source format should be absolutely minimal, and should
> >> +only be needed for the DT fragment definitions, and not the base boot DT.
> >> +
> >> +2.c) An explicit option should be used to instruct DTC to generate the required
> >> +information needed for object resolution. Platforms that don't use the
> >> +dynamic object format can safely ignore it.
> >> +
> >> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> >> +possible to express everything using the existing DT syntax.
> >> +
> >> +3. Implementation
> >> +-----------------
> >> +
> >> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> >> +relatively simple to extend the way phandles are generated and referenced
> >> +so that it's possible to dynamically convert symbolic references (labels)
> >> +to phandle values. This is a valid assumption as long as the author uses
> >> +reference syntax and does not assign phandle values manually (which might
> >> +be a problem with decompiled source files).
> >> +
> >> +We can roughly divide the operation into two steps.
> >> +
> >> +3.a) Compilation of the base board DTS file using the '-@' option
> >> +generates a valid DT blob with an added __symbols__ node at the root node,
> >> +containing a list of all nodes that are marked with a label.
> >> +
> >> +Using the foo.dts file above the following node will be generated;
> >> +
> >> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> >> +$ fdtdump foo.dtb
> >> +...
> >> +/ {
> >> +	...
> >> +	res {
> >> +		...
> >> +		phandle = <0x00000001>;
> >> +		...
> >> +	};
> >> +	ocp {
> >> +		...
> >> +		phandle = <0x00000002>;
> >> +		...
> >> +	};
> >> +	__symbols__ {
> >> +		res="/res";
> >> +		ocp="/ocp";
> >> +	};
> >> +};
> >> +
> >> +Notice that all the nodes that had a reference have been recorded, and that
> > 
> > s/reference/label/
> > 
> 
> OK
> 
> >> +phandles have been generated for them.
> > 
> > 
> >> +This blob can be used to boot the board normally, the __symbols__ node will
> >> +be safely ignored both by the bootloader and the kernel (the only loss will
> >> +be a few bytes of memory and disk space).
> >> +
> >> +3.b) The Device Tree fragments must be compiled with the same option but they
> >> +must also have a tag (/plugin/) that allows undefined references to nodes
> >> +that are not present at compilation time to be recorded so that the runtime
> >> +loader can fix them.
> >> +
> >> +So the bar peripheral's DTS format would be of the form:
> >> +
> >> +/dts-v1/ /plugin/;	/* allow undefined references and record them */
> >> +/ {
> >> +	....	/* various properties for loader use; i.e. part id etc. */
> >> +	fragment@0 {
> >> +		target = <&ocp>;
> >> +		__overlay__ {
> >> +			/* bar peripheral */
> >> +			bar {
> >> +				compatible = "corp,bar";
> >> +				... /* various properties and child nodes */
> >> +			}
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +Note that there's a target property that specifies the location where the
> >> +contents of the overlay node will be placed, and it references the node
> >> +in the foo.dts file.
> > 
> > Ugh.. I really don't like the target stuff appearing in the dts like
> > this.  I thought we were changing this so these appeared in the blob,
> > but in the source we just used the existing overlay syntax, so for the
> > above, something like:
> > 
> > &ocp {
> > 	...
> > };
> > 
> 
> This works, but it’s just syntactic sugar.

Hmmm....  The target= property and fragment@ nodes are part of the
internal overlay glue, rather than actual DT content.  So, I *really*
dislike including it inline in the dts file.  Come to that, I dislike
including it in the dtb, but I can see the rationale and we're kind of
stuck with it anyway.  The dts, not so much.

> It does not cover the cases where the target is a path, or a different
> kind of target.

Huh?  It certainly covers the case of a path
	&{/some/path} { ... }
What other sort of target did you have in mind?

> Besides the sugary part, a target is something that doesn’t have anything to
> do with the plugin format.
> 
> > Or have I gotten confused by the history of things.
> > 
> 
> It’s got a long history for sure :)
> 
> >> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
> >> +$ fdtdump bar.dtbo
> >> +...
> >> +/ {
> >> +	... /* properties */
> >> +	fragment@0 {
> >> +		target = <0xffffffff>;
> >> +		__overlay__ {
> >> +			bar {
> >> +				compatible = "corp,bar";
> >> +				... /* various properties and child nodes */
> >> +			}
> >> +		};
> >> +	};
> >> +	__fixups__ {
> >> +	    ocp = "/fragment@0:target:0";
> > 
> > I still hate this parse-requiring string, but I guess we're stuck with
> > it.
> > 
> >> +	};
> >> +};
> >> +
> >> +No __symbols__ has been generated (no label in bar.dts).
> >> +Note that the target's ocp label is undefined, so the phandle handle
> >> +value is filled with the illegal value '0xffffffff', while a __fixups__
> >> +node has been generated, which marks the location in the tree where
> >> +the label lookup should store the runtime phandle value of the ocp node.
> >> +
> >> +The format of the __fixups__ node entry is
> >> +
> >> +	<label> = "<local-full-path>:<property-name>:<offset>";
> >> +
> >> +<label> 		Is the label we're referring
> >> +<local-full-path>	Is the full path of the node the reference is
> >> +<property-name>		Is the name of the property containing the
> >> +			reference
> >> +<offset>		The offset (in bytes) of where the property's
> >> +			phandle value is located.
> >> +
> >> +Doing the same with the baz peripheral's DTS format is a little bit more
> >> +involved, since baz contains references to local labels which require
> >> +local fixups.
> >> +
> >> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
> >> +/ {
> >> +	....	/* various properties for loader use; i.e. part id etc. */
> >> +	fragment@0 {
> >> +		target = <&res>;
> >> +		__overlay__ {
> >> +			/* baz resources */
> >> +			baz_res: res_baz { ... };
> >> +		};
> >> +	};
> >> +	fragment@1 {
> >> +		target = <&ocp>;
> >> +		__overlay__ {
> >> +			/* baz peripheral */
> >> +			baz {
> >> +				compatible = "corp,baz";
> >> +				/* reference to another point in the tree */
> >> +				ref-to-res = <&baz_res>;
> >> +				... /* various properties and child nodes */
> >> +			}
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +Note that &bar_res reference.
> >> +
> >> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
> >> +$ fdtdump baz.dtbo
> >> +...
> >> +/ {
> >> +	... /* properties */
> >> +	fragment@0 {
> >> +		target = <0xffffffff>;
> >> +		__overlay__ {
> >> +			res_baz {
> >> +				....
> >> +				phandle = <0x00000001>;
> >> +			};
> >> +		};
> >> +	};
> >> +	fragment@1 {
> >> +		target = <0xffffffff>;
> >> +		__overlay__ {
> >> +			baz {
> >> +				compatible = "corp,baz";
> >> +				... /* various properties and child nodes */
> >> +				ref-to-res = <0x00000001>;
> >> +			}
> >> +		};
> >> +	};
> >> +	__fixups__ {
> >> +		res = "/fragment@0:target:0";
> >> +		ocp = "/fragment@1:target:0";
> >> +	};
> >> +	__local_fixups__ {
> >> +		fragment@1 {
> >> +			__overlay__ {
> >> +				baz {
> >> +					ref-to-res = <0>;
> >> +				};
> >> +			};
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +This is similar to the bar case, but the reference of a local label by the
> >> +baz node generates a __local_fixups__ entry that records the place that the
> >> +local reference is being made. No matter how phandles are allocated from dtc
> >> +the run time loader must apply an offset to each phandle in every dynamic
> >> +DT object loaded. The __local_fixups__ node records the place of every
> >> +local reference so that the loader can apply the offset.
> > 
> > I'm still utterly baffled why the __local_fixups__ encoding is totally
> > different from the __fixups__ encoding.  But, again, stuck with it, I
> > guess.
> > 
> > I'd really like to see a simplified, consolidated format defined and
> > deprecate this one, though.
> > 
> 
> I did explain it before, so here it goes again.
> 
> Although the names are similar (fixups vs local fixups) the operation
> performed on them is completely different.
> 
> The fixups are there so that we can resolve symbolic names to phandles, while
> the local fixups are there to track where local phandles exist in order to
> adjust them to be valid when the overlay is applied.
> 
> So:
> 
> fixups -> symbolic name to phandle
> local fixups -> local phandle locations to be adjusted on resolution

I get that distinction, but that's not the point.  While the method of
adjusting the phandle value is different, both types are a correction
to a single phandle value at a specific offset in a specific property.

In both cases you need to convey the location of a specific phandle
value, but that's encoded in a completely different way in each case.
That's what bothers me.

> >> +There is an alternative syntax to the expanded form for overlays with phandle
> >> +targets which makes the format similar to the one using in .dtsi include files.
> >> +
> >> +So for the &ocp target example above one can simply write:
> >> +
> >> +/dts-v1/ /plugin/;
> >> +&ocp {
> >> +	/* bar peripheral */
> >> +	bar {
> >> +		compatible = "corp,bar";
> >> +		... /* various properties and child nodes */
> >> +	}
> >> +};
> >> +
> >> +The resulting dtb object is identical.
> > 
> 
> Regards
> 
> — Pantelis
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 1/4] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Hi David,

> On May 24, 2016, at 13:50 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Tue, May 24, 2016 at 10:43:29AM +0300, Pantelis Antoniou wrote:
>> Hi David,
>> 
>>> On May 24, 2016, at 07:58 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6rL0yjEaa7Ru@public.gmane.orgau> wrote:
>>> 
>>> One small nit in the document itself.
>>> 
>>> I have other comments, but they're about the overlay format itself,
>>> rather than this patch as such.
>>> 
>> 
>> OK.
>> 
>>> On Thu, May 05, 2016 at 10:48:41PM +0300, Pantelis Antoniou wrote:
>>>> Provides the document explaining the internal mechanics of
>>>> plugins and options.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>> Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 318 insertions(+)
>>>> create mode 100644 Documentation/dt-object-internal.txt
>>>> 
>>>> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
>>>> new file mode 100644
>>>> index 0000000..734f447
>>>> --- /dev/null
>>>> +++ b/Documentation/dt-object-internal.txt
>>>> @@ -0,0 +1,318 @@
>>>> +Device Tree Dynamic Object format internals
>>>> +-------------------------------------------
>>>> +
>>>> +The Device Tree for most platforms is a static representation of
>>>> +the hardware capabilities. This is insufficient for many platforms
>>>> +that need to dynamically insert device tree fragments to the
>>>> +running kernel's live tree.
>>>> +
>>>> +This document explains the the device tree object format and the
>>>> +modifications made to the device tree compiler, which make it possible.
>>>> +
>>>> +1. Simplified Problem Definition
>>>> +--------------------------------
>>>> +
>>>> +Assume we have a platform which boots using following simplified device tree.
>>>> +
>>>> +---- foo.dts -----------------------------------------------------------------
>>>> +	/* FOO platform */
>>>> +	/ {
>>>> +		compatible = "corp,foo";
>>>> +
>>>> +		/* shared resources */
>>>> +		res: res {
>>>> +		};
>>>> +
>>>> +		/* On chip peripherals */
>>>> +		ocp: ocp {
>>>> +			/* peripherals that are always instantiated */
>>>> +			peripheral1 { ... };
>>>> +		};
>>>> +	};
>>>> +---- foo.dts -----------------------------------------------------------------
>>>> +
>>>> +We have a number of peripherals that after probing (using some undefined method)
>>>> +should result in different device tree configuration.
>>>> +
>>>> +We cannot boot with this static tree because due to the configuration of the
>>>> +foo platform there exist multiple conficting peripherals DT fragments.
>>>> +
>>>> +So for the bar peripheral we would have this:
>>>> +
>>>> +---- foo+bar.dts -------------------------------------------------------------
>>>> +	/* FOO platform + bar peripheral */
>>>> +	/ {
>>>> +		compatible = "corp,foo";
>>>> +
>>>> +		/* shared resources */
>>>> +		res: res {
>>>> +		};
>>>> +
>>>> +		/* On chip peripherals */
>>>> +		ocp: ocp {
>>>> +			/* peripherals that are always instantiated */
>>>> +			peripheral1 { ... };
>>>> +
>>>> +			/* bar peripheral */
>>>> +			bar {
>>>> +				compatible = "corp,bar";
>>>> +				... /* various properties and child nodes */
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +---- foo+bar.dts -------------------------------------------------------------
>>>> +
>>>> +While for the baz peripheral we would have this:
>>>> +
>>>> +---- foo+baz.dts -------------------------------------------------------------
>>>> +	/* FOO platform + baz peripheral */
>>>> +	/ {
>>>> +		compatible = "corp,foo";
>>>> +
>>>> +		/* shared resources */
>>>> +		res: res {
>>>> +			/* baz resources */
>>>> +			baz_res: res_baz { ... };
>>>> +		};
>>>> +
>>>> +		/* On chip peripherals */
>>>> +		ocp: ocp {
>>>> +			/* peripherals that are always instantiated */
>>>> +			peripheral1 { ... };
>>>> +
>>>> +			/* baz peripheral */
>>>> +			baz {
>>>> +				compatible = "corp,baz";
>>>> +				/* reference to another point in the tree */
>>>> +				ref-to-res = <&baz_res>;
>>>> +				... /* various properties and child nodes */
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +---- foo+baz.dts -------------------------------------------------------------
>>>> +
>>>> +We note that the baz case is more complicated, since the baz peripheral needs to
>>>> +reference another node in the DT tree.
>>>> +
>>>> +2. Device Tree Object Format Requirements
>>>> +-----------------------------------------
>>>> +
>>>> +Since the device tree is used for booting a number of very different hardware
>>>> +platforms it is imperative that we tread very carefully.
>>>> +
>>>> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
>>>> +modify the tree format at all and all the information we require should be
>>>> +encoded using device tree itself. We can add nodes that can be safely ignored
>>>> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
>>>> +with a different magic number in the header but otherwise they too are simple
>>>> +blobs.
>>>> +
>>>> +2.b) Changes to the DTS source format should be absolutely minimal, and should
>>>> +only be needed for the DT fragment definitions, and not the base boot DT.
>>>> +
>>>> +2.c) An explicit option should be used to instruct DTC to generate the required
>>>> +information needed for object resolution. Platforms that don't use the
>>>> +dynamic object format can safely ignore it.
>>>> +
>>>> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
>>>> +possible to express everything using the existing DT syntax.
>>>> +
>>>> +3. Implementation
>>>> +-----------------
>>>> +
>>>> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
>>>> +relatively simple to extend the way phandles are generated and referenced
>>>> +so that it's possible to dynamically convert symbolic references (labels)
>>>> +to phandle values. This is a valid assumption as long as the author uses
>>>> +reference syntax and does not assign phandle values manually (which might
>>>> +be a problem with decompiled source files).
>>>> +
>>>> +We can roughly divide the operation into two steps.
>>>> +
>>>> +3.a) Compilation of the base board DTS file using the '-@' option
>>>> +generates a valid DT blob with an added __symbols__ node at the root node,
>>>> +containing a list of all nodes that are marked with a label.
>>>> +
>>>> +Using the foo.dts file above the following node will be generated;
>>>> +
>>>> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
>>>> +$ fdtdump foo.dtb
>>>> +...
>>>> +/ {
>>>> +	...
>>>> +	res {
>>>> +		...
>>>> +		phandle = <0x00000001>;
>>>> +		...
>>>> +	};
>>>> +	ocp {
>>>> +		...
>>>> +		phandle = <0x00000002>;
>>>> +		...
>>>> +	};
>>>> +	__symbols__ {
>>>> +		res="/res";
>>>> +		ocp="/ocp";
>>>> +	};
>>>> +};
>>>> +
>>>> +Notice that all the nodes that had a reference have been recorded, and that
>>> 
>>> s/reference/label/
>>> 
>> 
>> OK
>> 
>>>> +phandles have been generated for them.
>>> 
>>> 
>>>> +This blob can be used to boot the board normally, the __symbols__ node will
>>>> +be safely ignored both by the bootloader and the kernel (the only loss will
>>>> +be a few bytes of memory and disk space).
>>>> +
>>>> +3.b) The Device Tree fragments must be compiled with the same option but they
>>>> +must also have a tag (/plugin/) that allows undefined references to nodes
>>>> +that are not present at compilation time to be recorded so that the runtime
>>>> +loader can fix them.
>>>> +
>>>> +So the bar peripheral's DTS format would be of the form:
>>>> +
>>>> +/dts-v1/ /plugin/;	/* allow undefined references and record them */
>>>> +/ {
>>>> +	....	/* various properties for loader use; i.e. part id etc. */
>>>> +	fragment@0 {
>>>> +		target = <&ocp>;
>>>> +		__overlay__ {
>>>> +			/* bar peripheral */
>>>> +			bar {
>>>> +				compatible = "corp,bar";
>>>> +				... /* various properties and child nodes */
>>>> +			}
>>>> +		};
>>>> +	};
>>>> +};
>>>> +
>>>> +Note that there's a target property that specifies the location where the
>>>> +contents of the overlay node will be placed, and it references the node
>>>> +in the foo.dts file.
>>> 
>>> Ugh.. I really don't like the target stuff appearing in the dts like
>>> this.  I thought we were changing this so these appeared in the blob,
>>> but in the source we just used the existing overlay syntax, so for the
>>> above, something like:
>>> 
>>> &ocp {
>>> 	...
>>> };
>>> 
>> 
>> This works, but it’s just syntactic sugar.
> 
> Hmmm....  The target= property and fragment@ nodes are part of the
> internal overlay glue, rather than actual DT content.  So, I *really*
> dislike including it inline in the dts file.  Come to that, I dislike
> including it in the dtb, but I can see the rationale and we're kind of
> stuck with it anyway.  The dts, not so much.
> 

There are more target variants to come, but they don’t require any
dtc changes.

The &label { }; transforms to fragment@0 { target = <&label>; __overlay__ { }; };

>> It does not cover the cases where the target is a path, or a different
>> kind of target.
> 
> Huh?  It certainly covers the case of a path
> 	&{/some/path} { ... }
> What other sort of target did you have in mind?
> 

That is just not supported right now. There is no phandle to resolve.

I’ll take a stab at it, but it will transform to the target-path variant.

>> Besides the sugary part, a target is something that doesn’t have anything to
>> do with the plugin format.
>> 
>>> Or have I gotten confused by the history of things.
>>> 
>> 
>> It’s got a long history for sure :)
>> 
>>>> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
>>>> +$ fdtdump bar.dtbo
>>>> +...
>>>> +/ {
>>>> +	... /* properties */
>>>> +	fragment@0 {
>>>> +		target = <0xffffffff>;
>>>> +		__overlay__ {
>>>> +			bar {
>>>> +				compatible = "corp,bar";
>>>> +				... /* various properties and child nodes */
>>>> +			}
>>>> +		};
>>>> +	};
>>>> +	__fixups__ {
>>>> +	    ocp = "/fragment@0:target:0";
>>> 
>>> I still hate this parse-requiring string, but I guess we're stuck with
>>> it.
>>> 
>>>> +	};
>>>> +};
>>>> +
>>>> +No __symbols__ has been generated (no label in bar.dts).
>>>> +Note that the target's ocp label is undefined, so the phandle handle
>>>> +value is filled with the illegal value '0xffffffff', while a __fixups__
>>>> +node has been generated, which marks the location in the tree where
>>>> +the label lookup should store the runtime phandle value of the ocp node.
>>>> +
>>>> +The format of the __fixups__ node entry is
>>>> +
>>>> +	<label> = "<local-full-path>:<property-name>:<offset>";
>>>> +
>>>> +<label> 		Is the label we're referring
>>>> +<local-full-path>	Is the full path of the node the reference is
>>>> +<property-name>		Is the name of the property containing the
>>>> +			reference
>>>> +<offset>		The offset (in bytes) of where the property's
>>>> +			phandle value is located.
>>>> +
>>>> +Doing the same with the baz peripheral's DTS format is a little bit more
>>>> +involved, since baz contains references to local labels which require
>>>> +local fixups.
>>>> +
>>>> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
>>>> +/ {
>>>> +	....	/* various properties for loader use; i.e. part id etc. */
>>>> +	fragment@0 {
>>>> +		target = <&res>;
>>>> +		__overlay__ {
>>>> +			/* baz resources */
>>>> +			baz_res: res_baz { ... };
>>>> +		};
>>>> +	};
>>>> +	fragment@1 {
>>>> +		target = <&ocp>;
>>>> +		__overlay__ {
>>>> +			/* baz peripheral */
>>>> +			baz {
>>>> +				compatible = "corp,baz";
>>>> +				/* reference to another point in the tree */
>>>> +				ref-to-res = <&baz_res>;
>>>> +				... /* various properties and child nodes */
>>>> +			}
>>>> +		};
>>>> +	};
>>>> +};
>>>> +
>>>> +Note that &bar_res reference.
>>>> +
>>>> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
>>>> +$ fdtdump baz.dtbo
>>>> +...
>>>> +/ {
>>>> +	... /* properties */
>>>> +	fragment@0 {
>>>> +		target = <0xffffffff>;
>>>> +		__overlay__ {
>>>> +			res_baz {
>>>> +				....
>>>> +				phandle = <0x00000001>;
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +	fragment@1 {
>>>> +		target = <0xffffffff>;
>>>> +		__overlay__ {
>>>> +			baz {
>>>> +				compatible = "corp,baz";
>>>> +				... /* various properties and child nodes */
>>>> +				ref-to-res = <0x00000001>;
>>>> +			}
>>>> +		};
>>>> +	};
>>>> +	__fixups__ {
>>>> +		res = "/fragment@0:target:0";
>>>> +		ocp = "/fragment@1:target:0";
>>>> +	};
>>>> +	__local_fixups__ {
>>>> +		fragment@1 {
>>>> +			__overlay__ {
>>>> +				baz {
>>>> +					ref-to-res = <0>;
>>>> +				};
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +};
>>>> +
>>>> +This is similar to the bar case, but the reference of a local label by the
>>>> +baz node generates a __local_fixups__ entry that records the place that the
>>>> +local reference is being made. No matter how phandles are allocated from dtc
>>>> +the run time loader must apply an offset to each phandle in every dynamic
>>>> +DT object loaded. The __local_fixups__ node records the place of every
>>>> +local reference so that the loader can apply the offset.
>>> 
>>> I'm still utterly baffled why the __local_fixups__ encoding is totally
>>> different from the __fixups__ encoding.  But, again, stuck with it, I
>>> guess.
>>> 
>>> I'd really like to see a simplified, consolidated format defined and
>>> deprecate this one, though.
>>> 
>> 
>> I did explain it before, so here it goes again.
>> 
>> Although the names are similar (fixups vs local fixups) the operation
>> performed on them is completely different.
>> 
>> The fixups are there so that we can resolve symbolic names to phandles, while
>> the local fixups are there to track where local phandles exist in order to
>> adjust them to be valid when the overlay is applied.
>> 
>> So:
>> 
>> fixups -> symbolic name to phandle
>> local fixups -> local phandle locations to be adjusted on resolution
> 
> I get that distinction, but that's not the point.  While the method of
> adjusting the phandle value is different, both types are a correction
> to a single phandle value at a specific offset in a specific property.
> 
> In both cases you need to convey the location of a specific phandle
> value, but that's encoded in a completely different way in each case.
> That's what bothers me.
> 

It might be possible for the format to change but it’s just not going to be the same.

There’s no place to put the label reference in the fixup form so no code-reuse is
going to happen.

>>>> +There is an alternative syntax to the expanded form for overlays with phandle
>>>> +targets which makes the format similar to the one using in .dtsi include files.
>>>> +
>>>> +So for the &ocp target example above one can simply write:
>>>> +
>>>> +/dts-v1/ /plugin/;
>>>> +&ocp {
>>>> +	/* bar peripheral */
>>>> +	bar {
>>>> +		compatible = "corp,bar";
>>>> +		... /* various properties and child nodes */
>>>> +	}
>>>> +};
>>>> +
>>>> +The resulting dtb object is identical.
>>> 
>> 
>> Regards
>> 
>> — Pantelis
>> 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards

— Pantelis

Re: [PATCH v6 1/4] dtc: Document the dynamic plugin internals

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 17493 bytes --]

On Tue, May 24, 2016 at 03:53:07PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On May 24, 2016, at 13:50 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Tue, May 24, 2016 at 10:43:29AM +0300, Pantelis Antoniou wrote:
> >> Hi David,
> >> 
> >>> On May 24, 2016, at 07:58 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >>> 
> >>> One small nit in the document itself.
> >>> 
> >>> I have other comments, but they're about the overlay format itself,
> >>> rather than this patch as such.
> >>> 
> >> 
> >> OK.
> >> 
> >>> On Thu, May 05, 2016 at 10:48:41PM +0300, Pantelis Antoniou wrote:
> >>>> Provides the document explaining the internal mechanics of
> >>>> plugins and options.
> >>>> 
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >>>> ---
> >>>> Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
> >>>> 1 file changed, 318 insertions(+)
> >>>> create mode 100644 Documentation/dt-object-internal.txt
> >>>> 
> >>>> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> >>>> new file mode 100644
> >>>> index 0000000..734f447
> >>>> --- /dev/null
> >>>> +++ b/Documentation/dt-object-internal.txt
> >>>> @@ -0,0 +1,318 @@
> >>>> +Device Tree Dynamic Object format internals
> >>>> +-------------------------------------------
> >>>> +
> >>>> +The Device Tree for most platforms is a static representation of
> >>>> +the hardware capabilities. This is insufficient for many platforms
> >>>> +that need to dynamically insert device tree fragments to the
> >>>> +running kernel's live tree.
> >>>> +
> >>>> +This document explains the the device tree object format and the
> >>>> +modifications made to the device tree compiler, which make it possible.
> >>>> +
> >>>> +1. Simplified Problem Definition
> >>>> +--------------------------------
> >>>> +
> >>>> +Assume we have a platform which boots using following simplified device tree.
> >>>> +
> >>>> +---- foo.dts -----------------------------------------------------------------
> >>>> +	/* FOO platform */
> >>>> +	/ {
> >>>> +		compatible = "corp,foo";
> >>>> +
> >>>> +		/* shared resources */
> >>>> +		res: res {
> >>>> +		};
> >>>> +
> >>>> +		/* On chip peripherals */
> >>>> +		ocp: ocp {
> >>>> +			/* peripherals that are always instantiated */
> >>>> +			peripheral1 { ... };
> >>>> +		};
> >>>> +	};
> >>>> +---- foo.dts -----------------------------------------------------------------
> >>>> +
> >>>> +We have a number of peripherals that after probing (using some undefined method)
> >>>> +should result in different device tree configuration.
> >>>> +
> >>>> +We cannot boot with this static tree because due to the configuration of the
> >>>> +foo platform there exist multiple conficting peripherals DT fragments.
> >>>> +
> >>>> +So for the bar peripheral we would have this:
> >>>> +
> >>>> +---- foo+bar.dts -------------------------------------------------------------
> >>>> +	/* FOO platform + bar peripheral */
> >>>> +	/ {
> >>>> +		compatible = "corp,foo";
> >>>> +
> >>>> +		/* shared resources */
> >>>> +		res: res {
> >>>> +		};
> >>>> +
> >>>> +		/* On chip peripherals */
> >>>> +		ocp: ocp {
> >>>> +			/* peripherals that are always instantiated */
> >>>> +			peripheral1 { ... };
> >>>> +
> >>>> +			/* bar peripheral */
> >>>> +			bar {
> >>>> +				compatible = "corp,bar";
> >>>> +				... /* various properties and child nodes */
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +---- foo+bar.dts -------------------------------------------------------------
> >>>> +
> >>>> +While for the baz peripheral we would have this:
> >>>> +
> >>>> +---- foo+baz.dts -------------------------------------------------------------
> >>>> +	/* FOO platform + baz peripheral */
> >>>> +	/ {
> >>>> +		compatible = "corp,foo";
> >>>> +
> >>>> +		/* shared resources */
> >>>> +		res: res {
> >>>> +			/* baz resources */
> >>>> +			baz_res: res_baz { ... };
> >>>> +		};
> >>>> +
> >>>> +		/* On chip peripherals */
> >>>> +		ocp: ocp {
> >>>> +			/* peripherals that are always instantiated */
> >>>> +			peripheral1 { ... };
> >>>> +
> >>>> +			/* baz peripheral */
> >>>> +			baz {
> >>>> +				compatible = "corp,baz";
> >>>> +				/* reference to another point in the tree */
> >>>> +				ref-to-res = <&baz_res>;
> >>>> +				... /* various properties and child nodes */
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +---- foo+baz.dts -------------------------------------------------------------
> >>>> +
> >>>> +We note that the baz case is more complicated, since the baz peripheral needs to
> >>>> +reference another node in the DT tree.
> >>>> +
> >>>> +2. Device Tree Object Format Requirements
> >>>> +-----------------------------------------
> >>>> +
> >>>> +Since the device tree is used for booting a number of very different hardware
> >>>> +platforms it is imperative that we tread very carefully.
> >>>> +
> >>>> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
> >>>> +modify the tree format at all and all the information we require should be
> >>>> +encoded using device tree itself. We can add nodes that can be safely ignored
> >>>> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
> >>>> +with a different magic number in the header but otherwise they too are simple
> >>>> +blobs.
> >>>> +
> >>>> +2.b) Changes to the DTS source format should be absolutely minimal, and should
> >>>> +only be needed for the DT fragment definitions, and not the base boot DT.
> >>>> +
> >>>> +2.c) An explicit option should be used to instruct DTC to generate the required
> >>>> +information needed for object resolution. Platforms that don't use the
> >>>> +dynamic object format can safely ignore it.
> >>>> +
> >>>> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> >>>> +possible to express everything using the existing DT syntax.
> >>>> +
> >>>> +3. Implementation
> >>>> +-----------------
> >>>> +
> >>>> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> >>>> +relatively simple to extend the way phandles are generated and referenced
> >>>> +so that it's possible to dynamically convert symbolic references (labels)
> >>>> +to phandle values. This is a valid assumption as long as the author uses
> >>>> +reference syntax and does not assign phandle values manually (which might
> >>>> +be a problem with decompiled source files).
> >>>> +
> >>>> +We can roughly divide the operation into two steps.
> >>>> +
> >>>> +3.a) Compilation of the base board DTS file using the '-@' option
> >>>> +generates a valid DT blob with an added __symbols__ node at the root node,
> >>>> +containing a list of all nodes that are marked with a label.
> >>>> +
> >>>> +Using the foo.dts file above the following node will be generated;
> >>>> +
> >>>> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> >>>> +$ fdtdump foo.dtb
> >>>> +...
> >>>> +/ {
> >>>> +	...
> >>>> +	res {
> >>>> +		...
> >>>> +		phandle = <0x00000001>;
> >>>> +		...
> >>>> +	};
> >>>> +	ocp {
> >>>> +		...
> >>>> +		phandle = <0x00000002>;
> >>>> +		...
> >>>> +	};
> >>>> +	__symbols__ {
> >>>> +		res="/res";
> >>>> +		ocp="/ocp";
> >>>> +	};
> >>>> +};
> >>>> +
> >>>> +Notice that all the nodes that had a reference have been recorded, and that
> >>> 
> >>> s/reference/label/
> >>> 
> >> 
> >> OK
> >> 
> >>>> +phandles have been generated for them.
> >>> 
> >>> 
> >>>> +This blob can be used to boot the board normally, the __symbols__ node will
> >>>> +be safely ignored both by the bootloader and the kernel (the only loss will
> >>>> +be a few bytes of memory and disk space).
> >>>> +
> >>>> +3.b) The Device Tree fragments must be compiled with the same option but they
> >>>> +must also have a tag (/plugin/) that allows undefined references to nodes
> >>>> +that are not present at compilation time to be recorded so that the runtime
> >>>> +loader can fix them.
> >>>> +
> >>>> +So the bar peripheral's DTS format would be of the form:
> >>>> +
> >>>> +/dts-v1/ /plugin/;	/* allow undefined references and record them */
> >>>> +/ {
> >>>> +	....	/* various properties for loader use; i.e. part id etc. */
> >>>> +	fragment@0 {
> >>>> +		target = <&ocp>;
> >>>> +		__overlay__ {
> >>>> +			/* bar peripheral */
> >>>> +			bar {
> >>>> +				compatible = "corp,bar";
> >>>> +				... /* various properties and child nodes */
> >>>> +			}
> >>>> +		};
> >>>> +	};
> >>>> +};
> >>>> +
> >>>> +Note that there's a target property that specifies the location where the
> >>>> +contents of the overlay node will be placed, and it references the node
> >>>> +in the foo.dts file.
> >>> 
> >>> Ugh.. I really don't like the target stuff appearing in the dts like
> >>> this.  I thought we were changing this so these appeared in the blob,
> >>> but in the source we just used the existing overlay syntax, so for the
> >>> above, something like:
> >>> 
> >>> &ocp {
> >>> 	...
> >>> };
> >>> 
> >> 
> >> This works, but it’s just syntactic sugar.
> > 
> > Hmmm....  The target= property and fragment@ nodes are part of the
> > internal overlay glue, rather than actual DT content.  So, I *really*
> > dislike including it inline in the dts file.  Come to that, I dislike
> > including it in the dtb, but I can see the rationale and we're kind of
> > stuck with it anyway.  The dts, not so much.
> > 
> 
> There are more target variants to come, but they don’t require any
> dtc changes.
> 
> The &label { }; transforms to fragment@0 { target = <&label>; __overlay__ { }; };
> 
> >> It does not cover the cases where the target is a path, or a different
> >> kind of target.
> > 
> > Huh?  It certainly covers the case of a path
> > 	&{/some/path} { ... }
> > What other sort of target did you have in mind?
> > 
> 
> That is just not supported right now. There is no phandle to resolve.
> 
> I’ll take a stab at it, but it will transform to the target-path variant.

Right, but my point is there's an obvious dts syntex for
path-targetted fragments.  My inclination is to create syntax for any
new fragment types, rather than expose the encoding of the overlay
metadata as if it were in-band information.

> >> Besides the sugary part, a target is something that doesn’t have anything to
> >> do with the plugin format.
> >> 
> >>> Or have I gotten confused by the history of things.
> >>> 
> >> 
> >> It’s got a long history for sure :)
> >> 
> >>>> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
> >>>> +$ fdtdump bar.dtbo
> >>>> +...
> >>>> +/ {
> >>>> +	... /* properties */
> >>>> +	fragment@0 {
> >>>> +		target = <0xffffffff>;
> >>>> +		__overlay__ {
> >>>> +			bar {
> >>>> +				compatible = "corp,bar";
> >>>> +				... /* various properties and child nodes */
> >>>> +			}
> >>>> +		};
> >>>> +	};
> >>>> +	__fixups__ {
> >>>> +	    ocp = "/fragment@0:target:0";
> >>> 
> >>> I still hate this parse-requiring string, but I guess we're stuck with
> >>> it.
> >>> 
> >>>> +	};
> >>>> +};
> >>>> +
> >>>> +No __symbols__ has been generated (no label in bar.dts).
> >>>> +Note that the target's ocp label is undefined, so the phandle handle
> >>>> +value is filled with the illegal value '0xffffffff', while a __fixups__
> >>>> +node has been generated, which marks the location in the tree where
> >>>> +the label lookup should store the runtime phandle value of the ocp node.
> >>>> +
> >>>> +The format of the __fixups__ node entry is
> >>>> +
> >>>> +	<label> = "<local-full-path>:<property-name>:<offset>";
> >>>> +
> >>>> +<label> 		Is the label we're referring
> >>>> +<local-full-path>	Is the full path of the node the reference is
> >>>> +<property-name>		Is the name of the property containing the
> >>>> +			reference
> >>>> +<offset>		The offset (in bytes) of where the property's
> >>>> +			phandle value is located.
> >>>> +
> >>>> +Doing the same with the baz peripheral's DTS format is a little bit more
> >>>> +involved, since baz contains references to local labels which require
> >>>> +local fixups.
> >>>> +
> >>>> +/dts-v1/ /plugin/;	/* allow undefined label references and record them */
> >>>> +/ {
> >>>> +	....	/* various properties for loader use; i.e. part id etc. */
> >>>> +	fragment@0 {
> >>>> +		target = <&res>;
> >>>> +		__overlay__ {
> >>>> +			/* baz resources */
> >>>> +			baz_res: res_baz { ... };
> >>>> +		};
> >>>> +	};
> >>>> +	fragment@1 {
> >>>> +		target = <&ocp>;
> >>>> +		__overlay__ {
> >>>> +			/* baz peripheral */
> >>>> +			baz {
> >>>> +				compatible = "corp,baz";
> >>>> +				/* reference to another point in the tree */
> >>>> +				ref-to-res = <&baz_res>;
> >>>> +				... /* various properties and child nodes */
> >>>> +			}
> >>>> +		};
> >>>> +	};
> >>>> +};
> >>>> +
> >>>> +Note that &bar_res reference.
> >>>> +
> >>>> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
> >>>> +$ fdtdump baz.dtbo
> >>>> +...
> >>>> +/ {
> >>>> +	... /* properties */
> >>>> +	fragment@0 {
> >>>> +		target = <0xffffffff>;
> >>>> +		__overlay__ {
> >>>> +			res_baz {
> >>>> +				....
> >>>> +				phandle = <0x00000001>;
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +	fragment@1 {
> >>>> +		target = <0xffffffff>;
> >>>> +		__overlay__ {
> >>>> +			baz {
> >>>> +				compatible = "corp,baz";
> >>>> +				... /* various properties and child nodes */
> >>>> +				ref-to-res = <0x00000001>;
> >>>> +			}
> >>>> +		};
> >>>> +	};
> >>>> +	__fixups__ {
> >>>> +		res = "/fragment@0:target:0";
> >>>> +		ocp = "/fragment@1:target:0";
> >>>> +	};
> >>>> +	__local_fixups__ {
> >>>> +		fragment@1 {
> >>>> +			__overlay__ {
> >>>> +				baz {
> >>>> +					ref-to-res = <0>;
> >>>> +				};
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +};
> >>>> +
> >>>> +This is similar to the bar case, but the reference of a local label by the
> >>>> +baz node generates a __local_fixups__ entry that records the place that the
> >>>> +local reference is being made. No matter how phandles are allocated from dtc
> >>>> +the run time loader must apply an offset to each phandle in every dynamic
> >>>> +DT object loaded. The __local_fixups__ node records the place of every
> >>>> +local reference so that the loader can apply the offset.
> >>> 
> >>> I'm still utterly baffled why the __local_fixups__ encoding is totally
> >>> different from the __fixups__ encoding.  But, again, stuck with it, I
> >>> guess.
> >>> 
> >>> I'd really like to see a simplified, consolidated format defined and
> >>> deprecate this one, though.
> >>> 
> >> 
> >> I did explain it before, so here it goes again.
> >> 
> >> Although the names are similar (fixups vs local fixups) the operation
> >> performed on them is completely different.
> >> 
> >> The fixups are there so that we can resolve symbolic names to phandles, while
> >> the local fixups are there to track where local phandles exist in order to
> >> adjust them to be valid when the overlay is applied.
> >> 
> >> So:
> >> 
> >> fixups -> symbolic name to phandle
> >> local fixups -> local phandle locations to be adjusted on resolution
> > 
> > I get that distinction, but that's not the point.  While the method of
> > adjusting the phandle value is different, both types are a correction
> > to a single phandle value at a specific offset in a specific property.
> > 
> > In both cases you need to convey the location of a specific phandle
> > value, but that's encoded in a completely different way in each case.
> > That's what bothers me.
> > 
> 
> It might be possible for the format to change but it’s just not going to be the same.

Not identical, no, but it could be much more similar than it is.  For
example:

/{
	fragment@0 {
		target = <&somewhere>;
		__overlay__ = {
			dev0: dev@0 {
				link = <&devx>;
			};
			dev@1 {
				link = <&dev0>;
			};
		};
	};
	__fixups__ = {
		somewhere = "/fragment@0:target:0";
		devx = "/fragment@0/__overlay__/dev@0:link:0";
		local,phandles = "/fragment@0/__overlay__/dev@1:link:0";
	};
};

(',' is not a valid character for labels, so this is unambiguous).

> There’s no place to put the label reference in the fixup form so no code-reuse is
> going to happen.

I don't follow what you mean by this.

> >>>> +There is an alternative syntax to the expanded form for overlays with phandle
> >>>> +targets which makes the format similar to the one using in .dtsi include files.
> >>>> +
> >>>> +So for the &ocp target example above one can simply write:
> >>>> +
> >>>> +/dts-v1/ /plugin/;
> >>>> +&ocp {
> >>>> +	/* bar peripheral */
> >>>> +	bar {
> >>>> +		compatible = "corp,bar";
> >>>> +		... /* various properties and child nodes */
> >>>> +	}
> >>>> +};
> >>>> +
> >>>> +The resulting dtb object is identical.
> >>> 
> >> 
> >> Regards
> >> 
> >> — Pantelis
> >> 
> > 
> 
> Regards
> 
> — Pantelis
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v6 2/4] dtc: Plugin and fixup support

[Pantelis Antoniou]

This patch enable the generation of symbols & local fixup information
for trees compiled with the -@ (--symbols) option.

Using this patch labels in the tree and their users emit information
in __symbols__ and __local_fixups__ nodes.

The __fixups__ node make possible the dynamic resolution of phandle
references which are present in the plugin tree but lie in the
tree that are applying the overlay against.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/manual.txt |  16 ++++
 checks.c                 |   8 +-
 dtc-lexer.l              |   5 ++
 dtc-parser.y             |  42 +++++++--
 dtc.c                    |  23 ++++-
 dtc.h                    |  28 +++++-
 flattree.c               |   2 +-
 fstree.c                 |   2 +-
 livetree.c               | 217 ++++++++++++++++++++++++++++++++++++++++++++++-
 9 files changed, 329 insertions(+), 14 deletions(-)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 398de32..3e16c24 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -119,6 +119,20 @@ Options:
 	Make space for <number> reserve map entries
 	Relevant for dtb and asm output only.
 
+    -@
+	Generates a __symbols__ node at the root node of the resulting blob
+	for any node labels used, and for any local references using phandles
+	it also generates a __local_fixups__ node that tracks them.
+
+	When using the /plugin/ tag all unresolved label references to
+	be tracked in the __fixups__ node, making dynamic resolution possible.
+
+    -A
+	Generate automatically aliases for all node labels. This is similar to
+	the -@ option (the __symbols__ node contain identical information) but
+	the semantics are slightly different since no phandles are automatically
+	generated for labeled nodes.
+
     -S <bytes>
 	Ensure the blob at least <bytes> long, adding additional
 	space if needed.
@@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
 
     devicetree:   '/' nodedef
 
+    plugindecl:   '/' 'plugin' '/' ';'
+
     nodedef:      '{' list_of_property list_of_subnode '}' ';'
 
     property:     label PROPNAME '=' propdata ';'
diff --git a/checks.c b/checks.c
index 386f956..3d4c3c6 100644
--- a/checks.c
+++ b/checks.c
@@ -490,8 +490,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
 
 		refnode = get_node_by_ref(dt, m->ref);
 		if (! refnode) {
-			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
-			     m->ref);
+			if (!(tree_get_versionflags(dt) & VF_PLUGIN))
+				FAIL(c, "Reference to non-existent node or "
+						"label \"%s\"\n", m->ref);
+			else /* mark the entry as unresolved */
+				*((cell_t *)(prop->val.val + m->offset)) =
+					cpu_to_fdt32(0xffffffff);
 			continue;
 		}
 
diff --git a/dtc-lexer.l b/dtc-lexer.l
index 790fbf6..40bbc87 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
 			return DT_V1;
 		}
 
+<*>"/plugin/"	{
+			DPRINT("Keyword: /plugin/\n");
+			return DT_PLUGIN;
+		}
+
 <*>"/memreserve/"	{
 			DPRINT("Keyword: /memreserve/\n");
 			BEGIN_DEFAULT();
diff --git a/dtc-parser.y b/dtc-parser.y
index 000873f..2d3399e 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -19,6 +19,7 @@
  */
 %{
 #include <stdio.h>
+#include <inttypes.h>
 
 #include "dtc.h"
 #include "srcpos.h"
@@ -52,9 +53,11 @@ extern bool treesource_error;
 	struct node *nodelist;
 	struct reserve_info *re;
 	uint64_t integer;
+	unsigned int flags;
 }
 
 %token DT_V1
+%token DT_PLUGIN
 %token DT_MEMRESERVE
 %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
 %token DT_BITS
@@ -71,6 +74,8 @@ extern bool treesource_error;
 
 %type <data> propdata
 %type <data> propdataprefix
+%type <flags> versioninfo
+%type <flags> plugindecl
 %type <re> memreserve
 %type <re> memreserves
 %type <array> arrayprefix
@@ -101,13 +106,31 @@ extern bool treesource_error;
 %%
 
 sourcefile:
-	  DT_V1 ';' memreserves devicetree
+	  versioninfo ';' memreserves devicetree
 		{
-			the_boot_info = build_boot_info($3, $4,
+			the_boot_info = build_boot_info($1, $3, $4,
 							guess_boot_cpuid($4));
 		}
 	;
 
+versioninfo:
+	DT_V1 plugindecl
+		{
+			$$ = VF_DT_V1 | $2;
+		}
+	;
+
+plugindecl:
+	DT_PLUGIN
+		{
+			$$ = VF_PLUGIN;
+		}
+	| /* empty */
+		{
+			$$ = 0;
+		}
+	;
+
 memreserves:
 	  /* empty */
 		{
@@ -156,10 +179,14 @@ devicetree:
 		{
 			struct node *target = get_node_by_ref($1, $2);
 
-			if (target)
+			if (target) {
 				merge_nodes(target, $3);
-			else
-				ERROR(&@2, "Label or path %s not found", $2);
+			} else {
+				if (symbol_fixup_support)
+					add_orphan_node($1, $3, $2);
+				else
+					ERROR(&@2, "Label or path %s not found", $2);
+			}
 			$$ = $1;
 		}
 	| devicetree DT_DEL_NODE DT_REF ';'
@@ -174,6 +201,11 @@ devicetree:
 
 			$$ = $1;
 		}
+	| /* empty */
+		{
+			/* build empty node */
+			$$ = name_node(build_node(NULL, NULL), "");
+		}
 	;
 
 nodedef:
diff --git a/dtc.c b/dtc.c
index 5fa23c4..f8277fb 100644
--- a/dtc.c
+++ b/dtc.c
@@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
 int minsize;		/* Minimum blob size */
 int padsize;		/* Additional padding to blob */
 int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
+int symbol_fixup_support;
+int auto_label_aliases;
 
 static void fill_fullpaths(struct node *tree, const char *prefix)
 {
@@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
 #define FDT_VERSION(version)	_FDT_VERSION(version)
 #define _FDT_VERSION(version)	#version
 static const char usage_synopsis[] = "dtc [options] <input file>";
-static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
 static struct option const usage_long_opts[] = {
 	{"quiet",            no_argument, NULL, 'q'},
 	{"in-format",         a_argument, NULL, 'I'},
@@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
 	{"phandle",           a_argument, NULL, 'H'},
 	{"warning",           a_argument, NULL, 'W'},
 	{"error",             a_argument, NULL, 'E'},
+	{"symbols",	     no_argument, NULL, '@'},
+	{"auto-alias",       no_argument, NULL, 'A'},
 	{"help",             no_argument, NULL, 'h'},
 	{"version",          no_argument, NULL, 'v'},
 	{NULL,               no_argument, NULL, 0x0},
@@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
 	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
 	"\n\tEnable/disable warnings (prefix with \"no-\")",
 	"\n\tEnable/disable errors (prefix with \"no-\")",
+	"\n\tEnable symbols/fixup support",
+	"\n\tEnable auto-alias of labels",
 	"\n\tPrint this help and exit",
 	"\n\tPrint version and exit",
 	NULL,
@@ -233,7 +239,12 @@ int main(int argc, char *argv[])
 		case 'E':
 			parse_checks_option(false, true, optarg);
 			break;
-
+		case '@':
+			symbol_fixup_support = 1;
+			break;
+		case 'A':
+			auto_label_aliases = 1;
+			break;
 		case 'h':
 			usage(NULL);
 		default:
@@ -294,6 +305,14 @@ int main(int argc, char *argv[])
 	if (sort)
 		sort_tree(bi);
 
+	if (auto_label_aliases)
+		generate_label_tree(bi->dt, "aliases", false);
+
+	if (symbol_fixup_support) {
+		generate_label_tree(bi->dt, "__symbols__", true);
+		generate_fixups_tree(bi->dt);
+	}
+
 	if (streq(outname, "-")) {
 		outf = stdout;
 	} else {
diff --git a/dtc.h b/dtc.h
index 56212c8..68cb109 100644
--- a/dtc.h
+++ b/dtc.h
@@ -20,7 +20,6 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
  *                                                                   USA
  */
-
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -54,6 +53,12 @@ extern int reservenum;		/* Number of memory reservation slots */
 extern int minsize;		/* Minimum blob size */
 extern int padsize;		/* Additional padding to blob */
 extern int phandle_format;	/* Use linux,phandle or phandle properties */
+extern int symbol_fixup_support;/* enable symbols & fixup support */
+extern int auto_label_aliases;	/* auto generate labels -> aliases */
+
+/*
+ * Tree source globals
+ */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
@@ -158,6 +163,9 @@ struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+
+	/* only for the root (parent == NULL) */
+	struct boot_info *bi;
 };
 
 #define for_each_label_withdel(l0, l) \
@@ -194,6 +202,7 @@ struct node *build_node_delete(void);
 struct node *name_node(struct node *node, char *name);
 struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
+void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
 
 void add_property(struct node *node, struct property *prop);
 void delete_property_by_name(struct node *node, char *name);
@@ -236,14 +245,29 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
 
 
 struct boot_info {
+	unsigned int versionflags;
 	struct reserve_info *reservelist;
 	struct node *dt;		/* the device tree */
 	uint32_t boot_cpuid_phys;
 };
 
-struct boot_info *build_boot_info(struct reserve_info *reservelist,
+/* version flags definitions */
+#define VF_DT_V1	0x0001	/* /dts-v1/ */
+#define VF_PLUGIN	0x0002	/* /plugin/ */
+
+static inline unsigned int tree_get_versionflags(struct node *dt)
+{
+	if (!dt || !dt->bi)
+		return 0;
+	return dt->bi->versionflags;
+}
+
+struct boot_info *build_boot_info(unsigned int versionflags,
+				  struct reserve_info *reservelist,
 				  struct node *tree, uint32_t boot_cpuid_phys);
 void sort_tree(struct boot_info *bi);
+void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
+void generate_fixups_tree(struct node *dt);
 
 /* Checks */
 
diff --git a/flattree.c b/flattree.c
index ec14954..5dde832 100644
--- a/flattree.c
+++ b/flattree.c
@@ -929,5 +929,5 @@ struct boot_info *dt_from_blob(const char *fname)
 
 	fclose(f);
 
-	return build_boot_info(reservelist, tree, boot_cpuid_phys);
+	return build_boot_info(VF_DT_V1, reservelist, tree, boot_cpuid_phys);
 }
diff --git a/fstree.c b/fstree.c
index 6d1beec..54f520b 100644
--- a/fstree.c
+++ b/fstree.c
@@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
 	tree = read_fstree(dirname);
 	tree = name_node(tree, "");
 
-	return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
+	return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
 }
 
diff --git a/livetree.c b/livetree.c
index e229b84..e7d0fe5 100644
--- a/livetree.c
+++ b/livetree.c
@@ -18,6 +18,7 @@
  *                                                                   USA
  */
 
+#define _GNU_SOURCE
 #include "dtc.h"
 
 /*
@@ -216,6 +217,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 	return old_node;
 }
 
+void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
+{
+	static unsigned int next_orphan_fragment = 0;
+	struct node *node = xmalloc(sizeof(*node));
+	struct property *p;
+	struct data d = empty_data;
+	char *name;
+	int ret;
+
+	memset(node, 0, sizeof(*node));
+
+	d = data_add_marker(d, REF_PHANDLE, ref);
+	d = data_append_integer(d, 0xffffffff, 32);
+
+	p = build_property("target", d);
+	add_property(node, p);
+
+	ret = asprintf(&name, "fragment@%u",
+			next_orphan_fragment++);
+	if (ret == -1)
+		die("asprintf() failed\n");
+	name_node(node, name);
+	name_node(new_node, "__overlay__");
+
+	add_child(dt, node);
+	add_child(node, new_node);
+}
+
 struct node *chain_node(struct node *first, struct node *list)
 {
 	assert(first->next_sibling == NULL);
@@ -335,15 +364,19 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
 	return list;
 }
 
-struct boot_info *build_boot_info(struct reserve_info *reservelist,
+struct boot_info *build_boot_info(unsigned int versionflags,
+				  struct reserve_info *reservelist,
 				  struct node *tree, uint32_t boot_cpuid_phys)
 {
 	struct boot_info *bi;
 
 	bi = xmalloc(sizeof(*bi));
+	bi->versionflags = versionflags;
 	bi->reservelist = reservelist;
 	bi->dt = tree;
 	bi->boot_cpuid_phys = boot_cpuid_phys;
+	/* link back */
+	tree->bi = bi;
 
 	return bi;
 }
@@ -709,3 +742,185 @@ void sort_tree(struct boot_info *bi)
 	sort_reserve_entries(bi);
 	sort_node(bi->dt);
 }
+
+/* utility helper to avoid code duplication */
+static struct node *build_and_name_child_node(struct node *parent, char *name)
+{
+	struct node *node;
+
+	node = build_node(NULL, NULL);
+	name_node(node, xstrdup(name));
+	add_child(parent, node);
+
+	return node;
+}
+
+static void generate_label_tree_internal(struct node *dt, struct node *node,
+					 char *gen_node_name, bool allocph)
+{
+	struct node *c, *an;
+	struct property *p;
+	struct label *l;
+
+	/* if if there are labels */
+	if (node->labels) {
+		/* an is the aliases/__symbols__ node */
+		an = get_subnode(dt, gen_node_name);
+		/* if no node exists, create it */
+		if (!an)
+			die("Could not get label node\n");
+
+		/* now add the label in the node */
+		for_each_label(node->labels, l) {
+			/* check whether the label already exists */
+			p = get_property(an, l->label);
+			if (p) {
+				fprintf(stderr, "WARNING: label %s already"
+					" exists in /%s", l->label,
+					gen_node_name);
+				continue;
+			}
+
+			/* insert it */
+			p = build_property(l->label,
+				data_copy_escape_string(node->fullpath,
+						strlen(node->fullpath)));
+			add_property(an, p);
+		}
+
+		/* force allocation of a phandle for this node */
+		if (allocph)
+			(void)get_node_phandle(dt, node);
+	}
+
+	for_each_child(node, c)
+		generate_label_tree_internal(dt, c, gen_node_name, allocph);
+}
+
+void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
+{
+	build_and_name_child_node(dt, gen_node_name);
+	generate_label_tree_internal(dt, dt, gen_node_name, allocph);
+}
+
+static char *fixups_name = "__fixups__";
+static char *local_fixups_name = "__local_fixups__";
+
+static void add_fixup_entry(struct node *dt, struct node *node,
+		struct property *prop, struct marker *m)
+{
+	struct node *fn;	/* local fixup node */
+	struct property *p;
+	struct data d;
+	char *entry;
+	int ret;
+
+	/* m->ref can only be a REF_PHANDLE, but check anyway */
+	if (m->type != REF_PHANDLE)
+		die("Fixup entry can only be a ref to a phandle\n");
+
+	/* fn is the node we're putting entries in */
+	fn = get_subnode(dt, fixups_name);
+	if (!fn)
+		die("Can't get fixups node\n");
+
+	/* there shouldn't be any ':' in the arguments */
+	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
+		die("arguments should not contain ':'\n");
+
+	ret = asprintf(&entry, "%s:%s:%u",
+			node->fullpath, prop->name, m->offset);
+	if (ret == -1)
+		die("asprintf() failed\n");
+
+	p = get_property(fn, m->ref);
+	if (p) {
+		d = data_append_data(p->val, entry, strlen(entry) + 1);
+		p->val = d;
+	} else {
+		d = data_append_data(empty_data, entry, strlen(entry) + 1);
+		add_property(fn, build_property(m->ref, d));
+	}
+}
+
+static void add_local_fixup_entry(struct node *dt, struct node *node,
+		struct property *prop, struct marker *m,
+		struct node *refnode)
+{
+	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
+	struct property *p;
+	struct data d;
+	char *s, *e, *comp;
+	int len;
+
+	/* fn is the node we're putting entries in */
+	lfn = get_subnode(dt, local_fixups_name);
+	if (!lfn)
+		die("Can't get local fixups node\n");
+
+	/* walk the path components creating nodes if they don't exist */
+	comp = xmalloc(strlen(node->fullpath) + 1);
+	/* start skipping the first / */
+	s = node->fullpath + 1;
+	wn = lfn;
+	while (*s) {
+		/* retrieve path component */
+		e = strchr(s, '/');
+		if (e == NULL)
+			e = s + strlen(s);
+		len = e - s;
+		memcpy(comp, s, len);
+		comp[len] = '\0';
+
+		/* if no node exists, create it */
+		nwn = get_subnode(wn, comp);
+		if (!nwn)
+			nwn = build_and_name_child_node(wn, comp);
+		wn = nwn;
+
+		/* last path component */
+		if (!*e)
+			break;
+
+		/* next path component */
+		s = e + 1;
+	}
+	free(comp);
+
+	p = get_property(wn, prop->name);
+	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
+	if (!p)
+		add_property(wn, build_property(prop->name, d));
+	else
+		p->val = d;
+}
+
+static void generate_fixups_tree_internal(struct node *dt, struct node *node)
+{
+	struct node *c;
+	struct property *prop;
+	struct marker *m;
+	struct node *refnode;
+
+	for_each_property(node, prop) {
+		m = prop->val.markers;
+		for_each_marker_of_type(m, REF_PHANDLE) {
+			refnode = get_node_by_ref(dt, m->ref);
+			if (!refnode)
+				add_fixup_entry(dt, node, prop, m);
+			else
+				add_local_fixup_entry(dt, node, prop, m,
+						refnode);
+		}
+	}
+
+	for_each_child(node, c)
+		generate_fixups_tree_internal(dt, c);
+}
+
+void generate_fixups_tree(struct node *dt)
+{
+	build_and_name_child_node(dt, fixups_name);
+	build_and_name_child_node(dt, local_fixups_name);
+	generate_fixups_tree_internal(dt, dt);
+}
-- 
1.7.12

Re: [PATCH v6 2/4] dtc: Plugin and fixup support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 20587 bytes --]

On Thu, May 05, 2016 at 10:48:42PM +0300, Pantelis Antoniou wrote:
> This patch enable the generation of symbols & local fixup information
> for trees compiled with the -@ (--symbols) option.
> 
> Using this patch labels in the tree and their users emit information
> in __symbols__ and __local_fixups__ nodes.
> 
> The __fixups__ node make possible the dynamic resolution of phandle
> references which are present in the plugin tree but lie in the
> tree that are applying the overlay against.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  Documentation/manual.txt |  16 ++++
>  checks.c                 |   8 +-
>  dtc-lexer.l              |   5 ++
>  dtc-parser.y             |  42 +++++++--
>  dtc.c                    |  23 ++++-
>  dtc.h                    |  28 +++++-
>  flattree.c               |   2 +-
>  fstree.c                 |   2 +-
>  livetree.c               | 217 ++++++++++++++++++++++++++++++++++++++++++++++-
>  9 files changed, 329 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 398de32..3e16c24 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -119,6 +119,20 @@ Options:
>  	Make space for <number> reserve map entries
>  	Relevant for dtb and asm output only.
>  
> +    -@
> +	Generates a __symbols__ node at the root node of the resulting blob
> +	for any node labels used, and for any local references using phandles
> +	it also generates a __local_fixups__ node that tracks them.
> +
> +	When using the /plugin/ tag all unresolved label references to
> +	be tracked in the __fixups__ node, making dynamic resolution possible.
> +
> +    -A
> +	Generate automatically aliases for all node labels. This is similar to
> +	the -@ option (the __symbols__ node contain identical information) but
> +	the semantics are slightly different since no phandles are automatically
> +	generated for labeled nodes.
> +
>      -S <bytes>
>  	Ensure the blob at least <bytes> long, adding additional
>  	space if needed.
> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
>  
>      devicetree:   '/' nodedef
>  
> +    plugindecl:   '/' 'plugin' '/' ';'

You added the new non-terminal definitiom, but didn't use it anywhere.

>      nodedef:      '{' list_of_property list_of_subnode '}' ';'
>  
>      property:     label PROPNAME '=' propdata ';'
> diff --git a/checks.c b/checks.c
> index 386f956..3d4c3c6 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -490,8 +490,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>  
>  		refnode = get_node_by_ref(dt, m->ref);
>  		if (! refnode) {
> -			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> -			     m->ref);
> +			if (!(tree_get_versionflags(dt) & VF_PLUGIN))
> +				FAIL(c, "Reference to non-existent node or "
> +						"label \"%s\"\n", m->ref);
> +			else /* mark the entry as unresolved */
> +				*((cell_t *)(prop->val.val + m->offset)) =
> +					cpu_to_fdt32(0xffffffff);
>  			continue;
>  		}
>  
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 790fbf6..40bbc87 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
>  			return DT_V1;
>  		}
>  
> +<*>"/plugin/"	{
> +			DPRINT("Keyword: /plugin/\n");
> +			return DT_PLUGIN;
> +		}
> +
>  <*>"/memreserve/"	{
>  			DPRINT("Keyword: /memreserve/\n");
>  			BEGIN_DEFAULT();
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 000873f..2d3399e 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -19,6 +19,7 @@
>   */
>  %{
>  #include <stdio.h>
> +#include <inttypes.h>
>  
>  #include "dtc.h"
>  #include "srcpos.h"
> @@ -52,9 +53,11 @@ extern bool treesource_error;
>  	struct node *nodelist;
>  	struct reserve_info *re;
>  	uint64_t integer;
> +	unsigned int flags;
>  }
>  
>  %token DT_V1
> +%token DT_PLUGIN
>  %token DT_MEMRESERVE
>  %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>  %token DT_BITS
> @@ -71,6 +74,8 @@ extern bool treesource_error;
>  
>  %type <data> propdata
>  %type <data> propdataprefix
> +%type <flags> versioninfo
> +%type <flags> plugindecl
>  %type <re> memreserve
>  %type <re> memreserves
>  %type <array> arrayprefix
> @@ -101,13 +106,31 @@ extern bool treesource_error;
>  %%
>  
>  sourcefile:
> -	  DT_V1 ';' memreserves devicetree
> +	  versioninfo ';' memreserves devicetree
>  		{
> -			the_boot_info = build_boot_info($3, $4,
> +			the_boot_info = build_boot_info($1, $3, $4,
>  							guess_boot_cpuid($4));
>  		}
>  	;
>  
> +versioninfo:
> +	DT_V1 plugindecl
> +		{
> +			$$ = VF_DT_V1 | $2;
> +		}
> +	;
> +
> +plugindecl:
> +	DT_PLUGIN
> +		{
> +			$$ = VF_PLUGIN;
> +		}
> +	| /* empty */
> +		{
> +			$$ = 0;
> +		}
> +	;
> +
>  memreserves:
>  	  /* empty */
>  		{
> @@ -156,10 +179,14 @@ devicetree:
>  		{
>  			struct node *target = get_node_by_ref($1, $2);
>  
> -			if (target)
> +			if (target) {
>  				merge_nodes(target, $3);
> -			else
> -				ERROR(&@2, "Label or path %s not found", $2);
> +			} else {
> +				if (symbol_fixup_support)

Surely this should depend on the /plugin/ tag, rather than the -@ option..?

> +					add_orphan_node($1, $3, $2);
> +				else
> +					ERROR(&@2, "Label or path %s not found", $2);
> +			}
>  			$$ = $1;
>  		}
>  	| devicetree DT_DEL_NODE DT_REF ';'
> @@ -174,6 +201,11 @@ devicetree:
>  
>  			$$ = $1;
>  		}
> +	| /* empty */
> +		{
> +			/* build empty node */
> +			$$ = name_node(build_node(NULL, NULL), "");
> +		}
>  	;
>  
>  nodedef:
> diff --git a/dtc.c b/dtc.c
> index 5fa23c4..f8277fb 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
>  int minsize;		/* Minimum blob size */
>  int padsize;		/* Additional padding to blob */
>  int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
> +int symbol_fixup_support;
> +int auto_label_aliases;
>  
>  static void fill_fullpaths(struct node *tree, const char *prefix)
>  {
> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>  #define FDT_VERSION(version)	_FDT_VERSION(version)
>  #define _FDT_VERSION(version)	#version
>  static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
>  static struct option const usage_long_opts[] = {
>  	{"quiet",            no_argument, NULL, 'q'},
>  	{"in-format",         a_argument, NULL, 'I'},
> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
>  	{"phandle",           a_argument, NULL, 'H'},
>  	{"warning",           a_argument, NULL, 'W'},
>  	{"error",             a_argument, NULL, 'E'},
> +	{"symbols",	     no_argument, NULL, '@'},
> +	{"auto-alias",       no_argument, NULL, 'A'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
>  	{NULL,               no_argument, NULL, 0x0},
> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
>  	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
>  	"\n\tEnable/disable warnings (prefix with \"no-\")",
>  	"\n\tEnable/disable errors (prefix with \"no-\")",
> +	"\n\tEnable symbols/fixup support",
> +	"\n\tEnable auto-alias of labels",
>  	"\n\tPrint this help and exit",
>  	"\n\tPrint version and exit",
>  	NULL,
> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
>  		case 'E':
>  			parse_checks_option(false, true, optarg);
>  			break;
> -
> +		case '@':
> +			symbol_fixup_support = 1;
> +			break;
> +		case 'A':
> +			auto_label_aliases = 1;
> +			break;
>  		case 'h':
>  			usage(NULL);
>  		default:
> @@ -294,6 +305,14 @@ int main(int argc, char *argv[])
>  	if (sort)
>  		sort_tree(bi);
>  
> +	if (auto_label_aliases)
> +		generate_label_tree(bi->dt, "aliases", false);
> +
> +	if (symbol_fixup_support) {
> +		generate_label_tree(bi->dt, "__symbols__", true);
> +		generate_fixups_tree(bi->dt);
> +	}
> +

These should go before the sort_tree() - since they add nodes, they
could result in something that *isn't* sorted when that was requested.

>  	if (streq(outname, "-")) {
>  		outf = stdout;
>  	} else {
> diff --git a/dtc.h b/dtc.h
> index 56212c8..68cb109 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -20,7 +20,6 @@
>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>   *                                                                   USA
>   */
> -

Unrelated whitespace change.

>  #include <stdio.h>
>  #include <string.h>
>  #include <stdlib.h>
> @@ -54,6 +53,12 @@ extern int reservenum;		/* Number of memory reservation slots */
>  extern int minsize;		/* Minimum blob size */
>  extern int padsize;		/* Additional padding to blob */
>  extern int phandle_format;	/* Use linux,phandle or phandle properties */
> +extern int symbol_fixup_support;/* enable symbols & fixup support */
> +extern int auto_label_aliases;	/* auto generate labels -> aliases */
> +
> +/*
> + * Tree source globals
> + */
>  
>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> @@ -158,6 +163,9 @@ struct node {
>  	int addr_cells, size_cells;
>  
>  	struct label *labels;
> +
> +	/* only for the root (parent == NULL) */
> +	struct boot_info *bi;

Ugh.. I hate creating circular references unless we really can't avoid
them.

>  };
>  
>  #define for_each_label_withdel(l0, l) \
> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
>  struct node *name_node(struct node *node, char *name);
>  struct node *chain_node(struct node *first, struct node *list);
>  struct node *merge_nodes(struct node *old_node, struct node *new_node);
> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>  
>  void add_property(struct node *node, struct property *prop);
>  void delete_property_by_name(struct node *node, char *name);
> @@ -236,14 +245,29 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>  
>  
>  struct boot_info {
> +	unsigned int versionflags;
>  	struct reserve_info *reservelist;
>  	struct node *dt;		/* the device tree */
>  	uint32_t boot_cpuid_phys;
>  };
>  
> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> +/* version flags definitions */
> +#define VF_DT_V1	0x0001	/* /dts-v1/ */

Is there any point to this, since dtc doesn't support v0 dts files any
more anyway?

> +#define VF_PLUGIN	0x0002	/* /plugin/ */
> +
> +static inline unsigned int tree_get_versionflags(struct node *dt)
> +{
> +	if (!dt || !dt->bi)
> +		return 0;
> +	return dt->bi->versionflags;
> +}
> +
> +struct boot_info *build_boot_info(unsigned int versionflags,
> +				  struct reserve_info *reservelist,
>  				  struct node *tree, uint32_t boot_cpuid_phys);
>  void sort_tree(struct boot_info *bi);
> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
> +void generate_fixups_tree(struct node *dt);
>  
>  /* Checks */
>  
> diff --git a/flattree.c b/flattree.c
> index ec14954..5dde832 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -929,5 +929,5 @@ struct boot_info *dt_from_blob(const char *fname)
>  
>  	fclose(f);
>  
> -	return build_boot_info(reservelist, tree, boot_cpuid_phys);
> +	return build_boot_info(VF_DT_V1, reservelist, tree, boot_cpuid_phys);
>  }
> diff --git a/fstree.c b/fstree.c
> index 6d1beec..54f520b 100644
> --- a/fstree.c
> +++ b/fstree.c
> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
>  	tree = read_fstree(dirname);
>  	tree = name_node(tree, "");
>  
> -	return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
> +	return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
>  }
>  
> diff --git a/livetree.c b/livetree.c
> index e229b84..e7d0fe5 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -18,6 +18,7 @@
>   *                                                                   USA
>   */
>  
> +#define _GNU_SOURCE

Ugh.  I mean, I know the work I did to get dtc working on BSD has
bitrotted already, but can we avoid gratuitously introducing
non-portability.

>  #include "dtc.h"
>  
>  /*
> @@ -216,6 +217,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  	return old_node;
>  }
>  
> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> +{
> +	static unsigned int next_orphan_fragment = 0;
> +	struct node *node = xmalloc(sizeof(*node));
> +	struct property *p;
> +	struct data d = empty_data;
> +	char *name;
> +	int ret;
> +
> +	memset(node, 0, sizeof(*node));
> +
> +	d = data_add_marker(d, REF_PHANDLE, ref);
> +	d = data_append_integer(d, 0xffffffff, 32);
> +
> +	p = build_property("target", d);
> +	add_property(node, p);
> +
> +	ret = asprintf(&name, "fragment@%u",
> +			next_orphan_fragment++);
> +	if (ret == -1)
> +		die("asprintf() failed\n");
> +	name_node(node, name);
> +	name_node(new_node, "__overlay__");
> +
> +	add_child(dt, node);
> +	add_child(node, new_node);
> +}




>  struct node *chain_node(struct node *first, struct node *list)
>  {
>  	assert(first->next_sibling == NULL);
> @@ -335,15 +364,19 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>  	return list;
>  }
>  
> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> +struct boot_info *build_boot_info(unsigned int versionflags,
> +				  struct reserve_info *reservelist,
>  				  struct node *tree, uint32_t boot_cpuid_phys)
>  {
>  	struct boot_info *bi;
>  
>  	bi = xmalloc(sizeof(*bi));
> +	bi->versionflags = versionflags;
>  	bi->reservelist = reservelist;
>  	bi->dt = tree;
>  	bi->boot_cpuid_phys = boot_cpuid_phys;
> +	/* link back */
> +	tree->bi = bi;
>  
>  	return bi;
>  }
> @@ -709,3 +742,185 @@ void sort_tree(struct boot_info *bi)
>  	sort_reserve_entries(bi);
>  	sort_node(bi->dt);
>  }
> +
> +/* utility helper to avoid code duplication */
> +static struct node *build_and_name_child_node(struct node *parent, char *name)
> +{
> +	struct node *node;
> +
> +	node = build_node(NULL, NULL);
> +	name_node(node, xstrdup(name));
> +	add_child(parent, node);
> +
> +	return node;
> +}
> +
> +static void generate_label_tree_internal(struct node *dt, struct node *node,
> +					 char *gen_node_name, bool allocph)
> +{
> +	struct node *c, *an;
> +	struct property *p;
> +	struct label *l;
> +
> +	/* if if there are labels */
> +	if (node->labels) {
> +		/* an is the aliases/__symbols__ node */
> +		an = get_subnode(dt, gen_node_name);

You can factor this out to the caller and just pass 'an' in.

> +		/* if no node exists, create it */

Comment does not match the code

> +		if (!an)
> +			die("Could not get label node\n");
> +
> +		/* now add the label in the node */
> +		for_each_label(node->labels, l) {
> +			/* check whether the label already exists */
> +			p = get_property(an, l->label);
> +			if (p) {
> +				fprintf(stderr, "WARNING: label %s already"
> +					" exists in /%s", l->label,
> +					gen_node_name);
> +				continue;
> +			}
> +
> +			/* insert it */
> +			p = build_property(l->label,
> +				data_copy_escape_string(node->fullpath,
> +						strlen(node->fullpath)));
> +			add_property(an, p);
> +		}
> +
> +		/* force allocation of a phandle for this node */
> +		if (allocph)
> +			(void)get_node_phandle(dt, node);
> +	}
> +
> +	for_each_child(node, c)
> +		generate_label_tree_internal(dt, c, gen_node_name, allocph);
> +}
> +
> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
> +{
> +	build_and_name_child_node(dt, gen_node_name);
> +	generate_label_tree_internal(dt, dt, gen_node_name, allocph);
> +}
> +
> +static char *fixups_name = "__fixups__";
> +static char *local_fixups_name = "__local_fixups__";
> +
> +static void add_fixup_entry(struct node *dt, struct node *node,
> +		struct property *prop, struct marker *m)
> +{
> +	struct node *fn;	/* local fixup node */

Comment is misleading since this is __fixups__ rather than
__local_fixups__.

> +	struct property *p;
> +	struct data d;
> +	char *entry;
> +	int ret;
> +
> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
> +	if (m->type != REF_PHANDLE)
> +		die("Fixup entry can only be a ref to a phandle\n");

If it's not a REF_PHANDLE, that indicates a bug elsewhere, yes?  In
which case assert() would be more appropriate.

> +	/* fn is the node we're putting entries in */
> +	fn = get_subnode(dt, fixups_name);
> +	if (!fn)
> +		die("Can't get fixups node\n");

Again, assert() would be better.

> +	/* there shouldn't be any ':' in the arguments */
> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> +		die("arguments should not contain ':'\n");
> +
> +	ret = asprintf(&entry, "%s:%s:%u",
> +			node->fullpath, prop->name, m->offset);
> +	if (ret == -1)
> +		die("asprintf() failed\n");

Looks like we really should put in an xasprintf() utility function,
both to address the BSD portability, and avoid clunky checks for
allocation failures.

> +	p = get_property(fn, m->ref);
> +	if (p) {
> +		d = data_append_data(p->val, entry, strlen(entry) + 1);
> +		p->val = d;
> +	} else {
> +		d = data_append_data(empty_data, entry, strlen(entry) + 1);
> +		add_property(fn, build_property(m->ref, d));
> +	}
> +}
> +
> +static void add_local_fixup_entry(struct node *dt, struct node *node,
> +		struct property *prop, struct marker *m,
> +		struct node *refnode)
> +{
> +	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
> +	struct property *p;
> +	struct data d;
> +	char *s, *e, *comp;
> +	int len;
> +
> +	/* fn is the node we're putting entries in */
> +	lfn = get_subnode(dt, local_fixups_name);
> +	if (!lfn)
> +		die("Can't get local fixups node\n");

Again, assert().

> +	/* walk the path components creating nodes if they don't exist */
> +	comp = xmalloc(strlen(node->fullpath) + 1);
> +	/* start skipping the first / */
> +	s = node->fullpath + 1;
> +	wn = lfn;
> +	while (*s) {

Given the break; below is the loop condition redundant?

> +		/* retrieve path component */
> +		e = strchr(s, '/');
> +		if (e == NULL)
> +			e = s + strlen(s);
> +		len = e - s;
> +		memcpy(comp, s, len);
> +		comp[len] = '\0';
> +
> +		/* if no node exists, create it */
> +		nwn = get_subnode(wn, comp);
> +		if (!nwn)
> +			nwn = build_and_name_child_node(wn, comp);
> +		wn = nwn;
> +
> +		/* last path component */
> +		if (!*e)
> +			break;
> +
> +		/* next path component */
> +		s = e + 1;
> +	}
> +	free(comp);
> +
> +	p = get_property(wn, prop->name);
> +	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
> +	if (!p)
> +		add_property(wn, build_property(prop->name, d));
> +	else
> +		p->val = d;

Hmm.. am append_to_property helper would simplify both this and the
other fixup function.

> +}
> +
> +static void generate_fixups_tree_internal(struct node *dt, struct node *node)
> +{
> +	struct node *c;
> +	struct property *prop;
> +	struct marker *m;
> +	struct node *refnode;
> +
> +	for_each_property(node, prop) {
> +		m = prop->val.markers;
> +		for_each_marker_of_type(m, REF_PHANDLE) {
> +			refnode = get_node_by_ref(dt, m->ref);
> +			if (!refnode)
> +				add_fixup_entry(dt, node, prop, m);
> +			else
> +				add_local_fixup_entry(dt, node, prop, m,
> +						refnode);
> +		}
> +	}
> +
> +	for_each_child(node, c)
> +		generate_fixups_tree_internal(dt, c);
> +}
> +
> +void generate_fixups_tree(struct node *dt)
> +{
> +	build_and_name_child_node(dt, fixups_name);
> +	build_and_name_child_node(dt, local_fixups_name);
> +	generate_fixups_tree_internal(dt, dt);
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 2/4] dtc: Plugin and fixup support

[Pantelis Antoniou]

Hi David,

> On May 24, 2016, at 13:39 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Thu, May 05, 2016 at 10:48:42PM +0300, Pantelis Antoniou wrote:
>> This patch enable the generation of symbols & local fixup information
>> for trees compiled with the -@ (--symbols) option.
>> 
>> Using this patch labels in the tree and their users emit information
>> in __symbols__ and __local_fixups__ nodes.
>> 
>> The __fixups__ node make possible the dynamic resolution of phandle
>> references which are present in the plugin tree but lie in the
>> tree that are applying the overlay against.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> ---
>> Documentation/manual.txt |  16 ++++
>> checks.c                 |   8 +-
>> dtc-lexer.l              |   5 ++
>> dtc-parser.y             |  42 +++++++--
>> dtc.c                    |  23 ++++-
>> dtc.h                    |  28 +++++-
>> flattree.c               |   2 +-
>> fstree.c                 |   2 +-
>> livetree.c               | 217 ++++++++++++++++++++++++++++++++++++++++++++++-
>> 9 files changed, 329 insertions(+), 14 deletions(-)
>> 
>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>> index 398de32..3e16c24 100644
>> --- a/Documentation/manual.txt
>> +++ b/Documentation/manual.txt
>> @@ -119,6 +119,20 @@ Options:
>> 	Make space for <number> reserve map entries
>> 	Relevant for dtb and asm output only.
>> 
>> +    -@
>> +	Generates a __symbols__ node at the root node of the resulting blob
>> +	for any node labels used, and for any local references using phandles
>> +	it also generates a __local_fixups__ node that tracks them.
>> +
>> +	When using the /plugin/ tag all unresolved label references to
>> +	be tracked in the __fixups__ node, making dynamic resolution possible.
>> +
>> +    -A
>> +	Generate automatically aliases for all node labels. This is similar to
>> +	the -@ option (the __symbols__ node contain identical information) but
>> +	the semantics are slightly different since no phandles are automatically
>> +	generated for labeled nodes.
>> +
>>     -S <bytes>
>> 	Ensure the blob at least <bytes> long, adding additional
>> 	space if needed.
>> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
>> 
>>     devicetree:   '/' nodedef
>> 
>> +    plugindecl:   '/' 'plugin' '/' ';'
> 
> You added the new non-terminal definitiom, but didn't use it anywhere.
> 

plugindecl is used by the referenced in versioninfo so it’s used implicitly.

>>     nodedef:      '{' list_of_property list_of_subnode '}' ';'
>> 
>>     property:     label PROPNAME '=' propdata ';'
>> diff --git a/checks.c b/checks.c
>> index 386f956..3d4c3c6 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -490,8 +490,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>> 
>> 		refnode = get_node_by_ref(dt, m->ref);
>> 		if (! refnode) {
>> -			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
>> -			     m->ref);
>> +			if (!(tree_get_versionflags(dt) & VF_PLUGIN))
>> +				FAIL(c, "Reference to non-existent node or "
>> +						"label \"%s\"\n", m->ref);
>> +			else /* mark the entry as unresolved */
>> +				*((cell_t *)(prop->val.val + m->offset)) =
>> +					cpu_to_fdt32(0xffffffff);
>> 			continue;
>> 		}
>> 
>> diff --git a/dtc-lexer.l b/dtc-lexer.l
>> index 790fbf6..40bbc87 100644
>> --- a/dtc-lexer.l
>> +++ b/dtc-lexer.l
>> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
>> 			return DT_V1;
>> 		}
>> 
>> +<*>"/plugin/"	{
>> +			DPRINT("Keyword: /plugin/\n");
>> +			return DT_PLUGIN;
>> +		}
>> +
>> <*>"/memreserve/"	{
>> 			DPRINT("Keyword: /memreserve/\n");
>> 			BEGIN_DEFAULT();
>> diff --git a/dtc-parser.y b/dtc-parser.y
>> index 000873f..2d3399e 100644
>> --- a/dtc-parser.y
>> +++ b/dtc-parser.y
>> @@ -19,6 +19,7 @@
>>  */
>> %{
>> #include <stdio.h>
>> +#include <inttypes.h>
>> 
>> #include "dtc.h"
>> #include "srcpos.h"
>> @@ -52,9 +53,11 @@ extern bool treesource_error;
>> 	struct node *nodelist;
>> 	struct reserve_info *re;
>> 	uint64_t integer;
>> +	unsigned int flags;
>> }
>> 
>> %token DT_V1
>> +%token DT_PLUGIN
>> %token DT_MEMRESERVE
>> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
>> %token DT_BITS
>> @@ -71,6 +74,8 @@ extern bool treesource_error;
>> 
>> %type <data> propdata
>> %type <data> propdataprefix
>> +%type <flags> versioninfo
>> +%type <flags> plugindecl
>> %type <re> memreserve
>> %type <re> memreserves
>> %type <array> arrayprefix
>> @@ -101,13 +106,31 @@ extern bool treesource_error;
>> %%
>> 
>> sourcefile:
>> -	  DT_V1 ';' memreserves devicetree
>> +	  versioninfo ';' memreserves devicetree
>> 		{
>> -			the_boot_info = build_boot_info($3, $4,
>> +			the_boot_info = build_boot_info($1, $3, $4,
>> 							guess_boot_cpuid($4));
>> 		}
>> 	;
>> 
>> +versioninfo:
>> +	DT_V1 plugindecl
>> +		{
>> +			$$ = VF_DT_V1 | $2;
>> +		}
>> +	;
>> +
>> +plugindecl:
>> +	DT_PLUGIN
>> +		{
>> +			$$ = VF_PLUGIN;
>> +		}
>> +	| /* empty */
>> +		{
>> +			$$ = 0;
>> +		}
>> +	;
>> +
>> memreserves:
>> 	  /* empty */
>> 		{
>> @@ -156,10 +179,14 @@ devicetree:
>> 		{
>> 			struct node *target = get_node_by_ref($1, $2);
>> 
>> -			if (target)
>> +			if (target) {
>> 				merge_nodes(target, $3);
>> -			else
>> -				ERROR(&@2, "Label or path %s not found", $2);
>> +			} else {
>> +				if (symbol_fixup_support)
> 
> Surely this should depend on the /plugin/ tag, rather than the -@ option..?
> 

the_boot_info is not yet initialized at this point. The only way to access it would
be to have version flags as a global. Is that OK?


>> +					add_orphan_node($1, $3, $2);
>> +				else
>> +					ERROR(&@2, "Label or path %s not found", $2);
>> +			}
>> 			$$ = $1;
>> 		}
>> 	| devicetree DT_DEL_NODE DT_REF ';'
>> @@ -174,6 +201,11 @@ devicetree:
>> 
>> 			$$ = $1;
>> 		}
>> +	| /* empty */
>> +		{
>> +			/* build empty node */
>> +			$$ = name_node(build_node(NULL, NULL), "");
>> +		}
>> 	;
>> 
>> nodedef:
>> diff --git a/dtc.c b/dtc.c
>> index 5fa23c4..f8277fb 100644
>> --- a/dtc.c
>> +++ b/dtc.c
>> @@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
>> int minsize;		/* Minimum blob size */
>> int padsize;		/* Additional padding to blob */
>> int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
>> +int symbol_fixup_support;
>> +int auto_label_aliases;
>> 
>> static void fill_fullpaths(struct node *tree, const char *prefix)
>> {
>> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>> #define FDT_VERSION(version)	_FDT_VERSION(version)
>> #define _FDT_VERSION(version)	#version
>> static const char usage_synopsis[] = "dtc [options] <input file>";
>> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
>> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
>> static struct option const usage_long_opts[] = {
>> 	{"quiet",            no_argument, NULL, 'q'},
>> 	{"in-format",         a_argument, NULL, 'I'},
>> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
>> 	{"phandle",           a_argument, NULL, 'H'},
>> 	{"warning",           a_argument, NULL, 'W'},
>> 	{"error",             a_argument, NULL, 'E'},
>> +	{"symbols",	     no_argument, NULL, '@'},
>> +	{"auto-alias",       no_argument, NULL, 'A'},
>> 	{"help",             no_argument, NULL, 'h'},
>> 	{"version",          no_argument, NULL, 'v'},
>> 	{NULL,               no_argument, NULL, 0x0},
>> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
>> 	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
>> 	"\n\tEnable/disable warnings (prefix with \"no-\")",
>> 	"\n\tEnable/disable errors (prefix with \"no-\")",
>> +	"\n\tEnable symbols/fixup support",
>> +	"\n\tEnable auto-alias of labels",
>> 	"\n\tPrint this help and exit",
>> 	"\n\tPrint version and exit",
>> 	NULL,
>> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
>> 		case 'E':
>> 			parse_checks_option(false, true, optarg);
>> 			break;
>> -
>> +		case '@':
>> +			symbol_fixup_support = 1;
>> +			break;
>> +		case 'A':
>> +			auto_label_aliases = 1;
>> +			break;
>> 		case 'h':
>> 			usage(NULL);
>> 		default:
>> @@ -294,6 +305,14 @@ int main(int argc, char *argv[])
>> 	if (sort)
>> 		sort_tree(bi);
>> 
>> +	if (auto_label_aliases)
>> +		generate_label_tree(bi->dt, "aliases", false);
>> +
>> +	if (symbol_fixup_support) {
>> +		generate_label_tree(bi->dt, "__symbols__", true);
>> +		generate_fixups_tree(bi->dt);
>> +	}
>> +
> 
> These should go before the sort_tree() - since they add nodes, they
> could result in something that *isn't* sorted when that was requested.
> 

OK

>> 	if (streq(outname, "-")) {
>> 		outf = stdout;
>> 	} else {
>> diff --git a/dtc.h b/dtc.h
>> index 56212c8..68cb109 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -20,7 +20,6 @@
>>  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>>  *                                                                   USA
>>  */
>> -
> 
> Unrelated whitespace change.
> 

OK

>> #include <stdio.h>
>> #include <string.h>
>> #include <stdlib.h>
>> @@ -54,6 +53,12 @@ extern int reservenum;		/* Number of memory reservation slots */
>> extern int minsize;		/* Minimum blob size */
>> extern int padsize;		/* Additional padding to blob */
>> extern int phandle_format;	/* Use linux,phandle or phandle properties */
>> +extern int symbol_fixup_support;/* enable symbols & fixup support */
>> +extern int auto_label_aliases;	/* auto generate labels -> aliases */
>> +
>> +/*
>> + * Tree source globals
>> + */
>> 
>> #define PHANDLE_LEGACY	0x1
>> #define PHANDLE_EPAPR	0x2
>> @@ -158,6 +163,9 @@ struct node {
>> 	int addr_cells, size_cells;
>> 
>> 	struct label *labels;
>> +
>> +	/* only for the root (parent == NULL) */
>> +	struct boot_info *bi;
> 
> Ugh.. I hate creating circular references unless we really can't avoid
> them.
> 

Either it’s here, or we pass boot_info to every method.
Need access to the version flags.

>> };
>> 
>> #define for_each_label_withdel(l0, l) \
>> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
>> struct node *name_node(struct node *node, char *name);
>> struct node *chain_node(struct node *first, struct node *list);
>> struct node *merge_nodes(struct node *old_node, struct node *new_node);
>> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>> 
>> void add_property(struct node *node, struct property *prop);
>> void delete_property_by_name(struct node *node, char *name);
>> @@ -236,14 +245,29 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>> 
>> 
>> struct boot_info {
>> +	unsigned int versionflags;
>> 	struct reserve_info *reservelist;
>> 	struct node *dt;		/* the device tree */
>> 	uint32_t boot_cpuid_phys;
>> };
>> 
>> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
>> +/* version flags definitions */
>> +#define VF_DT_V1	0x0001	/* /dts-v1/ */
> 
> Is there any point to this, since dtc doesn't support v0 dts files any
> more anyway?
> 

Completeness. I can take it out.

>> +#define VF_PLUGIN	0x0002	/* /plugin/ */
>> +
>> +static inline unsigned int tree_get_versionflags(struct node *dt)
>> +{
>> +	if (!dt || !dt->bi)
>> +		return 0;
>> +	return dt->bi->versionflags;
>> +}
>> +
>> +struct boot_info *build_boot_info(unsigned int versionflags,
>> +				  struct reserve_info *reservelist,
>> 				  struct node *tree, uint32_t boot_cpuid_phys);
>> void sort_tree(struct boot_info *bi);
>> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
>> +void generate_fixups_tree(struct node *dt);
>> 
>> /* Checks */
>> 
>> diff --git a/flattree.c b/flattree.c
>> index ec14954..5dde832 100644
>> --- a/flattree.c
>> +++ b/flattree.c
>> @@ -929,5 +929,5 @@ struct boot_info *dt_from_blob(const char *fname)
>> 
>> 	fclose(f);
>> 
>> -	return build_boot_info(reservelist, tree, boot_cpuid_phys);
>> +	return build_boot_info(VF_DT_V1, reservelist, tree, boot_cpuid_phys);
>> }
>> diff --git a/fstree.c b/fstree.c
>> index 6d1beec..54f520b 100644
>> --- a/fstree.c
>> +++ b/fstree.c
>> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
>> 	tree = read_fstree(dirname);
>> 	tree = name_node(tree, "");
>> 
>> -	return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
>> +	return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
>> }
>> 
>> diff --git a/livetree.c b/livetree.c
>> index e229b84..e7d0fe5 100644
>> --- a/livetree.c
>> +++ b/livetree.c
>> @@ -18,6 +18,7 @@
>>  *                                                                   USA
>>  */
>> 
>> +#define _GNU_SOURCE
> 
> Ugh.  I mean, I know the work I did to get dtc working on BSD has
> bitrotted already, but can we avoid gratuitously introducing
> non-portability.
> 

It was your suggestion last time, remember? :)

>> #include "dtc.h"
>> 
>> /*
>> @@ -216,6 +217,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>> 	return old_node;
>> }
>> 
>> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
>> +{
>> +	static unsigned int next_orphan_fragment = 0;
>> +	struct node *node = xmalloc(sizeof(*node));
>> +	struct property *p;
>> +	struct data d = empty_data;
>> +	char *name;
>> +	int ret;
>> +
>> +	memset(node, 0, sizeof(*node));
>> +
>> +	d = data_add_marker(d, REF_PHANDLE, ref);
>> +	d = data_append_integer(d, 0xffffffff, 32);
>> +
>> +	p = build_property("target", d);
>> +	add_property(node, p);
>> +
>> +	ret = asprintf(&name, "fragment@%u",
>> +			next_orphan_fragment++);
>> +	if (ret == -1)
>> +		die("asprintf() failed\n");
>> +	name_node(node, name);
>> +	name_node(new_node, "__overlay__");
>> +
>> +	add_child(dt, node);
>> +	add_child(node, new_node);
>> +}
> 
> 
> 
> 
>> struct node *chain_node(struct node *first, struct node *list)
>> {
>> 	assert(first->next_sibling == NULL);
>> @@ -335,15 +364,19 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>> 	return list;
>> }
>> 
>> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
>> +struct boot_info *build_boot_info(unsigned int versionflags,
>> +				  struct reserve_info *reservelist,
>> 				  struct node *tree, uint32_t boot_cpuid_phys)
>> {
>> 	struct boot_info *bi;
>> 
>> 	bi = xmalloc(sizeof(*bi));
>> +	bi->versionflags = versionflags;
>> 	bi->reservelist = reservelist;
>> 	bi->dt = tree;
>> 	bi->boot_cpuid_phys = boot_cpuid_phys;
>> +	/* link back */
>> +	tree->bi = bi;
>> 
>> 	return bi;
>> }
>> @@ -709,3 +742,185 @@ void sort_tree(struct boot_info *bi)
>> 	sort_reserve_entries(bi);
>> 	sort_node(bi->dt);
>> }
>> +
>> +/* utility helper to avoid code duplication */
>> +static struct node *build_and_name_child_node(struct node *parent, char *name)
>> +{
>> +	struct node *node;
>> +
>> +	node = build_node(NULL, NULL);
>> +	name_node(node, xstrdup(name));
>> +	add_child(parent, node);
>> +
>> +	return node;
>> +}
>> +
>> +static void generate_label_tree_internal(struct node *dt, struct node *node,
>> +					 char *gen_node_name, bool allocph)
>> +{
>> +	struct node *c, *an;
>> +	struct property *p;
>> +	struct label *l;
>> +
>> +	/* if if there are labels */
>> +	if (node->labels) {
>> +		/* an is the aliases/__symbols__ node */
>> +		an = get_subnode(dt, gen_node_name);
> 
> You can factor this out to the caller and just pass 'an' in.
> 

OK

>> +		/* if no node exists, create it */
> 
> Comment does not match the code
> 

Ugh.

>> +		if (!an)
>> +			die("Could not get label node\n");
>> +
>> +		/* now add the label in the node */
>> +		for_each_label(node->labels, l) {
>> +			/* check whether the label already exists */
>> +			p = get_property(an, l->label);
>> +			if (p) {
>> +				fprintf(stderr, "WARNING: label %s already"
>> +					" exists in /%s", l->label,
>> +					gen_node_name);
>> +				continue;
>> +			}
>> +
>> +			/* insert it */
>> +			p = build_property(l->label,
>> +				data_copy_escape_string(node->fullpath,
>> +						strlen(node->fullpath)));
>> +			add_property(an, p);
>> +		}
>> +
>> +		/* force allocation of a phandle for this node */
>> +		if (allocph)
>> +			(void)get_node_phandle(dt, node);
>> +	}
>> +
>> +	for_each_child(node, c)
>> +		generate_label_tree_internal(dt, c, gen_node_name, allocph);
>> +}
>> +
>> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
>> +{
>> +	build_and_name_child_node(dt, gen_node_name);
>> +	generate_label_tree_internal(dt, dt, gen_node_name, allocph);
>> +}
>> +
>> +static char *fixups_name = "__fixups__";
>> +static char *local_fixups_name = "__local_fixups__";
>> +
>> +static void add_fixup_entry(struct node *dt, struct node *node,
>> +		struct property *prop, struct marker *m)
>> +{
>> +	struct node *fn;	/* local fixup node */
> 
> Comment is misleading since this is __fixups__ rather than
> __local_fixups__.
> 

OK

>> +	struct property *p;
>> +	struct data d;
>> +	char *entry;
>> +	int ret;
>> +
>> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
>> +	if (m->type != REF_PHANDLE)
>> +		die("Fixup entry can only be a ref to a phandle\n");
> 
> If it's not a REF_PHANDLE, that indicates a bug elsewhere, yes?  In
> which case assert() would be more appropriate.
> 

OK

>> +	/* fn is the node we're putting entries in */
>> +	fn = get_subnode(dt, fixups_name);
>> +	if (!fn)
>> +		die("Can't get fixups node\n");
> 
> Again, assert() would be better.
> 

OK

>> +	/* there shouldn't be any ':' in the arguments */
>> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
>> +		die("arguments should not contain ':'\n");
>> +
>> +	ret = asprintf(&entry, "%s:%s:%u",
>> +			node->fullpath, prop->name, m->offset);
>> +	if (ret == -1)
>> +		die("asprintf() failed\n");
> 
> Looks like we really should put in an xasprintf() utility function,
> both to address the BSD portability, and avoid clunky checks for
> allocation failures.
> 

No worries, I’ll add it in a separate patch.

>> +	p = get_property(fn, m->ref);
>> +	if (p) {
>> +		d = data_append_data(p->val, entry, strlen(entry) + 1);
>> +		p->val = d;
>> +	} else {
>> +		d = data_append_data(empty_data, entry, strlen(entry) + 1);
>> +		add_property(fn, build_property(m->ref, d));
>> +	}
>> +}
>> +
>> +static void add_local_fixup_entry(struct node *dt, struct node *node,
>> +		struct property *prop, struct marker *m,
>> +		struct node *refnode)
>> +{
>> +	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
>> +	struct property *p;
>> +	struct data d;
>> +	char *s, *e, *comp;
>> +	int len;
>> +
>> +	/* fn is the node we're putting entries in */
>> +	lfn = get_subnode(dt, local_fixups_name);
>> +	if (!lfn)
>> +		die("Can't get local fixups node\n");
> 
> Again, assert().
> 
>> +	/* walk the path components creating nodes if they don't exist */
>> +	comp = xmalloc(strlen(node->fullpath) + 1);
>> +	/* start skipping the first / */
>> +	s = node->fullpath + 1;
>> +	wn = lfn;
>> +	while (*s) {
> 
> Given the break; below is the loop condition redundant?
> 

Sanity check if fullpath is empty. Could assert instead.

>> +		/* retrieve path component */
>> +		e = strchr(s, '/');
>> +		if (e == NULL)
>> +			e = s + strlen(s);
>> +		len = e - s;
>> +		memcpy(comp, s, len);
>> +		comp[len] = '\0';
>> +
>> +		/* if no node exists, create it */
>> +		nwn = get_subnode(wn, comp);
>> +		if (!nwn)
>> +			nwn = build_and_name_child_node(wn, comp);
>> +		wn = nwn;
>> +
>> +		/* last path component */
>> +		if (!*e)
>> +			break;
>> +
>> +		/* next path component */
>> +		s = e + 1;
>> +	}
>> +	free(comp);
>> +
>> +	p = get_property(wn, prop->name);
>> +	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
>> +	if (!p)
>> +		add_property(wn, build_property(prop->name, d));
>> +	else
>> +		p->val = d;
> 
> Hmm.. am append_to_property helper would simplify both this and the
> other fixup function.
> 

OK

>> +}
>> +
>> +static void generate_fixups_tree_internal(struct node *dt, struct node *node)
>> +{
>> +	struct node *c;
>> +	struct property *prop;
>> +	struct marker *m;
>> +	struct node *refnode;
>> +
>> +	for_each_property(node, prop) {
>> +		m = prop->val.markers;
>> +		for_each_marker_of_type(m, REF_PHANDLE) {
>> +			refnode = get_node_by_ref(dt, m->ref);
>> +			if (!refnode)
>> +				add_fixup_entry(dt, node, prop, m);
>> +			else
>> +				add_local_fixup_entry(dt, node, prop, m,
>> +						refnode);
>> +		}
>> +	}
>> +
>> +	for_each_child(node, c)
>> +		generate_fixups_tree_internal(dt, c);
>> +}
>> +
>> +void generate_fixups_tree(struct node *dt)
>> +{
>> +	build_and_name_child_node(dt, fixups_name);
>> +	build_and_name_child_node(dt, local_fixups_name);
>> +	generate_fixups_tree_internal(dt, dt);
>> +}
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [PATCH v6 2/4] dtc: Plugin and fixup support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 24491 bytes --]

On Tue, May 24, 2016 at 05:04:18PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On May 24, 2016, at 13:39 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Thu, May 05, 2016 at 10:48:42PM +0300, Pantelis Antoniou wrote:
> >> This patch enable the generation of symbols & local fixup information
> >> for trees compiled with the -@ (--symbols) option.
> >> 
> >> Using this patch labels in the tree and their users emit information
> >> in __symbols__ and __local_fixups__ nodes.
> >> 
> >> The __fixups__ node make possible the dynamic resolution of phandle
> >> references which are present in the plugin tree but lie in the
> >> tree that are applying the overlay against.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> ---
> >> Documentation/manual.txt |  16 ++++
> >> checks.c                 |   8 +-
> >> dtc-lexer.l              |   5 ++
> >> dtc-parser.y             |  42 +++++++--
> >> dtc.c                    |  23 ++++-
> >> dtc.h                    |  28 +++++-
> >> flattree.c               |   2 +-
> >> fstree.c                 |   2 +-
> >> livetree.c               | 217 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> 9 files changed, 329 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >> index 398de32..3e16c24 100644
> >> --- a/Documentation/manual.txt
> >> +++ b/Documentation/manual.txt
> >> @@ -119,6 +119,20 @@ Options:
> >> 	Make space for <number> reserve map entries
> >> 	Relevant for dtb and asm output only.
> >> 
> >> +    -@
> >> +	Generates a __symbols__ node at the root node of the resulting blob
> >> +	for any node labels used, and for any local references using phandles
> >> +	it also generates a __local_fixups__ node that tracks them.
> >> +
> >> +	When using the /plugin/ tag all unresolved label references to
> >> +	be tracked in the __fixups__ node, making dynamic resolution possible.
> >> +
> >> +    -A
> >> +	Generate automatically aliases for all node labels. This is similar to
> >> +	the -@ option (the __symbols__ node contain identical information) but
> >> +	the semantics are slightly different since no phandles are automatically
> >> +	generated for labeled nodes.
> >> +
> >>     -S <bytes>
> >> 	Ensure the blob at least <bytes> long, adding additional
> >> 	space if needed.
> >> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
> >> 
> >>     devicetree:   '/' nodedef
> >> 
> >> +    plugindecl:   '/' 'plugin' '/' ';'
> > 
> > You added the new non-terminal definitiom, but didn't use it anywhere.
> > 
> 
> plugindecl is used by the referenced in versioninfo so it’s used implicitly.

In the actual bison, yes, but not here in the manual - plugindecl is
never referenced.  Of course, neither is the version tag, so it's
already a bit wrong.

> >>     nodedef:      '{' list_of_property list_of_subnode '}' ';'
> >> 
> >>     property:     label PROPNAME '=' propdata ';'
> >> diff --git a/checks.c b/checks.c
> >> index 386f956..3d4c3c6 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -490,8 +490,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
> >> 
> >> 		refnode = get_node_by_ref(dt, m->ref);
> >> 		if (! refnode) {
> >> -			FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> >> -			     m->ref);
> >> +			if (!(tree_get_versionflags(dt) & VF_PLUGIN))
> >> +				FAIL(c, "Reference to non-existent node or "
> >> +						"label \"%s\"\n", m->ref);
> >> +			else /* mark the entry as unresolved */
> >> +				*((cell_t *)(prop->val.val + m->offset)) =
> >> +					cpu_to_fdt32(0xffffffff);
> >> 			continue;
> >> 		}
> >> 
> >> diff --git a/dtc-lexer.l b/dtc-lexer.l
> >> index 790fbf6..40bbc87 100644
> >> --- a/dtc-lexer.l
> >> +++ b/dtc-lexer.l
> >> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> >> 			return DT_V1;
> >> 		}
> >> 
> >> +<*>"/plugin/"	{
> >> +			DPRINT("Keyword: /plugin/\n");
> >> +			return DT_PLUGIN;
> >> +		}
> >> +
> >> <*>"/memreserve/"	{
> >> 			DPRINT("Keyword: /memreserve/\n");
> >> 			BEGIN_DEFAULT();
> >> diff --git a/dtc-parser.y b/dtc-parser.y
> >> index 000873f..2d3399e 100644
> >> --- a/dtc-parser.y
> >> +++ b/dtc-parser.y
> >> @@ -19,6 +19,7 @@
> >>  */
> >> %{
> >> #include <stdio.h>
> >> +#include <inttypes.h>
> >> 
> >> #include "dtc.h"
> >> #include "srcpos.h"
> >> @@ -52,9 +53,11 @@ extern bool treesource_error;
> >> 	struct node *nodelist;
> >> 	struct reserve_info *re;
> >> 	uint64_t integer;
> >> +	unsigned int flags;
> >> }
> >> 
> >> %token DT_V1
> >> +%token DT_PLUGIN
> >> %token DT_MEMRESERVE
> >> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> >> %token DT_BITS
> >> @@ -71,6 +74,8 @@ extern bool treesource_error;
> >> 
> >> %type <data> propdata
> >> %type <data> propdataprefix
> >> +%type <flags> versioninfo
> >> +%type <flags> plugindecl
> >> %type <re> memreserve
> >> %type <re> memreserves
> >> %type <array> arrayprefix
> >> @@ -101,13 +106,31 @@ extern bool treesource_error;
> >> %%
> >> 
> >> sourcefile:
> >> -	  DT_V1 ';' memreserves devicetree
> >> +	  versioninfo ';' memreserves devicetree
> >> 		{
> >> -			the_boot_info = build_boot_info($3, $4,
> >> +			the_boot_info = build_boot_info($1, $3, $4,
> >> 							guess_boot_cpuid($4));
> >> 		}
> >> 	;
> >> 
> >> +versioninfo:
> >> +	DT_V1 plugindecl
> >> +		{
> >> +			$$ = VF_DT_V1 | $2;
> >> +		}
> >> +	;
> >> +
> >> +plugindecl:
> >> +	DT_PLUGIN
> >> +		{
> >> +			$$ = VF_PLUGIN;
> >> +		}
> >> +	| /* empty */
> >> +		{
> >> +			$$ = 0;
> >> +		}
> >> +	;
> >> +
> >> memreserves:
> >> 	  /* empty */
> >> 		{
> >> @@ -156,10 +179,14 @@ devicetree:
> >> 		{
> >> 			struct node *target = get_node_by_ref($1, $2);
> >> 
> >> -			if (target)
> >> +			if (target) {
> >> 				merge_nodes(target, $3);
> >> -			else
> >> -				ERROR(&@2, "Label or path %s not found", $2);
> >> +			} else {
> >> +				if (symbol_fixup_support)
> > 
> > Surely this should depend on the /plugin/ tag, rather than the -@ option..?
> > 
> 
> the_boot_info is not yet initialized at this point. The only way to access it would
> be to have version flags as a global. Is that OK?

Hmm.  So using a global isn't necessarily bad, since the awkward
old-school yacc interface already makes it necessary in some places.
But.. relying on the order in which different yacc semantic actions
are executed (without an explicit data dependency) makes me nervous.

I think the really correct solution is to move the processing of
overlays - for both the dts only and dynamic plugin cases - to a later
pass after parsing.  In the non-plugin case it would merge the
subtrees then, for the plugin case it would construct the "meta-tree"
with the fragment/target info.

But, I can see why you wouldn't want to do that right now.

So, in the short term, I think use a global, with a FIXME comment.

> >> +					add_orphan_node($1, $3, $2);
> >> +				else
> >> +					ERROR(&@2, "Label or path %s not found", $2);
> >> +			}
> >> 			$$ = $1;
> >> 		}
> >> 	| devicetree DT_DEL_NODE DT_REF ';'
> >> @@ -174,6 +201,11 @@ devicetree:
> >> 
> >> 			$$ = $1;
> >> 		}
> >> +	| /* empty */
> >> +		{
> >> +			/* build empty node */
> >> +			$$ = name_node(build_node(NULL, NULL), "");
> >> +		}
> >> 	;
> >> 
> >> nodedef:
> >> diff --git a/dtc.c b/dtc.c
> >> index 5fa23c4..f8277fb 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -31,6 +31,8 @@ int reservenum;		/* Number of memory reservation slots */
> >> int minsize;		/* Minimum blob size */
> >> int padsize;		/* Additional padding to blob */
> >> int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
> >> +int symbol_fixup_support;
> >> +int auto_label_aliases;
> >> 
> >> static void fill_fullpaths(struct node *tree, const char *prefix)
> >> {
> >> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >> #define FDT_VERSION(version)	_FDT_VERSION(version)
> >> #define _FDT_VERSION(version)	#version
> >> static const char usage_synopsis[] = "dtc [options] <input file>";
> >> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
> >> static struct option const usage_long_opts[] = {
> >> 	{"quiet",            no_argument, NULL, 'q'},
> >> 	{"in-format",         a_argument, NULL, 'I'},
> >> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
> >> 	{"phandle",           a_argument, NULL, 'H'},
> >> 	{"warning",           a_argument, NULL, 'W'},
> >> 	{"error",             a_argument, NULL, 'E'},
> >> +	{"symbols",	     no_argument, NULL, '@'},
> >> +	{"auto-alias",       no_argument, NULL, 'A'},
> >> 	{"help",             no_argument, NULL, 'h'},
> >> 	{"version",          no_argument, NULL, 'v'},
> >> 	{NULL,               no_argument, NULL, 0x0},
> >> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
> >> 	 "\t\tboth   - Both \"linux,phandle\" and \"phandle\" properties",
> >> 	"\n\tEnable/disable warnings (prefix with \"no-\")",
> >> 	"\n\tEnable/disable errors (prefix with \"no-\")",
> >> +	"\n\tEnable symbols/fixup support",
> >> +	"\n\tEnable auto-alias of labels",
> >> 	"\n\tPrint this help and exit",
> >> 	"\n\tPrint version and exit",
> >> 	NULL,
> >> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
> >> 		case 'E':
> >> 			parse_checks_option(false, true, optarg);
> >> 			break;
> >> -
> >> +		case '@':
> >> +			symbol_fixup_support = 1;
> >> +			break;
> >> +		case 'A':
> >> +			auto_label_aliases = 1;
> >> +			break;
> >> 		case 'h':
> >> 			usage(NULL);
> >> 		default:
> >> @@ -294,6 +305,14 @@ int main(int argc, char *argv[])
> >> 	if (sort)
> >> 		sort_tree(bi);
> >> 
> >> +	if (auto_label_aliases)
> >> +		generate_label_tree(bi->dt, "aliases", false);
> >> +
> >> +	if (symbol_fixup_support) {
> >> +		generate_label_tree(bi->dt, "__symbols__", true);
> >> +		generate_fixups_tree(bi->dt);
> >> +	}
> >> +
> > 
> > These should go before the sort_tree() - since they add nodes, they
> > could result in something that *isn't* sorted when that was requested.
> > 
> 
> OK
> 
> >> 	if (streq(outname, "-")) {
> >> 		outf = stdout;
> >> 	} else {
> >> diff --git a/dtc.h b/dtc.h
> >> index 56212c8..68cb109 100644
> >> --- a/dtc.h
> >> +++ b/dtc.h
> >> @@ -20,7 +20,6 @@
> >>  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> >>  *                                                                   USA
> >>  */
> >> -
> > 
> > Unrelated whitespace change.
> > 
> 
> OK
> 
> >> #include <stdio.h>
> >> #include <string.h>
> >> #include <stdlib.h>
> >> @@ -54,6 +53,12 @@ extern int reservenum;		/* Number of memory reservation slots */
> >> extern int minsize;		/* Minimum blob size */
> >> extern int padsize;		/* Additional padding to blob */
> >> extern int phandle_format;	/* Use linux,phandle or phandle properties */
> >> +extern int symbol_fixup_support;/* enable symbols & fixup support */
> >> +extern int auto_label_aliases;	/* auto generate labels -> aliases */
> >> +
> >> +/*
> >> + * Tree source globals
> >> + */
> >> 
> >> #define PHANDLE_LEGACY	0x1
> >> #define PHANDLE_EPAPR	0x2
> >> @@ -158,6 +163,9 @@ struct node {
> >> 	int addr_cells, size_cells;
> >> 
> >> 	struct label *labels;
> >> +
> >> +	/* only for the root (parent == NULL) */
> >> +	struct boot_info *bi;
> > 
> > Ugh.. I hate creating circular references unless we really can't avoid
> > them.
> > 
> 
> Either it’s here, or we pass boot_info to every method.
> Need access to the version flags.

I think passing boot_info down instead of the root node is probably
the right approach here.

> 
> >> };
> >> 
> >> #define for_each_label_withdel(l0, l) \
> >> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
> >> struct node *name_node(struct node *node, char *name);
> >> struct node *chain_node(struct node *first, struct node *list);
> >> struct node *merge_nodes(struct node *old_node, struct node *new_node);
> >> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
> >> 
> >> void add_property(struct node *node, struct property *prop);
> >> void delete_property_by_name(struct node *node, char *name);
> >> @@ -236,14 +245,29 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
> >> 
> >> 
> >> struct boot_info {
> >> +	unsigned int versionflags;
> >> 	struct reserve_info *reservelist;
> >> 	struct node *dt;		/* the device tree */
> >> 	uint32_t boot_cpuid_phys;
> >> };
> >> 
> >> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> >> +/* version flags definitions */
> >> +#define VF_DT_V1	0x0001	/* /dts-v1/ */
> > 
> > Is there any point to this, since dtc doesn't support v0 dts files any
> > more anyway?
> > 
> 
> Completeness. I can take it out.

Please do.

> >> +#define VF_PLUGIN	0x0002	/* /plugin/ */
> >> +
> >> +static inline unsigned int tree_get_versionflags(struct node *dt)
> >> +{
> >> +	if (!dt || !dt->bi)
> >> +		return 0;
> >> +	return dt->bi->versionflags;
> >> +}
> >> +
> >> +struct boot_info *build_boot_info(unsigned int versionflags,
> >> +				  struct reserve_info *reservelist,
> >> 				  struct node *tree, uint32_t boot_cpuid_phys);
> >> void sort_tree(struct boot_info *bi);
> >> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
> >> +void generate_fixups_tree(struct node *dt);
> >> 
> >> /* Checks */
> >> 
> >> diff --git a/flattree.c b/flattree.c
> >> index ec14954..5dde832 100644
> >> --- a/flattree.c
> >> +++ b/flattree.c
> >> @@ -929,5 +929,5 @@ struct boot_info *dt_from_blob(const char *fname)
> >> 
> >> 	fclose(f);
> >> 
> >> -	return build_boot_info(reservelist, tree, boot_cpuid_phys);
> >> +	return build_boot_info(VF_DT_V1, reservelist, tree, boot_cpuid_phys);
> >> }
> >> diff --git a/fstree.c b/fstree.c
> >> index 6d1beec..54f520b 100644
> >> --- a/fstree.c
> >> +++ b/fstree.c
> >> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
> >> 	tree = read_fstree(dirname);
> >> 	tree = name_node(tree, "");
> >> 
> >> -	return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
> >> +	return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
> >> }
> >> 
> >> diff --git a/livetree.c b/livetree.c
> >> index e229b84..e7d0fe5 100644
> >> --- a/livetree.c
> >> +++ b/livetree.c
> >> @@ -18,6 +18,7 @@
> >>  *                                                                   USA
> >>  */
> >> 
> >> +#define _GNU_SOURCE
> > 
> > Ugh.  I mean, I know the work I did to get dtc working on BSD has
> > bitrotted already, but can we avoid gratuitously introducing
> > non-portability.
> > 
> 
> It was your suggestion last time, remember? :)

Apparently not :/

> >> #include "dtc.h"
> >> 
> >> /*
> >> @@ -216,6 +217,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> >> 	return old_node;
> >> }
> >> 
> >> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> >> +{
> >> +	static unsigned int next_orphan_fragment = 0;
> >> +	struct node *node = xmalloc(sizeof(*node));
> >> +	struct property *p;
> >> +	struct data d = empty_data;
> >> +	char *name;
> >> +	int ret;
> >> +
> >> +	memset(node, 0, sizeof(*node));
> >> +
> >> +	d = data_add_marker(d, REF_PHANDLE, ref);
> >> +	d = data_append_integer(d, 0xffffffff, 32);
> >> +
> >> +	p = build_property("target", d);
> >> +	add_property(node, p);
> >> +
> >> +	ret = asprintf(&name, "fragment@%u",
> >> +			next_orphan_fragment++);
> >> +	if (ret == -1)
> >> +		die("asprintf() failed\n");
> >> +	name_node(node, name);
> >> +	name_node(new_node, "__overlay__");
> >> +
> >> +	add_child(dt, node);
> >> +	add_child(node, new_node);
> >> +}
> > 
> > 
> > 
> > 
> >> struct node *chain_node(struct node *first, struct node *list)
> >> {
> >> 	assert(first->next_sibling == NULL);
> >> @@ -335,15 +364,19 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
> >> 	return list;
> >> }
> >> 
> >> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> >> +struct boot_info *build_boot_info(unsigned int versionflags,
> >> +				  struct reserve_info *reservelist,
> >> 				  struct node *tree, uint32_t boot_cpuid_phys)
> >> {
> >> 	struct boot_info *bi;
> >> 
> >> 	bi = xmalloc(sizeof(*bi));
> >> +	bi->versionflags = versionflags;
> >> 	bi->reservelist = reservelist;
> >> 	bi->dt = tree;
> >> 	bi->boot_cpuid_phys = boot_cpuid_phys;
> >> +	/* link back */
> >> +	tree->bi = bi;
> >> 
> >> 	return bi;
> >> }
> >> @@ -709,3 +742,185 @@ void sort_tree(struct boot_info *bi)
> >> 	sort_reserve_entries(bi);
> >> 	sort_node(bi->dt);
> >> }
> >> +
> >> +/* utility helper to avoid code duplication */
> >> +static struct node *build_and_name_child_node(struct node *parent, char *name)
> >> +{
> >> +	struct node *node;
> >> +
> >> +	node = build_node(NULL, NULL);
> >> +	name_node(node, xstrdup(name));
> >> +	add_child(parent, node);
> >> +
> >> +	return node;
> >> +}
> >> +
> >> +static void generate_label_tree_internal(struct node *dt, struct node *node,
> >> +					 char *gen_node_name, bool allocph)
> >> +{
> >> +	struct node *c, *an;
> >> +	struct property *p;
> >> +	struct label *l;
> >> +
> >> +	/* if if there are labels */
> >> +	if (node->labels) {
> >> +		/* an is the aliases/__symbols__ node */
> >> +		an = get_subnode(dt, gen_node_name);
> > 
> > You can factor this out to the caller and just pass 'an' in.
> > 
> 
> OK
> 
> >> +		/* if no node exists, create it */
> > 
> > Comment does not match the code
> > 
> 
> Ugh.
> 
> >> +		if (!an)
> >> +			die("Could not get label node\n");
> >> +
> >> +		/* now add the label in the node */
> >> +		for_each_label(node->labels, l) {
> >> +			/* check whether the label already exists */
> >> +			p = get_property(an, l->label);
> >> +			if (p) {
> >> +				fprintf(stderr, "WARNING: label %s already"
> >> +					" exists in /%s", l->label,
> >> +					gen_node_name);
> >> +				continue;
> >> +			}
> >> +
> >> +			/* insert it */
> >> +			p = build_property(l->label,
> >> +				data_copy_escape_string(node->fullpath,
> >> +						strlen(node->fullpath)));
> >> +			add_property(an, p);
> >> +		}
> >> +
> >> +		/* force allocation of a phandle for this node */
> >> +		if (allocph)
> >> +			(void)get_node_phandle(dt, node);
> >> +	}
> >> +
> >> +	for_each_child(node, c)
> >> +		generate_label_tree_internal(dt, c, gen_node_name, allocph);
> >> +}
> >> +
> >> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
> >> +{
> >> +	build_and_name_child_node(dt, gen_node_name);
> >> +	generate_label_tree_internal(dt, dt, gen_node_name, allocph);
> >> +}
> >> +
> >> +static char *fixups_name = "__fixups__";
> >> +static char *local_fixups_name = "__local_fixups__";
> >> +
> >> +static void add_fixup_entry(struct node *dt, struct node *node,
> >> +		struct property *prop, struct marker *m)
> >> +{
> >> +	struct node *fn;	/* local fixup node */
> > 
> > Comment is misleading since this is __fixups__ rather than
> > __local_fixups__.
> > 
> 
> OK
> 
> >> +	struct property *p;
> >> +	struct data d;
> >> +	char *entry;
> >> +	int ret;
> >> +
> >> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
> >> +	if (m->type != REF_PHANDLE)
> >> +		die("Fixup entry can only be a ref to a phandle\n");
> > 
> > If it's not a REF_PHANDLE, that indicates a bug elsewhere, yes?  In
> > which case assert() would be more appropriate.
> > 
> 
> OK
> 
> >> +	/* fn is the node we're putting entries in */
> >> +	fn = get_subnode(dt, fixups_name);
> >> +	if (!fn)
> >> +		die("Can't get fixups node\n");
> > 
> > Again, assert() would be better.
> > 
> 
> OK
> 
> >> +	/* there shouldn't be any ':' in the arguments */
> >> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> >> +		die("arguments should not contain ':'\n");
> >> +
> >> +	ret = asprintf(&entry, "%s:%s:%u",
> >> +			node->fullpath, prop->name, m->offset);
> >> +	if (ret == -1)
> >> +		die("asprintf() failed\n");
> > 
> > Looks like we really should put in an xasprintf() utility function,
> > both to address the BSD portability, and avoid clunky checks for
> > allocation failures.
> > 
> 
> No worries, I’ll add it in a separate patch.
> 
> >> +	p = get_property(fn, m->ref);
> >> +	if (p) {
> >> +		d = data_append_data(p->val, entry, strlen(entry) + 1);
> >> +		p->val = d;
> >> +	} else {
> >> +		d = data_append_data(empty_data, entry, strlen(entry) + 1);
> >> +		add_property(fn, build_property(m->ref, d));
> >> +	}
> >> +}
> >> +
> >> +static void add_local_fixup_entry(struct node *dt, struct node *node,
> >> +		struct property *prop, struct marker *m,
> >> +		struct node *refnode)
> >> +{
> >> +	struct node *lfn, *wn, *nwn;	/* local fixup node, walk node, new */
> >> +	struct property *p;
> >> +	struct data d;
> >> +	char *s, *e, *comp;
> >> +	int len;
> >> +
> >> +	/* fn is the node we're putting entries in */
> >> +	lfn = get_subnode(dt, local_fixups_name);
> >> +	if (!lfn)
> >> +		die("Can't get local fixups node\n");
> > 
> > Again, assert().
> > 
> >> +	/* walk the path components creating nodes if they don't exist */
> >> +	comp = xmalloc(strlen(node->fullpath) + 1);
> >> +	/* start skipping the first / */
> >> +	s = node->fullpath + 1;
> >> +	wn = lfn;
> >> +	while (*s) {
> > 
> > Given the break; below is the loop condition redundant?
> > 
> 
> Sanity check if fullpath is empty. Could assert instead.

I think assert() makes more sense.  The loop condition still won't
actually make this code do anything useful in that case.

> >> +		/* retrieve path component */
> >> +		e = strchr(s, '/');
> >> +		if (e == NULL)
> >> +			e = s + strlen(s);
> >> +		len = e - s;
> >> +		memcpy(comp, s, len);
> >> +		comp[len] = '\0';
> >> +
> >> +		/* if no node exists, create it */
> >> +		nwn = get_subnode(wn, comp);
> >> +		if (!nwn)
> >> +			nwn = build_and_name_child_node(wn, comp);
> >> +		wn = nwn;
> >> +
> >> +		/* last path component */
> >> +		if (!*e)
> >> +			break;
> >> +
> >> +		/* next path component */
> >> +		s = e + 1;
> >> +	}
> >> +	free(comp);
> >> +
> >> +	p = get_property(wn, prop->name);
> >> +	d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
> >> +	if (!p)
> >> +		add_property(wn, build_property(prop->name, d));
> >> +	else
> >> +		p->val = d;
> > 
> > Hmm.. am append_to_property helper would simplify both this and the
> > other fixup function.
> > 
> 
> OK
> 
> >> +}
> >> +
> >> +static void generate_fixups_tree_internal(struct node *dt, struct node *node)
> >> +{
> >> +	struct node *c;
> >> +	struct property *prop;
> >> +	struct marker *m;
> >> +	struct node *refnode;
> >> +
> >> +	for_each_property(node, prop) {
> >> +		m = prop->val.markers;
> >> +		for_each_marker_of_type(m, REF_PHANDLE) {
> >> +			refnode = get_node_by_ref(dt, m->ref);
> >> +			if (!refnode)
> >> +				add_fixup_entry(dt, node, prop, m);
> >> +			else
> >> +				add_local_fixup_entry(dt, node, prop, m,
> >> +						refnode);
> >> +		}
> >> +	}
> >> +
> >> +	for_each_child(node, c)
> >> +		generate_fixups_tree_internal(dt, c);
> >> +}
> >> +
> >> +void generate_fixups_tree(struct node *dt)
> >> +{
> >> +	build_and_name_child_node(dt, fixups_name);
> >> +	build_and_name_child_node(dt, local_fixups_name);
> >> +	generate_fixups_tree_internal(dt, dt);
> >> +}
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v6 3/4] plugin: Transparently support old style syntax

[Pantelis Antoniou]

The old style syntax for plugins is still out in the wild.
This patch transparently support it.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 dtc-parser.y | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/dtc-parser.y b/dtc-parser.y
index 2d3399e..3f006b4 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -76,6 +76,7 @@ extern bool treesource_error;
 %type <data> propdataprefix
 %type <flags> versioninfo
 %type <flags> plugindecl
+%type <flags> oldplugindecl
 %type <re> memreserve
 %type <re> memreserves
 %type <array> arrayprefix
@@ -106,10 +107,10 @@ extern bool treesource_error;
 %%
 
 sourcefile:
-	  versioninfo ';' memreserves devicetree
+	  versioninfo ';' oldplugindecl memreserves devicetree
 		{
-			the_boot_info = build_boot_info($1, $3, $4,
-							guess_boot_cpuid($4));
+			the_boot_info = build_boot_info($1 | $3, $4, $5,
+							guess_boot_cpuid($5));
 		}
 	;
 
@@ -131,6 +132,17 @@ plugindecl:
 		}
 	;
 
+oldplugindecl:
+	DT_PLUGIN ';'
+		{
+			$$ = VF_PLUGIN;
+		}
+	| /* empty */
+		{
+			$$ = 0;
+		}
+	;
+
 memreserves:
 	  /* empty */
 		{
-- 
1.7.12

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Rob Herring]

On Thu, May 5, 2016 at 2:48 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> The old style syntax for plugins is still out in the wild.

In the wild where?

IMO, too bad. That's the danger of deploying prototype code. Perhaps a
tool to convert them would be better.

Rob

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Phil Elwell]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Fri, May 6, 2016 at 1:58 AM, Rob Herring <robherring2@gmail.com> wrote:

> In the wild where?
>
> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
> tool to convert them would be better.


At Raspberry Pi we've been making good use of overlays for over a year now.
They have been an invaluable tool, allowing us to reduce the need for
downstream code while retaining and extending the flexibility our users
need. Without the circulating patches I would have pressed ahead with a
homegrown equivalent, which would have made switching to overlays much more
painful.

I hope to see Pantelis's overlay configfs code in a kernel release soon,
but until then we'll continue to apply the patches downstream.

Phil

[-- Attachment #2: Type: text/html, Size: 1217 bytes --]

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Phil Elwell]

> In the wild where?
>
> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
> tool to convert them would be better.


At Raspberry Pi we've been making good use of overlays for over a year now.
They have been an invaluable tool, allowing us to reduce the need for
downstream code while retaining and extending the flexibility our users
need. Without the circulating patches I would have pressed ahead with a
homegrown equivalent, which would have made switching to overlays much more
painful.

I hope to see Pantelis's overlay configfs code in a kernel release soon,
but until then we'll continue to apply the patches downstream.

Phil

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Nicolas Ferre]

Le 06/05/2016 11:16, Phil Elwell a écrit :
>> In the wild where?
>>
>> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
>> tool to convert them would be better.

Prototype code or no code, which one solve actual problems?


> At Raspberry Pi we've been making good use of overlays for over a year now.
> They have been an invaluable tool, allowing us to reduce the need for
> downstream code while retaining and extending the flexibility our users
> need. Without the circulating patches I would have pressed ahead with a
> homegrown equivalent, which would have made switching to overlays much more
> painful.
> 
> I hope to see Pantelis's overlay configfs code in a kernel release soon,
> but until then we'll continue to apply the patches downstream.

+1

Same situation for for Atmel Xplained boards.
I remember having identified DT overlays as the solution to my
out-of-tree nightmare nearly 2 years ago...

Hang in there Pantelis, I admire your determination!


Bye,
-- 
Nicolas Ferre

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Nicolas Ferre]

Le 06/05/2016 11:16, Phil Elwell a écrit :
>> In the wild where?
>>
>> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
>> tool to convert them would be better.

Prototype code or no code, which one solve actual problems?


> At Raspberry Pi we've been making good use of overlays for over a year now.
> They have been an invaluable tool, allowing us to reduce the need for
> downstream code while retaining and extending the flexibility our users
> need. Without the circulating patches I would have pressed ahead with a
> homegrown equivalent, which would have made switching to overlays much more
> painful.
> 
> I hope to see Pantelis's overlay configfs code in a kernel release soon,
> but until then we'll continue to apply the patches downstream.

+1

Same situation for for Atmel Xplained boards.
I remember having identified DT overlays as the solution to my
out-of-tree nightmare nearly 2 years ago...

Hang in there Pantelis, I admire your determination!


Bye,
-- 
Nicolas Ferre

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Rob Herring]

On Fri, May 6, 2016 at 4:30 AM, Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> Le 06/05/2016 11:16, Phil Elwell a écrit :
>>> In the wild where?
>>>
>>> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
>>> tool to convert them would be better.
>
> Prototype code or no code, which one solve actual problems?

I didn't say don't use it. You just have to understand the risks when
you take things off the list and be willing to deal with the changes.

In any case, we're only talking about changes in the source format.
Anything deployed should be fine. Folks can't update their dts files?

>> At Raspberry Pi we've been making good use of overlays for over a year now.
>> They have been an invaluable tool, allowing us to reduce the need for
>> downstream code while retaining and extending the flexibility our users
>> need. Without the circulating patches I would have pressed ahead with a
>> homegrown equivalent, which would have made switching to overlays much more
>> painful.

And I'm glad you did. It works great for me.

>> I hope to see Pantelis's overlay configfs code in a kernel release soon,
>> but until then we'll continue to apply the patches downstream.
>
> +1
>
> Same situation for for Atmel Xplained boards.
> I remember having identified DT overlays as the solution to my
> out-of-tree nightmare nearly 2 years ago...

I'm hardly saying you should have done something different.

Rob

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[Rob Herring]

On Fri, May 6, 2016 at 4:30 AM, Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> Le 06/05/2016 11:16, Phil Elwell a écrit :
>>> In the wild where?
>>>
>>> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
>>> tool to convert them would be better.
>
> Prototype code or no code, which one solve actual problems?

I didn't say don't use it. You just have to understand the risks when
you take things off the list and be willing to deal with the changes.

In any case, we're only talking about changes in the source format.
Anything deployed should be fine. Folks can't update their dts files?

>> At Raspberry Pi we've been making good use of overlays for over a year now.
>> They have been an invaluable tool, allowing us to reduce the need for
>> downstream code while retaining and extending the flexibility our users
>> need. Without the circulating patches I would have pressed ahead with a
>> homegrown equivalent, which would have made switching to overlays much more
>> painful.

And I'm glad you did. It works great for me.

>> I hope to see Pantelis's overlay configfs code in a kernel release soon,
>> but until then we'll continue to apply the patches downstream.
>
> +1
>
> Same situation for for Atmel Xplained boards.
> I remember having identified DT overlays as the solution to my
> out-of-tree nightmare nearly 2 years ago...

I'm hardly saying you should have done something different.

Rob

Re: [PATCH v6 3/4] plugin: Transparently support old style syntax

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Thu, May 05, 2016 at 07:58:28PM -0500, Rob Herring wrote:
> On Thu, May 5, 2016 at 2:48 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> > The old style syntax for plugins is still out in the wild.
> 
> In the wild where?
> 
> IMO, too bad. That's the danger of deploying prototype code. Perhaps a
> tool to convert them would be better.

I tend to agree.  If converting to the new format was fiddly, I'd see
the point of including compatibility.  But AFAICT it's literally a
matter of removing a single semicolon.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v6 4/4] DTBO magic and dtbo format options

[Pantelis Antoniou]

Introduce a new magic number for dynamic plugin objects,
which is enabled by selecting dtbo/input output options.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/manual.txt |  7 +++++++
 dtc.c                    | 14 +++++++++++---
 dtc.h                    |  4 ++--
 fdtdump.c                |  2 +-
 flattree.c               | 11 ++++++-----
 libfdt/fdt.c             |  2 +-
 libfdt/fdt.h             |  3 ++-
 libfdt/libfdt_internal.h |  1 +
 tests/mangle-layout.c    |  7 ++++---
 9 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 3e16c24..63066ec 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -60,6 +60,9 @@ The currently supported Input Formats are:
     - "dtb": "blob" format.  A flattened device-tree block with
         header in one binary blob.
 
+    - "dtbo" : "blob" format. Identical with "dtb" but meant
+        for use with dynamic-device tree objects.
+
     - "dts": "source" format.  A text file containing a "source"
         for a device-tree.
 
@@ -71,6 +74,8 @@ The currently supported Output Formats are:
 
      - "dtb": "blob" format
 
+     - "dtbo": "blob" format - for objects
+
      - "dts": "source" format
 
      - "asm": assembly language file.  A file that can be sourced
@@ -78,6 +83,8 @@ The currently supported Output Formats are:
         then simply be added to your Makefile.  Additionally, the
         assembly file exports some symbols that can be used.
 
+     - "asmo": assembly language file for objects. Identical to "asm"
+
 
 3) Command Line
 
diff --git a/dtc.c b/dtc.c
index f8277fb..1330e15 100644
--- a/dtc.c
+++ b/dtc.c
@@ -123,6 +123,8 @@ static const char *guess_type_by_name(const char *fname, const char *fallback)
 		return "dts";
 	if (!strcasecmp(s, ".dtb"))
 		return "dtb";
+	if (!strcasecmp(s, ".dtbo"))
+		return "dtbo";
 	return fallback;
 }
 
@@ -153,6 +155,8 @@ static const char *guess_input_format(const char *fname, const char *fallback)
 	magic = fdt32_to_cpu(magic);
 	if (magic == FDT_MAGIC)
 		return "dtb";
+	if (magic == FDT_MAGIC_DTBO)
+		return "dtbo";
 
 	return guess_type_by_name(fname, fallback);
 }
@@ -286,7 +290,7 @@ int main(int argc, char *argv[])
 		bi = dt_from_source(arg);
 	else if (streq(inform, "fs"))
 		bi = dt_from_fs(arg);
-	else if(streq(inform, "dtb"))
+	else if(streq(inform, "dtb") || streq(inform, "dtbo"))
 		bi = dt_from_blob(arg);
 	else
 		die("Unknown input format \"%s\"\n", inform);
@@ -325,9 +329,13 @@ int main(int argc, char *argv[])
 	if (streq(outform, "dts")) {
 		dt_to_source(outf, bi);
 	} else if (streq(outform, "dtb")) {
-		dt_to_blob(outf, bi, outversion);
+		dt_to_blob(outf, bi, FDT_MAGIC, outversion);
+	} else if (streq(outform, "dtbo")) {
+		dt_to_blob(outf, bi, FDT_MAGIC_DTBO, outversion);
 	} else if (streq(outform, "asm")) {
-		dt_to_asm(outf, bi, outversion);
+		dt_to_asm(outf, bi, FDT_MAGIC, outversion);
+	} else if (streq(outform, "asmo")) {
+		dt_to_asm(outf, bi, FDT_MAGIC_DTBO, outversion);
 	} else if (streq(outform, "null")) {
 		/* do nothing */
 	} else {
diff --git a/dtc.h b/dtc.h
index 68cb109..aea6509 100644
--- a/dtc.h
+++ b/dtc.h
@@ -276,8 +276,8 @@ void process_checks(bool force, struct boot_info *bi);
 
 /* Flattened trees */
 
-void dt_to_blob(FILE *f, struct boot_info *bi, int version);
-void dt_to_asm(FILE *f, struct boot_info *bi, int version);
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
 
 struct boot_info *dt_from_blob(const char *fname);
 
diff --git a/fdtdump.c b/fdtdump.c
index 9183555..11c2b8d 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -306,7 +306,7 @@ int main(int argc, char *argv[])
 			p = memchr(p, smagic[0], endp - p - 4);
 			if (!p)
 				break;
-			if (fdt_magic(p) == FDT_MAGIC) {
+			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
 				/* try and validate the main struct */
 				off_t this_len = endp - p;
 				fdt32_t max_version = 17;
diff --git a/flattree.c b/flattree.c
index 5dde832..4fe64d4 100644
--- a/flattree.c
+++ b/flattree.c
@@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
 }
 
 static void make_fdt_header(struct fdt_header *fdt,
+			    fdt32_t magic,
 			    struct version_info *vi,
 			    int reservesize, int dtsize, int strsize,
 			    int boot_cpuid_phys)
@@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
 
 	memset(fdt, 0xff, sizeof(*fdt));
 
-	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
+	fdt->magic = cpu_to_fdt32(magic);
 	fdt->version = cpu_to_fdt32(vi->version);
 	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
 
@@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
 		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
 }
 
-void dt_to_blob(FILE *f, struct boot_info *bi, int version)
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
 {
 	struct version_info *vi = NULL;
 	int i;
@@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
 	reservebuf = flatten_reserve_list(bi->reservelist, vi);
 
 	/* Make header */
-	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
+	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
 			bi->boot_cpuid_phys);
 
 	/*
@@ -460,7 +461,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
 	}
 }
 
-void dt_to_asm(FILE *f, struct boot_info *bi, int version)
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
 {
 	struct version_info *vi = NULL;
 	int i;
@@ -832,7 +833,7 @@ struct boot_info *dt_from_blob(const char *fname)
 	}
 
 	magic = fdt32_to_cpu(magic);
-	if (magic != FDT_MAGIC)
+	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
 		die("Blob has incorrect magic number\n");
 
 	rc = fread(&totalsize, sizeof(totalsize), 1, f);
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 22286a1..28d422c 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -57,7 +57,7 @@
 
 int fdt_check_header(const void *fdt)
 {
-	if (fdt_magic(fdt) == FDT_MAGIC) {
+	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
 		/* Complete tree */
 		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
 			return -FDT_ERR_BADVERSION;
diff --git a/libfdt/fdt.h b/libfdt/fdt.h
index 526aedb..493cd55 100644
--- a/libfdt/fdt.h
+++ b/libfdt/fdt.h
@@ -55,7 +55,7 @@
 #ifndef __ASSEMBLY__
 
 struct fdt_header {
-	fdt32_t magic;			 /* magic word FDT_MAGIC */
+	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
 	fdt32_t totalsize;		 /* total size of DT block */
 	fdt32_t off_dt_struct;		 /* offset to structure */
 	fdt32_t off_dt_strings;		 /* offset to strings */
@@ -93,6 +93,7 @@ struct fdt_property {
 #endif /* !__ASSEMBLY */
 
 #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
+#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
 #define FDT_TAGSIZE	sizeof(fdt32_t)
 
 #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
index 02cfa6f..773dfa3 100644
--- a/libfdt/libfdt_internal.h
+++ b/libfdt/libfdt_internal.h
@@ -91,5 +91,6 @@ static inline struct fdt_reserve_entry *_fdt_mem_rsv_w(void *fdt, int n)
 }
 
 #define FDT_SW_MAGIC		(~FDT_MAGIC)
+#define FDT_SW_MAGIC_DTBO	(~FDT_MAGIC_DTBO)
 
 #endif /* _LIBFDT_INTERNAL_H */
diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
index a76e51e..d29ebc6 100644
--- a/tests/mangle-layout.c
+++ b/tests/mangle-layout.c
@@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
 	buf->size = newsize;
 }
 
-static void new_header(struct bufstate *buf, int version, const void *fdt)
+static void new_header(struct bufstate *buf, fdt32_t magic, int version,
+		       const void *fdt)
 {
 	int hdrsize;
 
@@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
 	expand_buf(buf, hdrsize);
 	memset(buf->buf, 0, hdrsize);
 
-	fdt_set_magic(buf->buf, FDT_MAGIC);
+	fdt_set_magic(buf->buf, magic);
 	fdt_set_version(buf->buf, version);
 	fdt_set_last_comp_version(buf->buf, 16);
 	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
@@ -145,7 +146,7 @@ int main(int argc, char *argv[])
 	if (fdt_version(fdt) < 17)
 		CONFIG("Input tree must be v17");
 
-	new_header(&buf, version, fdt);
+	new_header(&buf, FDT_MAGIC, version, fdt);
 
 	while (*blockorder) {
 		add_block(&buf, version, *blockorder, fdt);
-- 
1.7.12

Re: [PATCH v6 4/4] DTBO magic and dtbo format options

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10079 bytes --]

On Thu, May 05, 2016 at 10:48:44PM +0300, Pantelis Antoniou wrote:
> Introduce a new magic number for dynamic plugin objects,
> which is enabled by selecting dtbo/input output options.

I'd prefer to see this earlier in the series (maybe merged with the
base support.

Basically using the new magic number for overlays should be the "base"
default behaviour implemented first.  Outputting the same magic number
of fdts should be the backwards compatibility hack implemented after.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/manual.txt |  7 +++++++
>  dtc.c                    | 14 +++++++++++---
>  dtc.h                    |  4 ++--
>  fdtdump.c                |  2 +-
>  flattree.c               | 11 ++++++-----
>  libfdt/fdt.c             |  2 +-
>  libfdt/fdt.h             |  3 ++-
>  libfdt/libfdt_internal.h |  1 +
>  tests/mangle-layout.c    |  7 ++++---
>  9 files changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 3e16c24..63066ec 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -60,6 +60,9 @@ The currently supported Input Formats are:
>      - "dtb": "blob" format.  A flattened device-tree block with
>          header in one binary blob.
>  
> +    - "dtbo" : "blob" format. Identical with "dtb" but meant
> +        for use with dynamic-device tree objects.
> +
>      - "dts": "source" format.  A text file containing a "source"
>          for a device-tree.
>  
> @@ -71,6 +74,8 @@ The currently supported Output Formats are:
>  
>       - "dtb": "blob" format
>  
> +     - "dtbo": "blob" format - for objects
> +
>       - "dts": "source" format
>  
>       - "asm": assembly language file.  A file that can be sourced
> @@ -78,6 +83,8 @@ The currently supported Output Formats are:
>          then simply be added to your Makefile.  Additionally, the
>          assembly file exports some symbols that can be used.
>  
> +     - "asmo": assembly language file for objects. Identical to "asm"
> +
>  
>  3) Command Line
>  
> diff --git a/dtc.c b/dtc.c
> index f8277fb..1330e15 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -123,6 +123,8 @@ static const char *guess_type_by_name(const char *fname, const char *fallback)
>  		return "dts";
>  	if (!strcasecmp(s, ".dtb"))
>  		return "dtb";
> +	if (!strcasecmp(s, ".dtbo"))
> +		return "dtbo";
>  	return fallback;
>  }
>  
> @@ -153,6 +155,8 @@ static const char *guess_input_format(const char *fname, const char *fallback)
>  	magic = fdt32_to_cpu(magic);
>  	if (magic == FDT_MAGIC)
>  		return "dtb";
> +	if (magic == FDT_MAGIC_DTBO)
> +		return "dtbo";
>  
>  	return guess_type_by_name(fname, fallback);
>  }
> @@ -286,7 +290,7 @@ int main(int argc, char *argv[])
>  		bi = dt_from_source(arg);
>  	else if (streq(inform, "fs"))
>  		bi = dt_from_fs(arg);
> -	else if(streq(inform, "dtb"))
> +	else if(streq(inform, "dtb") || streq(inform, "dtbo"))
>  		bi = dt_from_blob(arg);
>  	else
>  		die("Unknown input format \"%s\"\n", inform);
> @@ -325,9 +329,13 @@ int main(int argc, char *argv[])
>  	if (streq(outform, "dts")) {
>  		dt_to_source(outf, bi);
>  	} else if (streq(outform, "dtb")) {
> -		dt_to_blob(outf, bi, outversion);
> +		dt_to_blob(outf, bi, FDT_MAGIC, outversion);
> +	} else if (streq(outform, "dtbo")) {
> +		dt_to_blob(outf, bi, FDT_MAGIC_DTBO, outversion);
>  	} else if (streq(outform, "asm")) {
> -		dt_to_asm(outf, bi, outversion);
> +		dt_to_asm(outf, bi, FDT_MAGIC, outversion);
> +	} else if (streq(outform, "asmo")) {
> +		dt_to_asm(outf, bi, FDT_MAGIC_DTBO, outversion);
>  	} else if (streq(outform, "null")) {
>  		/* do nothing */
>  	} else {
> diff --git a/dtc.h b/dtc.h
> index 68cb109..aea6509 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -276,8 +276,8 @@ void process_checks(bool force, struct boot_info *bi);
>  
>  /* Flattened trees */
>  
> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>  
>  struct boot_info *dt_from_blob(const char *fname);
>  
> diff --git a/fdtdump.c b/fdtdump.c
> index 9183555..11c2b8d 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -306,7 +306,7 @@ int main(int argc, char *argv[])
>  			p = memchr(p, smagic[0], endp - p - 4);
>  			if (!p)
>  				break;
> -			if (fdt_magic(p) == FDT_MAGIC) {
> +			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
>  				/* try and validate the main struct */
>  				off_t this_len = endp - p;
>  				fdt32_t max_version = 17;
> diff --git a/flattree.c b/flattree.c
> index 5dde832..4fe64d4 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
>  }
>  
>  static void make_fdt_header(struct fdt_header *fdt,
> +			    fdt32_t magic,
>  			    struct version_info *vi,
>  			    int reservesize, int dtsize, int strsize,
>  			    int boot_cpuid_phys)
> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>  
>  	memset(fdt, 0xff, sizeof(*fdt));
>  
> -	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
> +	fdt->magic = cpu_to_fdt32(magic);
>  	fdt->version = cpu_to_fdt32(vi->version);
>  	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
>  
> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>  		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
>  }
>  
> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>  {
>  	struct version_info *vi = NULL;
>  	int i;
> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>  	reservebuf = flatten_reserve_list(bi->reservelist, vi);
>  
>  	/* Make header */
> -	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
> +	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
>  			bi->boot_cpuid_phys);
>  
>  	/*
> @@ -460,7 +461,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
>  	}
>  }
>  
> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>  {
>  	struct version_info *vi = NULL;
>  	int i;
> @@ -832,7 +833,7 @@ struct boot_info *dt_from_blob(const char *fname)
>  	}
>  
>  	magic = fdt32_to_cpu(magic);
> -	if (magic != FDT_MAGIC)
> +	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
>  		die("Blob has incorrect magic number\n");
>  
>  	rc = fread(&totalsize, sizeof(totalsize), 1, f);
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 22286a1..28d422c 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -57,7 +57,7 @@
>  
>  int fdt_check_header(const void *fdt)
>  {
> -	if (fdt_magic(fdt) == FDT_MAGIC) {
> +	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
>  		/* Complete tree */
>  		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>  			return -FDT_ERR_BADVERSION;
> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
> index 526aedb..493cd55 100644
> --- a/libfdt/fdt.h
> +++ b/libfdt/fdt.h
> @@ -55,7 +55,7 @@
>  #ifndef __ASSEMBLY__
>  
>  struct fdt_header {
> -	fdt32_t magic;			 /* magic word FDT_MAGIC */
> +	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
>  	fdt32_t totalsize;		 /* total size of DT block */
>  	fdt32_t off_dt_struct;		 /* offset to structure */
>  	fdt32_t off_dt_strings;		 /* offset to strings */
> @@ -93,6 +93,7 @@ struct fdt_property {
>  #endif /* !__ASSEMBLY */
>  
>  #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
> +#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
>  #define FDT_TAGSIZE	sizeof(fdt32_t)
>  
>  #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 02cfa6f..773dfa3 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -91,5 +91,6 @@ static inline struct fdt_reserve_entry *_fdt_mem_rsv_w(void *fdt, int n)
>  }
>  
>  #define FDT_SW_MAGIC		(~FDT_MAGIC)
> +#define FDT_SW_MAGIC_DTBO	(~FDT_MAGIC_DTBO)

This define is never used, remove it - there's no way to construct
dtbos with libfdt, serially or otherwise (maybe that should change,
but only add the define when it does).

>  #endif /* _LIBFDT_INTERNAL_H */
> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
> index a76e51e..d29ebc6 100644
> --- a/tests/mangle-layout.c
> +++ b/tests/mangle-layout.c
> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
>  	buf->size = newsize;
>  }
>  
> -static void new_header(struct bufstate *buf, int version, const void *fdt)
> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
> +		       const void *fdt)
>  {
>  	int hdrsize;
>  
> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
>  	expand_buf(buf, hdrsize);
>  	memset(buf->buf, 0, hdrsize);
>  
> -	fdt_set_magic(buf->buf, FDT_MAGIC);
> +	fdt_set_magic(buf->buf, magic);
>  	fdt_set_version(buf->buf, version);
>  	fdt_set_last_comp_version(buf->buf, 16);
>  	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
>  	if (fdt_version(fdt) < 17)
>  		CONFIG("Input tree must be v17");
>  
> -	new_header(&buf, version, fdt);
> +	new_header(&buf, FDT_MAGIC, version, fdt);
>  
>  	while (*blockorder) {
>  		add_block(&buf, version, *blockorder, fdt);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]