Patchworkβ Refactor remaining write wrappers

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2010-10-11 12:35:58
Message ID <4CB304AE.7000206@gmx.net>
Download mbox | patch
Permalink /patch/2091/
State Accepted
Commit r1210
Headers show

Comments

Carl-Daniel Hailfinger - 2010-10-11 12:35:58
On 10.10.2010 15:19, Uwe Hermann wrote:
> On Sat, Oct 09, 2010 at 11:59:24PM +0200, Carl-Daniel Hailfinger wrote:
>   
>> Refactor remaining write wrappers.
>> Kill duplicated code.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
>>     
>
> Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
>   

Thanks for the review. Here is an extended patch which has some more
refactoring to make the switchover to partial write less painful. Parts
have already been reviewed by Uwe.

If possible, please test this code on all chip classes, one per write
function listed below.
- write_82802ab()
- write_jedec()
- write_jedec_1()
- write_m29f400bt()
- write_28sf040()
Testing of SPI chips is not needed. They are not touched in this patch.

Refactor remaining write wrappers.
Kill duplicated code.
Annotate write functions with their chunk size.
Mark Fujitsu MBM29F400BC and ST M29F400BB as untested because their
write code no longer uses a broken layout.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Maciej Pijanka - 2010-10-11 13:12:26
On Mon, 11 Oct 2010, Carl-Daniel Hailfinger wrote:

> On 10.10.2010 15:19, Uwe Hermann wrote:
> > On Sat, Oct 09, 2010 at 11:59:24PM +0200, Carl-Daniel Hailfinger wrote:
> >   
> >> Refactor remaining write wrappers.
> >> Kill duplicated code.
> >>
> >> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
> >>     
> >
> > Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
> >   
> 
> Thanks for the review. Here is an extended patch which has some more
> refactoring to make the switchover to partial write less painful. Parts
> have already been reviewed by Uwe.
> 
> If possible, please test this code on all chip classes, one per write
> function listed below.
> - write_82802ab()
> - write_jedec()
> - write_jedec_1()
> - write_m29f400bt()
> - write_28sf040()
> Testing of SPI chips is not needed. They are not touched in this patch.
> 
> Refactor remaining write wrappers.
> Kill duplicated code.
> Annotate write functions with their chunk size.
> Mark Fujitsu MBM29F400BC and ST M29F400BB as untested because their
> write code no longer uses a broken layout.
> 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

I did only testing with this patch applied on nic3com programmer, and at29c512 chip
works without any problem here.
No problems with compilation or applying over r1209

Maciej
Idwer Vollering - 2010-10-12 13:32:58
2010/10/11 Maciej Pijanka <maciej.pijanka@gmail.com>

> On Mon, 11 Oct 2010, Carl-Daniel Hailfinger wrote:
>
> > On 10.10.2010 15:19, Uwe Hermann wrote:
> > > On Sat, Oct 09, 2010 at 11:59:24PM +0200, Carl-Daniel Hailfinger wrote:
> > >
> > >> Refactor remaining write wrappers.
> > >> Kill duplicated code.
> > >>
> > >> Signed-off-by: Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006@gmx.net>
> > >>
> > >
> > > Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
>

Acked-by: Idwer Vollering <vidwer@gmail.com>


