Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/44776
to review the following change.
Change subject: dediprog: Enable 4BA for all protocols
......................................................................
dediprog: Enable 4BA for all protocols
* Tested on SF600 with protocol version 3 with "W25Q256.W" (32768 kB)
* Tested on SF100 with protocol version 2 with "W25Q256.W" (32768 kB)
As it works with all protocol version, assume that the SF200 works fine
as well.
Change-Id: Iee0ba972245b9317ef86345432fec5fc32614888
Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com>
---
M dediprog.c
1 file changed, 1 insertion(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/44776/1
diff --git a/dediprog.c b/dediprog.c
index 827d5e4..13365cd 100644
--- a/dediprog.c
+++ b/dediprog.c
@@ -993,7 +993,7 @@
}
static struct spi_master spi_master_dediprog = {
- .features = SPI_MASTER_NO_4BA_MODES,
+ .features = SPI_MASTER_4BA,
.max_data_read = 16, /* 18 seems to work fine as well, but 19 times out sometimes with FW 5.15. */
.max_data_write = 16,
.command = dediprog_spi_send_command,
@@ -1268,12 +1268,6 @@
if (dediprog_standalone_mode())
return 1;
- if (dediprog_devicetype == DEV_SF100 && protocol() == PROTOCOL_V1)
- spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES;
-
- if (protocol() == PROTOCOL_V2)
- spi_master_dediprog.features |= SPI_MASTER_4BA;
-
if (register_spi_master(&spi_master_dediprog) || dediprog_set_leds(LED_NONE))
return 1;
--
To view, visit https://review.coreboot.org/c/flashrom/+/44776
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iee0ba972245b9317ef86345432fec5fc32614888
Gerrit-Change-Number: 44776
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: newchange
Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/40748 )
Change subject: ft2232_spi.c - Pack read cmd and nCS in one ftdi_write_data() call
......................................................................
ft2232_spi.c - Pack read cmd and nCS in one ftdi_write_data() call
Every ftdi_write_data() call is quite time consuming as the ftdi-chip takes 2-3ms to respond.
As the comment already says: Minimize USB transfers by packing as many commands as possible together.
As I cannot see any reason to put read cmd and deassertion of nCS into seperated ftdi calls,
I packed them together to save programming time.
Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de>
Change-Id: I3f76da3dd0124a0deec7f937dbc4b7dc488c8004
---
M ft2232_spi.c
1 file changed, 38 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/40748/1
diff --git a/ft2232_spi.c b/ft2232_spi.c
index 9f4c7f0..b2f5e40 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -398,7 +398,10 @@
msg_perr("Unable to set latency timer (%s).\n", ftdi_get_error_string(ftdic));
}
- if (ftdi_write_data_set_chunksize(ftdic, 270)) {
+ if (ftdi_write_data_set_chunksize(ftdic, 280)) {
+ /* 280 Byte = (9 Byte CMD + 1 Byte WREN) + (9 Byte CMD + 1 Byte op) + 4 Byte Addr
+ * + 256 Byte PageWrite-data
+ with op: PageProgram or Erase; CMD: FTDI-Chip commands*/
msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(ftdic));
}
@@ -467,31 +470,26 @@
struct ftdi_context *ftdic = &ftdic_context;
static unsigned char *buf = NULL;
/* failed is special. We use bitwise ops, but it is essentially bool. */
- int i = 0, ret = 0, failed = 0;
- size_t bufsize;
- static size_t oldbufsize = 0;
+ static int i = 0;
+ int ret = 0, failed = 0;
if (writecnt > 65536 || readcnt > 65536)
return SPI_INVALID_LENGTH;
- /* buf is not used for the response from the chip. */
- bufsize = max(writecnt + 9, 260 + 9);
- /* Never shrink. realloc() calls are expensive. */
- if (!buf || bufsize > oldbufsize) {
- buf = realloc(buf, bufsize);
+ if (!buf) {
+ /* set buf size to hardware buffer size of ftdi*/
+ buf = realloc(buf, 4096);
if (!buf) {
msg_perr("Out of memory!\n");
/* TODO: What to do with buf? */
return SPI_GENERIC_ERROR;
}
- oldbufsize = bufsize;
}
/*
* Minimize USB transfers by packing as many commands as possible
- * together. If we're not expecting to read, we can assert CS#, write,
- * and deassert CS# all in one shot. If reading, we do three separate
- * operations.
+ * together. We can assert CS#, write, (read,)
+ * and deassert CS# all in one shot.
*/
msg_pspew("Assert CS#\n");
buf[i++] = SET_BITS_LOW;
@@ -506,44 +504,44 @@
i += writecnt;
}
- /*
- * Optionally terminate this batch of commands with a
- * read command, then do the fetch of the results.
- */
+ /* A optionally read command */
if (readcnt) {
buf[i++] = MPSSE_DO_READ;
buf[i++] = (readcnt - 1) & 0xff;
buf[i++] = ((readcnt - 1) >> 8) & 0xff;
- ret = send_buf(ftdic, buf, i);
- failed = ret;
- /* We can't abort here, we still have to deassert CS#. */
- if (ret)
- msg_perr("send_buf failed before read: %i\n", ret);
- i = 0;
- if (ret == 0) {
- /*
- * FIXME: This is unreliable. There's no guarantee that
- * we read the response directly after sending the read
- * command. We may be scheduled out etc.
- */
- ret = get_buf(ftdic, readarr, readcnt);
- failed |= ret;
- /* We can't abort here either. */
- if (ret)
- msg_perr("get_buf failed: %i\n", ret);
- }
}
msg_pspew("De-assert CS#\n");
buf[i++] = SET_BITS_LOW;
buf[i++] = cs_bits;
buf[i++] = pindir;
- ret = send_buf(ftdic, buf, i);
- failed |= ret;
- if (ret)
- msg_perr("send_buf failed at end: %i\n", ret);
- return failed ? -1 : 0;
+ if (writearr[0] == JEDEC_WREN) {
+ /* Return to get second op (Program or Erase) without resetting buf nor i*/
+ return 0;
+ } else {
+ ret = send_buf(ftdic, buf, i);
+ failed |= ret;
+ if (ret)
+ msg_perr("send_buf failed: %i\n", ret);
+ i = 0;
+
+ if (readcnt) {
+ if (ret == 0) {
+ /*
+ * FIXME: This is unreliable. There's no guarantee that
+ * we read the response directly after sending the read
+ * command. We may be scheduled out etc.
+ */
+ ret = get_buf(ftdic, readarr, readcnt);
+ failed |= ret;
+ /* We can't abort here either. */
+ if (ret)
+ msg_perr("get_buf failed: %i\n", ret);
+ }
+ }
+ return failed ? -1 : 0;
+ }
}
#endif
--
To view, visit https://review.coreboot.org/c/flashrom/+/40748
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3f76da3dd0124a0deec7f937dbc4b7dc488c8004
Gerrit-Change-Number: 40748
Gerrit-PatchSet: 1
Gerrit-Owner: Simon Buhrow
Gerrit-MessageType: newchange
Attention is currently required from: Angel Pons, Daniel Campello.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51930 )
Change subject: Makefile,meson.build: Fix dependency issues with raiden_debug_spi
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Thanks! Having a last look, I've noticed another potential […]
No worries, luckily I saw the email in my uncontrollable corp inbox.
Do you mean line `1090: NEED_LIBUSB1 += CONFIG_DEDIPROG` isn't it already added or did I miss something?
--
To view, visit https://review.coreboot.org/c/flashrom/+/51930
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d6a8c33a2228abf006a9b278bcb7133765c7074
Gerrit-Change-Number: 51930
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Wed, 31 Mar 2021 23:41:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: Add unit test to run init/shutdown for enabled drivers
......................................................................
Patch Set 1:
(1 comment)
File mec1308.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/004d3454_73f426ba
PS1, Line 115: #endif /* UNIT_TEST_ENV */
> I know it won't be as easy, but I'd much prefer such mocks in […]
I was thinking about separate compilation units for a while, how exactly to do this. Maybe you have some concrete ideas in mind?
All that inb/outb functionality is in hwaccess.h. Can we have a hwaccess_unittest.h for test environment? Although it does not solve the question because for every driver needs its own mocking. For example mec1308 in this patch expects specific value from inb, not just any value.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51487
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc
Gerrit-Change-Number: 51487
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:27:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 )
Change subject: Enable dynamic memory allocation checks for cmocka unit tests
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/51243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f
Gerrit-Change-Number: 51243
Gerrit-PatchSet: 7
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com>
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:24:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Paul Menzel.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51979 )
Change subject: test_build.sh: use -C option of ninja to specify the build directory
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/51979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Gerrit-Change-Number: 51979
Gerrit-PatchSet: 3
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: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:24:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 )
Change subject: Enable dynamic memory allocation checks for cmocka unit tests
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
File meson.build:
https://review.coreboot.org/c/flashrom/+/51243/comment/27bad3fb_f29ac524
PS6, Line 477: -include
> I tried space after -include and it does not work: […]
Ah, sorry. I'm still very used to the comfort and pain of having
everything in a shell command line.
I suppose every string is always passed as one argument, so one
would never have to do any (shell) quoting. Then the separated
alternative would be something like:
'-include', 'stdlib.h',
'-include', 'unittest_env.h',
But I'm not even sure if I'd prefer that. Your call.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f
Gerrit-Change-Number: 51243
Gerrit-PatchSet: 7
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com>
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:04:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Paul Menzel, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51979 )
Change subject: test_build.sh: use -C option of ninja to specify the build directory
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/51979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Gerrit-Change-Number: 51979
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:00:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment