[PATCH v3 0/3] Variable sized cell support

[PATCH v3 0/3] Variable sized cell support

[Anton Staaf]

This patch set adds, tests, and documents support for variable sized cells in
cell arrays.  The new syntax is:

    property = /bits/ [8|16|32|64] <0x00 .... 0x12>;

So cell arrays of 8, 16, 32, or 64-bits can now be created.  No padding is
done on the resulting cell array.  So if three 8-bit cells are specified the
resulting property will contain three bytes.

Changes in v2:
- sized_data renamed to celllist
- celllist definition moved to dtc-parser.y
- celllist non-terminal renamed to celllistprefix
- references always appended as -1 masked to the size of the cell
- smaller test values derived from TEST_VALUE_1
- data_append_literal renamed to data_append_integer
- be_ removed from values in data_append_integer
- len renamed to bits in data_append_integer

Changes in v3:
- Renamed size to bits in docs, parser, commit messages, and celllist struct
- Remove die on overflow from data_append_integer
- Rewrite data_append_cell|addr in terms of data_append_integer
- Minor cosmetic change to property names in test case (added 'b' to last two)

Anton Staaf (3):
  libfdt: Add fdt16_to_cpu utility function
  dtc: Add data_append_integer function
  dtc: Add support for variable sized cells

 Documentation/dts-format.txt |   18 +++++++-
 data.c                       |   39 ++++++++++++++++---
 dtc-lexer.l                  |    6 +++
 dtc-parser.y                 |   67 ++++++++++++++++++++++-----------
 dtc.h                        |    1 +
 libfdt/libfdt_env.h          |    6 +++
 tests/.gitignore             |    1 +
 tests/Makefile.tests         |    1 +
 tests/run_tests.sh           |    3 +
 tests/sized_cells.c          |   84 ++++++++++++++++++++++++++++++++++++++++++
 tests/sized_cells.dts        |   11 +++++
 11 files changed, 205 insertions(+), 32 deletions(-)
 create mode 100644 tests/sized_cells.c
 create mode 100644 tests/sized_cells.dts

-- 
1.7.3.1

[PATCH v3 1/3] libfdt: Add fdt16_to_cpu utility function

[Anton Staaf]

This utility routine will be used in the variable size cell literal
append code.  It is a straightforward adaptation of the fdt32_to_cpu
function.

Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 libfdt/libfdt_env.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
index 449bf60..da952e7 100644
--- a/libfdt/libfdt_env.h
+++ b/libfdt/libfdt_env.h
@@ -6,6 +6,12 @@
 #include <string.h>
 
 #define _B(n)	((unsigned long long)((uint8_t *)&x)[n])
+static inline uint16_t fdt16_to_cpu(uint16_t x)
+{
+	return (_B(0) << 8) | _B(1);
+}
+#define cpu_to_fdt16(x) fdt16_to_cpu(x)
+
 static inline uint32_t fdt32_to_cpu(uint32_t x)
 {
 	return (_B(0) << 24) | (_B(1) << 16) | (_B(2) << 8) | _B(3);
-- 
1.7.3.1

Re: [PATCH v3 1/3] libfdt: Add fdt16_to_cpu utility function

[David Gibson]

On Fri, Sep 23, 2011 at 11:02:52AM -0700, Anton Staaf wrote:
> This utility routine will be used in the variable size cell literal
> append code.  It is a straightforward adaptation of the fdt32_to_cpu
> function.
> 
> Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
> Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

-- 
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

[PATCH v3 2/3] dtc: Add data_append_integer function

[Anton Staaf]

This function deals with appending integers of various sizes (8, 16
32, and 64 bit currently).  It handles endianess conversions.  If the
integer will not fit in the requested number of bits of storage it
will have it's high bits ignored.

This patch also rewrites data_append_cell and data_append_addr to use
data_append_integer.

Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 data.c |   39 ++++++++++++++++++++++++++++++++-------
 dtc.h  |    1 +
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/data.c b/data.c
index b5f3066..4a40c5b 100644
--- a/data.c
+++ b/data.c
@@ -168,11 +168,33 @@ struct data data_merge(struct data d1, struct data d2)
 	return d;
 }
 
