- flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: chipset_enable: Add support for C620-series Lewisburg PCH
by build bot (Jenkins) (Code Review) 22 Aug '17

22 Aug '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 5: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/522/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/478/ : 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: 5 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: Tue, 22 Aug 2017 03:27:18 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chipset_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) 22 Aug '17

22 Aug '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 4: (9 comments) New results: https://paste.flashrom.org/view.php?id=3047 https://review.coreboot.org/#/c/20922/4//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20922/4//COMMIT_MSG@7 PS4, Line 7: sp > chi*ps*et oops - done. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@292 PS4, Line 292: desc->component.FLILL1 == 0) { > Just realized that you changed parentheses here. Which is wrong. Darn! It felt a bit awkward since I also had to invert logic and a add set of parens. I tried to make this a bit more idiot-proof by breaking apart the if-statements for FLILL/FLILL1 and the chipset. This way we only need to check the chipset once, and it will make it easier to use a switch statement later if we want. At the very least it should require fewer functioning brain cells when going thru the tedious effort of adding a new chipset. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@320 PS4, Line 320: unknown > Region 6 (counting from 0) is the Secondary BIOS region, according to I went ahead and named it BIOS2 as suggested (it was one of a few suggestions actually) on IRC. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@321 PS4, Line 321: EC > IIRC, this is "BMC" for Lewisburg. Maybe name it "EC/BMC"? Done https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@371 PS4, Line 371: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { > missing C620 here Done. I gave it its own special case since since we have a longer region name length (5 chars instead of 4). Here's how it looks (I'll post another example in case gerrit mangles this): --- Details --- FD BIOS ME GbE Pltf DE BIOS2 Reg7 BMC DE2 IE 10GbE OpROM Reg13 Reg14 Reg15 BIOS rw rw rw rw rw r ME r rw rw r GbE rw DE rw rw rw BMC rw IE rw https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: , > We have to add a sixth region, "unknown", or we'd run into the Yes, 4th region is DE. Since I'm not sharing the Sunrise Point code, I guess this can just be "BMC"? Though in the future some PCH that calls it "EC" will re-use the Lewisburg codepath, in which case we can change it. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@381 PS4, Line 381: msg_pdbg2(" FD BIOS ME GbE Pltf Reg5 Reg6 Reg7 EC Reg9\n"); > More regions, sigh. Done https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@1133 PS4, Line 1133: }; > I just remembered, here's a third list. Though, this one uses identi- I went ahead and updated this list for what I would expect ifdtool to use, though I haven't gone thru all of ifdtool yet. I thought access to unallocated space was generally forbidden. I'm not certain of that, though. https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr… File util/ich_descriptors_tool/ich_descriptors_tool.c: https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr… PS4, Line 48: }; > That's a lot of maintenance, maybe we should move the list from Yes, in another patch :-) -- 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: 4 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: Tue, 22 Aug 2017 03:26:47 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chipset_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) 22 Aug '17

22 Aug '17

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 (#5). Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... chipset_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, 119 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/20922/5 -- 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: 5 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]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) 22 Aug '17

22 Aug '17

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 4: (1 comment) https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@292 PS4, Line 292: desc->component.FLILL1 == 0) { Just realized that you changed parentheses here. Which is wrong. Now, `FLILL1 == 0` is checked on older platforms too and suffices to say "No forbidden opcodes". Maybe it becomes clearer if we invert the condition (and switch bodies): if (desc->component.FLILL != 0 || ((cs == CHIPSET_100_SERIES_SUNRISE_POINT || cs == CHIPSET_C620_SERIES_LEWISBURG) && desc->component.FLILL1 != 0)) { ... } else { msg_pdbg2("No forbidden opcodes.\n"); } -- 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: 4 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 22:01:26 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: ichspi: "Fix" access permission reporting for regions > 7
by Nico Huber (Code Review) 21 Aug '17

21 Aug '17

Nico Huber has submitted this change and it was merged. ( 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> Reviewed-on: https://review.coreboot.org/21106 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> --- M ichspi.c 1 file changed, 18 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved 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: merged Gerrit-Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Gerrit-Change-Number: 21106 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) 21 Aug '17

21 Aug '17

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 4: (2 comments) Grep'ed for 100_SERIES and spotted one more place where C620 is missing. https://review.coreboot.org/#/c/20922/4//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20922/4//COMMIT_MSG@7 PS4, Line 7: sp chi*ps*et https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@303 PS4, Line 303: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { missing C620 -- 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: 4 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 21:18:08 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) 21 Aug '17

21 Aug '17

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 4: (5 comments) https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@371 PS4, Line 371: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { missing C620 here https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: EC EC/BMC? https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: unknown 4th region is Device Expansion, DE? https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: , We have to add a sixth region, "unknown", or we'd run into the check below. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@381 PS4, Line 381: msg_pdbg2(" FD BIOS ME GbE Pltf Reg5 Reg6 Reg7 EC Reg9\n"); More regions, sigh. -- 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: 4 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 20:44:39 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) 21 Aug '17

21 Aug '17

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 4: > I happen to have a Lewisburg reference board handy, here is the > output with PS4: https://paste.flashrom.org/view.php?id=3046 Cool. This looks odd: > Descr. BIOS ME GbE Platf. > BIOS w w r I'll have closer look at the 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: 4 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 20:33:23 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) 21 Aug '17

21 Aug '17

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 4: (4 comments) The list in layout_from_ich_descriptors() should be updated too. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@320 PS4, Line 320: unknown Region 6 (counting from 0) is the Secondary BIOS region, according to some register's descriptions that control it. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@321 PS4, Line 321: EC IIRC, this is "BMC" for Lewisburg. Maybe name it "EC/BMC"? https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@1133 PS4, Line 1133: }; I just remembered, here's a third list. Though, this one uses identi- fiers compatible to the ones in ifdtool from coreboot. The -N and --ifd options could also be helpful to make flashrom work with unallocated space in the chip, like the gaps Sandy has (as far as I followed the discussion). https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr… File util/ich_descriptors_tool/ich_descriptors_tool.c: https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr… PS4, Line 48: }; That's a lot of maintenance, maybe we should move the list from pprint_freg() into the header file and use it instead? -- 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: 4 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 20:24:11 +0000 Gerrit-HasComments: Yes

1 0

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

21 Aug '17

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 4: I forgot to add the region descriptions in ichspi.c, so PS4 adds them. Also, "GbE" is used for consistency (instead of "GBE"). I happen to have a Lewisburg reference board handy, here is the output with PS4: https://paste.flashrom.org/view.php?id=3046 -- 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: 4 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 20:10:35 +0000 Gerrit-HasComments: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit