This is the mail archive of the ecos-patches@sources.redhat.com 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: OMAP/innovator RedBoot port


Thanks... I really didn't want to nag... and I appreciate your taking the
time to review the patches so carefully.

> >  (FWIW, the "SRAM" version of RedBoot is one
> > that is configured as if it were a ROM version of RedBoot, but
> > runs solely within internal SRAM.
>
> Fine, although be careful of the conditionals that test
> CYG_HAL_STARTUP_ROM/RAM/ROMRAM in the arch HAL at least.
That is part of the reason I created the SRAM version to begin with.  I
wanted something that was, for all intents and purposes, a ROM version of
the monitor, but just happened to to live at a different address than the
default address (and, coincidentally, lived in internal SRAM of the device
itself, so no other initialization was required).  I found that when I tried
creating a CYG_HAL_STARTUP_SRAM, that there were assumptions elsewhere in
the code (e.g. the arch HAL) that did things differently than if
CYG_HAL_STARTUP_ROM were defined.  It seemed to me that the easiest way to
deal with this was to create a DCSPRI_HAL_ROM_MLT (soon to be renamed
CYGPRI_HAL_ROM_MLT) option that was used to select an alternative memory
layout for a ROM image.

FWIW, we did the same sort of thing in our '8260/VADS/TS6 port

> I added a ChangeLog at the top level and the following entry in
> the NEWS file:
> * Port added for Texas Instruments ARM9 OMAP Innovator board.
> Contributed by
> Patrick Doyle of Delphi Communication Systems.
>
Thanks, I always love seeing my name in lights! :-)

> Other comments: DCSPRI_HAL_ROM_MLT. Although many moons ago we
> originally thought the prefix would indicate the company, in fact
> this is too namespace polluting. Would you mind just using the
> CYG prefix throughout? Cygnus doesn't really exist any more
> anyway :-). Remember to change it in the .ecm files too.
>
OK, do you want me to change it and submit a new or revised patch, or do you
want to wave one of your magic wand scripts over the patch?

> If the ETH device driver is appropriate for the innovator, it
> (and the LAN91CXXX driver) should be in the target definition in
> ecos.db rather than just the .ecm files.
>
We should talk about this.  It may not always be appropriate for the
innovator.  As some background, the innovator comes from TI/PSI as a set of
three cards packed into a plastic case that looks a heck of a lot like a
reference design/mockup of a PDA.  One can also buy a breakout board, open
up the plasic case, and mount the three cards on the breakout board.  The
ethernet MAC is on the breakout board.  Since it is possible that somebody
would want to use eCos on an innovator without the breakout board, it seems
to me that the ETH device driver wouldn't go into the ecos.db.  I guess, one
could always manually remove the package later for that option.  I'll go
with whatever you think is the "most correct" approach.

> hal_diag.c contains a nasty "Confidential material and subject to
> Non-Disclosure." licence thing. I don't really feel I can check
> this in until you confirm your employer is happy with this
> (despite the assignment). I will then remove that copyright and
> licence comment before I check it in. Make sure they understand
> that's what will happen :-).
>
Oops, my fault.  That's the default header that my emacs macro stuffs into
new C files that I create.  Being incredibly lazy, I simply inserted the C
file I was using for my initial serial port debug into hal_diag.c.  Please
feel free to remove that stupid comment block, or ask me to resubmit
hal_diag.c with that comment block removed, whichever you prefer.

> In innovator_misc.c it should be okay to include <string.h>
> rather than define memset explicitly. Also, I'm somewhat wary of
> bit fields in structs as sometimes the alignment of such things
> has been known to change between compiler releases; but I don't
> want to press the issue as it isn't a big deal.
>
Most of innovcator_misc.c was stolen from excalibur_misc.c.  I haven't had
the time to scrub that file yet.  When I do, I expect that I would remove
the explicit prototype of 'memset()' (since that's the sort of thing I hate
to see in code).  I also want to think really hard about simplifying the MMU
stuff.  I hate the fact that I cut and pasted code from one platform to
another, for some very obvious reasons: did I happen to choose the correct
platform?  What happens when somebody finds a bug in the MMU stuff of one
platform -- how to the changes get propogated to the other(s)?  At the time,
I was just looking for something that would swap the FLASH and SDRAM memory
spaces as painlessly as possible.  Further down the road I hoped to compare
all (most?) of the different ARM platforms and pull out the things that were
obvious cut and paste operations into, at least, a common header file.

> I know you based the eth driver on the flexanet, but you might
> want to consider looking at the method used to configure the
> ESA/MAC address in, say,
> devs/eth/cl/cs8900a/current/src/if_cs8900a.c in cs8900a_init(),
> and an accompanying example like
> devs/eth/arm/cerf/current/include/devs_eth_arm_cerf.inl. i.e.
> allow redboot dynamically to provide it, or use compile time
> config, or an EEPROM, in that order. This then gets rid of the
> CYGSEM_DEVS_ETH_ARM_INNOVATOR_REDBOOT_ESA option. Obviously you'd
> need to tweak the lan91cxx driver (and possibly fix up the
> flexanet driver a tad) for this.
>
OK, I'll keep that in mind when I scrub the ethernet files.  Thanks for the
pointers.

> Apart from that this is fine, apart from the bits that are
> obviously work in progress :-), but you've marked most of those anyway.
>
Mainly, I want to offer to other folks who are using the innovator the
opportunity to move away from the old "rrload" bootloader to one that has
more features and more support.  I want to get this "out there" sooner and
ugly, rather than later and scrubbed.

>  >It's what I use to install RedBoot the first time on
> > a new Innovator -- I'll be writing up more detailed
> documentation on getting
> > RedBoot/eCos on the Innovator shortly).
>
> Thanks. The redboot Docbook documentation is the appropriate
> place for the redboot stuff. Obviously mention the new redboot
> commands (but only in the port-specific part, not the main
> section). The eCos stuff can go in a README in the platform HAL
> for the moment as we're still resolving what to do about
> individual port documentation.

Ahh yes, I almost forgot that part of the port -- the documentation.  Thanks
for the gentle reminder.  And thanks for the comments.  I'll await your
reply to know whether you want a new patch against the old patch, against
the current repository, etc...  (And this time, I won't nag you -- I promise
:-)

--wpd


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