Common Flash Interface probe code.

Common Flash Interface probe code.

[David Woodhouse]

Anyone with memory-mapped CFI-compliant NOR flash, especially if it's not 
in 16-bit mode like the one I'm testing on - could you test this probe code 
for me?

/* 
   Common Flash Interface probe code.
   (C) 2000 Red Hat. GPL'd.
   $Id$
*/


#include <linux/module.h>
#include <linux/types.h>
#include <linux/kernel.h>
#include <asm/io.h>
#include <asm/byteorder.h>
#include <linux/errno.h>

#ifdef __arm__
/* Shoot me. Better still, shoot rmk. dwmw2 */
#undef writeb
#undef writew
#undef readb
#undef readw
#define readb(l) (*(unsigned char *)(l))
#define writeb(c,l)  do {*((unsigned char *)(l)) = (c);} while (0)
#define readw(l) (*(unsigned short *)(l))
#define writew(c,l)  do {*((unsigned short *)(l)) = (c);} while (0)
/* It's OK. You can look back now. */
#endif /* __arm__ */

struct cfi_ident {
  __u8  qry[3];
  __u16 P_ID;
  __u16 P;
  __u16 A_ID;
  __u16 A;
  __u8  VccMin;
  __u8  VccMax;
  __u8  VppMin;
  __u8  VppMax;
  __u8  WordWriteTimeoutTyp;
  __u8  BufWriteTimeoutTyp;
  __u8  BlockEraseTimeoutTyp;
  __u8  ChipEraseTimeoutTyp;
  __u8  WordWriteTimeoutMax;
  __u8  BufWriteTimeoutMax;
  __u8  BlockEraseTimeoutMax;
  __u8  ChipEraseTimeoutMax;
  __u8  DevSize;
  __u16 InterfaceDesc;
  __u16 MaxBufWriteSize;
  __u8  NumEraseRegions;
  __u32 EraseRegionInfo[1];
} __attribute__((packed));


void print_cfi_ident(struct cfi_ident *cfip);


