Nico Huber has posted comments on this change. ( https://review.coreboot.org/18925 )
Change subject: chipset_enable: Add support for Intel Skylake / Kabylake
......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/18925/2/chipset_enable.c
File chipset_enable.c:
Line 852: struct pci_dev *const spi_dev = pci_get_dev(pci_acc, 0, 0, 0x1f, 5);
> It just confused me at first, rather than be recognizable, I guess I'd stil
It feels pretty common when you've looked too much at coreboot source.
Line 858: const int ret_bc = enable_flash_ich_bios_cntl_config_space(spi_dev, pch_generation, 0xdc);
> Humm.. could it be by any chance the EC chip ? The librems have a separate
The EC flash can be anywhere, even shared SPI chips are possible. But
usually, today, you have to ask the EC in a proprietary way to access
its flash. One configuration that was common ~10 years ago: a shared
SPI flash connected to the EC; the BIOS was then accessed over LPC
through the EC ;) that's why we have that scary warning about flashing
laptops.
The chromiumos patch didn't touch the LPC stuff too, IIRC.
--
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: 2
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
Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18925 )
Change subject: chipset_enable: Add support for Intel Skylake / Kabylake
......................................................................
Patch Set 5: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/18925/2/chipset_enable.c
File chipset_enable.c:
Line 852:
> > Here, I would do :
It just confused me at first, rather than be recognizable, I guess I'd still keep the dev->domain, and dev->bus, but you're right about the dev->dev not being used and using 0x1f instead. I'd at least put a comment to say "SPI interface is on D31:F5 according to spec". But maybe it's not clear to me just because I'm not used to this ?
Line 858:
> Firmware Hub (FWH) is basically a parallel flash chip on the LPC bus.
Humm.. could it be by any chance the EC chip ? The librems have a separate flash chip for the EC, but it's SPI too (afaik), and I've seen that chromebooks supports flashing the EC by using the 'internal:bus=lpc' option (which doesn't work with upstream flashrom). I don't know if it would apply to the librems (since librems use a second SPI flash), but it would be interesting for me to check that later. Right now I'm still wondering how I can read/write the EC firmware if I wanted to.
Good idea on the mention of 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: 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 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
Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git
......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push
File util/git-hooks/pre-push:
Line 41: else
Why put an else here if we exited anyway?
PS1, Line 63: pusing
*pushing
PS1, Line 57: # Make _really_ sure we do not rewrite precious history
: for lbranch in "${precious_branches[@]}"; do
: if [ "$remote_ref" = "refs/heads/$lbranch" ]; then
: nonreachable=$(git rev-list $remote_sha ^$local_sha)
: if [ -n "$nonreachable" ]; then
: echo "Only fast-forward pushes are allowed on $lbranch." >&2
: echo "$nonreachable is not included in $remote_sha while pusing to $remote_ref" >&2
: exit 1
: fi
: fi
: done
> Isn't this configurable in the upstream repo?
The code looks good, I was just worried that we reinvent the wheel.
There is the "receive.denyNonFastForwards" option that does the same
but for all branches. OTOH, will we have anything but precious bran-
ches with creation of new branches being forbidden?
--
To view, visit https://review.coreboot.org/19206
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2
Gerrit-PatchSet: 1
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.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)
Gerrit-HasComments: Yes
Patrick Georgi has restored this change. ( https://review.coreboot.org/18812 )
Change subject: trivial change to test jenkins
......................................................................
Restored
more tests required, it seems?
--
To view, visit https://review.coreboot.org/18812
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: restore
Gerrit-Change-Id: I49e4cca56938b9f945de2ccd2d0a4ec6acdb0a30
Gerrit-PatchSet: 1
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins)