Skip to contentSkip to conversation

Remove hardcoded number of CFI chips

PATCH In review Deepak Saxena 2 participants 2 messages Nov 6, 2003 Memory technology devices (MTD)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Remove hardcoded number of CFI chips

All,

The attached patch is an update of the patch I posted about a month ago [1] that removes the hardcoded CFI_MAX_CHIPS, which was originally written by Andrzej Mialkowski @ Intel. This patch fixes two issues:

- Get rid of extra structure probe_info that was just carrying around a long* variable.

- Fix bug in bitmap size on systems with 2-8 chips. The old patch had:

  +	max_chips = map->size >> cfi.chipshift;
  +	probe.chip_map = kmalloc((max_chips + 7) / 8, GFP_KERNEL);

This would cause systems with 2-8 chips to be allocated only one byte for the bitmap instead of the 2 needed. Fixed code does:

+ max_chips = map->size >> cfi.chipshift;
+ chip_map = kmalloc((max_chips / 8) + ((max_chip % 8) ? 1 : 0), GFP_KERNEL);

Probably a cleaner way to do this. We could just use a u32 of this kmalloc which would give us a limit of 32 chips, which _should_ be OK for most systems and when we need more, just move to a u64. If this is prefered, I'll update the patch.

Comments?

[1]: http://lists.infradead.org/pipermail/linux-mtd/2003-October/008608.html

Index: drivers/mtd/chips/cfi_probe.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_probe.c,v retrieving revision 1.72

mtd/chips/cfi_probe.c +15 -15
diff -u -r1.72 cfi_probe.c
@@ -26,7 +26,7 @@
 #endif
 
 static int cfi_probe_chip(struct map_info *map, __u32 base,
-			  struct flchip *chips, struct cfi_private *cfi);
+			  unsigned long *chip_map, struct cfi_private *cfi);
 static int cfi_chip_setup(struct map_info *map, struct cfi_private *cfi);
 
 struct mtd_info *cfi_probe(struct map_info *map);
@@ -49,7 +49,7 @@
 }
 
 static int cfi_probe_chip(struct map_info *map, __u32 base,
-			  struct flchip *chips, struct cfi_private *cfi)
+			  unsigned long *chip_map, struct cfi_private *cfi)
 {
 	int i;
 	
@@ -78,18 +78,24 @@
 	}
 
 	/* Check each previous chip to see if it's an alias */
