Anastasia Klimchuk has uploaded this change for review.
[WIP EXPERIMENT] Give priority to SFDP is chip supports it
The patch changes the idea how the chips are probed.
Previously, probing would go through the whole list of flashchips
and try each of them, and only if none of the entries match
then SFDP is attempted as a fallback.
This patch aims to probe with SFDP first, and if the chip responds,
probing stops there and all further work with the chip is done via
SFDP.
If the chip does not support SFDP, we fallback to going through the
list of flashchips and probing each.
Note this patch does not affect the behaviour when command line
option `-c <chip_name>` is used. Providing chip name explicitly will
only probe for one entry in flashchips, with the given name.
NOTE for this to land, we need to do one more feature:
parsing reg_bits info for write-protection from SFDP header (similar
to what rpmc implementation is doing). Without parsing reg_bits
from SFDP header, wp operations won't work with this patch, unless
using direct option `-c <chip_name>`.
Change-Id: I8bd96742b7365be16dc277d42b14c5a44ef7f7a9
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
---
M cli_classic.c
M flashchips.c
M flashrom.c
M libflashrom.c
M tests/dummyflasher.c
M tests/tests.c
M tests/tests.h
7 files changed, 52 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/86432/1
diff --git a/cli_classic.c b/cli_classic.c
index 1a0565b..401dc9f 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -1210,7 +1210,7 @@
startchip = 0;
while (chipcount < (int)ARRAY_SIZE(flashes)) {
startchip = probe_flash(®istered_masters[j], startchip, &flashes[chipcount], 0, options.chip_to_probe);
- if (startchip == -1)
+ if (startchip == -1 || (flashes[chipcount].chip->model_id == SFDP_DEVICE_ID))
break;
chipcount++;
startchip++;
diff --git a/flashchips.c b/flashchips.c
index 615419d..0bd04ef 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -55,6 +55,31 @@
* .voltage = Voltage range in millivolt
*/
+ /*
+ * SFDP chip entry is intentionally placed first in the list.
+ */
+ {
+ .vendor = "Any",
+ .name = "SFDP-capable chip",
+ .bustype = BUS_SPI,
+ .manufacture_id = GENERIC_MANUF_ID,
+ .model_id = SFDP_DEVICE_ID,
+ .total_size = 0, /* set by probing function */
+ .page_size = 0, /* set by probing function */
+ .feature_bits = 0, /* set by probing function */
+ /* We present our own "report this" text hence we do not */
+ /* want the default "This flash part has status UNTESTED..." */
+ /* text to be printed. */
+ .tested = { .probe = OK, .read = OK, .erase = OK, .write = OK, .wp = NA },
+ .probe = PROBE_SPI_SFDP,
+ .block_erasers = {}, /* set by probing function */
+ .unlock = SPI_DISABLE_BLOCKPROTECT, /* is this safe? */
+ .write = 0, /* set by probing function */
+ .read = SPI_CHIP_READ,
+ /* FIXME: some vendor extensions define this */
+ .voltage = {0},
+ },
+
/* TODO: Refactor implementation to avoid these .c includes */
#include "flashchips/amd.c"
#include "flashchips/amic.c"
@@ -95,28 +120,6 @@
* These entries are intentionally placed at the end.
*/
{
- .vendor = "Unknown",
- .name = "SFDP-capable chip",
- .bustype = BUS_SPI,
- .manufacture_id = GENERIC_MANUF_ID,
- .model_id = SFDP_DEVICE_ID,
- .total_size = 0, /* set by probing function */
- .page_size = 0, /* set by probing function */
- .feature_bits = 0, /* set by probing function */
- /* We present our own "report this" text hence we do not */
- /* want the default "This flash part has status UNTESTED..." */
- /* text to be printed. */
- .tested = { .probe = OK, .read = OK, .erase = OK, .write = OK, .wp = NA },
- .probe = PROBE_SPI_SFDP,
- .block_erasers = {}, /* set by probing function */
- .unlock = SPI_DISABLE_BLOCKPROTECT, /* is this safe? */
- .write = 0, /* set by probing function */
- .read = SPI_CHIP_READ,
- /* FIXME: some vendor extensions define this */
- .voltage = {0},
- },
-
- {
.vendor = "Programmer",
.name = "Opaque flash chip",
.bustype = BUS_PROG,
diff --git a/flashrom.c b/flashrom.c
index 8daad74..228db69 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1153,7 +1153,10 @@
/* If this is the first chip found, accept it.
* If this is not the first chip found, accept it only if it is
- * a non-generic match. SFDP and CFI are generic matches.
+ * a non-generic match. CFI is a generic match.
+ *
+ * SFDP is a special match which takes priority when it's found.
+ *
* startchip==0 means this call to probe_flash() is the first
* one for this programmer interface (master) and thus no other chip has
* been found on this interface.
@@ -1178,7 +1181,7 @@
if (startchip == 0)
break;
/* Not the first flash chip detected on this bus, but not a generic match either. */
- if ((flash->chip->model_id != GENERIC_DEVICE_ID) && (flash->chip->model_id != SFDP_DEVICE_ID))
+ if (flash->chip->model_id != GENERIC_DEVICE_ID)
break;
/* Not the first flash chip detected on this bus, and it's just a generic match. Ignore it. */
notfound:
diff --git a/libflashrom.c b/libflashrom.c
index 78e5c9b..db8625b 100644
--- a/libflashrom.c
+++ b/libflashrom.c
@@ -21,6 +21,7 @@
#include <stdarg.h>
#include "flash.h"
+#include "flashchips.h"
#include "fmap.h"
#include "programmer.h"
#include "layout.h"
@@ -249,8 +250,10 @@
int flash_idx = -1;
if (!ret || (flash_idx = probe_flash(®istered_masters[i], 0, *flashctx, 0, chip_name)) != -1) {
ret = 0;
- /* We found one chip, now check that there is no second match. */
- if (probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0, chip_name) != -1) {
+ /* We found one chip, if this is SFDP-capable chip then we don't need to look further.
+ * Otherwise we check that there is no second match. */
+ if (((*flashctx)->chip->model_id != SFDP_DEVICE_ID)
+ && probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0, chip_name) != -1) {
flashrom_layout_release(second_flashctx.default_layout);
free(second_flashctx.chip);
ret = 3;
diff --git a/tests/dummyflasher.c b/tests/dummyflasher.c
index 9878bef..80cfe38 100644
--- a/tests/dummyflasher.c
+++ b/tests/dummyflasher.c
@@ -55,6 +55,19 @@
run_probe_lifecycle(state, &dummy_io, &programmer_dummy, "size=8388608,emulate=VARIABLE_SIZE", "Opaque flash chip");
}
+void dummy_probe_sdfp_chip(void **state)
+{
+ struct io_mock_fallback_open_state dummy_fallback_open_state = {
+ .noc = 0,
+ .paths = { NULL },
+ };
+ const struct io_mock dummy_io = {
+ .fallback_open_state = &dummy_fallback_open_state,
+ };
+
+ run_probe_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=MX25L6436", NULL);
+}
+
void dummy_init_fails_unhandled_param_test_success(void **state)
{
struct io_mock_fallback_open_state dummy_fallback_open_state = {
@@ -164,6 +177,7 @@
SKIP_TEST(dummy_basic_lifecycle_test_success)
SKIP_TEST(dummy_probe_lifecycle_test_success)
SKIP_TEST(dummy_probe_variable_size_test_success)
+ SKIP_TEST(dummy_probe_sdfp_chip)
SKIP_TEST(dummy_init_fails_unhandled_param_test_success)
SKIP_TEST(dummy_init_success_invalid_param_test_success)
SKIP_TEST(dummy_init_success_unhandled_param_test_success)
diff --git a/tests/tests.c b/tests/tests.c
index 72b4868..6d863ee 100644
--- a/tests/tests.c
+++ b/tests/tests.c
@@ -457,6 +457,7 @@
cmocka_unit_test(dummy_basic_lifecycle_test_success),
cmocka_unit_test(dummy_probe_lifecycle_test_success),
cmocka_unit_test(dummy_probe_variable_size_test_success),
+ cmocka_unit_test(dummy_probe_sdfp_chip),
cmocka_unit_test(dummy_init_fails_unhandled_param_test_success),
cmocka_unit_test(dummy_init_success_invalid_param_test_success),
cmocka_unit_test(dummy_init_success_unhandled_param_test_success),
diff --git a/tests/tests.h b/tests/tests.h
index 85c4f52..ccd52ed 100644
--- a/tests/tests.h
+++ b/tests/tests.h
@@ -48,6 +48,7 @@
void dummy_basic_lifecycle_test_success(void **state);
void dummy_probe_lifecycle_test_success(void **state);
void dummy_probe_variable_size_test_success(void **state);
+void dummy_probe_sdfp_chip(void **state);
void dummy_init_fails_unhandled_param_test_success(void **state);
void dummy_init_success_invalid_param_test_success(void **state);
void dummy_init_success_unhandled_param_test_success(void **state);
To view, visit change 86432. To unsubscribe, or for help writing mail filters, visit settings.