Change in ...coreboot[master]: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips
Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30746
to review the following change.
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips
Required for ACPI S3 suspend support at some motherboards. Synchronizing with flashchips.c/h flashrom source code.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I4d15d5acf0e2044e5128ce809c282fbcb35f24f0 --- M src/drivers/spi/winbond.c 1 file changed, 58 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/30746/1
diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index f8ea247..8bf8fcd 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -104,6 +104,38 @@
static const struct winbond_spi_flash_params winbond_spi_flash_table[] = { { + .id = 0x2014, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 4, + .name = "W25P80", + }, + { + .id = 0x2015, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 5, + .name = "W25P16", + }, + { + .id = 0x2016, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 6, + .name = "W25P32", + }, + { + .id = 0x3014, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 4, + .name = "W25X80", + }, + { .id = 0x3015, .l2_page_size_shift = 8, .pages_per_sector_shift = 4, @@ -133,7 +165,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 4, - .name = "W25Q80", + .name = "W25Q80_V", }, { .id = 0x4015, @@ -141,7 +173,17 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 5, - .name = "W25Q16", + .name = "W25Q16_V", + .protection_granularity_shift = 16, + .bp_bits = 3, + }, + { + .id = 0x6015, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 5, + .name = "W25Q16DW", .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -151,7 +193,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 6, - .name = "W25Q32", + .name = "W25Q32_V", .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -171,7 +213,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 7, - .name = "W25Q64", + .name = "W25Q64_V", .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -191,7 +233,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 8, - .name = "W25Q128", + .name = "W25Q128_V", .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -221,7 +263,17 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 9, - .name = "W25Q256", + .name = "W25Q256_V", + .protection_granularity_shift = 16, + .bp_bits = 4, + }, + { + .id = 0x7019, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 9, + .name = "W25Q256J", .protection_granularity_shift = 16, .bp_bits = 4, },
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1:
Nico, please could you review this question - https://review.coreboot.org/c/coreboot/+/30746#message-cf0f667739d5bf056dc15...
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1:
Nico, please could you review this question - https://review.coreboot.org/c/coreboot/+/30746#message-cf0f667739d5bf056dc15...
I have know idea what that granularity is supposed to mean, neither do I agree with the existence of SPI flash drivers in coreboot ;) so I'm not going to look into it.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1:
Nico, please could you review this question - https://review.coreboot.org/c/coreboot/+/30746#message-cf0f667739d5bf056dc15...
I have know idea what that granularity is supposed to mean, neither do I agree with the existence of SPI flash drivers in coreboot ;) so I'm not going to look into it.
Unless it's for reading the boot medium, of course.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1: Code-Review+2
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
Patch Set 1: Code-Review+1
I think this change could be accepted because if .protection_granularity_shift=16 worked for W25Q256_V then it should work for a similar W25Q256J(V). Later I could send a separate patch which will change .protection_granularity_shift for W25Q256* chips from 16 to 19 , following the logic of W25Q32*=16 / W25Q64*=17 / W25Q128*=18
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30746 )
Change subject: drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips ......................................................................
drivers/spi/winbond.c: Add the rest of >=1MB Winbond W25 chips
Required for ACPI S3 suspend support at some motherboards. Synchronizing with flashchips.c/h flashrom source code.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I4d15d5acf0e2044e5128ce809c282fbcb35f24f0 Reviewed-on: https://review.coreboot.org/c/30746 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M src/drivers/spi/winbond.c 1 file changed, 58 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Kyösti Mälkki: Looks good to me, but someone else must approve Paul Menzel: Looks good to me, but someone else must approve mikeb mikeb: Looks good to me, but someone else must approve
diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index f8ea247..8bf8fcd 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -104,6 +104,38 @@
static const struct winbond_spi_flash_params winbond_spi_flash_table[] = { { + .id = 0x2014, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 4, + .name = "W25P80", + }, + { + .id = 0x2015, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 5, + .name = "W25P16", + }, + { + .id = 0x2016, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 6, + .name = "W25P32", + }, + { + .id = 0x3014, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 4, + .name = "W25X80", + }, + { .id = 0x3015, .l2_page_size_shift = 8, .pages_per_sector_shift = 4, @@ -133,7 +165,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 4, - .name = "W25Q80", + .name = "W25Q80_V", }, { .id = 0x4015, @@ -141,7 +173,17 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 5, - .name = "W25Q16", + .name = "W25Q16_V", + .protection_granularity_shift = 16, + .bp_bits = 3, + }, + { + .id = 0x6015, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 5, + .name = "W25Q16DW", .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -151,7 +193,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 6, - .name = "W25Q32", + .name = "W25Q32_V", .protection_granularity_shift = 16, .bp_bits = 3, }, @@ -171,7 +213,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 7, - .name = "W25Q64", + .name = "W25Q64_V", .protection_granularity_shift = 17, .bp_bits = 3, }, @@ -191,7 +233,7 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 8, - .name = "W25Q128", + .name = "W25Q128_V", .protection_granularity_shift = 18, .bp_bits = 3, }, @@ -221,7 +263,17 @@ .pages_per_sector_shift = 4, .sectors_per_block_shift = 4, .nr_blocks_shift = 9, - .name = "W25Q256", + .name = "W25Q256_V", + .protection_granularity_shift = 16, + .bp_bits = 4, + }, + { + .id = 0x7019, + .l2_page_size_shift = 8, + .pages_per_sector_shift = 4, + .sectors_per_block_shift = 4, + .nr_blocks_shift = 9, + .name = "W25Q256J", .protection_granularity_shift = 16, .bp_bits = 4, },
-
Kyösti Mälkki (Code Review) -
mikeb mikeb (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)