[SLOF] [PATCH slof] fdt: Pass the resulting device tree to QEMU

[Segher Boessenkool]

Hi Alexey,

Just some random comments, not a full review:

On Fri, Sep 29, 2017 at 07:07:29PM +1000, Alexey Kardashevskiy wrote:
> +: fdt-struct-add ( bytes len nullterminate -- )
> +    >r
> +    dup >r                  ( bytes len r: nullterminate len )
> +    fdt-struct-cur @ swap   ( bytes cur len r: nullterminate len )
> +    move
> +    fdt-struct-cur @        ( cur r: nullterminate len )
> +    r> +                    ( cur r: nullterminate )
> +    r> IF
> +        0 over c!
> +        1+
> +    THEN
> +    3 + -4 and
> +    fdt-struct-cur !
> +;

You want a comment on this function, and you probably want a convenience
word to do the zero-termination (e.g. fdt-struct-add-zero-terminated).

> +: fdt-add-long ( token -- )
> +    fdt-struct-cur @
> +    l!
> +    fdt-struct-cur @
> +    4 +
> +    fdt-struct-cur !
> +;

: fdt-add-long ( token -- )
   fdt-struct-cur @ l!  /l fdt-struct-cur +! ;

"add" is not such a great name, maybe you should name this like property
encoding (and maybe structure it like it even), so fdt-encode-int etc.?

> +: skip-to-next ( cur -- cur )
> +    BEGIN
> +        dup c@
> +    WHILE
> +        1+
> +    REPEAT
> +    4 + -4 and
> +;

: skip-to-next ( addr -- addr )  dup zcount + 1+ 4 #aligned ;

Skip to next what?  "fdt-skip-string" perhaps?

> +: string-equal ( name namelen str -- true | false )
> +    3dup swap comp 0 <> IF
> +        3drop
> +        false
> +        EXIT
> +    THEN
> +    + c@ 0 = nip
> +
> +    dup IF
> +        fdt-strings-reused @ 1+ fdt-strings-reused !
> +    THEN
> +;

So "str" is a zero-terminated string here.

: zstring=  ( str len zstr -- flag )
   2dup + c@ IF 3drop false EXIT THEN  swap comp 0= ;

\ Not a great name, it does this "reused" thing, too?
: string-equal ( name namelen str -- true | false )
   zstring= dup IF 1 fdt-strings-reused +! ;

> +: fdt-get-string ( name namelen -- nameoff )
> +    \ lookup
> +    fdt-strings @
> +    BEGIN
> +        dup fdt-strings-cur @
> +        = not

Don't use NOT, it does not do what you think (it is INVERT, but you want
0= here; could just say <> of course).

This word is so big that it is very hard to follow, btw.

> +: fdt-begin-node ( name namelen -- )
> +    OF_DT_BEGIN_NODE fdt-add-long
> +    2dup 1 = swap c@ 2F = and  \ is it "/"?

This would be ideal for COMPARE except that was never implemented ;-)
   s" /" compare
(It's also probably more readable to write   [char] /   instead of 2F,
if you don't want to use the string words).

I think this takes so long on huge device trees because you do way too
many string compares, but it is hard to tell.


Segher