[PATCH 3/3] dtc: Add support for variable sized cells
Anton Staaf
robotboy at chromium.org
Fri Sep 23 04:02:59 EST 2011
On Wed, Sep 21, 2011 at 7:42 PM, David Gibson
<david at gibson.dropbear.id.au> wrote:
> On Wed, Sep 21, 2011 at 01:42:11PM -0700, Anton Staaf wrote:
>> Cells of size 8, 16, 32, and 64 bits are supported. The new
>> /size/ 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 = /size/ 16 <0x1234 0x5678 0x0 0xffff>;
>
> I'm now thinking that just plain "/size/" is maybe a bit too generic
> for this keyword. But I'm not sure what would be better. Maybe
> /cellsize/ or /intsize/ or /arraysize/.
Also /bits/ or /cellbits/ might make good choices because the size is in
bits and not in bytes. I'm not fond of /arraysize/ because we are
specifying the size of each element in the array, not the array itself.
>>
>> is equivalent to:
>>
>> property = <0x12345678 0x0000ffff>;
>>
>> It is now also possible to directly specify a 64 bit literal in a
>> cell list using:
>>
>> property = /size/ 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 = /size/ 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 at chromium.org>
>> Cc: Jon Loeliger <jdl at jdl.com>
>> Cc: David Gibson <david at gibson.dropbear.id.au>
>> Cc: Grant Likely <grant.likely at secretlab.ca>
>> ---
>> Documentation/dts-format.txt | 18 +++++++-
>> dtc-lexer.l | 6 +++
>> dtc-parser.y | 59 +++++++++++++++++++----------
>> dtc.h | 4 ++
>> tests/.gitignore | 1 +
>> tests/Makefile.tests | 1 +
>> tests/run_tests.sh | 3 +
>> tests/sized_cells.c | 84 ++++++++++++++++++++++++++++++++++++++++++
>> tests/sized_cells.dts | 11 +++++
>> tests/testdata.h | 2 +
>> 10 files changed, 165 insertions(+), 24 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..0502f49 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 /size/ prefix. The
>> + /size/ 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 = /size/ 8 <17 0xc>;
>> + e.g. clock-frequenct = /size/ 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 at 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 at gibson.dropbear.id.au>
>> -- Yoder Stuart <stuart.yoder at freescale.com>
>> + -- Anton Staaf <robotboy at chromium.org>
>> diff --git a/dtc-lexer.l b/dtc-lexer.l
>> index 494e342..01ed628 100644
>> --- a/dtc-lexer.l
>> +++ b/dtc-lexer.l
>> @@ -97,6 +97,12 @@ static int pop_input_file(void);
>> return DT_MEMRESERVE;
>> }
>>
>> +<*>"/size/" {
>> + DPRINT("Keyword: /size/\n");
>> + BEGIN_DEFAULT();
>> + return DT_SIZE;
>> + }
>> +
>> <*>{LABEL}: {
>> DPRINT("Label: %s\n", yytext);
>> yylval.labelref = xstrdup(yytext);
>> diff --git a/dtc-parser.y b/dtc-parser.y
>> index bc05a24..6355ce2 100644
>> --- a/dtc-parser.y
>> +++ b/dtc-parser.y
>> @@ -44,9 +44,9 @@ static unsigned char eval_char_literal(const char *s);
>> unsigned int cbase;
>> uint8_t byte;
>> struct data data;
>> + struct sized_data sized_data;
>>
>> uint64_t addr;
>> - cell_t cell;
>> struct property *prop;
>> struct property *proplist;
>> struct node *node;
>> @@ -56,6 +56,7 @@ static unsigned char eval_char_literal(const char *s);
>>
>> %token DT_V1
>> %token DT_MEMRESERVE
>> +%token DT_SIZE
>> %token <propnodename> DT_PROPNODENAME
>> %token <literal> DT_LITERAL
>> %token <literal> DT_CHAR_LITERAL
>> @@ -71,8 +72,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 <sized_data> celllist
>> %type <data> bytestring
>> %type <prop> propdef
>> %type <proplist> proplist
>> @@ -182,9 +182,9 @@ propdata:
>> {
>> $$ = data_merge($1, $2);
>> }
>> - | propdataprefix '<' celllist '>'
>> + | propdataprefix celllist '>'
>
> I'd rename celllist to cellistprefix or something, or this looks
> decidedly odd.
Done
>> {
>> - $$ = data_merge($1, $3);
>> + $$ = data_merge($1, $2.data);
>> }
>> | propdataprefix '[' bytestring ']'
>> {
>> @@ -243,33 +243,50 @@ propdataprefix:
>> ;
>>
>> celllist:
>> - /* empty */
>> + DT_SIZE DT_LITERAL '<'
>> {
>> - $$ = empty_data;
>> + $$.data = empty_data;
>> + $$.size = eval_literal($2, 0, 7);
>> +
>> + if (($$.size != 8) &&
>> + ($$.size != 16) &&
>> + ($$.size != 32) &&
>> + ($$.size != 64))
>> + {
>> + print_error("Only 8, 16, 32 and 64-bit cell"
>> + " sizes are currently supported");
>> + $$.size = 32;
>> + }
>> }
>> - | celllist cellval
>> + | '<'
>> {
>> - $$ = data_append_cell($1, $2);
>> + $$.data = empty_data;
>> + $$.size = 32;
>> }
>> - | celllist DT_REF
>> + | celllist DT_LITERAL
>> {
>> - $$ = data_append_cell(data_add_marker($1, REF_PHANDLE,
>> - $2), -1);
>> + uint64_t val = eval_literal($2, 0, $1.size);
>> +
>> + $$.data = data_append_literal($1.data, val, $1.size);
>> }
>> - | celllist DT_LABEL
>> + | celllist DT_CHAR_LITERAL
>> {
>> - $$ = data_add_marker($1, LABEL, $2);
>> - }
>> - ;
>> + uint64_t val = eval_char_literal($2);
>>
>> -cellval:
>> - DT_LITERAL
>> + $$.data = data_append_literal($1.data, val, $1.size);
>> + }
>> + | celllist DT_REF
>> {
>> - $$ = eval_literal($1, 0, 32);
>> + if ($1.size != 32)
>> + print_error("References only allowed in 32-bit"
>> + " cell lists");
>> +
>> + $$.data = data_add_marker($1.data, REF_PHANDLE, $2);
>> + $$.data = data_append_cell($$.data, -1);
>
> Ouch, this is kind of nasty. If you get a REF in a celllist of the
> wrong size, you print an error, but then insert the ref as a 32-bit
> quantity, regardless of the actual cell size. While you can argue
> that correct behaviour is undefined in this error case, I think this
> is a fair way from least-surprise.
>
> I'd suggest instead that in the error case, you insert either a 0 or
> -1 of the correct cell size instead of the reference.
Done, I opted for 0xffffffffffffffff masked to the right size so the check in
data_append_integer wouldn't fail out. And for consistency with the -1
that is stored in the normal 32-bit case.
>> }
>> - | DT_CHAR_LITERAL
>> + | celllist DT_LABEL
>> {
>> - $$ = eval_char_literal($1);
>> + $$.data = data_add_marker($1.data, LABEL, $2);
>> }
>> ;
>>
>> diff --git a/dtc.h b/dtc.h
>> index 50433f6..199b3f1 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -87,6 +87,10 @@ struct data {
>> struct marker *markers;
>> };
>>
>> +struct sized_data {
>> + struct data data;
>> + int size;
>> +};
>
> This is only used internal to the parser, so the definition can just
> go in dtc-parser.y. And maybe call it cellarray or something more
> specific to its usage. "sized_data" suggests it holds the size of the
> whole of the data, which doesn't make sense, since a struct data does
> that already.
Done, I didn't see a simple way to add the definition to the part of the
generated header that comes before the definition of the union. So
instead I added this as an anonymous struct directly in the union.
>> #define empty_data ((struct data){ /* all .members = 0 or NULL */ })
>>
>> 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 e718b63..60f3af0 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 1246df1..df97f83 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..5a5e7c2
>> --- /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_3};
>> + 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_4);
>> + 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-16", "cells-one-32");
>> +
>> + PASS();
>> +}
>> diff --git a/tests/sized_cells.dts b/tests/sized_cells.dts
>> new file mode 100644
>> index 0000000..abd0caf
>> --- /dev/null
>> +++ b/tests/sized_cells.dts
>> @@ -0,0 +1,11 @@
>> +/dts-v1/;
>> +
>> +/ {
>> + cells-8b = /size/ 8 <'\r' 'b' '\0' '\'' '\xff' 0xde>;
>> + cells-16b = /size/ 16 <'\r' 'b' '\0' '\'' '\xff' 0xdead>;
>> + cells-32b = /size/ 32 <'\r' 'b' '\0' '\'' '\xff' 0xdeadbeef>;
>> + cells-64b = /size/ 64 <'\r' 'b' '\0' '\'' '\xff' 0xdeadbeef00000000>;
>> +
>> + cells-one-16 = /size/ 16 <0x1234 0x5678 0x0 0xffff>;
>> + cells-one-32 = <0x12345678 0x0000ffff>;
>> +};
>> diff --git a/tests/testdata.h b/tests/testdata.h
>> index d4c6759..2d89470 100644
>> --- a/tests/testdata.h
>> +++ b/tests/testdata.h
>> @@ -11,6 +11,8 @@
>>
>> #define TEST_VALUE_1 0xdeadbeef
>> #define TEST_VALUE_2 123456789
>> +#define TEST_VALUE_3 0xde
>> +#define TEST_VALUE_4 0xdead
>
> I think it would be better to use masked versions of the existing
> constants, rather than defining new short ones.
>
>>
>> #define PHANDLE_1 0x2000
>> #define PHANDLE_2 0x2001
>
> --
> 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
>
Thanks,
Anton
More information about the devicetree-discuss
mailing list