Re: [U-Boot] [PATCH v3 01/20] spl: fix binman_sym output check

[Simon Glass]

On 13 March 2018 at 21:36, Maxime Ripard  wrote:
> Hi Simon,
>
> On Wed, Feb 28, 2018 at 08:51:43PM +0100, Miquel Raynal wrote:
>> A previous commit introduced the use of binman in the SPL.
>>
>> After the binman_sym call over the 'pos' symbol, the output value is
>> checked against BINMAN_SYM_MISSING (-1UL). According to the
>> documentation (tools/binman/README), when it comes to the 'pos'
>> attribute:
>>
>> pos:
>>   This sets the position of an entry within the image. The first
>>   byte of the image is normally at position 0. If 'pos' is not
>>   provided, binman sets it to the end of the previous region, or
>>   the start of the image's entry area (normally 0) if there is no
>>   previous region.
>>
>> So instead of checking if the return value is BINMAN_SYM_MISSING, we
>> should also check if the value is not null.
>>
>> The failure happens when using both the SPL file and the U-Boot file
>> independently instead of the concatenated file (SPL + padding + U-Boot).
>> This is because the U-Boot binary file alone does not have the U-Boot
>> header while it is present in the concatenation file. Not having the
>> header forces the SPL to discover where it should load U-Boot. The
>> binman_sym call is supposed to do that but fails. Because of the wrong
>> check, the destination address was set to 0 while it should have been
>> somewhere in RAM. This, obviously, stalls the board.
>>
>> Fixes: 8bee2d251afb ("binman: Add binman symbol support to SPL")
>> Signed-off-by: Miquel Raynal 
>
> I'm not sure why it wasn't sent to you, but could you please have a
> look at that patch?
>
> Thanks!
> Maxime
>
>> ---
>>  common/spl/spl.c | 10 --
>>  1 file changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 01/20] spl: fix binman_sym output check

[Maxime Ripard]

On Tue, Mar 13, 2018 at 02:36:06PM +0100, Maxime Ripard wrote:
> Hi Simon,
> 
> On Wed, Feb 28, 2018 at 08:51:43PM +0100, Miquel Raynal wrote:
> > A previous commit introduced the use of binman in the SPL.
> > 
> > After the binman_sym call over the 'pos' symbol, the output value is
> > checked against BINMAN_SYM_MISSING (-1UL). According to the
> > documentation (tools/binman/README), when it comes to the 'pos'
> > attribute:
> > 
> > pos:
> > This sets the position of an entry within the image. The first
> > byte of the image is normally at position 0. If 'pos' is not
> > provided, binman sets it to the end of the previous region, or
> > the start of the image's entry area (normally 0) if there is no
> > previous region.
> > 
> > So instead of checking if the return value is BINMAN_SYM_MISSING, we
> > should also check if the value is not null.
> > 
> > The failure happens when using both the SPL file and the U-Boot file
> > independently instead of the concatenated file (SPL + padding + U-Boot).
> > This is because the U-Boot binary file alone does not have the U-Boot
> > header while it is present in the concatenation file. Not having the
> > header forces the SPL to discover where it should load U-Boot. The
> > binman_sym call is supposed to do that but fails. Because of the wrong
> > check, the destination address was set to 0 while it should have been
> > somewhere in RAM. This, obviously, stalls the board.
> > 
> > Fixes: 8bee2d251afb ("binman: Add binman symbol support to SPL")
> > Signed-off-by: Miquel Raynal 
> 
> I'm not sure why it wasn't sent to you, but could you please have a
> look at that patch?

Ping?

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 01/20] spl: fix binman_sym output check

[Maxime Ripard]

Hi Simon,

