Attention is currently required from: Michał Żygowski, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 )
Change subject: acpi_ec: Implement basic ACPI embedded controller API
......................................................................
Patch Set 4:
(4 comments)
File acpi_ec.h:
https://review.coreboot.org/c/flashrom/+/55714/comment/3bdd0749_0977fb56
PS4, Line 8: * the Free Software Foundation; version 2 of the License.
It would be nice if you could convince TUXEDO to put it under GPLv2+.
We try to suppress GPLv2-only to make libflashrom more useful.
https://review.coreboot.org/c/flashrom/+/55714/comment/e19e6def_1052951e
PS4, Line 26: /* Standard ports */
: #define EC_DATA 0x62
: #define EC_CONTROL 0x66 /* Read status, write commands */
:
: /* Standard commands */
: #define EC_CMD_READ_REG 0x80 /* Read register's value */
: #define EC_CMD_WRITE_REG 0x81 /* Write register's value */
:
: /* Some of the status bits */
: #define EC_STS_IBF (1 << 1) /* EC's input buffer full (host can't write) */
: #define EC_STS_OBF (1 << 0) /* EC's output buffer full (host can read) */
:
Do these need to be exported?
https://review.coreboot.org/c/flashrom/+/55714/comment/40ca7034_40eeb849
PS4, Line 41: bool ec_wait_for_ibuf(uint8_t control_port, unsigned int max_checks);
: bool ec_wait_for_obuf(uint8_t control_port, unsigned int max_checks);
Do these need to be exported?
https://review.coreboot.org/c/flashrom/+/55714/comment/75e0c4f0_9bf972f1
PS4, Line 44: bool ec_write_cmd(uint8_t control_port, uint8_t cmd, unsigned int max_checks);
: bool ec_read_byte(uint8_t control_port, uint8_t data_port, uint8_t *data,
: unsigned int max_checks);
: bool ec_write_byte(uint8_t control_port, uint8_t data_port, uint8_t data,
: unsigned int max_checks);
:
If these are part of an `acpi_ec` API and ACPI standardized the ports,
why allow to specify the port? Does the ITE driver talk on multiple
interfaces at once?
--
To view, visit https://review.coreboot.org/c/flashrom/+/55714
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d
Gerrit-Change-Number: 55714
Gerrit-PatchSet: 4
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 25 Jun 2021 13:39:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Angel Pons, Michael Niewöhner.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55713 )
Change subject: flashrom.c: Add 64KiB write granularity
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/55713/comment/40e22f8f_77b3f46d
PS1, Line 506: 1024 * 64
> Done
We have a `KiB` macro by now, so this could be `64*KiB`.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55713
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f
Gerrit-Change-Number: 55713
Gerrit-PatchSet: 3
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Fri, 25 Jun 2021 13:20:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55295 )
Change subject: tests: Do not run a test if its driver is not built
......................................................................
tests: Do not run a test if its driver is not built
For all tests that exist as of today, drivers are built by default,
however config options can be disabled and in that case test should
not be run.
Technically, this is done by skipping the test.
BUG=b:181803212
TEST=1) Tested by adding into tests/meson.build
-DCONFIG_xxx=0
4 times (for every driver with test), and then running ninja test
Result: corresponding test is skipped, all other tests are passed
2) Running ninja test with default config settings (everything is
enabled, no overriding in test meson).
Result: all tests are passed.
3) Replacing one of config options in the patch with CONFIG_JLINK_SPI
which is disabled by default.
Result: corresponding test is skipped.
Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/55295
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M tests/init_shutdown.c
1 file changed, 16 insertions(+), 0 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
Angel Pons: Looks good to me, approved
Edward O'Callaghan: Looks good to me, approved
diff --git a/tests/init_shutdown.c b/tests/init_shutdown.c
index 80440bd..48448f8 100644
--- a/tests/init_shutdown.c
+++ b/tests/init_shutdown.c
@@ -34,7 +34,11 @@
void dummy_init_and_shutdown_test_success(void **state)
{
+#if CONFIG_DUMMY == 1
run_lifecycle(state, &programmer_dummy, "bus=parallel+lpc+fwh+spi");
+#else
+ skip();
+#endif
}
struct mec1308_io_state {
@@ -62,6 +66,7 @@
void mec1308_init_and_shutdown_test_success(void **state)
{
+#if CONFIG_MEC1308 == 1
struct mec1308_io_state mec1308_io_state = { 0 };
const struct io_mock mec1308_io = {
.state = &mec1308_io_state,
@@ -75,6 +80,9 @@
run_lifecycle(state, &programmer_mec1308, "");
io_mock_register(NULL);
+#else
+ skip();
+#endif
}
struct ene_lpc_io_state {
@@ -118,6 +126,7 @@
void ene_lpc_init_and_shutdown_test_success(void **state)
{
+#if CONFIG_ENE_LPC == 1
/*
* Current implementation tests for chip ENE_KB932.
* Another chip which is not tested here is ENE_KB94X.
@@ -134,6 +143,9 @@
run_lifecycle(state, &programmer_ene_lpc, "");
io_mock_register(NULL);
+#else
+ skip();
+#endif
}
void linux_spi_init_and_shutdown_test_success(void **state)
@@ -144,5 +156,9 @@
* and the fallback to getpagesize(). This test does the latter (fallback to
* getpagesize).
*/
+#if CONFIG_LINUX_SPI == 1
run_lifecycle(state, &programmer_linux_spi, "dev=/dev/null");
+#else
+ skip();
+#endif
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/55295
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77
Gerrit-Change-Number: 55295
Gerrit-PatchSet: 6
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(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: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: merged
Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Angel Pons, Michael Niewöhner.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS4:
> > Forcing to run the programmer code on an unknown board would be new […]
All other EC drivers were added lately without proper review. That they skip
sanity checks (and documentation too), is the foremost reason why I'm currently
holding a release back. The situation of the superio probing is bad indeed, but
not as bad as starting with EC RAM writes. The comment in probe_superio() proves
the point. IMHO, we should only probe for expected chips, `board_enable` provides
the infrastructure for it.
Patches (also to clean up the superio situation) are welcome of course.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55715
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031
Gerrit-Change-Number: 55715
Gerrit-PatchSet: 9
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
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-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Fri, 25 Jun 2021 13:14:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Angel Pons.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 )
Change subject: acpi_ec: Implement basic ACPI embedded controller API
......................................................................
Patch Set 4:
(1 comment)
File acpi_ec.c:
https://review.coreboot.org/c/flashrom/+/55714/comment/851fde2a_a4647fe6
PS4, Line 23: ontrol_port
cmd_port
--
To view, visit https://review.coreboot.org/c/flashrom/+/55714
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d
Gerrit-Change-Number: 55714
Gerrit-PatchSet: 4
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 25 Jun 2021 13:12:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55695 )
Change subject: ft2232_spi: Revise comments about output pin states
......................................................................
ft2232_spi: Revise comments about output pin states
The meaning of the variables is easy to misunderstand as some
states are merely implicit: All output pins that are not set
in the `cs_bits` mask will be constantly driven low. This may
be sheer coincidence as all programmers that need additional
pins driven use active-low signals to enable buffers.
While other pins stay low, *all* pins set in the `cs_bits`
mask are supposed to be toggled during SPI transactions.
Also drop some irritating dead code and try to explain things
in a comment.
Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/55695
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
---
M ft2232_spi.c
1 file changed, 21 insertions(+), 9 deletions(-)
Approvals:
build bot (Jenkins): Verified
Angel Pons: Looks good to me, approved
diff --git a/ft2232_spi.c b/ft2232_spi.c
index eb98933..0d0c32c 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -86,15 +86,27 @@
#define BITMODE_BITBANG_NORMAL 1
#define BITMODE_BITBANG_SPI 2
-/* The variables cs_bits and pindir store the values for the "set data bits low byte" MPSSE command that
- * sets the initial state and the direction of the I/O pins. The pin offsets are as follows:
- * SCK is bit 0.
- * DO is bit 1.
- * DI is bit 2.
- * CS is bit 3.
+/*
+ * The variables `cs_bits` and `pindir` store the values for the
+ * "set data bits low byte" MPSSE command that sets the initial
+ * state and the direction of the I/O pins. `cs_bits` pins default
+ * to high and will be toggled during SPI transactions. All other
+ * output pins will be kept low all the time. On exit, all pins
+ * will be reconfigured as inputs.
*
- * The default values (set below in ft2232_spi_init) are used for most devices:
- * value: 0x08 CS=high, DI=low, DO=low, SK=low
+ * The pin offsets are as follows:
+ * TCK/SK is bit 0.
+ * TDI/DO is bit 1.
+ * TDO/DI is bit 2.
+ * TMS/CS is bit 3.
+ * GPIOL0 is bit 4.
+ * GPIOL1 is bit 5.
+ * GPIOL2 is bit 6.
+ * GPIOL3 is bit 7.
+ *
+ * The default values (set below in ft2232_spi_init) are used for
+ * most devices:
+ * value: 0x08 CS=high, DI=low, DO=low, SK=low
* dir: 0x0b CS=output, DI=input, DO=output, SK=output
*/
struct ft2232_data {
@@ -219,7 +231,7 @@
msg_pspew("Assert CS#\n");
buf[i++] = SET_BITS_LOW;
- buf[i++] = 0 & ~spi_data->cs_bits; /* assertive */
+ buf[i++] = 0; /* assert CS# pins, all other output pins stay low */
buf[i++] = spi_data->pindir;
/* WREN, OP(PROGRAM, ERASE), ADDR, DATA */
--
To view, visit https://review.coreboot.org/c/flashrom/+/55695
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107
Gerrit-Change-Number: 55695
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Alan Green <avg(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: merged
Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Angel Pons.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS4:
> Forcing to run the programmer code on an unknown board would be new
to flashrom. I don't see why we would suddenly need that.
I haven't checked all flash drivers in flashrom but I don't remember any DMI checks or anything like that in other ec or superio drivers. Why do we need it now?
--
To view, visit https://review.coreboot.org/c/flashrom/+/55715
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031
Gerrit-Change-Number: 55715
Gerrit-PatchSet: 9
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
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-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 25 Jun 2021 12:52:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment