Some bugs

Some bugs

[Florian Schirmer / TayTron]

Hi!

Thanks a lot for this great piece of software!

While playing with it i noticed some bugs:



1. CFI-Intel P_ID 3 not supported

We have 2 interlaced Intel TE28F320 chips. They report P_ID = 3 which isnt
handled by mtd. After forcing this value to 1 by hand everything works very
well.

Workaround: cfi_probe.c ~ line 562:

/* Do any necessary byteswapping */
cfi.cfiq->P_ID = le16_to_cpu(cfi.cfiq->P_ID);

if (cfi.cfiq->P_ID == 3)
  cfi.cfiq->P_ID = 1;

cfi.cfiq->P_ADR = le16_to_cpu(cfi.cfiq->P_ADR);



2. CFI-Intel 16 Bit mode broken

Intel 16 bit wide access is broken because the extended query table is read
via read8() calls. Using cfi_read_query instead solves this issue.

Workaround: cfi_cmd_set_0001.c ~ line 105:

/* Read in the Extended Query Table */
for (i=0; i<sizeof(*extp); i++) {
  ((unsigned char *)extp)[i] =
     cfi_read_query(map, (base+((adr+i)*cfi->interleave*cfi->device_type)));
}


3. Write support is broken on ppc because of endianess problem

cat bla.img > /dev/mtd/0
cat /dev/mtd/0 > bla2.img

Results in a swapped file. I can only _guess_ where the problem is exactly.
Reading is done via read_fromio() while writing is done by using writew()
calls. One of these funtions seem to swapp the bytes. Maybe it isnt really a
mtd isse. Maybe it's my fault. I've read something in one of the include
files about the endianess issue but did not find the information again.

Workaround:

cfi_cmd_set.c ~ line 468:

ENABLE_VPP(map);
cfi_write(map, CMD(0x40), adr);
cfi_write(map, ___arch__swab16(datum), adr);  <--- added swab16
chip->state = FL_WRITING;


cfi_cmd_set.c ~ line 748:

if (cfi_buswidth_is_1()) {
   map->write8 (map, *((__u8*)buf)++, adr+z);
} else if (cfi_buswidth_is_2()) {
   map->write16 (map, ___arch__swab16(*((__u16*)buf)++), adr+z);  <--- added
swab16 (only for 16 bit type chips)
} else if (cfi_buswidth_is_4()) {
   map->write32 (map, *((__u32*)buf)++, adr+z);
} else {
   DISABLE_VPP(map);
   return -EINVAL;
}


4. Erase size is wrong

Chip is an CFI-Intel F640. Erase size is 128KB (reported correctly by mtd
while probing for the chip (DEBUG_CFI)!).

cfi_probe.c ~ 583:

printk("  Erase Region #%d: BlockSize 0x%4.4X bytes, %d blocks\n",
              i, (cfi.cfiq->EraseRegionInfo[i] >> 8) & ~0xff,
              (cfi.cfiq->EraseRegionInfo[i] & 0xffff) + 1);

Works corretly.

cfi_cmd_set.c ~ line 223:

mtd->erasesize = cfi16_to_cpu( (u16)(cfi->cfiq->EraseRegionInfo[0]
                                        >> 16) )*256 * cfi->interleave;

Calculates the wrong size (0x2000 instead of 0x20000) as far as i correctly
remember.


Workaround: cfi_cmd_set.c ~ line 223 (Ugly hack :-):

/* We have to assume that all the erase blocks are the same size. */
//mtd->erasesize = cfi16_to_cpu( (u16)(cfi->cfiq->EraseRegionInfo[0]
//                                        >> 16) )*256 * cfi->interleave;
mtd->erasesize = 128*1024;
/* Also select the correct geometry setup too */


Maybe this information helps a bit to fix the last bugs in mtd :) If more
infos are needed please contact me!

Thanks a lot!
   Florian Schirmer



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

Re: Some bugs

[David Woodhouse]

On Fri, 2 Mar 2001, Florian Schirmer / TayTron wrote:

> 1. CFI-Intel P_ID 3 not supported
> 
> We have 2 interlaced Intel TE28F320 chips. They report P_ID = 3 which isnt
> handled by mtd. After forcing this value to 1 by hand everything works very
> well.

Sensible workaround for now. We need to make sure the cfi_cmdset_0001
code doesn't try to use any commands from the extended command set, but I 
don't think it supports any yet, so it's not an issue.

We need to rework the loading of submodules anyway, so we can fix this 
properly at that time.

> 2. CFI-Intel 16 Bit mode broken
> 
> Intel 16 bit wide access is broken because the extended query table is read
> via read8() calls. Using cfi_read_query instead solves this issue.

OK, applied. Thanks.

> 3. Write support is broken on ppc because of endianess problem
> 
> cat bla.img > /dev/mtd/0
> cat /dev/mtd/0 > bla2.img
> 
> Results in a swapped file. I can only _guess_ where the problem is exactly.
> Reading is done via read_fromio() while writing is done by using writew()
> calls. One of these funtions seem to swapp the bytes. Maybe it isnt really a
> mtd isse. Maybe it's my fault. I've read something in one of the include
> files about the endianess issue but did not find the information again.

Once upon a time, the PPC map drivers were doing byte-swapping in write8 
but not in the copy_{to,from} calls. This was wrong - the chip driver 
needs to handle the correct endianness for the commands, and the map 
driver should do no byteswapping at all.

Sounds like there's still an element of confusion somewhere.

> cfi_write(map, ___arch__swab16(datum), adr);  <--- added swab16

>    map->write16 (map, ___arch__swab16(*((__u16*)buf)++), adr+z);  <--- added swab16 (only for 16 bit type chips)

That's not a generic solution. There should be no byteswapping of data 
anywhere. What map driver are you using? It sounds like it's byteswapping 
on write8(), which is wrong. Make it stop doing that and define 
CFI_HOST_ENDIAN or CFI_BIG_ENDIAN in include/linux/mtd/cfi_endian.h

> 4. Erase size is wrong
> 
> Chip is an CFI-Intel F640. Erase size is 128KB (reported correctly by mtd
> while probing for the chip (DEBUG_CFI)!).
> 
> cfi_probe.c ~ 583:
> 
> printk("  Erase Region #%d: BlockSize 0x%4.4X bytes, %d blocks\n",
>               i, (cfi.cfiq->EraseRegionInfo[i] >> 8) & ~0xff,
>               (cfi.cfiq->EraseRegionInfo[i] & 0xffff) + 1);
> 
> Works corretly.
> 
> cfi_cmd_set.c ~ line 223:
> 
> mtd->erasesize = cfi16_to_cpu( (u16)(cfi->cfiq->EraseRegionInfo[0]
>                                         >> 16) )*256 * cfi->interleave;
> 
> Calculates the wrong size (0x2000 instead of 0x20000) as far as i correctly
> remember.

That is very very strange. Either something is changing EraseRegionInfo[0] 
or cfi->interleave, despite being an integer datatype, contains 1/16.

Can you print the contents of EraseRegionInfo[0] at both locations?


-- 
dwmw2




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

AW: Some bugs

[Florian Schirmer / TayTron]

Hi!

>> 3. Write support is broken on ppc because of endianess problem
>>
>> cat bla.img > /dev/mtd/0
>> cat /dev/mtd/0 > bla2.img
>>
>> Results in a swapped file. I can only _guess_ where the problem is
exactly.
>> Reading is done via read_fromio() while writing is done by using writew()
>> calls. One of these funtions seem to swapp the bytes. Maybe it isnt
really a
>> mtd isse. Maybe it's my fault. I've read something in one of the include
>> files about the endianess issue but did not find the information again.

[...]

>That's not a generic solution. There should be no byteswapping of data
>anywhere. What map driver are you using? It sounds like it's byteswapping
>on write8(), which is wrong. Make it stop doing that and define
>CFI_HOST_ENDIAN or CFI_BIG_ENDIAN in include/linux/mtd/cfi_endian.h

physmap.c. It isn't doing any byteswapping. Thats the point. Yes this was
the include file i was looking for. But it doesnt solve the problem. Since
physmap ist unsing it at all changing the mode to big_endian breaks the
whole thing. The chips wont be detected since their bytes are flipped :)

So the problem must be either in the mtd cfi_cmd_set module (not swapping
while writing data) or somethere in the kernel stuff. Any ideas? This is the
only true problem at the moment i dont know a good workaround for.

>> 4. Erase size is wrong
>>
>That is very very strange. Either something is changing EraseRegionInfo[0]
>or cfi->interleave, despite being an integer datatype, contains 1/16.
>Can you print the contents of EraseRegionInfo[0] at both locations?

Okay found the problem: EraseRegionInfo[0] is equal at both positions. And
it's already in correct byteorder in BOTH positions. Swapping the order
again in cfi_cmd_set_0001 causes the trouble. This results in erase size
0x200 instead of 0x20000. So please remove it.

So please can you commit the 3 solved issues to cvs?

Thanks a lot
   Florian Schirmer



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

