| Message ID | 20221010072947.8300-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ImageBuilder,v2] Add support for 64-bit addresses/sizes | expand |
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
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.
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 }
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
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(-)