-struct data data_append_cell(struct data d, cell_t word)
+struct data data_append_integer(struct data d, uint64_t value, int bits)
 {
-	cell_t beword = cpu_to_fdt32(word);
-
-	return data_append_data(d, &beword, sizeof(beword));
+	uint8_t value_8;
+	uint16_t value_16;
+	uint32_t value_32;
+	uint64_t value_64;
+
+	switch (bits) {
+	case 8:
+		value_8 = value;
+		return data_append_data(d, &value_8, 1);
+
+	case 16:
+		value_16 = cpu_to_fdt16(value);
+		return data_append_data(d, &value_16, 2);
+
+	case 32:
+		value_32 = cpu_to_fdt32(value);
+		return data_append_data(d, &value_32, 4);
+
+	case 64:
+		value_64 = cpu_to_fdt64(value);
+		return data_append_data(d, &value_64, 8);
+
+	default:
+		die("Invalid literal size (%d)\n", bits);
+	}
 }
 
 struct data data_append_re(struct data d, const struct fdt_reserve_entry *re)
@@ -185,11 +207,14 @@ struct data data_append_re(struct data d, const struct fdt_reserve_entry *re)
 	return data_append_data(d, &bere, sizeof(bere));
 }
 
-struct data data_append_addr(struct data d, uint64_t addr)
+struct data data_append_cell(struct data d, cell_t word)
 {
-	uint64_t beaddr = cpu_to_fdt64(addr);
+	return data_append_integer(d, word, sizeof(word) * 8);
+}
 
-	return data_append_data(d, &beaddr, sizeof(beaddr));
+struct data data_append_addr(struct data d, uint64_t addr)
+{
+	return data_append_integer(d, addr, sizeof(addr) * 8);
 }
 
 struct data data_append_byte(struct data d, uint8_t byte)
diff --git a/dtc.h b/dtc.h
index f37c97e..7b4c65b 100644
--- a/dtc.h
+++ b/dtc.h
@@ -109,6 +109,7 @@ struct data data_insert_at_marker(struct data d, struct marker *m,
 				  const void *p, int len);
 struct data data_merge(struct data d1, struct data d2);
 struct data data_append_cell(struct data d, cell_t word);
+struct data data_append_integer(struct data d, uint64_t word, int bits);
 struct data data_append_re(struct data d, const struct fdt_reserve_entry *re);
 struct data data_append_addr(struct data d, uint64_t addr);
 struct data data_append_byte(struct data d, uint8_t byte);
-- 
1.7.3.1

Re: [PATCH v3 2/3] dtc: Add data_append_integer function

[David Gibson]

On Fri, Sep 23, 2011 at 11:02:53AM -0700, Anton Staaf wrote:
> This function deals with appending integers of various sizes (8, 16
> 32, and 64 bit currently).  It handles endianess conversions.  If the
> integer will not fit in the requested number of bits of storage it
> will have it's high bits ignored.
> 
> This patch also rewrites data_append_cell and data_append_addr to use
> data_append_integer.
> 
> Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

-- 
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

[PATCH v3 3/3] dtc: Add support for variable sized cells

[Anton Staaf]

Cells of size 8, 16, 32, and 64 bits are supported.  The new
/bits/ syntax was selected so as to not pollute the reserved
keyword space with uint8/uint16/... type names.

With this patch the following property assignment:

    property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;

is equivalent to:

    property = <0x12345678 0x0000ffff>;

It is now also possible to directly specify a 64 bit literal in a
cell list using:

    property = /bits/ 64 <0xdeadbeef00000000>;

It is an error to attempt to store a literal into a cell that is too
small to hold the literal, and the compiler will generate an error
when it detects this.  For instance:

    property = /bits/ 8 <256>;

Will fail to compile.  It is also an error to attempt to place a
reference in a non 32-bit cell.

The sized_cells test tests the creation and access of 8, 16, 32,
and 64-bit sized cells.  It also tests that the creation of two
properties, one with 16 bit cells and one with 32 bit cells result
in the same property contents.

Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 Documentation/dts-format.txt |   18 +++++++-
 dtc-lexer.l                  |    6 +++
 dtc-parser.y                 |   67 ++++++++++++++++++++++-----------
 tests/.gitignore             |    1 +
 tests/Makefile.tests         |    1 +
 tests/run_tests.sh           |    3 +
 tests/sized_cells.c          |   84 ++++++++++++++++++++++++++++++++++++++++++
 tests/sized_cells.dts        |   11 +++++
 8 files changed, 166 insertions(+), 25 deletions(-)
 create mode 100644 tests/sized_cells.c
 create mode 100644 tests/sized_cells.dts

diff --git a/Documentation/dts-format.txt b/Documentation/dts-format.txt
index 555bd89..64a862e 100644
--- a/Documentation/dts-format.txt
+++ b/Documentation/dts-format.txt
@@ -29,18 +29,27 @@ except for properties with empty (zero length) value which have the
 form:
 	[label:] property-name;
 
