Change in coreboot[master]: spi: Add struct spi_flash_bpbits, a common rep. for block protection

[Daniel Gröber (dxld) (Code Review)]

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 <[e-mail]>
---
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 {
 	/*

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber <[e-mail]>
Gerrit-MessageType: newchange

[Julius Werner (Code Review)]

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.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Julius Werner <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 18 Jun 2020 03:03:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Daniel Gröber (dxld) (Code Review)]

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.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Julius Werner <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 18 Jun 2020 10:33:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <[e-mail]>
Gerrit-MessageType: comment

[S] Change in coreboot[master]: spi: Move block protection bits to new struct spi_flash_bpbits

[Daniel Gröber (dxld) (Code Review)]

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 <[e-mail]>.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 23
Gerrit-Owner: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Julius Werner <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: deleteReviewer

[S] Change in coreboot[main]: spi: Move block protection bits to new struct spi_flash_bpbits

[Jakub Czapiga (Code Review)]

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?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 25
Gerrit-Owner: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Reviewer: Daniel Gröber <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Jakub Czapiga <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-Attention: Daniel Gröber <[e-mail]>
Gerrit-Attention: Julius Werner <[e-mail]>
Gerrit-Attention: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Comment-Date: Mon, 02 Oct 2023 12:50:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[S] Change in coreboot[main]: spi: Add new struct spi_flash_bpbits for block protection bits

[Daniel Gröber (dxld) (Code Review)]

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 <[e-mail]>
---
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
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 26
Gerrit-Owner: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Daniel Gröber <[e-mail]>
Gerrit-CC: Jakub Czapiga <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-Attention: Julius Werner <[e-mail]>
Gerrit-Attention: Daniel Gröber (dxld) <[e-mail]>
Gerrit-MessageType: newpatchset

[S] Change in coreboot[main]: spi: Add new struct spi_flash_bpbits for block protection bits

[Daniel Gröber (dxld) (Code Review)]

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

[S] Change in coreboot[main]: spi: Add new struct spi_flash_bpbits for block protection bits

[Jakub Czapiga (Code Review)]

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


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 26
Gerrit-Owner: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Reviewer: Jakub Czapiga <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Daniel Gröber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-Attention: Julius Werner <[e-mail]>
Gerrit-Attention: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Comment-Date: Tue, 03 Oct 2023 10:39:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[S] Change in coreboot[main]: spi: Add new struct spi_flash_bpbits for block protection bits

[Felix Held (Code Review)]

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 <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42112
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Jakub Czapiga <[e-mail]>
---
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 {
 	/*

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/42112?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I02828b1f764aea29374e794001e74cdc86a94c92
Gerrit-Change-Number: 42112
Gerrit-PatchSet: 27
Gerrit-Owner: Daniel Gröber (dxld) <[e-mail]>
Gerrit-Reviewer: Felix Held <[e-mail]>
Gerrit-Reviewer: Jakub Czapiga <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Daniel Gröber
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-CC: Stefan Reinauer <[e-mail]>
Gerrit-MessageType: merged