August 2017 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chispet_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 3: (5 comments) https://review.coreboot.org/#/c/20922/2/chipset_enable.c File chipset_enable.c: https://review.coreboot.org/#/c/20922/2/chipset_enable.c@836 PS2, Line 836: or_ > s/and/or/? (ich you read the identifier as a statement...) Done https://review.coreboot.org/#/c/20922/2/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/2/ich_descriptors.c@76 PS2, Line 76: default: : if (cont->N > You could remove both else paths and just return -1 at the end. Done. https://review.coreboot.org/#/c/20922/2/ichspi.c File ichspi.c: https://review.coreboot.org/#/c/20922/2/ichspi.c@398 PS2, Line 398: ich_generation == CHIPSET_C620_SERIES_LEWISBURG) { > Please don't align the indendation with the body. Done. BTW - I wonder if we should change most of these `if (ich_generation == ...)` statements to `switch (ich_generation)`. That would solve any indentation issue now and moving forward, and would also be more uniform with other parts of the ichspi and ich_desctriptors code that have to deal with multiple generations. https://review.coreboot.org/#/c/20922/2/ichspi.c@1703 PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */ > 10 is correct, the registers read with 0x00007fff just what is set So should we leave it at 12 to match the datasheet, even though FREG10 and 11 read 0 and are ignored? Should we explain this behavior in the comment? LMK your thoughts on how to proceed from here. https://review.coreboot.org/#/c/20922/2/ichspi.c@1704 PS2, Line 1704: 6; /* Includes GPR0 */ > 6 including the GPR0 register, sorry should have added a comment... Ah, that makes sense. I went ahead and added a comment for Sunrise Point and Lewisburg. ICH8/9 don't have a GPR register, so 5 is already correct to account for PR0-PR4. -- To view, visit https://review.coreboot.org/20922 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b Gerrit-Change-Number: 20922 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 21 Aug 2017 04:41:32 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by build bot (Jenkins) (Code Review) Aug. 21, 2017

Aug. 21, 2017

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chispet_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 3: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/519/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/476/ : SUCCESS -- To view, visit https://review.coreboot.org/20922 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b Gerrit-Change-Number: 20922 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 21 Aug 2017 04:40:50 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/20922 to look at the new patch set (#3). Change subject: chispet_enable: Add support for C620-series Lewisburg PCH ...................................................................... chispet_enable: Add support for C620-series Lewisburg PCH This adds PCI IDs for C620-series PCHs and adds CHIPSET_C620_SERIES_LEWISBURG as a new entry in the ich_chipset enum. Lewisburg is very similar to Sunrise Point for Flashrom's purposes, however one important difference is the way the "number of masters" is interpreted from the flash descriptor (0-based vs. 1-based). Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b Signed-off-by: David Hendricks <dhendricks(a)fb.com> --- M chipset_enable.c M ich_descriptors.c M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 5 files changed, 80 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/20922/3 -- To view, visit https://review.coreboot.org/20922 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newpatchset Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b Gerrit-Change-Number: 20922 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: chipset_enable: Mark Braswell as tested
by Nico Huber (Code Review) Aug. 19, 2017

Aug. 19, 2017

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/20923 ) Change subject: chipset_enable: Mark Braswell as tested ...................................................................... chipset_enable: Mark Braswell as tested Reported by Uwe Vieweg: https://mail.coreboot.org/pipermail/flashrom/2017-August/015059.html Change-Id: Iaf7558af8737af36401f577ca7aba9fd7114a3df Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> Reviewed-on: https://review.coreboot.org/20923 Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M chipset_enable.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Paul Menzel: Looks good to me, but someone else must approve Nico Huber: Looks good to me, approved build bot (Jenkins): Verified diff --git a/chipset_enable.c b/chipset_enable.c index 6a93d0d..36e2838 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1740,7 +1740,7 @@ {0x8086, 0x1f39, NT, "Intel", "Avoton/Rangeley", enable_flash_silvermont}, {0x8086, 0x1f3a, NT, "Intel", "Avoton/Rangeley", enable_flash_silvermont}, {0x8086, 0x1f3b, NT, "Intel", "Avoton/Rangeley", enable_flash_silvermont}, - {0x8086, 0x229c, NT, "Intel", "Braswell", enable_flash_silvermont}, + {0x8086, 0x229c, OK, "Intel", "Braswell", enable_flash_silvermont}, {0x8086, 0x2310, NT, "Intel", "DH89xxCC (Cave Creek)", enable_flash_pch7}, {0x8086, 0x2390, NT, "Intel", "Coleto Creek", enable_flash_pch7}, {0x8086, 0x2410, OK, "Intel", "ICH", enable_flash_ich0}, -- To view, visit https://review.coreboot.org/20923 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Iaf7558af8737af36401f577ca7aba9fd7114a3df Gerrit-Change-Number: 20923 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: chipset_enable: Mark Braswell as tested
by build bot (Jenkins) (Code Review) Aug. 19, 2017

Aug. 19, 2017

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20923 ) Change subject: chipset_enable: Mark Braswell as tested ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/517/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/475/ : SUCCESS -- To view, visit https://review.coreboot.org/20923 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iaf7558af8737af36401f577ca7aba9fd7114a3df Gerrit-Change-Number: 20923 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 19 Aug 2017 20:34:14 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ichspi: "Fix" access permission reporting for regions > 7
by build bot (Jenkins) (Code Review) Aug. 19, 2017

Aug. 19, 2017

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21106 ) Change subject: ichspi: "Fix" access permission reporting for regions > 7 ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/516/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/474/ : SUCCESS -- To view, visit https://review.coreboot.org/21106 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Gerrit-Change-Number: 21106 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 19 Aug 2017 15:07:38 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ichspi: "Fix" access permission reporting for regions > 7
by Nico Huber (Code Review) Aug. 19, 2017