-Property values may be defined as an array of 32-bit integer cells, as
-NUL-terminated strings, as bytestrings or a combination of these.
+Property values may be defined as an array of 8, 16, 32, or 64-bit integer
+cells, as NUL-terminated strings, as bytestrings or a combination of these.
 
 * Arrays of cells are represented by angle brackets surrounding a
   space separated list of C-style integers or character literals.
+  Cells default to 32-bits in size.
 
 	e.g. interrupts = <17 0xc>;
 
-* A 64-bit value is represented with two 32-bit cells.
+* A 64-bit value can be represented with two 32-bit cells.
 
 	e.g. clock-frequency = <0x00000001 0x00000000>;
 
+* The storage size of a cell can be changed using the /bits/ prefix.  The
+  /bits/ prefix allows for the creation of 8, 16, 32, and 64-bit cells.
+  The resulting cell array will not be padded to a multiple of the default
+  32-bit cell size.
+
+	e.g. interrupts = /bits/ 8 <17 0xc>;
+	e.g. clock-frequency = /bits/ 64 <0x0000000100000000>;
+
 * A NUL-terminated string value is represented using double quotes
   (the property value is considered to include the terminating NUL
   character).
@@ -65,6 +74,8 @@ NUL-terminated strings, as bytestrings or a combination of these.
 	e.g. interrupt-parent = < &mpic >;
   or they may be '&' followed by a node's full path in braces:
 	e.g. interrupt-parent = < &{/soc/interrupt-controller@40000} >;
+  References are only permitted in cell arrays that have a cell size of
+  32-bits.
 
 * Outside a cell array, a reference to another node will be expanded
   to that node's full path.
@@ -109,3 +120,4 @@ Version 1 DTS files have the overall layout:
 
 	-- David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
 	-- Yoder Stuart <stuart.yoder-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+	-- Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
diff --git a/dtc-lexer.l b/dtc-lexer.l
index 494e342..73d190c 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -97,6 +97,12 @@ static int pop_input_file(void);
 			return DT_MEMRESERVE;
 		}
 
+<*>"/bits/"	{
+			DPRINT("Keyword: /bits/\n");
+			BEGIN_DEFAULT();
+			return DT_BITS;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
diff --git a/dtc-parser.y b/dtc-parser.y
index bc05a24..cd48c86 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -45,8 +45,12 @@ static unsigned char eval_char_literal(const char *s);
 	uint8_t byte;
 	struct data data;
 
+	struct {
+		struct data	data;
+		int		bits;
+	} celllist;
+
 	uint64_t addr;
-	cell_t cell;
 	struct property *prop;
 	struct property *proplist;
 	struct node *node;
@@ -56,6 +60,7 @@ static unsigned char eval_char_literal(const char *s);
 
 %token DT_V1
 %token DT_MEMRESERVE
+%token DT_BITS
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
 %token <literal> DT_CHAR_LITERAL
@@ -71,8 +76,7 @@ static unsigned char eval_char_literal(const char *s);
 %type <re> memreserve
 %type <re> memreserves
 %type <addr> addr
-%type <data> celllist
-%type <cell> cellval
+%type <celllist> celllistprefix
 %type <data> bytestring
 %type <prop> propdef
 %type <proplist> proplist
@@ -182,9 +186,9 @@ propdata:
 		{
 			$$ = data_merge($1, $2);
 		}
-	| propdataprefix '<' celllist '>'
+	| propdataprefix celllistprefix '>'
 		{
-			$$ = data_merge($1, $3);
+			$$ = data_merge($1, $2.data);
 		}
 	| propdataprefix '[' bytestring ']'
 		{
@@ -242,34 +246,53 @@ propdataprefix:
 		}
 	;
 
-celllist:
-	  /* empty */
+celllistprefix:
+	DT_BITS DT_LITERAL '<'
 		{
-			$$ = empty_data;
+			$$.data = empty_data;
+			$$.bits = eval_literal($2, 0, 7);
+
+			if (($$.bits !=  8) &&
+			    ($$.bits != 16) &&
+			    ($$.bits != 32) &&
+			    ($$.bits != 64))
+			{
+				print_error("Only 8, 16, 32 and 64-bit cell"
+					    " sizes are currently supported");
+				$$.bits = 32;
+			}
 		}
-	| celllist cellval
+	| '<'
 		{
-			$$ = data_append_cell($1, $2);
+			$$.data = empty_data;
+			$$.bits = 32;
 		}
-	| celllist DT_REF
+	| celllistprefix DT_LITERAL
 		{
-			$$ = data_append_cell(data_add_marker($1, REF_PHANDLE,
-							      $2), -1);
+			uint64_t val = eval_literal($2, 0, $1.bits);
+
+			$$.data = data_append_integer($1.data, val, $1.bits);
 		}
-	| celllist DT_LABEL
+	| celllistprefix DT_CHAR_LITERAL
 		{
-			$$ = data_add_marker($1, LABEL, $2);
-		}
-	;
+			uint64_t val = eval_char_literal($2);
 
-cellval:
-	  DT_LITERAL
+			$$.data = data_append_integer($1.data, val, $1.bits);
+		}
+	| celllistprefix DT_REF
 		{
-			$$ = eval_literal($1, 0, 32);
+			uint64_t val = ~0ULL >> (64 - $1.bits);
+
+			if ($1.bits != 32)
+				print_error("References only allowed in 32-bit"
+					    " cell lists");
+
+			$$.data = data_add_marker($1.data, REF_PHANDLE, $2);
+			$$.data = data_append_integer($$.data, val, $1.bits);
 		}
