- flashrom-gerrit - mail.coreboot.org

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

19 Aug '17

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) 19 Aug '17

19 Aug '17

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) 19 Aug '17

19 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 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) 19 Aug '17

19 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 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) 19 Aug '17

19 Aug '17

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

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

19 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 2: (1 comment) https://review.coreboot.org/#/c/20922/2/ichspi.c File ichspi.c: 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 am missing something... -- 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 03:15:59 +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) 19 Aug '17

19 Aug '17

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 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/515/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/473/ : 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: 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 03:14:42 +0000 Gerrit-HasComments: No

1 0

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

19 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 (#2). 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 list. 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, 81 insertions(+), 20 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/20922/2 -- 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: 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>

1 0

Change in flashrom[refs/meta/config]: Refine rights to create/delete any branches
by Patrick Georgi (Code Review) 18 Aug '17

18 Aug '17

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/21090 ) Change subject: Refine rights to create/delete any branches ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/21090 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-MessageType: comment Gerrit-Change-Id: Ia8ad1dc3dcdb6e35bfb47cbe437f2f7ea1cc560b Gerrit-Change-Number: 21090 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 18 Aug 2017 11:47:01 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[refs/meta/config]: Refine rights to create/delete any branches
by Stefan Tauner (Code Review) 18 Aug '17

18 Aug '17

Stefan Tauner has abandoned this change. ( https://review.coreboot.org/21089 ) Change subject: Refine rights to create/delete any branches ...................................................................... Abandoned webkacke. -- To view, visit https://review.coreboot.org/21089 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-MessageType: abandon Gerrit-Change-Id: Iee7ced1353cf354f5284281929f4d4a4095ab0fe Gerrit-Change-Number: 21089 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit