various submissions

[Jonathan Larmour]

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