[PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Simon Glass]

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;
 }
 
-- 
1.7.7.3

[U-Boot] [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Simon Glass]

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;
 }
 
-- 
1.7.7.3

[U-Boot] [PATCH 2/2] arm: Check for valid FDT after console is up

[Simon Glass]

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 !!
-- 
1.7.7.3

Re: [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Tom Warren]

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 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;
>  }
> 
> --
> 1.7.7.3
-- 
nvpublic

[U-Boot] [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Tom Warren]

Simon,

> -----Original Message-----
> From: Simon Glass [mailto:sjg at 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 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;
>  }
> 
> --
> 1.7.7.3
-- 
nvpublic

[U-Boot] [PATCH 2/2] arm: Check for valid FDT after console is up

[Tom Warren]

Simon,

> -----Original Message-----
> From: Simon Glass [mailto:sjg at 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
-- 
nvpublic

[U-Boot] [PATCH 2/2] arm: Check for valid FDT after console is up

[Simon Glass]

+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 at 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

[U-Boot] [PATCH 2/2] arm: Check for valid FDT after console is up

[Stephen Warren]

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>

[U-Boot] [PATCH 2/2] arm: Check for valid FDT after console is up

[Simon Glass]

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

Re: [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Wolfgang Denk]

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

-- 
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

[U-Boot] [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Wolfgang Denk]

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

-- 
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 at denx.de
Life. Don't talk to me about life.      - Marvin the Paranoid Android

Re: [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Simon Glass]

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

[U-Boot] [PATCH 1/2] fdt: Avoid early panic() when there is no FDT present

[Simon Glass]

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 at denx.de
> Life. Don't talk to me about life. ? ? ?- Marvin the Paranoid Android