int init_module(void) 
{
  unsigned char tmpzero;
  unsigned long base = 0xd0000000;
  int devwidth=0, numdevs=0;

  int i;
  struct cfi_ident cfip;

  tmpzero=readb(base);
  
  writeb(0x98, base+0x55);
  
  if (*(unsigned char*) (base + 0x10) == 'Q' &&
      *(unsigned char*) (base + 0x11) == 'R' &&
      *(unsigned char*) (base + 0x12) == 'Y') {
    printk("Found a CFI device at location %lx in 8 bit mode\n", base);
    devwidth=1;
    numdevs=1;
  }

  if (!devwidth) {
    writew(0x9898, base+0xAA);
    
    if (*(__u16*) (base + 0x20) == cpu_to_le16(0x0051) &&
	*(__u16*) (base + 0x22) == cpu_to_le16(0x0052) &&
	*(__u16*) (base + 0x24) == cpu_to_le16(0x0059)) {
	  printk("Found a CFI device at location %lx in 16 bit mode\n", base);
	  devwidth=2;
	  numdevs=1;
    }
    else if (*(__u16*) (base + 0x20) == 0x5151 &&
	  *(__u16*) (base + 0x22) == 0x5252 &&
	  *(__u16*) (base + 0x24) == 0x5959) {
	    printk("Found a coupled pair of CFI devices at location %lx in 8 bit mode\n", base);
	    devwidth=2;
	    numdevs=2;
    }
  }

  if (!devwidth) {
    writel(0x98989898, base+0x154);
    
    if (*(__u32*) (base + 0x40) == cpu_to_le32(0x00000051) &&
	*(__u32*) (base + 0x44) == cpu_to_le32(0x00000052) &&
	*(__u32*) (base + 0x48) == cpu_to_le32(0x00000059)) {
      /* This isn't actually in the CFI spec - only x8 and x16 are. */
      printk("Found a CFI-like device at location %lx which appears to be in 32 bit mode(!)\n", base);
      devwidth=4;
      numdevs=1;
    } 
    else if (*(__u32*) (base + 0x40) == cpu_to_le32(0x00510051) &&
	*(__u32*) (base + 0x44) == cpu_to_le32(0x00520052) &&
	*(__u32*) (base + 0x48) == cpu_to_le32(0x00590059)) {
      printk("Found a coupled pair of CFI devices at location %lx in 16 bit mode\n", base);
      devwidth=4;
      numdevs=2;
    }
    else if (*(__u32*) (base + 0x40) == 0x51515151 &&
	*(__u32*) (base + 0x44) == 0x52525252 &&
	*(__u32*) (base + 0x48) == 0x59595959) {
      printk("Found four side-by-side CFI devices at location %lx in 8 bit mode\n", base);
      devwidth=4;
      numdevs=4;
    }
  }
  
  /* We won't bother with 64-bit wide detection at the moment */

  if (!devwidth) {
    printk("Found no CFI device at location %lx\n", base);
    return -ENXIO;
  }

  for (i=0; i<sizeof(struct cfi_ident); i++) {
    ((unsigned char *)&cfip)[i] = *(unsigned char *) (base + ((0x10 + i)*devwidth));
  }
#if 0
  for (i=0; i<8; i++) {
      printk ("CFI[%2.2X]: %2.2X %2.2X %2.2X %2.2X %2.2X %2.2X %2.2X %2.2X\n",
	      i*8,
	      ((unsigned char *)&cfip)[(i*8)+0], 
	      ((unsigned char *)&cfip)[(i*8)+1], 
	      ((unsigned char *)&cfip)[(i*8)+2], 
	      ((unsigned char *)&cfip)[(i*8)+3], 
	      ((unsigned char *)&cfip)[(i*8)+4], 
	      ((unsigned char *)&cfip)[(i*8)+5], 
	      ((unsigned char *)&cfip)[(i*8)+6], 
	      ((unsigned char *)&cfip)[(i*8)+7]
	      );
  }
#endif
  print_cfi_ident(&cfip);
  for (i=0; i<cfip.NumEraseRegions; i++) {
    __u16 EraseRegionInfoNum = (*(unsigned char *) (base + ((0x2d + (4*i))*devwidth))) + 
      ((*(unsigned char *) (base + ((0x2e + (4*i))*devwidth)) << 8));
    __u16 EraseRegionInfoSize = (*(unsigned char *) (base + ((0x2f + (4*i))*devwidth))) + 
      ((*(unsigned char *) (base + ((0x30 + (4*i))*devwidth)) << 8));

    printk("  Erase Region #%d: BlockSize 0x%4.4X bytes, %d blocks\n",
	   i, EraseRegionInfoSize * 256, EraseRegionInfoNum+1);
  }
  

  return -ENXIO;

}
void print_cfi_ident(struct cfi_ident *cfip)
{
  if (cfip->qry[0] == 'Q' && cfip->qry[1] == 'R' && cfip->qry[2] == 'Y') {
    printk("Primary vendor: %4.4X\n", le16_to_cpu(cfip->P_ID));
    printk("Alternatve vendor: %4.4X\n", le16_to_cpu(cfip->A_ID));
    if (cfip->P)
	  printk("Primary Algorithm Table at  %4.4X\n", le16_to_cpu(cfip->P));
    else
      printk("No Primary Algorithm Table\n");
    
    if (cfip->A)
      printk("Alternate Algorithm Table at  %4.4X\n", le16_to_cpu(cfip->A));
    else
      printk("No Alternate Algorithm Table\n");
    printk("Vcc Minimum: %x.%x V\n", cfip->VccMin >> 4, cfip->VccMin & 0xf);
    printk("Vcc Maximum: %x.%x V\n", cfip->VccMax >> 4, cfip->VccMax & 0xf);
    if (cfip->VppMin) {
      printk("Vpp Minimum: %x.%x V\n", cfip->VppMin >> 4, cfip->VppMin & 0xf);
      printk("Vpp Maximum: %x.%x V\n", cfip->VppMax >> 4, cfip->VppMax & 0xf);
    }
    else
      printk("No Vpp line\n");
    
    printk("Typical byte/word write timeout: %d µs\n", 1<<cfip->WordWriteTimeoutTyp);
    printk("Maximum byte/word write timeout: %d µs\n", (1<<cfip->WordWriteTimeoutMax) * (1<<cfip->WordWriteTimeoutTyp));
    
    if (cfip->BufWriteTimeoutTyp || cfip->BufWriteTimeoutMax) {
      printk("Typical full buffer write timeout: %d µs\n", 1<<cfip->BufWriteTimeoutTyp);
      printk("Maximum full buffer write timeout: %d µs\n", (1<<cfip->BufWriteTimeoutMax) * (1<<cfip->BufWriteTimeoutTyp));
    }
    else
      printk("Full buffer write not supported\n");
    
    printk("Typical block erase timeout: %d µs\n", 1<<cfip->BlockEraseTimeoutTyp);
    printk("Maximum block erase timeout: %d µs\n", (1<<cfip->BlockEraseTimeoutMax) * (1<<cfip->BlockEraseTimeoutTyp));
    if (cfip->ChipEraseTimeoutTyp || cfip->ChipEraseTimeoutMax) {
      printk("Typical chip erase timeout: %d µs\n", 1<<cfip->ChipEraseTimeoutTyp); 
      printk("Maximum chip erase timeout: %d µs\n", (1<<cfip->ChipEraseTimeoutMax) * (1<<cfip->ChipEraseTimeoutTyp));
    }
    else
      printk("Chip erase not supported\n");
    
    printk("Device size: 0x%X bytes (%d Mb)\n", 1 << cfip->DevSize, 1<< (cfip->DevSize - 20));
    printk("Flash Device Interface description: 0x%4.4X\n", le16_to_cpu(cfip->InterfaceDesc));
    printk("Max. bytes in buffer write: 0x%x\n", 1<< le16_to_cpu(cfip->MaxBufWriteSize));
    printk("Number of Erase Block Regions: %d\n", cfip->NumEraseRegions);

  }
  else printk("Invalid CFI ident structure.\n");
}

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Common Flash Interface probe code.