-	| DT_CHAR_LITERAL
+	| celllistprefix DT_LABEL
 		{
-			$$ = eval_char_literal($1);
+			$$.data = data_add_marker($1.data, LABEL, $2);
 		}
 	;
 
diff --git a/tests/.gitignore b/tests/.gitignore
index a3e9bd1..9e062c3 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -40,6 +40,7 @@
 /set_name
 /setprop
 /setprop_inplace
+/sized_cells
 /string_escapes
 /subnode_offset
 /supernode_atdepth_offset
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index cae8390..215a8c5 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -6,6 +6,7 @@ LIB_TESTS_L = get_mem_rsv \
 	node_check_compatible node_offset_by_compatible \
 	get_alias \
 	char_literal \
+	sized_cells \
 	notfound \
 	setprop_inplace nop_property nop_node \
 	sw_tree1 \
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index e2c3046..da6f970 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -209,6 +209,9 @@ dtc_tests () {
     run_dtc_test -I dts -O dtb -o dtc_char_literal.test.dtb char_literal.dts
     run_test char_literal dtc_char_literal.test.dtb
 
+    run_dtc_test -I dts -O dtb -o dtc_sized_cells.test.dtb sized_cells.dts
+    run_test sized_cells dtc_sized_cells.test.dtb
+
     run_dtc_test -I dts -O dtb -o dtc_extra-terminating-null.test.dtb extra-terminating-null.dts
     run_test extra-terminating-null dtc_extra-terminating-null.test.dtb
 
diff --git a/tests/sized_cells.c b/tests/sized_cells.c
new file mode 100644
index 0000000..847ec96
--- /dev/null
+++ b/tests/sized_cells.c
@@ -0,0 +1,84 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for variable sized cells in dtc
+ * Copyright (C) 2006 David Gibson, IBM Corporation.
+ * Copyright (C) 2011 The Chromium Authors. All rights reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+static void check_compare_properties(void *fdt,
+				     char const *name_one,
+				     char const *name_two)
+{
+	const void *propval;
+	int proplen;
+
+	propval = fdt_getprop(fdt, 0, name_one, &proplen);
+
+	if (!propval)
+		FAIL("fdt_getprop(\"%s\"): %s",
+		     name_one,
+		     fdt_strerror(proplen));
+
+	check_getprop(fdt, 0, name_two, proplen, propval);
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+	uint8_t expected_8[6] = {TEST_CHAR1,
+				 TEST_CHAR2,
+				 TEST_CHAR3,
+				 TEST_CHAR4,
+				 TEST_CHAR5,
+				 TEST_VALUE_1 >> 24};
+	uint16_t expected_16[6];
+	uint32_t expected_32[6];
+	uint64_t expected_64[6];
+	int i;
+
+	for (i = 0; i < 5; ++i) {
+		expected_16[i] = cpu_to_fdt16(expected_8[i]);
+		expected_32[i] = cpu_to_fdt32(expected_8[i]);
+		expected_64[i] = cpu_to_fdt64(expected_8[i]);
+	}
+
+	expected_16[5] = cpu_to_fdt16(TEST_VALUE_1 >> 16);
+	expected_32[5] = cpu_to_fdt32(TEST_VALUE_1);
+	expected_64[5] = cpu_to_fdt64(TEST_ADDR_1);
+
+	test_init(argc, argv);
+	fdt = load_blob_arg(argc, argv);
+
+	check_getprop(fdt, 0, "cells-8b", sizeof(expected_8), expected_8);
+	check_getprop(fdt, 0, "cells-16b", sizeof(expected_16), expected_16);
+	check_getprop(fdt, 0, "cells-32b", sizeof(expected_32), expected_32);
+	check_getprop(fdt, 0, "cells-64b", sizeof(expected_64), expected_64);
+
+	check_compare_properties(fdt, "cells-one-16b", "cells-one-32b");
+
+	PASS();
+}
diff --git a/tests/sized_cells.dts b/tests/sized_cells.dts
new file mode 100644
index 0000000..efea9f5
--- /dev/null
+++ b/tests/sized_cells.dts
@@ -0,0 +1,11 @@
+/dts-v1/;
+
+/ {
+	cells-8b = /bits/ 8 <'\r' 'b' '\0' '\'' '\xff' 0xde>;
+	cells-16b = /bits/ 16 <'\r' 'b' '\0' '\'' '\xff' 0xdead>;
+	cells-32b = /bits/ 32 <'\r' 'b' '\0' '\'' '\xff' 0xdeadbeef>;
+	cells-64b = /bits/ 64 <'\r' 'b' '\0' '\'' '\xff' 0xdeadbeef00000000>;
+
+	cells-one-16b = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
+	cells-one-32b = <0x12345678 0x0000ffff>;
+};
-- 
1.7.3.1

Re: [PATCH v3 3/3] dtc: Add support for variable sized cells

[David Gibson]

On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
> Cells of size 8, 16, 32, and 64 bits are supported.  The new
> /bits/ syntax was selected so as to not pollute the reserved
> keyword space with uint8/uint16/... type names.
> 
> With this patch the following property assignment:
> 
>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
> 
> is equivalent to:
> 
>     property = <0x12345678 0x0000ffff>;
> 
> It is now also possible to directly specify a 64 bit literal in a
> cell list using:
> 
>     property = /bits/ 64 <0xdeadbeef00000000>;
> 
> It is an error to attempt to store a literal into a cell that is too
> small to hold the literal, and the compiler will generate an error
> when it detects this.  For instance:
> 
>     property = /bits/ 8 <256>;
> 
> Will fail to compile.  It is also an error to attempt to place a
> reference in a non 32-bit cell.

So, there's one small but serious error left in the patch, see below.

Other than that, only two things concern me:

	1) Exactly what to call the keyword.  /bits/ is better than
/size/, but I'd still like to stew a bit on it to see if we can come
up with anything better.

	2) In the documentation and internal naming, we're using
the term "cell" ambiguously.  In the discussion of this extension
we've been using "cell" to mean one array element, which is fair
enough since it's more-or-less standard CS terminology.  However, in
previous FDT and OF terminology "cell" refers to an exactly 32-bit
quantity [well, to be exact OF old-timers say this isn't strictly
right in old OF-ese, but it's close enough to meaning that for most
purposes].
	So, I think we need to find a different name for array cells
to clarify the terminology.  "elements" maybe?

[snip]
> +	| celllistprefix DT_REF
>  		{
> -			$$ = eval_literal($1, 0, 32);
> +			uint64_t val = ~0ULL >> (64 - $1.bits);
> +
> +			if ($1.bits != 32)
> +				print_error("References only allowed in 32-bit"
> +					    " cell lists");
> +
> +			$$.data = data_add_marker($1.data, REF_PHANDLE, $2);
> +			$$.data = data_append_integer($$.data, val, $1.bits);

Uh, so, this is actually worse than what you had before.  Remember
print_error() just prints an error, without aborting the parse.  So
now, if bits != 32 it will print the error, then add a phandle marker
*and* a bits-sized -1.  So later, when we go through and fix up the
REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
which could overrun the end of the data if the element size is less
than 32.

So, basically the data_add_marker() needs to be in an else clause.

-- 
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 v3 3/3] dtc: Add support for variable sized cells

[Anton Staaf]

