This is the mail archive of the ecos-discuss@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]