[U-Boot] [PATCH] common: Fix logic in fpga programming

[Michal Simek]

Stop boot process if fpga programming fails.

Signed-off-by: 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");

[Mike Looijmans]

Post by Michal Simek
Stop boot process if fpga programming fails.
---
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;

Wouldn't "return err;" or "return -EIO;" make sense here instead of a magic "1"?

Post by Michal Simek
+
+ printf(" Programming %s bitstream... OK", name);
break;
printf("The given image format is not supported (corrupt?)\n");

Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: [phone]
E-mail: ***@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

[Michal Simek]

Post by Mike Looijmans

Post by Michal Simek
Stop boot process if fpga programming fails.
---
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;

Wouldn't "return err;" or "return -EIO;" make sense here instead of a magic "1"?

Good idea. Will send v2

Thanks,
Michal

[Wolfgang Denk]

Dear Michal,

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

[Michal Simek]

Dear Wolfgang,

Post by Wolfgang Denk
Dear Michal,

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.

Post by Wolfgang Denk

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