On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
>> Cells of size 8, 16, 32, and 64 bits are supported.  The new
>> /bits/ syntax was selected so as to not pollute the reserved
>> keyword space with uint8/uint16/... type names.
>>
>> With this patch the following property assignment:
>>
>>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>>
>> is equivalent to:
>>
>>     property = <0x12345678 0x0000ffff>;
>>
>> It is now also possible to directly specify a 64 bit literal in a
>> cell list using:
>>
>>     property = /bits/ 64 <0xdeadbeef00000000>;
>>
>> It is an error to attempt to store a literal into a cell that is too
>> small to hold the literal, and the compiler will generate an error
>> when it detects this.  For instance:
>>
>>     property = /bits/ 8 <256>;
>>
>> Will fail to compile.  It is also an error to attempt to place a
>> reference in a non 32-bit cell.
>
> So, there's one small but serious error left in the patch, see below.
>
> Other than that, only two things concern me:
>
>        1) Exactly what to call the keyword.  /bits/ is better than
> /size/, but I'd still like to stew a bit on it to see if we can come
> up with anything better.
>
>        2) In the documentation and internal naming, we're using
> the term "cell" ambiguously.  In the discussion of this extension
> we've been using "cell" to mean one array element, which is fair
> enough since it's more-or-less standard CS terminology.  However, in
> previous FDT and OF terminology "cell" refers to an exactly 32-bit
> quantity [well, to be exact OF old-timers say this isn't strictly
> right in old OF-ese, but it's close enough to meaning that for most
> purposes].
>        So, I think we need to find a different name for array cells
> to clarify the terminology.  "elements" maybe?
>
> [snip]
>> +     | celllistprefix DT_REF
>>               {
>> -                     $$ = eval_literal($1, 0, 32);
>> +                     uint64_t val = ~0ULL >> (64 - $1.bits);
>> +
>> +                     if ($1.bits != 32)
>> +                             print_error("References only allowed in 32-bit"
>> +                                         " cell lists");
>> +
>> +                     $$.data = data_add_marker($1.data, REF_PHANDLE, $2);
>> +                     $$.data = data_append_integer($$.data, val, $1.bits);
>
> Uh, so, this is actually worse than what you had before.  Remember
> print_error() just prints an error, without aborting the parse.  So
> now, if bits != 32 it will print the error, then add a phandle marker
> *and* a bits-sized -1.  So later, when we go through and fix up the
> REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
> which could overrun the end of the data if the element size is less
> than 32.
>
> So, basically the data_add_marker() needs to be in an else clause.

Yup, you're right.  I should first ask, I couldn't figure out how to write
a test that tests parse error cases like this.  The closest I found was the
check tests.  But they are specific to the check messages.  Is there a
good example of a parse error test?  Or should I create a new type of test?

I'll fix this up today.

Thanks,
    Anton

> --
> 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 v3 3/3] dtc: Add support for variable sized cells

[David Gibson]

On Mon, Sep 26, 2011 at 10:15:48AM -0700, Anton Staaf wrote:
> On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
[snip]
> > Uh, so, this is actually worse than what you had before.  Remember
> > print_error() just prints an error, without aborting the parse.  So
> > now, if bits != 32 it will print the error, then add a phandle marker
> > *and* a bits-sized -1.  So later, when we go through and fix up the
> > REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
> > which could overrun the end of the data if the element size is less
> > than 32.
> >
> > So, basically the data_add_marker() needs to be in an else clause.
> 
> Yup, you're right.  I should first ask, I couldn't figure out how to write
> a test that tests parse error cases like this.  The closest I found was the
> check tests.  But they are specific to the check messages.  Is there a
> good example of a parse error test?  Or should I create a new type of test?

Yeah, I don't think we really have any tests like this.  In general
I've been less concerned about testing the error paths than the
working path, though testing error paths as well is not a bad idea.

So feel free to make a new test if you can think up one that makes
sense.  A simple "doesn't crash" test would be a start, that doesn't
care about the program's results, as long as it isn't killed by a
signal.  Actually testing that an error message is generated would
require more work though.

-- 
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 v3 3/3] dtc: Add support for variable sized cells

[Anton Staaf]

On Mon, Sep 26, 2011 at 5:55 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Sep 26, 2011 at 10:15:48AM -0700, Anton Staaf wrote:
>> On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
> [snip]
>> > Uh, so, this is actually worse than what you had before.  Remember
>> > print_error() just prints an error, without aborting the parse.  So
>> > now, if bits != 32 it will print the error, then add a phandle marker
>> > *and* a bits-sized -1.  So later, when we go through and fix up the
>> > REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
>> > which could overrun the end of the data if the element size is less
>> > than 32.
>> >
>> > So, basically the data_add_marker() needs to be in an else clause.
>>
>> Yup, you're right.  I should first ask, I couldn't figure out how to write
>> a test that tests parse error cases like this.  The closest I found was the
>> check tests.  But they are specific to the check messages.  Is there a
>> good example of a parse error test?  Or should I create a new type of test?
>
> Yeah, I don't think we really have any tests like this.  In general
> I've been less concerned about testing the error paths than the
> working path, though testing error paths as well is not a bad idea.
>
> So feel free to make a new test if you can think up one that makes
> sense.  A simple "doesn't crash" test would be a start, that doesn't
> care about the program's results, as long as it isn't killed by a
> signal.  Actually testing that an error message is generated would
> require more work though.