[Jason Gunthorpe]

On Mon, 12 Jun 2000, David Woodhouse wrote:

> Anyone with memory-mapped CFI-compliant NOR flash, especially if it's not 
> in 16-bit mode like the one I'm testing on - could you test this probe code 

Nifty.. I have a FLASH SIMM at work which AFAIK is 32bits wide, with 4
8-bit CFI chips (I hope) interleaved and then another 4 cascaded at the
end of that set. So the bottom two address lines and top address line
select which of the 8 chips and the rest of them index the memory.

You should be able do two things with this sort of flash configuration:
  1) Small erase sectors and slower 8/16bit IO - this is done by abusing
     the address lines and indexing 1 chip at a time.
  2) Large erase sectors and fast 32bit IO, including 4 chip parallel
     writes.

It is hanging right off the CPU's main 32-bit memory bus with a chipselect
controlling a 16meg window to access it.

Assuming the flash is CFI it looks like your detector will find the first
4 chips, but not the second set.

> #ifdef __arm__
> /* Shoot me. Better still, shoot rmk. dwmw2 */
> #undef writeb
> #undef writew
> #undef readb
> #undef readw
> #define readb(l) (*(unsigned char *)(l))
> #define writeb(c,l)  do {*((unsigned char *)(l)) = (c);} while (0)
> #define readw(l) (*(unsigned short *)(l))
> #define writew(c,l)  do {*((unsigned short *)(l)) = (c);} while (0)
> /* It's OK. You can look back now. */
> #endif /* __arm__ */

Wots up with this? Some of the ARM ports have 'interesting' IO..

Jason



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

M-Systems Disk-On-Chip driver

[Adi Linden]

Hi,

Is the kernel driver for the doc devices working? I haven't tried mtd in a
while but I'd really like to replace the IGEL driver with something open
source.