Aug. 19, 2017

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21106 Change subject: ichspi: "Fix" access permission reporting for regions > 7 ...................................................................... ichspi: "Fix" access permission reporting for regions > 7 Can't find bits that tell us the actual permissions in charge. So report them as unknown. Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M ichspi.c 1 file changed, 18 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/21106/1 diff --git a/ichspi.c b/ichspi.c index 5f3e394..9ee8044 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1543,6 +1543,7 @@ static const char *const access_names[4] = { "locked", "read-only", "write-only", "read-write" }; + const int rwperms_unknown = ARRAY_SIZE(access_names); static const char *const region_names[5] = { "Flash Descriptor", "BIOS", "Management Engine", "Gigabit Ethernet", "Platform Data" @@ -1550,10 +1551,20 @@ const char *const region_name = i < ARRAY_SIZE(region_names) ? region_names[i] : "unknown"; uint32_t base, limit; - int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) | - (((ICH_BRRA(frap) >> i) & 1) << 0); + int rwperms; int offset = ICH9_REG_FREG0 + i * 4; uint32_t freg = mmio_readl(ich_spibar + offset); + + if (i < 8) { + rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) | + (((ICH_BRRA(frap) >> i) & 1) << 0); + } else { + /* Datasheets don't define any access bits for regions > 7. We + can't rely on the actual descriptor settings either as there + are several overrides for them (those by other masters are + not even readable by us, *shrug*). */ + rwperms = rwperms_unknown; + } base = ICH_FREG_BASE(freg); limit = ICH_FREG_LIMIT(freg); @@ -1569,6 +1580,11 @@ region_name, base, limit, access_names[rwperms]); return 0; } + if (rwperms == rwperms_unknown) { + msg_pdbg("FREG%i: %s region (0x%08x-0x%08x) has unknown permissions.\n", + i, region_name, base, limit); + return 0; + } msg_pwarn("FREG%i: Warning: %s region (0x%08x-0x%08x) is %s.\n", i, region_name, base, limit, access_names[rwperms]); -- To view, visit https://review.coreboot.org/21106 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newchange Gerrit-Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Gerrit-Change-Number: 21106 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) Aug. 19, 2017

Aug. 19, 2017

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chispet_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 2: (3 comments) https://review.coreboot.org/#/c/20922/2/chipset_enable.c File chipset_enable.c: https://review.coreboot.org/#/c/20922/2/chipset_enable.c@836 PS2, Line 836: and s/and/or/? (ich you read the identifier as a statement...) https://review.coreboot.org/#/c/20922/2/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/2/ich_descriptors.c@76 PS2, Line 76: else : return -1; You could remove both else paths and just return -1 at the end. https://review.coreboot.org/#/c/20922/2/ichspi.c File ichspi.c: https://review.coreboot.org/#/c/20922/2/ichspi.c@1703 PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */ > Depending on what the regular PCH returns for the register addresses, 10 is correct, the registers read with 0x00007fff just what is set in the descriptor. 12 would work too for the regular PCH (registers read 0 and are ignored by the current code). -- To view, visit https://review.coreboot.org/20922 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b Gerrit-Change-Number: 20922 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 19 Aug 2017 13:45:19 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) Aug. 19, 2017

Aug. 19, 2017

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chispet_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 2: (5 comments) https://review.coreboot.org/#/c/20922/2/ichspi.c File ichspi.c: https://review.coreboot.org/#/c/20922/2/ichspi.c@398 PS2, Line 398: ich_generation == CHIPSET_C620_SERIES_LEWISBURG) { Please don't align the indendation with the body. https://review.coreboot.org/#/c/20922/2/ichspi.c@1557 PS2, Line 1557: (((ICH_BRRA(frap) >> i) & 1) << 0); Overflow for i >= 8 (have a fix in progress somewhere). https://review.coreboot.org/#/c/20922/2/ichspi.c@1600 PS2, Line 1600: /* From 5 on we have GPR registers and start from 0 again. */ Oh, here's the comment ;) https://review.coreboot.org/#/c/20922/2/ichspi.c@1703 PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */ Depending on what the regular PCH returns for the register addresses, 12 might not hurt there either. I'm not sure any more why I set it to 10, maybe based on some observation rather than the datasheet... in the end, we only dump the registers anyway. https://review.coreboot.org/#/c/20922/2/ichspi.c@1704 PS2, Line 1704: 6; /* FIXME: should be 5? */ > num_pr = 6 seemed wrong for both Sunrise Point and Lewisburg, but maybe I a 6 including the GPR0 register, sorry should have added a comment... -- To view, visit https://review.coreboot.org/20922 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b Gerrit-Change-Number: 20922 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 19 Aug 2017 13:03:20 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: ich_descriptors: Fix off-by-one error
by David Hendricks (Code Review) Aug. 19, 2017

Aug. 19, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20937 ) Change subject: ich_descriptors: Fix off-by-one error ...................................................................... Patch Set 1: Code-Review-2 Out of caution, I went ahead and added Lewisburg as its own ICH in https://review.coreboot.org/#/c/20922/ . So far the only notable differences I've come across are the number of masters and maybe the number of protected range registers. Let's discuss further in 20922. -- To view, visit https://review.coreboot.org/20937 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I92627332265cf79720b98241d73bc36b1f6a7167 Gerrit-Change-Number: 20937 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 19 Aug 2017 03:18:13 +0000 Gerrit-HasComments: No

1 0