November 2021 - flashrom-gerrit

Change in flashrom[master]: chipset_enable.c: Mark Intel Z390 as DEP
by Nico Huber (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Melvyn, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58774 ) Change subject: chipset_enable.c: Mark Intel Z390 as DEP ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/58774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If14d45c144bb32a1d1046185d4476ea29e4d0912 Gerrit-Change-Number: 58774 Gerrit-PatchSet: 4 Gerrit-Owner: Melvyn <melvyn2(a)brcok.tk> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Melvyn <melvyn2(a)brcok.tk> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 12:23:54 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Tiger Lake U Premium support
by Nico Huber (Code Review) 17 Nov '21

17 Nov '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55578 ) Change subject: Add Tiger Lake U Premium support ...................................................................... Add Tiger Lake U Premium support Tiger Lake has very low ICCRIBA (TGL=0x11, CNL=0x34 and CML=0x34) and detects as unknown chipset compatible with 300 series chipset. Add a new enum CHIPSET_500_SERIES_TIGER_POINT and treat it identically to CHIPSET_400_SERIES_COMET_POINT. There are some exceptions though, ICCRIBA is no longer present n descriptor content so a new union has been defined for new fields and used in descriptor guessing. freq_read field is not present on Tiger Lake, moreover in CannonPoint and Comet Point this field is used as eSPI/EC frequency, so a new function to print read frequency has ben added. Finally Tiger lake boot straps include eSPI, so a new bus has been added for the new straps. TEST=Flash BIOS region on Intel i5-1135G7 Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Reviewed-on: https://review.coreboot.org/c/flashrom/+/55578 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M chipset_enable.c M ich_descriptors.c M ich_descriptors.h M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 6 files changed, 92 insertions(+), 8 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/chipset_enable.c b/chipset_enable.c index cb0fbf1..26db472 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -600,6 +600,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: reg_name = "BIOS_SPI_BC"; @@ -653,6 +654,9 @@ static const struct boot_straps boot_straps_pch8_lp[] = { { "SPI", BUS_SPI }, { "LPC", BUS_LPC | BUS_FWH } }; + static const struct boot_straps boot_straps_pch500[] = + { { "SPI", BUS_SPI }, + { "eSPI", BUS_NONE } }; static const struct boot_straps boot_straps_apl[] = { { "SPI", BUS_SPI }, { "reserved", BUS_NONE } }; @@ -699,6 +703,9 @@ case CHIPSET_400_SERIES_COMET_POINT: boot_straps = boot_straps_pch8_lp; break; + case CHIPSET_500_SERIES_TIGER_POINT: + boot_straps = boot_straps_pch500; + break; case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: boot_straps = boot_straps_apl; @@ -727,6 +734,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: bbs = (gcs >> 6) & 0x1; @@ -974,6 +982,11 @@ return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_COMET_POINT); } +static int enable_flash_pch500(struct pci_dev *const dev, const char *const name) +{ + return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_500_SERIES_TIGER_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); @@ -2029,6 +2042,7 @@ {0x8086, 0x9d84, B_S, DEP, "Intel", "Cannon Lake U Premium", enable_flash_pch300}, {0x8086, 0x0284, B_S, DEP, "Intel", "Comet Lake U Premium", enable_flash_pch400}, {0x8086, 0x0285, B_S, DEP, "Intel", "Comet Lake U Base", enable_flash_pch400}, + {0x8086, 0xa082, B_S, DEP, "Intel", "Tiger Lake U Premium", enable_flash_pch500}, {0x8086, 0xa141, B_S, NT, "Intel", "Sunrise Point Desktop Sample", enable_flash_pch100}, {0x8086, 0xa142, B_S, NT, "Intel", "Sunrise Point Unknown Sample", enable_flash_pch100}, {0x8086, 0xa143, B_S, DEP, "Intel", "H110", enable_flash_pch100}, diff --git a/ich_descriptors.c b/ich_descriptors.c index 5edaaf4..56a3c67 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -45,6 +45,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: return 16; case CHIPSET_100_SERIES_SUNRISE_POINT: return 10; @@ -108,7 +109,7 @@ "8 series Lynx Point", "Baytrail", "8 series Lynx Point LP", "8 series Wellsburg", "9 series Wildcat Point", "9 series Wildcat Point LP", "100 series Sunrise Point", "C620 series Lewisburg", "300 series Cannon Point", "400 series Comet Point", - "Apollo Lake", "Gemini Lake", + "500 series Tiger Point", "Apollo Lake", "Gemini Lake", }; if (cs < CHIPSET_ICH8 || cs - CHIPSET_ICH8 + 1 >= ARRAY_SIZE(chipset_names)) cs = 0; @@ -202,6 +203,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: { uint8_t size_enc; @@ -222,7 +224,7 @@ static const char *pprint_freq(enum ich_chipset cs, uint8_t value) { - static const char *const freq_str[3][8] = { { + static const char *const freq_str[4][8] = { { "20 MHz", "33 MHz", "reserved", @@ -249,7 +251,16 @@ "reserved", "14 MHz / 17 MHz", "reserved" - } }; + }, { + "100 MHz", + "50 MHz", + "reserved", + "33 MHz", + "25 MHz", + "reserved", + "14 MHz", + "reserved" + }}; switch (cs) { case CHIPSET_ICH8: @@ -276,12 +287,41 @@ case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: return freq_str[2][value]; + case CHIPSET_500_SERIES_TIGER_POINT: + return freq_str[3][value]; case CHIPSET_ICH_UNKNOWN: default: return "unknown"; } } +static void pprint_read_freq(enum ich_chipset cs, uint8_t value) +{ + static const char *const freq_str[1][8] = { { + "20 MHz", + "24 MHz", + "30 MHz", + "48 MHz", + "60 MHz", + "reserved", + "reserved", + "reserved" + }}; + + switch (cs) { + case CHIPSET_300_SERIES_CANNON_POINT: + case CHIPSET_400_SERIES_COMET_POINT: + msg_pdbg2("eSPI/EC Bus Clock Frequency: %s\n", freq_str[0][value]); + return; + case CHIPSET_500_SERIES_TIGER_POINT: + msg_pdbg2("Read Clock Frequency: %s\n", "reserved"); + return; + default: + msg_pdbg2("Read Clock Frequency: %s\n", pprint_freq(cs, value)); + return; + } +} + void prettyprint_ich_descriptor_component(enum ich_chipset cs, const struct ich_descriptors *desc) { bool has_flill1; @@ -291,6 +331,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: has_flill1 = true; @@ -313,7 +354,9 @@ msg_pdbg2("Component 2 density: %s\n", pprint_density(cs, desc, 1)); else msg_pdbg2("Component 2 is not used.\n"); - msg_pdbg2("Read Clock Frequency: %s\n", pprint_freq(cs, desc->component.modes.freq_read)); + + pprint_read_freq(cs, desc->component.modes.freq_read); + msg_pdbg2("Read ID and Status Clock Freq.: %s\n", pprint_freq(cs, desc->component.modes.freq_read_id)); msg_pdbg2("Write and Erase Clock Freq.: %s\n", pprint_freq(cs, desc->component.modes.freq_write)); msg_pdbg2("Fast Read is %ssupported.\n", desc->component.modes.fastread ? "" : "not "); @@ -411,7 +454,8 @@ msg_pdbg2("--- Details ---\n"); if (cs == CHIPSET_100_SERIES_SUNRISE_POINT || cs == CHIPSET_300_SERIES_CANNON_POINT || - cs == CHIPSET_400_SERIES_COMET_POINT) { + cs == CHIPSET_400_SERIES_COMET_POINT || + cs == CHIPSET_500_SERIES_TIGER_POINT) { const char *const master_names[] = { "BIOS", "ME", "GbE", "unknown", "EC", }; @@ -970,8 +1014,10 @@ } else { if (content->ICCRIBA == 0x34) return CHIPSET_300_SERIES_CANNON_POINT; - msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); - return CHIPSET_300_SERIES_CANNON_POINT; + if (content->CSSL == 0x11) + return CHIPSET_500_SERIES_TIGER_POINT; + msg_pwarn("Unknown flash descriptor, assuming 500 series compatibility.\n"); + return CHIPSET_500_SERIES_TIGER_POINT; } } @@ -990,6 +1036,7 @@ switch (guess) { case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_GEMINI_LAKE: /* `freq_read` was repurposed, so can't check on it any more. */ break; @@ -1144,6 +1191,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: if (idx == 0) { @@ -1181,6 +1229,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: mmio_le_writel(control, spibar + PCH100_REG_FDOC); diff --git a/ich_descriptors.h b/ich_descriptors.h index d00af63..cd5a45c 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -95,6 +95,13 @@ ICCRIBA :8, /* ICC Reg. Init Base Addr. (new since Sandy Bridge) */ RIL :8; /* Register Init Length (new since Hawell) */ }; + struct { /* new since Tiger Point */ + uint32_t :2, + CSSO :10, /* CPU Soft Strap Offset from PMC Base */ + :4, + CSSL :8, /* CPU Soft Strap Length */ + :8; + }; }; }; diff --git a/ichspi.c b/ichspi.c index d345af2..290deaa 100644 --- a/ichspi.c +++ b/ichspi.c @@ -426,6 +426,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: break; default: pprint_reg(HSFS, BERASE, reg_val, ", "); @@ -437,6 +438,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: pprint_reg(HSFS, PRR34_LOCKDN, reg_val, ", "); pprint_reg(HSFS, WRSDIS, reg_val, ", "); break; @@ -457,6 +459,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: _pprint_reg(HSFC, PCH100_HSFC_FCYCLE, PCH100_HSFC_FCYCLE_OFF, reg_val, ", "); pprint_reg(HSFC, WET, reg_val, ", "); break; @@ -1735,6 +1738,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: num_pr = 6; /* Includes GPR0 */ @@ -1768,6 +1772,7 @@ break; case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: num_freg = 16; @@ -1865,6 +1870,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: tmp = mmio_readl(spibar + PCH100_REG_DLOCK); @@ -1941,6 +1947,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_BAYTRAIL: @@ -1975,6 +1982,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: + case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: break; @@ -2008,7 +2016,8 @@ if (ich_spi_mode == ich_auto && (ich_gen == CHIPSET_100_SERIES_SUNRISE_POINT || ich_gen == CHIPSET_300_SERIES_CANNON_POINT || - ich_gen == CHIPSET_400_SERIES_COMET_POINT)) { + ich_gen == CHIPSET_400_SERIES_COMET_POINT || + ich_gen == CHIPSET_500_SERIES_TIGER_POINT)) { msg_pdbg("Enabling hardware sequencing by default for 100+ series PCH.\n"); ich_spi_mode = ich_hwseq; } diff --git a/programmer.h b/programmer.h index ec9503c..b4f04e7 100644 --- a/programmer.h +++ b/programmer.h @@ -359,6 +359,7 @@ CHIPSET_C620_SERIES_LEWISBURG, CHIPSET_300_SERIES_CANNON_POINT, CHIPSET_400_SERIES_COMET_POINT, + CHIPSET_500_SERIES_TIGER_POINT, CHIPSET_APOLLO_LAKE, CHIPSET_GEMINI_LAKE, }; diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index 0807f5e..a1e353b 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -136,6 +136,7 @@ "\t- \"100\" or \"sunrise\" for Intel's 100 series chipsets.\n" "\t- \"300\" or \"cannon\" for Intel's 300 series chipsets.\n" "\t- \"400\" or \"comet\" for Intel's 400 series chipsets.\n" +"\t- \"500\" or \"tiger\" for Intel's 500 series chipsets.\n" "If '-d' is specified some regions such as the BIOS image as seen by the CPU or\n" "the GbE blob that is required to initialize the GbE are also dumped to files.\n", argv[0], argv[0]); @@ -230,6 +231,9 @@ else if ((strcmp(csn, "400") == 0) || (strcmp(csn, "comet") == 0)) cs = CHIPSET_400_SERIES_COMET_POINT; + else if ((strcmp(csn, "500") == 0) || + (strcmp(csn, "tiger") == 0)) + cs = CHIPSET_500_SERIES_TIGER_POINT; else if (strcmp(csn, "apollo") == 0) cs = CHIPSET_APOLLO_LAKE; else if (strcmp(csn, "gemini") == 0) -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 15 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: Add Tiger Lake U Premium support
by Nico Huber (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Tim Crawford, Michał Żygowski, David Hendricks. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 ) Change subject: Add Tiger Lake U Premium support ...................................................................... Patch Set 14: Code-Review+2 (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55578/comment/fbc20a81_d2a85239 PS13, Line 1016: if (content->CSSL != 0x11) > Okay, done. Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 14 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 12:06:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Tiger Lake U Premium support
by Michał Żygowski (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Tim Crawford, Nico Huber, David Hendricks. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 ) Change subject: Add Tiger Lake U Premium support ...................................................................... Patch Set 14: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55578/comment/1ed545c2_e3e9ef1e PS13, Line 1016: if (content->CSSL != 0x11) > AIUI, David's wants an explicit check+return for each platform. So this should become […] Okay, done. -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 14 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 11:47:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Tiger Lake U Premium support
by Michał Żygowski (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Tim Crawford, Michał Żygowski, David Hendricks. Hello build bot (Jenkins), Tim Crawford, Nico Huber, David Hendricks, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55578 to look at the new patch set (#14). Change subject: Add Tiger Lake U Premium support ...................................................................... Add Tiger Lake U Premium support Tiger Lake has very low ICCRIBA (TGL=0x11, CNL=0x34 and CML=0x34) and detects as unknown chipset compatible with 300 series chipset. Add a new enum CHIPSET_500_SERIES_TIGER_POINT and treat it identically to CHIPSET_400_SERIES_COMET_POINT. There are some exceptions though, ICCRIBA is no longer present n descriptor content so a new union has been defined for new fields and used in descriptor guessing. freq_read field is not present on Tiger Lake, moreover in CannonPoint and Comet Point this field is used as eSPI/EC frequency, so a new function to print read frequency has ben added. Finally Tiger lake boot straps include eSPI, so a new bus has been added for the new straps. TEST=Flash BIOS region on Intel i5-1135G7 Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd --- M chipset_enable.c M ich_descriptors.c M ich_descriptors.h M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 6 files changed, 92 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/55578/14 -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 14 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: pcidev: Move pci_get_dev() logic into canonical place
by Peter Marheine (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 ) Change subject: pcidev: Move pci_get_dev() logic into canonical place ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/59276 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7 Gerrit-Change-Number: 59276 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 17 Nov 2021 03:42:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Validate before allocate in verify_range()
by Edward O'Callaghan (Code Review) 17 Nov '21

17 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59372 ) Change subject: flashrom.c: Validate before allocate in verify_range() ...................................................................... flashrom.c: Validate before allocate in verify_range() Simplify a goto away for free'ing a buffer by validating before attempting to allocate. BUG=none TEST=builds Change-Id: Iae886f203d1c59ae9a89421f7483a4ec3f747256 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 8 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/59372/1 diff --git a/flashrom.c b/flashrom.c index 48d953b..b62d38c 100644 --- a/flashrom.c +++ b/flashrom.c @@ -417,6 +417,13 @@ if (!len) return -1; + if (start + len > flash->chip->total_size * 1024) { + msg_gerr("Error: %s called with start 0x%x + len 0x%x >" + " total_size 0x%x\n", __func__, start, len, + flash->chip->total_size * 1024); + return -1; + } + if (!flash->chip->read) { msg_cerr("ERROR: flashrom has no read function for this flash chip.\n"); return -1; @@ -427,17 +434,8 @@ msg_gerr("Could not allocate memory!\n"); return -1; } - int ret = 0; - if (start + len > flash->chip->total_size * 1024) { - msg_gerr("Error: %s called with start 0x%x + len 0x%x >" - " total_size 0x%x\n", __func__, start, len, - flash->chip->total_size * 1024); - ret = -1; - goto out_free; - } - - ret = flash->chip->read(flash, readbuf, start, len); + int ret = flash->chip->read(flash, readbuf, start, len); if (ret) { msg_gerr("Verification impossible because read failed " "at 0x%x (len 0x%x)\n", start, len); -- To view, visit https://review.coreboot.org/c/flashrom/+/59372 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iae886f203d1c59ae9a89421f7483a4ec3f747256 Gerrit-Change-Number: 59372 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pcidev: Move pci_get_dev() logic into canonical place
by Anastasia Klimchuk (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 ) Change subject: pcidev: Move pci_get_dev() logic into canonical place ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/59276 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7 Gerrit-Change-Number: 59276 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 17 Nov 2021 02:29:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dmi.c: Hide has_dmi_support global behind method
by Anastasia Klimchuk (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59283 ) Change subject: dmi.c: Hide has_dmi_support global behind method ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/59283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibded9714998ea6f2e5d4e0512fa7c6b105f9638a Gerrit-Change-Number: 59283 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 17 Nov 2021 01:59:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] tests: test write protection
by Anastasia Klimchuk (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59075 ) Change subject: [RFC] tests: test write protection ...................................................................... Patch Set 5: (13 comments) Patchset: PS5: Thank you for working on tests!! I did more detailed code review. I didn't look into previous patches in this chain, but tell me if I should. File tests/meson.build: https://review.coreboot.org/c/flashrom/+/59075/comment/1b8079ce_3cc9d432 PS5, Line 26: write_protect I know I asked you to rename the file, but now I did more detailed review and I think the better name would be chip_wp.c, since there are similarities with existing chip.c. How bad is to rename it again? :) And old name is still in tests.c, tests.h so you would need to go through the code once more anyway. Thanks!! File tests/tests.h: https://review.coreboot.org/c/flashrom/+/59075/comment/bb96dfa6_a862f7c5 PS5, Line 67: write_protection.c chip_wp.c File tests/tests.c: https://review.coreboot.org/c/flashrom/+/59075/comment/1115a91d_69922325 PS5, Line 357: write_protection chip_wp https://review.coreboot.org/c/flashrom/+/59075/comment/8fe473a5_2f91010c PS5, Line 364: write_protection.c chip_wp.c File tests/write_protect.c: https://review.coreboot.org/c/flashrom/+/59075/comment/3f5c0b70_e44e4e6e PS5, Line 27: No mocking. Using WP emulation in dummy programmer. Maybe a complete phrase is better, like "Tests in this file do not use any mocking, because using write-protect emulation in dummyflasher programmer is sufficient". https://review.coreboot.org/c/flashrom/+/59075/comment/fb8b3054_3c2f6241 PS5, Line 101: invalid_setup We can be more specific in test name, since it covers invalid range (not just any invalid setup). Invalid wp range, to be even more specific. So maybe invalid_wp_range_dummyflasher_test_success https://review.coreboot.org/c/flashrom/+/59075/comment/37d9d026_95013534 PS5, Line 117: struct flashrom_wp_chip_config cfg; Lets move this declaration in the beginning , together with flash, layout, mock_chip. (And the same for other test methods) https://review.coreboot.org/c/flashrom/+/59075/comment/3afbe98d_8de24338 PS5, Line 120: /* Invalid range. */ : range.start = 0x1000; : range.len = 0x1000; why chip_len is initialised at creation time, but start/len initialised separately, few lines later? they are initialised from constants anyway. https://review.coreboot.org/c/flashrom/+/59075/comment/3fddf531_213b8902 PS5, Line 143: wp=no I haven't looked at the rest of your chain (tell me if I should), but this looks confusing: param says "wp=no", but test name is "setup_wp". So it wp enabled or disabled? what are you testing? https://review.coreboot.org/c/flashrom/+/59075/comment/4c246984_b2c4c0e6 PS5, Line 152: /* Use first 4 KiB for a range. */ : range.len = 0x1000; : assert_int_equal(0, flashrom_wp_set_range(&cfg, &range)); : : /* Change range to last 4 KiB. */ : range.start = 0x00fff000; : assert_int_equal(0, flashrom_wp_set_range(&cfg, &range)); This code looks like as if test is testing two scenarios? If there are indeed two different scenarios, you need to split into two tests. Each test would init wp_range once (and init the rest) and then run scenario. To have multiple assertions in one test is totally fine. But this looks like: "lets setup a range, test it works... now lets setup a different range, test it works" which should be two tests. https://review.coreboot.org/c/flashrom/+/59075/comment/0557a3de_ab8f252e PS5, Line 164: /* Switch modes in configuration only. */ : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_HARDWARE)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_DISABLED)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_PERMANENT)); Do all these need flash context, chip, layout? If not, extract into separate small tests that only declare `struct flashrom_wp_chip_config cfg;` and then do *one* assertion. Yes, you will have many small tests! This is perfect :) Small granular tests are very easy to debug, and easy to understand what's broken when they fail on someone's patch. https://review.coreboot.org/c/flashrom/+/59075/comment/2a56f918_722059f9 PS5, Line 170: /* Switch modes in dummyflasher. */ : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : : /* Check final mode. */ : assert_int_equal(0, flashrom_wp_read_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode)); : assert_int_equal(WP_MODE_PERMANENT, mode); Which ones of these need layout, if any? Which ones of these need dummyflasher? Do these actions have to be performed in this sequence, or are they independent? Independent actions need to be independent tests. Also for setup, you don't need to invoke heavy-weight setup if it's not needed. Ideally you only setup what you need for the given test. Same questions apply to "wp_init_from_status_dummyflasher_test_success". -- To view, visit https://review.coreboot.org/c/flashrom/+/59075 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I49af7f6d173eb4c56c22d80b01a473b8c499c0f8 Gerrit-Change-Number: 59075 Gerrit-PatchSet: 5 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 01:32:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2021