TTYL,
Adi




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Common Flash Interface probe code.

[David Woodhouse]

jgg@deltatee.com said:
>  Assuming the flash is CFI it looks like your detector will find the
> first 4 chips, but not the second set. 

That's correct so far. Don't worry - the machine I'm working with has two 
flash chips mapped one after the other, so I'm going to be fixing that.

I'll even throw in actual read/write/erase code, rather than just probe :)

jgg@deltatee.com said:
>  Wots up with this? Some of the ARM ports have 'interesting' IO..

#define readb(p)	(panic("readb called, but not implemented"),0)

Which brings me to the other thing I wanted to discuss with you - because
Linux drivers basically needs to have a readb function per bus, the old
'mapped' code isn't quite generic enough.

I've put together a 'struct map_info' which contains read/write functions 
for the raw memory addresses, and these can handle paging if necessary. See 
include/linux/mtd/map.h and what I've done to kernel/vmax301.c for details.

There's an indirection per access, but it's cleaner like that, and in fact 
the overhead isn't _too_ horrible - flash is bloody slow anyway so we don't 
care too much about it there, and for accessing RAM or even reading flash, 
you have the copy_from() and copy_to() functions where the cost is O(1).

Care to offer an opinion? Especially as it's your code I'm replacing, so I 
know you've thought about this stuff.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: M-Systems Disk-On-Chip driver

[David Woodhouse]

adi@adis.on.ca said:
> Is the kernel driver for the doc devices working? I haven't tried mtd
> in a while but I'd really like to replace the IGEL driver with
> something open source.

It works fine in read-only mode. Some problems have been reported in write 
mode, but I haven't been able to reproduce them.

It's not yet very efficient in write mode, but it ought to work.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: M-Systems Disk-On-Chip driver

[Adi Linden]

Hi,

Hmmm... So are there some really easy instructions on how to build the
driver? Then I will give it a try!

TTYL,
Adi

On Tue, 13 Jun 2000, David Woodhouse wrote:

> 
> adi@adis.on.ca said:
> > Is the kernel driver for the doc devices working? I haven't tried mtd
> > in a while but I'd really like to replace the IGEL driver with
> > something open source.
> 
> It works fine in read-only mode. Some problems have been reported in write 
> mode, but I haven't been able to reproduce them.
> 
> It's not yet very efficient in write mode, but it ought to work.
> 
> --
> dwmw2
> 
> 



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: M-Systems Disk-On-Chip driver

[David Woodhouse]

adi@adis.on.ca said:
>  Hmmm... So are there some really easy instructions on how to build
> the driver? Then I will give it a try!

Try the 2 cvs instructions given on the web page.
follow with...

 cd mtd
 ./utils/MAKEDEV
 make
 cd kernel
 insmod ./mtdcore.o
 insmod ./doc2000.o
 insmod ./docprobe.o
 insmod ./nftl.o



--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Common Flash Interface probe code.

[Jason Gunthorpe]

On Tue, 13 Jun 2000, David Woodhouse wrote:

> jgg@deltatee.com said:
> >  Assuming the flash is CFI it looks like your detector will find the
> > first 4 chips, but not the second set. 
 
> That's correct so far. Don't worry - the machine I'm working with has two 
> flash chips mapped one after the other, so I'm going to be fixing that.
> 
> I'll even throw in actual read/write/erase code, rather than just probe :)

Oooh :> 
 
> jgg@deltatee.com said:
> >  Wots up with this? Some of the ARM ports have 'interesting' IO..
> 
> #define readb(p)	(panic("readb called, but not implemented"),0)
> 
> Which brings me to the other thing I wanted to discuss with you - because
> Linux drivers basically needs to have a readb function per bus, the old
> 'mapped' code isn't quite generic enough.

Well, yes and no, AFAIK. The mapped driver was always indented to be used
for memory mapped flash, that is flash that is connected directly to the
CPU data bus (ie it is RAM). On my two ARM boards the flash is like that,
on my three Intel boards it is like that and I'm sure that on other boards
it is like that too. 