Re: AW: Some bugs

[David Woodhouse]

On Sat, 3 Mar 2001, Florian Schirmer / TayTron wrote:

> So the problem must be either in the mtd cfi_cmd_set module (not swapping
> while writing data) or somethere in the kernel stuff. Any ideas? This is the
> only true problem at the moment i dont know a good workaround for.

Aha - writew is defined as little-endian. We should be using __raw_writew() 
in physmap.c, and possibly mb() after it.

I've committed that change - please test.

> Okay found the problem: EraseRegionInfo[0] is equal at both positions. And
> it's already in correct byteorder in BOTH positions. Swapping the order
> again in cfi_cmd_set_0001 causes the trouble. This results in erase size
> 0x200 instead of 0x20000. So please remove it.

OK, that's applied too, thanks.

> So please can you commit the 3 solved issues to cvs?

The first was already there. The above two just joined it. Please confirm 
that the writew change is working.

Thanks.

-- 
dwmw2




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

Re: AW: Some bugs

[Florian Schirmer / TayTron]

Hi..

> > So please can you commit the 3 solved issues to cvs?
>
> The first was already there. The above two just joined it. Please confirm
> that the writew change is working.

Looks very good. We've tried Intel F640, 2x Intel F320 (interleaved) and 2x
AMD (interleaved). Everything is fine now.

There are still some points on my list:

1. cfi_cmdset_0001 broken:

/* We have to assume that all the erase blocks are the same size. */
mtd->erasesize = ((cfi.cfiq->EraseRegionInfo[i] >> 8) & ~0xff) *
cfi->interleave;

should be

mtd->erasesize = ((cfi->cfiq->EraseRegionInfo[0] >> 8) & ~0xff) *
cfi->interleave;

(cfi-> and  [0])

2. physmap (?) broken:

The inter_module stuff thems to have a problem somewhere. The modules cant
be unloaded since the usagecount wont decrement correctly. Dont know wether
it's the fault of physmap or not.

Happing hacking!
   Florian Schirmer



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

Re: AW: Some bugs

[David Woodhouse]

schirmer@taytron.net said:
> 1. cfi_cmdset_0001 broken: 

Doh! That's fixed, sorry.


schirmer@taytron.net said:
>  2. physmap (?) broken:
> The inter_module stuff thems to have a problem somewhere. The modules
> cant be unloaded since the usagecount wont decrement correctly. Dont
> know wether it's the fault of physmap or not. 

The inter_module stuff is just fundamentally screwed, as far as I'm 
concerned. I'm severely tempted just to take it out and let the modules 
have hard dependencies on each other.

--
dwmw2




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

MTD Patches

[Florian Schirmer / TayTron]

Hi!

I've two little patches pending. Can someone please commit them to cvs?

Patch1:

Adds partition support to physmap.c. Support can be enabled via kernel
config, so if you dont need it you wont get bigger modules/kernels.

Patch2: Fixes a small bug (?) in mtdblock_ro.c Please verify this. Seems
someone disabled a function but did not disable the corresponding function
call :-)

Thanks for your help and happing hacking
   Florian Schirmer

[......]

diff -Naur mtd-old/kernel/Config.in mtd/kernel/Config.in
--- mtd-old/kernel/Config.in Sat Feb 17 22:17:48 2001
+++ mtd/kernel/Config.in Mon Mar  5 10:51:27 2001
@@ -79,6 +79,7 @@
       hex '    Physical start address of flash mapping'
CONFIG_MTD_PHYSMAP_START 0x8000000
       hex '    Physical length of flash mapping' CONFIG_MTD_PHYSMAP_LEN
0x4000000
       int '    Bus width in octets' CONFIG_MTD_PHYSMAP_BUSWIDTH 2
+      bool '    Partition suport' CONFIG_MTD_PHYSMAP_PARTITION
    fi
    dep_tristate '  CFI Flash device mapped on Nora' CONFIG_MTD_NORA
$CONFIG_MTD_CFI
    dep_tristate '  CFI Flash device mapped on Photron PNC-2000'
CONFIG_MTD_PNC2000 $CONFIG_MTD_CFI
diff -Naur mtd-old/kernel/mtdblock_ro.c mtd/kernel/mtdblock_ro.c
--- mtd-old/kernel/mtdblock_ro.c Fri Nov 17 09:51:35 2000
+++ mtd/kernel/mtdblock_ro.c Mon Mar  5 11:20:51 2001
@@ -222,7 +222,9 @@
       // Grab the lock and re-thread the item onto the linked list
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,2,0)
  spin_lock_irq(&io_request_lock);