I was thinking of a "does crash" test.  One that we can use to test that
incorrectly formatted or semantically incorrect dts files generate the
appropriate dtc error messages.  I'll look into adding such a test.  But
I think I'll make that a separate patch set.

Ahh, I think I just grokked what you want the "doesn't crash" test for.
It would be used to run a test that hits a syntax error, but ensures
that the continued execution past that point doesn't cause a segfault
or other error.  I'll see what I can do about creating a test harness that
checks both that a specific error was emitted and that the dtc didn't
crash as a result of the bad input data.

Thanks,
    Anton

> --
> 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 v3 3/3] dtc: Add support for variable sized cells

[Anton Staaf]

On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
>> Cells of size 8, 16, 32, and 64 bits are supported.  The new
>> /bits/ syntax was selected so as to not pollute the reserved
>> keyword space with uint8/uint16/... type names.
>>
>> With this patch the following property assignment:
>>
>>     property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>>
>> is equivalent to:
>>
>>     property = <0x12345678 0x0000ffff>;
>>
>> It is now also possible to directly specify a 64 bit literal in a
>> cell list using:
>>
>>     property = /bits/ 64 <0xdeadbeef00000000>;
>>
>> It is an error to attempt to store a literal into a cell that is too
>> small to hold the literal, and the compiler will generate an error
>> when it detects this.  For instance:
>>
>>     property = /bits/ 8 <256>;
>>
>> Will fail to compile.  It is also an error to attempt to place a
>> reference in a non 32-bit cell.
>
> So, there's one small but serious error left in the patch, see below.
>
> Other than that, only two things concern me:
>
>        1) Exactly what to call the keyword.  /bits/ is better than
> /size/, but I'd still like to stew a bit on it to see if we can come
> up with anything better.

Sounds good.  I have a slight preference for /bits/ over something like
/element-bits/ or /elementbits/.  But could be convinced if that was
preferred.

>        2) In the documentation and internal naming, we're using
> the term "cell" ambiguously.  In the discussion of this extension
> we've been using "cell" to mean one array element, which is fair
> enough since it's more-or-less standard CS terminology.  However, in
> previous FDT and OF terminology "cell" refers to an exactly 32-bit
> quantity [well, to be exact OF old-timers say this isn't strictly
> right in old OF-ese, but it's close enough to meaning that for most
> purposes].
>        So, I think we need to find a different name for array cells
> to clarify the terminology.  "elements" maybe?

The documentation all talks about arrays of cells.  Where as the source code
always calls them lists of cells.  I agree that calling them cells once they are
no longer fixed to 32-bits should be avoided.  One solution would be to rename
the non-terminal and semantic variables in the parser from celllistprefix and
celllist to arrayprefix and array respectively.  I would also change the error
message about the size not being one of the supported sizes to say "element"
instead of "cell".

I could likewise clean up the documentation so that the only mentions of the
cell type were consistent with 32-bit cells.  In fact, reading over the
documentation, it looks like a lot of the references to "cell array"
can be turned
into just "array", and the remaining references to "cell" can become "element".

How does this sound?

> [snip]
>> +     | celllistprefix DT_REF
>>               {
>> -                     $$ = eval_literal($1, 0, 32);
>> +                     uint64_t val = ~0ULL >> (64 - $1.bits);
>> +
>> +                     if ($1.bits != 32)
>> +                             print_error("References only allowed in 32-bit"
>> +                                         " cell lists");
>> +
>> +                     $$.data = data_add_marker($1.data, REF_PHANDLE, $2);
>> +                     $$.data = data_append_integer($$.data, val, $1.bits);
>
> Uh, so, this is actually worse than what you had before.  Remember
> print_error() just prints an error, without aborting the parse.  So
> now, if bits != 32 it will print the error, then add a phandle marker
> *and* a bits-sized -1.  So later, when we go through and fix up the
> REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
> which could overrun the end of the data if the element size is less
> than 32.
>
> So, basically the data_add_marker() needs to be in an else clause.

Done.

Thanks,
     Anton

> --
> 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 v3 3/3] dtc: Add support for variable sized cells

