Re: [flashrom] [PATCH] Add support for S25FL128P, S25FL129P and refine it for S25FL128S chips.

[Antonio Ospite]

On Mon,  4 Aug 2014 00:46:55 +0200
Stefan Tauner  wrote:
> Additionally to the existing S25FL128S......0 definition this patch
> adds S25FL128P......0, S25FL128P......1 and S25FL128S......1, as well as
> S25FL129P......0 and S25FL129P......1 definitions.
> S25FL12xP seem to be the predecessor families of S25FL128S. All
> associated chips can not be distinguished with RDID alone.
>
> Besides the new chips, this patch also fixes the name of the  previously
> supported S25FL128S model with uniform 256 kB sectors
> (S25FL128P......1 not 0) and adds the hybrid sector version (0) as well.
>
> Due to the shared IDs the user has to select the right chip manually
> with the -c parameter. To make this even possible, this patch enlarges
> the respective array for results to 6.

Hi Stefan, the chip I've got is a S25FL129P......0, logs attached and
some comments inlined below.

Thanks.
> Signed-off-by: Stefan Tauner

Tested-by: Antonio Ospite
> cli_classic.c |   2 +-
>  flashchips.c  | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  flashchips.h  |   2 +-
>  3 files changed, 181 insertions(+), 5 deletions(-)
>
> diff --git a/cli_classic.c b/cli_classic.c
> index ee988a3..60b8b88 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -94,7 +94,7 @@ int main(int argc, char *argv[])
>    unsigned long size;
>    /* Probe for up to three flash chips. */
>    const struct flashchip *chip = NULL;
> struct flashctx flashes[3] = {{0}};
> struct flashctx flashes[6] = {{0}};
> struct flashctx *fill_flash;
> const char *name;
> int namelen, opt, i, j;
>
> diff --git a/flashchips.c b/flashchips.c
> index a1166e0..e587b28 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -10389,7 +10389,69 @@ const struct flashchip flashchips[] = {
>
> {
>    	.vendor		= "Spansion",
> .name		= "S25FL128S......0", /* uniform 256kB sectors */
> .name		= "S25FL128P......0", /* uniform 64 kB sectors */
> .bustype	= BUS_SPI,
> .manufacture_id	= SPANSION_ID,
> .model_id	= SPANSION_S25FL128,
> .total_size	= 16384,
> .page_size	= 256,
> .feature_bits	= FEATURE_WRSR_WREN,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	= {
> 	{
> 		.eraseblocks = { {64 * 1024, 256} },
> 		.block_erase = spi_block_erase_20,
> 	}, {
> 		.eraseblocks = { {64 * 1024, 256} },
> 		.block_erase = spi_block_erase_d8,
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_60,
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_c7,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_bp3_srwd,
> .unlock		= spi_disable_blockprotect_bp3_srwd,
> .write		= spi_chip_write_256,
> .read		= spi_chip_read, /* Fast read (0x0B) supported */
> .voltage	= {2700, 3600},
> },
> {
> .vendor		= "Spansion",
> .name		= "S25FL128P......1", /* uniform 256kB sectors */
> .bustype	= BUS_SPI,
> .manufacture_id	= SPANSION_ID,
> .model_id	= SPANSION_S25FL128,
> .total_size	= 16384,
> .page_size	= 256,
> .feature_bits	= FEATURE_WRSR_WREN,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	= {
> 	{
> 		.eraseblocks = { {256 * 1024, 64} },
> 		.block_erase = spi_block_erase_d8,
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_c7,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_bp2_srwd,
> .unlock		= spi_disable_blockprotect_bp2_srwd,
> .write		= spi_chip_write_256,
> .read		= spi_chip_read, /* Fast read (0x0B) supported */
> .voltage	= {2700, 3600},
> },
> {
> .vendor		= "Spansion",
> .name		= "S25FL128S......0", /* hybrid: 32 (top or bottom) 4 kB sub-sectors + 64 kB sectors */
> .bustype	= BUS_SPI,
> .manufacture_id	= SPANSION_ID,
> .model_id	= SPANSION_S25FL128,
>
> @@ -10403,9 +10465,46 @@ const struct flashchip flashchips[] = {
>    	.probe_timing	= TIMING_ZERO,
>    	.block_erasers	= {
>    		{
> 		.eraseblocks = { {4 * 1024, 4096} },
> 		.block_erase = spi_block_erase_20,
> 	/* FIXME: erase opcodes 20h for (top or bottom) 4 kB parameter sectors only
> 		.eraseblocks = {
> 			{4 * 1024, 32},
> 			{64 * 1024, 254}, // NOP
> 		},
> 		.block_erase = NULL,
> 	}, { */
> 		.eraseblocks = { { 64 * 1024, 256} },
> 		.block_erase = spi_block_erase_d8,
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_60,
> }, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_c7,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_bp2_ep_srwd, /* TODO: SR2 and many others */
> .unlock		= spi_disable_blockprotect_bp2_srwd, /* TODO: various other locks */
> .write		= spi_chip_write_256, /* Multi I/O supported */
> .read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
> .voltage	= {2700, 3600},
> },
> {
> .vendor		= "Spansion",
> .name		= "S25FL128S......1", /* uniform 256 kB sectors */
> .bustype	= BUS_SPI,
> .manufacture_id	= SPANSION_ID,
> .model_id	= SPANSION_S25FL128,
> .total_size	= 16384,
> .page_size	= 512,
> /* supports 4B addressing */
> /* OTP: 1024B total, 32B reserved; read 0x4B; write 0x42 */
> .feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	= {
> 	{
> 	.eraseblocks = { {256 * 1024, 64} },
> 	.block_erase = spi_block_erase_d8,
> }, {
>
> @@ -10422,6 +10521,83 @@ const struct flashchip flashchips[] = {
>    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
>    	.voltage	= {2700, 3600},
>    },
> {
> .vendor		= "Spansion",
> .name		= "S25FL129P......0", /* hybrid: 32 (top or bottom) 4 kB sub-sectors + 64 kB sectors */
> .bustype	= BUS_SPI,
> .manufacture_id	= SPANSION_ID,
> .model_id	= SPANSION_S25FL128,
> .total_size	= 16384,
> .page_size	= 256,
> /* OTP: 506B total, 16B reserved; read 0x4B; write 0x42 */
> .feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
> .tested		= TEST_UNTESTED,

Probe and read work, and I tested write for all the supported
block_erasers, each time with random data generated with:

dd if=/dev/urandom count=$((16 * 1024 * 1024)) iflag=count_bytes of=random_data.bin
> .probe		= probe_spi_rdid,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	= {
> 	{
> 	/* FIXME: erase opcodes 20h for (top or bottom) 4 kB parameter sectors only
> 		.eraseblocks = {
> 			{4 * 1024, 32},

I also wanted to understand this "parameter sub-sector" thing a little
better, it was not obvious to me what "(top or bottom)" meant in this
context. Then I realized than sometimes these parameter sub-subsectors
are at the start of the chip, sometimes they are at the end.

Anyway, spi_block_erase_20 works with these 4KiB sub-sectors on the
first two 64KiB sectors (i.e. up to 0x01f000-0x01ffff):

... 0x01f000-0x01ffff:EW, 0x020000-0x02ffff:EFAILED ...

Just toe xperiment, I even tried to enable 4KiB sub-sectors for the
whole flash but verification fails on the third 64KiB sector.

...  0x01f000-0x01ffff:EW, 0x020000-0x020fff:EFAILED ...
> 			{64 * 1024, 254}, // NOP

as said verification fails here with spi_block_erase_20, is this because
spi_block_erase_20 is a NOP from the third 64KiB sector on? If so, maybe
the comment is a little too essential, it becomes clear only after you
_know_ that it refers to opcode 20h.

Maybe the comments can be modeled after the similar ones for some Intel
chips (e.g. "25F160S33B8")?

So if someone wanted to make this work should flashrom be changed so
that it'd be possible to specify different eraser functions for
different flash regions? Just curios.
> 		},
> 		.block_erase = NULL,

mention spi_block_erase_20 here and spi_block_erase_40 in the following item?
> 	}, { */
> 	/* FIXME: erase opcodes 40h for (top or bottom) 2*4 kB parameter sectors only
> 		.eraseblocks = {
> 			{8 * 1024, 16},
> 			{64 * 1024, 254}, // NOP
> 		},
> 		.block_erase = NULL,

I didn't play with this, because there's no spi_block_erase_40, but
I guess the results would be consistent with the 4KiB blocks.
> 	}, { */
> 		.eraseblocks = { { 64 * 1024, 256} },
> 		.block_erase = spi_block_erase_d8,

This one is OK.
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_60,

This one is OK.
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_c7,

This one is OK.
> 	}
> },
> .printlock	= spi_prettyprint_status_register_bp2_ep_srwd, /* TODO: Configuration register */
> .unlock		= spi_disable_blockprotect_bp2_srwd,
> .write		= spi_chip_write_256, /* Multi I/O supported */
> .read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
> .voltage	= {2700, 3600},
> },
> {
> .vendor		= "Spansion",
> .name		= "S25FL128P......1", /* uniform 256 kB sectors */

^
This should be "S25FL129P......1".
> .bustype	= BUS_SPI,
> .manufacture_id	= SPANSION_ID,
> .model_id	= SPANSION_S25FL128,
> .total_size	= 16384,
> .page_size	= 512,
> /* OTP: 506B total, 16B reserved; read 0x4B; write 0x42 */
> .feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	= {
> 	{
> 		.eraseblocks = { {256 * 1024, 64} },
> 		.block_erase = spi_block_erase_d8,
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_60,
> 	}, {
> 		.eraseblocks = { { 16384 * 1024, 1} },
> 		.block_erase = spi_block_erase_c7,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_bp2_ep_srwd, /* TODO: Configuration register */
> .unlock		= spi_disable_blockprotect_bp2_srwd,
> .write		= spi_chip_write_256, /* Multi I/O supported */
> .read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
> .voltage	= {2700, 3600},
>
> },
>
> {
> .vendor		= "SST",
>
> diff --git a/flashchips.h b/flashchips.h
> index 029bae8..b2c1d4d 100644
> --- a/flashchips.h
> +++ b/flashchips.h
> @@ -590,7 +590,7 @@
>  #define SPANSION_S25FL016A	0x0214
>  #define SPANSION_S25FL032A	0x0215	/* Same as S25FL032P, but the latter supports EDI and CFI */
>  #define SPANSION_S25FL064A	0x0216	/* Same as S25FL064P, but the latter supports EDI and CFI */
> -#define SPANSION_S25FL128	0x2018
> +#define SPANSION_S25FL128	0x2018	/* Same ID for various S25FL128P, S25FL128S and S25FL129P (including dual-die S70FL256P) variants (EDI supported) */
>  #define SPANSION_S25FL256	0x0219
>  #define SPANSION_S25FL512	0x0220
> #define SPANSION_S25FL204	0x4013
>
> Kind regards, Stefan Tauner

[PATCH] Add support for S25FL128P, S25FL129P and refine it for S25FL128S chips.

[Stefan Tauner]

On Wed, 6 Aug 2014 13:52:19 +0200
Antonio Ospite  wrote:
> On Mon,  4 Aug 2014 00:46:55 +0200
> Stefan Tauner  wrote:
> > Additionally to the existing S25FL128S......0 definition this patch
> > adds S25FL128P......0, S25FL128P......1 and S25FL128S......1, as well as
> > S25FL129P......0 and S25FL129P......1 definitions.
> > S25FL12xP seem to be the predecessor families of S25FL128S. All
> > associated chips can not be distinguished with RDID alone.
> >
> > Besides the new chips, this patch also fixes the name of the  previously
> > supported S25FL128S model with uniform 256 kB sectors
> > (S25FL128P......1 not 0) and adds the hybrid sector version (0) as well.
> >
> > Due to the shared IDs the user has to select the right chip manually
> > with the -c parameter. To make this even possible, this patch enlarges
> > the respective array for results to 6.
>
> Hi Stefan, the chip I've got is a S25FL129P......0, logs attached and
> some comments inlined below.
>
> Thanks.
> > Signed-off-by: Stefan Tauner
>
> Tested-by: Antonio Ospite
> > cli_classic.c |   2 +-
> >  flashchips.c  | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  flashchips.h  |   2 +-
> >  3 files changed, 181 insertions(+), 5 deletions(-)

[…]
> > diff --git a/flashchips.c b/flashchips.c
> > index a1166e0..e587b28 100644
> > --- a/flashchips.c
> > +++ b/flashchips.c

[…]
> > @@ -10422,6 +10521,83 @@ const struct flashchip flashchips[] = {
> >    	.read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
> >    	.voltage	= {2700, 3600},
> >    },
> > {

fixed this missing empty line here as well...
> > .vendor		= "Spansion",
> > .name		= "S25FL129P......0", /* hybrid: 32 (top or bottom) 4 kB sub-sectors + 64 kB sectors */
> > .bustype	= BUS_SPI,
> > .manufacture_id	= SPANSION_ID,
> > .model_id	= SPANSION_S25FL128,
> > .total_size	= 16384,
> > .page_size	= 256,
> > /* OTP: 506B total, 16B reserved; read 0x4B; write 0x42 */
> > .feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
> > .tested		= TEST_UNTESTED,
>
> Probe and read work, and I tested write for all the supported
> block_erasers, each time with random data generated with:
>
> dd if=/dev/urandom count=$((16 * 1024 * 1024)) iflag=count_bytes of=random_data.bin

great.
> > .probe		= probe_spi_rdid,
> > .probe_timing	= TIMING_ZERO,
> > .block_erasers	= {
> > 	{
> > 	/* FIXME: erase opcodes 20h for (top or bottom) 4 kB parameter sectors only
> > 		.eraseblocks = {
> > 			{4 * 1024, 32},
>
> I also wanted to understand this "parameter sub-sector" thing a little
> better, it was not obvious to me what "(top or bottom)" meant in this
> context. Then I realized than sometimes these parameter sub-subsectors
> are at the start of the chip, sometimes they are at the end.
>
> Anyway, spi_block_erase_20 works with these 4KiB sub-sectors on the
> first two 64KiB sectors (i.e. up to 0x01f000-0x01ffff):
>
> ... 0x01f000-0x01ffff:EW, 0x020000-0x02ffff:EFAILED ...
>
> Just toe xperiment, I even tried to enable 4KiB sub-sectors for the
> whole flash but verification fails on the third 64KiB sector.
>
> ...  0x01f000-0x01ffff:EW, 0x020000-0x020fff:EFAILED ...
> > 			{64 * 1024, 254}, // NOP
>
> as said verification fails here with spi_block_erase_20, is this because
> spi_block_erase_20 is a NOP from the third 64KiB sector on? If so, maybe
> the comment is a little too essential, it becomes clear only after you
> _know_ that it refers to opcode 20h.
>
> Maybe the comments can be modeled after the similar ones for some Intel
> chips (e.g. "25F160S33B8")?

Definitely... I knew that there were other chips with similar
semantics, but I forgot about these very similar chips. Thanks for the
pointer.
> So if someone wanted to make this work should flashrom be changed so
> that it'd be possible to specify different eraser functions for
> different flash regions? Just curios.

Yes, exactly, or alternatively to annotate only a part of the blocks as
affected. It is quite unlikely that either is implemented soonish...
unless someone unexpectedly steps up to do it ;)
> > 		},
> > 		.block_erase = NULL,
>
> mention spi_block_erase_20 here and spi_block_erase_40 in the following item?

like for the intel chips, yes...
> > 	}, { */
> > 	/* FIXME: erase opcodes 40h for (top or bottom) 2*4 kB parameter sectors only
> > 		.eraseblocks = {
> > 			{8 * 1024, 16},
> > 			{64 * 1024, 254}, // NOP
> > 		},
> > 		.block_erase = NULL,
>
> I didn't play with this, because there's no spi_block_erase_40, but
> I guess the results would be consistent with the 4KiB blocks.

hopefully, else flashrom or the chips or their documentation are
faulty :)
> > 	}, { */
> > 		.eraseblocks = { { 64 * 1024, 256} },
> > 		.block_erase = spi_block_erase_d8,
>
> This one is OK.
> > 	}, {
> > 		.eraseblocks = { { 16384 * 1024, 1} },
> > 		.block_erase = spi_block_erase_60,
>
> This one is OK.
> > 	}, {
> > 		.eraseblocks = { { 16384 * 1024, 1} },
> > 		.block_erase = spi_block_erase_c7,
>
> This one is OK.
> > 	}
> > },
> > .printlock	= spi_prettyprint_status_register_bp2_ep_srwd, /* TODO: Configuration register */
> > .unlock		= spi_disable_blockprotect_bp2_srwd,
> > .write		= spi_chip_write_256, /* Multi I/O supported */
> > .read		= spi_chip_read, /* Fast read (0x0B) and multi I/O supported */
> > .voltage	= {2700, 3600},
> > },
> > {
> > .vendor		= "Spansion",
> > .name		= "S25FL128P......1", /* uniform 256 kB sectors */
>                                       ^
>
> This should be "S25FL129P......1".

Thanks

I have refined and committed this patch in r1838, thanks for the very
thorough review!