diff mbox

Wrong flash type in m25p80 driver

Message ID 4BF011D7.4010907@shemesh.biz
State New, archived
Headers show

Commit Message

Shachar Shemesh May 16, 2010, 3:40 p.m. UTC 
Hi all,

I've found a problem in the m25p80 driver (drives a bunch of SPI flash 
chips based on similar commands). While the commands that drive those 
chips are almost exactly the same, some variance between the chips does 
exist. Almost all of them have 64KB erase blocks, but some also have 
smaller 4KB erase sectors (terminology differs between chips, but the 
two sizes, fairly universally, don't).

My understanding of the mtd_info struct is that this is precisely what 
the "writesize" field is for - specifying different maximal and minimal 
erase sizes. The current m25p80 driver does not specify the two sizes 
correctly, which leads to total failure when trying to use it to create 
UBIFS volumes.

The second problem I'm having with this driver is that it reports itself 
as NOR flash. This is obviously not the case, but I'm a bit at a loss as 
to what is. These are not, exactly, NAND flashes. DATAFLASH is closer to 
the mark, as both these and the DATAFLASH had SPI interfaces, but 
setting the type to DATAFLASH triggers nasty behavior by the jffs2 code, 
which assumes that data flashes have erase blocks of either 536 or 1056 
bytes. The result is that it allocates 512KB of erase blocks, causing a 
1.5MB of overhead on each volume. For a 4MB flash, that is a large 
overhead to accept.

I'm attaching a patch to the m25p80 driver. I would love to receive 
comments on the correct flash type to put there for proper operation. 
Something that is compatible with jffs2 would be appreciated. I've 
written a fix to the jffs2 code, but I'm not sure of its correctness.

Thanks,
Shachar

Comments

Mike Frysinger May 16, 2010, 6:40 p.m. UTC | #1 
2010/5/16 Shachar Shemesh:
> The second problem I'm having with this driver is that it reports itself as
> NOR flash.

the response is: so what ?  you havent described how this is
problematic in any way.
-mike
Mike Frysinger May 16, 2010, 7:17 p.m. UTC | #2 
On Sun, May 16, 2010 at 15:04, Shachar Shemesh wrote:
> Mike Frysinger wrote:
>> 2010/5/16 Shachar Shemesh:
>>> The second problem I'm having with this driver is that it reports itself as
>>> NOR flash.
>>
>> the response is: so what ?  you havent described how this is
>> problematic in any way.
>
> Then why have flash type at all, if no-one cares?

so your answer is "there is no problem with calling it NOR flash"

if it looks like a duck, quacks like a duck, and walks like a duck,
then why waste time calling it a DataDuck ?  for all intents and
purposes, all the kernel code/utilities that work with the flashes can
treat these SPI flashes as NOR flashes and everything works fine.

> Either way, the patch in my previous message is intended for inclusion.

it isnt acceptable in the current format.  so until you clean it up
and submit it properly, nothing is going to happen.

this of course ignores the errors in the patch like calling it
DataFlash which you yourself clearly have shown is wrong.  DataFlash
is a special type of flash made by Atmel.  it has properties which
other SPI flashes do not.
-mike
Mike Frysinger May 17, 2010, 4:54 a.m. UTC | #3 
On Sun, May 16, 2010 at 15:55, Shachar Shemesh wrote:
> Mike Frysinger wrote:
>> so your answer is "there is no problem with calling it NOR flash"
>>
>> if it looks like a duck, quacks like a duck, and walks like a duck,
>> then why waste time calling it a DataDuck ?  for all intents and
>> purposes, all the kernel code/utilities that work with the flashes can
>> treat these SPI flashes as NOR flashes and everything works fine.
>
> Actually, that is not the case. Not even remotely.
>
> NOR flashes (and the m25p80 driver broadcasts it this way) have the ability
> to change individual bytes (and, some flashes, individual bits). The SPI
> flashes need to erase an entire block. NOR flashes are accessible via the
> data bus, these require complicated SPI transactions. They neither quack nor
> look anywhere near the same.

you havent shown how the current behavior results in non-working
flashes.  you probably can claim that things dont work as optimally as
possible according to the hardware, but as you noted, people work on
what interests/annoys them rather than what other people report.

> You might want to claim that they are NAND flashes, but, again, those are
> somewhat different. Dataflashes are the closest these come to, as far as I
> can tell.

it doesnt make sense to lump SPI and NAND together because NAND allows
for bad blocks.  both NOR and SPI are guaranteed to be good throughout
or the device is dead.  so for all practical uses thus far, SPI and
NOR operate the same way.

>> it isnt acceptable in the current format.  so until you clean it up
>> and submit it properly, nothing is going to happen.
>
> Care to elaborate? Have never submitted patches to Linux before. I don't
> mind RTFM, but please point me to the right manual.

all Linux documentation is in the folder aptly named "Documentation",
and from there it's a short hop to "SubmittingPatches".

>> this of course ignores the errors in the patch like calling it
>> DataFlash which you yourself clearly have shown is wrong.  DataFlash
>> is a special type of flash made by Atmel.  it has properties which
>> other SPI flashes do not.
>
> What would have really been necessary was a property saying "Serial flash".
> I'm afraid I don't fully understand all of the consequence of adding such a
> property, however. For one thing, this type is exported to user space. If
> that's the only way to get this patch in, I'll do it, of course. I was
> attempting to defer to your experience on the matter.

since it is an exported ABI as you noted, the details need to be
thought & hashed out before committing things as typically you cant go
back.  so why not start with a list of the differences that matter to
the MTD layers where the NOR flashes differ from SPI flashes.

emphasis here is on "where it matters to the MTD layers".  if the MTD
layers operate no differently whatsoever when encountering NOR and SPI
flashes, then it doesnt really make sense to declare a new type for
it.

> I am not so sure about the "properties which other SPI flashes do not". Not,
> at least, according to my research (which does not beat experience, but does
> beat obscure abstract statements).

iirc, dataflashes allow programming on individual bytes just like NOR
flashes.  most serial flashes (like the ST micro that started the SPI
flash driver in the first place) have to be programmed on a page
basis.  dataflashes also have more flexible erase structures (4kb,
32kb, or 64kb) while ST micro can only be erased at 64kb sectors.

my point is just that calling all SPI flashes "dataflashes" is
inherently wrong since only Atmel makes dataflashes.  it's also a bit
funny since you started off the thread with the complaint that calling
SPI flashes NOR flashes was wrong because SPI flashes arent NOR
flashes.  if you're going to declare a new category for SPI flashes,
then it should be something that applies to all the current SPI
flashes (just look in the m25p80.c driver to see all the ones
supported).

> Again, I'm trying to recruit the collective knowledge of this list in order
> to do the right thing, not get into a fight.

i'm just a schlub here.  i'm not a MTD maintainer and wont be
approving any patches.  but this list suffers from responses, so i
figured i'd throw something out i have a passing knowledge of.

> I don't mind doing the work,
> but I need help filling in the knowledge. I was told that newbies are
> welcome, so long as they show a willingness not to dump their desires on
> others, say by submitting actual patches.....

that is indeed the way.  but you cant really say "this patch is
intended for inclusion" when (1) it is incorrectly formatted and (2)
it has clearly unacceptable known bugs before the patch was even
reviewed.
-mike
Carl-Daniel Hailfinger May 17, 2010, 4:50 p.m. UTC | #4 
On 17.05.2010 06:54, Mike Frysinger wrote:
> On Sun, May 16, 2010 at 15:55, Shachar Shemesh wrote:
>   
>> Mike Frysinger wrote:
>>     
>>> so your answer is "there is no problem with calling it NOR flash"
>>>
>>> if it looks like a duck, quacks like a duck, and walks like a duck,
>>> then why waste time calling it a DataDuck ?  for all intents and
>>> purposes, all the kernel code/utilities that work with the flashes can
>>> treat these SPI flashes as NOR flashes and everything works fine.
>>>       
>> Actually, that is not the case. Not even remotely.
>>
>> NOR flashes (and the m25p80 driver broadcasts it this way) have the ability
>> to change individual bytes (and, some flashes, individual bits). The SPI
>> flashes need to erase an entire block. NOR flashes are accessible via the
>> data bus, these require complicated SPI transactions. They neither quack nor
>> look anywhere near the same.
>>     
>
> you havent shown how the current behavior results in non-working
> flashes.  you probably can claim that things dont work as optimally as
> possible according to the hardware, but as you noted, people work on
> what interests/annoys them rather than what other people report.
>   

ST M25P80 can change individual bytes.


>> You might want to claim that they are NAND flashes, but, again, those are
>> somewhat different. Dataflashes are the closest these come to, as far as I
>> can tell.
>>     
>
> it doesnt make sense to lump SPI and NAND together because NAND allows
> for bad blocks.  both NOR and SPI are guaranteed to be good throughout
> or the device is dead.  so for all practical uses thus far, SPI and
> NOR operate the same way.
>   

Yes.


>> I am not so sure about the "properties which other SPI flashes do not". Not,
>> at least, according to my research (which does not beat experience, but does
>> beat obscure abstract statements).
>>     
>
> iirc, dataflashes allow programming on individual bytes just like NOR
> flashes.  most serial flashes (like the ST micro that started the SPI
> flash driver in the first place) have to be programmed on a page
> basis.  dataflashes also have more flexible erase structures (4kb,
> 32kb, or 64kb) while ST micro can only be erased at 64kb sectors.
>   

The ST M25P80 has a minimum write size of 1 bit (datasheet is a bit
unclear, could also be 1 byte) and a maximum write size of 256 bytes.


> my point is just that calling all SPI flashes "dataflashes" is
> inherently wrong since only Atmel makes dataflashes.  it's also a bit
> funny since you started off the thread with the complaint that calling
> SPI flashes NOR flashes was wrong because SPI flashes arent NOR
> flashes.  if you're going to declare a new category for SPI flashes,
> then it should be something that applies to all the current SPI
> flashes (just look in the m25p80.c driver to see all the ones
> supported).
>   

DataFlash is special because some of the chips have a user configurable
page size: 512 or 528 Bytes per page.

There's always the option of looking at how flashrom
<http://www.flashrom.org/> handles those chips. flashrom an
OS-independent userspace tool specialized on chips which are used for
BIOS/firmware, but it handles some other flash chips as well.


Regards,
Carl-Daniel
Shachar Shemesh May 18, 2010, 6:37 a.m. UTC | #5 
I should point out that I have sent a retraction to the patch, but my 
messages got held up for moderation with "suspicious header" and no 
further explanation. I'm taking a wild guess as to the reason, in the 
hope that this message does get through. If it does, holding messages 
with "list does not support HTML or mixed format mail" will be a more 
useful error report.

Carl-Daniel Hailfinger wrote:
>   
>   
>
> The ST M25P80 has a minimum write size of 1 bit (datasheet is a bit
> unclear, could also be 1 byte) and a maximum write size of 256 bytes.
>
>   
I have not studies the M25P80 data sheet, but did the M25P32 and the 
MX25L6405D chips, and I believe all SPI flahses handled by the m25p80 
driver behave the same in that regard (which is why they were clamped 
together to begin with).

The minimal "program" length is 1 byte, but since a program can only 
change a 1 bit into 0, effectively, setting a word to "11...101...11", 
where the zero is at the bit you want to set, will program 1 bit.
>
> There's always the option of looking at how flashrom
> <http://www.flashrom.org/> handles those chips. flashrom an
> OS-independent userspace tool specialized on chips which are used for
> BIOS/firmware, but it handles some other flash chips as well.
>   
It lists them as "SPI", and the chip support page claims, at least for 
most of them, that it does not know how to erase them 
(http://www.flashrom.org/Supported_hardware).

Shachar
Carl-Daniel Hailfinger May 18, 2010, 12:12 p.m. UTC | #6 
On 18.05.2010 08:37, Shachar Shemesh wrote:
> Carl-Daniel Hailfinger wrote:
>> The ST M25P80 has a minimum write size of 1 bit (datasheet is a bit
>> unclear, could also be 1 byte) and a maximum write size of 256 bytes.
>>   
> I have not studies the M25P80 data sheet, but did the M25P32 and the
> MX25L6405D chips, and I believe all SPI flahses handled by the m25p80
> driver behave the same in that regard (which is why they were clamped
> together to begin with).
>
> The minimal "program" length is 1 byte, but since a program can only
> change a 1 bit into 0, effectively, setting a word to "11...101...11",
> where the zero is at the bit you want to set, will program 1 bit.

Yes. The situation is a bit complicated. Some flash chips will not
accept further writes to a location that has been written once even if
those further writes would only change more bits to 0. Such chips have
true 1 byte granularity. Other chips (and AFAIK the whole ST M25 series)
can do at least two writes per byte location as long as the second write
does not set any 0 bit to 1.


>> There's always the option of looking at how flashrom
>> <http://www.flashrom.org/> handles those chips. flashrom an
>> OS-independent userspace tool specialized on chips which are used for
>> BIOS/firmware, but it handles some other flash chips as well.
>>   
> It lists them as "SPI", and the chip support page claims, at least for
> most of them, that it does not know how to erase them
> (http://www.flashrom.org/Supported_hardware).

Thanks for pointing out that this page is misleading. The status "?"
means that it is untested and should work. Only a read "No" means
unsupported. I'll change the wiki to be more readable.

Regards,
Carl-Daniel
diff mbox

Patch

Index: drivers/mtd/devices/m25p80.c
===================================================================
--- drivers/mtd/devices/m25p80.c	(revision 3333)
+++ drivers/mtd/devices/m25p80.c	(working copy)
@@ -212,7 +212,7 @@ 
 {
 	DEBUG(MTD_DEBUG_LEVEL3, "%s: %s %dKiB at 0x%08x\n",
 			dev_name(&flash->spi->dev), __func__,
-			flash->mtd.erasesize / 1024, offset);
+			flash->mtd.writesize / 1024, offset);
 
 	/* Wait until finished previous write command. */
 	if (wait_till_ready(flash))
@@ -255,7 +255,7 @@ 
 	/* sanity checks */
 	if (instr->addr + instr->len > flash->mtd.size)
 		return -EINVAL;
-	div_u64_rem(instr->len, mtd->erasesize, &rem);
+	div_u64_rem(instr->len, mtd->writesize, &rem);
 	if (rem)
 		return -EINVAL;
 
@@ -286,8 +286,8 @@ 
 				return -EIO;
 			}
 
-			addr += mtd->erasesize;
-			len -= mtd->erasesize;
+			addr += mtd->writesize;
+			len -= mtd->writesize;
 		}
 	}
 
