Re: [ECOS] Flash infrastructure rework

[Bart Veer <bartv at ecoscentric dot com>]

Some thoughts on the new flash interface. I know it has been a while
since you asked for comments...

Interrupts
----------

I believe it is very important we sort out the interrupt issue.
Disabling interrupts at a high level while erasing or writing multiple
blocks will affect real-time responsiveness, and should be
unnecessary. Instead that sort of thing should be handled by the
device drivers at a finer grain, to the extent permitted by the
hardware.

For example, the current AMD flash_program_buf() routine does not
perform any locking, relying on higher levels. Each write operation
requires a number of atomic flash writes and reads, with no other
flash accesses allowed (some devices do allow concurrent reads from
other areas of the flash, but that gets messy in a generic AMD
driver). The only safe way to do this is to disable interrupts, but we
need only disable interrupts for each byte or word, not for the whole
block.

flash_erase_block() is more complicated because it requires polling
while the chip does its stuff. Most devices support erase-suspend
(AFAIK) so we could have a sequence: disable interrupts, start the
erase, poll for completion for a short while, suspend it, reenable
interrupts, disable interrupts again, and resume the erase. There is
no need to have interrupts disabled for the entire operation.

If we are going to move interrupt handling down into the drivers then
we'll also have to move any cache manipulation there. The alternative
would be e.g. a potential context switch while the caches are
disabled, and then the system is messed up. Manipulating interrupts
and cache at a finer grain than at present will have a significant
effect on the time taken by the various operations. I haven't done any
experiments yet to know how much longer, and obviously it is going to
depend on the hardware (especially the cache-related bits). Possibly
it can be handled by configury, but that needs investigation as well.

Should we be addressing this issue at this time? Any changes along
these lines involve a serious risk of breaking things for various
targets, but that will always be true. Since we have a flash
development branch I think this is probably going to be as good a time
as any.


Packages
--------

While considering major changes, there is another topic I want to
raise. The current convention is to have per-platform flash device
driver packages, for example devs/flash/arm/ contains 31 separate
packages. Packages are very useful if they can be shared and re-used.
They are also very useful to isolate hardware-specific non-shareable
code in one place, e.g. the platform HAL. Platform-specific flash
driver packages do not fit into either category.

For the various customer ports I have been doing I have been putting
the flash support into the platform HALs. After all the flash device
is an integral part of the platform, just like the RAM, and we don't
have separate device drivers for each platform's RAM subsystem. In the
platform CDL I have something like:

   cdl_option CYGHWR_HAL_M68K_M5272C3_FLASH {
        display		"FLASH memory support"
	parent		CYGPKG_IO_FLASH
	active_if	CYGPKG_IO_FLASH
	implements	CYGINT_DEVS_FLASH_AMD_AM29XXXXX_REQUIRED
	requires	CYGHWR_DEVS_FLASH_AMD_AM29PL160
	flavor		none
	compile		m5272c3_flash.c
	description "
            The M5272C3 evaluation board comes with a single AM29PL160C
            flash memory device."
    }

In the platform HAL's src directory I then have a file m5272c3_flash.c
which instantiates the flash device, but only if the generic flash
support support is enabled because of the active_if. Hence all the
platform-specific information remains in one place, the platform HAL,
and there is no proliferation of non-reusable packages elsewhere in
the hierarchy. There is no change to the user experience, everything
works much as before.

Given the number of existing platforms in the repository changing them
all over is probably not an option, but for new platforms I would prefer
not to see unnecessary packages.


The API
-------

Mostly this looks fine, although I think you are trying too hard to
make it similar to the old API. Since we are making major changes we
may as well make the new API as clean as possible.

cyg_flash_init(): could we get rid of this and replace it with a
prioritized static constructor? That way we would know for sure that
the flash drivers have been initialized before the application starts
up. AFAIK the existing flash drivers init routines are all pretty
straightforward so this is not going to add significantly to startup
time. It would eliminate the need in various places to check that the
flash subsystem has been initialized, and the corresponding error
conditions.

Currently cyg_flash_init() provides the cyg_flash_printf function so
we would need a different approach for that. Possibly an exported weak
variable with a default value of &diag_printf, which application code
can then override.

cyg_flash_get_block_info() and cyg_flash_get_limits(): I think we
should just get rid of these completely.  Full information on the
flash is available by other calls, and higher-level code such as
RedBoot should be made to use these.

cyg_flash_code_overlaps(): I am not sure this can be cleanly defined.
For example, consider a platform with one flash chip that uses bottom
boot blocks. If we want to make efficient use of this chip then we
want to place the fis and fconfig data in these boot blocks. Hence the
first boot block will contain some startup code followed by a jump to
later in the flash, then we'll have the fis and fconfig blocks, and
then the rest of the code. How would cyg_flash_code_overlaps() work
with that scenario?

cyg_flash_lock() and cyg_flash_unlock(): possibly this functionality
should be provided by a more generic cyg_flash_ioctl() function. That
would also give us a way of supporting device-specific flash
operations in future. Usually I don't like general-purpose ioctl()
routines because they cannot easily be garbage collected by the
linker, but in this case it may be worthwhile.

cyg_flash_errmsg(): should we consider eliminating flash-specific
error codes and switching to errno values? Resulting diagnostics
output will be generic rather than flash-specific, so less useful,
but arguably it would simplify the system. I am not sure we want to
make this change, but we should consider it.

cyg_flash_mutex_lock() and cyg_flash_mutex_unlock(): there may be a
problem with these in some scenarios. If there is a virtual vector
function which calls into RedBoot to e.g. change an fconfig option,
the flash code inside RedBoot won't see the kernel mutex. This is part
of a larger problem, properly synchronizing between virtual vector
functions and a multi-threaded application, but it needs to be
considered.


The Implementation
------------------

I have various comments on the CDL, headers, sources, and
documentation, but this message is long enough already. Some of these
will require discussion, others can be handled by patches. I'll check
out the flash_v2 branch and start experimenting on some of the boards
I have available. Most of these use AMD-compatible chips, and I can
rescue them via BDM when necessary.

Bart


-- 
Bart Veer                       eCos Configuration Architect
http://www.ecoscentric.com/     The eCos and RedBoot experts

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