This is the mail archive of the ecos-discuss@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: Reworking eCos flash interfaces...


Ian Campbell wrote:
Many moons ago (in February, I hadn't realized it was quite so long!) I
discussed improving the eCos flash interface, initially my requirement
was to support paged flash, but it turned into a fairly general cleanup
with a secondary aim of supporting multiple flash devices
  http://sources.redhat.com/ml/ecos-discuss/2003-02/msg00262.html

This is an impressive patch! I'm very glad you were able to keep the backward compatibility as this is something that's been a problem with proposals before.


I know this isn't a formal submission yet, but I suggest you have a think about copyright assignments (unfortunately) which will be required to integrate this stuff. However we are about to switch assignments from Red Hat to FSF (more on this real soon!) so no need to send anything off just yet (or if you do want to, then go ahead with Red Hat).

In addition the public API has been updated to be prefixed with cyg_ and
to use offsets into flash rather than absolute memory addresses. In
addition all the functions take as there first parameter a pointer to a
flash_info structure. The old API is supported (via a CDL option) using
#defines.

You may have to look at flashiodev.c too - this is used by JFFS2.


We have updated RedBoot to the new API, so legacy support has not been
extensively tested. I did try briefly with an unpatched redboot and it
seemed to work.

Such confidence ;-).


Three patches are attached.

flash2_io_flash_2003_07_15.patch is the main API update
flash2_redboot_2003_07_15.patch and
flash2_devs_flash_strata_2003_07_15.patch update RedBoot to the new
public API and the strata flash to the new driver API respectivly.

In theory, once the io/flash part is applied then one, both or neither
of the other patches can be applied and the various legacy support bits
should keep everything working. In practice, well...

I think it's safe to say that some caution is advised. I wonder about the best way to do this. A lot of people use CVS and we don't want to break it badly (particularly where flash is concerned if someone programs a dead image on with RedBoot and doesn't have hardware to fix it).


I think you're hint is right that a two stage approach would indeed be best so there isn't too much to fix at once: apply just io/flash changes first, stabilise, and then the others later

What we can do is warn people it's being applied, apply it and let people know to watch out for breakage, and rest a finger on the "revert" trigger at worst :).

The patches are against CVS (HEAD I think) from 2003-05-29. I appreciate
that this is quite some time ago, I need to find the time to pull the
latest stuff into our local CVS, then I'll try and post updated patches.

That would be better, although I don't think you should have much trouble merging.


I've attached interfaces.txt which is my notes on what the interfaces
are. They weren't really intended as documentation.

Sure.


I've also attached the flash support for one of our (unreleased)
platforms which uses the new flash API as an example... It's not very
exciting. It has just now occured to me that any HAL which provides
strataflash support needs to provide a devtab entry, which breaks
compatability. Possibly the strataflash driver could provide the entry
itself if CYGBLD_LEGACY_FLASH_DEVICE (or another option) says it should.

I think that would be best and don't see any reason why it would be a problem.


As for the patch, I could nitpick some bits (as usual for me) but I won't yet.

But some things worth mentioning:

- There are some ordinal types that are worth typedef'ing, rather than using "int" or "unsigned long" etc. which could bite us later. For a radical overhaul like this these should just be changed. Existing drivers are unlikely to mind if the types have gotten larger. Examples are in cyg_io_flash_getconfig * structures, but more importantly struct flash_info.

- The stuff to do with the sizes of functions copied to RAM in struct flash_info can be ifdef CYGHWR_IO_FLASH_DEVICE_NOT_IN_RAM

- externC should be used in many places (there are a few "extern"s)

- I'm curious why you use this syntax? Why not just { _name, _query etc.?

+cyg_flashdevtab_entry_t _l CYG_HAL_TABLE_ENTRY(flashdev) = {         \
+   .name = _name,                                                    \
+   .query = _query,                                                  \
+   .driver_private = _priv,                                          \
+   .info = &_l ## _info                                              \
+};

- I'm not asking you to fix it as its a big job, but we will still need to address the problem in the future that the flash subsystem assumes fixed block sizes. This is very wasteful - especially when using RedBoot and it gobbles up the size of the largest flash sector for config/FIS data.

- A lot more of the internals appear to be exposed outside _FLASH_PRIVATE_ in flash.h now, but I don't really see why.

- Because it's pervasive it's worth pointing out that CYGHWR_IO_FLASH_DEVICE_V2 probably isn't an ideal name (and you can clarify the display/description too :-)). I know it's copied from CYGHWR_IO_FLASH_DEVICE but that's not what it's being used for here really. HWR implies a hardware property, whereas this is a software API :). Since it's a CDL interface just CYGINT_... would do. Global search and replace is your friend ;).

- You have both CYGBLD_LEGACY_FLASH_DEVICE and CYGSEM_IO_FLASH_LEGACY_COMPAT but you only need one surely. Whichever name you choose, the CDL option should obviously get no_define removed, and it can also be calculated 1 rather than default value... if you don't have a V2 driver it only makes sense to have the legacy interface!

- cyg_flash_program has
+#ifdef HACK_OUT_CYGSEM_IO_FLASH_VERIFY_PROGRAM  /* NOT SUPORTTED YET  */
what's the issue?

- In cyg_flash_lock there is:
+#ifdef CYGHWR_IO_FLASH_DEVICE_V2
+ stat = (*_flash_lock_block)(flash_info, block);
+#else
+ stat = (*_flash_lock_block)((volatile CYG_ADDRESS ) ((cyg_uint8 *)block + block));
+#endif


the expression in the #else looks wrong.

- in the strata driver's flash_erase_block.c I notice you use unsigned long's instead of cyg_flash_offset_t for eb and ca. The same applies for most other files in the strata driver. It would be a shame to spoil the good abstraction work elsewhere :-).

- strata_flash_unlock_block has a TODO and #warning

- Since these functions are global not "static", a cyg_ prefix would be nice.

- strata.c disables the extra checking for oversize addresses. To help clarify what it's for, a comment in the ChangeLog says:

        * src/strata.c (flash_hwr_init): If we find a flash device that is
        larger than is possible given the memory map, shrink to fit.
        We know about the memory map from CYGNUM_FLASH_BASE_MASK (if
        defined); it tells us the stride from one device to another.

        This is to cope with installing, say, a 28F640 in a slot designed
        for a 28F320 because of supply issues - it all works fine so long
        as A22 is grounded.

In other words, while the flash part itself is, say, 64MB you may only be able to access 32MB of it due to limited address space, or some other address space configuration. Don't ask me where it's from, but if it's there it's probably relied on somewhere.

- I think in general the new strata driver is much nicer. Far, far cleaner.

- CYGNUM_FLASH_IS_PAGED? Should be CYGNUM_FLASH_BANKS surely.

- In redboot, if CYGSEM_REDBOOT_FLASH_CONFIG_READONLY_FALLBACK is to be disabled, the CDL should be commented out or something instead.

- _show_invalid_flash_offset() should tak ea cyg_flash_offset_t? There's a few other new unsigned long's in the redboot flash.c patch similarly.

- flash_info() and do_flash_init() don't seem ideal. What if there is >1 driver or part? RedBoot won't be able to see it. There should probably be some way to enumerate all installed flash parts/drivers provided by the API in io/flash (which would iterate through FLASHDEVTAB obviously).

That's about as much as I can see without actually playing with it (and out of interest I don't have any strataflash hardware here so I'll only be able to test the legacy interface myself, although another eCosCentric guy, Bart, does have an iPAQ but obviously there's no way to de-brick that if there was a flash problem).

Great work, thanks!

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine


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


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