> > >
> >
> > Thanks for the review. Here is an extended patch which has some more
> > refactoring to make the switchover to partial write less painful. Parts
> > have already been reviewed by Uwe.
> >
> > If possible, please test this code on all chip classes, one per write
> > function listed below.
> > - write_82802ab()
> > - write_jedec()
> > - write_jedec_1()
> > - write_m29f400bt()
> > - write_28sf040()
> > Testing of SPI chips is not needed. They are not touched in this patch.
> >
> > Refactor remaining write wrappers.
> > Kill duplicated code.
> > Annotate write functions with their chunk size.
> > Mark Fujitsu MBM29F400BC and ST M29F400BB as untested because their
> > write code no longer uses a broken layout.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net
> >
>
> I did only testing with this patch applied on nic3com programmer, and
> at29c512 chip
> works without any problem here.
> No problems with compilation or applying over r1209
>
> Maciej
>
> --
> Maciej Pijanka
> I don't fear computers, I fear lack of them -- Isaac Asimov
>
> _______________________________________________
> flashrom mailing list
> flashrom@flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom
>
Sean Nelson - 2010-10-13 18:09:04
On 10/11/10 5:35 AM, Carl-Daniel Hailfinger wrote:
> On 10.10.2010 15:19, Uwe Hermann wrote:
>    
>> On Sat, Oct 09, 2010 at 11:59:24PM +0200, Carl-Daniel Hailfinger wrote:
>>
>>      
>>> Refactor remaining write wrappers.
>>> Kill duplicated code.
>>>
>>> Signed-off-by: Carl-Daniel Hailfinger<c-d.hailfinger.devel.2006@gmx.net>
>>>
>>>        
>> Acked-by: Uwe Hermann<uwe@hermann-uwe.de>
>>
>>      
> Thanks for the review. Here is an extended patch which has some more
> refactoring to make the switchover to partial write less painful. Parts
> have already been reviewed by Uwe.
>
> If possible, please test this code on all chip classes, one per write
> function listed below.
> - write_82802ab()
> - write_jedec()
> - write_jedec_1()
> - write_m29f400bt()
> - write_28sf040()
> Testing of SPI chips is not needed. They are not touched in this patch.
>
> Refactor remaining write wrappers.
> Kill duplicated code.
> Annotate write functions with their chunk size.
> Mark Fujitsu MBM29F400BC and ST M29F400BB as untested because their
> write code no longer uses a broken layout.
>
> Signed-off-by: Carl-Daniel Hailfinger<c-d.hailfinger.devel.2006@gmx.net>
>    
Tested on hardware.
Acked-by: Sean Nelson <audiohacked@gmail.com>
Carl-Daniel Hailfinger - 2010-10-13 21:49:49
On 13.10.2010 20:09, Sean Nelson wrote:
> On 10/11/10 5:35 AM, Carl-Daniel Hailfinger wrote:
>> Refactor remaining write wrappers.
>> Kill duplicated code.
>> Annotate write functions with their chunk size.
>> Mark Fujitsu MBM29F400BC and ST M29F400BB as untested because their
>> write code no longer uses a broken layout.
>>
>> Signed-off-by: Carl-Daniel Hailfinger<c-d.hailfinger.devel.2006@gmx.net>
>>    
> Tested on hardware.
> Acked-by: Sean Nelson <audiohacked@gmail.com>

Thanks to everyone for the reviews and tests.
Committed in r1210.

Regards,
Carl-Daniel
David Hendricks - 2010-10-13 22:25:02
On Wed, Oct 13, 2010 at 2:49 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006@gmx.net> wrote:

> On 13.10.2010 20:09, Sean Nelson wrote:
> > On 10/11/10 5:35 AM, Carl-Daniel Hailfinger wrote:
> >> Refactor remaining write wrappers.
> >> Kill duplicated code.
> >> Annotate write functions with their chunk size.
> >> Mark Fujitsu MBM29F400BC and ST M29F400BB as untested because their
> >> write code no longer uses a broken layout.
> >>
> >> Signed-off-by: Carl-Daniel Hailfinger<c-d.hailfinger.devel.2006@gmx.net
> >
> >>
> > Tested on hardware.
> > Acked-by: Sean Nelson <audiohacked@gmail.com>
>
> Thanks to everyone for the reviews and tests.
> Committed in r1210.
>

Also tested using a w39v080 and sst49lf040 :-)

(better late than never...)

Patch

Index: flashrom-partial_write_inner_function_cleanup_annotate/jedec.c
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/jedec.c	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/jedec.c	(Arbeitskopie)
@@ -92,6 +92,25 @@ 
 		msg_cdbg("%s: excessive loops, i=0x%x\n", __func__, i);
 }
 
+static int getaddrmask(struct flashchip *flash)
+{
+	switch (flash->feature_bits & FEATURE_ADDR_MASK) {
+	case FEATURE_ADDR_FULL:
+		return MASK_FULL;
+		break;
+	case FEATURE_ADDR_2AA:
+		return MASK_2AA;
+		break;
+	case FEATURE_ADDR_AAA:
+		return MASK_AAA;
+		break;
+	default:
+		msg_cerr("%s called with unknown mask\n", __func__);
+		return 0;
+		break;
+	}
+}
+
 static void start_program_jedec_common(struct flashchip *flash, unsigned int mask)
 {
 	chipaddr bios = flash->virtual_memory;
@@ -317,12 +336,15 @@ 
 	return failed;
 }
 
-int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, int start, int len, unsigned int mask)
+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, int start, int len)
 {
 	int i, failed = 0;
 	chipaddr dst = flash->virtual_memory + start;
 	chipaddr olddst;
+	int mask;
 
+	mask = getaddrmask(flash);
+
 	olddst = dst;
 	for (i = 0; i < len; i++) {
 		if (write_byte_program_jedec_common(flash, src, dst, mask))
@@ -335,14 +357,17 @@ 
 	return failed;
 }
 
-int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src, int start, int page_size, unsigned int mask)
+int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src, int start, int page_size)
 {
 	int i, tried = 0, failed;
 	uint8_t *s = src;
 	chipaddr bios = flash->virtual_memory;
 	chipaddr dst = bios + start;
 	chipaddr d = dst;
+	int mask;
 
+	mask = getaddrmask(flash);
+
 retry:
 	/* Issue JEDEC Start Program command */
 	start_program_jedec_common(flash, mask);
@@ -373,49 +398,55 @@ 
 	return failed;
 }
 
-static int getaddrmask(struct flashchip *flash)
+/*
+ * Write a part of the flash chip.
+ * FIXME: Use the chunk code from Michael Karcher instead.
+ * This function is a slightly modified copy of spi_write_chunked.
+ * Each page is written separately in chunks with a maximum size of chunksize.
+ */
+int write_jedec_pages(struct flashchip *flash, uint8_t *buf, int start, int len)
 {
-	switch (flash->feature_bits & FEATURE_ADDR_MASK) {
-	case FEATURE_ADDR_FULL:
-		return MASK_FULL;
-		break;
-	case FEATURE_ADDR_2AA:
-		return MASK_2AA;
-		break;
-	case FEATURE_ADDR_AAA:
-		return MASK_AAA;
-		break;
-	default:
-		msg_cerr("%s called with unknown mask\n", __func__);
-		return 0;
-		break;
-	}
-}
-
-int write_jedec(struct flashchip *flash, uint8_t *buf)
-{
-	int mask;
-	int i, failed = 0;
-	int total_size = flash->total_size * 1024;
+	int i, starthere, lenhere;
+	/* FIXME: page_size is the wrong variable. We need max_writechunk_size
+	 * in struct flashchip to do this properly. All chips using
+	 * write_jedec have page_size set to max_writechunk_size, so
+	 * we're OK for now.
+	 */
 	int page_size = flash->page_size;
 
-	mask = getaddrmask(flash);
+	/* Warning: This loop has a very unusual condition and body.
+	 * The loop needs to go through each page with at least one affected
+	 * byte. The lowest page number is (start / page_size) since that
+	 * division rounds down. The highest page number we want is the page
+	 * where the last byte of the range lives. That last byte has the
+	 * address (start + len - 1), thus the highest page number is
+	 * (start + len - 1) / page_size. Since we want to include that last
+	 * page as well, the loop condition uses <=.
+	 */
+	for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
+		/* Byte position of the first byte in the range in this page. */
+		/* starthere is an offset to the base address of the chip. */
+		starthere = max(start, i * page_size);
+		/* Length of bytes in the range in this page. */
+		lenhere = min(start + len, (i + 1) * page_size) - starthere;
 
-	for (i = 0; i < total_size / page_size; i++) {
-		if (write_page_write_jedec_common(flash, buf + i * page_size, i * page_size, page_size, mask))
-			failed = 1;
+		if (write_page_write_jedec_common(flash, buf + starthere - start, starthere, lenhere))
+			return 1;
 	}
 
-	return failed;
+	return 0;
 }
 
+/* chunksize is page_size */
+int write_jedec(struct flashchip *flash, uint8_t *buf)
+{
+	return write_jedec_pages(flash, buf, 0, flash->total_size * 1024);
+}
+
+/* chunksize is 1 */
 int write_jedec_1(struct flashchip *flash, uint8_t * buf)
 {
-	int mask;
-
-	mask = getaddrmask(flash);
-
-	return write_sector_jedec_common(flash, buf, 0, flash->total_size * 1024, mask);
+	return write_sector_jedec_common(flash, buf, 0, flash->total_size * 1024);
 }
 
 /* erase chip with block_erase() prototype */
