Hello Thomas Heijligen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/34071
to review the following change.
Change subject: chipset_enable: Add support for discrete Cannon Lake PCHs ......................................................................
chipset_enable: Add support for discrete Cannon Lake PCHs
The Cannon Lake "300 Series" PCHs [1,2] share the register layout of the Skylake "100 Series". Mark them as BAD until `ichspi.c` is adapted.
[1] Intel® 300 Series and Intel® C240 Seri Chipset Family Platform Controller Hub Datasheet - Volume 1 of 2 Revison 4 (Dec 2018) Document Number 337347
[2] Intel® 300 Series Chipset Families Platform Controller Hu Datasheet - Volume 2 of 2 Revision 2? (Oct 2018) Document Number 337348
Change-Id: If0b54799d5b93169ee660409bad57ae14677340c Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com Signed-off-by: Nico Huber nico.huber@secunet.com --- M chipset_enable.c M programmer.h 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/34071/1
diff --git a/chipset_enable.c b/chipset_enable.c index 08feda5..4b0bab5 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -598,6 +598,7 @@ break; case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_APOLLO_LAKE: reg_name = "BIOS_SPI_BC"; gcs = pci_read_long(dev, 0xdc); @@ -692,6 +693,7 @@ case CHIPSET_9_SERIES_WILDCAT_POINT_LP: case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_300_SERIES_CANNON_POINT: boot_straps = boot_straps_pch8_lp; break; case CHIPSET_APOLLO_LAKE: @@ -719,6 +721,7 @@ break; case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_APOLLO_LAKE: bbs = (gcs >> 6) & 0x1; break; @@ -947,6 +950,11 @@ return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_C620_SERIES_LEWISBURG); }
+static int enable_flash_pch300(struct pci_dev *const dev, const char *const name) +{ + return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_300_SERIES_CANNON_POINT); +} + static int enable_flash_apl(struct pci_dev *const dev, const char *const name) { return enable_flash_pch100_or_c620(dev, name, 0x0d, 2, CHIPSET_APOLLO_LAKE); @@ -2027,6 +2035,16 @@ {0x8086, 0xa2c9, B_S, NT, "Intel", "Z370", enable_flash_pch100}, {0x8086, 0xa2d2, B_S, NT, "Intel", "X299", enable_flash_pch100}, {0x8086, 0x5ae8, B_S, DEP, "Intel", "Apollo Lake", enable_flash_apl}, + {0x8086, 0xa303, B_S, BAD, "Intel", "H310", enable_flash_pch300}, + {0x8086, 0xa304, B_S, BAD, "Intel", "H370", enable_flash_pch300}, + {0x8086, 0xa305, B_S, BAD, "Intel", "Z390", enable_flash_pch300}, + {0x8086, 0xa306, B_S, BAD, "Intel", "Q370", enable_flash_pch300}, + {0x8086, 0xa308, B_S, BAD, "Intel", "B360", enable_flash_pch300}, + {0x8086, 0xa309, B_S, BAD, "Intel", "C246", enable_flash_pch300}, + {0x8086, 0xa30a, B_S, BAD, "Intel", "C242", enable_flash_pch300}, + {0x8086, 0xa30c, B_S, BAD, "Intel", "QM370", enable_flash_pch300}, + {0x8086, 0xa30d, B_S, BAD, "Intel", "HM370", enable_flash_pch300}, + {0x8086, 0xa30e, B_S, BAD, "Intel", "CM246", enable_flash_pch300}, #endif {0}, }; diff --git a/programmer.h b/programmer.h index f4e8b46..4a51243 100644 --- a/programmer.h +++ b/programmer.h @@ -626,6 +626,7 @@ CHIPSET_9_SERIES_WILDCAT_POINT_LP, CHIPSET_100_SERIES_SUNRISE_POINT, /* also 6th/7th gen Core i/o (LP) variants */ CHIPSET_C620_SERIES_LEWISBURG, + CHIPSET_300_SERIES_CANNON_POINT, CHIPSET_APOLLO_LAKE, };
Hello Thomas Heijligen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/34071
to look at the new patch set (#2).
Change subject: chipset_enable: Add support for discrete Cannon Lake PCHs ......................................................................
chipset_enable: Add support for discrete Cannon Lake PCHs
The Cannon Lake "300 Series" PCHs [1,2] share the register layout of the Skylake "100 Series". Mark them as BAD until `ichspi.c` is adapted.
[1] Intel(R) 300 Series and Intel(R) C240 Series Chipset Family Platform Controller Hub Datasheet - Volume 1 of 2 Revison 4 (Dec 2018) Document Number 337347
[2] Intel(R) 300 Series Chipset Families Platform Controller Hub Datasheet - Volume 2 of 2 Revision 2? (Oct 2018) Document Number 337348
Change-Id: If0b54799d5b93169ee660409bad57ae14677340c Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com Signed-off-by: Nico Huber nico.huber@secunet.com --- M chipset_enable.c M programmer.h 2 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/34071/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34071 )
Change subject: chipset_enable: Add support for discrete Cannon Lake PCHs ......................................................................
Patch Set 2: Code-Review+1
entire patch train tested together
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34071 )
Change subject: chipset_enable: Add support for discrete Cannon Lake PCHs ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34071 )
Change subject: chipset_enable: Add support for discrete Cannon Lake PCHs ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/34071 )
Change subject: chipset_enable: Add support for discrete Cannon Lake PCHs ......................................................................
chipset_enable: Add support for discrete Cannon Lake PCHs
The Cannon Lake "300 Series" PCHs [1,2] share the register layout of the Skylake "100 Series". Mark them as BAD until `ichspi.c` is adapted.
[1] Intel(R) 300 Series and Intel(R) C240 Series Chipset Family Platform Controller Hub Datasheet - Volume 1 of 2 Revison 4 (Dec 2018) Document Number 337347
[2] Intel(R) 300 Series Chipset Families Platform Controller Hub Datasheet - Volume 2 of 2 Revision 2? (Oct 2018) Document Number 337348
Change-Id: If0b54799d5b93169ee660409bad57ae14677340c Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/34071 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Matt DeVillier matt.devillier@gmail.com Reviewed-by: Jeremy Soller jackpot51@gmail.com --- M chipset_enable.c M programmer.h 2 files changed, 19 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Matt DeVillier: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved Jeremy Soller: Looks good to me, but someone else must approve
diff --git a/chipset_enable.c b/chipset_enable.c index 8c884a8..877b8b6 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -598,6 +598,7 @@ break; case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_APOLLO_LAKE: reg_name = "BIOS_SPI_BC"; gcs = pci_read_long(dev, 0xdc); @@ -692,6 +693,7 @@ case CHIPSET_9_SERIES_WILDCAT_POINT_LP: case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_300_SERIES_CANNON_POINT: boot_straps = boot_straps_pch8_lp; break; case CHIPSET_APOLLO_LAKE: @@ -719,6 +721,7 @@ break; case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_APOLLO_LAKE: bbs = (gcs >> 6) & 0x1; break; @@ -947,6 +950,11 @@ return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_C620_SERIES_LEWISBURG); }
+static int enable_flash_pch300(struct pci_dev *const dev, const char *const name) +{ + return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_300_SERIES_CANNON_POINT); +} + static int enable_flash_apl(struct pci_dev *const dev, const char *const name) { return enable_flash_pch100_or_c620(dev, name, 0x0d, 2, CHIPSET_APOLLO_LAKE); @@ -2027,6 +2035,16 @@ {0x8086, 0xa2c9, B_S, NT, "Intel", "Z370", enable_flash_pch100}, {0x8086, 0xa2d2, B_S, NT, "Intel", "X299", enable_flash_pch100}, {0x8086, 0x5ae8, B_S, DEP, "Intel", "Apollo Lake", enable_flash_apl}, + {0x8086, 0xa303, B_S, BAD, "Intel", "H310", enable_flash_pch300}, + {0x8086, 0xa304, B_S, BAD, "Intel", "H370", enable_flash_pch300}, + {0x8086, 0xa305, B_S, BAD, "Intel", "Z390", enable_flash_pch300}, + {0x8086, 0xa306, B_S, BAD, "Intel", "Q370", enable_flash_pch300}, + {0x8086, 0xa308, B_S, BAD, "Intel", "B360", enable_flash_pch300}, + {0x8086, 0xa309, B_S, BAD, "Intel", "C246", enable_flash_pch300}, + {0x8086, 0xa30a, B_S, BAD, "Intel", "C242", enable_flash_pch300}, + {0x8086, 0xa30c, B_S, BAD, "Intel", "QM370", enable_flash_pch300}, + {0x8086, 0xa30d, B_S, BAD, "Intel", "HM370", enable_flash_pch300}, + {0x8086, 0xa30e, B_S, BAD, "Intel", "CM246", enable_flash_pch300}, #endif {0}, }; diff --git a/programmer.h b/programmer.h index dfa6ebd..34ef33d 100644 --- a/programmer.h +++ b/programmer.h @@ -626,6 +626,7 @@ CHIPSET_9_SERIES_WILDCAT_POINT_LP, CHIPSET_100_SERIES_SUNRISE_POINT, /* also 6th/7th gen Core i/o (LP) variants */ CHIPSET_C620_SERIES_LEWISBURG, + CHIPSET_300_SERIES_CANNON_POINT, CHIPSET_APOLLO_LAKE, };
-
Angel Pons (Code Review) -
Jeremy Soller (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)