[ECOS] Re: [PATCH] redboot - add exec option to set board revision

[Gary Thomas]

On 05/13/2010 11:06 AM, Jose Vasconcellos wrote:
> On Thu, May 13, 2010 at 10:56 AM, Gary Thomas<gary@mlbassoc.com>  wrote:
>> Please keep your replies on the list so that all may benefit.
>>
>> On 05/13/2010 09:33 AM, Jose Vasconcellos wrote:
>>>
>>> This allows setting the ATAG_REVISION so it gets passed to
>>> the kernel. This is useful for systems that share the same id
>>> but have some small hardware differences between board
>>> revisions.
>>>
>>> On Thu, May 13, 2010 at 9:47 AM, Gary Thomas<gary@mlbassoc.com>    wrote:
>>>>
>>>> On 05/13/2010 08:02 AM, Jose Vasconcellos wrote:
>>>>>
>>>>> This patch adds an exec option (-v) to set the board revision
>>>>> via the ATAG_REVISION tag.
>>>>
>>>> What's it used for?  (Why do you need this tag)
>>
>> I think it would be better in this case to have this provided
>> automatically.  Whatever value you would be passing seems to
>> be specific to your target, so just define it as a platform
>> defined value and make the RedBoot exec code pass it.
>>
>> Define it wherever you've defined HAL_PLATFORM_MACHINE_TYPE, e.g.
>>
>> #define HAL_PLATFORM_REVISION 123
>>
>> Then in the code
>> #if defined(HAL_PLATFORM_REVISION)
>>    ...
>> #endif
>>
>> Otherwise, you pollute the command with options which do nothing
>> on most platforms and will just be confusing to users.
>>
>> --
>> ------------------------------------------------------------
>> Gary Thomas                 |  Consulting for the
>> MLB Associates              |    Embedded world
>> ------------------------------------------------------------
>>
>
> It would be better to autodetect the hardware revision and pass that
> when loading the kernel. It would have to be something that's detected
> in something like plf_hardware_init. I'm not sure how to pass that info
> to do_exec.

Similarly to what I said before, just make 'HAL_PLATFORM_REVISION'
be defined as a function that returns the appropriate value, or even
the name of the variable that holds it - your choice.

> Note that my proposed change also fixed another bug. The opts array
> is declared with a size of 7 but it should be 8 if the option
> CYGHWR_REDBOOT_LINUX_EXEC_X_SWITCH is used; otherwise
> it corrupts the stack.

Noted, thanks

> --- a/packages/hal/arm/arch/current/src/redboot_linux_exec.c
> +++ b/packages/hal/arm/arch/current/src/redboot_linux_exec.c
> @@ -313,7 +313,7 @@
>       bool ramdisk_addr_set, ramdisk_size_set;
>       unsigned long base_addr, length;
>       unsigned long ramdisk_addr, ramdisk_size;
> -    struct option_info opts[7];
> +    struct option_info opts[8];
>       char line[8];
>       char *cmd_line;
>       struct tag *params = (struct tag *)CYGHWR_REDBOOT_ARM_LINUX_TAGS_ADDRESS;
>

-- 
Please keep your replies on the mailing list(s) so that all
may benefit.  Private support is available under contract
from various agents, including MLB Associates.  Private
email to me without a contract will be ignored.

------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------



-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss