David Hendricks has posted comments on this change. ( https://review.coreboot.org/19031 )
Change subject: ich_descriptors: Fix range checks for dumps
......................................................................
Patch Set 4: Code-Review+2
(3 comments)
Looks like parts of the code need to be updated, but it looks like you're already doing that in some follow-up patches so feel free to ignore the comments.
https://review.coreboot.org/#/c/19031/4/ich_descriptors.c
File ich_descriptors.c:
PS4, Line 739: desc->component.FLPB = dump[(getFCBA(&desc->content) >> 2) + 2];
Side note: This field seems to be absent in many of the most recent PCHes, so that might confuse the length check for code intended for newer platforms.
Line 748: desc->region.FLREGs[4] = dump[(getFRBA(&desc->content) >> 2) + 4];
A couple more FLREGs have been added in recent PCHes.
Line 755: desc->master.FLMSTR3 = dump[(getFMBA(&desc->content) >> 2) + 2];
Same as above - More have been added.
--
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) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Nico Huber has posted comments on this change. ( https://review.coreboot.org/19526 )
Change subject: Obtain correct virtual address for 32-bit BARs on PPC
......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/19526/1/pcidev.c
File pcidev.c:
Line 106: addr = dev->base_addr[(bar - 0x10) / 0x4] & PCI_BASE_ADDRESS_MEM_MASK;
>From my understanding of the libpci API (which is sparsely docu-
mented), the usage of `base_addr` should be preceded by a check
of `dev->known_fields` and a call to pci_fill_info() if it's not
filled yet.
Also, I guess, your assumption that `base_addr` works is only
true when an appropriate libpci access method is used. Which
we leave at the default PCI_ACCESS_AUTO before the call to
pci_init(). I would expect that PCI_ACCESS_SYS_BUS_PCI works,
but enforcing that might not work on all OS.
I guess what we need is to check with pci_get_method_name() if
PCI_ACCESS_SYS_BUS_PCI (or whatever you need) is available (it
will return "" if not) and fallback to PCI_ACCESS_AUTO if not.
--
To view, visit https://review.coreboot.org/19526
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7a37ae98f54aab62e0937985220d1dcd097109f3
Gerrit-PatchSet: 1
Gerrit-Project: flashrom
Gerrit-Branch: stable
Gerrit-Owner: Timothy Pearson <tpearson(a)raptorengineering.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-HasComments: Yes
Timothy Pearson has posted comments on this change. ( https://review.coreboot.org/19531 )
Change subject: Fix erase block structure on Spansion S25FL128P
......................................................................
Patch Set 2: Code-Review-2
> >> I think what you have is a S25FL128P......0 the 0 marks 64KiB
> sectors.
> >
> > The exact part number printed on the Flash chip is:
> > Spansion FL128P1F
> >
> > Is the "P1" a red herring? If so, perhaps the Flashrom sources
> > need a comment to help with this confusing aspect of the device
> > naming.
>
> I guess it's PI and the I is for Industrial temperature range. It's
> all
> in the datasheet ("Package Marking"). The chip you are editing
> should
> have a FL128PIFL marking. And yes, a comment with the package
> marking
> would be helpful.
The chip I have here is missing the trailing L, and the "I" really looks like a 1, but what you're describing seems reasonable looking at the code. I'll just chalk this up to a poor marking job at the factory and give the other chip name a test / update the patch to mark it verified if it works.
--
To view, visit https://review.coreboot.org/19531
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b4e3673f7019f38a3b6ab2f7002bbe56dc6de07
Gerrit-PatchSet: 2
Gerrit-Project: flashrom
Gerrit-Branch: stable
Gerrit-Owner: Timothy Pearson <tpearson(a)raptorengineering.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: Timothy Pearson <tpearson(a)raptorengineering.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/19531 )
Change subject: Fix erase block structure on Spansion S25FL128P
......................................................................
Patch Set 2:
>> I think what you have is a S25FL128P......0 the 0 marks 64KiB sectors.
>
> The exact part number printed on the Flash chip is:
> Spansion FL128P1F
>
> Is the "P1" a red herring? If so, perhaps the Flashrom sources
> need a comment to help with this confusing aspect of the device
> naming.
I guess it's PI and the I is for Industrial temperature range. It's all
in the datasheet ("Package Marking"). The chip you are editing should
have a FL128PIFL marking. And yes, a comment with the package marking
would be helpful.
--
To view, visit https://review.coreboot.org/19531
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b4e3673f7019f38a3b6ab2f7002bbe56dc6de07
Gerrit-PatchSet: 2
Gerrit-Project: flashrom
Gerrit-Branch: stable
Gerrit-Owner: Timothy Pearson <tpearson(a)raptorengineering.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: Timothy Pearson <tpearson(a)raptorengineering.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
David Hendricks has posted comments on this change. ( https://review.coreboot.org/19532 )
Change subject: flashchips: Add ISSI IS25WP128
......................................................................
Patch Set 1: Code-Review-2
Uploaded for convenience only for now...
--
To view, visit https://review.coreboot.org/19532
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia31764f0bf3ba1ed609f83b95e21343d7c23a508
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No