various submissions

[Jonathan Larmour]

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

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

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