RE: various submissions

["Koeller, T." <Thomas dot Koeller at baslerweb dot com>]

Hi Gary and Jonathan,

please see my comments below.
tk

> -----Original Message-----
> From: Gary Thomas [mailto:gthomas@ecoscentric.com]
> Sent: Tuesday, October 22, 2002 6:34 AM
> To: Jonathan Larmour
> Cc: thomas.koeller@baslerweb.com; eCos patches
> Subject: Re: various submissions
> 
> 
> On Mon, 2002-10-21 at 20:06, Jonathan Larmour wrote:
> > Thomas Koeller wrote:
> > > Hi,
> > > 
> > > here's some stuff I made, submitted for itegration into ecos cvs.
> > > There's the modular AT91 HAL, consisting of hal_at91.epk and
> > [snip]
> > 
> > Thanks for the contrib. Sorry it's taken so long to get round to 
> > reviewing, but the larger a patch is, the less easy it is 
> to set aside time...
> > 
> > Anyway, okay, I've been reviewing this, and there are a 
> fair few issues 
> > that we need to work out before I can put it in. The best 
> approach to take 
> > is to comment on bits of this, rather than all at once, or 
> having many 
> > overlapping threads. I'll start with io_flash.diff as that 
> has a knock-on 
> > effect.
> > 
> > - Why is flash_block_query a separate extern? Can't it just 
> be a field in 
> > the struct flash_info? BTW, not something I would intend 
> you to deal with 
> > definitely, but the situation in the flash drivers is 
> already a bit naff 
> > in terms of namespace pollution (the flash drivers were 
> unfortunately only 
> > written in the context of redboot), and we don't want to 
> make the issue worse.

If you prefer that, its fine for me.

> > 
> > - Call it personal taste, but I'm not fond of the interface. In 
> > particular, the old function flash_get_block_info() is 
> still left behind. 
> > I don't think a function that will effectively be "broken" 
> for certain 
> > flash parts should be left. That function is only used in a 
> very small 
> > number of places (and it's not an official API function, so 
> backward 
> > compatibility should not be a problem). Only redboot and 
> > devs/flash/synth/current/tests/flash1.c use it. I think 
> > flash_get_block_info should have its arguments tweaked to 
> be the new 
> > interface.
> > 

It seem that backwards compatibility is much less of an issue than
I anticipated. I do not need that function either.

> > 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.
The iteration only takes place if block size is non-constant,
and your proposal would require traversal of the block table
then.
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.

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

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

> 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. Since we wanted to use RedBoot and FIS,
we were no longer able to download our program under development to
the target due to lack of free RAM caused by the huge FIS directory buffer
set aside permanently. I can easily think of other situations where
the flash block buffer size matters - embedded systems tend to be
resource constrained, and putting small data structures into huge
flash blocks is wasteful for both flash and RAM.

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.

> > 
> > Oh and BTW, the copyright assignment you have signed 
> permits you to make 
> > further contributions if you wish without a new assignment. 
> Have a read of 
> > http://sources.redhat.com/ecos/assign.html
> > 
> > I'd appreciate other feedback on what I suggest above. (Hi Gary!).
> 
> I'll give this a closer look as well.
> 
> -- 
> ------------------------------------------------------------
> Gary Thomas                  |
> eCosCentric, Ltd.            |  
> +1 (970) 229-1963            |  eCos & RedBoot experts
> gthomas@ecoscentric.com      |
> http://www.ecoscentric.com/  |
> ------------------------------------------------------------
>