- flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: chipset_enable: Add support for Intel Skylake
by Nico Huber (Code Review) 27 Apr '17

27 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/18925 ) Change subject: chipset_enable: Add support for Intel Skylake ...................................................................... Patch Set 3: (2 comments) https://review.coreboot.org/#/c/18925/2/chipset_enable.c File chipset_enable.c: Line 852: > Here, I would do : > Here, I would do : > pci_get_dev(pci_acc, dev->domain, dev->bus, dev->dev, 5); Hmmm, 1f.5 (or rather D31F5) is what one would read in a datasheet and I don't want to make it less recognizable. > Might even make that '5' into a #define to avoid having the magic > number here. > #define PCH_100_SPI_INTERFACE_PCI_FUNCTION I would agree if that number would be used somewhere else to. But as long as that's not the case, I'd choose readability. Line 858: > Any reason this doesn't call enable_flash_ich_fwh ? I'm having a hard time Firmware Hub (FWH) is basically a parallel flash chip on the LPC bus. While booting from LPC is still supported, IIRC, the system needs a SPI chip anyway (Firmware Descriptor, ME etc) and I haven't seen any Intel system with an LPC flash in years. So it doesn't make sense to put any effort there (couldn't test it anyway), but I should probably mention SPI-only in the commit message. -- To view, visit https://review.coreboot.org/18925 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I000819aff25fbe9764f33df85f040093b82cd948 Gerrit-PatchSet: 3 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: ichspi: Add support for Intel Skylake
by Youness Alaoui (Code Review) 26 Apr '17

26 Apr '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18962 ) Change subject: ichspi: Add support for Intel Skylake ...................................................................... Patch Set 4: (1 comment) ichspi.c has too much code and too many unknown concepts for me to review at this time. Maybe I'll do that later when I get more familiar with it all... https://review.coreboot.org/#/c/18962/4/ich_descriptors.c File ich_descriptors.c: Line 920: for (i = 0; i <= nr; i++) Here, nr could be == 5 (above check is for nr > 5, not nr >= 5), and the loop now does i<=nr, so 'i' might be == 5 in the loop, but desc->region.FLREGs has a size of 5, so you'll get an out of bounds write. -- To view, visit https://review.coreboot.org/18962 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0f4565a3c39f5fe3aec4fc8863605cebed1ad4ee Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: ichspi: Drop `dev` parameter from init functions
by Youness Alaoui (Code Review) 26 Apr '17

26 Apr '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/19460 ) Change subject: ichspi: Drop `dev` parameter from init functions ...................................................................... Patch Set 1: Code-Review+1 It looks sane, I think the dev is only used to get the spibar anyways and everything else is done through the SPIBAR MMIO. -- To view, visit https://review.coreboot.org/19460 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0d4e7cc731364e86eff214b9022b842a577f9ef4 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chipset_enable: Add support for Intel Skylake
by Youness Alaoui (Code Review) 26 Apr '17

26 Apr '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18925 ) Change subject: chipset_enable: Add support for Intel Skylake ...................................................................... Patch Set 3: (2 comments) https://review.coreboot.org/#/c/18925/2/chipset_enable.c File chipset_enable.c: Line 852: Here, I would do : pci_get_dev(pci_acc, dev->domain, dev->bus, dev->dev, 5); Might even make that '5' into a #define to avoid having the magic number here. #define PCH_100_SPI_INTERFACE_PCI_FUNCTION Line 858: Any reason this doesn't call enable_flash_ich_fwh ? I'm having a hard time understanding what the FWH is/does and why it's needed for PCH <=9 but not for PCH 100. -- To view, visit https://review.coreboot.org/18925 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I000819aff25fbe9764f33df85f040093b82cd948 Gerrit-PatchSet: 3 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Handle Intel Wildcat Point *LP* like Lynx Point LP
by Youness Alaoui (Code Review) 26 Apr '17

26 Apr '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18883 ) Change subject: Handle Intel Wildcat Point *LP* like Lynx Point LP ...................................................................... Patch Set 5: Code-Review+1 Yeah, I also noticed the ich_descriptors_tool missing the LP change. Looks good to me, thanks! -- To view, visit https://review.coreboot.org/18883 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6b7ca3c0bde111b04ed7c745ed76d28d3d05f01c Gerrit-PatchSet: 5 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Handle Intel Wildcat Point *LP* like Lynx Point LP
by Nico Huber (Code Review) 26 Apr '17

26 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/18883 ) Change subject: Handle Intel Wildcat Point *LP* like Lynx Point LP ...................................................................... Patch Set 5: (1 comment) > The ich_descriptors.c has 2 other functions that refer to > WILDCAT_POINT but not WILDCAT_POINT_LP. Fixed. I remember now why I skipped them: The ich_descriptors_tool doesn't know LP variants. So I tried to avoid that path and ended up fixing the wrong function (._.) https://review.coreboot.org/#/c/18883/4/chipset_enable.c File chipset_enable.c: PS4, Line 666: LP PCHs use a > I would also change this comment here to say "LP family". Done -- To view, visit https://review.coreboot.org/18883 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6b7ca3c0bde111b04ed7c745ed76d28d3d05f01c Gerrit-PatchSet: 5 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chipset_enable: Add support for Intel Skylake
by Nico Huber (Code Review) 26 Apr '17

26 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/18925 ) Change subject: chipset_enable: Add support for Intel Skylake ...................................................................... Patch Set 3: (3 comments) https://review.coreboot.org/#/c/18925/2/chipset_enable.c File chipset_enable.c: PS2, Line 869: > The parameter is unused in ich_init_spi(). I will just remove it. Done PS2, Line 869: > We do anyways, despite the if constructs below. > We do anyways, despite the if constructs below. Right. I was obviously in doubt how to implement it. Now that the parameter is gone, it's also clear that we don't have to keep the `struct pci_access`. Line 870: else > Do we really need a goto here? Done -- To view, visit https://review.coreboot.org/18925 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I000819aff25fbe9764f33df85f040093b82cd948 Gerrit-PatchSet: 3 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chipset_enable: Add support for Intel Skylake
by build bot (Jenkins) (Code Review) 26 Apr '17

26 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/18925 ) Change subject: chipset_enable: Add support for Intel Skylake ...................................................................... Patch Set 3: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/253/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/262/ : SUCCESS -- To view, visit https://review.coreboot.org/18925 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I000819aff25fbe9764f33df85f040093b82cd948 Gerrit-PatchSet: 3 Gerrit-Project: flashrom Gerrit-Branch: staging 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ichspi: Drop `dev` parameter from init functions
by build bot (Jenkins) (Code Review) 26 Apr '17

26 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19460 ) Change subject: ichspi: Drop `dev` parameter from init functions ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/258/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/267/ : SUCCESS -- To view, visit https://review.coreboot.org/19460 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0d4e7cc731364e86eff214b9022b842a577f9ef4 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ich_descriptors: Fix range checks for dumps
by build bot (Jenkins) (Code Review) 26 Apr '17

26 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19031 ) Change subject: ich_descriptors: Fix range checks for dumps ...................................................................... Patch Set 4: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/250/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/260/ : SUCCESS -- To view, visit https://review.coreboot.org/19031 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If57c92ba28f91c4d72123ef0cfd2d9d5ac0a0656 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit