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

[Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

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

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

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

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

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

How does this sound?

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

Done.

Thanks,
     Anton

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