Index: flashrom-partial_write_inner_function_cleanup_annotate/sst49lfxxxc.c
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/sst49lfxxxc.c	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/sst49lfxxxc.c	(Arbeitskopie)
@@ -75,15 +75,3 @@ 
 	}
 	return 0;
 }
-
-int write_49lfxxxc(struct flashchip *flash, uint8_t *buf)
-{
-	chipaddr bios = flash->virtual_memory;
-
-	write_lockbits_49lfxxxc(flash, 0);
-	write_page_82802ab(flash, buf, 0, flash->total_size * 1024);
-
-	chip_writeb(0xFF, bios);
-
-	return 0;
-}
Index: flashrom-partial_write_inner_function_cleanup_annotate/82802ab.c
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/82802ab.c	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/82802ab.c	(Arbeitskopie)
@@ -160,6 +160,7 @@ 
 	return 0;
 }
 
+/* chunksize is 1 */
 int write_82802ab(struct flashchip *flash, uint8_t *buf)
 {
 	return write_page_82802ab(flash, buf, 0, flash->total_size * 1024);
Index: flashrom-partial_write_inner_function_cleanup_annotate/flashchips.c
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/flashchips.c	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/flashchips.c	(Arbeitskopie)
@@ -2995,7 +2995,7 @@ 
 		.total_size	= 512,
 		.page_size	= 64 * 1024,
 		.feature_bits	= FEATURE_ADDR_SHIFTED | FEATURE_EITHER_RESET,
-		.tested		= TEST_BAD_WRITE, /* Implicit eraseblock layout in write_m29f400bt is broken. */
+		.tested		= TEST_UNTESTED,
 		.probe		= probe_m29f400bt,
 		.probe_timing	= TIMING_IGNORED, /* routine don't use probe_timing (m29f400bt.c) */
 		.block_erasers	=
@@ -3013,7 +3013,7 @@ 
 				.block_erase = block_erase_chip_m29f400bt,
 			},
 		},
-		.write		= NULL,
+		.write		= write_m29f400bt,
 		.read		= read_memmapped,
 	},
 
@@ -5107,6 +5107,7 @@ 
 				.block_erase = erase_chip_28sf040,
 			}
 		},
+		.unlock		= unprotect_28sf040,
 		.write		= write_28sf040,
 		.read		= read_memmapped,
 	},
@@ -5564,7 +5565,7 @@ 
 			}
 		},
 		.unlock		= unlock_49lfxxxc,
-		.write		= write_49lfxxxc,
+		.write		= write_82802ab,
 		.read		= read_memmapped,
 	},
 
@@ -5627,7 +5628,7 @@ 
 			}
 		},
 		.unlock		= unlock_49lfxxxc,
-		.write		= write_49lfxxxc,
+		.write		= write_82802ab,
 		.read		= read_memmapped,
 	},
 
@@ -5659,7 +5660,7 @@ 
 			}
 		},
 		.unlock		= unlock_49lfxxxc,
-		.write		= write_49lfxxxc,
+		.write		= write_82802ab,
 		.read		= read_memmapped,
 	},
 
@@ -5837,7 +5838,7 @@ 
 			}
 		},
 		.unlock		= unlock_49lfxxxc,
-		.write		= write_49lfxxxc,
+		.write		= write_82802ab,
 		.read		= read_memmapped,
 	},
 
@@ -6315,7 +6316,7 @@ 
 		.total_size	= 512,
 		.page_size	= 64 * 1024,
 		.feature_bits	= FEATURE_ADDR_SHIFTED | FEATURE_EITHER_RESET,
-		.tested		= TEST_BAD_WRITE, /* Implicit eraseblock layout in write_m29f400bt is broken. */
+		.tested		= TEST_UNTESTED,
 		.probe		= probe_m29f400bt,
 		.probe_timing	= TIMING_IGNORED, /* routine doesn't use probe_timing (m29f400bt.c) */
 		.block_erasers	=
@@ -6333,7 +6334,7 @@ 
 				.block_erase = block_erase_chip_m29f400bt,
 			}
 		},