Basically, if the CPU can boot from the flash then you should be able to
use the mapped interface for it. 

Boards where it is not, say the flash is on a PCI, should not use the
mapped interface. Remember most of these drivers are not general things
like network drivers - they only run on one very specific bit of hardware.
If mapped works, then it works, there is no worry about readb/writeb.

IMHO you should add another abstraction to keep this tidy:
  top) Full blow MTD interface with write, read, erase functions. This
       is what the MTD char, block and filesystems talk to
  flash) Common functions shared by direct memory mapped flash, rom and
         rams. Contains CFI and JEDEC routines, ram and rom reader and ram
         writer. Maybe some file systems speak to this layer too..
  mapped) Aide for Memory Mapped flash, provides a many
          readers, one writer type locking for the paging function, and
          the necessary fast IO functions using simple memory references.
  low level mapped driver) Provides a paging function and sets up the
          memory window [turn on write through, bring the physical memory
          into the page tables, etc]

So move the JEDEC probe/erase/write routines up a level and move the new
_read/writeXX functions up from the drivers into the mapped interface,
then clean up the JEDEC functions to use your new IO functions (right now
it does not look correct to me)

The intention I had was to keep the low level driver as dead simple as
possible, all it does is setup a memory window and provide a paging
function, it should never need changing. If I was going to do any locking
for SMP it would have been done in the mapped driver, since that is where
the long operations are hiding. 

Locking in the low level driver is probably not very usefull, the flash
driver also will need locking to ensure nobdy tries to access the flash
while it is erasing, so it has to wrap all those functions anyhow :<

It is really hard to test the low level drivers, most people don't have
the hardware to do it with, but everything else is perfectly generic.

> There's an indirection per access, but it's cleaner like that, and in fact 
> the overhead isn't _too_ horrible - flash is bloody slow anyway so we don't 
> care too much about it there, and for accessing RAM or even reading flash, 
> you have the copy_from() and copy_to() functions where the cost is O(1).

Thats true, but I would worry more about keeping those drivers as small
and simple as possible, they are less likely to break and easier to
write.

Jason



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Common Flash Interface probe code.

[David Woodhouse]

jgg@ualberta.ca said:
> On Tue, 13 Jun 2000, David Woodhouse wrote:
> > I'll even throw in actual read/write/erase code, rather than just
> > probe :)

> Oooh :> 

For the Intel/Sharp Extended Command Set (0001), that is.

> Well, yes and no, AFAIK. The mapped driver was always indented to be
> used for memory mapped flash, that is flash that is connected directly
> to the CPU data bus (ie it is RAM).

IIRC it shouldn't have been using readb at all, in that case. Just 
dereferencing pointers would be fine. (Thinks.... if you've been using it 
on ARM, was it me that added the readb() then?)


> IMHO you should add another abstraction to keep this tidy:
>   top) Full blow MTD interface with write, read, erase functions. This
>        is what the MTD char, block and filesystems talk to
>   flash) Common functions shared by direct memory mapped flash, rom and
>          rams. Contains CFI and JEDEC routines, ram and rom reader and ram
>          writer. Maybe some file systems speak to this layer too..
>   mapped) Aide for Memory Mapped flash, provides a many
>           readers, one writer type locking for the paging function, and
>           the necessary fast IO functions using simple memory references.
>   low level mapped driver) Provides a paging function and sets up the
>           memory window [turn on write through, bring the physical memory
>           into the page tables, etc]

I'm not sure why you want to separate the bottom two. The locking issues
don't work wonderfully if you make the 'page' function callable outside the
low-level mapped driver. Different hardware requires different locking, and 
it's worth having it all-in-one, with a set of access operations which 
lock, page and do the operation, then unlock.

What I have at the moment is...

    Map hardware driver produces a 'struct map_info' with access functions.

    This is passed to a various probe functions, for example ram_probe.,
	until one of them appears to like it.

    If ram_probe likes the stuff in the map (i.e. it behaves like RAM)
	it returns a (struct mtd_info *) which is populated with
	appropriate read/write/erase/sync methods and other data.

    The hardware driver then registers this with add_mtd_device.