@@ -831,9 +831,8 @@ 
 	else
 		flash->mtd.name = dev_name(&spi->dev);
 
-	flash->mtd.type = MTD_NORFLASH;
-	flash->mtd.writesize = 1;
-	flash->mtd.flags = MTD_CAP_NORFLASH;
+	flash->mtd.type = MTD_DATAFLASH;
+	flash->mtd.flags = MTD_WRITEABLE;
 	flash->mtd.size = info->sector_size * info->n_sectors;
 	flash->mtd.erase = m25p80_erase;
 	flash->mtd.read = m25p80_read;
@@ -845,12 +844,13 @@ 
 		flash->mtd.write = m25p80_write;
 
 	/* prefer "small sector" erase if possible */
-	if (info->flags & SECT_4K) {
+	flash->mtd.erasesize = info->sector_size;
+	if (info->flags & SECT_4K && (info->sector_size%4096)==0 ) {
 		flash->erase_opcode = OPCODE_BE_4K;
-		flash->mtd.erasesize = 4096;
+		flash->mtd.writesize = 4096;
 	} else {
 		flash->erase_opcode = OPCODE_SE;
-		flash->mtd.erasesize = info->sector_size;
+		flash->mtd.writesize = info->sector_size;
 	}
 
 	flash->mtd.dev.parent = &spi->dev;
@@ -860,10 +860,11 @@ 
 
 	DEBUG(MTD_DEBUG_LEVEL2,
 		"mtd .name = %s, .size = 0x%llx (%lldMiB) "
-			".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n",
+			".erasesize = 0x%.8x (%uKiB) .writesize = 0x%.8x (%uKiB) .numeraseregions = %d\n",
 		flash->mtd.name,
 		(long long)flash->mtd.size, (long long)(flash->mtd.size >> 20),
 		flash->mtd.erasesize, flash->mtd.erasesize / 1024,
+		flash->mtd.writesize, flash->mtd.writesize / 1024,
 		flash->mtd.numeraseregions);
 
 	if (flash->mtd.numeraseregions)