On Fri, Aug 15, 2014 at 12:33 PM, Simon Glass <s...@chromium.org> wrote:
> Hi,
>
> On 15 August 2014 12:25, Bryan Wu <coolo...@gmail.com> wrote:
>> On Fri, Aug 15, 2014 at 11:04 AM, Stephen Warren <swar...@wwwdotorg.org>
>> wrote:
>>> On 08/15/2014 11:49 AM, Bryan Wu wrote:
>>>>
>>>> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
>>>
>>>
>>> Can we spell out the commit description too, so it's easier to know what it
>>> refers too, and is useful if someone cherry-picks the commit so the commit
>>> ID changes:
>>>
>>> Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
>>>
>>>
>>>> booting FIT image. It's because calling fit_parse_config() twice will
>>>> give us wrong value in img_addr.
>>>>
>>>> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
>>>> return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
>>>
>>>
>>>> diff --git a/common/image.c b/common/image.c
>>>
>>>
>>>> -ulong genimg_get_kernel_addr(char * const img_addr)
>>>> -{
>>>> #if defined(CONFIG_FIT)
>>>> - const char *fit_uname_config = NULL;
>>>> - const char *fit_uname_kernel = NULL;
>>>> +ulong genimg_get_kernel_addr(char * const img_addr,
>>>> + const char **fit_uname_config,
>>>> + const char **fit_uname_kernel)
>>>> +#else
>
> Let's avoid these #ifdefs inside a function signature, just too ugly.
> If the extra arguments are a big problem you could create a new
> function perhaps?
>
OK, I will try to define a new function.
> Also, how about updating test/image/fit.py to test this behaviour?
> Then we can catch the bug.
>
York, I might need your information to add some test in the test/image/fit.py.
So a) you download a FIT image to somewhere
b) what's kind of FIT image you're testing? how many configs?
c) and you don't have load_addr?
-Bryan
>>>> +ulong genimg_get_kernel_addr(char * const img_addr)
>>>> #endif
>>>
>>>
>>> Indentation looks wrong on that #endif.
>>>
>>> Wouldn't it be better to avoid repeating the common parts, so this instead:
>>>
>>> ulong genimg_get_kernel_addr(char * const img_addr,
>>> #if defined(CONFIG_FIT)
>>> const char **fit_uname_config,
>>> const char **fit_uname_kernel)
>>> #endif
>>> )
>>> {
>>>
>>
>> I got compiling error like this when I try this method then I moved to
>> 2 current one:
>>
>> ---
>> ulong genimg_get_kernel_addr(char * const img_addr,
>> #if defined(CONFIG_FIT)
>> const char **fit_uname_config,
>> const char **fit_uname_kernel
>> #endif
>> );
>> ---
>> include/image.h:432:1: error: expected declaration specifiers or ‘...’
>> before ‘)’ token
>> );
>> ^
>>
>> But if I use this:
>>
>> ----
>> ulong genimg_get_kernel_addr(char * const img_addr
>> #if defined(CONFIG_FIT)
>> , const char **fit_uname_config,
>> const char **fit_uname_kernel
>> #endif
>> );
>> ----
>> Then compiler is happy, but the style looks weird to me.
>>
>> This is for declaration but definition has the same problem.
>>
>> -Bryan
>>
>>
>>>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
>>>
>>>
>>>> +#define CONFIG_FIT
>>>
>>>
>>> I think that crept in by mistake.
>>>
>>>
>>>> diff --git a/include/image.h b/include/image.h
>>>> index ca2fe86..a47c146 100644
>>>
>>>
>>>> +#if defined(CONFIG_FIT)
>>>> +ulong genimg_get_kernel_addr(char * const img_addr,
>>>> + const char **fit_uname_config,
>>>> + const char **fit_uname_kernel);
>>>> +#else
>>>> ulong genimg_get_kernel_addr(char * const img_addr);
>>>> +#endif
>>>
>>>
>>> Same comment about #ifdef placement here.
>
> Regards,
> Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
