Hi!
I've encountered some issues in my recent experience with flashrom and wanted
to drop you a line, just on the off chance that someone on your side cares. I'm
also attaching a couple of patches I've been using. I realize you have a
provenance policy in place. This is a pseudonymous account. You're welcome to
use this patches for any purpose, under any license/terms of your choosing,
including putting your own name on the commits. If that's not good enough -
hey, I tried.
1. Certain popular chips (W25Q128.V/GD25Q128, others) support a `fast_read`
(0xb command),
which simply adds a dummy cycle on the bus before data is returned. This
command must be used
for reads performed above some threshold speed (e.g. 50Mhz for W25Q128JV).
This is mentioned for some in flashchips.c. But there's no support for it
(perhaps there was once?).
Nicer programmers (like CH347) can read at over 50Mhz (through the last time I
checked, the
driver had a fixed default frequency which is 15Mhz - slower but safe). It's
possible that
some users will configure the programmer to a higher speed and violate this
constraint.
It'd be nicer to use the fast_read command whenever available. Patch attached.
2. The Winbond Chips W25Q16 (same datasheet as W25Q80, 2007), W25Q16V
(Datasheet 2009), W25Q16BV (2010),W25Q16JV (2019) all return the same flash ID
(0x1540ef), but are different
in some "advanced" ways. For one thing, the older W25Q16/W25Q16BV chips have
only 2 status registers,
while W25Q16JV has 3. The chips are all treated the same in `flashchips.c`. I
wasted a day with a salvaged
(old) chip, after I used flashrom to ID the chip and ended up looking at the
wrong datasheet.
The absence of the 3rd status register suggests a way to "sniff" for which chip
is present. When status3 is missing, the (invalid) read_SR3 command returns
0xff, which is an invalid value.
3. With W25Q128.V, If SRL=1 in STATUS2, `flashrom --wp-status` reports that the
chip is locked until a power cycle. However, the datasheet explains that one can
do an OTP write to status register 2, which permanently locks the status
register(s?).
I had to figure this out with a salvaged chip, when flashrom suggested a
power-cycle is all that is required. In fact the chip has its status registers
permantently
locked. a better message would be nice.
4. When using -c to specify a particular device, the device name param can be
onerous for
entries which span multiplechips, see for example the device name for
`MX25L12833F/etc`.
It'd be nice if specifying the name for any one chip covered by an entry would
also be
accepted. Better for self-documenting scripts too. It's also more future-proof,
if it turns
out some chips aren't fully compatible and need to be split into separate
entries. Any
script would still work, and would automatically switch to the new entry. That
could be
a good or bad thing. Just a thought.
Thanks for your work! Flashrom is incredibly useful.
Sent with [Proton Mail](https://proton.me/) secure email.
From: woot <w...@whitehouse.gov>
Subject: [PATCH 1/2] ch347_spi: add ch347_speed param
---
ch347_spi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/ch347_spi.c b/ch347_spi.c
index 570e25be..bf66b506 100644
--- a/ch347_spi.c
+++ b/ch347_spi.c
@@ -259,6 +259,60 @@ static const struct spi_master spi_master_ch347_spi = {
.shutdown = ch347_spi_shutdown,
};
+/* 0: 60MHz, 1: 30MHz, 2: 15MHz, 3: 7.5MHz, 4: 3.75MHz, 5: 1.875MHz, 6: 937.5KHzï¼7: 468.75KHz */
+enum ch347_speed_divisor {
+ CH347_SPI_SPEED_0_60MHZ = 0,
+ CH347_SPI_SPEED_1_30MHZ,
+ CH347_SPI_SPEED_2_15MHZ,
+ CH347_SPI_SPEED_3_7_5MHZ,
+ CH347_SPI_SPEED_4_3_75MHZ,
+ CH347_SPI_SPEED_5_1_875MHZ,
+ CH347_SPI_SPEED_6_937_5KHZ,
+ CH347_SPI_SPEED_7_468_75KHZ,
+};
+
+static int get_ch347_spi_speed(const struct programmer_cfg *cfg, enum ch347_speed_divisor *ch347_spi_speed)
+{
+ const char *arg = extract_programmer_param_str(cfg, "ch347_speed");
+
+ if(!arg) {
+ // default value is 15MHz
+ *ch347_spi_speed = CH347_SPI_SPEED_2_15MHZ;
+ msg_pinfo("Setting SPI speed to 15 MHz\n");
+ return 0;
+ }
+
+ if (!strcmp(arg, "0")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_0_60MHZ;
+ msg_pinfo("Setting SPI speed to 60 MHz\n");
+ } else if (!strcmp(arg, "1")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_1_30MHZ;
+ msg_pinfo("Setting SPI speed to 30 MHz\n");
+ } else if (!strcmp(arg, "2")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_2_15MHZ;
+ msg_pinfo("Setting SPI speed to 15 MHz\n");
+ } else if (!strcmp(arg, "3")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_3_7_5MHZ;
+ msg_pinfo("Setting SPI speed to 7.5 MHz\n");
+ } else if (!strcmp(arg, "4")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_4_3_75MHZ;
+ msg_pinfo("Setting SPI speed to 3.75 MHz\n");
+ } else if (!strcmp(arg, "5")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_5_1_875MHZ;
+ msg_pinfo("Setting SPI speed to 1.875 MHz\n");
+ } else if (!strcmp(arg, "6")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_6_937_5KHZ;
+ msg_pinfo("Setting SPI speed to 937.5 kHz\n");
+ } else if (!strcmp(arg, "7")) {
+ *ch347_spi_speed = CH347_SPI_SPEED_7_468_75KHZ;
+ msg_pinfo("Setting SPI speed to 468.75 kHz\n");
+ } else {
+ msg_perr("Illegal value for ch347_speed: %s (Valid values are 0-7)\n", arg);
+ return ERROR_FLASHROM_FATAL;
+ }
+ return 0;
+}
+
/* Largely copied from ch341a_spi.c */
static int ch347_spi_init(const struct programmer_cfg *cfg)
{
@@ -325,7 +379,12 @@ static int ch347_spi_init(const struct programmer_cfg *cfg)
(desc.bcdDevice >> 0) & 0x000F);
/* TODO: add programmer cfg for things like CS pin and divisor */
- if (ch347_spi_config(ch347_data, 2) < 0)
+ enum ch347_speed_divisor ch347_speed_divisor = CH347_SPI_SPEED_2_15MHZ;
+ ret = get_ch347_spi_speed(cfg, &ch347_speed_divisor);
+ if (ret < 0) {
+ goto error_exit;
+ }
+ if (ch347_spi_config(ch347_data, ch347_speed_divisor) < 0)
goto error_exit;
return register_spi_master(&spi_master_ch347_spi, ch347_data);
--
2.40.1
From: woot <w...@whitehouse.gov>
Subject: [PATCH 2/2] Add FEATURE_0B_FAST_READ, and use it in spi_nbyte_read
---
flashchips.c | 4 ++--
include/flash.h | 3 +++
spi25.c | 16 ++++++++++++++--
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/flashchips.c b/flashchips.c
index 6a5cf495..b0688f04 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -17690,7 +17690,7 @@ const struct flashchip flashchips[] = {
.page_size = 256,
/* supports SFDP */
/* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44, read ID 0x4B */
- .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP |
+ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_0B_FAST_READ |
FEATURE_WRSR_EXT2 | FEATURE_WRSR2 | FEATURE_WRSR3,
.tested = TEST_OK_PREWB,
.probe = PROBE_SPI_RDID,
@@ -17888,7 +17888,7 @@ const struct flashchip flashchips[] = {
.page_size = 256,
/* supports SFDP */
/* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44, read ID 0x4B */
- .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP,
+ .feature_bits = FEATURE_WRSR_WREN | FEATURE_0B_FAST_READ | FEATURE_OTP,
.tested = TEST_OK_PREW,
.probe = PROBE_SPI_RDID,
.probe_timing = TIMING_ZERO,
diff --git a/include/flash.h b/include/flash.h
index 0eace15d..46775745 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -164,6 +164,9 @@ enum write_granularity {
/* Whether chip has configuration register (RDCR/WRSR_EXT2 commands) */
#define FEATURE_CFGR (1 << 25)
+/* Non-4BA Fast Read (0x0b) Support, with 3 byte address followed by dummy byte */
+#define FEATURE_0B_FAST_READ (1 << 26)
+
#define ERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff)
#define UNERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0xff : 0x00)
diff --git a/spi25.c b/spi25.c
index 6a6ee75d..6a358ad0 100644
--- a/spi25.c
+++ b/spi25.c
@@ -661,15 +661,27 @@ static int spi_nbyte_program(struct flashctx *flash, unsigned int addr, const ui
int spi_nbyte_read(struct flashctx *flash, unsigned int address, uint8_t *bytes,
unsigned int len)
{
+ uint8_t cmd[1 + JEDEC_MAX_ADDR_LEN] = { JEDEC_READ, };
+ int dummy_len = 0;
const bool native_4ba = flash->chip->feature_bits & FEATURE_4BA_READ && spi_master_4ba(flash);
- uint8_t cmd[1 + JEDEC_MAX_ADDR_LEN] = { native_4ba ? JEDEC_READ_4BA : JEDEC_READ, };
+ const bool fast_read_0b = flash->chip->feature_bits & FEATURE_0B_FAST_READ;
+
+ if (native_4ba) { // prefer 4ba if available, so we always support max flash size
+ cmd[0] = JEDEC_READ_4BA;
+ } else if (fast_read_0b) {
+ cmd[0] = JEDEC_READ_FAST;
+ dummy_len = 1;
+ } else {
+ cmd[0] = JEDEC_READ;
+ }
const int addr_len = spi_prepare_address(flash, cmd, native_4ba, address);
if (addr_len < 0)
return 1;
/* Send Read */
- return spi_send_command(flash, 1 + addr_len, len, cmd, bytes);
+ int cmd_len = 1 + addr_len + dummy_len;
+ return spi_send_command(flash, cmd_len, len, cmd, bytes);
}
/*
--
2.40.1
_______________________________________________
flashrom mailing list -- flashrom@flashrom.org
To unsubscribe send an email to flashrom-le...@flashrom.org