There's also a 'cfi_probe' function which will return a populated MTD
if it finds CFI flash. I haven't yet ported the JEDEC probe to this setup.
rom_probe is a last resort, and returns an MTD device which is readonly and 
has only a read() function. 

Those probe functions correspond to the 'flash' layer to which you refer -
common functions shared by mmap'd flash chips in different physical devices.
It's not kept layered at runtime for efficiency reasons - the top-level MTD
access methods point directly at the routines provided by flash layer, and
they all use mtd->priv as a pointer to the struct map_info for that
particular device/mapping.


jgg@ualberta.ca said:
> The intention I had was to keep the low level driver as dead simple as
> possible, all it does is setup a memory window and provide a paging
> function, it should never need changing. If I was going to do any
> locking for SMP it would have been done in the mapped driver, since
> that is where the long operations are hiding. 

But the locking only needs to be done over individual access operations, 
and only the bottom-level hardware driver knows exactly what the locking 
rules are for its particular device. 

nora.c, for example, needs no locking because it's all memory-mapped 
linearly - there's only about 100 lines of code. It's the right place to do 
locking, but that doesn't mean it can't be dead simple. vmax301.c does 
locking now - would you claim it's not still simple?


jgg@ualberta.ca said:
>  Locking in the low level driver is probably not very usefull, the
> flash driver also will need locking to ensure nobdy tries to access
> the flash while it is erasing, so it has to wrap all those functions
> anyhow :< 

Most recent flash chips support Suspending of in-progress erases. The one 
I'm playing with ATM (Intel 28F128) can even do Program operations while an 
erase is suspended. You don't need the same kind of locking. The lock in 
the low-level driver isn't supposed to be a mutual exclusion lock, it's 
only supposed to protect against the page register being changed in the 
middle of an operation. I've done it with spinlocks because that's simple - 
but it {sh,c}ould be optimised to rwlocks, so you can have multiple 
concurrent accesses as long as none of them want to touch the page register.


jgg@ualberta.ca said:
>  Thats true, but I would worry more about keeping those drivers as
> small and simple as possible, they are less likely to break and easier
> to write.

That was my reason for wanting to do it this way. The map drivers provide a
set of very simple functions to access the underlying memory space. They 
handle their own locking so that the page register doesn't get abused, if 
they have one. That's all they do.

Splitting the map drivers into two separate layers would only make that 
more complex. There might be fewer lines of code in each individual map 
driver, but as a whole each would be 'less obviously correct' because the 
interaction with the higher-level map driver would have to be taken into 
account.


--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Common Flash Interface probe code.

[Jason Gunthorpe]

On Tue, 13 Jun 2000, David Woodhouse wrote:

> > Well, yes and no, AFAIK. The mapped driver was always indented to be
> > used for memory mapped flash, that is flash that is connected directly
> > to the CPU data bus (ie it is RAM).
 
> IIRC it shouldn't have been using readb at all, in that case. Just 
> dereferencing pointers would be fine. (Thinks.... if you've been using it 
> on ARM, was it me that added the readb() then?)

I just got the ARM boards two weeks ago.. the mapped driver probably
should not have been using readb..
 
> > IMHO you should add another abstraction to keep this tidy:
> >   top) Full blow MTD interface with write, read, erase functions. This
> >        is what the MTD char, block and filesystems talk to
> >   flash) Common functions shared by direct memory mapped flash, rom and
> >          rams. Contains CFI and JEDEC routines, ram and rom reader and ram
> >          writer. Maybe some file systems speak to this layer too..
> >   mapped) Aide for Memory Mapped flash, provides a many
> >           readers, one writer type locking for the paging function, and
> >           the necessary fast IO functions using simple memory references.
> >   low level mapped driver) Provides a paging function and sets up the
> >           memory window [turn on write through, bring the physical memory
> >           into the page tables, etc]
 