-		.write		= NULL,
+		.write		= write_m29f400bt,
 		.read		= read_memmapped,
 	},
 	{
Index: flashrom-partial_write_inner_function_cleanup_annotate/sst28sf040.c
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/sst28sf040.c	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/sst28sf040.c	(Arbeitskopie)
@@ -30,7 +30,7 @@ 
 #define RESET			0xFF
 #define READ_ID			0x90
 
-static void protect_28sf040(struct flashchip *flash)
+int protect_28sf040(struct flashchip *flash)
 {
 	chipaddr bios = flash->virtual_memory;
 
@@ -41,9 +41,11 @@ 
 	chip_readb(bios + 0x041B);
 	chip_readb(bios + 0x0419);
 	chip_readb(bios + 0x040A);
+
+	return 0;
 }
 
-static void unprotect_28sf040(struct flashchip *flash)
+int unprotect_28sf040(struct flashchip *flash)
 {
 	chipaddr bios = flash->virtual_memory;
 
@@ -54,12 +56,15 @@ 
 	chip_readb(bios + 0x041B);
 	chip_readb(bios + 0x0419);
 	chip_readb(bios + 0x041A);
+
+	return 0;
 }
 
 int erase_sector_28sf040(struct flashchip *flash, unsigned int address, unsigned int sector_size)
 {
 	chipaddr bios = flash->virtual_memory;
 
+	/* This command sequence is very similar to erase_block_82802ab. */
 	chip_writeb(AUTO_PG_ERASE1, bios);
 	chip_writeb(AUTO_PG_ERASE2, bios + address);
 
@@ -101,10 +106,8 @@ 
 {
 	chipaddr bios = flash->virtual_memory;
 
-	unprotect_28sf040(flash);
 	chip_writeb(CHIP_ERASE, bios);
 	chip_writeb(CHIP_ERASE, bios);
-	protect_28sf040(flash);
 
 	programmer_delay(10);
 	toggle_ready_jedec(bios);
@@ -116,15 +119,10 @@ 
 	return 0;
 }
 
+/* chunksize is 1 */
 int write_28sf040(struct flashchip *flash, uint8_t *buf)
 {
-	unprotect_28sf040(flash);
-
-	write_sector_28sf040(flash, buf, 0, flash->total_size * 1024);
-
-	protect_28sf040(flash);
-
-	return 0;
+	return write_sector_28sf040(flash, buf, 0, flash->total_size * 1024);
 }
 
 int erase_chip_28sf040(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
Index: flashrom-partial_write_inner_function_cleanup_annotate/m29f400bt.c
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/m29f400bt.c	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/m29f400bt.c	(Arbeitskopie)
@@ -140,6 +140,7 @@ 
 	return erase_m29f400bt(flash);
 }
 
+/* chunksize is 1 */
 int write_m29f400bt(struct flashchip *flash, uint8_t *buf)
 {
 	return write_page_m29f400bt(flash, buf, 0, flash->total_size * 1024);
Index: flashrom-partial_write_inner_function_cleanup_annotate/chipdrivers.h
===================================================================
--- flashrom-partial_write_inner_function_cleanup_annotate/chipdrivers.h	(Revision 1209)
+++ flashrom-partial_write_inner_function_cleanup_annotate/chipdrivers.h	(Arbeitskopie)
@@ -86,8 +86,8 @@ 
 int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize);
 int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize);
 int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize);
-int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, int start, int len, unsigned int mask);
-int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src, int start, int page_size, unsigned int mask);
+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, int start, int len);
+int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src, int start, int page_size);
 
 /* m29f400bt.c */
 int probe_m29f400bt(struct flashchip *flash);
@@ -106,10 +106,11 @@ 
 int erase_sector_28sf040(struct flashchip *flash, unsigned int address, unsigned int sector_size);
 int write_28sf040(struct flashchip *flash, uint8_t *buf);
 int write_sector_28sf040(struct flashchip *flash, uint8_t *src, int start, int len);
+int unprotect_28sf040(struct flashchip *flash);
+int protect_28sf040(struct flashchip *flash);
 
 /* sst49lfxxxc.c */
 int erase_sector_49lfxxxc(struct flashchip *flash, unsigned int address, unsigned int sector_size);
-int write_49lfxxxc(struct flashchip *flash, uint8_t *buf);
 int unlock_49lfxxxc(struct flashchip *flash);
 
 /* sst_fwhub.c */