+#if 0
  mtdblock_end_request(current_request, res);
+#endif
 #endif
  end_request(res);
    }
diff -Naur mtd-old/kernel/physmap.c mtd/kernel/physmap.c
--- mtd-old/kernel/physmap.c Sat Mar  3 19:17:09 2001
+++ mtd/kernel/physmap.c Mon Mar  5 11:17:25 2001
@@ -12,6 +12,21 @@
 #include <linux/mtd/map.h>
 #include <linux/config.h>

+#ifdef CONFIG_MTD_PHYSMAP_PARTITION
+#include <linux/mtd/partitions.h>
+
+/* partition_info gives details on the logical partitions that the split
the
+ * single flash device into. If the size if zero we use up to the end of
the
+ * device. */
+const static struct mtd_partition partition_info[] = {/*{name:
"Bootloader",
+             offset: MTDPART_OFS_APPEND,
+             size: 128 * 1024},*/
+            {name: "Physically mapped flash",
+             offset: MTDPART_OFS_APPEND,
+             size: MTDPART_SIZ_FULL}};
+
+#define NUM_PARTITIONS (sizeof(partition_info) / sizeof(partition_info[0]))
+#endif // CONFIG_MTD_PHYSMAP_PARTITION

 #define WINDOW_ADDR CONFIG_MTD_PHYSMAP_START
 #define WINDOW_SIZE CONFIG_MTD_PHYSMAP_LEN
@@ -95,7 +110,13 @@
 #ifdef MODULE
   mymtd->module = &__this_module;
 #endif
+
+#ifdef CONFIG_MTD_PHYSMAP_PARTITION
+                /* Create MTD devices for each partition. */
+         add_mtd_partitions(mymtd, partition_info, NUM_PARTITIONS);
+#else
   add_mtd_device(mymtd);
+#endif // CONFIG_MTD_PHYSMAP_PARTITION
   return 0;
  }

@@ -106,7 +127,11 @@
 mod_exit_t cleanup_physmap(void)
 {
  if (mymtd) {
+#ifdef CONFIG_MTD_PHYSMAP_PARTITION
+  del_mtd_partitions(mymtd);
+#else
   del_mtd_device(mymtd);
+#endif // CONFIG_MTD_PHYSMAP_PARTITION
   map_destroy(mymtd);
  }
  if (physmap_map.map_priv_1) {




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

Re: AW: Some bugs

[Florian Schirmer / TayTron]

Hi!

> > 1. cfi_cmdset_0001 broken:
> Doh! That's fixed, sorry.

Damn. All the cut and paste junkies out there :-) (Including me of course!)

> The inter_module stuff is just fundamentally screwed, as far as I'm
> concerned. I'm severely tempted just to take it out and let the modules
> have hard dependencies on each other.

For what reason it is still in there? I'm not that expirienced kernel
hacker. So maybe you can give me some hints. It makes developing rather
complicated since i have to reset the board to get rid of the cfi modules.
Maybe we should disable to stuff until it is fixed?

Cya
   Florian Schirmer



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

Re: AW: Some bugs

[David Woodhouse]

schirmer@taytron.net said:
>  For what reason it is still in there? I'm not that expirienced kernel
> hacker. So maybe you can give me some hints. It makes developing
> rather complicated since i have to reset the board to get rid of the
> cfi modules. Maybe we should disable to stuff until it is fixed?

The code originally used get_module_symbol(), which did what I wanted quite 
nicely - it meant that modules like cfi_probe didn't have to have a hard 
dependency on every individual command set module. It was changed to the 
new inter_module_xxx abomination when Linus decided to include it in 
2.4.0-test$something, and it introduced link order dependencies and 
generally broke. Let's just strip it out and let the modules depend on each 
other. We can play ugly ifdef tricks to 'fix' that, it's better than using 
inter_module_get().



--
dwmw2




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

Re: AW: Some bugs

[Florian Schirmer / TayTron]

Hi!

> The code originally used get_module_symbol(), which did what I wanted
quite
> nicely - it meant that modules like cfi_probe didn't have to have a hard
> dependency on every individual command set module. It was changed to the
> new inter_module_xxx abomination when Linus decided to include it in
> 2.4.0-test$something, and it introduced link order dependencies and
> generally broke. Let's just strip it out and let the modules depend on
each
> other. We can play ugly ifdef tricks to 'fix' that, it's better than using
> inter_module_get().

Yeah. That is what i thought too :-) Lets do some ifdefs and get rid off the
beast. I will commit a patch for the cfi stuff soon.

Thanks
  Florian Schirmer





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