RE: Intel FLASH

["Doyle, Patrick" <WPD at dtccom dot com>]

> > I wonder if my changes are significant enough, or the risk 
> of breaking
> > existing platforms is high enough, that I should change this to an
> > entirely new device driver, perhaps intel/cfiflash, or 
> something like
> > that; perhaps even move it up one layer in the tree, since it is
> > nolonger specific to Intel -- in theory any flash that implents CFI
> > should work with this.
> 
> I think it would be better to not make yet another driver. 
I keep going back and forth on this issue.  On the one hand, I really don't
want to add yet another driver to the tree that provides the same support
for the same devices as the existing drivers do (although, if we put
comments/#warnings in the existing drivers, perhaps folks would migrate to
the new driver).  On the other hand, this driver is no longer Intel and/or
strata specific (although it supports those parts), so how would a newcomer
know that this driver might be more appropriate than, for example the 28Fxxx
driver?  It's a judgement call.

> Here's my changes against your patch to make it compile
> with my two configs I tested with. In the UNLOCK_SINGLE_BLOCKS turned
> off case there were some vars missing. 
Oops, sorry 'bout that Chief.  I actually did compile the nano redboot
(which uses the B3 device), but I didn't realize that it didn't support
block locking/unlocking.  I just tried assabet -- it failed until I applied
your patches.

> I also moved the new block_op in strata.c and made it not 
> inline. It is
> relatively big so calling it saves some space but more importanatly
> since it is not guarranteed that it is inlined even if you tell gcc to
> try it can end up not in .2ram section so relocations truncated errors
> are given at linkage.That's what happened  here anyway. I put it in
> .2ram as well.
That makes sense.  I was originally maintaining three sets of code that did
essentially the same thing, so I created 'flash_block_op()' because I got
tired of that.  Your comments about inline functions and GCC makes sense.
Thanks for the rework.

>I changed the bootblock detection to work with 
> SERIES and
> with the B3 device.

Now for some good, old fashioned, controversy.  :-)
I deliberately did not fold in your changes for CYGNUM_FLASH_BASE_MASK.  My
understanding of that parameter is that it exists to cope with oversized
devices fitted with some high address lines ignored, into an existing
platform.  Given that is the case, it doesn't make sense to me populate
additional devices in series.  On my platform, I (essentially) have two
devices attached to two separate Chip-select pins, and I use the MMU to make
them show up sequentially in the virtual address space.  The
'CYGNUM_FLASH_BASE_MASK' parameter specifies the size of each device.

Unfortunately, while the comment in "strata.c" implies that
'CYGNUM_FLASH_BASE_MASK' is optional, code elsewhere in the driver assumes
it is defined.

I'm not actually happy with this solution as it stands.  Since, downstream,
we may be forced to buy larger parts (where I use the term "forced" to mean
"we can get them cheaper because there is more demand for them).  I would
like for RedBoot, and my application(s) to adapt dynamically to the size of
the FLASH without requiring a recompile.  In my case, I could adjust the MMU
mappings at bootup based upon information returned from the FLASH device(s)
via CFI.  But I'm not ready to bite off that project right now.

> So far with these changes it seems to work on the B3 and W3 
> parts I have
> here.I'll keep testing though.
Great, and thanks.

I have (perhaps) attached a new version of the complete patch, folding in
your changes (except for the CYGNUM_FLASH_BASE_MASK change as discussed
above).  Enjoy!

> 
> Jani
>

Attachment: st3.diff
Description: Binary data