[U-Boot] [PATCH] common: Fix logic in fpga programming
Stop boot process if fpga programming fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
common/image.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/common/image.c b/common/image.c index bd07e86701a4..e3486e46a459 100644 --- a/common/image.c +++ b/common/image.c @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, img_len, BIT_PARTIAL); }
- printf(" Programming %s bitstream... ", name); if (err) - printf("failed\n"); - else - printf("OK\n"); + return 1; + + printf(" Programming %s bitstream... OK", name); break; default: printf("The given image format is not supported (corrupt?)\n");
On 16-12-16 10:45, Michal Simek wrote:
Stop boot process if fpga programming fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com
common/image.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/common/image.c b/common/image.c index bd07e86701a4..e3486e46a459 100644 --- a/common/image.c +++ b/common/image.c @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, img_len, BIT_PARTIAL); }
printf(" Programming %s bitstream... ", name);printf("failed\n");elseprintf("OK\n");
return 1;
Wouldn't "return err;" or "return -EIO;" make sense here instead of a magic "1"?
printf(" Programming %s bitstream... OK", name);
Kind regards,
Mike Looijmans System Expert
TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com
Please consider the environment before printing this e-mail
On 16.12.2016 10:50, Mike Looijmans wrote:
On 16-12-16 10:45, Michal Simek wrote:
Stop boot process if fpga programming fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com
common/image.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/common/image.c b/common/image.c index bd07e86701a4..e3486e46a459 100644 --- a/common/image.c +++ b/common/image.c @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, img_len, BIT_PARTIAL); }
printf(" Programming %s bitstream... ", name); if (err)printf("failed\n");elseprintf("OK\n");
return 1;Wouldn't "return err;" or "return -EIO;" make sense here instead of a magic "1"?
Good idea. Will send v2
Thanks, Michal
Dear Michal...
In message 1e029d3a67722c20d8b6194d3388efe5ca92f5bf.1481881501.git.michal.simek@xilinx.com you wrote:
Stop boot process if fpga programming fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com
common/image.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/common/image.c b/common/image.c index bd07e86701a4..e3486e46a459 100644 --- a/common/image.c +++ b/common/image.c @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, img_len, BIT_PARTIAL); }
printf(" Programming %s bitstream... ", name);printf("failed\n");elseprintf("OK\n");
return 1;printf(" Programming %s bitstream... OK", name);
Good old U-Boot style says we print a message when we start an operation, and then an OK / failed when done. When the system crashes in this command, you can see at last where it crashed, i. e. what the last running command was.
Your change breaks this - in the error path nothing gets printed.
This is even worse, as even though the system keeps running the user has no indication of what failed...
I suggest to keep the printf() as before, and just fix the return code thing.
Best regards,
Wolfgang Denk
On 20.12.2016 18:30, Wolfgang Denk wrote:
Dear Michal...
In message 1e029d3a67722c20d8b6194d3388efe5ca92f5bf.1481881501.git.michal.simek@xilinx.com you wrote:
Stop boot process if fpga programming fails.
Signed-off-by: Michal Simek michal.simek@xilinx.com
common/image.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/common/image.c b/common/image.c index bd07e86701a4..e3486e46a459 100644 --- a/common/image.c +++ b/common/image.c @@ -1375,11 +1375,10 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, img_len, BIT_PARTIAL); }
printf(" Programming %s bitstream... ", name);printf("failed\n");elseprintf("OK\n");
return 1;printf(" Programming %s bitstream... OK", name);Good old U-Boot style says we print a message when we start an operation, and then an OK / failed when done. When the system crashes in this command, you can see at last where it crashed, i. e. what the last running command was.
Based on this style the first part of this message should be called at the beginning of this function not in this possition.
Your change breaks this - in the error path nothing gets printed.
If you look at the code 253 ret = boot_get_fpga(argc, argv, &images, IH_ARCH_DEFAULT, 254 NULL, NULL); 255 if (ret) { 256 printf("FPGA image is corrupted or invalid\n"); 257 return 1; 258 }
There is already printf for error.
This is even worse, as even though the system keeps running the user has no indication of what failed...
Isn't it message above enough?
Thanks, Michal
Dear Michal,
In message 0e8b363f-f84f-cea2-7784-8cc10e1e751b@xilinx.com you wrote:
Good old U-Boot style says we print a message when we start an operation, and then an OK / failed when done. When the system crashes in this command, you can see at last where it crashed, i. e. what the last running command was.
Based on this style the first part of this message should be called at the beginning of this function not in this possition.
True,,,
255 if (ret) { 256 printf("FPGA image is corrupted or invalid\n"); 257 return 1; 258 }
There is already printf for error.
Hm... Sorry, I did not follow all brachnes thrugh the code this i snot exactly obvious from the patch.
This is even worse, as even though the system keeps running the user has no indication of what failed...
Isn't it message above enough?
In the patch, I see only a "return 1;".
Best regards,
Wolfgang Denk
Dear Wolfgang,
On 21.12.2016 00:19, Wolfgang Denk wrote:
Dear Michal,
In message 0e8b363f-f84f-cea2-7784-8cc10e1e751b@xilinx.com you wrote:
Good old U-Boot style says we print a message when we start an operation, and then an OK / failed when done. When the system crashes in this command, you can see at last where it crashed, i. e. what the last running command was.
Based on this style the first part of this message should be called at the beginning of this function not in this possition.
True,,,
255 if (ret) { 256 printf("FPGA image is corrupted or invalid\n"); 257 return 1; 258 }
There is already printf for error.
Hm... Sorry, I did not follow all brachnes thrugh the code this i snot exactly obvious from the patch.
ok.
This is even worse, as even though the system keeps running the user has no indication of what failed...
Isn't it message above enough?
In the patch, I see only a "return 1;".
Like in every patch which error out in the middle of function and return 0 at the end. Please look at that function and you will see that at the end there is return 0.
Thanks, Michal
participants (3)
-
Michal Simek -
Mike Looijmans -
Wolfgang Denk