Change in coreboot[master]: spi: Add struct spi_flash_bpbits, a common rep. for block protection
Hello Daniel Gröber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42112
to review the following change.
Change subject: spi: Add struct spi_flash_bpbits, a common rep. for block protection ......................................................................
spi: Add struct spi_flash_bpbits, a common rep. for block protection
Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/include/spi_flash.h 1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/42112/1
diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index 35b02db..7dce216 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -49,6 +49,39 @@ int (*status)(const struct spi_flash *flash, u8 *reg); };
+struct spi_flash_bpbits { + unsigned int bp; /*< block protection select bits */ + bool cmp; /*< complement protect */ + bool tb; /*< top=0 / bottom=1 select */ + union { + struct { + union { bool srp1, srl; }; + union { bool srp0, srp; }; + /* + * For W25Q*{,F}* parts: + * srp1 srp0 + * 0 0 | writable if WEL==1 + * 0 1 | writable if WEL==1 && #WP==Vcc + * 1 0 | not writable until next power-down + * 1 1 | not writable, permanently + * + * checked datasheets: W25Q128FV, (W25Q80, W25Q16, + * W25Q32) + * + * For W25Q*{J,D}* parts: + * + * srl srp + * 0 0 | writable if WEL==1 + * 0 1 | writable if WEL==1 && #WP==Vcc + * 1 x | not writable until next power-down + * + * checked datasheets: W25Q132JW, W25Q128JW, W25Q256JV. + * W25Q16DW + */ + } winbond; + }; +}; + /* Current code assumes all callbacks are supplied in this object. */ struct spi_flash_protection_ops { /*
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42112 )
Change subject: spi: Add struct spi_flash_bpbits, a common rep. for block protection ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42112/2/src/include/spi_flash.h File src/include/spi_flash.h:
https://review.coreboot.org/c/coreboot/+/42112/2/src/include/spi_flash.h@73 PS2, Line 73: * srl srp Do we need to carry this naming distinction through our code? I think Winbond just renamed these at some point for all their newer chips and it's not really related to the type (e.g. I also found a W25Q128JV that uses SRP/SRL notation but allows the permanent protection). I would just stick with the SRP0/SRP1 notation everywhere to avoid adding confusion where it's not really needed.
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42112 )
Change subject: spi: Add struct spi_flash_bpbits, a common rep. for block protection ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42112/2/src/include/spi_flash.h File src/include/spi_flash.h:
https://review.coreboot.org/c/coreboot/+/42112/2/src/include/spi_flash.h@73 PS2, Line 73: * srl srp
Do we need to carry this naming distinction through our code? I think Winbond just renamed these at […]
W25Q128JV also has the behaviour I'm documenting here. The new parts with srp/srl have this special (undocumented AFAICS[1]) status register write sequence that makes writing this bit permanent. Normal writes have the behaviour as documented here though.
[1]: Or at least they say: Note: Please contact Winbond for details regarding the special instruction sequence.
I really just wanted to put a note somewhere pointing out the difference in behavour between these two generations of parts. I suppose I could move the tables to winbond_region_to_bpbits instead and remove the bool unions if you prefer.
Daniel Gröber (dxld) has removed Daniel Gröber from this change. ( https://review.coreboot.org/c/coreboot/+/42112 )
Change subject: spi: Move block protection bits to new struct spi_flash_bpbits ......................................................................
Removed reviewer Daniel Gröber dxld@darkboxed.org.
Attention is currently required from: Daniel Gröber, Daniel Gröber (dxld), Julius Werner.
Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42112?usp=email )
Change subject: spi: Move block protection bits to new struct spi_flash_bpbits ......................................................................
Patch Set 25:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/42112/comment/37cba46f_497fdb5d : PS24, Line 7: ove block protection bits to new struct spi_flash_bpbits This patch does not actually move mentioned values, but only creates new struct for them. Please adjust commit message accordingly.
File src/include/spi_flash.h:
https://review.coreboot.org/c/coreboot/+/42112/comment/08512c65_549ef041 : PS24, Line 53: bp nit: I think it would be better to use more descriptive field names when possible. Applies to `bp`, `cmp` and `tb`.
https://review.coreboot.org/c/coreboot/+/42112/comment/a0f5ee91_b45e0108 : PS24, Line 56: union { Is this union necessary?
Attention is currently required from: Daniel Gröber (dxld), Julius Werner.
Hello Daniel Gröber, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42112?usp=email
to look at the new patch set (#26).
Change subject: spi: Add new struct spi_flash_bpbits for block protection bits ......................................................................
spi: Add new struct spi_flash_bpbits for block protection bits
Currently the block protection bits are being passed around as individual arguments. We will use this new struct to replace the corresponding arguments in the winbond_bpbits_to_region and winbond_set_write_protection functions.
Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92 Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/include/spi_flash.h 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/42112/26
Attention is currently required from: Jakub Czapiga, Julius Werner.
Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42112?usp=email )
Change subject: spi: Add new struct spi_flash_bpbits for block protection bits ......................................................................
Patch Set 26:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/42112/comment/2ca4c236_dad847fa : PS24, Line 7: ove block protection bits to new struct spi_flash_bpbits
This patch does not actually move mentioned values, but only creates new struct for them. […]
Done
File src/include/spi_flash.h:
https://review.coreboot.org/c/coreboot/+/42112/comment/3e904441_85500abe : PS24, Line 53: bp
nit: I think it would be better to use more descriptive field names when possible. […]
The names are taken from the existing code I'm refactoring. See https://review.coreboot.org/c/coreboot/+/42115/26/src/drivers/spi/winbond.c#...
They are based on the winbond datasheet register descriptions, so I think it'd be ill advised to rename them in the code for no good reason.
https://review.coreboot.org/c/coreboot/+/42112/comment/b3fbdb5f_e8645e81 : PS24, Line 56: union {
Is this union necessary?
It's filled with a macronix specific struct later in the series, see https://review.coreboot.org/c/coreboot/+/41750/38/src/include/spi_flash.h
Attention is currently required from: Daniel Gröber (dxld), Julius Werner.
Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42112?usp=email )
Change subject: spi: Add new struct spi_flash_bpbits for block protection bits ......................................................................
Patch Set 26: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42112?usp=email )
Change subject: spi: Add new struct spi_flash_bpbits for block protection bits ......................................................................
spi: Add new struct spi_flash_bpbits for block protection bits
Currently the block protection bits are being passed around as individual arguments. We will use this new struct to replace the corresponding arguments in the winbond_bpbits_to_region and winbond_set_write_protection functions.
Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92 Signed-off-by: Daniel Gröber dxld@darkboxed.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/42112 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Jakub Czapiga czapiga@google.com --- M src/include/spi_flash.h 1 file changed, 11 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Jakub Czapiga: Looks good to me, approved
diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index fe77e3e..d5471a4 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -49,6 +49,17 @@ int (*status)(const struct spi_flash *flash, u8 *reg); };
+struct spi_flash_bpbits { + unsigned int bp; /*< block protection select bits */ + bool cmp; /*< complement protect */ + bool tb; /*< top=0 / bottom=1 select */ + union { + struct { + bool srp1, srp0; + } winbond; + }; +}; + /* Current code assumes all callbacks are supplied in this object. */ struct spi_flash_protection_ops { /*
-
Daniel Gröber (dxld) (Code Review) -
Felix Held (Code Review)
-
Jakub Czapiga (Code Review)
-
Julius Werner (Code Review)