| Submitter | Zeus Castro |
|---|---|
| Date | 2011-04-12 19:55:58 |
| Message ID | <BANLkTim2KohMp6PwfsJXFqtgEJFNuOVnMA@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/2871/ |
| State | Accepted |
| Commit | r1415 |
| Headers | show |
Comments
Forgot: Signed-off-by: Zeus Castro <thezeusjuice@gmail.com> On Tue, Apr 12, 2011 at 3:55 PM, Zeus Castro <thezeusjuice@gmail.com> wrote: > Added support for SST25LF080A flash chip, based on public datasheet > available here: http://www.sst.com/dotAsset/40316.pdf >
On Tue, 12 Apr 2011 15:55:58 -0400 Zeus Castro <thezeusjuice@gmail.com> wrote: > Added support for SST25LF080A flash chip, based on public datasheet > available here: http://www.sst.com/dotAsset/40316.pdf hello <awesomely named guy> :) disclamer: this is my first patch review for flashrom. > Index: flashchips.h > -#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode */ > +#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode, same as SST25LF080A */ since the SST25VF080 is not implemented and that define is not used at all as far as i (and grep) can see, there is no reason to use the rems suffix imho. > Index: flashchips.c the whole block should be moved up (with the following SST25LF040A chip too). it is not alphabetically sorted. > =================================================================== > --- flashchips.c (revision 1285) > +++ flashchips.c (working copy) > @@ -5342,6 +5342,35 @@ > > { > .vendor = "SST", > + .name = "SST25LF080A.RES", i dont see a reason why you are using .RES here. according to the datasheet REMS work as well as RES. but maybe i dont know some detail. > + .bustype = CHIP_BUSTYPE_SPI, > + .manufacture_id = SST_ID, > + .model_id = SST_SST25VF080_REMS, > + .total_size = 1024, ok > + .page_size = 256, ok, because page_size on the whole is fucked up. :) > + .tested = TEST_UNTESTED, did you really not test it? why have you added it? > + .probe = probe_spi_res2, ok, or probe_spi_rems > + .probe_timing = TIMING_ZERO, > + .block_erasers = > + { > + { > + .eraseblocks = { {4 * 1024, 256} }, > + .block_erase = spi_block_erase_20, > + }, { > + .eraseblocks = { {32 * 1024, 32} }, > + .block_erase = spi_block_erase_52, > + }, { > + .eraseblocks = { {1024 * 1024, 1} }, > + .block_erase = spi_block_erase_60, > + }, > + }, correct > + .unlock = spi_disable_blockprotect, this will not work if bit 7 (BPL) is set some of the at25* write protections are similar. one might use them. > + .write = spi_chip_write_1, > + .read = spi_chip_read, > + }, > + > + { > + .vendor = "SST", > .name = "SST25LF040A.RES", > .bustype = CHIP_BUSTYPE_SPI, > .manufacture_id = SST_ID, ok
Hello. > disclamer: this is my first patch review for flashrom. disclamer: this is my first patch ever :-) Also, although I'm a decent programmer, my knowledge of C is pretty limited. Based on the datasheets of both, the SST25LF080A is basically a larger version of the SST25LF040A. However, flashrom had an entry for SST25LF040A, but not for SST25LF080A. As such, I modeled it after the entry for SST25LF040A, and altered the values that differed. >> + .name = "SST25LF080A.RES", > i dont see a reason why you are using .RES here. > according to the datasheet REMS work as well as RES. > but maybe i dont know some detail. > >> + .probe = probe_spi_res2, > ok, or probe_spi_rems > I was confused as well as to why the oscillation between REMS, RES, and RES2 in the SST25LF040A entry, but figured that there was probably a reason I did not understand, and copied it over in the same manner. So, you recommend I change it all to REMS? >> Index: flashchips.h > >> -#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode */ >> +#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode, same as SST25LF080A */ > > since the SST25VF080 is not implemented and that define is not used at > all as far as i (and grep) can see, there is no reason to use the rems > suffix imho. > Apparently SST25VFxxx and SST25LFxxx chips often share the same REMS/RES device id. And since the one for SST25LF040A was "tacked on" to the define for SST25VF040_REMS (since they use the same byte), and there was an already existing define for SST25VF080_REMS (which uses the same byte as I was planning to include for the SST25LF080A), I just "tacked on" the SST25LF080A in the same manner. So, you recommend I change it to read?: #define SST_SST25LF080A 0x80" >> + .tested = TEST_UNTESTED, > did you really not test it? why have you added it? > The only sample I have is currently inside my laptop, and flashrom access to it is being blocked by the EC/KBC (an ENE KB926). Besides, there are about 170 untested chips in flashchips.c, so I figured one more, especially based on an incremental upgrade to a probe-tested chip (SST25LF040A), wouldn't be problematic. > >> + .unlock = spi_disable_blockprotect, > this will not work if bit 7 (BPL) is set > some of the at25* write protections are similar. one might use them. > spi_disable_blockprotect is sufficient for this chip: The important bits are 2-5 (although this chip only cares about 2 and 3). If those bits are 0, the chip isn't protected. If there is protection, spi_disable_blockprotect correctly attempts to set them to 0, and checks if it succeeded. The only way it can fail is if bit 7 is set and the Write Protect Pin (WP#) is being pulled low, in which case there is nothing that can be done (block protection cannot be disabled without reseting the chip) and spi_disable_blockprotect returns 1. No additional functionality is possible. Best regards, Zeus Castro
On Wed, 13 Apr 2011 14:11:00 -0400 Zeus Castro <thezeusjuice@gmail.com> wrote: > > disclamer: this is my first patch review for flashrom. > disclamer: this is my first patch ever :-) > Also, although I'm a decent programmer, my knowledge of C is pretty > limited. > > Based on the datasheets of both, the SST25LF080A is basically a larger > version of the SST25LF040A. > However, flashrom had an entry for SST25LF040A, but not for > SST25LF080A. As such, I modeled it after the entry for SST25LF040A, > and altered the values that differed. thought so. that's what i and probably all without better insights usually do. my comments were mostly meant to show what i think is the right thing to do, not in your patch alone but on the whole in the code related to it. that's probably the wrong stand when one reviews a concrete patch, but it feels natural and the reviews i received were similar. in this case my patch would probably be very similar to yours. with the exception of the .RES _REMS suffices: > >> + .name = "SST25LF080A.RES", > > i dont see a reason why you are using .RES here. > > according to the datasheet REMS work as well as RES. > > but maybe i dont know some detail. > > > >> + .probe = probe_spi_res2, > > ok, or probe_spi_rems > > > I was confused as well as to why the oscillation between REMS, RES, > and RES2 in the SST25LF040A entry, but figured that there was probably > a reason I did not understand, and copied it over in the same manner. > So, you recommend I change it all to REMS? i would remove the .RES from the name, the probe method is ok. i dont understand it either, maybe someone else can clear that up. > >> Index: flashchips.h > > > >> -#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode */ > >> +#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode, same > >> as SST25LF080A */ > > > > since the SST25VF080 is not implemented and that define is not used > > at all as far as i (and grep) can see, there is no reason to use > > the rems suffix imho. > > > Apparently SST25VFxxx and SST25LFxxx chips often share the same > REMS/RES device id. And since the one for SST25LF040A was "tacked on" > to the define for SST25VF040_REMS (since they use the same byte), and > there was an already existing define for SST25VF080_REMS (which uses > the same byte as I was planning to include for the SST25LF080A), I > just "tacked on" the SST25LF080A in the same manner. So, you recommend > I change it to read?: > #define SST_SST25LF080A 0x80" i dont like the _REMS suffix, just like above. the "same as xyz"-strategy is used all over in flashrom.h and is ok. > >> + .tested = TEST_UNTESTED, > > did you really not test it? why have you added it? > > > The only sample I have is currently inside my laptop, and flashrom > access to it is being blocked by the EC/KBC (an ENE KB926). > Besides, there are about 170 untested chips in flashchips.c, so I > figured one more, especially based on an incremental upgrade to a > probe-tested chip (SST25LF040A), wouldn't be problematic. no problem at all, i just wanted to know. > >> + .unlock = spi_disable_blockprotect, > > this will not work if bit 7 (BPL) is set > > some of the at25* write protections are similar. one might use them. > > > spi_disable_blockprotect is sufficient for this chip: > The important bits are 2-5 (although this chip only cares about 2 and > 3). If those bits are 0, the chip isn't protected. > If there is protection, spi_disable_blockprotect correctly attempts to > set them to 0, and checks if it succeeded. > The only way it can fail is if bit 7 is set and the Write Protect Pin > (WP#) is being pulled low, in which case there is nothing that can be > done (block protection cannot be disabled without reseting the chip) > and spi_disable_blockprotect returns 1. > No additional functionality is possible. > correct. i missed the point, that WRSR is not possible when BPL is set. all in all it looks ok, thanks for your effort! due to the rems/res stuff someone else needs to take a look (i dont have commit rights anyway).
On Wed, 13 Apr 2011 21:04:38 +0200 Stefan Tauner <stefan.tauner@student.tuwien.ac.at> wrote: > On Wed, 13 Apr 2011 14:11:00 -0400 > Zeus Castro <thezeusjuice@gmail.com> wrote: > > > > > disclamer: this is my first patch review for flashrom. > > disclamer: this is my first patch ever :-) > > Also, although I'm a decent programmer, my knowledge of C is pretty > > limited. > > > > Based on the datasheets of both, the SST25LF080A is basically a larger > > version of the SST25LF040A. > > However, flashrom had an entry for SST25LF040A, but not for > > SST25LF080A. As such, I modeled it after the entry for SST25LF040A, > > and altered the values that differed. > thought so. that's what i and probably all without better insights > usually do. my comments were mostly meant to show what i think is the > right thing to do, not in your patch alone but on the whole in the code > related to it. that's probably the wrong stand when one reviews a > concrete patch, but it feels natural and the reviews i received were > similar. in this case my patch would probably be very similar to yours. > with the exception of the .RES _REMS suffices: > > > >> + .name = "SST25LF080A.RES", > > > i dont see a reason why you are using .RES here. > > > according to the datasheet REMS work as well as RES. > > > but maybe i dont know some detail. > > > > > >> + .probe = probe_spi_res2, > > > ok, or probe_spi_rems > > > > > I was confused as well as to why the oscillation between REMS, RES, > > and RES2 in the SST25LF040A entry, but figured that there was probably > > a reason I did not understand, and copied it over in the same manner. > > So, you recommend I change it all to REMS? > i would remove the .RES from the name, the probe method is ok. > i dont understand it either, maybe someone else can clear that up. > > > >> Index: flashchips.h > > > > > >> -#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode */ > > >> +#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode, same > > >> as SST25LF080A */ > > > > > > since the SST25VF080 is not implemented and that define is not used > > > at all as far as i (and grep) can see, there is no reason to use > > > the rems suffix imho. > > > > > Apparently SST25VFxxx and SST25LFxxx chips often share the same > > REMS/RES device id. And since the one for SST25LF040A was "tacked on" > > to the define for SST25VF040_REMS (since they use the same byte), and > > there was an already existing define for SST25VF080_REMS (which uses > > the same byte as I was planning to include for the SST25LF080A), I > > just "tacked on" the SST25LF080A in the same manner. So, you recommend > > I change it to read?: > > #define SST_SST25LF080A 0x80" > i dont like the _REMS suffix, just like above. > the "same as xyz"-strategy is used all over in flashrom.h and is ok. > > > >> + .tested = TEST_UNTESTED, > > > did you really not test it? why have you added it? > > > > > The only sample I have is currently inside my laptop, and flashrom > > access to it is being blocked by the EC/KBC (an ENE KB926). > > Besides, there are about 170 untested chips in flashchips.c, so I > > figured one more, especially based on an incremental upgrade to a > > probe-tested chip (SST25LF040A), wouldn't be problematic. > no problem at all, i just wanted to know. > > > >> + .unlock = spi_disable_blockprotect, > > > this will not work if bit 7 (BPL) is set > > > some of the at25* write protections are similar. one might use them. > > > > > spi_disable_blockprotect is sufficient for this chip: > > The important bits are 2-5 (although this chip only cares about 2 and > > 3). If those bits are 0, the chip isn't protected. > > If there is protection, spi_disable_blockprotect correctly attempts to > > set them to 0, and checks if it succeeded. > > The only way it can fail is if bit 7 is set and the Write Protect Pin > > (WP#) is being pulled low, in which case there is nothing that can be > > done (block protection cannot be disabled without reseting the chip) > > and spi_disable_blockprotect returns 1. > > No additional functionality is possible. > > > correct. i missed the point, that WRSR is not possible when BPL is set. > > all in all it looks ok, thanks for your effort! > due to the rems/res stuff someone else needs to take a look (i dont > have commit rights anyway). > i have sorted out the naming problems, moved the definition a bit up and committed in r1415. thank you!
Patch
Index: flashchips.c =================================================================== --- flashchips.c (revision 1285) +++ flashchips.c (working copy) @@ -5342,6 +5342,35 @@ { .vendor = "SST", + .name = "SST25LF080A.RES", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = SST_ID, + .model_id = SST_SST25VF080_REMS, + .total_size = 1024, + .page_size = 256, + .tested = TEST_UNTESTED, + .probe = probe_spi_res2, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 256} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 32} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, + }, + .unlock = spi_disable_blockprotect, + .write = spi_chip_write_1, + .read = spi_chip_read, + }, + + { + .vendor = "SST", .name = "SST25LF040A.RES", .bustype = CHIP_BUSTYPE_SPI, .manufacture_id = SST_ID, Index: flashchips.h =================================================================== --- flashchips.h (revision 1285) +++ flashchips.h (working copy) @@ -468,7 +468,7 @@ #define SST_SST25VF040_REMS 0x44 /* REMS or RES opcode, same as SST25LF040A */ #define SST_SST25VF040B 0x258D #define SST_SST25VF040B_REMS 0x8D /* REMS or RES opcode */ -#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode */ +#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode, same as SST25LF080A */ #define SST_SST25VF080B 0x258E #define SST_SST25VF080B_REMS 0x8E /* REMS or RES opcode */ #define SST_SST25VF016B 0x2541