[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