1. Patchew
  2. Xen
  3. View series

[PATCH] Use hex for bitstream_size as expected by u-boot

Stefano Stabellini posted 1 patch 1 year ago
Failed in applying to current master (apply log)
[PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Stefano Stabellini 1 year ago 
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`
         if test "$UBOOT_SOURCE"
         then
             # we assume the FPGA device is 0 here
Re: [PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Julien Grall 1 year ago 
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

-- 
Julien Grall
Re: [PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Michal Orzel 1 year ago 
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
Re: [PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Julien Grall 1 year ago 

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

-- 
Julien Grall
Re: [PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Julien Grall 1 year ago 

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,

-- 
Julien Grall
Re: [PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Stefano Stabellini 1 year ago 
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.
Re: [PATCH] Use hex for bitstream_size as expected by u-boot
Posted by Michal Orzel 1 year ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.