At the moment, ImageBuilder assumes that all addresses/sizes are
32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
value for the first cell. Because of that, we cannot specify
MEMORY_START and MEMORY_END to be above 32-bits (e.g. to place the images
in the upper memory bank).

Add support to properly handle 64-bit addresses/sizes:
 - add function split_into_halves to split the value passed as a first
   argument into upper and lower halves. These are then set as values for
   variables passed respetively as the second and third argument,
 - add function split_addr_size to split address and size and form a
   string to be passed to dt_set as data argument for reg property.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v2:
- redesign a patch based on master-next instead of NXP dynamic assignment patch
---
 scripts/uboot-script-gen | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index b24dca2b7f7e..09d237d192c1 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -22,6 +22,29 @@ function dt_mknode()
     fi
 }
 
+# Usage:
+# split_into_halves <value> <variable_to_store_upper> <variable_to_store_lower>
+function split_into_halves()
+{
+    local value=$1
+    local upper=$2
+    local lower=$3
+
+    eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
+    eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
+}
+
+function split_addr_size()
+{
+    local addr=$1
+    local size=$2
+
+    split_into_halves $addr addr_upper addr_lower
+    split_into_halves $size size_upper size_lower
+
+    echo "$addr_upper $addr_lower $size_upper $size_lower"
+}
+
 # data_type is either
 #   int
 #   hex
@@ -93,7 +116,7 @@ function add_device_tree_kernel()
 
     dt_mknode "$path" "module$addr"
     dt_set "$path/module$addr" "compatible" "str_a" "multiboot,kernel multiboot,module"
-    dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" $size)"
+    dt_set "$path/module$addr" "reg" "hex"  "$(split_addr_size $addr $size)"
     dt_set "$path/module$addr" "bootargs" "str" "$bootargs"
 }
 
@@ -106,7 +129,7 @@ function add_device_tree_ramdisk()
 
     dt_mknode "$path"  "module$addr"
     dt_set "$path/module$addr" "compatible" "str_a" "multiboot,ramdisk multiboot,module"
-    dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" $size)"
+    dt_set "$path/module$addr" "reg" "hex"  "$(split_addr_size $addr $size)"
 }
 
 
@@ -118,7 +141,7 @@ function add_device_tree_passthrough()
 
     dt_mknode "$path"  "module$addr"
     dt_set "$path/module$addr" "compatible" "str_a" "multiboot,device-tree multiboot,module"
-    dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" $size)"
+    dt_set "$path/module$addr" "reg" "hex"  "$(split_addr_size $addr $size)"
 }
 
 function add_device_tree_mem()
@@ -260,7 +283,7 @@ function xen_device_tree_editing()
     then
         dt_mknode "/chosen" "dom0"
         dt_set "/chosen/dom0" "compatible" "str_a" "xen,linux-zimage xen,multiboot-module multiboot,module"
-        dt_set "/chosen/dom0" "reg" "hex" "0x0 $dom0_kernel_addr 0x0 $(printf "0x%x" $dom0_kernel_size)"
+        dt_set "/chosen/dom0" "reg" "hex" "$(split_addr_size $dom0_kernel_addr $dom0_kernel_size)"
         dt_set "/chosen" "xen,dom0-bootargs" "str" "$DOM0_CMD"
     fi
 
@@ -268,7 +291,7 @@ function xen_device_tree_editing()
     then
         dt_mknode "/chosen" "dom0-ramdisk"
         dt_set "/chosen/dom0-ramdisk" "compatible" "str_a" "xen,linux-initrd xen,multiboot-module multiboot,module"
-        dt_set "/chosen/dom0-ramdisk" "reg" "hex" "0x0 $ramdisk_addr 0x0 $(printf "0x%x" $ramdisk_size)"
+        dt_set "/chosen/dom0-ramdisk" "reg" "hex" "$(split_addr_size $ramdisk_addr $ramdisk_size)"
     fi
 
     i=0
-- 
2.25.1

