[SLOF] [PATCH slof 1/2] fdt: Factor out code to replace a phandle in place

[Alexey Kardashevskiy]

On 19/5/18 2:10 am, Thomas Huth wrote:
> On 14.05.2018 12:03, Alexey Kardashevskiy wrote:
>> We generate a fake XICS phandle in QEMU and SLOF replaces that phandle
>> with the real one (i.e. SLOF's node address) in interrupt-parent and
>> interrupt-map properties. These properties are handled differently -
>> the interrupt-map is fixed in place while interrupt-parent is
>> decoded+encoded+set as a property.
>>
>> This changes interrupt-parent fixing code to do what the interrupt-map
>> code does because soon we are going to have more phandles to fix and some
>> contain an array of phandles (such as "ibm,npu").
>>
>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>> ---
>>  board-qemu/slof/fdt.fs | 27 +++++++++++++++++++--------
>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
>> index 8e8921f..5284ae6 100644
>> --- a/board-qemu/slof/fdt.fs
>> +++ b/board-qemu/slof/fdt.fs
>> @@ -279,6 +279,23 @@ fdt-claim-reserve
>>     2drop
>>  ;
>>  
>> +: fdt-replace-l, 4 - swap 4 + swap ;
> 
> Why "l," ? I thought the "comma" words would be used for compiling stuff
> into the dictionary, but apparently you're not doing this here?


Not just dictionary, fdt-fl.fs uses this naming approach when adding to
flatten DT. Segher suggested, looks nicer.


> 
>> +: (fdt-replace-phandles) ( old new propname propnamelen node )
> 
> Please use the ( before -- after ) notation here:
> 
> ( old new propname propnamelen node -- )
> 
>> +    get-property IF 2drop EXIT THEN
>> +    BEGIN
>> +        dup
>> +    WHILE                   ( old new prop-addr prop-len )
>> +        over l@
>> +        4 pick = IF
>> +            2 pick 2 pick l! \ replace old with new in place
>> +            TRUE TO (fdt-phandle-replaced)
>> +        THEN
>> +        fdt-replace-l,
> 
> Maybe you could simply use the "4 - swap 4 + swap" directly here? It's
> only one line of code, so not sure whether it's worth to put this into a
> separate Forth word.


"l," kind of allocates some bytes in some heap.


> 
>> +    REPEAT
>> +    2drop 2drop
>> +;
>> +
>>  \ Replace one phandle "old" with a phandle "new" in "node" and recursively
>>  \ in its child nodes:
>>  : fdt-replace-all-phandles ( old new node -- )
>> @@ -288,14 +305,8 @@ fdt-claim-reserve
>>        ( old new prop-addr prop-len  R: node )
>>        fdt-replace-interrupt-map
>>     THEN
>> -   s" interrupt-parent" r@ get-property 0= IF
>> -      ( old new prop-addr prop-len  R: node )
>> -      decode-int -rot 2drop                  ( old new val  R: node )
>> -      2 pick = IF                            ( old new      R: node )
>> -         dup encode-int s" interrupt-parent" r@ set-property
>> -         TRUE TO (fdt-phandle-replaced)
>> -      THEN
>> -   THEN
>> +
>> +   2dup s" interrupt-parent" r@ (fdt-replace-phandles)
>>     \ ... add more properties that have to be fixed here ...
>>     r>
>>     \ Now recurse over all child nodes:       ( old new node )
> 
> Apart from the nits, this looks ok to me for small properties. But how
> do you make sure that you do not accidentally replace the wrong spot in
> bigger properties later?


Unlike already present and potentially dangerous "interrupt-xxx" hacking,
the new properties I am adding are storing only phandles (one or two). It
is worth commenting though, will add.



-- 
Alexey