Change in ...coreboot[master]: spi/winbond.c: change protection_granularity_shift for W25Q256* from ...

[mikeb mikeb (Code Review)]

Hello Mike Banon,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/31019

to review the following change.

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19

Following the logic of W25Q32*=16 / W25Q64*=17 / W25Q128*=18 ,
protection_granularity_shift value for W25Q256* chips should be 19.

Signed-off-by: Mike Banon 
Change-Id: I488d20898fccd1e967334fdd982b31eedf48f412
---
M src/drivers/spi/winbond.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31019/1

diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c
index 8bf8fcd..9cfa41a 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -264,7 +264,7 @@
    	.sectors_per_block_shift	= 4,
    	.nr_blocks_shift		= 9,
    	.name				= "W25Q256_V",
-		.protection_granularity_shift	= 16,
+		.protection_granularity_shift	= 19,
    	.bp_bits			= 4,
    },
    {
@@ -274,7 +274,7 @@
    	.sectors_per_block_shift	= 4,
    	.nr_blocks_shift		= 9,
    	.name				= "W25Q256J",
-		.protection_granularity_shift	= 16,
+		.protection_granularity_shift	= 19,
    	.bp_bits			= 4,
    },
 };

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Patch Set 1: Code-Review-2

Please read the datasheets.

[mikeb mikeb (Code Review)]

mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-2
>
> Please read the datasheets.

Sorry I checked the datasheets and haven't been able to find any mention of granularity. Could you please clarify where I should be looking, or at least confirm that W25Q256* are really 16 while W25Q128* are 18 (>16) because it looks strange to me

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Patch Set 1:

For SEC=0:

https://www.winbond.com/resource-files/w25q256jv%20spi%20revb%2009202016.pdf
Page 20
The smallest "Protected density" is 64KB which is 1 << 16.

https://www.winbond.com/resource-files/w25q128fv_revhh1_100913_website1.pdf
Page 22
The smallest "Protected density" is 256KB which is 1 << 18.

[mikeb mikeb (Code Review)]

mikeb mikeb has removed Kyösti Mälkki from this change.  ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Removed reviewer Kyösti Mälkki.

[mikeb mikeb (Code Review)]

mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Patch Set 1:
> Patch Set 1:
>
> For SEC=0:
>
> https://www.winbond.com/resource-files/w25q256jv%20spi%20revb%2009202016.pdf
> Page 20
> The smallest "Protected density" is 64KB which is 1 << 16.
>
> https://www.winbond.com/resource-files/w25q128fv_revhh1_100913_website1.pdf
> Page 22
> The smallest "Protected density" is 256KB which is 1 << 18.

Patrick, thank you very much for providing this info and stopping my (as it turned out) harmful patch. I will abandon it now

[mikeb mikeb (Code Review)]

mikeb mikeb has removed Mike Banon from this change.  ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Removed reviewer Mike Banon.

[mikeb mikeb (Code Review)]

mikeb mikeb has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31019 )

Change subject: spi/winbond.c: change protection_granularity_shift for W25Q256* from 16 to 19
......................................................................

Abandoned

Patrick Rudolph proved this patch wrong using the datasheets: min("Protected density") = .protection_granularity_shift