This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: various submissions
- From: Jonathan Larmour <jifl at eCosCentric dot com>
- To: "Koeller, T." <Thomas dot Koeller at baslerweb dot com>
- Cc: "'Gary Thomas'" <gthomas at eCosCentric dot com>, eCos patches <ecos-patches at sources dot redhat dot com>
- Date: Tue, 12 Nov 2002 04:07:50 +0000
- Subject: Re: various submissions
- References: <850597605E79D21182830008C7A4B9CF1EB42473@COMM1>
Koeller, T. wrote:
Hi Gary and Jonathan,
please see my comments below.
[I could have sworn I replied to this, sigh]
-----Original Message-----
In any case, instead of the (computationally expensive)
iterator concept,
it would be much simpler for the driver to fill in a static
table of
regions and their size in the struct flash_info. Most times
the table will
be pretty small - most commonly just 2. So e.g. the driver
would have:
// io_flash.h contains:
struct flash_block_table_entry {
void *offset_addr; // offset of this address to the
start of the device
unsigned long size;
unsigned long block_size;
// potential for expansion in future...
};
// underlying flash driver contains:
static const struct flash_block_table_entry block_table[3] = {
{0x800000, 0x10000, 0x2000},
{0x810000, 0x1f0000, 0x10000},
{0x0, 0x0, 0x0} // end of table
}
// flash_hwr_init() contains:
flash_info.block_table = block_table;
size == 0 indicates the last table entry.
Otherwise we go through a lot of effort to support
something that is
actually 99% of the time quite simple.
In the simple case where the block size is actually constant,
flash_get_block_info_default() is used and computation of the
block index is trivial, so almost no overhead is introduced.
I would still advocate using CDL to control the (more common) case of
there being just a single block size, so in fact I'm advocating zero overhead.
The iteration only takes place if block size is non-constant,
and your proposal would require traversal of the block table
then.
Yes, but in a much simpler and straightforward way.
The implementation as a function (as opposed to a table)
allows the chip driver to take advantage of any simplification
that a particular block layout may allow.
Well I suppose it's possible that a flash chip could have alternating
sizes of blocks, i.e. no two adjacent blocks are the same size, but I
think that's pretty unlikely.
- I see no reason for flash_get_block_for_index() or
flash_iterate_blocks() to be externs. statics should suffice.
- There are plenty of drivers we have that will never need
variable block
sizes, but there's a non-trivial amount of code now
permanently included
to support it. Instead I think this support should be
controlled by a CDL
interface. The underlying flash driver would implement the
interface and
io/flash would react accordingly. Obviously something more
intelligent can
be done than just #ifdeffing every line you changed in your
patch :-).
Something like CYGINT_IO_FLASH_VARIABLE_BLOCK_SIZE.
Of course that could be done. I just thought the amount of code
added wasn't worth adding another configuration option.
Given the fairly lightweight size of the flash drivers, it's one of the
areas where I think we should be watchful. I'm still happier with taking a
simpler approach too.
If you like I'm prepared to implement this in io/flash for
you. If you can
then make the changes to your flash driver and test it
(since I can't - no
at91!).
I am currently _very_ busy, so I'm very much inclined to accept this
offer. If you send patches, I will be happy to test them with my flash
chip driver.
My next mail will have a patch.
I'll have to be convinced of the usefulness of bothering with this at
all. Currently we have a number of drivers that can handle
devices with
variable block sizes, without any impact to the rest of the FLASH
system. This is done by letting the driver treat a set of
smaller/variable sized blocks as equivalent to one of the "normal"
sized blocks. For the simple uses that we make of the FLASH (namely
the FIS in RedBoot and overlaying JFFS2 onto large chunks), I don't
see that anything more elaborate is required.
the primary reason why I invented all this was that I have to support
a target with only a small amount of RAM and a flash chip that is
organized as 8 blocks of size 8KB and 31 blocks of size 64KB. I
originally tried to implement it the way you suggest, treating all
the smaller blocks as one more large block. However, this turned out
to be impractical because in order to manipulate data stored in flash
we would then need a buffer the size of the large flash blocks, and we
could not afford that much RAM.
A good argument since eCos is very much intended to be usable for memory
constrained targets.
Based on my experience flash chips with variable block sizes are by no
means exotic or even unsusual, and the inablity to deal with them
appropriatly is a major shortcoming of ecos.
Agreed. Also people with custom hardware probably chose these flash parts
because they _wanted_ to use the smaller blocks. And you don't want to
have to update many of them when you only need to update one.
In Red Hat in the past, we've discussed ways to reduce RedBoot's flash
footprint, particularly with a whole flash sector used by the each of the
config data and FIS directory, but one of the most obvious ways is to use
small sectored flash when the hardware is available.
Jifl
--
eCosCentric http://www.eCosCentric.com/ <info@eCosCentric.com>
--[ "You can complain because roses have thorns, or you ]--
--[ can rejoice because thorns have roses." -Lincoln ]-- Opinions==mine