[flashrom] [PATCH 3/3] add support for SFDP (JESD216)

[Stefan Tauner]

On Thu, 30 Jun 2011 20:24:30 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 24.06.2011 16:53 schrieb Stefan Tauner:
> > Similar to ICH Hardware Sequencing this uses a generic struct flashchip
> > element in flashrom.c with dummy values and a special probe function that fills the
> > obtained values into that generic struct.
> > Documentation used:
> > http://www.jedec.org/standards-documents/docs/jesd216 (2011-04)
> > W25Q32BV data sheet Revision F (2011-04-01)
> > EN25QH16 data sheet Revision F (2011-06-01)
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> >   
> 
> Nice feature.

we will have to wait to see if that is true. :)

> > ---
> >  chipdrivers.h |    1 +
> >  flashchips.c  |   24 +++++
> >  flashchips.h  |    1 +
> >  flashrom.c    |   28 ++++++-
> >  spi.h         |    5 +
> >  spi25.c       |  273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 331 insertions(+), 1 deletions(-)
> >
> > diff --git a/chipdrivers.h b/chipdrivers.h
> > index 92ddbea..0a3df50 100644
> > --- a/chipdrivers.h
> > +++ b/chipdrivers.h
> > @@ -26,6 +26,7 @@
> >  #define __CHIPDRIVERS_H__ 1
> >  
> >  /* spi.c, should probably be in spi_chip.c */
> > +int probe_spi_sfdp(struct flashchip *flash);
> >  int probe_spi_rdid(struct flashchip *flash);
> >  int probe_spi_rdid4(struct flashchip *flash);
> >  int probe_spi_rems(struct flashchip *flash);
> > diff --git a/flashchips.c b/flashchips.c
> > index 865ba2f..6fbbd2c 100644
> > --- a/flashchips.c
> > +++ b/flashchips.c
> > @@ -8507,6 +8507,30 @@ const struct flashchip flashchips[] = {
> >  		.write		= write_jedec_1,
> >  		.read		= read_memmapped,
> >  	},
> > +	
> > +	{
> > +		.vendor		= "Unknown",
> > +		.name		= "SFDP device",
> > +		.bustype	= CHIP_BUSTYPE_SPI,
> > +		.manufacture_id	= GENERIC_MANUF_ID,
> > +		.model_id	= SFDP_DEVICE_ID,
> > +		/* We present our own "report this" text hence we do not
> > +		 * want the default "This flash part has status UNTESTED..."
> > +		 * text to be printed. */
> > +		.tested		= TEST_OK_PREW,
> > +		.probe		= probe_spi_sfdp,
> > +		.page_size	= 256,
> > +		.write		= spi_chip_write_256,
> >   
> 
> Does SFDP really specify 256-byte write capability?

ah right. i forgot about that.
there is a field in the first dword which is the only thing mentioned
regarding this subject:
"Write Granularity
0: 1 Byte – Use this setting for single byte programmable devices or buffer
programmable devices when the buffer is less than 64 bytes (32 Words).
1: Use this setting for buffer programmable devices when the buffer size is 64 bytes (32
Words) or larger.

0 is clear and i should set .write = spi_chip_write_1, but what about the other case?

> > +		.read		= spi_chip_read,
> > +		/* FIXME: some vendor extensions define this */
> > +		.voltage	= {},
> > +		 /* Everything below will be set by the probing function. */
> > +		.total_size	= 0,
> > +		.feature_bits	= 0,
> > +		.block_erasers	= {},
> > +		.unlock		= NULL,
> > +		.printlock	= NULL,
> > +	},
> >  
> >  	{
> >  		.vendor		= "AMIC",
> > diff --git a/flashchips.h b/flashchips.h
> > index 3b2b94f..82333c8 100644
> > --- a/flashchips.h
> > +++ b/flashchips.h
> > @@ -36,6 +36,7 @@
> >  
> >  #define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
> >  #define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
> > +#define SFDP_DEVICE_ID		0xfffe
> >   
> 
> Just a comment, not something that needs to be changed: We probably need
> CFI_DEVICE_ID as well once we support CFI (which is SFDP for non-SPI flash).

and there is the id for hwseq, which is currently also 0xfffe. please
advice. hint: .manufacture_id and .model_id are uint32_t.

> 
> >  
> >  #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
> >  #define ALLIANCE_AS29F002B	0x34
> > diff --git a/flashrom.c b/flashrom.c
> > index aed10aa..56af373 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1206,7 +1206,33 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force)
> >  		 * probe_flash() is the first one and thus no chip has been
> >  		 * found before.
> >  		 */
> > -		if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
> > +		if (startchip == 0 && fill_flash->model_id == SFDP_DEVICE_ID) {
> > +			msg_cinfo("===\n"
> > +				  "SFDP has autodetected a flash chip which is "
> > +				  "not natively supported by flashrom yet.\n");
> > +			if (!check_block_erasers(flash, 0))
> > +				msg_cinfo("The standard operations read and "
> > +					  "verify should work, but to support "
> > +					  "erase, write and all other "
> > +					  "possible features");
> > +			else
> > +				msg_cinfo("All standard operations (read, "
> > +					  "verify, erase and write) should "
> > +					  "work, but to support all possible "
> > +					  "features");
> > +
> > +			msg_cinfo(" we need to add them manually.\nYou "
> > +				  "can help us by mailing us the output of "
> > +				  "the following command to flashrom at flashrom."
> > +				  "org: \n'flashrom -VV [plus the "
> > +				  "-p/--programmer parameter (if needed)]"
> > +				  "'\nThanks for your help!\n"
> > +				  "===\n");
> > +		}
> >   
> 
> Should we refactor those SFDP messages into a separate function and do
> that for the untested chip messages as well? It would probably improve
> code readability here.

THAT is not the problem with probe_flash imho. i have refactoring that
function (and its caller) on my agenda. that does not contradict
refactoring those messages out, but it wont solve the readability issue
that exists imho.

> 
> > +
> > +		if (startchip == 0 ||
> > +		    ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> > +		     (fill_flash->model_id != SFDP_DEVICE_ID)))
> >  			break;
> >  
> >  notfound:
> > diff --git a/spi.h b/spi.h
> > index b908603..5f07eae 100644
> > --- a/spi.h
> > +++ b/spi.h
> > @@ -40,6 +40,11 @@
> >  #define JEDEC_REMS_OUTSIZE	0x04
> >  #define JEDEC_REMS_INSIZE	0x02
> >  
> > +/* Read Serial Flash Discoverable Parameters (SFDP) */
> > +#define JEDEC_SFDP		0x5a
> > +#define JEDEC_SFDP_OUTSIZE	0x05	/* 8b op, 24b addr, 8b dummy */
> >   
> 
> Ouch. A few SPI masters will have pretty explosions with that outsize.
> This reminds me... I should resend my patch which can deal with dummy
> bytes between write and read in a SPI command.

can you point me to one, so that i can look at that code please?

> 
> > +/*      JEDEC_SFDP_INSIZE : any length */
> > +
> >  /* Read Electronic Signature */
> >  #define JEDEC_RES		0xab
> >  #define JEDEC_RES_OUTSIZE	0x04
> > diff --git a/spi25.c b/spi25.c
> > index d3680fb..af81f19 100644
> > --- a/spi25.c
> > +++ b/spi25.c
> > @@ -23,6 +23,7 @@
> >   */
> >  
> >  #include <string.h>
> > +#include <stdlib.h>
> >  #include "flash.h"
> >  #include "flashchips.h"
> >  #include "chipdrivers.h"
> > @@ -113,6 +114,278 @@ int spi_write_disable(void)
> >  	return spi_send_command(sizeof(cmd), 0, cmd, NULL);
> >  }
> >  
> >   
> 
> The code below is huge, and I'd argue it is for a chip family and should
> live in sfdp25.c (or something with a similar name).

sfdp.c? or spi_sfdp.c?
why 25? probably some jedec thing, but i dont know exactly what that
indicates (and if it is appropriate here).

>> […]
> 
> Could you resend this with the comments addressed and with the squash
> patch merged in? I didn't review the SFDP code yet, but the right
> presentation makes reviewing easier.

sure. do you always prefer the squashes/fixes already merged? i wonder
what is better for reviewers in general.

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