[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 <ao2 at ao2.it> wrote:

> On Mon,  4 Aug 2014 00:46:55 +0200
> Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at> 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 <stefan.tauner at alumni.tuwien.ac.at>
> 
> Tested-by: Antonio Ospite <ao2 at ao2.it>
> 
> > ---
> >  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!

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner