Re: [flashrom] [PATCH] spi25.c: Refactor spi_write_status_register helpers.

[Carl-Daniel Hailfinger]

Am 31.01.2012 06:50 schrieb Stefan Tauner:
> In r1115 "Write protection handling for Atmel AT25*" the old spi_write_status_register
> function was duplicated to send WREN and EWSR commands respectively controlled
> by a new common wrapper function spi_write_status_register without a reason.
> Both functions' resulting code is  equal apart from the opcode used. The code
> itself does also differ in the macros used, but their value (apart from the opcode)
> is equal. This patch adds a new parameter for the opcode to the helper function
> which allows removal of the other one. This relies on the fact that EWSR and WREN
> have the same INSIZE and OUTSIZE though. If that is really seen as an issue, the
> sizes could be made parameters too.
>
> This patch also changes the wrapper so that it no longer sets the feature bits
> of the struct flash(ctx) argument. This may result in changed output, because it
> no longer implicitly disables the debug message in following executions.

Ahem. Once we do the per-region unlock dance, this will be extremely
painful. We have three choices:
- Keep that part of the code as is. Not good.
- Spit out that message for every status register write. Not good.
- Fix all flash chip definitions. Good, but separate patch.
How do we proceed?

I'd like to keep the feature bit setting until the flash chip
definitions have been fixed, but this is not a veto-style decision.
> Also, spi_write_status_enable has been dead code since r658 or so. Remove it.
>
> Signed-off-by: Stefan Tauner

Once the feature bit setting is decided upon, this is
Acked-by: Carl-Daniel Hailfinger 
with one small comment below.
> spi25.c |   90 ++++++++++-----------------------------------------------------
>  1 files changed, 14 insertions(+), 76 deletions(-)
>
> diff --git a/spi25.c b/spi25.c
> index 3ce7f08..d81616c 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -720,81 +720,19 @@ int spi_block_erase_c7(struct flashctx *flash, unsigned int addr,
>    return spi_chip_erase_c7(flash);
>  }
>
> -int spi_write_status_enable(struct flashctx *flash)
> -{
> static const unsigned char cmd[JEDEC_EWSR_OUTSIZE] = { JEDEC_EWSR };
> int result;
> /* Send EWSR (Enable Write Status Register). */
> result = spi_send_command(flash, sizeof(cmd), JEDEC_EWSR_INSIZE, cmd, NULL);
> if (result)
> msg_cerr("%s failed\n", __func__);
> return result;
>
> -}
>
> -/*
> This is according the SST25VF016 datasheet, who knows it is more
> generic that this...
> */
>
> -static int spi_write_status_register_ewsr(struct flashctx *flash, int status)
> +static int spi_write_status_register_flag(struct flashctx *flash, int status, const unsigned char opcode)