> I'm not sure why you want to separate the bottom two. The locking issues
> don't work wonderfully if you make the 'page' function callable outside the
> low-level mapped driver. Different hardware requires different locking, and 
> it's worth having it all-in-one, with a set of access operations which 
> lock, page and do the operation, then unlock.

Well, that would be the point, you wouldn't make the page function
callable outside the low level driver. If ever two EC's could get into the
page function at once that would be a bug in the higher level drivers.

> What I have at the moment is...

Yes, I saw..

>     This is passed to a various probe functions, for example ram_probe.,
> 	until one of them appears to like it.

IMHO the driver should simply populate the structure with the functions it
supports (either read/write/etc or page) and then pass that to a detect
function which does all supported probes. Then you can compile in/out
support for various kinds of devices if you want to.

Having the drivers call seperate probe functions is not what I had in mind
at all when I wrote that, I wanted to have a single probe function and a
'hint' structure.
 
> nora.c, for example, needs no locking because it's all memory-mapped 
> linearly - there's only about 100 lines of code. It's the right place to do 
> locking, but that doesn't mean it can't be dead simple. vmax301.c does 
> locking now - would you claim it's not still simple?

Well, it is about 2x as long as the original one :> It is simple right
now, but if you wanted to add a read/write lock then you'd have to put all
that complexity into all the low level drivers again, when they don't
really need it.

What I am saying is that a good fraction of your low level drivers will
look *exactly* the same:

void device_writebXX(...)
{
   lock();
   page();
   write();
   unlock();
}

or if there is no paging function:

void device_writebXX(..)
{
   write()
}

So why not refactor that code, it makes it simpler for everyone to
understand and takes about 100 lines out of half the current low level
drivers.

A little later you can add in a rwlock so you can do concurrent
reads/writes to the same physical page.

> Most recent flash chips support Suspending of in-progress erases. The one 
> I'm playing with ATM (Intel 28F128) can even do Program operations while an 
> erase is suspended. You don't need the same kind of locking. The lock in 

I know, the AMD ones I was fiddling with last year did this too. It does
mean that you have to wrap all possible IO functions so you can send the
abort sequence, then restart the erase. 

> the low-level driver isn't supposed to be a mutual exclusion lock, it's 
> only supposed to protect against the page register being changed in the 
> middle of an operation. I've done it with spinlocks because that's simple - 
> but it {sh,c}ould be optimised to rwlocks, so you can have multiple 
> concurrent accesses as long as none of them want to touch the page register.

I think if you clarify that the low level driver routines will never be
called concurrently with the same mtd structure that would be a suitably
simple design.. AFAIK this is what you are saying, which is great. Pity
both my boards share the IO window so they do need locking :<

> Splitting the map drivers into two separate layers would only make that 
> more complex. There might be fewer lines of code in each individual map 
> driver, but as a whole each would be 'less obviously correct' because the 
> interaction with the higher-level map driver would have to be taken into 
> account.

I disagree, all you should be looking at in the driver is if the paging
and memory setup functions are correct and that the page window is not
shared by multiple MTD devices. The mapped driver could handle all
remaining issues. You can test the mapped driver quite easially, you
cannot really test the device drivers.

Jason



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Common Flash Interface probe code.

[David Woodhouse]

jgg@ualberta.ca said:
>  Well, that would be the point, you wouldn't make the page function
> callable outside the low level driver. If ever two EC's could get into
> the page function at once that would be a bug in the higher level
> drivers. 

I have a board here which has two flash chips in the same 'map'. I
definitely want to be able to talk to each one individually and
concurrently. Locking individual chips against concurrent access is
_different_ to locking the page register.


jgg@ualberta.ca said:
>  IMHO the driver should simply populate the structure with the
> functions it supports (either read/write/etc or page) and then pass
> that to a detect function which does all supported probes. Then you
> can compile in/out support for various kinds of devices if you want
> to.

> Having the drivers call seperate probe functions is not what I had in
> mind at all when I wrote that, I wanted to have a single probe
> function and a 'hint' structure.

We can do that quite simply, probably with an inline routine in map.h to 
call all known probe functions in a sensible order. Map drivers which know 
exactly what type of device they will contain can still call the right 
probe function directly.


jgg@ualberta.ca said:
>  What I am saying is that a good fraction of your low level drivers
> will look *exactly* the same:

It's close, but it's not exactly identical. You outlined one case - 'paged
or unpaged'. There's also 'which bus is it on' and strange things like the 
octagon board where the page register is actually shared by two devices. You 
can't handle a decent 'cache' of the currently-available page, you have to 
call the page function each time. That's an out-of-line function call at 
_every_ access rather than only the inline checks that it currently does.

It's _possible_ to make a generic mid-level map driver handle all those
cases  - but it means extra conditional jumps and/or indirect function calls
at runtime, which I'm trying to avoid.


jgg@ualberta.ca said:
> I think if you clarify that the low level driver routines will never
> be called concurrently with the same mtd structure that would be a
> suitably simple design..

Suitably simple perhaps, but it's a restriction which should be unnecessary. 
Concurrent access is fine. Locking may want to be done at a chip level for 
flash (not RAM or ROM) but not at the 'map' level except as is necessary 
for paging reasons.


jgg@ualberta.ca said:
> Pity both my boards share the IO window so they do need locking :<

This particular feature means that your hypothetical middle layer map 
driver cannot cache the last accessed page for any map device, as it may
change when the device in question hasn't been accessed.

That's an important optimisation which is being thrown away because we're 
trying to be too generic.

This is entirely my point - to handle that case and the other strange 
cases which will occur, the mid-level driver is going to have to be 
horribly ugly. All this special-casing is going to have to be done at 
runtime, rather than at compile-time as it is at the moment. 

I agree that the amount of redundancy is a little less than ideal, but the
alternative is reduced _runtime_ performance.

It's not difficult for an author of a new driver to cut'n'paste an existing 
one. Yes it'll be largely similar, but that's not a problem. That's why 
it's under GPL - it's not a problem to cut'n'paste.








--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Common Flash Interface probe code

[Reynolds-Lear, Matthew]

Hi,
I've written a flash driver based on CFI.  It was going so well until I
needed to detect if a device was top or bottom boot.
 
Basically, from looking at different manufacturer datasheets, I am
totally lost as to the correct approach to determining if a device is
top or bottom boot, and if the CFI device's geometry data is presented
in a different order depending if the device is top or bottom boot. 
 
As I see it, here are my issues:
 
1) I could determine if a device is top or bottom boot by reading the
private vendor specific table in the CFI.  However, it seems that this
table is not standardised and each manufacturer has their own version of
it.  Because the CFI indicates what algorithm can be used to perform
operations on the device, you shouldn't have to check the device ID etc
to determine anything about it - thus the idea behind the CFI.  It seems
though that each manufacturer can put slightly different data in the
private table so to find out exactly you'd need to read it's
manufacturer ID.  When you've done this you might as well have done a
JEDEC probe!
 
2) Some manufacturers (e.g. AMD) don't even indicate in their private
table if the device is top or bottom boot so how are users of the CFI
supposed to find out apart from fetching the ID and looking it up in
some sort of table (again, the CFI was intended to remove having to do
this)?
 
3) From reading some datasheets, some devices present their device
geometry CFI data differently if the device is top or bottom boot.
Thing is, other datasheets indicate that some devices do not do this.  
 
Basically, I want to avoid having to read the device's ID to determine
if it is top or bottom boot and what its sector map is since this
involves keeping a flash driver look up table up to date and drop in
replacement of flash devices won't be possible without updating this
table.  It seems that some device's CFI data in the private table will
do this tell me if it is top or bottom boot but the format of the table
is completely specific to the manufacturer (so I'd have to check the
manufacturer ID anyway) but other device's CFI data doesn't provide this
information so how are we supposed to know?!
 
I've seen a few postings related to this problem and can't find anything
'solid' which will assist.
 
Can anyone help?  It's all a bit frustrating.
 
Many thanks.
  Matt
--