On Wed, Feb 28, 2018 at 08:51:43PM +0100, Miquel Raynal wrote:
> A previous commit introduced the use of binman in the SPL.
> 
> After the binman_sym call over the 'pos' symbol, the output value is
> checked against BINMAN_SYM_MISSING (-1UL). According to the
> documentation (tools/binman/README), when it comes to the 'pos'
> attribute:
> 
> pos:
>   This sets the position of an entry within the image. The first
>   byte of the image is normally at position 0. If 'pos' is not
>   provided, binman sets it to the end of the previous region, or
>   the start of the image's entry area (normally 0) if there is no
>   previous region.
> 
> So instead of checking if the return value is BINMAN_SYM_MISSING, we
> should also check if the value is not null.
> 
> The failure happens when using both the SPL file and the U-Boot file
> independently instead of the concatenated file (SPL + padding + U-Boot).
> This is because the U-Boot binary file alone does not have the U-Boot
> header while it is present in the concatenation file. Not having the
> header forces the SPL to discover where it should load U-Boot. The
> binman_sym call is supposed to do that but fails. Because of the wrong
> check, the destination address was set to 0 while it should have been
> somewhere in RAM. This, obviously, stalls the board.
> 
> Fixes: 8bee2d251afb ("binman: Add binman symbol support to SPL")
> Signed-off-by: Miquel Raynal 

I'm not sure why it wasn't sent to you, but could you please have a
look at that patch?

Thanks!
Maxime

> ---
>  common/spl/spl.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index b1ce56d0d0..61d3071324 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -127,8 +127,14 @@ void spl_set_header_raw_uboot(struct spl_image_info 
> *spl_image)
>   ulong u_boot_pos = binman_sym(ulong, u_boot_any, pos);
>  
>   spl_image->size = CONFIG_SYS_MONITOR_LEN;
> - if (u_boot_pos != BINMAN_SYM_MISSING) {
> - /* biman does not support separate entry addresses at present */
> +
> + /*
> +  * Binman error cases: address of the end of the previous region or the
> +  * start of the image's entry area (usually 0) if there is no previous
> +  * region.
> +  */
> + if (u_boot_pos && u_boot_pos != BINMAN_SYM_MISSING) {
> + /* Binman does not support separated entry addresses */
>   spl_image->entry_point = u_boot_pos;
>   spl_image->load_addr = u_boot_pos;
>   } else {
> -- 
> 2.14.1
> 

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v3 01/20] spl: fix binman_sym output check

[Miquel Raynal]

A previous commit introduced the use of binman in the SPL.

After the binman_sym call over the 'pos' symbol, the output value is
checked against BINMAN_SYM_MISSING (-1UL). According to the
documentation (tools/binman/README), when it comes to the 'pos'
attribute:

pos:
This sets the position of an entry within the image. The first
byte of the image is normally at position 0. If 'pos' is not
provided, binman sets it to the end of the previous region, or
the start of the image's entry area (normally 0) if there is no
previous region.

So instead of checking if the return value is BINMAN_SYM_MISSING, we
should also check if the value is not null.

The failure happens when using both the SPL file and the U-Boot file
independently instead of the concatenated file (SPL + padding + U-Boot).
This is because the U-Boot binary file alone does not have the U-Boot
header while it is present in the concatenation file. Not having the
header forces the SPL to discover where it should load U-Boot. The
binman_sym call is supposed to do that but fails. Because of the wrong
check, the destination address was set to 0 while it should have been
somewhere in RAM. This, obviously, stalls the board.

Fixes: 8bee2d251afb ("binman: Add binman symbol support to SPL")
Signed-off-by: Miquel Raynal 
---
 common/spl/spl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index b1ce56d0d0..61d3071324 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -127,8 +127,14 @@ void spl_set_header_raw_uboot(struct spl_image_info 
*spl_image)
ulong u_boot_pos = binman_sym(ulong, u_boot_any, pos);
 
spl_image->size = CONFIG_SYS_MONITOR_LEN;
-   if (u_boot_pos != BINMAN_SYM_MISSING) {
-   /* biman does not support separate entry addresses at present */
+
+   /*
+* Binman error cases: address of the end of the previous region or the
+* start of the image's entry area (usually 0) if there is no previous
+* region.
+*/
+   if (u_boot_pos && u_boot_pos != BINMAN_SYM_MISSING) {
+   /* Binman does not support separated entry addresses */
spl_image->entry_point = u_boot_pos;
spl_image->load_addr = u_boot_pos;
} else {
-- 
2.14.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot