Re: [flashrom] [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!