-	for (i=0; i<cfi->numchips; i++) {
+ 	for (i=0; i < (base >> cfi->chipshift); i++) {
+ 		unsigned long start;
+ 		if(!test_bit(i, chip_map)) {
+			/* Skip location; no valid chip at this address */
+ 			continue; 
+ 		}
+ 		start = i << cfi->chipshift;
 		/* This chip should be in read mode if it's one
 		   we've already touched. */
-		if (qry_present(map,chips[i].start,cfi)) {
+		if (qry_present(map, start, cfi)) {
 			/* Eep. This chip also had the QRY marker. 
 			 * Is it an alias for the new one? */
-			cfi_send_gen_cmd(0xF0, 0, chips[i].start, map, cfi, cfi->device_type, NULL);
+			cfi_send_gen_cmd(0xF0, 0, start, map, cfi, cfi->device_type, NULL);
 
 			/* If the QRY marker goes away, it's an alias */
-			if (!qry_present(map, chips[i].start, cfi)) {
+			if (!qry_present(map, start, cfi)) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 			/* Yes, it's actually got QRY for data. Most 
@@ -100,7 +106,7 @@
 			
 			if (qry_present(map, base, cfi)) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 		}
@@ -108,13 +114,7 @@
 	
 	/* OK, if we got to here, then none of the previous chips appear to
 	   be aliases for the current one. */
-	if (cfi->numchips == MAX_CFI_CHIPS) {
-		printk(KERN_WARNING"%s: Too many flash chips detected. Increase MAX_CFI_CHIPS from %d.\n", map->name, MAX_CFI_CHIPS);
-		/* Doesn't matter about resetting it to Read Mode - we're not going to talk to it anyway */
-		return -1;
-	}
-	chips[cfi->numchips].start = base;
-	chips[cfi->numchips].state = FL_READY;
+	set_bit((base >> cfi->chipshift), chip_map); /* Update chip map */
 	cfi->numchips++;
 	
 	/* Put it back into Read Mode */

Index: drivers/mtd/chips/gen_probe.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/gen_probe.c,v retrieving revision 1.13

mtd/chips/gen_probe.c +33 -21

95 lines not on this page · raw message

Index: drivers/mtd/chips/jedec_probe.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/jedec_probe.c,v retrieving revision 1.38

mtd/chips/jedec_probe.c +15 -18
diff -u -r1.38 jedec_probe.c
@@ -1416,7 +1416,7 @@
 static int cfi_jedec_setup(struct cfi_private *p_cfi, int index);
 
 static int jedec_probe_chip(struct map_info *map, __u32 base,
-			    struct flchip *chips, struct cfi_private *cfi);
+			    unsigned long *chip_map, struct cfi_private *cfi);
 
 struct mtd_info *jedec_probe(struct map_info *map);
 
@@ -1644,7 +1644,7 @@
 
 
 static int jedec_probe_chip(struct map_info *map, __u32 base,
-			      struct flchip *chips, struct cfi_private *cfi)
+			    unsigned long *chip_map, struct cfi_private *cfi)
 {
 	int i;
 	enum uaddr uaddr_idx = MTD_UADDR_NOT_SUPPORTED;
@@ -1731,21 +1731,24 @@
 		}
 	}
 	
-	/* Check each previous chip to see if it's an alias */
-	for (i=0; i<cfi->numchips; i++) {
-		/* This chip should be in read mode if it's one
-		   we've already touched. */
-		if (jedec_read_mfr(map, chips[i].start, cfi) == cfi->mfr &&
-		    jedec_read_id(map, chips[i].start, cfi) == cfi->id) {
+	/* Check each previous chip locations to see if it's an alias */
+	for (i=0; i < (base >> cfi->chipshift); i++) {
+		unsigned long start;
+		if(!test_bit(i, chip_map)) {
+			continue; /* Skip location; no valid chip at this address */
+		}
+		start = i << cfi->chipshift;
+		if (jedec_read_mfr(map, start, cfi) == cfi->mfr &&
+		    jedec_read_id(map, start, cfi) == cfi->id) {
 			/* Eep. This chip also looks like it's in autoselect mode.
 			   Is it an alias for the new one? */
-			jedec_reset(chips[i].start, map, cfi);
+			jedec_reset(start, map, cfi);
 
 			/* If the device IDs go away, it's an alias */
 			if (jedec_read_mfr(map, base, cfi) != cfi->mfr ||
 			    jedec_read_id(map, base, cfi) != cfi->id) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 			
@@ -1757,7 +1760,7 @@
 			if (jedec_read_mfr(map, base, cfi) == cfi->mfr &&
 			    jedec_read_id(map, base, cfi) == cfi->id) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 		}
@@ -1765,13 +1768,7 @@
 		
 	/* OK, if we got to here, then none of the previous chips appear to
 	   be aliases for the current one. */
-	if (cfi->numchips == MAX_CFI_CHIPS) {
-		printk(KERN_WARNING"%s: Too many flash chips detected. Increase MAX_CFI_CHIPS from %d.\n", map->name, MAX_CFI_CHIPS);
-		/* Doesn't matter about resetting it to Read Mode - we're not going to talk to it anyway */
-		return -1;
-	}
-	chips[cfi->numchips].start = base;
-	chips[cfi->numchips].state = FL_READY;
+	set_bit((base >> cfi->chipshift), chip_map); /* Update chip map */
 	cfi->numchips++;
 		
 ok_out:

Index: include/linux/mtd/cfi.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/mtd/cfi.h,v
retrieving revision 1.37

linux/mtd/cfi.h +0 -2
diff -u -r1.37 cfi.h
@@ -335,8 +335,6 @@
 	struct flchip chips[0];  /* per-chip data structure for each chip */
 };
 
-#define MAX_CFI_CHIPS 8 /* Entirely arbitrary to avoid realloc() */
-
 /*
  * Returns the command address according to the given geometry.
  */

Index: include/linux/mtd/gen_probe.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/mtd/gen_probe.h,v retrieving revision 1.1

linux/mtd/gen_probe.h +2 -2
diff -u -r1.1 gen_probe.h
@@ -10,12 +10,12 @@
 #include <linux/mtd/flashchip.h>
 #include <linux/mtd/map.h> 
 #include <linux/mtd/cfi.h>
+#include <asm/bitops.h>
 
 struct chip_probe {
 	char *name;
 	int (*probe_chip)(struct map_info *map, __u32 base,
-			  struct flchip *chips, struct cfi_private *cfi);
-
+			  unsigned long *chip_map, struct cfi_private *cfi);
 };
 
 struct mtd_info *mtd_do_chip_probe(struct map_info *map, struct chip_probe *cp);
Signature · 6 lines
Deepak Saxena - [email protected]

"To eliminate the concept of waste means to design things - products,
 packaging, and systems - from the very beggining on the understanding 
 that waste does not exist" - William McDonough & Michael Braungart,
                   from Cradle to Cradle: Remaking the Way We Make Things
#1 lore raw

Cytowanie Deepak Saxena <[email protected]>:

Quoted 9 lines

All,

The attached patch is an update of the patch I posted about a month ago [1] that removes the hardcoded CFI_MAX_CHIPS, which was originally written by Andrzej Mialkowski @ Intel. This patch fixes two issues:

- Get rid of extra structure probe_info that was just carrying around a long* variable.

This 'stupid' single field structure was intentional to allow easier expansion if somebody fill need for additional fields.

Quoted 6 lines

- Fix bug in bitmap size on systems with 2-8 chips. The old patch had:

  +	max_chips = map->size >> cfi.chipshift;
  +	probe.chip_map = kmalloc((max_chips + 7) / 8, GFP_KERNEL);
Quoted 7 lines

This would cause systems with 2-8 chips to be allocated only one byte for the bitmap instead of the 2 needed. Fixed code does:

+ max_chips = map->size >> cfi.chipshift;
+ chip_map = kmalloc((max_chips / 8) + ((max_chip % 8) ? 1 : 0),
GFP_KERNEL);

Why I need two bytes to store up to 8 bits? By the way 2/8 + 2?1:0 = 1. Initially I thought to put higher alignment here for instance ((max_chips +
31) / 32)*4) but found no reason to do this. Maybe I was wrong...

Andrzej