On 12/13/16 22:29, Simon Glass wrote: > Hi Igor, > > On 12 December 2016 at 01:37, Igor Grinberg <grinb...@compulab.co.il> wrote: >> On 12/11/16 22:27, Simon Glass wrote: >>> Hi Igor, >>> >>> On 11 December 2016 at 10:37, Igor Grinberg <grinb...@compulab.co.il> wrote: >>>> Hi Tomas, Simon, >>>> >>>> Sorry, to break in that late... >>>> I have a quick question below. >>>> >>>> On 12/05/16 09:36, Tomas Melin wrote: >>>>> Enable support for loading a splash image from within a FIT image. >>>>> The image is assumed to be generated with mkimage -E flag to hold >>>>> the data external to the FIT. >>>>> >>>>> Signed-off-by: Tomas Melin <tomas.me...@vaisala.com> >>>> >>>> [...] >>>> >>>>> diff --git a/common/splash_source.c b/common/splash_source.c >>>>> index 70d724f..94b46d3 100644 >>>>> --- a/common/splash_source.c >>>>> +++ b/common/splash_source.c >>>> >>>> [...] >>>> >>>>> +#ifdef CONFIG_FIT >>>>> +static int splash_load_fit(struct splash_location *location, u32 >>>>> bmp_load_addr) >>>>> +{ >>>>> + int res; >>>>> + int node_offset; >>>>> + int splash_offset; >>>>> + int splash_size; >>>>> + struct image_header *img_header; >>>>> + const u32 *fit_header; >>>>> + u32 fit_size; >>>>> + const size_t header_size = sizeof(struct image_header); >>>>> + >>>>> + /* Read in image header */ >>>>> + res = splash_storage_read_raw(location, bmp_load_addr, header_size); >>>>> + if (res < 0) >>>>> + return res; >>>>> + >>>>> + img_header = (struct image_header *)bmp_load_addr; >>>>> + fit_size = fdt_totalsize(img_header); >>>>> + >>>>> + /* Read in entire FIT */ >>>>> + fit_header = (const u32 *)(bmp_load_addr + header_size); >>>>> + res = splash_storage_read_raw(location, (u32)fit_header, fit_size); >>>>> + if (res < 0) >>>>> + return res; >>>>> + >>>>> + res = fit_check_format(fit_header); >>>>> + if (!res) { >>>>> + debug("Could not find valid FIT image\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + node_offset = fit_image_get_node(fit_header, location->name); >>>>> + if (node_offset < 0) { >>>>> + debug("Could not find splash image '%s' in FIT\n", >>>>> + location->name); >>>>> + return -ENOENT; >>>>> + } >>>>> + >>>> >>>> I think two above debug() are very legitimate - no need to shout if no FIT >>>> image >>>> or no splash in it... >>>> >>>>> + res = fit_image_get_data_offset(fit_header, node_offset, >>>>> + &splash_offset); >>>>> + if (res < 0) { >>>>> + debug("Could not find 'data-offset' property in FIT\n"); >>>>> + return res; >>>>> + } >>>>> + >>>>> + res = fit_image_get_data_size(fit_header, node_offset, >>>>> &splash_size); >>>>> + if (res < 0) { >>>>> + debug("Could not find 'data-size' property in FIT\n"); >>>>> + return res; >>>>> + } >>>> >>>> Now regarding these two, I'm not sure. >>>> Since we have found a valid FIT and also a node with a correct splash name, >>>> probably the intent is that we show the splash, right? >>>> But in the two above checks, we find inconsistencies that do not allow us >>>> to >>>> show the splash - meaning the FIT is not actually good (am I right here?). >>>> So may be we should report it to the 'user' and allow correcting the FIT? >>>> Otherwise, it is impossible to debug the image w/o a debug version of >>>> U-Boot... >>>> Do I make sense, or do I miss something? >>> >>> Yes that makes some sense, but the problem is that then you are >>> including error messages always which would never happen in a working >>> system (i.e. it just bloats the code). >> >> Unless, there a kind of corruption or a user mistake and then that same >> user can't even understand what happened because we do not help him with >> an error message. >> You cannot know that these error messages will never happen... >> This is a generic code which can be used by a wide variety of platforms - >> I don't think you can foresee all the possible use cases. >> >> We are talking about several tens of bytes vs. usability. >> If there is an error, it should be stated as such. It should not just >> exit silently... > > I agree with that, there should definitely be an error printed. It > should say something like 'Failed to load splash screen (err=-4)' or > something like that. The error number should provide some clues and > people can dig in.
Great idea! > > This patch would add around 200 bytes if debug() was changed to > printf(). Multiply that by several hundred if we did that sort of > thing throughout U-Boot. Well, I thought about using printk only on a half of the above messages and that gives about ~90 bytes, while it also can be optimized by using the same parameterized string and thus add only ~50 bytes... > >> >> I think that debug() is a good thing to use, but we shouldn't be obsessed >> with it. > > Fair enough, I don't want to get obsessed about it either! But general > code-size bloat is a real problem. so I think in general we should > make sure an error is printed when one occurs, and only cover some > common cases where the user can actually fix it (e.g SD card missing) > with more detailed error messages. > >> >>> >>> So long as the error is reported (even if it is not a very specific >>> error), people can add DEBUG and track it down. >> >> That depends who 'people' are? Devs? Users? > > Well in this case the user will never see the problem, unless someone > has screwed up the splash screen image. Mostly I'm talking about devs. > > Better would be to have an expanded debug() system which lets you turn > debugging on globally when hunting for a problem. That would be a nice > project for someone... Yes, indeed that sounds like a nice project. It would be great to be able to specify the debug verbosity on per build basis (e.g. Kconfig). -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot