| Message ID | pmWqI0TGUDRmqMI5DLqXqdO5cOjz01CFAXQG_Dj8z7dw4RCzNgL0Cat-qdakb4vtI9iGbOAJ_YCAKYdjTyhLK0zyLs5sgILc20CJT9s8zNo=@proton.me |
|---|---|
| State | New |
| Headers | show |
| Series | Some flashrom issues reported and a couple of patches | expand |
Leonard, Thank you so much for your ideas and patches! Will have a look at these. As for signing off the patches, we recently updated the policy (in the same way as Linux did) so now it doesn't have to be a real real name. However it still needs to be a known identity, for example if everyone knows you by the nickname that's fine. If at any point in future you decide to send a patch for review from yourself, you are very welcome.
------- Original Message ------- On Friday, June 16th, 2023 at 2:06 PM, Anastasia Klimchuk <aklm@chromium.org> wrote: > Leonard, > Thank you so much for your ideas and patches! Will have a look at these. > As for signing off the patches, we recently updated the policy (in the > same way as Linux did) so now it doesn't have to be a real real name. > However it still needs to be a known identity, for example if everyone > knows you by the nickname that's fine. > If at any point in future you decide to send a patch for review from > yourself, you are very welcome. > > -- > Anastasia. Hi Anastasia, Thank you for responding. I hope my notes will end up being of some use. Thank you also for clarifying your contribution policy, though I'm afraid it still doesn't apply in my case. There's another issue I've encountered since my e-mail. Since you indicates it's not a total waste of time to report such issues, I will. Currently, if any write-protection mechanism is active on a chip, flashrom will try to read the WP state, unlock it, and then restore the WP protection state. There's a corner case which the logic doesn't handle well. It's possible for WP ranges to be *locked* via OTP, but that actually the "protection" allows access everywhere (or anywhere needed, anyway). Currently, flashrom produces a warning message in `prepare_flash_access` with the notice "Failed to unlock flash status reg with wp support". However, there's no actual reason to try to unlock the register, since the "protection", while locked, doesn't hinder any access. In other words, flashrom tries to unlock the register seemingly only because it's locked, and not because a region targeted for write is protected. Even stranger is the fact that `prepare_flash_access` is called from `flashrom_image_read`. That mean trying to *read* such a chip produces a warning about failing to disable write-protection. The read does succeed though, so this is would be a low-priority quirk. Kind regards,
From: woot <woot@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