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: Flash infrastructure rework


On Thu, Aug 19, 2004 at 04:21:14PM +0100, Bart Veer wrote:
> Some thoughts on the new flash interface. I know it has been a while
> since you asked for comments...

No problems, comments are always welcome
 
> Interrupts
> ----------
> 
> I believe it is very important we sort out the interrupt issue.

You are braver than i am. I decided this was too thorny an issue to
tackle and just documented the problem. It is only a problem when the
application or RedBoot is running from ROM. Currently interrupt
disabling is left to the application programmer do use if needed.

> 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.

Agreed. As a general approach, what i would like to do is get a
generic CFI driver working which has all these bells and whistles and
uses the new driver API. Since many flash devices nowadays are CFI
compatible, i think it makes sense to do this rather than look at the
AMD generic driver, the Intel Generic Driver, the SST generic driver
etc. 
 
> 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.

Why is it messed up? The current macros don't just disable it, they
flush and invalidate it first. So a context switch is safe. The higher
priority threads which gets to run will run slower since it does not
have any caching, but thats how it currently works anyway. The only
read danger is if something else unconditional re-enables the cache
and then we are dead when the flash driver gets control again.

> 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. 

This is mainly a documentation issue. I would like to spend some time
extending the documentation to include hints for people writing new
drivers. Or maybe it should be part of the Porting guide? But before
spending time on the documentation i would like to see the code stable
and well tested.

> 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.

The danger here is when it tries to print something. I fell fouls of
this myself. I had a configuration with both sst and strata drivers
configured in because our hardware can have one or the other at the
same address. It worked fine on the sst version but locked solid on
the strata version. And it only locked when i had jffs2. jffs2
requires the block device driver for the flash. This gets initialized
by a constructor early on and it called the cyg_flash_init
function. That was calling the sst init function, which failed to find
its device and was printing an error message. But at this point the
virtual vectors were not setup and so diag_printf just locked the
target solid.

For this to work reliably, we need to look at every flash driver and
make sure it cannot print anything during its initialisation routine.
Or we need to be 100% sure the virtual vectors are setup.

We would also have to be careful with the constructor ordering. The
block devices would have to be after the flash device or jffs2 would
get into problems.
 
> 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.

That would work. jffs2 would also want to override this or you get
lots of output.

> 
> 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.

I thought about that and changing redoot to use the other calls is
something that is needed when making redboot use bootblocks. Removing
these calls just makes it harder for people porting there own
applications to the new api.

> 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?

The HAL needs to be able to implement it. That is the only thing that
knows what goes where. I didn't really like this function to start
with, but redboot uses it quite a lot for user input sanity
checking. We could remove it and hope the user knows what (s)he is
doing.
 
> 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.

The generally semantic of ioctl is that you have to open the device
first, ie the unix file model. The flash API is not file based so i
don't really think ioctl fits.  Im also not sure what extensions could
be added except for NAND devices where you need to add access to the
OOB area. Gary is working on this at the moment i think. It would be
interesting to know how he decided to do 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 calling side of the VV functions which can change the flash should
call these functions. This is exactly what they are for. The problem
comes with knowing what area to lock. Maybe a rogue address is needed
to indicate all devices should be locked?
 
> 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.

Great. More testing is definitely needed. I keep finding
configurations which i've not tried and either don't compile or don't
work.

        Andrew

-- 
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]