[Patch] Added SST25LF080A Flash Chip

[Zeus Castro]

Added support for SST25LF080A flash chip, based on public datasheet
available here: http://www.sst.com/dotAsset/40316.pdf

[Zeus Castro]

Forgot: Signed-off-by: Zeus Castro

On Tue, Apr 12, 2011 at 3:55 PM, Zeus Castro  wrote:
> Added support for SST25LF080A flash chip, based on public datasheet
> available here: http://www.sst.com/dotAsset/40316.pdf

[Stefan Tauner]

On Tue, 12 Apr 2011 15:55:58 -0400
Zeus Castro  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

[Zeus Castro]

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

[Stefan Tauner]

On Wed, 13 Apr 2011 14:11:00 -0400
Zeus Castro  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).

[Stefan Tauner]

On Wed, 13 Apr 2011 21:04:38 +0200
Stefan Tauner  wrote:
> On Wed, 13 Apr 2011 14:11:00 -0400
> Zeus Castro  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!