On 10/10/22 10:29, Michal Orzel wrote:

Hi Michal

> At the moment, ImageBuilder assumes that all addresses/sizes are
> 32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
> value for the first cell. Because of that, we cannot specify
> MEMORY_START and MEMORY_END to be above 32-bits (e.g. to place the images
> in the upper memory bank).
> 
> Add support to properly handle 64-bit addresses/sizes:
>   - add function split_into_halves to split the value passed as a first
>     argument into upper and lower halves. These are then set as values for
>     variables passed respetively as the second and third argument,

s/respetively/respectively/

>   - add function split_addr_size to split address and size and form a
>     string to be passed to dt_set as data argument for reg property.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Changes in v2:
> - redesign a patch based on master-next instead of NXP dynamic assignment patch
> ---
>   scripts/uboot-script-gen | 33 ++++++++++++++++++++++++++++-----
>   1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index b24dca2b7f7e..09d237d192c1 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -22,6 +22,29 @@ function dt_mknode()
>       fi
>   }
>   
> +# Usage:
> +# split_into_halves <value> <variable_to_store_upper> <variable_to_store_lower>
> +function split_into_halves()
> +{
> +    local value=$1
> +    local upper=$2
> +    local lower=$3
> +
> +    eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
> +    eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
> +}
> +
> +function split_addr_size()
> +{
> +    local addr=$1
> +    local size=$2
> +
> +    split_into_halves $addr addr_upper addr_lower
> +    split_into_halves $size size_upper size_lower
> +

Just a minor observation, the variables addr_upper, addr_lower, 
size_upper and size_lower can be declared local.

> +    echo "$addr_upper $addr_lower $size_upper $size_lower"
> +}
> +
>   # data_type is either
>   #   int
>   #   hex
> @@ -93,7 +116,7 @@ function add_device_tree_kernel()
>   
>       dt_mknode "$path" "module$addr"
>       dt_set "$path/module$addr" "compatible" "str_a" "multiboot,kernel multiboot,module"
> -    dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" $size)"
> +    dt_set "$path/module$addr" "reg" "hex"  "$(split_addr_size $addr $size)"
>       dt_set "$path/module$addr" "bootargs" "str" "$bootargs"
>   }
>   
> @@ -106,7 +129,7 @@ function add_device_tree_ramdisk()
>   
>       dt_mknode "$path"  "module$addr"
>       dt_set "$path/module$addr" "compatible" "str_a" "multiboot,ramdisk multiboot,module"
> -    dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" $size)"
> +    dt_set "$path/module$addr" "reg" "hex"  "$(split_addr_size $addr $size)"
>   }
>   
>   
> @@ -118,7 +141,7 @@ function add_device_tree_passthrough()
>   
>       dt_mknode "$path"  "module$addr"
>       dt_set "$path/module$addr" "compatible" "str_a" "multiboot,device-tree multiboot,module"
> -    dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" $size)"
> +    dt_set "$path/module$addr" "reg" "hex"  "$(split_addr_size $addr $size)"
>   }
>   
>   function add_device_tree_mem()
> @@ -260,7 +283,7 @@ function xen_device_tree_editing()
>       then
>           dt_mknode "/chosen" "dom0"
>           dt_set "/chosen/dom0" "compatible" "str_a" "xen,linux-zimage xen,multiboot-module multiboot,module"
> -        dt_set "/chosen/dom0" "reg" "hex" "0x0 $dom0_kernel_addr 0x0 $(printf "0x%x" $dom0_kernel_size)"
> +        dt_set "/chosen/dom0" "reg" "hex" "$(split_addr_size $dom0_kernel_addr $dom0_kernel_size)"
>           dt_set "/chosen" "xen,dom0-bootargs" "str" "$DOM0_CMD"
>       fi
>   
> @@ -268,7 +291,7 @@ function xen_device_tree_editing()
>       then
>           dt_mknode "/chosen" "dom0-ramdisk"
>           dt_set "/chosen/dom0-ramdisk" "compatible" "str_a" "xen,linux-initrd xen,multiboot-module multiboot,module"
> -        dt_set "/chosen/dom0-ramdisk" "reg" "hex" "0x0 $ramdisk_addr 0x0 $(printf "0x%x" $ramdisk_size)"
> +        dt_set "/chosen/dom0-ramdisk" "reg" "hex" "$(split_addr_size $ramdisk_addr $ramdisk_size)"
>       fi
>   
>       i=0

-- 
Xenia

Hi Xenia,

On 10/10/2022 10:52, Xenia Ragiadakou wrote:
> 
> 
> On 10/10/22 10:29, Michal Orzel wrote:
> 
> Hi Michal
> 
>> At the moment, ImageBuilder assumes that all addresses/sizes are
>> 32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
>> value for the first cell. Because of that, we cannot specify
>> MEMORY_START and MEMORY_END to be above 32-bits (e.g. to place the images
>> in the upper memory bank).
>>
>> Add support to properly handle 64-bit addresses/sizes:
>>   - add function split_into_halves to split the value passed as a first
>>     argument into upper and lower halves. These are then set as values for
>>     variables passed respetively as the second and third argument,
> 
> s/respetively/respectively/
Ok.

> 
>>   - add function split_addr_size to split address and size and form a
>>     string to be passed to dt_set as data argument for reg property.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> Changes in v2:
>> - redesign a patch based on master-next instead of NXP dynamic assignment patch
>> ---
>>   scripts/uboot-script-gen | 33 ++++++++++++++++++++++++++++-----
>>   1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>> index b24dca2b7f7e..09d237d192c1 100755
>> --- a/scripts/uboot-script-gen
>> +++ b/scripts/uboot-script-gen
>> @@ -22,6 +22,29 @@ function dt_mknode()
>>       fi
>>   }
>>
>> +# Usage:
>> +# split_into_halves <value> <variable_to_store_upper> <variable_to_store_lower>
>> +function split_into_halves()
>> +{
>> +    local value=$1
>> +    local upper=$2
>> +    local lower=$3
>> +
>> +    eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
>> +    eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
>> +}
>> +
>> +function split_addr_size()
>> +{
>> +    local addr=$1
>> +    local size=$2
>> +
>> +    split_into_halves $addr addr_upper addr_lower
>> +    split_into_halves $size size_upper size_lower
>> +
> 
> Just a minor observation, the variables addr_upper, addr_lower,
> size_upper and size_lower can be declared local.
> 
This function is to be called to perform substitution and as such
is always executed within a subshell so no need for local.

~Michal

On 10/10/22 12:48, Michal Orzel wrote:
> Hi Xenia,
> 
> On 10/10/2022 10:52, Xenia Ragiadakou wrote:
>>
>>
>> On 10/10/22 10:29, Michal Orzel wrote:
>>
>> Hi Michal
>>
>>> At the moment, ImageBuilder assumes that all addresses/sizes are
>>> 32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
>>> value for the first cell. Because of that, we cannot specify
>>> MEMORY_START and MEMORY_END to be above 32-bits (e.g. to place the images
>>> in the upper memory bank).
>>>
>>> Add support to properly handle 64-bit addresses/sizes:
>>>    - add function split_into_halves to split the value passed as a first
>>>      argument into upper and lower halves. These are then set as values for
>>>      variables passed respetively as the second and third argument,
>>
>> s/respetively/respectively/
> Ok.
> 
>>
>>>    - add function split_addr_size to split address and size and form a
>>>      string to be passed to dt_set as data argument for reg property.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> Changes in v2:
>>> - redesign a patch based on master-next instead of NXP dynamic assignment patch
>>> ---
>>>    scripts/uboot-script-gen | 33 ++++++++++++++++++++++++++++-----
>>>    1 file changed, 28 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>>> index b24dca2b7f7e..09d237d192c1 100755
>>> --- a/scripts/uboot-script-gen
>>> +++ b/scripts/uboot-script-gen
>>> @@ -22,6 +22,29 @@ function dt_mknode()
>>>        fi
>>>    }
>>>
>>> +# Usage:
>>> +# split_into_halves <value> <variable_to_store_upper> <variable_to_store_lower>
>>> +function split_into_halves()
>>> +{
>>> +    local value=$1
>>> +    local upper=$2
>>> +    local lower=$3
>>> +
>>> +    eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
>>> +    eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
>>> +}
>>> +
>>> +function split_addr_size()
>>> +{
>>> +    local addr=$1
>>> +    local size=$2
>>> +
>>> +    split_into_halves $addr addr_upper addr_lower
>>> +    split_into_halves $size size_upper size_lower
>>> +
>>
>> Just a minor observation, the variables addr_upper, addr_lower,
>> size_upper and size_lower can be declared local.
>>
> This function is to be called to perform substitution and as such
> is always executed within a subshell so no need for local.

So split_addr_size() is supposed to be executed only in a subshell ... 
Ok I did not think of that. So neither addr or size need to be declared 
local.

-- 
Xenia

On 10/10/2022 12:48, Xenia Ragiadakou wrote:
> 
> 
> On 10/10/22 12:48, Michal Orzel wrote:
>> Hi Xenia,
>>
>> On 10/10/2022 10:52, Xenia Ragiadakou wrote:
>>>
>>>
>>> On 10/10/22 10:29, Michal Orzel wrote:
>>>
>>> Hi Michal
>>>
>>>> At the moment, ImageBuilder assumes that all addresses/sizes are
>>>> 32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
>>>> value for the first cell. Because of that, we cannot specify
>>>> MEMORY_START and MEMORY_END to be above 32-bits (e.g. to place the images
>>>> in the upper memory bank).
>>>>
>>>> Add support to properly handle 64-bit addresses/sizes:
>>>>    - add function split_into_halves to split the value passed as a first
>>>>      argument into upper and lower halves. These are then set as values for
>>>>      variables passed respetively as the second and third argument,
>>>
>>> s/respetively/respectively/
>> Ok.
>>
>>>
>>>>    - add function split_addr_size to split address and size and form a
>>>>      string to be passed to dt_set as data argument for reg property.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>> Changes in v2:
>>>> - redesign a patch based on master-next instead of NXP dynamic assignment patch
>>>> ---
>>>>    scripts/uboot-script-gen | 33 ++++++++++++++++++++++++++++-----
>>>>    1 file changed, 28 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>>>> index b24dca2b7f7e..09d237d192c1 100755
>>>> --- a/scripts/uboot-script-gen
>>>> +++ b/scripts/uboot-script-gen
>>>> @@ -22,6 +22,29 @@ function dt_mknode()
>>>>        fi
>>>>    }
>>>>
>>>> +# Usage:
>>>> +# split_into_halves <value> <variable_to_store_upper> <variable_to_store_lower>
>>>> +function split_into_halves()
>>>> +{
>>>> +    local value=$1
>>>> +    local upper=$2
>>>> +    local lower=$3
>>>> +
>>>> +    eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
>>>> +    eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
>>>> +}
>>>> +
>>>> +function split_addr_size()
>>>> +{
>>>> +    local addr=$1
>>>> +    local size=$2
>>>> +
>>>> +    split_into_halves $addr addr_upper addr_lower
>>>> +    split_into_halves $size size_upper size_lower
>>>> +
>>>
>>> Just a minor observation, the variables addr_upper, addr_lower,
>>> size_upper and size_lower can be declared local.
>>>
>> This function is to be called to perform substitution and as such
>> is always executed within a subshell so no need for local.
> 
> So split_addr_size() is supposed to be executed only in a subshell ...
> Ok I did not think of that. So neither addr or size need to be declared
> local.
Exactly, but in ImageBuilder we don't seem to use $1, $2, ... directly so
that is why I added local only for the arguments passed to this function.

> 
> --
> Xenia

~Michal

On Mon, 10 Oct 2022, Michal Orzel wrote:
> On 10/10/2022 12:48, Xenia Ragiadakou wrote:
> > 
> > 
> > On 10/10/22 12:48, Michal Orzel wrote:
> >> Hi Xenia,
> >>
> >> On 10/10/2022 10:52, Xenia Ragiadakou wrote:
> >>>
> >>>
> >>> On 10/10/22 10:29, Michal Orzel wrote:
> >>>
> >>> Hi Michal
> >>>
> >>>> At the moment, ImageBuilder assumes that all addresses/sizes are
> >>>> 32-bit max. It sets #{address,size}-cells to 0x2 and puts 0x0 as the
> >>>> value for the first cell. Because of that, we cannot specify
> >>>> MEMORY_START and MEMORY_END to be above 32-bits (e.g. to place the images
> >>>> in the upper memory bank).
> >>>>
> >>>> Add support to properly handle 64-bit addresses/sizes:
> >>>>    - add function split_into_halves to split the value passed as a first
> >>>>      argument into upper and lower halves. These are then set as values for
> >>>>      variables passed respetively as the second and third argument,
> >>>
> >>> s/respetively/respectively/
> >> Ok.
> >>
> >>>
> >>>>    - add function split_addr_size to split address and size and form a
> >>>>      string to be passed to dt_set as data argument for reg property.
> >>>>
> >>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> >>>> ---
> >>>> Changes in v2:
> >>>> - redesign a patch based on master-next instead of NXP dynamic assignment patch
> >>>> ---
> >>>>    scripts/uboot-script-gen | 33 ++++++++++++++++++++++++++++-----
> >>>>    1 file changed, 28 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> >>>> index b24dca2b7f7e..09d237d192c1 100755
> >>>> --- a/scripts/uboot-script-gen
> >>>> +++ b/scripts/uboot-script-gen
> >>>> @@ -22,6 +22,29 @@ function dt_mknode()
> >>>>        fi
> >>>>    }
> >>>>
> >>>> +# Usage:
> >>>> +# split_into_halves <value> <variable_to_store_upper> <variable_to_store_lower>
> >>>> +function split_into_halves()
> >>>> +{
> >>>> +    local value=$1
> >>>> +    local upper=$2
> >>>> +    local lower=$3
> >>>> +
> >>>> +    eval "$upper=$(printf "0x%X\n" $(($value >> 32)))"
> >>>> +    eval "$lower=$(printf "0x%X\n" $(($value & 0xFFFFFFFF)))"
> >>>> +}
> >>>> +
> >>>> +function split_addr_size()
> >>>> +{
> >>>> +    local addr=$1
> >>>> +    local size=$2
> >>>> +
> >>>> +    split_into_halves $addr addr_upper addr_lower
> >>>> +    split_into_halves $size size_upper size_lower
> >>>> +
> >>>
> >>> Just a minor observation, the variables addr_upper, addr_lower,
> >>> size_upper and size_lower can be declared local.
> >>>
> >> This function is to be called to perform substitution and as such
> >> is always executed within a subshell so no need for local.
> > 
> > So split_addr_size() is supposed to be executed only in a subshell ...
> > Ok I did not think of that. So neither addr or size need to be declared
> > local.
> Exactly, but in ImageBuilder we don't seem to use $1, $2, ... directly so
> that is why I added local only for the arguments passed to this function.

Thanks Michal for the patch and Xenia for the review.

I like to use local, not just because of necessity, but also just as a
way to "tag" variables that we know are not going to be used outside a
specific function. Almost like a naming convention.

I ended up committing a slightly modified version of split_addr_size that
should be simpler and slightly more performant:

function split_value()
{
    local value=$1
    printf "0x%X " "$(($value >> 32))"
    printf "0x%X " "$(($value & 0xFFFFFFFF))"
}

function split_addr_size()
{
    local addr=$1
    local size=$2

    split_value $addr
    split_value $size
}