[PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[Andrei Ziureaev]

Currently, in yaml output, negative values are indistinguishable from
positive ones. Some bindings (for example
Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
values. If those binding are converted to yaml, dt-schema validation
wouldn't work with them.

The first patch is a mechanical change and shouldn't affect dtc's
behaviour.

The second one is the functional change. When applied, dts to dts and
dts to yaml conversions preserve the '-' sign in front of integers.

For now, in dts parsing, only the unary '-' operator creates negative
values (the other operators just set the 'is_negative' flag to false),
but it should be easy to add support for other operators if needed.

One issue is that there are two ways to format an array of bytes in dts:
'/bits/ 8 <...>' and '[...]'. Only the former supports negative
integers. But, in dts output, only the latter is used. Therefore, I
didn't include support for negative 8-bit values in dts output (I did
add support for them in yaml output). Some possible alternatives are:

- switch to the '/bits/ 8 <>' format in dts output

- only switch to the '/bits/ 8 <>' format if the array contains negative
  values

- add another marker to differentiate between the two formats

Thanks,
Andrei.

Andrei Ziureaev (3):
  dtc: Turn 'uint64_t integer' into a struct
  dtc: Preserve negative integers in yaml and dts output
  dtc: Add sign preservation tests

 dtc-lexer.l                           |  10 +-
 dtc-parser.y                          | 132 +++++++++++++++++++-------
 dtc.h                                 |  14 +++
 tests/run_tests.sh                    |   4 +-
 tests/sign-preservation-bytes.dt.yaml |   5 +
 tests/sign-preservation-bytes.dts     |   7 ++
 tests/sign-preservation.dt.yaml       |  20 ++++
 tests/sign-preservation.dts           |  28 ++++++
 treesource.c                          |  35 +++++--
 yamltree.c                            |  32 ++++---
 10 files changed, 225 insertions(+), 62 deletions(-)
 create mode 100644 tests/sign-preservation-bytes.dt.yaml
 create mode 100644 tests/sign-preservation-bytes.dts
 create mode 100644 tests/sign-preservation.dt.yaml
 create mode 100644 tests/sign-preservation.dts

-- 
2.17.1

[PATCH 1/3] dtc: Turn 'uint64_t integer' into a struct

[Andrei Ziureaev]

This allows adding support for negative integers in yaml and dts output.

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 dtc-lexer.l  |  10 ++--
 dtc-parser.y | 127 +++++++++++++++++++++++++++++++++++++--------------
 dtc.h        |   3 ++
 3 files changed, 102 insertions(+), 38 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index b3b7270300de..a8982bd4edf2 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -156,7 +156,9 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
 			DPRINT("Integer Literal: '%s'\n", yytext);
 
 			errno = 0;
-			yylval.integer = strtoull(yytext, &e, 0);
+			yylval.integer = (struct integer){
+				.i = strtoull(yytext, &e, 0)
+			};
 
 			if (*e && e[strspn(e, "UL")]) {
 				lexical_error("Bad integer literal '%s'",
@@ -180,9 +182,11 @@ static void PRINTF(1, 2) lexical_error(const char *fmt, ...);
 			d = data_copy_escape_string(yytext+1, yyleng-2);
 			if (d.len == 1) {
 				lexical_error("Empty character literal");
-				yylval.integer = 0;
+				yylval.integer = (struct integer){ 0 };
 			} else {
-				yylval.integer = (unsigned char)d.val[0];
+				yylval.integer = (struct integer){
+					.i = (unsigned char)d.val[0]
+				};
 
 				if (d.len > 2)
 					lexical_error("Character literal has %d"
diff --git a/dtc-parser.y b/dtc-parser.y
index 40dcf4f149da..d3924e382065 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -41,7 +41,7 @@ extern bool treesource_error;
 	struct node *node;
 	struct node *nodelist;
 	struct reserve_info *re;
-	uint64_t integer;
+	struct integer integer;
 	unsigned int flags;
 }
 
@@ -140,7 +140,7 @@ memreserves:
 memreserve:
 	  DT_MEMRESERVE integer_prim integer_prim ';'
 		{
-			$$ = build_reserve_entry($2, $3);
+			$$ = build_reserve_entry($2.i, $3.i);
 		}
 	| DT_LABEL memreserve
 		{
@@ -310,13 +310,13 @@ propdata:
 			FILE *f = srcfile_relative_open($4.val, NULL);
 			struct data d;
 
-			if ($6 != 0)
-				if (fseek(f, $6, SEEK_SET) != 0)
+			if ($6.i != 0)
+				if (fseek(f, $6.i, SEEK_SET) != 0)
 					die("Couldn't seek to offset %llu in \"%s\": %s",
-					    (unsigned long long)$6, $4.val,
+					    (unsigned long long)$6.i, $4.val,
 					    strerror(errno));
 
-			d = data_copy_file(f, $8);
+			d = data_copy_file(f, $8.i);
 
 			$$ = data_merge($1, d);
 			fclose(f);
@@ -358,7 +358,7 @@ arrayprefix:
 			unsigned long long bits;
 			enum markertype type = TYPE_UINT32;
 
-			bits = $2;
+			bits = $2.i;
 
 			switch (bits) {
 			case 8: type = TYPE_UINT8; break;
@@ -391,12 +391,12 @@ arrayprefix:
 				 * within the mask to one (i.e. | in the
 				 * mask), all bits are one.
 				 */
-				if (($2 > mask) && (($2 | mask) != -1ULL))
+				if (($2.i > mask) && (($2.i | mask) != -1ULL))
 					ERROR(&@2, "Value out of range for"
 					      " %d-bit array element", $1.bits);
 			}
 
-			$$.data = data_append_integer($1.data, $2, $1.bits);
+			$$.data = data_append_integer($1.data, $2.i, $1.bits);
 		}
 	| arrayprefix dt_ref
 		{
@@ -433,78 +433,126 @@ integer_expr:
 
 integer_trinary:
 	  integer_or
-	| integer_or '?' integer_expr ':' integer_trinary { $$ = $1 ? $3 : $5; }
+	| integer_or '?' integer_expr ':' integer_trinary { $$ = $1.i ? $3 : $5; }
 	;
 
 integer_or:
 	  integer_and
-	| integer_or DT_OR integer_and { $$ = $1 || $3; }
+	| integer_or DT_OR integer_and
+		{
+			$$ = (struct integer){ $1.i || $3.i };
+		}
 	;
 
 integer_and:
 	  integer_bitor
-	| integer_and DT_AND integer_bitor { $$ = $1 && $3; }
+	| integer_and DT_AND integer_bitor
+		{
+			$$ = (struct integer){ $1.i && $3.i };
+		}
 	;
 
 integer_bitor:
 	  integer_bitxor
-	| integer_bitor '|' integer_bitxor { $$ = $1 | $3; }
+	| integer_bitor '|' integer_bitxor
+		{
+			$$ = (struct integer){ $1.i | $3.i };
+		}
 	;
 
 integer_bitxor:
 	  integer_bitand
-	| integer_bitxor '^' integer_bitand { $$ = $1 ^ $3; }
+	| integer_bitxor '^' integer_bitand
+		{
+			$$ = (struct integer){ $1.i ^ $3.i };
+		}
 	;
 
 integer_bitand:
 	  integer_eq
-	| integer_bitand '&' integer_eq { $$ = $1 & $3; }
+	| integer_bitand '&' integer_eq
+		{
+			$$ = (struct integer){ $1.i & $3.i };
+		}
 	;
 
 integer_eq:
 	  integer_rela
-	| integer_eq DT_EQ integer_rela { $$ = $1 == $3; }
-	| integer_eq DT_NE integer_rela { $$ = $1 != $3; }
+	| integer_eq DT_EQ integer_rela
+		{
+			$$ = (struct integer){ $1.i == $3.i };
+		}
+	| integer_eq DT_NE integer_rela
+		{
+			$$ = (struct integer){ $1.i != $3.i };
+		}
 	;
 
 integer_rela:
 	  integer_shift
-	| integer_rela '<' integer_shift { $$ = $1 < $3; }
-	| integer_rela '>' integer_shift { $$ = $1 > $3; }
-	| integer_rela DT_LE integer_shift { $$ = $1 <= $3; }
-	| integer_rela DT_GE integer_shift { $$ = $1 >= $3; }
+	| integer_rela '<' integer_shift
+		{
+			$$ = (struct integer){ $1.i < $3.i };
+		}
+	| integer_rela '>' integer_shift
+		{
+			$$ = (struct integer){ $1.i > $3.i };
+		}
+	| integer_rela DT_LE integer_shift
+		{
+			$$ = (struct integer){ $1.i <= $3.i };
+		}
+	| integer_rela DT_GE integer_shift
+		{
+			$$ = (struct integer){ $1.i >= $3.i };
+		}
 	;
 
 integer_shift:
-	  integer_shift DT_LSHIFT integer_add { $$ = $1 << $3; }
-	| integer_shift DT_RSHIFT integer_add { $$ = $1 >> $3; }
+	  integer_shift DT_LSHIFT integer_add
+		{
+			$$ = (struct integer){ $1.i << $3.i };
+		}
+	| integer_shift DT_RSHIFT integer_add
+		{
+			$$ = (struct integer){ $1.i >> $3.i };
+		}
 	| integer_add
 	;
 
 integer_add:
-	  integer_add '+' integer_mul { $$ = $1 + $3; }
-	| integer_add '-' integer_mul { $$ = $1 - $3; }
+	  integer_add '+' integer_mul
+		{
+			$$ = (struct integer){ $1.i + $3.i };
+		}
+	| integer_add '-' integer_mul
+		{
+			$$ = (struct integer){ $1.i - $3.i };
+		}
 	| integer_mul
 	;
 
 integer_mul:
-	  integer_mul '*' integer_unary { $$ = $1 * $3; }
+	  integer_mul '*' integer_unary
+		{
+			$$ = (struct integer){ $1.i * $3.i };
+		}
 	| integer_mul '/' integer_unary
 		{
-			if ($3 != 0) {
-				$$ = $1 / $3;
+			if ($3.i != 0) {
+				$$ = (struct integer){ $1.i / $3.i };
 			} else {
 				ERROR(&@$, "Division by zero");
-				$$ = 0;
+				$$ = (struct integer){ 0 };
 			}
 		}
 	| integer_mul '%' integer_unary
 		{
-			if ($3 != 0) {
-				$$ = $1 % $3;
+			if ($3.i != 0) {
+				$$ = (struct integer){ $1.i % $3.i };
 			} else {
 				ERROR(&@$, "Division by zero");
-				$$ = 0;
+				$$ = (struct integer){ 0 };
 			}
 		}
 	| integer_unary
@@ -512,9 +560,18 @@ integer_mul:
 
 integer_unary:
 	  integer_prim
-	| '-' integer_unary { $$ = -$2; }
-	| '~' integer_unary { $$ = ~$2; }
-	| '!' integer_unary { $$ = !$2; }
+	| '-' integer_unary
+		{
+			$$ = (struct integer){ -$2.i };
+		}
+	| '~' integer_unary
+		{
+			$$ = (struct integer){ ~$2.i };
+		}
+	| '!' integer_unary
+		{
+			$$ = (struct integer){ !$2.i };
+		}
 	;
 
 bytestring:
diff --git a/dtc.h b/dtc.h
index a08f4159cd03..fb1ade37b715 100644
--- a/dtc.h
+++ b/dtc.h
@@ -116,6 +116,9 @@ struct data {
 	struct marker *markers;
 };
 
+struct integer {
+	uint64_t i;
+};
 
 #define empty_data ((struct data){ 0 /* all .members = 0 or NULL */ })
 
-- 
2.17.1

[PATCH 2/3] dtc: Preserve negative integers in yaml and dts output

[Andrei Ziureaev]

The dts format supports signed integers, but dts to dts and dts to yaml
conversions don't preserve the sign on output. This is a problem for
doing validation with schema that define a signed type.

Preserve the '-' sign in front of negative values in dts and yaml
output.

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 dtc-parser.y |  9 +++++++--
 dtc.h        | 11 +++++++++++
 treesource.c | 35 ++++++++++++++++++++++++++---------
 yamltree.c   | 32 +++++++++++++++++++-------------
 4 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/dtc-parser.y b/dtc-parser.y
index d3924e382065..4b7a9eaa65ff 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -381,6 +381,8 @@ arrayprefix:
 		}
 	| arrayprefix integer_prim
 		{
+			struct data data = $1.data;
+
 			if ($1.bits < 64) {
 				uint64_t mask = (1ULL << $1.bits) - 1;
 				/*
@@ -396,7 +398,10 @@ arrayprefix:
 					      " %d-bit array element", $1.bits);
 			}
 
-			$$.data = data_append_integer($1.data, $2.i, $1.bits);
+			if ($2.is_negative)
+				data = data_add_marker(data, TYPE_NEGATIVE, NULL);
+
+			$$.data = data_append_integer(data, $2.i, $1.bits);
 		}
 	| arrayprefix dt_ref
 		{
@@ -562,7 +567,7 @@ integer_unary:
 	  integer_prim
 	| '-' integer_unary
 		{
-			$$ = (struct integer){ -$2.i };
+			$$ = (struct integer){ -$2.i, !$2.is_negative };
 		}
 	| '~' integer_unary
 		{
diff --git a/dtc.h b/dtc.h
index fb1ade37b715..e1ae3a785b73 100644
--- a/dtc.h
+++ b/dtc.h
@@ -95,6 +95,7 @@ enum markertype {
 	REF_PHANDLE,
 	REF_PATH,
 	LABEL,
+	TYPE_NEGATIVE,
 	TYPE_UINT8,
 	TYPE_UINT16,
 	TYPE_UINT32,
@@ -118,6 +119,7 @@ struct data {
 
 struct integer {
 	uint64_t i;
+	bool is_negative;
 };
 
 #define empty_data ((struct data){ 0 /* all .members = 0 or NULL */ })
@@ -128,6 +130,15 @@ struct integer {
 	for_each_marker(m) \
 		if ((m)->type == (t))
 
+static inline bool has_marker_of_type_at_offset(struct marker *m,
+						enum markertype type, int off)
+{
+	for_each_marker_of_type(m, type)
+		if (m->offset == off)
+			return true;
+	return false;
+}
+
 size_t type_marker_length(struct marker *m);
 
 void data_free(struct data d);
diff --git a/treesource.c b/treesource.c
index 2acb920d7775..915cd6d6cf75 100644
--- a/treesource.c
+++ b/treesource.c
@@ -99,24 +99,39 @@ static void write_propval_string(FILE *f, const char *s, size_t len)
 	fprintf(f, "\"");
 }
 
-static void write_propval_int(FILE *f, const char *p, size_t len, size_t width)
+static void write_propval_int(FILE *f, struct marker *markers,
+			      const char *p, size_t len, size_t width)
 {
+	int start_offset = markers->offset;
+	const char *start = p;
 	const char *end = p + len;
 	assert(len % width == 0);
 
 	for (; p < end; p += width) {
+		int off = start_offset + (int)(p - start);
+		bool is_neg = has_marker_of_type_at_offset(markers, TYPE_NEGATIVE, off);
+
 		switch (width) {
 		case 1:
 			fprintf(f, "%02"PRIx8, *(const uint8_t*)p);
 			break;
 		case 2:
-			fprintf(f, "0x%02"PRIx16, dtb_ld16(p));
+			if (is_neg)
+				fprintf(f, "(-0x%02"PRIx16")", (uint16_t)-dtb_ld16(p));
+			else
+				fprintf(f, "0x%02"PRIx16, dtb_ld16(p));
 			break;
 		case 4:
-			fprintf(f, "0x%02"PRIx32, dtb_ld32(p));
+			if (is_neg)
+				fprintf(f, "(-0x%02"PRIx32")", -dtb_ld32(p));
+			else
+				fprintf(f, "0x%02"PRIx32, dtb_ld32(p));
 			break;
 		case 8:
-			fprintf(f, "0x%02"PRIx64, dtb_ld64(p));
+			if (is_neg)
+				fprintf(f, "(-0x%02"PRIx64")", -dtb_ld64(p));
+			else
+				fprintf(f, "0x%02"PRIx64, dtb_ld64(p));
 			break;
 		}
 		if (p + width < end)
@@ -196,6 +211,7 @@ static enum markertype guess_value_type(struct property *prop)
 static void write_propval(FILE *f, struct property *prop)
 {
 	size_t len = prop->val.len;
+	size_t last_type_off = 0;
 	struct marker *m = prop->val.markers;
 	struct marker dummy_marker;
 	enum markertype emit_type = TYPE_NONE;
@@ -231,11 +247,12 @@ static void write_propval(FILE *f, struct property *prop)
 		const char *p = &prop->val.val[m->offset];
 
 		if (has_data_type_information(m)) {
+			last_type_off = m->offset;
 			emit_type = m->type;
 			fprintf(f, " %s", delim_start[emit_type]);
 		} else if (m->type == LABEL)
 			fprintf(f, " %s:", m->ref);
-		else if (m->offset)
+		else if (m->offset != last_type_off)
 			fputc(' ', f);
 
 		if (emit_type == TYPE_NONE) {
@@ -245,19 +262,19 @@ static void write_propval(FILE *f, struct property *prop)
 
 		switch(emit_type) {
 		case TYPE_UINT16:
-			write_propval_int(f, p, chunk_len, 2);
+			write_propval_int(f, m, p, chunk_len, 2);
 			break;
 		case TYPE_UINT32:
-			write_propval_int(f, p, chunk_len, 4);
+			write_propval_int(f, m, p, chunk_len, 4);
 			break;
 		case TYPE_UINT64:
-			write_propval_int(f, p, chunk_len, 8);
+			write_propval_int(f, m, p, chunk_len, 8);
 			break;
 		case TYPE_STRING:
 			write_propval_string(f, p, chunk_len);
 			break;
 		default:
-			write_propval_int(f, p, chunk_len, 1);
+			write_propval_int(f, m, p, chunk_len, 1);
 		}
 
 		if (chunk_len == data_len) {
diff --git a/yamltree.c b/yamltree.c
index 4e93c12dc658..1710caebc111 100644
--- a/yamltree.c
+++ b/yamltree.c
@@ -51,29 +51,35 @@ static void yaml_propval_int(yaml_emitter_t *emitter, struct marker *markers, ch
 
 	for (off = 0; off < len; off += width) {
 		char buf[32];
-		struct marker *m;
 		bool is_phandle = false;
+		bool is_neg = has_marker_of_type_at_offset(markers, TYPE_NEGATIVE, start_offset + off);
 
 		switch(width) {
 		case 1:
-			sprintf(buf, "0x%"PRIx8, *(uint8_t*)(data + off));
+			if (is_neg)
+				sprintf(buf, "-0x%"PRIx8, (uint8_t)-*(uint8_t*)(data + off));
+			else
+				sprintf(buf, "0x%"PRIx8, *(uint8_t*)(data + off));
 			break;
 		case 2:
-			sprintf(buf, "0x%"PRIx16, dtb_ld16(data + off));
+			if (is_neg)
+				sprintf(buf, "-0x%"PRIx16, (uint16_t)-dtb_ld16(data + off));
+			else
+				sprintf(buf, "0x%"PRIx16, dtb_ld16(data + off));
 			break;
 		case 4:
-			sprintf(buf, "0x%"PRIx32, dtb_ld32(data + off));
-			m = markers;
-			is_phandle = false;
-			for_each_marker_of_type(m, REF_PHANDLE) {
-				if (m->offset == (start_offset + off)) {
-					is_phandle = true;
-					break;
-				}
-			}
+			if (is_neg)
+				sprintf(buf, "-0x%"PRIx32, -dtb_ld32(data + off));
+			else
+				sprintf(buf, "0x%"PRIx32, dtb_ld32(data + off));
+
+			is_phandle = has_marker_of_type_at_offset(markers, REF_PHANDLE, start_offset + off);
 			break;
 		case 8:
-			sprintf(buf, "0x%"PRIx64, dtb_ld64(data + off));
+			if (is_neg)
+				sprintf(buf, "-0x%"PRIx64, -dtb_ld64(data + off));
+			else
+				sprintf(buf, "0x%"PRIx64, dtb_ld64(data + off));
 			break;
 		}
 
-- 
2.17.1

[PATCH 3/3] dtc: Add sign preservation tests

[Andrei Ziureaev]

Test whether dts and yaml outputs preserve the signs of integers.

Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
---
 tests/run_tests.sh                    |  4 ++--
 tests/sign-preservation-bytes.dt.yaml |  5 +++++
 tests/sign-preservation-bytes.dts     |  7 +++++++
 tests/sign-preservation.dt.yaml       | 20 +++++++++++++++++++
 tests/sign-preservation.dts           | 28 +++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 2 deletions(-)
 create mode 100644 tests/sign-preservation-bytes.dt.yaml
 create mode 100644 tests/sign-preservation-bytes.dts
 create mode 100644 tests/sign-preservation.dt.yaml
 create mode 100644 tests/sign-preservation.dts

diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 294585b991e2..7745e5178995 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -619,7 +619,7 @@ dtc_tests () {
     done
 
     # Check -Odts preserving type information
-    for tree in type-preservation.dts; do
+    for tree in type-preservation.dts sign-preservation.dts; do
         run_dtc_test -I dts -O dts -o $tree.test.dts "$SRCDIR/$tree"
         run_dtc_test -I dts -O dts $tree.test.dts
         run_wrap_test cmp "$SRCDIR/$tree" $tree.test.dts
@@ -633,7 +633,7 @@ dtc_tests () {
 
     # Check -Oyaml output
     if ! $no_yaml; then
-            for tree in type-preservation; do
+            for tree in type-preservation sign-preservation sign-preservation-bytes; do
                 run_dtc_test -I dts -O yaml -o $tree.test.dt.yaml "$SRCDIR/$tree.dts"
                 run_wrap_test cmp "$SRCDIR/$tree.dt.yaml" $tree.test.dt.yaml
             done
diff --git a/tests/sign-preservation-bytes.dt.yaml b/tests/sign-preservation-bytes.dt.yaml
new file mode 100644
index 000000000000..6b807e345fb6
--- /dev/null
+++ b/tests/sign-preservation-bytes.dt.yaml
@@ -0,0 +1,5 @@
+---
+- int8-neg: [!u8 [-0xff]]
+  int8-array: [!u8 [-0x1, 0x2, -0xfe, 0xff]]
+  int8-array-neg: [!u8 [-0x1, -0x2, -0xfe, -0xff]]
+...
diff --git a/tests/sign-preservation-bytes.dts b/tests/sign-preservation-bytes.dts
new file mode 100644
index 000000000000..8d0bccd0fb0e
--- /dev/null
+++ b/tests/sign-preservation-bytes.dts
@@ -0,0 +1,7 @@
+/dts-v1/;
+
+/ {
+	int8-neg = /bits/ 8 <(-0xff)>;
+	int8-array = /bits/ 8 <(-0x01) 0x02 (-0xfe) 0xff>;
+	int8-array-neg = /bits/ 8 <(-0x01) (-0x02) (-0xfe) (-0xff)>;
+};
diff --git a/tests/sign-preservation.dt.yaml b/tests/sign-preservation.dt.yaml
new file mode 100644
index 000000000000..7a701f84b0a0
--- /dev/null
+++ b/tests/sign-preservation.dt.yaml
@@ -0,0 +1,20 @@
+---
+- '#address-cells': [[0x1]]
+  '#size-cells': [[0x0]]
+  subnode@1:
+    reg: [[0x1]]
+    int-neg: [[-0x1]]
+    int-matrix: [[0x0, -0x1], [0xfffffffe, -0xffffffff]]
+    int-matrix-neg: [[-0x1, -0x2], [-0xfffffffe, -0xffffffff]]
+    int16-neg: [!u16 [-0x3210]]
+    int16-array: [!u16 [-0x1234, 0x5678, 0x90ab, -0xcdef]]
+    int16-array-neg: [!u16 [-0x1234, -0x5678, -0x90ab, -0xcdef]]
+    int16-matrix: [!u16 [0x1234, -0x5678], [-0x90ab, 0xcdef]]
+    int16-matrix-neg: [!u16 [-0x1234, -0x5678], [-0x90ab, -0xcdef]]
+    int64-neg: [!u64 [-0xffffffffffffffff]]
+    int64-array: [!u64 [0x1, 0xffffffffffffffff, -0x1, -0xffffffffffffffff]]
+    int64-array-neg: [!u64 [-0x2, -0x8000000000000000]]
+    subsubnode:
+      subsubsubnode:
+        compatible: ["subsubsubnode1", [0x1234], [-0x1234], "subsubsubnode"]
+...
diff --git a/tests/sign-preservation.dts b/tests/sign-preservation.dts
new file mode 100644
index 000000000000..1d4b714628ed
--- /dev/null
+++ b/tests/sign-preservation.dts
@@ -0,0 +1,28 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <0x01>;
+	#size-cells = <0x00>;
+
+	sub1: subnode@1 {
+		reg = <0x01>;
+		int-neg = <(-0x01)>;
+		int-matrix = <0x00 (-0x01)>, int_matrix_label: <0xfffffffe (-0xffffffff)>;
+		int-matrix-neg = <(-0x01) (-0x02)>, int_matrix_neg_label: <(-0xfffffffe) (-0xffffffff)>;
+		int16-neg = /bits/ 16 <(-0x3210)>;
+		int16-array = /bits/ 16 <(-0x1234) 0x5678 0x90ab (-0xcdef)>;
+		int16-array-neg = /bits/ 16 <(-0x1234) (-0x5678) (-0x90ab) (-0xcdef)>;
+		int16-matrix = /bits/ 16 <0x1234 (-0x5678)>, <(-0x90ab) 0xcdef>;
+		int16-matrix-neg = /bits/ 16 <(-0x1234) (-0x5678)>, <(-0x90ab) (-0xcdef)>;
+		int64-neg = /bits/ 64 <(-0xffffffffffffffff)>;
+		int64-array = /bits/ 64 <0x01 0xffffffffffffffff (-0x01) (-0xffffffffffffffff)> int64_array_label_end:;
+		int64-array-neg = /bits/ 64 <(-0x02) (-0x8000000000000000)> int64_array_neg_label_end:;
+
+		subsub1: subsubnode {
+
+			subsubsub1: subsubsubnode {
+				compatible = "subsubsubnode1", <0x1234>, <(-0x1234)>, valuea: valueb: "subsubsubnode";
+			};
+		};
+	};
+};
-- 
2.17.1

Re: [PATCH 3/3] dtc: Add sign preservation tests

[Rob Herring]

On Fri, Jun 26, 2020 at 10:25 AM Andrei Ziureaev
<andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
>
> Test whether dts and yaml outputs preserve the signs of integers.
>
> Signed-off-by: Andrei Ziureaev <andrei.ziureaev-5wv7dgnIgG8@public.gmane.org>
> ---
>  tests/run_tests.sh                    |  4 ++--
>  tests/sign-preservation-bytes.dt.yaml |  5 +++++
>  tests/sign-preservation-bytes.dts     |  7 +++++++
>  tests/sign-preservation.dt.yaml       | 20 +++++++++++++++++++
>  tests/sign-preservation.dts           | 28 +++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+), 2 deletions(-)
>  create mode 100644 tests/sign-preservation-bytes.dt.yaml
>  create mode 100644 tests/sign-preservation-bytes.dts
>  create mode 100644 tests/sign-preservation.dt.yaml
>  create mode 100644 tests/sign-preservation.dts

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[Rob Herring]

On Fri, Jun 26, 2020 at 10:25 AM Andrei Ziureaev
<andrei.ziureaev-5wv7dgnIgG8@public.gmane.org> wrote:
>
> Currently, in yaml output, negative values are indistinguishable from
> positive ones. Some bindings (for example
> Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> values. If those binding are converted to yaml, dt-schema validation
> wouldn't work with them.
>
> The first patch is a mechanical change and shouldn't affect dtc's
> behaviour.
>
> The second one is the functional change. When applied, dts to dts and
> dts to yaml conversions preserve the '-' sign in front of integers.
>
> For now, in dts parsing, only the unary '-' operator creates negative
> values (the other operators just set the 'is_negative' flag to false),
> but it should be easy to add support for other operators if needed.
>
> One issue is that there are two ways to format an array of bytes in dts:
> '/bits/ 8 <...>' and '[...]'. Only the former supports negative
> integers. But, in dts output, only the latter is used. Therefore, I
> didn't include support for negative 8-bit values in dts output (I did
> add support for them in yaml output). Some possible alternatives are:
>
> - switch to the '/bits/ 8 <>' format in dts output

IMO, we should do this which effectively deprecates the [] syntax.

>
> - only switch to the '/bits/ 8 <>' format if the array contains negative
>   values
>
> - add another marker to differentiate between the two formats
>
> Thanks,
> Andrei.
>
> Andrei Ziureaev (3):
>   dtc: Turn 'uint64_t integer' into a struct
>   dtc: Preserve negative integers in yaml and dts output
>   dtc: Add sign preservation tests
>
>  dtc-lexer.l                           |  10 +-
>  dtc-parser.y                          | 132 +++++++++++++++++++-------
>  dtc.h                                 |  14 +++
>  tests/run_tests.sh                    |   4 +-
>  tests/sign-preservation-bytes.dt.yaml |   5 +
>  tests/sign-preservation-bytes.dts     |   7 ++
>  tests/sign-preservation.dt.yaml       |  20 ++++
>  tests/sign-preservation.dts           |  28 ++++++
>  treesource.c                          |  35 +++++--
>  yamltree.c                            |  32 ++++---
>  10 files changed, 225 insertions(+), 62 deletions(-)
>  create mode 100644 tests/sign-preservation-bytes.dt.yaml
>  create mode 100644 tests/sign-preservation-bytes.dts
>  create mode 100644 tests/sign-preservation.dt.yaml
>  create mode 100644 tests/sign-preservation.dts
>
> --
> 2.17.1
>

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[David Gibson]

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

On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> Currently, in yaml output, negative values are indistinguishable from
> positive ones. Some bindings (for example
> Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> values. If those binding are converted to yaml, dt-schema validation
> wouldn't work with them.

> The first patch is a mechanical change and shouldn't affect dtc's
> behaviour.
> 
> The second one is the functional change. When applied, dts to dts and
> dts to yaml conversions preserve the '-' sign in front of integers.
> 
> For now, in dts parsing, only the unary '-' operator creates negative
> values (the other operators just set the 'is_negative' flag to false),
> but it should be easy to add support for other operators if needed.

Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
fiddly work to preserve the sign only in a pretty narrow set of cases.
Basically it will only be reliably correct in the case of just
(-literal).  Something like (5-3) will still show as 0xfe rather than
-0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
looking "-0xfe".

I also don't see why this is vital to schema validation.  The
validator should know the binding, and will therefore know which
values are signed, and can appropriately treat 0xfe and -2 (or
equivalent at whatever width) as the same.

> One issue is that there are two ways to format an array of bytes in dts:
> '/bits/ 8 <...>' and '[...]'.

So, these are intended for different purposes.  If you want to think
of your bytes as a bunch of 8-bit numbers, the former is appropriate.
The [ ... ] notation is more intended to be a compact way of putting
in "untyped" binary data.  For example, a blob of information which
gets verbatim copied into a device without interpretation by the
driver would make sense to use this form.

> Only the former supports negative
> integers. But, in dts output, only the latter is used. Therefore, I
> didn't include support for negative 8-bit values in dts output (I did
> add support for them in yaml output). Some possible alternatives are:
> 
> - switch to the '/bits/ 8 <>' format in dts output
> 
> - only switch to the '/bits/ 8 <>' format if the array contains negative
>   values
> 
> - add another marker to differentiate between the two formats

I'd be ok with this, given the distinction above.  This is similar to
putting different for markers for say "abc" and <0x61626300> which
likewise represent the same bytes.

-- 
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: 833 bytes --]

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[Rob Herring]

On Sun, Jul 5, 2020 at 2:59 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> > Currently, in yaml output, negative values are indistinguishable from
> > positive ones. Some bindings (for example
> > Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> > values. If those binding are converted to yaml, dt-schema validation
> > wouldn't work with them.
>
> > The first patch is a mechanical change and shouldn't affect dtc's
> > behaviour.
> >
> > The second one is the functional change. When applied, dts to dts and
> > dts to yaml conversions preserve the '-' sign in front of integers.
> >
> > For now, in dts parsing, only the unary '-' operator creates negative
> > values (the other operators just set the 'is_negative' flag to false),
> > but it should be easy to add support for other operators if needed.
>
> Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
> fiddly work to preserve the sign only in a pretty narrow set of cases.
> Basically it will only be reliably correct in the case of just
> (-literal).  Something like (5-3) will still show as 0xfe rather than
> -0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
> looking "-0xfe".

It's a narrow set of cases if you assume an equal distribution of
expressions, but I'd bet you'd be hard pressed to really find examples
beyond (-literal).

But I guess at least the above examples could be supported. We'd have
to assume that if there's any subtraction, then types are signed and
we carry that thru. And perhaps give up and revert to unsigned if
there's anything more complex (signed values plus logic ops). In that
case, rather than tracking 'is_negative', we'd track is_signed and
then the output side would have to check that plus the data value to
output a negative.

> I also don't see why this is vital to schema validation.  The
> validator should know the binding, and will therefore know which
> values are signed, and can appropriately treat 0xfe and -2 (or
> equivalent at whatever width) as the same.

We could do that to some extent. The type is abstracted from the
binding and it just specifies 'int8' for example. That could be
internally set to { maximum: 0xff, minimum: 0 }. This works until you
want to further constrain the values for the specific bindings (e.g. {
enum: [ -1, -2, 127 ]}). We could process the schemas to find every
negative number and transform them to an unsigned value, but this gets
messy and fragile and is something I want to minimize.


> > One issue is that there are two ways to format an array of bytes in dts:
> > '/bits/ 8 <...>' and '[...]'.
>
> So, these are intended for different purposes.  If you want to think
> of your bytes as a bunch of 8-bit numbers, the former is appropriate.
> The [ ... ] notation is more intended to be a compact way of putting
> in "untyped" binary data.  For example, a blob of information which
> gets verbatim copied into a device without interpretation by the
> driver would make sense to use this form.

That probably means we should have a schema type for [...], but so far
I haven't seen a need.

> > Only the former supports negative
> > integers. But, in dts output, only the latter is used. Therefore, I
> > didn't include support for negative 8-bit values in dts output (I did
> > add support for them in yaml output). Some possible alternatives are:
> >
> > - switch to the '/bits/ 8 <>' format in dts output
> >
> > - only switch to the '/bits/ 8 <>' format if the array contains negative
> >   values
> >
> > - add another marker to differentiate between the two formats
>
> I'd be ok with this, given the distinction above.  This is similar to
> putting different for markers for say "abc" and <0x61626300> which
> likewise represent the same bytes.

Okay, fine by me.

Rob

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[David Gibson]

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

On Tue, Jul 07, 2020 at 09:47:49AM -0600, Rob Herring wrote:
> On Sun, Jul 5, 2020 at 2:59 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> > > Currently, in yaml output, negative values are indistinguishable from
> > > positive ones. Some bindings (for example
> > > Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> > > values. If those binding are converted to yaml, dt-schema validation
> > > wouldn't work with them.
> >
> > > The first patch is a mechanical change and shouldn't affect dtc's
> > > behaviour.
> > >
> > > The second one is the functional change. When applied, dts to dts and
> > > dts to yaml conversions preserve the '-' sign in front of integers.
> > >
> > > For now, in dts parsing, only the unary '-' operator creates negative
> > > values (the other operators just set the 'is_negative' flag to false),
> > > but it should be easy to add support for other operators if needed.
> >
> > Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
> > fiddly work to preserve the sign only in a pretty narrow set of cases.
> > Basically it will only be reliably correct in the case of just
> > (-literal).  Something like (5-3) will still show as 0xfe rather than
> > -0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
> > looking "-0xfe".
> 
> It's a narrow set of cases if you assume an equal distribution of
> expressions, but I'd bet you'd be hard pressed to really find examples
> beyond (-literal).

You might be right, but I still really dislike guessing the type from
how the user has happened to format it.  Oh, and another case: it's
not uncommon to use "-1" just as a shorthand for 0xfffffffff (or
whatever) in an essentially unsigned field.

> But I guess at least the above examples could be supported. We'd have
> to assume that if there's any subtraction, then types are signed and
> we carry that thru. And perhaps give up and revert to unsigned if
> there's anything more complex (signed values plus logic ops). In that
> case, rather than tracking 'is_negative', we'd track is_signed and
> then the output side would have to check that plus the data value to
> output a negative.
> 
> > I also don't see why this is vital to schema validation.  The
> > validator should know the binding, and will therefore know which
> > values are signed, and can appropriately treat 0xfe and -2 (or
> > equivalent at whatever width) as the same.
> 
> We could do that to some extent. The type is abstracted from the
> binding and it just specifies 'int8' for example. That could be
> internally set to { maximum: 0xff, minimum: 0 }. This works until you
> want to further constrain the values for the specific bindings (e.g. {
> enum: [ -1, -2, 127 ]}). We could process the schemas to find every
> negative number and transform them to an unsigned value, but this gets
> messy and fragile and is something I want to minimize.

I find it really hard to believe this would be *more* messy and
fragile than what you're proposing here.  You're putting the onus on
dtc which fundamentally *does not have* the information it would need
to do this correctly.  So you're resting the validation on some
heuristic of how the user expresses the value, which really doesn't
seem like a good idea.

The schema checking has (or should have) the binding, which specifies
the types (including signedness, I would hope) of each field.  That's
what you need to correctly analyze this

I don't see why constraining to particular values makes this hard.
You know the type, so you can translate each of the specific values
into that type

> > > One issue is that there are two ways to format an array of bytes in dts:
> > > '/bits/ 8 <...>' and '[...]'.
> >
> > So, these are intended for different purposes.  If you want to think
> > of your bytes as a bunch of 8-bit numbers, the former is appropriate.
> > The [ ... ] notation is more intended to be a compact way of putting
> > in "untyped" binary data.  For example, a blob of information which
> > gets verbatim copied into a device without interpretation by the
> > driver would make sense to use this form.
> 
> That probably means we should have a schema type for [...], but so far
> I haven't seen a need.

Sure.

> > > Only the former supports negative
> > > integers. But, in dts output, only the latter is used. Therefore, I
> > > didn't include support for negative 8-bit values in dts output (I did
> > > add support for them in yaml output). Some possible alternatives are:
> > >
> > > - switch to the '/bits/ 8 <>' format in dts output
> > >
> > > - only switch to the '/bits/ 8 <>' format if the array contains negative
> > >   values
> > >
> > > - add another marker to differentiate between the two formats
> >
> > I'd be ok with this, given the distinction above.  This is similar to
> > putting different for markers for say "abc" and <0x61626300> which
> > likewise represent the same bytes.
> 
> Okay, fine by me.
> 
> Rob
> 

-- 
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: 833 bytes --]

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[Rob Herring]

On Wed, Jul 8, 2020 at 5:12 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Jul 07, 2020 at 09:47:49AM -0600, Rob Herring wrote:
> > On Sun, Jul 5, 2020 at 2:59 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> > > > Currently, in yaml output, negative values are indistinguishable from
> > > > positive ones. Some bindings (for example
> > > > Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> > > > values. If those binding are converted to yaml, dt-schema validation
> > > > wouldn't work with them.
> > >
> > > > The first patch is a mechanical change and shouldn't affect dtc's
> > > > behaviour.
> > > >
> > > > The second one is the functional change. When applied, dts to dts and
> > > > dts to yaml conversions preserve the '-' sign in front of integers.
> > > >
> > > > For now, in dts parsing, only the unary '-' operator creates negative
> > > > values (the other operators just set the 'is_negative' flag to false),
> > > > but it should be easy to add support for other operators if needed.
> > >
> > > Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
> > > fiddly work to preserve the sign only in a pretty narrow set of cases.
> > > Basically it will only be reliably correct in the case of just
> > > (-literal).  Something like (5-3) will still show as 0xfe rather than
> > > -0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
> > > looking "-0xfe".
> >
> > It's a narrow set of cases if you assume an equal distribution of
> > expressions, but I'd bet you'd be hard pressed to really find examples
> > beyond (-literal).
>
> You might be right, but I still really dislike guessing the type from
> how the user has happened to format it.  Oh, and another case: it's
> not uncommon to use "-1" just as a shorthand for 0xfffffffff (or
> whatever) in an essentially unsigned field.

~0 would be more correct for this IMO.

Shouldn't we follow C rules here? If we do signed operations we
default to signed. And since we don't have casting, the above should
not be valid. (I couldn't find any examples of the above).

> > But I guess at least the above examples could be supported. We'd have
> > to assume that if there's any subtraction, then types are signed and
> > we carry that thru. And perhaps give up and revert to unsigned if
> > there's anything more complex (signed values plus logic ops). In that
> > case, rather than tracking 'is_negative', we'd track is_signed and
> > then the output side would have to check that plus the data value to
> > output a negative.
> >
> > > I also don't see why this is vital to schema validation.  The
> > > validator should know the binding, and will therefore know which
> > > values are signed, and can appropriately treat 0xfe and -2 (or
> > > equivalent at whatever width) as the same.
> >
> > We could do that to some extent. The type is abstracted from the
> > binding and it just specifies 'int8' for example. That could be
> > internally set to { maximum: 0xff, minimum: 0 }. This works until you
> > want to further constrain the values for the specific bindings (e.g. {
> > enum: [ -1, -2, 127 ]}). We could process the schemas to find every
> > negative number and transform them to an unsigned value, but this gets
> > messy and fragile and is something I want to minimize.
>
> I find it really hard to believe this would be *more* messy and
> fragile than what you're proposing here.

You are basing that statement on having looked at the schema tools?

The code to walk the schema and doing this for all the possible schema
structures that can exist is messy. I've re-written the code 3 times
and still don't like it. So I don't want to add more to it.

>  You're putting the onus on
> dtc which fundamentally *does not have* the information it would need
> to do this correctly.  So you're resting the validation on some
> heuristic of how the user expresses the value, which really doesn't
> seem like a good idea.
>
> The schema checking has (or should have) the binding, which specifies
> the types (including signedness, I would hope) of each field.  That's
> what you need to correctly analyze this
>
> I don't see why constraining to particular values makes this hard.
> You know the type, so you can translate each of the specific values
> into that type

Actually, I wasn't thinking about that, but you may not have all the
information. There can be multiple schemas for a given property. This
is a very common pattern where we have a common definition containing
the type and then specific bindings have further constraints such as
valid values. Even within a single binding we can have this pattern:

properties:
  signed-prop:
    $ref: /schemas/types.yaml#/definitions/int8

if:
  properties:
    compatible:
      const: foo,bar
then:
  properties:
    signed-prop:
      enum: [ -1, 0, 1 ]
else:
  properties:
    signed-prop:
      enum: [ -2, 0, 2 ]


While we could fixup this case and all the possible variations in
structures (such as allOf/anyOf/oneOf schemas), I don't see how we can
do it when the schema is split across different schema files.

If we don't address this in dtc, signed types are so rare (I found 22
cases of (-literal) and one case of subtraction in 1200 dts files),
I'm inclined to just make the schemas define unsigned values. That's
awful for readability and writers and reviewers of schema files,
but...


I'll throw out another idea. What if we extend '/bits/' to include the
sign. So we could have '/bits/ -8' for signed 8-bit. Then it's
explicit and there's no guessing.

Rob

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[David Gibson]

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

On Wed, Jul 08, 2020 at 10:26:07AM -0600, Rob Herring wrote:
> On Wed, Jul 8, 2020 at 5:12 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Jul 07, 2020 at 09:47:49AM -0600, Rob Herring wrote:
> > > On Sun, Jul 5, 2020 at 2:59 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6ryscVyMRj84@public.gmane.org.au> wrote:
> > > >
> > > > On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> > > > > Currently, in yaml output, negative values are indistinguishable from
> > > > > positive ones. Some bindings (for example
> > > > > Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> > > > > values. If those binding are converted to yaml, dt-schema validation
> > > > > wouldn't work with them.
> > > >
> > > > > The first patch is a mechanical change and shouldn't affect dtc's
> > > > > behaviour.
> > > > >
> > > > > The second one is the functional change. When applied, dts to dts and
> > > > > dts to yaml conversions preserve the '-' sign in front of integers.
> > > > >
> > > > > For now, in dts parsing, only the unary '-' operator creates negative
> > > > > values (the other operators just set the 'is_negative' flag to false),
> > > > > but it should be easy to add support for other operators if needed.
> > > >
> > > > Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
> > > > fiddly work to preserve the sign only in a pretty narrow set of cases.
> > > > Basically it will only be reliably correct in the case of just
> > > > (-literal).  Something like (5-3) will still show as 0xfe rather than
> > > > -0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
> > > > looking "-0xfe".
> > >
> > > It's a narrow set of cases if you assume an equal distribution of
> > > expressions, but I'd bet you'd be hard pressed to really find examples
> > > beyond (-literal).
> >
> > You might be right, but I still really dislike guessing the type from
> > how the user has happened to format it.  Oh, and another case: it's
> > not uncommon to use "-1" just as a shorthand for 0xfffffffff (or
> > whatever) in an essentially unsigned field.
> 
> ~0 would be more correct for this IMO.

Eh, maybe, nonetheless it's an idiom that's common in C and therefore
likely to make its way into dts files.

> Shouldn't we follow C rules here? If we do signed operations we
> default to signed.

That's not C rules.  In C the values themselves are typed, and that's
what determines the output type, never the operations.  Plus type
promotions always go signed to unsigned, not the other way around.

> And since we don't have casting, the above should
> not be valid. (I couldn't find any examples of the above).

Uh.. that doesn't follow.  At present all integers in dtc are
implicitly unsigned, so (-1) in dtc is equivalent to (-1U) in C, which
is both valid and pretty common.

> > > But I guess at least the above examples could be supported. We'd have
> > > to assume that if there's any subtraction, then types are signed and
> > > we carry that thru. And perhaps give up and revert to unsigned if
> > > there's anything more complex (signed values plus logic ops). In that
> > > case, rather than tracking 'is_negative', we'd track is_signed and
> > > then the output side would have to check that plus the data value to
> > > output a negative.
> > >
> > > > I also don't see why this is vital to schema validation.  The
> > > > validator should know the binding, and will therefore know which
> > > > values are signed, and can appropriately treat 0xfe and -2 (or
> > > > equivalent at whatever width) as the same.
> > >
> > > We could do that to some extent. The type is abstracted from the
> > > binding and it just specifies 'int8' for example. That could be
> > > internally set to { maximum: 0xff, minimum: 0 }. This works until you
> > > want to further constrain the values for the specific bindings (e.g. {
> > > enum: [ -1, -2, 127 ]}). We could process the schemas to find every
> > > negative number and transform them to an unsigned value, but this gets
> > > messy and fragile and is something I want to minimize.
> >
> > I find it really hard to believe this would be *more* messy and
> > fragile than what you're proposing here.
> 
> You are basing that statement on having looked at the schema tools?

No... but I guess I am judging the schema tools poorly if they can't
handle this.

> The code to walk the schema and doing this for all the possible schema
> structures that can exist is messy. I've re-written the code 3 times
> and still don't like it. So I don't want to add more to it.

Hmm.

> >  You're putting the onus on
> > dtc which fundamentally *does not have* the information it would need
> > to do this correctly.  So you're resting the validation on some
> > heuristic of how the user expresses the value, which really doesn't
> > seem like a good idea.
> >
> > The schema checking has (or should have) the binding, which specifies
> > the types (including signedness, I would hope) of each field.  That's
> > what you need to correctly analyze this
> >
> > I don't see why constraining to particular values makes this hard.
> > You know the type, so you can translate each of the specific values
> > into that type
> 
> Actually, I wasn't thinking about that, but you may not have all the
> information. There can be multiple schemas for a given property. This
> is a very common pattern where we have a common definition containing
> the type and then specific bindings have further constraints such as
> valid values. Even within a single binding we can have this pattern:

Yes, and..?  I don't see why that makes things any harder.

> properties:
>   signed-prop:
>     $ref: /schemas/types.yaml#/definitions/int8
> 
> if:
>   properties:
>     compatible:
>       const: foo,bar
> then:
>   properties:
>     signed-prop:
>       enum: [ -1, 0, 1 ]

You have the signed-prop flag right there, why can't you use that to
interpret the enum values correctly?

> else:
>   properties:
>     signed-prop:
>       enum: [ -2, 0, 2 ]
> 
> 
> While we could fixup this case and all the possible variations in
> structures (such as allOf/anyOf/oneOf schemas), I don't see how we can
> do it when the schema is split across different schema files.
> 
> If we don't address this in dtc, signed types are so rare (I found 22
> cases of (-literal) and one case of subtraction in 1200 dts files),
> I'm inclined to just make the schemas define unsigned values. That's
> awful for readability and writers and reviewers of schema files,
> but...

Um.. why can't you basically do the same thing interpreting the
schemas as we do in dtc.  You can specify signed values, but they're
basically just implicitly cast into unsigned values for comparison to
the values from the dt.

> I'll throw out another idea. What if we extend '/bits/' to include the
> sign. So we could have '/bits/ -8' for signed 8-bit. Then it's
> explicit and there's no guessing.

That's definitely a better idea that what we have so for, but I'm
still not following why introducing signed values as a concept in dtc
actually buys anything.

Plus... to really do that correctly we really out to use signed
arithmetic in any expressions after a /bits/ -8 (or whatever).  Which
means implementing signed multiply and shifts and all the rest in
dtc.  Which isn't that much work of itself, but the larger problem is
that getting the sign information from earlier to the right part of
the parser, I suspect will be quite a drag.

-- 
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: 833 bytes --]

Re: [PATCH 0/3] dtc: Preserve negative integers in yaml and dts output

[Rob Herring]

On Fri, Jul 17, 2020 at 5:13 AM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Wed, Jul 08, 2020 at 10:26:07AM -0600, Rob Herring wrote:
> > On Wed, Jul 8, 2020 at 5:12 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Tue, Jul 07, 2020 at 09:47:49AM -0600, Rob Herring wrote:
> > > > On Sun, Jul 5, 2020 at 2:59 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > >
> > > > > On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> > > > > > Currently, in yaml output, negative values are indistinguishable from
> > > > > > positive ones. Some bindings (for example
> > > > > > Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> > > > > > values. If those binding are converted to yaml, dt-schema validation
> > > > > > wouldn't work with them.
> > > > >
> > > > > > The first patch is a mechanical change and shouldn't affect dtc's
> > > > > > behaviour.
> > > > > >
> > > > > > The second one is the functional change. When applied, dts to dts and
> > > > > > dts to yaml conversions preserve the '-' sign in front of integers.
> > > > > >
> > > > > > For now, in dts parsing, only the unary '-' operator creates negative
> > > > > > values (the other operators just set the 'is_negative' flag to false),
> > > > > > but it should be easy to add support for other operators if needed.
> > > > >
> > > > > Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
> > > > > fiddly work to preserve the sign only in a pretty narrow set of cases.
> > > > > Basically it will only be reliably correct in the case of just
> > > > > (-literal).  Something like (5-3) will still show as 0xfe rather than
> > > > > -0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
> > > > > looking "-0xfe".
> > > >
> > > > It's a narrow set of cases if you assume an equal distribution of
> > > > expressions, but I'd bet you'd be hard pressed to really find examples
> > > > beyond (-literal).
> > >
> > > You might be right, but I still really dislike guessing the type from
> > > how the user has happened to format it.  Oh, and another case: it's
> > > not uncommon to use "-1" just as a shorthand for 0xfffffffff (or
> > > whatever) in an essentially unsigned field.
> >
> > ~0 would be more correct for this IMO.
>
> Eh, maybe, nonetheless it's an idiom that's common in C and therefore
> likely to make its way into dts files.

So we follow C, but...

> > Shouldn't we follow C rules here? If we do signed operations we
> > default to signed.
>
> That's not C rules.  In C the values themselves are typed, and that's
> what determines the output type, never the operations.  Plus type
> promotions always go signed to unsigned, not the other way around.
>
> > And since we don't have casting, the above should
> > not be valid. (I couldn't find any examples of the above).
>
> Uh.. that doesn't follow.  At present all integers in dtc are
> implicitly unsigned, so (-1) in dtc is equivalent to (-1U) in C, which
> is both valid and pretty common.

...don't follow C? The 'U' is essentially casting.

> > > > But I guess at least the above examples could be supported. We'd have
> > > > to assume that if there's any subtraction, then types are signed and
> > > > we carry that thru. And perhaps give up and revert to unsigned if
> > > > there's anything more complex (signed values plus logic ops). In that
> > > > case, rather than tracking 'is_negative', we'd track is_signed and
> > > > then the output side would have to check that plus the data value to
> > > > output a negative.
> > > >
> > > > > I also don't see why this is vital to schema validation.  The
> > > > > validator should know the binding, and will therefore know which
> > > > > values are signed, and can appropriately treat 0xfe and -2 (or
> > > > > equivalent at whatever width) as the same.
> > > >
> > > > We could do that to some extent. The type is abstracted from the
> > > > binding and it just specifies 'int8' for example. That could be
> > > > internally set to { maximum: 0xff, minimum: 0 }. This works until you
> > > > want to further constrain the values for the specific bindings (e.g. {
> > > > enum: [ -1, -2, 127 ]}). We could process the schemas to find every
> > > > negative number and transform them to an unsigned value, but this gets
> > > > messy and fragile and is something I want to minimize.
> > >
> > > I find it really hard to believe this would be *more* messy and
> > > fragile than what you're proposing here.
> >
> > You are basing that statement on having looked at the schema tools?
>
> No... but I guess I am judging the schema tools poorly if they can't
> handle this.
>
> > The code to walk the schema and doing this for all the possible schema
> > structures that can exist is messy. I've re-written the code 3 times
> > and still don't like it. So I don't want to add more to it.
>
> Hmm.
>
> > >  You're putting the onus on
> > > dtc which fundamentally *does not have* the information it would need
> > > to do this correctly.  So you're resting the validation on some
> > > heuristic of how the user expresses the value, which really doesn't
> > > seem like a good idea.
> > >
> > > The schema checking has (or should have) the binding, which specifies
> > > the types (including signedness, I would hope) of each field.  That's
> > > what you need to correctly analyze this
> > >
> > > I don't see why constraining to particular values makes this hard.
> > > You know the type, so you can translate each of the specific values
> > > into that type
> >
> > Actually, I wasn't thinking about that, but you may not have all the
> > information. There can be multiple schemas for a given property. This
> > is a very common pattern where we have a common definition containing
> > the type and then specific bindings have further constraints such as
> > valid values. Even within a single binding we can have this pattern:
>
> Yes, and..?  I don't see why that makes things any harder.

The different schema whether in different files or the same file are
essentially completely independent from each other. That's just how
json-schema works.

>
> > properties:
> >   signed-prop:
> >     $ref: /schemas/types.yaml#/definitions/int8
> >
> > if:
> >   properties:
> >     compatible:
> >       const: foo,bar
> > then:
> >   properties:
> >     signed-prop:
> >       enum: [ -1, 0, 1 ]
>
> You have the signed-prop flag right there, why can't you use that to
> interpret the enum values correctly?

What flag? 'signed-prop' is a completely opaque DT property name here.
At the location of the enum, I don't know if the -1 should be 0xff,
0xffff, or 0xffffffff. I guess when I have the schema and the instance
data, I'd have the size. Though the size is part of the parent array,
not the scalar, so maybe not.

> > else:
> >   properties:
> >     signed-prop:
> >       enum: [ -2, 0, 2 ]
> >
> >
> > While we could fixup this case and all the possible variations in
> > structures (such as allOf/anyOf/oneOf schemas), I don't see how we can
> > do it when the schema is split across different schema files.
> >
> > If we don't address this in dtc, signed types are so rare (I found 22
> > cases of (-literal) and one case of subtraction in 1200 dts files),
> > I'm inclined to just make the schemas define unsigned values. That's
> > awful for readability and writers and reviewers of schema files,
> > but...
>
> Um.. why can't you basically do the same thing interpreting the
> schemas as we do in dtc.  You can specify signed values, but they're
> basically just implicitly cast into unsigned values for comparison to
> the values from the dt.

Maybe, I suppose there's some way to do that in python, and we'd have
to override what standard tools already do for us.

> > I'll throw out another idea. What if we extend '/bits/' to include the
> > sign. So we could have '/bits/ -8' for signed 8-bit. Then it's
> > explicit and there's no guessing.
>
> That's definitely a better idea that what we have so for, but I'm
> still not following why introducing signed values as a concept in dtc
> actually buys anything.
>
> Plus... to really do that correctly we really out to use signed
> arithmetic in any expressions after a /bits/ -8 (or whatever).

Why? This is just like doing a C cast to signed *after* you do all
operations. We're just passing thru extra type information.

> Which
> means implementing signed multiply and shifts and all the rest in
> dtc.  Which isn't that much work of itself, but the larger problem is
> that getting the sign information from earlier to the right part of
> the parser, I suspect will be quite a drag.
>
> --
> 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 0/3] dtc: Preserve negative integers in yaml and dts output

[David Gibson]

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

On Fri, Jul 17, 2020 at 02:25:30PM -0600, Rob Herring wrote:
> On Fri, Jul 17, 2020 at 5:13 AM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Wed, Jul 08, 2020 at 10:26:07AM -0600, Rob Herring wrote:
> > > On Wed, Jul 8, 2020 at 5:12 AM David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6ryscVyMRj84@public.gmane.org.au> wrote:
> > > >
> > > > On Tue, Jul 07, 2020 at 09:47:49AM -0600, Rob Herring wrote:
> > > > > On Sun, Jul 5, 2020 at 2:59 AM David Gibson <david-xT8FGy+AXnSZP5U4TJ/6Hg@public.gmane.orgr.id.au> wrote:
> > > > > >
> > > > > > On Fri, Jun 26, 2020 at 05:25:25PM +0100, Andrei Ziureaev wrote:
> > > > > > > Currently, in yaml output, negative values are indistinguishable from
> > > > > > > positive ones. Some bindings (for example
> > > > > > > Documentation/devicetree/bindings/iio/accel/lis302.txt) mention negative
> > > > > > > values. If those binding are converted to yaml, dt-schema validation
> > > > > > > wouldn't work with them.
> > > > > >
> > > > > > > The first patch is a mechanical change and shouldn't affect dtc's
> > > > > > > behaviour.
> > > > > > >
> > > > > > > The second one is the functional change. When applied, dts to dts and
> > > > > > > dts to yaml conversions preserve the '-' sign in front of integers.
> > > > > > >
> > > > > > > For now, in dts parsing, only the unary '-' operator creates negative
> > > > > > > values (the other operators just set the 'is_negative' flag to false),
> > > > > > > but it should be easy to add support for other operators if needed.
> > > > > >
> > > > > > Hmmmmm.  I'm really not convinced by this.  It seems like a lot of
> > > > > > fiddly work to preserve the sign only in a pretty narrow set of cases.
> > > > > > Basically it will only be reliably correct in the case of just
> > > > > > (-literal).  Something like (5-3) will still show as 0xfe rather than
> > > > > > -0x2, and if you do -(3-5) or (-3+5) you'll get the very strange
> > > > > > looking "-0xfe".
> > > > >
> > > > > It's a narrow set of cases if you assume an equal distribution of
> > > > > expressions, but I'd bet you'd be hard pressed to really find examples
> > > > > beyond (-literal).
> > > >
> > > > You might be right, but I still really dislike guessing the type from
> > > > how the user has happened to format it.  Oh, and another case: it's
> > > > not uncommon to use "-1" just as a shorthand for 0xfffffffff (or
> > > > whatever) in an essentially unsigned field.
> > >
> > > ~0 would be more correct for this IMO.
> >
> > Eh, maybe, nonetheless it's an idiom that's common in C and therefore
> > likely to make its way into dts files.
> 
> So we follow C, but...
> 
> > > Shouldn't we follow C rules here? If we do signed operations we
> > > default to signed.
> >
> > That's not C rules.  In C the values themselves are typed, and that's
> > what determines the output type, never the operations.  Plus type
> > promotions always go signed to unsigned, not the other way around.
> >
> > > And since we don't have casting, the above should
> > > not be valid. (I couldn't find any examples of the above).
> >
> > Uh.. that doesn't follow.  At present all integers in dtc are
> > implicitly unsigned, so (-1) in dtc is equivalent to (-1U) in C, which
> > is both valid and pretty common.
> 
> ...don't follow C? The 'U' is essentially casting.

Well, we try to follow C as a rule, but there are occasional
differences.  Some are deliberate choices, some are basically
mistakes, but we can't change them now.  Defaulting to unsigned rather
than signed is.. borderline.  Unsigned values (like addresses) are a
lot more common in DT data, but whether that's enough to justify the
change is debatable.

But in any case, defaulting to unsigned rather than signed is a far
smaller and easier to understand change than having the '-' operator
change the expression type.

[snip]
> > > >  You're putting the onus on
> > > > dtc which fundamentally *does not have* the information it would need
> > > > to do this correctly.  So you're resting the validation on some
> > > > heuristic of how the user expresses the value, which really doesn't
> > > > seem like a good idea.
> > > >
> > > > The schema checking has (or should have) the binding, which specifies
> > > > the types (including signedness, I would hope) of each field.  That's
> > > > what you need to correctly analyze this
> > > >
> > > > I don't see why constraining to particular values makes this hard.
> > > > You know the type, so you can translate each of the specific values
> > > > into that type
> > >
> > > Actually, I wasn't thinking about that, but you may not have all the
> > > information. There can be multiple schemas for a given property. This
> > > is a very common pattern where we have a common definition containing
> > > the type and then specific bindings have further constraints such as
> > > valid values. Even within a single binding we can have this pattern:
> >
> > Yes, and..?  I don't see why that makes things any harder.
> 
> The different schema whether in different files or the same file are
> essentially completely independent from each other. That's just how
> json-schema works.

Ah, ok, I think I get you.  Really it seems like the type checks in
json-schema are kind of bogus - they're "checking" values on the JSON
side, but on the dtb side those types are more or less structurally
enforced (i.e. if you interpret 8 bits of your bytestring as an u8, it
can't fail to be a valid u8).

As I feared, this is really again showing up the mismatch between the
dt and json data models.  I can understand the impetus to want to
re-use the existing json schema tooling, but there's some real uglies
in the interface.

Basically the problem is that you want to run your schemas on a
"sensible" json representation of the data - you want strings as json
strings, lists of values as json lists of json integers and so forth.
But dtb files fundamentally *do not know* the right json
representation.  dtc doesn't either, since it was built around the dtb
data model where properties are just bytestrings.

The only way it's working at all by abusing dts side information - how
the user has chosen to enter the data - to infer the likely json
representation.  But it's a pretty flawed approach:

1) It will break if for whatever reason the user used a source
   representation that wasn't an obvious match to the json structuring
   we want

2) It means we can only check dts files, not dtb files

3) Most importantly, it breaks down when we get to details that don't
   have a natural representation in dts syntax; like the signedness of
   integers.

I think signedness won't be the end of it either.  Just off the top of
my head, I think there's a whole other can of worms in the fact that
dtb "strings" are basically just bytestrings that happen to have no
\0s until the terminating \0, whereas JSON strings are Unicode.

> > > properties:
> > >   signed-prop:
> > >     $ref: /schemas/types.yaml#/definitions/int8
> > >
> > > if:
> > >   properties:
> > >     compatible:
> > >       const: foo,bar
> > > then:
> > >   properties:
> > >     signed-prop:
> > >       enum: [ -1, 0, 1 ]
> >
> > You have the signed-prop flag right there, why can't you use that to
> > interpret the enum values correctly?
> 
> What flag? 'signed-prop' is a completely opaque DT property name here.
> At the location of the enum, I don't know if the -1 should be 0xff,
> 0xffff, or 0xffffffff. I guess when I have the schema and the instance
> data, I'd have the size. Though the size is part of the parent array,
> not the scalar, so maybe not.

Yeah, sorry, I misunderstood the example.

> > > else:
> > >   properties:
> > >     signed-prop:
> > >       enum: [ -2, 0, 2 ]
> > >
> > >
> > > While we could fixup this case and all the possible variations in
> > > structures (such as allOf/anyOf/oneOf schemas), I don't see how we can
> > > do it when the schema is split across different schema files.
> > >
> > > If we don't address this in dtc, signed types are so rare (I found 22
> > > cases of (-literal) and one case of subtraction in 1200 dts files),
> > > I'm inclined to just make the schemas define unsigned values. That's
> > > awful for readability and writers and reviewers of schema files,
> > > but...
> >
> > Um.. why can't you basically do the same thing interpreting the
> > schemas as we do in dtc.  You can specify signed values, but they're
> > basically just implicitly cast into unsigned values for comparison to
> > the values from the dt.
> 
> Maybe, I suppose there's some way to do that in python, and we'd have
> to override what standard tools already do for us.
> 
> > > I'll throw out another idea. What if we extend '/bits/' to include the
> > > sign. So we could have '/bits/ -8' for signed 8-bit. Then it's
> > > explicit and there's no guessing.
> >
> > That's definitely a better idea that what we have so for, but I'm
> > still not following why introducing signed values as a concept in dtc
> > actually buys anything.
> >
> > Plus... to really do that correctly we really out to use signed
> > arithmetic in any expressions after a /bits/ -8 (or whatever).
> 
> Why? This is just like doing a C cast to signed *after* you do all
> operations. We're just passing thru extra type information.

Hmm, I guess.  Not sure that really satisfies principle of least
surprise.

> > Which
> > means implementing signed multiply and shifts and all the rest in
> > dtc.  Which isn't that much work of itself, but the larger problem is
> > that getting the sign information from earlier to the right part of
> > the parser, I suspect will be quite a drag.

-- 
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: 833 bytes --]