| Message ID | alpine.DEB.2.22.394.2311091740490.3478774@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use hex for bitstream_size as expected by u-boot | expand |
Hi Stefano, On 10/11/2023 02:44, Stefano Stabellini wrote: > > > Reported-by: Brian Woods <brian@woods.art> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi Stefano, On 10/11/2023 01:44, Stefano Stabellini wrote: > Reported-by: Brian Woods <brian@woods.art> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen > index b284887..6e52da5 100755 > --- a/scripts/uboot-script-gen > +++ b/scripts/uboot-script-gen > @@ -790,7 +790,7 @@ bitstream_load_and_config() > check_file_type "$BITSTREAM" "BIT data" > bitstream_addr=$memaddr > load_file $BITSTREAM "fpga_bitstream" > - bitstream_size=$filesize > + bitstream_size=`printf "0x%X\n" $filesize` Looking at [1], there is no indication that the size parameter for "fpga load" should be hexadecimal. At the contrary, all the example I have found seems to use $filesize. Furthermore, this code also seems to have been added more than two years ago. I would have hope this was tested back then. So I think the commit message needs to contain a bit more information about why this is needed now. Is this a change in U-boot? Different U-boot configuration? Or just this was never tested/used. > if test "$UBOOT_SOURCE" > then > # we assume the FPGA device is 0 here > Cheers, [1] https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/124682257/U-Boot+FPGA+Driver
Hi Julien, On 10/11/2023 10:27, Julien Grall wrote: > > > Hi Stefano, > > On 10/11/2023 01:44, Stefano Stabellini wrote: >> Reported-by: Brian Woods <brian@woods.art> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >> >> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen >> index b284887..6e52da5 100755 >> --- a/scripts/uboot-script-gen >> +++ b/scripts/uboot-script-gen >> @@ -790,7 +790,7 @@ bitstream_load_and_config() >> check_file_type "$BITSTREAM" "BIT data" >> bitstream_addr=$memaddr >> load_file $BITSTREAM "fpga_bitstream" >> - bitstream_size=$filesize >> + bitstream_size=`printf "0x%X\n" $filesize` > > Looking at [1], there is no indication that the size parameter for "fpga > load" should be hexadecimal. At the contrary, all the example I have > found seems to use $filesize. U-boot expects size to be passed in hex format. You can see it here: https://github.com/u-boot/u-boot/blob/master/cmd/fpga.c#L60C20-L60C27 Also, AFAICT $filesize var that gets updated after image load (e.g. tftpb) is in hex format. ~Michal
On 10/11/2023 10:01, Michal Orzel wrote: > Hi Julien, > > On 10/11/2023 10:27, Julien Grall wrote: >> >> >> Hi Stefano, >> >> On 10/11/2023 01:44, Stefano Stabellini wrote: >>> Reported-by: Brian Woods <brian@woods.art> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >>> >>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen >>> index b284887..6e52da5 100755 >>> --- a/scripts/uboot-script-gen >>> +++ b/scripts/uboot-script-gen >>> @@ -790,7 +790,7 @@ bitstream_load_and_config() >>> check_file_type "$BITSTREAM" "BIT data" >>> bitstream_addr=$memaddr >>> load_file $BITSTREAM "fpga_bitstream" >>> - bitstream_size=$filesize >>> + bitstream_size=`printf "0x%X\n" $filesize` >> >> Looking at [1], there is no indication that the size parameter for "fpga >> load" should be hexadecimal. At the contrary, all the example I have >> found seems to use $filesize. > U-boot expects size to be passed in hex format. You can see it here: > https://github.com/u-boot/u-boot/blob/master/cmd/fpga.c#L60C20-L60C27 > > Also, AFAICT $filesize var that gets updated after image load (e.g. tftpb) is in hex format. Hmmm, now that you are saying this... I vaguely recall we had some issues in the past with the size. The format was not consistent across U-boot. Maybe this is the same problem here? [1] https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions
On 10/11/2023 10:08, Julien Grall wrote: > > > On 10/11/2023 10:01, Michal Orzel wrote: >> Hi Julien, >> >> On 10/11/2023 10:27, Julien Grall wrote: >>> >>> >>> Hi Stefano, >>> >>> On 10/11/2023 01:44, Stefano Stabellini wrote: >>>> Reported-by: Brian Woods <brian@woods.art> >>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >>>> >>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen >>>> index b284887..6e52da5 100755 >>>> --- a/scripts/uboot-script-gen >>>> +++ b/scripts/uboot-script-gen >>>> @@ -790,7 +790,7 @@ bitstream_load_and_config() >>>> check_file_type "$BITSTREAM" "BIT data" >>>> bitstream_addr=$memaddr >>>> load_file $BITSTREAM "fpga_bitstream" >>>> - bitstream_size=$filesize >>>> + bitstream_size=`printf "0x%X\n" $filesize` >>> >>> Looking at [1], there is no indication that the size parameter for "fpga >>> load" should be hexadecimal. At the contrary, all the example I have >>> found seems to use $filesize. >> U-boot expects size to be passed in hex format. You can see it here: >> https://github.com/u-boot/u-boot/blob/master/cmd/fpga.c#L60C20-L60C27 >> >> Also, AFAICT $filesize var that gets updated after image load (e.g. >> tftpb) is in hex format. > > Hmmm, now that you are saying this... I vaguely recall we had some > issues in the past with the size. The format was not consistent across > U-boot. > > Maybe this is the same problem here? I had a chat with Michal on Matrix. 'filesize' is a variable internal to the image-builder rather than U-boot. Hence the confusion. The variable will be set in decimal. So it needs to be converted to hexadecimal. So the code seems to be correct. That said, I think some clarifications is needed in the commit message to help understanding the code and at least giving a hint whether this code was ever tested (or this was introduced by a follow-up change). Cheers,
On Fri, 10 Nov 2023, Julien Grall wrote: > On 10/11/2023 10:08, Julien Grall wrote: > > On 10/11/2023 10:01, Michal Orzel wrote: > > > Hi Julien, > > > > > > On 10/11/2023 10:27, Julien Grall wrote: > > > > > > > > > > > > Hi Stefano, > > > > > > > > On 10/11/2023 01:44, Stefano Stabellini wrote: > > > > > Reported-by: Brian Woods <brian@woods.art> > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > > > > > > > > diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen > > > > > index b284887..6e52da5 100755 > > > > > --- a/scripts/uboot-script-gen > > > > > +++ b/scripts/uboot-script-gen > > > > > @@ -790,7 +790,7 @@ bitstream_load_and_config() > > > > > check_file_type "$BITSTREAM" "BIT data" > > > > > bitstream_addr=$memaddr > > > > > load_file $BITSTREAM "fpga_bitstream" > > > > > - bitstream_size=$filesize > > > > > + bitstream_size=`printf "0x%X\n" $filesize` > > > > > > > > Looking at [1], there is no indication that the size parameter for "fpga > > > > load" should be hexadecimal. At the contrary, all the example I have > > > > found seems to use $filesize. > > > U-boot expects size to be passed in hex format. You can see it here: > > > https://github.com/u-boot/u-boot/blob/master/cmd/fpga.c#L60C20-L60C27 > > > > > > Also, AFAICT $filesize var that gets updated after image load (e.g. tftpb) > > > is in hex format. > > > > Hmmm, now that you are saying this... I vaguely recall we had some issues in > > the past with the size. The format was not consistent across U-boot. > > > > Maybe this is the same problem here? > > I had a chat with Michal on Matrix. 'filesize' is a variable internal to the > image-builder rather than U-boot. Hence the confusion. > > The variable will be set in decimal. So it needs to be converted to > hexadecimal. So the code seems to be correct. > > That said, I think some clarifications is needed in the commit message to help > understanding the code and at least giving a hint whether this code was ever > tested (or this was introduced by a follow-up change). Makes sense, I added an explanation on commit. Passing the parameter as hex with 0x should make it work on both old and new versions of u-boot.
diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen index b284887..6e52da5 100755 --- a/scripts/uboot-script-gen +++ b/scripts/uboot-script-gen @@ -790,7 +790,7 @@ bitstream_load_and_config() check_file_type "$BITSTREAM" "BIT data" bitstream_addr=$memaddr load_file $BITSTREAM "fpga_bitstream" - bitstream_size=$filesize + bitstream_size=`printf "0x%X\n" $filesize` if test "$UBOOT_SOURCE" then # we assume the FPGA device is 0 here
Reported-by: Brian Woods <brian@woods.art> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>