[U-Boot] [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present
CONFIG_OF_CONTROL requires a valid device tree. However, we cannot call panic() before the console is set up since the message does not appear, and we get a silent failure.
Remove the panic from fdtdec_check_fdt() and provide a new function to prepare the fdt for use. This will be called after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org --- include/fdtdec.h | 17 +++++++++++++++-- lib/fdtdec.c | 24 +++++++++++++++++++----- 2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/include/fdtdec.h b/include/fdtdec.h index 766e0bd..d45acbe 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -157,8 +157,21 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name, int fdtdec_get_is_enabled(const void *blob, int node);
/** - * Checks whether we have a valid fdt available to control U-Boot, and panic - * if not. + * Make sure we have a valid fdt available to control U-Boot. + * + * If not, a message is printed to the console if the console is ready. + * + * @return 0 if all ok, -1 if not + */ +int fdtdec_prepare_fdt(void); + +/** + * Checks that we have a valid fdt available to control U-Boot. + + * However, if not then for the moment nothing is done, since this function + * is called too early to panic(). + * + * @returns 0 */ int fdtdec_check_fdt(void);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 9241d13..0fb6d17 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -276,17 +276,31 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name, return num_found; }
+int fdtdec_check_fdt(void) +{ + /* + * We must have an FDT, but we cannot panic() yet since the console + * is not ready. So for now, just assert(). Boards which need an early + * FDT (prior to console ready) will need to make their own + * arrangements and do their own checks. + */ + assert(!fdtdec_prepare_fdt()); + return 0; +} + /* * This function is a little odd in that it accesses global data. At some * point if the architecture board.c files merge this will make more sense. * Even now, it is common code. */ -int fdtdec_check_fdt(void) +int fdtdec_prepare_fdt(void) { - /* We must have an fdt */ - if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob)) - panic("No valid fdt found - please append one to U-Boot\n" - "binary or define CONFIG_OF_EMBED\n"); + if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob)) { + printf("No valid FDT found - please append one to U-Boot " + "binary, use u-boot-dtb.bin or define " + "CONFIG_OF_EMBED\n"); + return -1; + } return 0; }
When using CONFIG_OF_CONTROL, add a check that we have a valid FDT and panic() if not. This must be done after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org --- arch/arm/lib/board.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index 81293c3..ab88e9c 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -291,6 +291,14 @@ void board_init_f(ulong bootflag) } }
+#ifdef CONFIG_OF_CONTROL + /* For now, put this check after the console is ready */ + if (fdtdec_prepare_fdt()) { + panic("** CONFIG_OF_CONTROL defined but no FDT - please see " + "doc/README.fdt-control"); + } +#endif + debug("monitor len: %08lX\n", gd->mon_len); /* * Ram is setup, size stored in gd !!
Simon,
-----Original Message----- From: Simon Glass [mailto:sjg@chromium.org] Sent: Wednesday, March 28, 2012 1:08 PM To: U-Boot Mailing List Cc: Tom Warren; Stephen Warren; Albert Aribaud; Simon Glass Subject: [PATCH 2/2] arm: Check for valid FDT after console is up
When using CONFIG_OF_CONTROL, add a check that we have a valid FDT and panic() if not. This must be done after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org
This works (u-boot.bin gives an error message and then resets). If you're happy with the constant reset loop and not a hang, I'm OK with it, too.
Tested-by: Tom Warren twarren@nvidia.com Acked-by: Tom Warren twarren@nvidia.com
arch/arm/lib/board.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index 81293c3..ab88e9c 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -291,6 +291,14 @@ void board_init_f(ulong bootflag) } }
+#ifdef CONFIG_OF_CONTROL
- /* For now, put this check after the console is ready */
- if (fdtdec_prepare_fdt()) {
panic("** CONFIG_OF_CONTROL defined but no FDT - please see ""doc/README.fdt-control");- }
+#endif
- debug("monitor len: %08lX\n", gd->mon_len); /*
- Ram is setup, size stored in gd !!
-- 1.7.7.3
+Wolfgang
Hi Tom,
On Wed, Mar 28, 2012 at 1:34 PM, Tom Warren TWarren@nvidia.com wrote:
Simon,
-----Original Message----- From: Simon Glass [mailto:sjg@chromium.org] Sent: Wednesday, March 28, 2012 1:08 PM To: U-Boot Mailing List Cc: Tom Warren; Stephen Warren; Albert Aribaud; Simon Glass Subject: [PATCH 2/2] arm: Check for valid FDT after console is up
When using CONFIG_OF_CONTROL, add a check that we have a valid FDT and panic() if not. This must be done after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org
This works (u-boot.bin gives an error message and then resets). If you're happy with the constant reset loop and not a hang, I'm OK with it, too.
Tested-by: Tom Warren twarren@nvidia.com Acked-by: Tom Warren twarren@nvidia.com
Thanks. You can define CONFIG_PANIC_HANG for that behaviour.
I would like to get these applied for the upcoming release, since the recent revert of the pre-console putc() has left us otherwise completely without a solution to this problem.
Regards, Simon
arch/arm/lib/board.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index 81293c3..ab88e9c 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -291,6 +291,14 @@ void board_init_f(ulong bootflag) } }
+#ifdef CONFIG_OF_CONTROL
- /* For now, put this check after the console is ready */
- if (fdtdec_prepare_fdt()) {
- panic("** CONFIG_OF_CONTROL defined but no FDT - please see "
- "doc/README.fdt-control");
- }
+#endif
debug("monitor len: %08lX\n", gd->mon_len); /* * Ram is setup, size stored in gd !! -- 1.7.7.3
-- nvpublic
On 03/28/2012 02:08 PM, Simon Glass wrote:
When using CONFIG_OF_CONTROL, add a check that we have a valid FDT and panic() if not. This must be done after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org
Excellent. The behavior after this series is exactly what I was looking for.
One query on this patch though: Is there a need for arch/arm/lib/board.c:init_sequence[] to still include the call to fdtdec_check_fdt()? After all, if the console doesn't come from FDT, then the panic() call this patch adds will show the message so there's no need to check it earlier, and if the console does come from FDT, there's little point executing that early assert() since the message goes nowhere - presumably the code that extracts the console from FDT would perform this panic if required. So, I would have just moved the call to the existing fdtdec_check_fdt() myself, rather than splitting it into two.
Still, as far as I'm concerned, this comment can be addressed in a later cleanup patch. So, the series:
Tested-by: Stephen Warren swarren@wwwdotorg.org Acked-by: Stephen Warren swarren@wwwdotorg.org
Hi Stephen,
On Wed, Mar 28, 2012 at 2:11 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 03/28/2012 02:08 PM, Simon Glass wrote:
When using CONFIG_OF_CONTROL, add a check that we have a valid FDT and panic() if not. This must be done after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org
Excellent. The behavior after this series is exactly what I was looking for.
Thanks, I'm pleased. The behavior was never in question, only the means to make it work.
One query on this patch though: Is there a need for arch/arm/lib/board.c:init_sequence[] to still include the call to fdtdec_check_fdt()? After all, if the console doesn't come from FDT, then the panic() call this patch adds will show the message so there's no need to check it earlier, and if the console does come from FDT, there's little point executing that early assert() since the message goes nowhere - presumably the code that extracts the console from FDT would perform this panic if required. So, I would have just moved the call to the existing fdtdec_check_fdt() myself, rather than splitting it into two.
I did certainly look at this, but I know we are going to hit this problem again. As you say the console-from-FDT code will currently have to do its own check, and I'm not entirely comfortable with that - it would be nice it the fdtdec_check_fdt() could actually do something useful. But I'm going to wait until another sub-arch has FDT support (and I have put up my fdt_serial series) before worrying about it further.
Still, as far as I'm concerned, this comment can be addressed in a later cleanup patch. So, the series:
Tested-by: Stephen Warren swarren@wwwdotorg.org Acked-by: Stephen Warren swarren@wwwdotorg.org
Great!
Regards, Simon
Simon,
-----Original Message----- From: Simon Glass [mailto:sjg@chromium.org] Sent: Wednesday, March 28, 2012 1:08 PM To: U-Boot Mailing List Cc: Tom Warren; Stephen Warren; Albert Aribaud; Simon Glass; Jerry Van Baren; Devicetree Discuss Subject: [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present
CONFIG_OF_CONTROL requires a valid device tree. However, we cannot call panic() before the console is set up since the message does not appear, and we get a silent failure.
Remove the panic from fdtdec_check_fdt() and provide a new function to prepare the fdt for use. This will be called after the console is ready.
Signed-off-by: Simon Glass sjg@chromium.org
Tested-by: Tom Warren twarren@nvidia.com Acked-by: Tom Warren twarren@nvidia.com
include/fdtdec.h | 17 +++++++++++++++-- lib/fdtdec.c | 24 +++++++++++++++++++----- 2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/include/fdtdec.h b/include/fdtdec.h index 766e0bd..d45acbe 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -157,8 +157,21 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name, int fdtdec_get_is_enabled(const void *blob, int node);
/**
- Checks whether we have a valid fdt available to control U-Boot, and
panic
- if not.
- Make sure we have a valid fdt available to control U-Boot.
- If not, a message is printed to the console if the console is ready.
- @return 0 if all ok, -1 if not
- */
+int fdtdec_prepare_fdt(void);
+/**
- Checks that we have a valid fdt available to control U-Boot.
- However, if not then for the moment nothing is done, since this
- function
- is called too early to panic().
*/
- @returns 0
int fdtdec_check_fdt(void);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 9241d13..0fb6d17 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -276,17 +276,31 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name, return num_found; }
+int fdtdec_check_fdt(void) +{
- /*
* We must have an FDT, but we cannot panic() yet since the console* is not ready. So for now, just assert(). Boards which need anearly
* FDT (prior to console ready) will need to make their own* arrangements and do their own checks.*/- assert(!fdtdec_prepare_fdt());
- return 0;
+}
/*
- This function is a little odd in that it accesses global data. At some
- point if the architecture board.c files merge this will make more sense.
- Even now, it is common code.
*/ -int fdtdec_check_fdt(void) +int fdtdec_prepare_fdt(void) {
- /* We must have an fdt */
- if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob))
panic("No valid fdt found - please append one to U-Boot\n""binary or define CONFIG_OF_EMBED\n");
- if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob))
{
printf("No valid FDT found - please append one to U-Boot ""binary, use u-boot-dtb.bin or define ""CONFIG_OF_EMBED\n");return -1;- } return 0;
}
-- 1.7.7.3
Dear Simon Glass,
In message 1332965305-21151-1-git-send-email-sjg@chromium.org you wrote:
CONFIG_OF_CONTROL requires a valid device tree. However, we cannot call panic() before the console is set up since the message does not appear, and we get a silent failure.
...
+int fdtdec_prepare_fdt(void);
+/**
- Checks that we have a valid fdt available to control U-Boot.
- However, if not then for the moment nothing is done, since this function
- is called too early to panic().
- @returns 0
If the function always returns 0, then it makes no sense to return any value at all. Please make it void, then.
+int fdtdec_check_fdt(void) +{
- /*
* We must have an FDT, but we cannot panic() yet since the console* is not ready. So for now, just assert(). Boards which need an early* FDT (prior to console ready) will need to make their own* arrangements and do their own checks.*/- assert(!fdtdec_prepare_fdt());
- return 0;
+}
Ditto - make that "void fdtdec_check_fdt(void)".
+int fdtdec_prepare_fdt(void)
...
return 0; }
Ditto - make that "void fdtdec_prepare_fdt(void)".
I have to admit that I do not understand how this patch will help you anything. You write "we cannot panic()", but "just assert()".
If the assertion fails, it will call __assert_fail() - which in turn will just call panic(). So you are out of the frying pan into the fire.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Wed, Mar 28, 2012 at 11:30 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1332965305-21151-1-git-send-email-sjg@chromium.org you wrote:
CONFIG_OF_CONTROL requires a valid device tree. However, we cannot call panic() before the console is set up since the message does not appear, and we get a silent failure.
...
+int fdtdec_prepare_fdt(void);
+/**
- Checks that we have a valid fdt available to control U-Boot.
- However, if not then for the moment nothing is done, since this function
- is called too early to panic().
- @returns 0
If the function always returns 0, then it makes no sense to return any value at all. Please make it void, then.
fdtdec_check_fdt() is used in the ARM init sequence, so I need to follow that signature and return a value. In fact I must return 0 or the board will hang.
+int fdtdec_check_fdt(void) +{
- /*
- * We must have an FDT, but we cannot panic() yet since the console
- * is not ready. So for now, just assert(). Boards which need an early
- * FDT (prior to console ready) will need to make their own
- * arrangements and do their own checks.
- */
- assert(!fdtdec_prepare_fdt());
- return 0;
+}
Ditto - make that "void fdtdec_check_fdt(void)".
+int fdtdec_prepare_fdt(void)
...
return 0; }
Ditto - make that "void fdtdec_prepare_fdt(void)".
This fdtdec_prepare_fdt() is intended to provide a way to find out if there is a valid fdt. The 'return 0' that you have shown is only if it succeeds - it does actually return -1 on failure. I want a function which just checks this, and returns the result (rather than dying if it fails, which the behaviour that in principle I would like in the init sequence).
I have to admit that I do not understand how this patch will help you anything. You write "we cannot panic()", but "just assert()".
If the assertion fails, it will call __assert_fail() - which in turn will just call panic(). So you are out of the frying pan into the fire.
Yes, but only if DEBUG is turned on in fdtdec. Otherwise we just continue and panic later when we can report the error.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Life. Don't talk to me about life. - Marvin the Paranoid Android
participants (4)
-
Simon Glass -
Stephen Warren -
Tom Warren -
Wolfgang Denk