[David Gibson]

On Mon, Sep 26, 2011 at 01:48:31PM -0700, Anton Staaf wrote:
> On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
> >        1) Exactly what to call the keyword.  /bits/ is better than
> > /size/, but I'd still like to stew a bit on it to see if we can come
> > up with anything better.
> 
> Sounds good.  I have a slight preference for /bits/ over something like
> /element-bits/ or /elementbits/.  But could be convinced if that was
> preferred.

I'm certainly leaning towards /bits/ in preference to those
alternatives, for the time being at least.

> >        2) In the documentation and internal naming, we're using
> > the term "cell" ambiguously.  In the discussion of this extension
> > we've been using "cell" to mean one array element, which is fair
> > enough since it's more-or-less standard CS terminology.  However, in
> > previous FDT and OF terminology "cell" refers to an exactly 32-bit
> > quantity [well, to be exact OF old-timers say this isn't strictly
> > right in old OF-ese, but it's close enough to meaning that for most
> > purposes].
> >        So, I think we need to find a different name for array cells
> > to clarify the terminology.  "elements" maybe?
> 
> The documentation all talks about arrays of cells.  Where as the source code
> always calls them lists of cells.  I agree that calling them cells once they are
> no longer fixed to 32-bits should be avoided.  One solution would be to rename
> the non-terminal and semantic variables in the parser from celllistprefix and
> celllist to arrayprefix and array respectively.  I would also change the error
> message about the size not being one of the supported sizes to say "element"
> instead of "cell".

That sounds good.

> I could likewise clean up the documentation so that the only mentions of the
> cell type were consistent with 32-bit cells.  In fact, reading over the
> documentation, it looks like a lot of the references to "cell array"
> can be turned
> into just "array", and the remaining references to "cell" can become "element".
> 
> How does this sound?

Sounds good.  I'd perhaps add a explanatory note in there saying that
an array with element size 32-bits is a "list of cells", since many
existing bindings describe things in terms of x cells of this, y cells
of that and so forth.

-- 
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 v3 3/3] dtc: Add support for variable sized cells

[Anton Staaf]

On Mon, Sep 26, 2011 at 5:58 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Sep 26, 2011 at 01:48:31PM -0700, Anton Staaf wrote:
>> On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
>> >        1) Exactly what to call the keyword.  /bits/ is better than
>> > /size/, but I'd still like to stew a bit on it to see if we can come
>> > up with anything better.
>>
>> Sounds good.  I have a slight preference for /bits/ over something like
>> /element-bits/ or /elementbits/.  But could be convinced if that was
>> preferred.
>
> I'm certainly leaning towards /bits/ in preference to those
> alternatives, for the time being at least.
>
>> >        2) In the documentation and internal naming, we're using
>> > the term "cell" ambiguously.  In the discussion of this extension
>> > we've been using "cell" to mean one array element, which is fair
>> > enough since it's more-or-less standard CS terminology.  However, in
>> > previous FDT and OF terminology "cell" refers to an exactly 32-bit
>> > quantity [well, to be exact OF old-timers say this isn't strictly
>> > right in old OF-ese, but it's close enough to meaning that for most
>> > purposes].
>> >        So, I think we need to find a different name for array cells
>> > to clarify the terminology.  "elements" maybe?
>>
>> The documentation all talks about arrays of cells.  Where as the source code
>> always calls them lists of cells.  I agree that calling them cells once they are
>> no longer fixed to 32-bits should be avoided.  One solution would be to rename
>> the non-terminal and semantic variables in the parser from celllistprefix and
>> celllist to arrayprefix and array respectively.  I would also change the error
>> message about the size not being one of the supported sizes to say "element"
>> instead of "cell".
>
> That sounds good.

Done.

>> I could likewise clean up the documentation so that the only mentions of the
>> cell type were consistent with 32-bit cells.  In fact, reading over the
>> documentation, it looks like a lot of the references to "cell array"
>> can be turned
>> into just "array", and the remaining references to "cell" can become "element".
>>
>> How does this sound?
>
> Sounds good.  I'd perhaps add a explanatory note in there saying that
> an array with element size 32-bits is a "list of cells", since many
> existing bindings describe things in terms of x cells of this, y cells
> of that and so forth.

Done.

Thanks,
     Anton

> --
> 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
>