enable_opcode instead of opcode.
> {
>    int result;
>    int i = 0;
> struct spi_command cmds[] = {
> {
> /* WRSR requires either EWSR or WREN depending on chip type. */
> .writecnt	= JEDEC_EWSR_OUTSIZE,
> .writearr	= (const unsigned char[]){ JEDEC_EWSR },
> .readcnt	= 0,
> .readarr	= NULL,
> }, {
> .writecnt	= JEDEC_WRSR_OUTSIZE,
> .writearr	= (const unsigned char[]){ JEDEC_WRSR, (unsigned char) status },
> .readcnt	= 0,
> .readarr	= NULL,
> }, {
> .writecnt	= 0,
> .writearr	= NULL,
> .readcnt	= 0,
> .readarr	= NULL,
> }};
> result = spi_send_multicommand(flash, cmds);
> if (result) {
> msg_cerr("%s failed during command execution\n",
> 	__func__);
> /* No point in waiting for the command to complete if execution
>  * failed.
>  */
> return result;
> }
> /* WRSR performs a self-timed erase before the changes take effect.
> * This may take 50-85 ms in most cases, and some chips apparently
> * allow running RDSR only once. Therefore pick an initial delay of
> * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
> /*
> * WRSR requires either EWSR or WREN depending on chip type.
> * The code below relies on the fact hat EWSR and WREN have the same
> * INSIZE and OUTSIZE.
> */
> programmer_delay(100 * 1000);
> while (spi_read_status_register(flash) & JEDEC_RDSR_BIT_WIP) {
> if (++i > 490) {
> 	msg_cerr("Error: WIP bit after WRSR never cleared\n");
> 	return TIMEOUT_ERROR;
> }
> programmer_delay(10 * 1000);
> }
> return 0;
>
> -}
>
> -static int spi_write_status_register_wren(struct flashctx *flash, int status)
> -{
> int result;
> int i = 0;
> struct spi_command cmds[] = {
> {
> /* WRSR requires either EWSR or WREN depending on chip type. */
> .writecnt	= JEDEC_WREN_OUTSIZE,
> .writearr	= (const unsigned char[]){ JEDEC_WREN },
> .writearr	= (const unsigned char[]){ opcode },
> .readcnt	= 0,
> .readarr	= NULL,
> }, {
>
> @@ -811,8 +749,7 @@ static int spi_write_status_register_wren(struct flashctx *flash, int status)
>
> result = spi_send_multicommand(flash, cmds);
>    if (result) {
> msg_cerr("%s failed during command execution\n",
> 	__func__);
> msg_cerr("%s failed during command execution\n", __func__);
> /* No point in waiting for the command to complete if execution
> failed.
> */
>
> @@ -836,17 +773,18 @@ static int spi_write_status_register_wren(struct flashctx *flash, int status)
>
> int spi_write_status_register(struct flashctx *flash, int status)
>  {
> int feature_bits = flash->feature_bits;
> int ret = 1;
> if (!(flash->feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
> if (!(feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
> msg_cdbg("Missing status register write definition, assuming "
> 	 "EWSR is needed\n");
> flash->feature_bits |= FEATURE_WRSR_EWSR;
> feature_bits |= FEATURE_WRSR_EWSR;
> }
> if (flash->feature_bits & FEATURE_WRSR_WREN)
> ret = spi_write_status_register_wren(flash, status);
> if (ret && (flash->feature_bits & FEATURE_WRSR_EWSR))
> ret = spi_write_status_register_ewsr(flash, status);
> if (feature_bits & FEATURE_WRSR_WREN)
> ret = spi_write_status_register_flag(flash, status, JEDEC_WREN);
> if (ret && (feature_bits & FEATURE_WRSR_EWSR))
> ret = spi_write_status_register_flag(flash, status, JEDEC_EWSR);
> return ret;
>
> }

Regards,
Carl-Daniel

Tagging all EWSR chips correctly (was: [PATCH] spi25.c: Refactor spi_write_status_register helpers.)

[Stefan Tauner]

On Tue, 07 Feb 2012 23:31:50 +0100
Carl-Daniel Hailfinger  wrote:
> Ahem. Once we do the per-region unlock dance, this will be extremely
> painful. We have three choices:
> Keep that part of the code as is. Not good.
> Spit out that message for every status register write. Not good.
> Fix all flash chip definitions. Good, but separate patch.
>
> How do we proceed?

fixing the stuff that needs fixing, of course.

steve said he might look into it, so this is what has to be done:
i have attached a file containing what seems to be the spi chips
without correctly set feature_bits fields. we need to check the
datasheets for the opcodes needed to enable writing to the status
register. normally this is either WREN (write enable) or EWSR (enable
write status register), or both.
currently we are only interested in the non-volatile bits of the status
register, but there are chips that require another write enable command
for the volatile bits. if anyone creates a spreadsheet, he should
probably note those too if feasible or maybe even add a comment in the
patch.

the field should be added after page_size and either set to:
    	.feature_bits	= FEATURE_WRSR_EWSR
or
    	.feature_bits	= FEATURE_WRSR_EITHER
if EWSR and WREN are allowed (all other SPI chips are already set
to FEATURE_WRSR_WREN). in the case we encounter a chip that does use
something else a comment should suffice for the time being.

we could also split the work up, if it takes too long otherwise...

Tagging all EWSR chips correctly (was: [PATCH] spi25.c: Refactor spi_write_status_register helpers.)

[Steven Zakulec]

On Tue, Feb 7, 2012 at 7:25 PM, Stefan Tauner <
[e-mail]> wrote:
> On Tue, 07 Feb 2012 23:31:50 +0100
> Carl-Daniel Hailfinger  wrote:
> > Ahem. Once we do the per-region unlock dance, this will be extremely
> > painful. We have three choices:
> > Keep that part of the code as is. Not good.
> > Spit out that message for every status register write. Not good.
> > Fix all flash chip definitions. Good, but separate patch.
> >
> > How do we proceed?
>
> fixing the stuff that needs fixing, of course.
>
> steve said he might look into it, so this is what has to be done:
> i have attached a file containing what seems to be the spi chips
> without correctly set feature_bits fields. we need to check the
> datasheets for the opcodes needed to enable writing to the status
> register. normally this is either WREN (write enable) or EWSR (enable
> write status register), or both.
> currently we are only interested in the non-volatile bits of the status
> register, but there are chips that require another write enable command
> for the volatile bits. if anyone creates a spreadsheet, he should
> probably note those too if feasible or maybe even add a comment in the
> patch.
>
> the field should be added after page_size and either set to:
>                .feature_bits   = FEATURE_WRSR_EWSR
> or
>                .feature_bits   = FEATURE_WRSR_EITHER
> if EWSR and WREN are allowed (all other SPI chips are already set
> to FEATURE_WRSR_WREN). in the case we encounter a chip that does use
> something else a comment should suffice for the time being.
> we could also split the work up, if it takes too long otherwise...
>
> Kind regards/Mit freundlichen Grüßen, Stefan Tauner

I have tagged all of the ones in this list Stefan gave me except the Sanyo
one (no datasheet- apparently not a new issue either).

If there are any questions or comments, please send them.

Signed-off-by: Steven Zakulec