From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Tue, 1 Sep 2015 22:14:16 -0500 Subject: [U-Boot] [PATCH 2/2] fdt_support: Don't panic if unable to perform stdout fixup In-Reply-To: References: <1441073150-15143-1-git-send-email-scottwood@freescale.com> <1441073484-15640-1-git-send-email-scottwood@freescale.com> <1441077387.4966.49.camel@freescale.com> <1441162821.4966.140.camel@freescale.com> Message-ID: <1441163656.4966.148.camel@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, 2015-09-01 at 21:10 -0600, Simon Glass wrote: > Hi Scott, > > On 1 September 2015 at 21:00, Scott Wood wrote: > > On Tue, 2015-09-01 at 20:48 -0600, Simon Glass wrote: > > > Hi Scott, > > > > > > On 31 August 2015 at 21:16, Scott Wood wrote: > > > > On Mon, 2015-08-31 at 21:13 -0600, Simon Glass wrote: > > > > > Hi Scott, > > > > > > > > > > On 31 August 2015 at 20:11, Scott Wood > > > > > wrote: > > > > > > Currently, using fdt_fixup_stdout() on a device tree that is > > > > > > missing > > > > > > the relevant alias results in this: > > > > > > > > > > > > WARNING: could not set linux,stdout-path FDT_ERR_NOTFOUND. > > > > > > ERROR: /chosen node create failed > > > > > > - must RESET the board to recover. > > > > > > > > > > > > FDT creation failed! hanging...### ERROR ### Please RESET the > > > > > > board > > > > > > ### > > > > > > > > > > > > There is no reason for this to be a fatal error rather than a > > > > > > warning, > > > > > > and removing this allows for a smooth transition on a platform > > > > > > where > > > > > > the device tree currently lacks the correct aliases but will have > > > > > > them > > > > > > in the future. > > > > > > > > > > Why do we need this patch - what platform? > > > > > > > > LS2085A > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Scott Wood > > > > > > Cc: Kumar Gala > > > > > > Cc: Simon Glass > > > > > > --- > > > > > > Resent with correct address for Simon Glass. > > > > > > > > > > > > common/fdt_support.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/common/fdt_support.c b/common/fdt_support.c > > > > > > index f86365e..6052c77 100644 > > > > > > --- a/common/fdt_support.c > > > > > > +++ b/common/fdt_support.c > > > > > > @@ -308,7 +308,8 @@ int fdt_chosen(void *fdt) > > > > > > } > > > > > > } > > > > > > > > > > > > - return fdt_fixup_stdout(fdt, nodeoffset); > > > > > > + fdt_fixup_stdout(fdt, nodeoffset); > > > > > > > > > > Will some platforms will not boot correctly with this failing? > > > > > Should > > > > > we make your new feature a Kconfig options perhaps? I worry that it > > > > > will become the default behaviour and then it will be hard to remove > > > > > later. > > > > > > > > A warning will still be printed. I'm not sure how "### ERROR ### > > > > Please > > > > RESET the board ###" is more useful than trying to continue and > > > > possibly > > > > failing. > > > > > > Only that if it indicates a fatal error the board code can at least > > > find out about it and deal with it. Perhaps booting will just result > > > in a hang? > > > > If booting results in a hang, then you're no worse off than the current > > > > situation. Either way, the user sees a message that indicates the > > problem. > > > > > I think ignoring errors is fine but here we make it impossible to > > > detect a failure. So I think that a Kconfig is the best idea, so we > > > can remove it later. > > > > It seems excessive... Config options aren't free, from a maintenance > > > > perspective, and I have a hard time imagining a scenario where proceeding > > to > > > > boot causes a real problem. > > > > Also, from a consistency perspective: > > - The OF_STDOUT_PATH version doesn't cause a panic if the destination > > path > > doesn't exist. > > - None of the callers of fdt_status__by_alias() panic if the alias > > is > > not found. > > - do_fixup_by_path() doesn't panic if the node is not found. > > - Neither does do_fixup_by_compatible(). > > - Neither does fdt_fixup_ethernet(). > > Yes this code needs cleaning up. I wasn't citing them as problems. :-P > The particular problem you have is that there are no aliases, right? > How about returning 0 from fdt_fixup_stdout() in that case? Then you > will not affect the behaviour of other boards which perhaps do have > /aliases but do not have space in their tree for the fdt_setprop(). That would be fine. -Scott