Re: various submissions

[Gary Thomas <gthomas at ecoscentric dot com>]

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

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