Attention is currently required from: Nico Huber, Thomas Walker.
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/52310
to look at the new patch set (#2).
Change subject: Added support for Spansion/Cypress chip S25FL256L.
......................................................................
Added support for Spansion/Cypress chip S25FL256L.
Conducted random write test using Adafruit FT232H programmer.
Confirmed read/write/erase/probe and VERIFIED on completion.
Signed-off-by: Thomas Walker <thh.walker(a)gmail.com>
Change-Id: I4e8cba554d0590c94dac92aa91e9ab400efca281
---
M chipdrivers.h
M flashchips.c
M flashchips.h
M spi.h
M spi25.c
5 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/52310/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52310
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4e8cba554d0590c94dac92aa91e9ab400efca281
Gerrit-Change-Number: 52310
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Walker <thh.walker(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Walker <thh.walker(a)gmail.com>
Gerrit-MessageType: newpatchset
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/51978 )
Change subject: test_build.sh: use sh from env
......................................................................
test_build.sh: use sh from env
Change-Id: I3897b8d980425ecbb89b238d4a766f628cf9d3e6
Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/51978
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Paul Menzel <paulepanter(a)mailbox.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M test_build.sh
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Paul Menzel: Looks good to me, but someone else must approve
Angel Pons: Looks good to me, approved
diff --git a/test_build.sh b/test_build.sh
index 0e43cd3..8f11ccd 100755
--- a/test_build.sh
+++ b/test_build.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/usr/bin/env sh
set -e
make CONFIG_EVERYTHING=yes WARNERROR=yes
--
To view, visit https://review.coreboot.org/c/flashrom/+/51978
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3897b8d980425ecbb89b238d4a766f628cf9d3e6
Gerrit-Change-Number: 51978
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Thomas Heijligen, Paul Menzel.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51978 )
Change subject: test_build.sh: use sh from env
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Given that we exactly know the environment in which
test_build.sh runs (cf. CB:46894), I don't think this
makes any difference.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51978
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3897b8d980425ecbb89b238d4a766f628cf9d3e6
Gerrit-Change-Number: 51978
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Wed, 14 Apr 2021 15:53:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Paul Menzel.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51978 )
Change subject: test_build.sh: use sh from env
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Which ones? I'm curious. […]
with an absolute path as interpreter you are sure that your script will run by that binary, or the linked one. With /usr/bin/env the first suitable binary with this name in your path is chosen to run the script.
On "/bin/sh" it's not this necessary, since most distributions provide /bin/sh for compatibility. Other interpreter may be only available though PATH and not as /bin/xyz
--
To view, visit https://review.coreboot.org/c/flashrom/+/51978
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3897b8d980425ecbb89b238d4a766f628cf9d3e6
Gerrit-Change-Number: 51978
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Wed, 14 Apr 2021 11:37:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Walker.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52310 )
Change subject: Added support for Spansion/Cypress chip S25FL256L.
......................................................................
Patch Set 1:
(8 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/52310/comment/d1e017ff_a91c06e1
PS1, Line 7: Added support for Spansion/Cypress chip S25FL256L.
Commit messages are written in present tense, do not end with a period and usually have a prefix. I'd suggest:
flashchips: Add Spansion/Cypress S25FL256L
https://review.coreboot.org/c/flashrom/+/52310/comment/69990759_7a45c3f5
PS1, Line 9: Testing has been conducted on ~60 individual chips and confirmed as working.
One thing about write tests: flashrom compares the current contents of the flash chip to the provided data. If they are equal, flashrom does nothing.
For example, you may see something like this in a verbose log:
0x000000-0x000fff:S
The S tells us that the block was *S*kipped. To really test writes, one could use random data, e.g.
dd if=/dev/urandom bs=1M count=32 of=random.rom
flashrom -p ... -w random.rom
If flashrom says VERIFIED, we're all good.
Patchset:
PS1:
Welcome!
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/52310/comment/b82cff71_03b288fa
PS1, Line 16387: |
Please add spaces around `|`
https://review.coreboot.org/c/flashrom/+/52310/comment/af561af6_19e55bfd
PS1, Line 16391: .block_erasers =
Many block erasers are missing, and only the first entry is correct. This happened to work because flashrom only uses the other block erasers when the first one fails.
From the datasheet, the correct block erasers should be, in order:
.block_erasers =
{
{
.eraseblocks = { {4 * 1024, 8192} },
.block_erase = spi_block_erase_21,
}, {
.eraseblocks = { {4 * 1024, 8192} },
.block_erase = spi_block_erase_20,
}, {
.eraseblocks = { {32 * 1024, 1024} },
.block_erase = spi_block_erase_53,
}, {
.eraseblocks = { {32 * 1024, 1024} },
.block_erase = spi_block_erase_52,
}, {
.eraseblocks = { {64 * 1024, 512} },
.block_erase = spi_block_erase_dc,
}, {
.eraseblocks = { {64 * 1024, 512} },
.block_erase = spi_block_erase_d8,
}, {
.eraseblocks = { {32 * 1024 * 1024, 1} },
.block_erase = spi_block_erase_60,
}, {
.eraseblocks = { {32 * 1024 * 1024, 1} },
.block_erase = spi_block_erase_c7,
}
},
Other chips place the 4BA (4-Byte Address) opcodes first, so I did the same here.
https://review.coreboot.org/c/flashrom/+/52310/comment/7fe33be1_b808e25f
PS1, Line 16401: spi_prettyprint_status_register_plain
spi_prettyprint_status_register_bp3_srwd
https://review.coreboot.org/c/flashrom/+/52310/comment/49e4ffce_f6adb287
PS1, Line 16402: spi_disable_blockprotect
spi_disable_blockprotect_bp3_srwd
https://review.coreboot.org/c/flashrom/+/52310/comment/225fd020_3bb6432c
PS1, Line 16406: },
Please use tabs for indentation
--
To view, visit https://review.coreboot.org/c/flashrom/+/52310
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4e8cba554d0590c94dac92aa91e9ab400efca281
Gerrit-Change-Number: 52310
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Walker <thh.walker(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Walker <thh.walker(a)gmail.com>
Gerrit-Comment-Date: Wed, 14 Apr 2021 10:25:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Edward O'Callaghan, Arthur Heymans, Carl-Daniel Hailfinger.
Daniel Campello has uploaded a new patch set (#25) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support
......................................................................
layout: Add -i <region>[:<file>] support
Add an optional sub-parameter to the -i parameter to allow building the
image to be written from multiple files. This will also allow regions to
be read from flash and written to separate image files.
This is a rebase of a patch that was ported from chromiumos. A lot of
things have changed, but the idea is the same.
Original patch by Louis Yung-Chieh Lo <yjlou(a)chromium.org>:
Summary: Support -i partition:file feature for both read and write.
Commit: 9c7525f
Review URL: http://codereview.chromium.org/6611015
Ported version by Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
and Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>:
Summary: [PATCH 2/6] layout: Add -i <region>[:<file>] support.
Review URL: https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html
Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: Nico Huber <nico.huber(a)secunet.com>
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Co-Authored-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Authored-by: Daniel Campello <campello(a)chromium.org>
---
M cli_classic.c
M flashrom.8.tmpl
M flashrom.c
M layout.c
M layout.h
5 files changed, 306 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/25
--
To view, visit https://review.coreboot.org/c/flashrom/+/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 25
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nicola Corna <nicola(a)corna.info>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Edward O'Callaghan, Arthur Heymans, Carl-Daniel Hailfinger.
Daniel Campello has uploaded a new patch set (#24) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support
......................................................................
layout: Add -i <region>[:<file>] support
Add an optional sub-parameter to the -i parameter to allow building the
image to be written from multiple files. This will also allow regions to
be read from flash and written to separate image files.
This is a rebase of a patch that was ported from chromiumos. A lot of
things have changed, but the idea is the same.
Original patch by Louis Yung-Chieh Lo <yjlou(a)chromium.org>:
Summary: Support -i partition:file feature for both read and write.
Commit: 9c7525f
Review URL: http://codereview.chromium.org/6611015
Ported version by Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
and Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>:
Summary: [PATCH 2/6] layout: Add -i <region>[:<file>] support.
Review URL: https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html
Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: Nico Huber <nico.huber(a)secunet.com>
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Co-Authored-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Authored-by: Daniel Campello <campello(a)chromium.org>
---
M cli_classic.c
M flashrom.8.tmpl
M flashrom.c
M layout.c
M layout.h
5 files changed, 305 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/24
--
To view, visit https://review.coreboot.org/c/flashrom/+/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 24
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nicola Corna <nicola(a)corna.info>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-MessageType: newpatchset