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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Jun 30 22:54:15 CEST 2011


Am 30.06.2011 20:58 schrieb 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?
>   

spi_chip_write_256, and page_size=64. This reminds me... page_size must die!


>>> +		.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.
>   

Right, that is something I wanted to comment on once hwseq works. I'll
send my response in that thread.


>>>  
>>>  #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.
>   

probe_flash will see another refactoring soon once the programmer
registration stuff is completely done and merged. By then, probing will
be per-busmaster, not per-chip. Don't invest too much (or any) time in a
probe_flash refactoring, it might get rewritten again.


>>> +
>>> +		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?
>   

I think it87 can't deal with this. Some SPI masters (At least SB600,
ICH/VIA, Dediprog, maybe even others) have explicit ways to handle one
dummy byte between write and read, but we currently ignore that feature
everywhere.


>>> +/*      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).
>   

The 25 comes from the *25* in most SPI flash chip model names.
spi_sfdp.c might be an option, but it does not fit with the names of the
other chip driver files.


>>> […]
>>>       
>> 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.
>   

If a patch had a full review, it makes sense to post a separate fixup
(if that fixup needs review), but I prefer a merged patch pretty much
for everything. You could post a fixup as reply to the original patch if
you want quick feedback for the fixup, but if you repost a whole series,
please use merged patches always.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list