Re: [ImageBuilder v2] Add support for 64-bit addresses/sizes

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> >>>> ---
> >>>> 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
}